From 782e71d568957e05233f63fa8dca7cc53ba1afa1 Mon Sep 17 00:00:00 2001
From: Russ Allbery <eagle@eyrie.org>
Date: Mon, 6 Jan 2014 21:09:00 -0800
Subject: 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>
---
 client/keytab.c         | 52 +++++++++++++++++++------------------------------
 client/wallet-rekey.pod | 15 +++++++-------
 2 files changed, 27 insertions(+), 40 deletions(-)

(limited to 'client')

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
 
-- 
cgit v1.2.3