When downloading a keytab to a file that already exists, merge the new

keytab keys into that file rather than moving aside the old keytab and
creating a new keytab with only the new keys.

Also fix get handling in the client for all types other than keytabs.
This isn't visible yet since the server doesn't yet support other types
of objects.

11 files changed, 201 insertions, 62 deletions

diff --git a/NEWS b/NEWS
index 852599a..d2f12bd 100644
--- a/NEWS
+++ b/NEWS
@@ -10,6 +10,10 @@ wallet 0.6 (unreleased)
     write the keytab to standard output rather than dying with a cryptic
     error.
 
+    When downloading a keytab to a file that already exists, merge the new
+    keytab keys into that file rather than moving aside the old keytab and
+    creating a new keytab with only the new keys.
+
     Support enforcing a naming policy for wallet objects via a Perl
     function in the wallet server configuration file.
 
diff --git a/TODO b/TODO
index c161606..2a09965 100644
--- a/TODO
+++ b/TODO
@@ -35,8 +35,6 @@ Release 1.0:
 * Log failures in the wallet-backend properly, which also requires
   catching all exceptions.
 
-* Implement special handling for keytabs in the wallet client.
-
 * Add support to the wallet client for getting Kerberos tickets, using the
   -u option similar to leland_srvtab.  Needs good error messages on
   Kerberos failures.
diff --git a/client/error.c b/client/error.c
index e95b284..22e7e5b 100644
--- a/client/error.c
+++ b/client/error.c
@@ -85,7 +85,7 @@ free_error(krb5_context ctx, const char *msg)
 **  Report a Kerberos error and exit.
 */
 void
-die_krb5(krb5_context ctx,  krb5_error_code code, const char *format, ...)
+die_krb5(krb5_context ctx, krb5_error_code code, const char *format, ...)
 {
     const char *k5_msg = NULL;
     char *message;
@@ -93,10 +93,10 @@ die_krb5(krb5_context ctx,  krb5_error_code code, const char *format, ...)
 
     k5_msg = get_error(ctx, code);
     va_start(args, format);
-    if (xasprintf(&message, format, args) < 0)
+    if (xvasprintf(&message, format, args) < 0)
         die("internal error: unable to format error message");
     va_end(args);
-    die("%s: %s\n", message, k5_msg);
+    die("%s: %s", message, k5_msg);
 }
 
 
@@ -104,7 +104,7 @@ die_krb5(krb5_context ctx,  krb5_error_code code, const char *format, ...)
 **  Report a Kerberos error.
 */
 void
-warn_krb5(krb5_context ctx,  krb5_error_code code, const char *format, ...)
+warn_krb5(krb5_context ctx, krb5_error_code code, const char *format, ...)
 {
     const char *k5_msg = NULL;
     char *message;
@@ -112,10 +112,10 @@ warn_krb5(krb5_context ctx,  krb5_error_code code, const char *format, ...)
 
     k5_msg = get_error(ctx, code);
     va_start(args, format);
-    if (xasprintf(&message, format, args) < 0)
+    if (xvasprintf(&message, format, args) < 0)
         die("internal error: unable to format error message");
     va_end(args);
-    warn("%s: %s\n", message, k5_msg);
+    warn("%s: %s", message, k5_msg);
     free(message);
     free_error(ctx, k5_msg);
 }
diff --git a/client/file.c b/client/file.c
index 8e16103..ce25ab5 100644
--- a/client/file.c
+++ b/client/file.c
@@ -3,7 +3,7 @@
 **  File handling for the wallet client.
 **
 **  Written by Russ Allbery <rra@stanford.edu>
-**  Copyright 2007 Board of Trustees, Leland Stanford Jr. University
+**  Copyright 2007, 2008 Board of Trustees, Leland Stanford Jr. University
 **
 **  See LICENSE for licensing terms.
 */
@@ -18,28 +18,40 @@
 
 /*
 **  Given a filename, some data, and a length, write that data to the given
-**  file safely and atomically by creating file.new, writing the data, linking
-**  file to file.bak, and then renaming file.new to file.
+**  file safely, but overwrite any existing file by that name.
 */
 void
-write_file(const char *name, const void *data, size_t length)
+overwrite_file(const char *name, const void *data, size_t length)
 {
     int fd;
     ssize_t status;
-    char *temp, *backup;
 
-    temp = concat(name, ".new", (char *) 0);
-    backup = concat(name, ".bak", (char *) 0);
-    fd = open(temp, O_WRONLY | O_CREAT | O_TRUNC, 0600);
+    fd = open(name, O_WRONLY | O_CREAT | O_TRUNC, 0600);
     if (fd < 0)
-        sysdie("open of %s failed", temp);
+        sysdie("open of %s failed", name);
     status = write(fd, data, length);
     if (status < 0)
-        sysdie("write to %s failed", temp);
+        sysdie("write to %s failed", name);
     else if (status != (ssize_t) length)
-        die("write to %s truncated", temp);
+        die("write to %s truncated", name);
     if (close(fd) < 0)
-        sysdie("close of %s failed (file probably truncated)", temp);
+        sysdie("close of %s failed (file probably truncated)", name);
+}
+
+
+/*
+**  Given a filename, some data, and a length, write that data to the given
+**  file safely and atomically by creating file.new, writing the data, linking
+**  file to file.bak, and then renaming file.new to file.
+*/
+void
+write_file(const char *name, const void *data, size_t length)
+{
+    char *temp, *backup;
+
+    temp = concat(name, ".new", (char *) 0);
+    backup = concat(name, ".bak", (char *) 0);
+    overwrite_file(temp, data, length);
     if (access(name, F_OK) == 0) {
         if (access(backup, F_OK) == 0)
             if (unlink(backup) < 0)
@@ -52,3 +64,41 @@ write_file(const char *name, const void *data, size_t length)
     free(temp);
     free(backup);
 }
+
+
+/*
+**  Given a remctl object, the command prefix, object type, and object name,
+**  and a file (which may be NULL), send a wallet get command and write the
+**  results to the provided file.  If the file is NULL, write the results to
+**  standard output instead.  Returns 0 on success and an exit status on
+**  failure.
+*/
+int
+get_file(struct remctl *r, const char *prefix, const char *type,
+         const char *name, const char *file)
+{
+    const char *command[5];
+    char *data = NULL;
+    size_t length = 0;
+    int status;
+
+    command[0] = prefix;
+    command[1] = "get";
+    command[2] = type;
+    command[3] = name;
+    command[4] = NULL;
+    status = run_command(r, command, &data, &length);
+    if (status != 0)
+        return status;
+    if (data == NULL) {
+        warn("no data returned by wallet server");
+        return 255;
+    }
+    if (file != NULL)
+        write_file(file, data, length);
+    else {
+        if (fwrite(data, length, 1, stdout) != 1)
+            sysdie("cannot write to standard output");
+    }
+    return 0;
+}
diff --git a/client/internal.h b/client/internal.h
index 1dcb608..8595412 100644
--- a/client/internal.h
+++ b/client/internal.h
@@ -37,6 +37,14 @@ BEGIN_DECLS
 int run_command(struct remctl *, const char **command, char **data,
                 size_t *length);
 
+/* Given a remctl object, the type for the wallet interface, object type,
+   object name, and a file (which may be NULL), send a wallet get command and
+   write the results to the provided file.  If the file is NULL, write the
+   results to standard output instead.  Returns 0 on success and an exit
+   status on failure. */
+int get_file(struct remctl *, const char *prefix, const char *type,
+             const char *name, const char *file);
+
 /* Given a remctl object, the Kerberos context, the type for the wallet
    interface, the name of a keytab object, and a file name, call the correct
    wallet commands to download a keytab and write it to that file.  If srvtab
@@ -46,6 +54,10 @@ int get_keytab(struct remctl *, krb5_context, const char *type,
                const char *name, const char *file, const char *srvtab);
 
 /* Given a filename, some data, and a length, write that data to the given
+   file with error checking, overwriting any existing contents. */
+void overwrite_file(const char *name, const void *data, size_t length);
+
+/* Given a filename, some data, and a length, write that data to the given
    file safely and atomically by creating file.new, writing the data, linking
    file to file.bak, and then renaming file.new to file. */
 void write_file(const char *name, const void *data, size_t length);
diff --git a/client/keytab.c b/client/keytab.c
index 7554f50..a3e35ed 100644
--- a/client/keytab.c
+++ b/client/keytab.c
@@ -18,6 +18,55 @@
 
 
 /*
+**  Given keytab data as a pointer to memory and a length and the path of a
+**  second keytab, merge the keys in the memory keytab into the file keytab.
+**  Currently, this doesn't do any cleanup of old kvnos and doesn't handle
+**  duplicate kvnos correctly.  Dies on any error.
+*/
+static void
+merge_keytab(krb5_context ctx, const char *name, const char *data,
+             size_t length)
+{
+    char *tempfile, *oldfile;
+    krb5_keytab old = NULL, temp = NULL;
+    krb5_kt_cursor cursor;
+    krb5_keytab_entry entry;
+    krb5_error_code status;
+
+    tempfile = concat(name, ".new", (char *) 0);
+    oldfile = concat("WRFILE:", name, (char *) 0);
+    overwrite_file(tempfile, data, length);
+    memset(&entry, 0, sizeof(entry));
+    status = krb5_kt_resolve(ctx, oldfile, &old);
+    if (status != 0)
+        die_krb5(ctx, status, "cannot open keytab %s", name);
+    free(oldfile);
+    status = krb5_kt_resolve(ctx, tempfile, &temp);
+    if (status != 0)
+        die_krb5(ctx, status, "cannot open temporary keytab %s", tempfile);
+    status = krb5_kt_start_seq_get(ctx, temp, &cursor);
+    if (status != 0)
+        die_krb5(ctx, status, "cannot read temporary keytab %s", tempfile);
+    while ((status = krb5_kt_next_entry(ctx, temp, &entry, &cursor)) == 0) {
+        status = krb5_kt_add_entry(ctx, old, &entry);
+        if (status != 0)
+            die_krb5(ctx, status, "cannot write to keytab %s", name);
+        krb5_free_keytab_entry_contents(ctx, &entry);
+    }
+    if (status != KRB5_KT_END)
+        die_krb5(ctx, status, "error reading temporary keytab %s", tempfile);
+    krb5_kt_end_seq_get(ctx, temp, &cursor);
+    if (unlink(tempfile) < 0)
+        sysdie("unlink of temporary keytab file %s failed", tempfile);
+    free(tempfile);
+    if (old != NULL)
+        krb5_kt_close(ctx, old);
+    if (temp != NULL)
+        krb5_kt_close(ctx, temp);
+}
+
+
+/*
 **  Configure a given keytab to be synchronized with an AFS kaserver if it
 **  isn't already.  Returns true on success, false on failure.
 */
@@ -79,12 +128,10 @@ get_keytab(struct remctl *r, krb5_context ctx, const char *type,
         warn("no data returned by wallet server");
         return 255;
     }
-    if (file != NULL)
+    if (access(file, F_OK) == 0)
+        merge_keytab(ctx, file, data, length);
+    else
         write_file(file, data, length);
-    else {
-        if (fwrite(data, length, 1, stdout) != 1)
-            sysdie("write to standard output failed");
-    }
     if (srvtab != NULL)
         write_srvtab(ctx, srvtab, name, file);
     return 0;
diff --git a/client/wallet.c b/client/wallet.c
index d48a52c..9dc97c2 100644
--- a/client/wallet.c
+++ b/client/wallet.c
@@ -207,12 +207,16 @@ main(int argc, char *argv[])
     if (!remctl_open(r, options.server, options.port, options.principal))
         die("%s", remctl_error(r));
 
-    /* Most commands, we handle ourselves, but keytab get commands with -f are
-       special. */
-    if (strcmp(argv[0], "get") == 0 && strcmp(argv[1], "keytab") == 0) {
+    /* Most commands, we handle ourselves, but get commands are special and
+       keytab get commands with -f are doubly special. */
+    if (strcmp(argv[0], "get") == 0) {
         if (argc > 3)
             die("too many arguments");
-        status = get_keytab(r, ctx, options.type, argv[2], file, srvtab);
+        if (strcmp(argv[1], "keytab") == 0 && file != NULL) {
+            status = get_keytab(r, ctx, options.type, argv[2], file, srvtab);
+        } else {
+            status = get_file(r, options.type, argv[1], argv[2], file);
+        }
     } else {
         command = xmalloc(sizeof(char *) * (argc + 2));
         command[0] = options.type;
diff --git a/tests/client/basic-t.in b/tests/client/basic-t.in
index b4f539c..d983786 100644
--- a/tests/client/basic-t.in
+++ b/tests/client/basic-t.in
@@ -12,7 +12,7 @@
 . "@abs_top_srcdir@/tests/libtest.sh"
 
 # Print the number of tests.
-total=23
+total=27
 count=1
 echo "$total"
 
@@ -78,29 +78,29 @@ if [ -z "$krb5conf" ] ; then
 fi
 
 # Make sure everything's clean.
-rm -f keytab keytab.bak srvtab srvtab.bak sync-kaserver
+rm -f output output.bak keytab keytab.bak srvtab srvtab.bak sync-kaserver
 
 # Now, we can finally run our tests.  First, basic operations.
 runsuccess "" "$wallet" -k "$principal" -p 14373 -s localhost -c fake-wallet \
-    get keytab -f keytab service/fake-test
-if cmp keytab data/fake-data >/dev/null 2>&1 ; then
+    get file -f output fake-test
+if cmp output data/fake-data >/dev/null 2>&1 ; then
     printcount "ok"
 else
     printcount "not ok"
 fi
-if [ -f keytab.bak ] || [ -f keytab.new ] ; then
+if [ -f output.bak ] || [ -f output.new ] ; then
     printcount "not ok"
 else
     printcount "ok"
 fi
 runsuccess "" "$wallet" -k "$principal" -p 14373 -s localhost -c fake-wallet \
-    get keytab -f keytab service/fake-test
-if cmp keytab data/fake-data >/dev/null 2>&1 ; then
+    get file -f output fake-test
+if cmp output data/fake-data >/dev/null 2>&1 ; then
     printcount "ok"
 else
     printcount "not ok"
 fi
-if [ -f keytab.new ] || [ ! -f keytab.bak ] ; then
+if [ -f output.new ] || [ ! -f output.bak ] ; then
     printcount "not ok"
 else
     printcount "ok"
@@ -118,27 +118,36 @@ cat >> krb5.conf <<EOF
         wallet_principal = $principal
     }
 EOF
-runsuccess "" "$wallet" get keytab -f keytab service/fake-test
-if cmp keytab data/fake-data >/dev/null 2>&1 ; then
+runsuccess "" "$wallet" -f output get file fake-test
+if cmp output data/fake-data >/dev/null 2>&1 ; then
     printcount "ok"
 else
     printcount "not ok"
 fi
+rm -f output output.bak
 
-# Test srvtab support.
-runsuccess "" "$wallet" get keytab -f keytab -S srvtab service/fake-srvtab
+# Test keytab support.
+runsuccess "" "$wallet" get -f keytab keytab service/fake-srvtab
 if cmp keytab data/fake-keytab >/dev/null 2>&1 ; then
     printcount "ok"
     rm keytab
 else
     printcount "not ok"
 fi
-if cmp keytab.bak data/fake-data >/dev/null 2>&1 ; then
+if [ ! -f sync-kaserver ] ; then
     printcount "ok"
-    rm keytab.bak
 else
     printcount "not ok"
 fi
+
+# Test srvtab support.
+runsuccess "" "$wallet" get keytab -f keytab -S srvtab service/fake-srvtab
+if cmp keytab data/fake-keytab >/dev/null 2>&1 ; then
+    printcount "ok"
+else
+    printcount "not ok"
+fi
+rm keytab
 if [ -f sync-kaserver ] ; then
     printcount "ok"
 else
@@ -147,7 +156,6 @@ fi
 runsuccess "" "$wallet" get keytab -f keytab -S srvtab service/fake-srvtab
 if cmp keytab data/fake-keytab >/dev/null 2>&1 ; then
     printcount "ok"
-    rm keytab
 else
     printcount "not ok"
 fi
@@ -159,26 +167,38 @@ else
 fi
 if cmp srvtab data/fake-srvtab >/dev/null 2>&1 ; then
     printcount "ok"
-    rm srvtab
 else
     printcount "not ok"
 fi
 if cmp srvtab.bak data/fake-srvtab >/dev/null 2>&1 ; then
     printcount "ok"
-    rm srvtab.bak
 else
     printcount "not ok"
 fi
+rm -f srvtab srvtab.bak
+
+# Test keytab merging.
+runsuccess "" "$wallet" -f keytab get keytab service/fake-keytab
+(klist -keK keytab 2>&1) | sed '/Keytab name:/d' > klist-seen
+(klist -keK data/fake-keytab-merge 2>&1) | sed '/Keytab name:/d' > klist-good
+if cmp klist-seen klist-good >/dev/null 2>&1 ; then
+    printcount "ok"
+    rm -f keytab klist-seen klist-good
+else
+    printcount "not ok"
+fi
+
+# Test various other client functions and errors.
 runsuccess "This is a fake keytab." "$wallet" get keytab service/fake-output
-runsuccess "Some stuff about service/fake-test" \
-    "$wallet" show keytab service/fake-test
+runsuccess "Some stuff about file fake-test" \
+    "$wallet" show file fake-test
 runfailure 1 "wallet: Unknown object type srvtab" \
     "$wallet" get srvtab service/fake-test
 runfailure 1 "wallet: Unknown keytab service/unknown" \
     "$wallet" show keytab service/unknown
 runfailure 1 "wallet: Unknown keytab service/unknown" \
     "$wallet" get keytab service/unknown
-runsuccess "Expiration date of service/fake-test" \
+runsuccess "Expiration date of keytab service/fake-test" \
     "$wallet" expires keytab service/fake-test
 
 # Clean up.
diff --git a/tests/data/cmd-fake b/tests/data/cmd-fake
index e9744a2..4b97f43 100755
--- a/tests/data/cmd-fake
+++ b/tests/data/cmd-fake
@@ -5,14 +5,14 @@
 # the client test suite.  It doesn't test any of the wallet server code.
 #
 # Written by Russ Allbery <rra@stanford.edu>
-# Copyright 2007 Board of Trustees, Leland Stanford Jr. University
+# Copyright 2007, 2008 Board of Trustees, Leland Stanford Jr. University
 # See LICENSE for licensing terms.
 
 command="$1"
 shift
 type="$1"
 shift
-if [ "$type" != "keytab" ] ; then
+if [ "$type" != "keytab" ] && [ "$type" != "file" ] ; then
     echo "Unknown object type $type" >&2
     exit 1
 fi
@@ -23,7 +23,7 @@ getattr)
         echo "Too many arguments" >&2
         exit 1
     fi
-    if [ "$2" != sync ] ; then
+    if [ "$type" != "keytab" ] || [ "$2" != sync ] ; then
         echo "Unknown attribute $2" >&2
         exit 1
     fi
@@ -44,7 +44,7 @@ setattr)
         echo "Too many arguments" >&2
         exit 1
     fi
-    if [ "$2" != sync ] ; then
+    if [ "$type" != "keytab" ] || [ "$2" != sync ] ; then
         echo "Unknown attribute $2" >&2
         exit 1
     fi
@@ -67,21 +67,25 @@ get)
         echo "Too many arguments" >&2
         exit 1
     fi
-    case "$1" in
-    service/fake-test)
+    case "${type}:${1}" in
+    file:fake-test)
         cat data/fake-data
         exit 0
         ;;
-    service/fake-srvtab)
+    keytab:service/fake-srvtab)
         cat data/fake-keytab
         exit 0
         ;;
-    service/fake-output)
+    keytab:service/fake-keytab)
+        cat data/fake-keytab-2
+        exit 0
+        ;;
+    keytab:service/fake-output)
         printf 'This is a fake keytab.'
         exit 0
         ;;
     *)
-        echo "Unknown keytab $1" >&2
+        echo "Unknown $type $1" >&2
         exit 1
         ;;
     esac
@@ -91,11 +95,11 @@ show)
         echo "Too many arguments" >&2
         exit 1
     fi
-    if [ "$1" = "service/fake-test" ] ; then
-        echo "Some stuff about $1"
+    if [ "$type" = "file" ] && [ "$1" = "fake-test" ] ; then
+        echo "Some stuff about $type $1"
         exit 0
     else
-        echo "Unknown keytab $1" >&2
+        echo "Unknown $type $1" >&2
         exit 1
     fi
     ;;
@@ -104,7 +108,7 @@ expires)
         echo "Too many arguments" >&2
         exit 1
     fi
-    echo "Expiration date of $1"
+    echo "Expiration date of $type $1"
     exit 0
     ;;
 *)
diff --git a/tests/data/fake-keytab-2 b/tests/data/fake-keytab-2
new file mode 100644
index 0000000..0440930
--- /dev/null
+++ b/tests/data/fake-keytab-2
diff --git a/tests/data/fake-keytab-merge b/tests/data/fake-keytab-merge
new file mode 100644
index 0000000..31ddc49
--- /dev/null
+++ b/tests/data/fake-keytab-merge