Fix wallet-rekey on keytabs containing multiple principals

Fix wallet-rekey on keytabs containing multiple principals.  Previous
versions assumed one could concatenate keytab files together to make a
valid keytab file, which doesn't work with some Kerberos libraries.
This caused new keys downloaded for principals after the first to be
discarded.  As a side effect of this fix, wallet-rekey always appends
new keys directly to the existing keytab file, and never creates a
backup copy of that file.

Change-Id: I5f863239ce4ebba66b35ff09454f2897367bd359
Reviewed-on: https://gerrit.stanford.edu/1369
Reviewed-by: Russ Allbery <rra@stanford.edu>
Tested-by: Russ Allbery <rra@stanford.edu>

4 files changed, 42 insertions, 51 deletions

diff --git a/NEWS b/NEWS
index 5ff85d0..165622a 100644
--- a/NEWS
+++ b/NEWS
@@ -2,6 +2,14 @@
 
 wallet 1.1 (unreleased)
 
+    Fix wallet-rekey on keytabs containing multiple principals.  Previous
+    versions assumed one could concatenate keytab files together to make a
+    valid keytab file, which doesn't work with some Kerberos libraries.
+    This caused new keys downloaded for principals after the first to be
+    discarded.  As a side effect of this fix, wallet-rekey always appends
+    new keys directly to the existing keytab file, and never creates a
+    backup copy of that file.
+
     Fix the code to set enctype restrictions for keytab objects in the
     wallet server.
 
diff --git a/client/keytab.c b/client/keytab.c
index d7106e1..80d6978 100644
--- a/client/keytab.c
+++ b/client/keytab.c
@@ -2,7 +2,7 @@
  * Implementation of keytab handling for the wallet client.
  *
  * Written by Russ Allbery <rra@stanford.edu>
- * Copyright 2007, 2008, 2010, 2013
+ * Copyright 2007, 2008, 2010, 2013, 2014
  *     The Board of Trustees of the Leland Stanford Junior University
  *
  * See LICENSE for licensing terms.
@@ -218,7 +218,7 @@ rekey_keytab(struct remctl *r, krb5_context ctx, const char *type,
 {
     char *realm = NULL;
     char *data = NULL;
-    char *tempfile, *backupfile;
+    char *tempfile;
     size_t length = 0;
     int status;
     bool error = false, rekeyed = false;
@@ -231,15 +231,25 @@ rekey_keytab(struct remctl *r, krb5_context ctx, const char *type,
         status = download_keytab(r, type, current->princ, &data, &length);
         if (status != 0) {
             warn("error rekeying for principal %s", current->princ);
-            if (!rekeyed)
-                die("aborting, keytab unchanged");
             error = true;
-        } else if (data != NULL) {
-            if (access(tempfile, F_OK) == 0)
-                append_file(tempfile, data, length);
-            else
-                write_file(tempfile, data, length);
-            rekeyed = true;
+            continue;
+        }
+        write_file(tempfile, data, length);
+        rekeyed = true;
+
+        /*
+         * Now merge the original keytab file with the one containing the new
+         * keys from the rekeying of this principal.
+         */
+        if (access(file, F_OK) != 0) {
+            if (link(tempfile, file) < 0)
+                sysdie("rename of temporary keytab %s to %s failed", tempfile,
+                       file);
+        } else {
+            merge_keytab(ctx, tempfile, file);
+            if (unlink(tempfile) < 0)
+                syswarn("unlink of temporary keytab file %s failed",
+                        tempfile);
         }
     }
 
@@ -247,28 +257,6 @@ rekey_keytab(struct remctl *r, krb5_context ctx, const char *type,
     if (!rekeyed)
         die("no rekeyable principals found");
 
-    /*
-     * Now merge the original keytab file with the one containing the new
-     * keys.  If there is an error, first make a backup of the current keytab
-     * file as keytab.old.
-     */
-    if (access(file, F_OK) != 0) {
-        if (link(tempfile, file) < 0)
-            sysdie("rename of temporary keytab %s to %s failed", tempfile,
-                   file);
-    } else {
-        if (error) {
-            data = read_file(file, &length);
-            xasprintf(&backupfile, "%s.old", file);
-            overwrite_file(backupfile, data, length);
-            warn("partial failure to rekey keytab %s, old keytab left in %s",
-                 file, backupfile);
-            free(backupfile);
-        }
-        merge_keytab(ctx, tempfile, file);
-    }
-    if (unlink(tempfile) < 0)
-        sysdie("unlink of temporary keytab file %s failed", tempfile);
     free(tempfile);
     return !error;
 }
diff --git a/client/wallet-rekey.pod b/client/wallet-rekey.pod
index 47413ad..5892244 100644
--- a/client/wallet-rekey.pod
+++ b/client/wallet-rekey.pod
@@ -1,6 +1,6 @@
 =for stopwords
 wallet-rekey rekey rekeying keytab -hv Heimdal remctl remctld PKINIT kinit
-appdefaults Allbery
+appdefaults Allbery kadmin
 
 =head1 NAME
 
@@ -21,11 +21,8 @@ from the local default realm, requests new wallet keytab objects for each
 principal (removing the realm when naming the keytab), and merges the new
 keys into the keytab.
 
-If an error occurs before any new keys were downloaded, B<wallet-rekey>
-aborts.  If some new keys were successfully downloaded, B<wallet-rekey>
-warns about errors but continues to rekey all principals that it can.  In
-this case, a copy of the existing keytab prior to the rekeying is saved in
-a file named by appending C<.old> to the file name.
+If an error occurs, B<wallet-rekey> continues to rekey all principals that
+it can, producing error messages for those that it cannot rekey.
 
 If no keytab file name is given on the command line, B<wallet-rekey>
 attempts to rekey F</etc/krb5.keytab>, the system default keytab file.
@@ -43,8 +40,10 @@ or:
 
     ktutil -k <keytab> purge
 
-for Heimdal.  This functionality will eventually be provided by
-B<wallet-rekey> directly.
+for Heimdal.  The Heimdal command can be run by any user with access to
+the keytab, but the MIT Kerberos command unfortunately has to be run by a
+someone with direct B<kadmin> access.  This functionality will eventually
+be provided by B<wallet-rekey> directly.
 
 =head1 OPTIONS
 
diff --git a/tests/client/rekey-t.in b/tests/client/rekey-t.in
index 0cfcb5d..c6d0e41 100644
--- a/tests/client/rekey-t.in
+++ b/tests/client/rekey-t.in
@@ -45,7 +45,7 @@ elif [ -z '@REMCTLD@' ] ; then
     rm krb5.conf
     skip_all 'No remctld found'
 else
-    plan 9
+    plan 8
 fi
 remctld_start '@REMCTLD@' "$SOURCE/data/basic.conf"
 wallet="$BUILD/../client/wallet-rekey"
@@ -68,31 +68,27 @@ ok '...and the keytab was untouched' cmp keytab data/fake-keytab-foreign
 rm -f keytab
 
 # Rekeying a keytab where we can't retrieve the principal should produce an
-# error message and abort when it's the first principal.
+# error message.
 cp data/fake-keytab-unknown keytab
 ok_program 'unknown wallet-rekey' 1 \
 'wallet: Unknown keytab service/real-keytab
 wallet: error rekeying for principal service/real-keytab
-wallet: aborting, keytab unchanged' \
+wallet: no rekeyable principals found' \
     "$wallet" -k "$principal" -p 14373 -s localhost -c fake-wallet keytab
 ok '...and the keytab was untouched' cmp keytab data/fake-keytab-unknown
 rm -f keytab
 
-# Rekeying a keytab where we can't retrieve a later principal should leave the
-# original keytab as keytab.old and store, in the new keytab, only the things
-# that it was able to rekey.
+# Rekeying a keytab where we can't retrieve a later principal should add the
+# things we were able to download and produce a warning.
 cp data/fake-keytab-partial keytab
 ok_program 'partial wallet-rekey' 1 \
 'wallet: Unknown keytab service/real-keytab
-wallet: error rekeying for principal service/real-keytab
-wallet: partial failure to rekey keytab keytab, old keytab left in keytab.old'\
+wallet: error rekeying for principal service/real-keytab'\
     "$wallet" -k "$principal" -p 14373 -s localhost -c fake-wallet keytab
 ktutil_list keytab klist-seen
 ktutil_list data/fake-keytab-partial-result klist-good
 ok '...and the rekeyed keytab is correct' cmp klist-seen klist-good
-ok '...and the backup keytab is correct' \
-    cmp keytab.old data/fake-keytab-partial
-rm -f keytab keytab.old klist-seen klist-good
+rm -f keytab klist-seen klist-good
 
 # Clean up.
 rm -f autocreated krb5.conf