Fixes for updates of KADM5_KVNO but not KEY_DATA and vice-versa.

It turns out that updates of kvno but not key data and vice-versa are
both, allowed and actually done (e.g, in kadmin's ank).  Doing the right
thing in these cases turns out to be a bit tricky, but this commit ought
to do it.

kadmin/ank.c

@@ -160,6 +160,12 @@ add_one_principal (const char *name,
 	kadm5_get_principal(kadm_handle, princ_ent, &princ,
 			    KADM5_PRINCIPAL | KADM5_KVNO | KADM5_ATTRIBUTES);
 	princ.attributes &= (~KRB5_KDB_DISALLOW_ALL_TIX);
+	/*
+	 * Updating kvno w/o key data and vice-versa gives _kadm5_setup_entry()
+	 * and _kadm5_set_keys2() headaches.  But we used to, so we handle
+	 * this in in those two functions.  Might as well leave this code as
+	 * it was then.
+	 */
 	princ.kvno = 1;
 	kadm5_modify_principal(kadm_handle, &princ,
 			       KADM5_ATTRIBUTES | KADM5_KVNO);

kadmin/del_enctype.c

@@ -106,6 +106,10 @@ del_enctype(void *opt, int argc, char **argv)
     }
 
     free (princ.key_data);
+    if (j == 0) {
+	free(new_key_data);
+	new_key_data = NULL;
+    }
     princ.n_key_data = j;
     princ.key_data   = new_key_data;
 

kadmin/get.c

@@ -60,7 +60,7 @@ static struct field_name {
     { "last_failed", KADM5_LAST_FAILED, 0, 0, "Last fail", "Last failed login", 0 },
     { "fail_auth_count", KADM5_FAIL_AUTH_COUNT, 0, 0, "Fail count", "Failed login count", RTBL_ALIGN_RIGHT },
     { "policy", KADM5_POLICY, 0, 0, "Policy", "Policy", 0 },
-    { "keytypes", KADM5_KEY_DATA, 0, KADM5_PRINCIPAL, "Keytypes", "Keytypes", 0 },
+    { "keytypes", KADM5_KEY_DATA, 0, KADM5_PRINCIPAL | KADM5_KVNO, "Keytypes", "Keytypes", 0 },
     { "password", KADM5_TL_DATA, KRB5_TL_PASSWORD, KADM5_KEY_DATA, "Password", "Password", 0 },
     { "pkinit-acl", KADM5_TL_DATA, KRB5_TL_PKINIT_ACL, 0, "PK-INIT ACL", "PK-INIT ACL", 0 },
     { "aliases", KADM5_TL_DATA, KRB5_TL_ALIASES, 0, "Aliases", "Aliases", 0 },

lib/kadm5/common_glue.c

@@ -258,7 +258,7 @@ kadm5_setkey_principal_3(void *server_handle,
 	return KADM5_SETKEY3_ETYPE_MISMATCH;
 
     ret = kadm5_get_principal(server_handle, princ, &princ_ent,
-                              KADM5_PRINCIPAL | KADM5_KEY_DATA);
+                              KADM5_KVNO | KADM5_PRINCIPAL | KADM5_KEY_DATA);
     if (ret)
 	return ret;
 

lib/kadm5/create_s.c

@@ -111,6 +111,12 @@ kadm5_s_create_principal_with_key(void *server_handle,
     hdb_entry_ex ent;
     kadm5_server_context *context = server_handle;
 
+    if ((mask & KADM5_KVNO) == 0) {
+	/* create_principal() through _kadm5_setup_entry(), will need this */
+	princ->kvno = 1;
+	mask |= KADM5_KVNO;
+    }
+
     ret = create_principal(context, princ, mask, &ent,
 			   KADM5_PRINCIPAL | KADM5_KEY_DATA,
 			   KADM5_LAST_PWD_CHANGE | KADM5_MOD_TIME
@@ -121,9 +127,6 @@ kadm5_s_create_principal_with_key(void *server_handle,
     if(ret)
 	goto out;
 
-    if ((mask & KADM5_KVNO) == 0)
-	ent.entry.kvno = 1;
-
     ret = hdb_seal_keys(context->context, context->db, &ent.entry);
     if (ret)
 	goto out;
@@ -153,6 +156,12 @@ kadm5_s_create_principal(void *server_handle,
     hdb_entry_ex ent;
     kadm5_server_context *context = server_handle;
 
+    if ((mask & KADM5_KVNO) == 0) {
+	/* create_principal() through _kadm5_setup_entry(), will need this */
+	princ->kvno = 1;
+	mask |= KADM5_KVNO;
+    }
+
     ret = create_principal(context, princ, mask, &ent,
 			   KADM5_PRINCIPAL,
 			   KADM5_LAST_PWD_CHANGE | KADM5_MOD_TIME
@@ -163,9 +172,6 @@ kadm5_s_create_principal(void *server_handle,
     if(ret)
 	goto out;
 
-    if ((mask & KADM5_KVNO) == 0)
-	ent.entry.kvno = 1;
-
     ent.entry.keys.len = 0;
     ent.entry.keys.val = NULL;
 

lib/kadm5/ent_setup.c

@@ -178,8 +178,27 @@ _kadm5_setup_entry(kadm5_server_context *context,
 	}
     }
     if(mask & KADM5_KVNO
-       && princ_mask & KADM5_KVNO)
-	ent->entry.kvno = princ->kvno;
+       && princ_mask & KADM5_KVNO) {
+	/*
+	 * For some reason kadmin's ank changes the kvno after calling
+	 * randkey.  Now that we have key history, what are we to do
+	 * when we update kvno but not keys?!
+	 *
+	 * For now just clear the key history if the kvno changes.
+	 * 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) {
 	  if(princ->max_renewable_life)

lib/kadm5/set_keys.c

@@ -102,19 +102,71 @@ _kadm5_set_keys2(kadm5_server_context *context,
     krb5_error_code ret;
     size_t i, k;
     HDB_extension ext;
-    HDB_extension *extp;
+    HDB_extension *extp = NULL;
     HDB_Ext_KeySet *hist_keys = &ext.data.u.hist_keys;
     Key key;
     Salt salt;
     Keys keys;
     hdb_keyset hkset;
+    krb5_kvno kvno = -1;
+    int one_key_set = 1;
+    int replace_hist_keys = 0;
+
+    if (n_key_data == 0) {
+	/* Clear all keys! */
+	ret = hdb_clear_extension(context->context, ent,
+				  choice_HDB_extension_data_hist_keys);
+	if (ret)
+	    return ret;
+	free_Keys(&ent->keys);
+	return 0;
+    }
 
     memset(&keys, 0, sizeof (keys));
     memset(&hkset, 0, sizeof (hkset)); /* set set_time */
     ext.data.element = choice_HDB_extension_data_hist_keys;
     memset(hist_keys, 0, sizeof (*hist_keys));
 
-    for(i = 0; i < n_key_data; i++) {
+    for (i = 0; i < n_key_data; i++) {
+	if (kvno != -1 && kvno != key_data[i].key_data_kvno) {
+	    one_key_set = 0;
+	    break;
+	}
+	kvno = key_data[i].key_data_kvno;
+    }
+    if (one_key_set) {
+	/*
+	 * If we're updating KADM5_KEY_DATA with a single keyset then we
+	 * assume we must be setting the principal's kvno as well!
+	 *
+	 * Just have to be careful about old clients that might have
+	 * sent 0 as the kvno...  This may seem ugly, but it's the price
+	 * of backwards compatibility with pre-multi-kvno kadmin clients
+	 * (besides, who's to say that updating KADM5_KEY_DATA requires
+	 * updating the entry's kvno?)
+	 *
+	 * Note that we do nothing special for the case where multiple
+	 * keysets are given but the entry's kvno is not set and not in
+	 * the given set of keysets.  If this happens we'll just update
+	 * the key history only and leave the current keyset alone.
+	 */
+	if (kvno == 0) {
+	    /* Force kvno to 1 if it was 0; (ank would do this anyways) */
+	    if (ent->kvno == 0)
+		ent->kvno = 1;
+	    /* Below we need key_data[*].kvno to be reasonable */
+	    for (i = 0; i < n_key_data; i++)
+		key_data[i].key_data_kvno = ent->kvno;
+	} else {
+	    /*
+	     * Or force the entry's kvno to match the one from the new,
+	     * singular keyset
+	     */
+	    ent->kvno = kvno;
+	}
+    }
+
+    for (i = 0; i < n_key_data; i++) {
 	if (key_data[i].key_data_kvno == ent->kvno) {
 	    /* A current key; add to current key set */
 	    setup_Key(&key, &salt, key_data, i);
@@ -150,22 +202,20 @@ _kadm5_set_keys2(kadm5_server_context *context,
 	ret = add_HDB_Ext_KeySet(hist_keys, &hkset);
 	if (ret)
 	    goto out;
+	replace_hist_keys = 1;
     }
-    
-    /*
-     * A structure copy is more efficient here than this would be:
-     *
-     * copy_Keys(&keys, &ent->keys);
-     * free_Keys(&keys);
-     */
-    free_Keys(&ent->keys);
-    ent->keys = keys;
 
-    /* Try to keep the set_time values from the old hist keys */
-    extp = hdb_find_extension(ent, choice_HDB_extension_data_hist_keys);
+    if (replace_hist_keys)
+	/* No key history given -> leave it alone */
+	extp = hdb_find_extension(ent, choice_HDB_extension_data_hist_keys);
     if (extp != NULL) {
 	HDB_Ext_KeySet *old_hist_keys;
 
+	/*
+	 * Try to keep the very useful set_time values from the old hist
+	 * keys.  kadm5 loses this info, so this heuristic is the best we
+	 * can do.
+	 */
 	old_hist_keys = &extp->data.u.hist_keys;
 	for (i = 0; i < old_hist_keys->len; i++) {
 	    if (old_hist_keys->val[i].set_time == NULL)
@@ -179,7 +229,23 @@ _kadm5_set_keys2(kadm5_server_context *context,
 	}
     }
 
-    hdb_replace_extension(context->context, ent, &ext);
+    if (replace_hist_keys) {
+	/* If hist keys not given in key_data then don't blow away hist_keys */
+	ret = hdb_replace_extension(context->context, ent, &ext);
+	if (ret)
+	    goto out;
+    }
+    
+    /*
+     * A structure copy is more efficient here than this would be:
+     *
+     * copy_Keys(&keys, &ent->keys);
+     * free_Keys(&keys);
+     *
+     * Of course, the above hdb_replace_extension() is not at all efficient...
+     */
+    free_Keys(&ent->keys);
+    ent->keys = keys;
     hdb_entry_set_pw_change_time(context->context, ent, 0);
     hdb_entry_clear_password(context->context, ent);