Fixed booboos from kadm5 key history patch set

Also: add support for ignoring null enctype / zero-length keys,
    which *can* be found in MIT DB entries created in pre-historic
    times.

    Also: make the mitdb HDB backend more elegant (e.g., use the ASN.1
    compiler's generated sequence/array utility functions.

    Also: add a utility function needed for kadm5 kvno change
    improvements and make kadmin's mod --kvno work correctly and
    naturally.

Signed-off-by: Love Hornquist Astrand <lha@h5l.org>

lib/hdb/hdb-mitdb.c

@@ -196,133 +196,6 @@ fix_salt(krb5_context context, hdb_entry *ent, int key_num)
     return 0;
 }
 
-/**
- * This function outputs a pointer to a Key or array of @key_count Keys
- * where the caller may place Keys.
- *
- * @param context   Context
- * @param entry	    HDB entry
- * @param kvno	    kvno of the keys to be added
- * @param is_hist   Whether the keys will be historical keys or current keys
- * @param key_count Size of array of keys to set.  MUST be zero if !is_hist.
- * @param out	    Pointer to Key * variable where to put the resulting Key *
- *
- * See three call sites below for more information.
- */
-static krb5_error_code
-get_entry_key_location(krb5_context context, hdb_entry *entry, krb5_kvno kvno,
-		       krb5_boolean is_hist, size_t key_count, Key **out)
-{
-    HDB_extension ext;
-    HDB_Ext_KeySet *hist_keys;
-    hdb_keyset *keyset = NULL;
-    size_t keyset_count = 0;
-    Key *k = NULL;
-    size_t i;
-    krb5_error_code ret;
-
-    *out = NULL;
-
-    if (!is_hist) {
-	Key *tmp;
-
-	/* Extend current keyset */
-	tmp = realloc(entry->keys.val, sizeof(entry->keys.val[0]) * (entry->keys.len + 1));
-	if (tmp == NULL) {
-	    ret = ENOMEM;
-	    goto out;
-	}
-	entry->keys.val = tmp;
-
-	/* k points to current Key */
-	k = &entry->keys.val[entry->keys.len];
-
-	memset(k, 0, sizeof(*k));
-	entry->keys.len += 1;
-
-	goto done;
-    }
-
-    /* Find a history keyset and extend it or extend the history keyset */
-    memset(&ext, 0, sizeof (ext));
-    ext.data.element = choice_HDB_extension_data_hist_keys;
-    hist_keys = &ext.data.u.hist_keys;
-
-    /* hdb_replace_extension() makes a copy of ext */
-    ret = hdb_replace_extension(context, entry, &ext);
-    if (ret)
-	return ret;
-
-    for (i = 0; i < hist_keys->len; i++) {
-	if (hist_keys->val[i].kvno == kvno) {
-	    /* We're adding a key to an existing history keyset */
-	    keyset = &hist_keys->val[i];
-	    if ((keyset->keys.len % 8) == 0) {
-		Key *tmp;
-
-		/* We're adding the 9th, 17th, ... key to the set */
-		tmp = realloc(keyset->keys.val,
-			      (keyset->keys.len + 8) * sizeof (*tmp));
-		if (tmp == NULL) {
-		    ret = ENOMEM;
-		    goto out;
-		}
-	    }
-	    break;
-	}
-    }
-
-    if (keyset == NULL) {
-	/* We're adding the first key of a new history keyset */
-	if (hist_keys->val == NULL) {
-	    if (key_count == 0)
-		keyset_count = 8; /* There's not that many enctypes */
-	    else
-		keyset_count = key_count;
-	    hist_keys->val = calloc(keyset_count,
-				    sizeof (*hist_keys->val));
-	    if (hist_keys->val == NULL) {
-		ret = ENOMEM;
-		goto out;
-	    }
-	    keyset = &hist_keys->val[0];
-	} else if (hist_keys->len == keyset_count) {
-	    hdb_keyset *tmp;
-
-	    keyset_count *= 2;
-	    tmp = realloc(hist_keys->val,
-			  keyset_count * sizeof (*hist_keys->val));
-	    if (tmp == NULL) {
-		ret = ENOMEM;
-		goto out;
-	    }
-	    hist_keys->val = tmp;
-	}
-    }
-
-    k = &keyset->keys.val[keyset->keys.len];
-
-    if (key_count != 0)
-	keyset->keys.len += key_count;
-    
-done:
-    memset(k, 0, sizeof (*k));
-    k->mkvno = malloc(sizeof(*k->mkvno));
-    if (k->mkvno == NULL) {
-	ret = ENOMEM;
-	goto out;
-    }
-    *k->mkvno = 1;
-    *out = k;
-
-out:
-    if (ret && !is_hist)
-	entry->keys.len--;
-    if (is_hist)
-	free_HDB_extension(&ext);
-    return ret;
-}
-
 
 /**
  * This function takes a key from a krb5_storage from an MIT KDB encoded
@@ -342,6 +215,13 @@ mdb_keyvalue2key(krb5_context context, hdb_entry *entry, krb5_storage *sp, uint1
     uint16_t u16, type;
     krb5_error_code ret;
 
+    k->mkvno = malloc(sizeof(*k->mkvno));
+    if (k->mkvno == NULL) {
+	ret = ENOMEM;
+	goto out;
+    }
+    *k->mkvno = 1;
+
     for (i = 0; i < version; i++) {
 	CHECK(ret = krb5_ret_uint16(sp, &type));
 	CHECK(ret = krb5_ret_uint16(sp, &u16));
@@ -400,7 +280,6 @@ mdb_keyvalue2key(krb5_context context, hdb_entry *entry, krb5_storage *sp, uint1
 
 out:
     free_Key(k);
-    memset(k, 0, sizeof (*k));
     return ret;
 }
 
@@ -408,19 +287,28 @@ out:
 /**
  * This function parses an MIT krb5 encoded KDB entry and fills in the
  * given HDB entry with it.
+ *
+ * @param context	krb5_context
+ * @param data		Encoded MIT KDB entry
+ * @param target_kvno	Desired kvno, or 0 for the entry's current kvno
+ * @param entry		Desired kvno, or 0 for the entry's current kvno
  */
 static krb5_error_code
-mdb_value2entry(krb5_context context, krb5_data *data, krb5_kvno kvno, hdb_entry *entry)
+mdb_value2entry(krb5_context context, krb5_data *data, krb5_kvno target_kvno,
+		hdb_entry *entry)
 {
     krb5_error_code ret;
     krb5_storage *sp;
-    Key *k;
+    Key k;
     krb5_kvno key_kvno;
     uint32_t u32;
     uint16_t u16, num_keys, num_tl;
-    size_t i, j;
+    size_t i;
     char *p;
 
+    memset(&k, 0, sizeof (k));
+    memset(entry, 0, sizeof(*entry));
+
     sp = krb5_storage_from_data(data);
     if (sp == NULL) {
 	krb5_set_error_message(context, ENOMEM, "out of memory");
@@ -442,7 +330,8 @@ mdb_value2entry(krb5_context context, krb5_data *data, krb5_kvno kvno, hdb_entry
      * XXX But... surely we ought to log about this extra data, or skip
      * it, or something, in case anyone has MIT KDBs with ancient
      * entries in them...  Logging would allow the admin to know which
-     * entries to dump with MIT krb5's kdb5_util.
+     * entries to dump with MIT krb5's kdb5_util.  But logging would be
+     * noisy.  For now we do nothing.
      */
     CHECK(ret = krb5_ret_uint16(sp, &u16));
     if (u16 != KDB_V1_BASE_LENGTH) { ret = EINVAL; goto out; }
@@ -540,7 +429,6 @@ mdb_value2entry(krb5_context context, krb5_data *data, krb5_kvno kvno, hdb_entry
      * That's right... hold that gag reflex, you can do it.
      */
     for (i = 0; i < num_keys; i++) {
-	int keep = 0;
 	uint16_t version;
 
 	CHECK(ret = krb5_ret_uint16(sp, &u16));
@@ -548,70 +436,81 @@ mdb_value2entry(krb5_context context, krb5_data *data, krb5_kvno kvno, hdb_entry
 	CHECK(ret = krb5_ret_uint16(sp, &u16));
 	key_kvno = u16;
 
+	ret = mdb_keyvalue2key(context, entry, sp, version, &k);
+	if (ret)
+	    goto out;
+	if (k.key.keytype == 0 || k.key.keyvalue.length == 0) {
 	    /*
-	 * First time through, and until we find one matching key,
+	     * Older MIT KDBs may have enctype 0 / length 0 keys.  We
-	 * entry->kvno == 0.
+	     * ignore these.
 	     */
-	if ((entry->kvno < key_kvno) && (kvno == 0 || kvno == key_kvno)) {
+	    free_Key(&k);
+	    continue;
+	}
+
+	if ((target_kvno == 0 && entry->kvno < key_kvno) ||
+	    (target_kvno == key_kvno && entry->kvno != target_kvno)) {
 	    /*
-	     * Found a higher kvno than earlier, we aren't looking for
+	     * MIT's KDB doesn't keep track of kvno.  The highest kvno
-	     * any particular kvno, so save the previously saved keys as
+	     * is the current kvno, and we just found a new highest
-	     * historical keys.
+	     * kvno or the desired kvno.
+	     *
+	     * Note that there's no guarantee of any key ordering, but
+	     * generally MIT KDB entries have keys in strictly
+	     * descending kvno order.
+	     *
+	     * XXX We do assume that keys are clustered by kvno.  If
+	     * not, then bad.  It might be possible to construct
+	     * non-clustered keys via the kadm5 API.  It wouldn't be
+	     * hard to cope with this, since if it happens the worst
+	     * that will happen is that some of the current keys can be
+	     * found in the history extension, and we could just pull
+	     * them back out in that case.
 	     */
-	    keep = 1;
+	    ret = hdb_add_current_keys_to_history(context, entry);
-
+	    if (ret)
-	    /* Get an array of Keys to save the current keyset into */
+		goto out;
-	    ret = get_entry_key_location(context, entry, entry->kvno, TRUE,
+	    free_Keys(&entry->keys);
-					 entry->keys.len, &k);
+	    ret = add_Keys(&entry->keys, &k);
-
+	    free_Key(&k);
-	    for (j = 0; j < entry->keys.len; j++)
+	    if (ret)
-		copy_Key(&entry->keys.val[j], &k[j]);
+		goto out;
-
-	    /* Change the entry's current kvno */
 	    entry->kvno = key_kvno;
+	    continue;
+	}
 
-	    for (j = 0; j < entry->keys.len; j++)
+	if (entry->kvno == key_kvno) {
-		free_Key(&entry->keys.val[j]);
-	    free(entry->keys.val);
-	    entry->keys.len = 0;
-	    entry->keys.val = NULL;
-	} else if (entry->kvno == key_kvno)
-	    /* Accumulate keys */
-	    keep = 1;
-
-	if (keep) {
-	    ret = get_entry_key_location(context, entry, key_kvno,
-					 FALSE, 0, &k);
-	    if (ret)
-		goto out;
-
-	    ret = mdb_keyvalue2key(context, entry, sp, version, k);
-	    if (ret)
-		goto out;
-	} else {
 	    /*
-	     * XXX For now we skip older kvnos, but we should extract
+	     * Note that if key_kvno == 0 and target_kvno == 0 then we
-	     * them... XXX Finish.
+	     * end up adding those keys here.  Yeah, kvno 0 is very
+	     * special for us, but just in case, we keep such keys.
 	     */
-	    ret = get_entry_key_location(context, entry, key_kvno, TRUE, 0, &k);
+	    ret = add_Keys(&entry->keys, &k);
+	    free_Key(&k);
 	    if (ret)
 		goto out;
-	    ret = mdb_keyvalue2key(context, entry, sp, version, k);
+	    entry->kvno = key_kvno;
+	} else  {
+	    ret = hdb_add_history_key(context, entry, key_kvno, &k);
 	    if (ret)
 		goto out;
+	    free_Key(&k);
 	}
     }
 
-    if (entry->kvno == 0 && kvno != 0) {
+    if (target_kvno != 0 && entry->kvno != target_kvno) {
-	ret = HDB_ERR_NOT_FOUND_HERE;
+	ret = HDB_ERR_KVNO_NOT_FOUND;
 	goto out;
     }
 
     return 0;
- out:
+
+out:
     if (ret == HEIM_ERR_EOF)
 	/* Better error code than "end of file" */
 	ret = HEIM_ERR_BAD_HDBENT_ENCODING;
+    free_hdb_entry(entry);
+    free_Key(&k);
     return ret;
 }
 

lib/hdb/hdb_err.et

@@ -27,5 +27,6 @@ error_code MANDATORY_OPTION,	"Entry contains unknown mandatory extension"
 error_code NO_WRITE_SUPPORT,	"HDB backend doesn't contain write support"
 error_code NOT_FOUND_HERE,	"The secret for this entry is not replicated to this database"
 error_code MISUSE,		"Incorrect use of the API"
+error_code KVNO_NOT_FOUND,	"Entry key version number not found"
 
 end

lib/hdb/keys.c

@@ -211,9 +211,11 @@ hdb_add_current_keys_to_history(krb5_context context, hdb_entry *entry)
     krb5_boolean replace = FALSE;
     krb5_error_code ret;
     HDB_extension *ext;
-    hdb_keyset newkey;
+    hdb_keyset newkeyset;
     time_t newtime;
 
+    if (entry->keys.len == 0)
+	return 0; /* nothing to do */
 
     ext = hdb_find_extension(entry, choice_HDB_extension_data_hist_keys);
     if (ext == NULL) {
@@ -228,17 +230,16 @@ hdb_add_current_keys_to_history(krb5_context context, hdb_entry *entry)
     /*
      * Copy in newest old keyset
      */
-
     ret = hdb_entry_get_pw_change_time(entry, &newtime);
     if (ret)
 	goto out;
 
-    memset(&newkey, 0, sizeof(newkey));
+    memset(&newkeyset, 0, sizeof(newkeyset));
-    newkey.keys = entry->keys;
+    newkeyset.keys = entry->keys;
-    newkey.kvno = entry->kvno;
+    newkeyset.kvno = entry->kvno;
-    newkey.set_time = &newtime;
+    newkeyset.set_time = &newtime;
 
-    ret = add_HDB_Ext_KeySet(&ext->data.u.hist_keys, &newkey);
+    ret = add_HDB_Ext_KeySet(&ext->data.u.hist_keys, &newkeyset);
     if (ret)
 	goto out;
 
@@ -257,6 +258,124 @@ hdb_add_current_keys_to_history(krb5_context context, hdb_entry *entry)
     return ret;
 }
 
+/**
+ * This function adds a key to an HDB entry's key history.
+ *
+ * @param context   Context
+ * @param entry	    HDB entry
+ * @param kvno	    Key version number of the key to add to the history
+ * @param key	    The Key to add
+ */
+krb5_error_code
+hdb_add_history_key(krb5_context context, hdb_entry *entry, krb5_kvno kvno, Key *key)
+{
+    size_t i;
+    hdb_keyset keyset;
+    HDB_Ext_KeySet *hist_keys;
+    HDB_extension ext;
+    HDB_extension *extp;
+    krb5_error_code ret;
+
+    memset(&keyset, 0, sizeof (keyset));
+    memset(&ext, 0, sizeof (ext));
+
+    extp = hdb_find_extension(entry, choice_HDB_extension_data_hist_keys);
+    if (extp == NULL) {
+	ext.data.element = choice_HDB_extension_data_hist_keys;
+	extp = &ext;
+    }
+
+    hist_keys = &extp->data.u.hist_keys;
+
+    for (i = 0; i < hist_keys->len; i++) {
+	if (hist_keys->val[i].kvno == kvno) {
+	    ret = add_Keys(&hist_keys->val[i].keys, key);
+	    goto out;
+	}
+    }
+
+    keyset.kvno = kvno;
+    ret = add_Keys(&keyset.keys, key);
+    if (ret)
+	goto out;
+    ret = add_HDB_Ext_KeySet(hist_keys, &keyset);
+    if (ret)
+	goto out;
+    if (extp == &ext) {
+	ret = hdb_replace_extension(context, entry, &ext);
+	if (ret)
+	    goto out;
+    }
+
+out:
+    free_hdb_keyset(&keyset);
+    free_HDB_extension(&ext);
+    return ret;
+}
+
+
+/**
+ * This function changes an hdb_entry's kvno, swapping the current key
+ * set with a historical keyset.  If no historical keys are found then
+ * an error is returned (the caller can still set entry->kvno directly).
+ *
+ * @param context	krb5_context
+ * @param new_kvno	New kvno for the entry
+ * @param entry		hdb_entry to modify
+ */
+krb5_error_code
+hdb_change_kvno(krb5_context context, krb5_kvno new_kvno, hdb_entry *entry)
+{
+    HDB_extension ext;
+    HDB_extension *extp;
+    hdb_keyset keyset;
+    HDB_Ext_KeySet *hist_keys;
+    size_t i;
+    int found = 0;
+    krb5_error_code ret;
+
+    if (entry->kvno == new_kvno)
+	return 0;
+
+    extp = hdb_find_extension(entry, choice_HDB_extension_data_hist_keys);
+    if (extp == NULL) {
+	memset(&ext, 0, sizeof (ext));
+	ext.data.element = choice_HDB_extension_data_hist_keys;
+	extp = &ext;
+    }
+
+    memset(&keyset, 0, sizeof (keyset));
+    hist_keys = &extp->data.u.hist_keys;
+    for (i = 0; i < hist_keys->len; i++) {
+	if (hist_keys->val[i].kvno == new_kvno) {
+	    found = 1;
+	    ret = copy_hdb_keyset(&hist_keys->val[i], &keyset);
+	    if (ret)
+		goto out;
+	    ret = remove_HDB_Ext_KeySet(hist_keys, i);
+	    if (ret)
+		goto out;
+	    break;
+	}
+    }
+
+    if (!found)
+	return HDB_ERR_KVNO_NOT_FOUND;
+
+    ret = hdb_add_current_keys_to_history(context, entry);
+    if (ret)
+	goto out;
+
+    /* Note: we do nothing with keyset.set_time */
+    entry->kvno = new_kvno;
+    entry->keys = keyset.keys; /* shortcut */
+    memset(&keyset.keys, 0, sizeof (keyset.keys));
+
+out:
+    free_hdb_keyset(&keyset);
+    return ret;
+}
+
 
 static krb5_error_code
 add_enctype_to_key_set(Key **key_set, size_t *nkeyset,

lib/hdb/version-script.map

@@ -5,6 +5,7 @@ HEIMDAL_HDB_1.0 {
 		encode_hdb_keyset;
 		hdb_add_master_key;
 		hdb_add_current_keys_to_history;
+		hdb_change_kvno;
 		hdb_check_db_format;
 		hdb_clear_extension;
 		hdb_clear_master_key;

lib/kadm5/ent_setup.c

@@ -178,26 +178,13 @@ _kadm5_setup_entry(kadm5_server_context *context,
 	}
     }
     if(mask & KADM5_KVNO
-       && princ_mask & KADM5_KVNO) {
+       && (princ_mask & KADM5_KVNO)) {
-	/*
+	krb5_error_code ret;
-	 * For some reason kadmin's ank changes the kvno after calling
+
-	 * randkey.  Now that we have key history, what are we to do
+	ret = hdb_change_kvno(context->context, princ->kvno, &ent->entry);
-	 * when we update kvno but not keys?!
+	if (ret && ret != HDB_ERR_KVNO_NOT_FOUND)
-	 *
+	    return ret;
-	 * For now just clear the key history if the kvno changes.
+	ent->entry.kvno = princ->kvno; /* force it */
-	 * Eventually we may want to search the key history for matching
-	 * keys and use those to replace the current key set (putting
-	 * the old current keyset in the history keysets list?!).
-	 */
-	if (ent->entry.kvno != princ->kvno &&
-	    (mask & princ_mask & KADM5_KEY_DATA)) {
-	    hdb_clear_extension(context->context, &ent->entry,
-				choice_HDB_extension_data_hist_keys);
-	    princ->kvno = ent->entry.kvno;
-	} else {
-	    /* _kadm5_set_keys2() expects this to have been done here */
-	    ent->entry.kvno = princ->kvno;
-	}
     }
     if(mask & KADM5_MAX_RLIFE) {
 	if(princ_mask & KADM5_MAX_RLIFE) {