ldb: move struct ldb_debug_ops to ldb_private.h
[samba.git] / source4 / auth / kerberos / srv_keytab.c
index 1fc8b4cfed288eb474b20e70628bec6099a87515..875d06971cc2dfd849776f583f60c7dec11514e4 100644 (file)
    along with this program.  If not, see <http://www.gnu.org/licenses/>.
 */
 
+/**
+ * @file srv_keytab.c
+ *
+ * @brief Kerberos keytab utility functions
+ *
+ */
 
 #include "includes.h"
 #include "system/kerberos.h"
+#include "auth/credentials/credentials.h"
+#include "auth/credentials/credentials_krb5.h"
 #include "auth/kerberos/kerberos.h"
+#include "auth/kerberos/kerberos_util.h"
 #include "auth/kerberos/kerberos_srv_keytab.h"
+#include "librpc/gen_ndr/ndr_gmsa.h"
+#include "dsdb/samdb/samdb.h"
 
-static void keytab_principals_free(krb5_context context, krb5_principal *set)
+static void keytab_principals_free(krb5_context context,
+                                  uint32_t num_principals,
+                                  krb5_principal *set)
 {
-       int i;
-       for (i = 0; set[i] != NULL; i++) {
-               krb5_free_principal(context, set[i]);
-       }
-}
-
-static krb5_error_code principals_from_list(TALLOC_CTX *parent_ctx,
-                                       const char *samAccountName,
-                                       const char *realm,
-                                       const char **SPNs, int num_SPNs,
-                                       krb5_context context,
-                                       krb5_principal **principals_out,
-                                       const char **error_string)
-{
-       unsigned int i;
-       krb5_error_code ret;
-       char *upper_realm;
-       TALLOC_CTX *tmp_ctx;
-       krb5_principal *principals = NULL;
-       tmp_ctx = talloc_new(parent_ctx);
-       if (!tmp_ctx) {
-               *error_string = "Cannot allocate tmp_ctx";
-               return ENOMEM;
-       }
-
-       if (!realm) {
-               *error_string = "Cannot make principal without a realm";
-               ret = EINVAL;
-               goto done;
-       }
-
-       upper_realm = strupper_talloc(tmp_ctx, realm);
-       if (!upper_realm) {
-               *error_string = "Cannot allocate full upper case realm";
-               ret = ENOMEM;
-               goto done;
-       }
-
-       principals = talloc_zero_array(tmp_ctx, krb5_principal,
-                                       num_SPNs ? (num_SPNs + 2) : 2);
-
-       for (i = 0; num_SPNs && i < num_SPNs; i++) {
-               ret = krb5_parse_name(context, SPNs[i], &principals[i]);
-
-               if (ret) {
-                       *error_string = smb_get_krb5_error_message(context, ret,
-                                                                  parent_ctx);
-                       goto done;
-               }
-       }
-
-       if (samAccountName) {
-               ret = smb_krb5_make_principal(context, &principals[i],
-                                         upper_realm, samAccountName,
-                                         NULL);
-               if (ret) {
-                       *error_string = smb_get_krb5_error_message(context, ret,
-                                                                  parent_ctx);
-                       goto done;
-               }
-       }
-
-done:
-       if (ret) {
-               keytab_principals_free(context, principals);
-       } else {
-               *principals_out = talloc_steal(parent_ctx, principals);
-       }
-       talloc_free(tmp_ctx);
-       return ret;
-}
-
-static krb5_error_code salt_principal(TALLOC_CTX *parent_ctx,
-                               const char *samAccountName,
-                               const char *realm,
-                               const char *saltPrincipal,
-                               krb5_context context,
-                               krb5_principal *salt_princ,
-                               const char **error_string)
-{
-
-       krb5_error_code ret;
-       char *machine_username;
-       char *salt_body;
-       char *lower_realm;
-       char *upper_realm;
-
-       TALLOC_CTX *tmp_ctx;
-
-       if (saltPrincipal) {
-               ret = krb5_parse_name(context, saltPrincipal, salt_princ);
-               if (ret) {
-                       *error_string = smb_get_krb5_error_message(
-                                               context, ret, parent_ctx);
-               }
-               return ret;
-       }
-
-       if (!samAccountName) {
-               (*error_string) = "Cannot determine salt principal, no "
-                               "saltPrincipal or samAccountName specified";
-               return EINVAL;
-       }
-
-       if (!realm) {
-               *error_string = "Cannot make principal without a realm";
-               return EINVAL;
-       }
-
-       tmp_ctx = talloc_new(parent_ctx);
-       if (!tmp_ctx) {
-               *error_string = "Cannot allocate tmp_ctx";
-               return ENOMEM;
-       }
-
-       machine_username = talloc_strdup(tmp_ctx, samAccountName);
-       if (!machine_username) {
-               *error_string = "Cannot duplicate samAccountName";
-               talloc_free(tmp_ctx);
-               return ENOMEM;
-       }
-
-       if (machine_username[strlen(machine_username)-1] == '$') {
-               machine_username[strlen(machine_username)-1] = '\0';
-       }
-
-       lower_realm = strlower_talloc(tmp_ctx, realm);
-       if (!lower_realm) {
-               *error_string = "Cannot allocate to lower case realm";
-               talloc_free(tmp_ctx);
-               return ENOMEM;
-       }
-
-       upper_realm = strupper_talloc(tmp_ctx, realm);
-       if (!upper_realm) {
-               *error_string = "Cannot allocate to upper case realm";
-               talloc_free(tmp_ctx);
-               return ENOMEM;
-       }
-
-       salt_body = talloc_asprintf(tmp_ctx, "%s.%s",
-                                   machine_username, lower_realm);
-       if (!salt_body) {
-               *error_string = "Cannot form salt principal body";
-               talloc_free(tmp_ctx);
-               return ENOMEM;
-       }
+       uint32_t i;
 
-       ret = smb_krb5_make_principal(context, salt_princ, upper_realm,
-                                               "host", salt_body, NULL);
-       if (ret) {
-               *error_string = smb_get_krb5_error_message(context,
-                                                          ret, parent_ctx);
+       for (i = 0; i < num_principals; i++) {
+               krb5_free_principal(context, set[i]);
        }
-
-       talloc_free(tmp_ctx);
-       return ret;
 }
 
 static krb5_error_code keytab_add_keys(TALLOC_CTX *parent_ctx,
+                                      uint32_t num_principals,
                                       krb5_principal *principals,
                                       krb5_principal salt_princ,
                                       int kvno,
@@ -210,19 +72,48 @@ static krb5_error_code keytab_add_keys(TALLOC_CTX *parent_ctx,
 
                ZERO_STRUCT(entry);
 
-               ret = create_kerberos_key_from_string_direct(context,
-                                               salt_princ, &password,
-                                               KRB5_KT_KEY(&entry),
-                                               enctypes[i]);
+               ret = smb_krb5_create_key_from_string(context,
+                                                     salt_princ,
+                                                     NULL,
+                                                     &password,
+                                                     enctypes[i],
+                                                     KRB5_KT_KEY(&entry));
                if (ret != 0) {
+                       *error_string = talloc_strdup(parent_ctx,
+                                                     "Failed to create key from string");
                        return ret;
                }
 
-                entry.vno = kvno;
+               entry.vno = kvno;
+
+               for (p = 0; p < num_principals; p++) {
+                       bool found = false;
 
-               for (p = 0; principals[p]; p++) {
                        unparsed = NULL;
                        entry.principal = principals[p];
+
+                       ret = smb_krb5_is_exact_entry_in_keytab(parent_ctx,
+                                                               context,
+                                                               keytab,
+                                                               &entry,
+                                                               &found,
+                                                               error_string);
+                       if (ret != 0) {
+                               krb5_free_keyblock_contents(context,
+                                                           KRB5_KT_KEY(&entry));
+                               return ret;
+                       }
+
+                       /*
+                        * Do not add the exact same key twice, this
+                        * will allow "samba-tool domain exportkeytab"
+                        * to refresh a keytab rather than infinitely
+                        * extend it
+                        */
+                       if (found) {
+                               continue;
+                       }
+
                        ret = krb5_kt_add_entry(context, keytab, &entry);
                        if (ret != 0) {
                                char *k5_error_string =
@@ -251,24 +142,28 @@ static krb5_error_code keytab_add_keys(TALLOC_CTX *parent_ctx,
        return 0;
 }
 
-static krb5_error_code create_keytab(TALLOC_CTX *parent_ctx,
-                                    const char *samAccountName,
-                                    const char *realm,
-                                    const char *saltPrincipal,
-                                    int kvno,
-                                    const char *new_secret,
-                                    const char *old_secret,
-                                    uint32_t supp_enctypes,
-                                    krb5_principal *principals,
-                                    krb5_context context,
-                                    krb5_keytab keytab,
-                                    bool add_old,
-                                    const char **error_string)
+/*
+ * This is the inner part of smb_krb5_update_keytab on an open keytab
+ * and without the deletion
+ */
+static krb5_error_code smb_krb5_fill_keytab(TALLOC_CTX *parent_ctx,
+                                           const char *saltPrincipal,
+                                           int kvno,
+                                           const char *new_secret,
+                                           const char *old_secret,
+                                           uint32_t supp_enctypes,
+                                           uint32_t num_principals,
+                                           krb5_principal *principals,
+                                           krb5_context context,
+                                           krb5_keytab keytab,
+                                           bool add_old,
+                                           const char **perror_string)
 {
        krb5_error_code ret;
        krb5_principal salt_princ = NULL;
        krb5_enctype *enctypes;
        TALLOC_CTX *mem_ctx;
+       const char *error_string = NULL;
 
        if (!new_secret) {
                /* There is no password here, so nothing to do */
@@ -277,40 +172,51 @@ static krb5_error_code create_keytab(TALLOC_CTX *parent_ctx,
 
        mem_ctx = talloc_new(parent_ctx);
        if (!mem_ctx) {
-               *error_string = "unable to allocate tmp_ctx for create_keytab";
+               *perror_string = talloc_strdup(parent_ctx,
+                       "unable to allocate tmp_ctx for smb_krb5_fill_keytab");
                return ENOMEM;
        }
 
        /* The salt used to generate these entries may be different however,
         * fetch that */
-       ret = salt_principal(mem_ctx, samAccountName, realm, saltPrincipal,
-                            context, &salt_princ, error_string);
+       ret = krb5_parse_name(context, saltPrincipal, &salt_princ);
        if (ret) {
+               *perror_string = smb_get_krb5_error_message(context,
+                                                          ret,
+                                                          parent_ctx);
                talloc_free(mem_ctx);
                return ret;
        }
 
        ret = ms_suptypes_to_ietf_enctypes(mem_ctx, supp_enctypes, &enctypes);
        if (ret) {
-               *error_string = talloc_asprintf(parent_ctx,
-                                       "create_keytab: generating list of "
+               *perror_string = talloc_asprintf(parent_ctx,
+                                       "smb_krb5_fill_keytab: generating list of "
                                        "encryption types failed (%s)\n",
                                        smb_get_krb5_error_message(context,
                                                                ret, mem_ctx));
                goto done;
        }
 
-       ret = keytab_add_keys(mem_ctx, principals,
+       ret = keytab_add_keys(mem_ctx,
+                             num_principals,
+                             principals,
                              salt_princ, kvno, new_secret,
-                             context, enctypes, keytab, error_string);
+                             context, enctypes, keytab, &error_string);
        if (ret) {
+               *perror_string = talloc_steal(parent_ctx, error_string);
                goto done;
        }
 
        if (old_secret && add_old && kvno != 0) {
-               ret = keytab_add_keys(mem_ctx, principals,
+               ret = keytab_add_keys(mem_ctx,
+                                     num_principals,
+                                     principals,
                                      salt_princ, kvno - 1, old_secret,
-                                     context, enctypes, keytab, error_string);
+                                     context, enctypes, keytab, &error_string);
+               if (ret) {
+                       *perror_string = talloc_steal(parent_ctx, error_string);
+               }
        }
 
 done:
@@ -319,136 +225,224 @@ done:
        return ret;
 }
 
-/*
- * Walk the keytab, looking for entries of this principal name,
- * with KVNO other than current kvno -1.
- *
- * These entries are now stale,
- * we only keep the current and previous entries around.
- *
- * Inspired by the code in Samba3 for 'use kerberos keytab'.
- */
-
-static krb5_error_code remove_old_entries(TALLOC_CTX *parent_ctx,
-                                         int kvno,
-                                         krb5_principal *principals,
-                                         bool delete_all_kvno,
-                                         krb5_context context,
-                                         krb5_keytab keytab,
-                                         bool *found_previous,
-                                         const char **error_string)
+NTSTATUS smb_krb5_fill_keytab_gmsa_keys(TALLOC_CTX *mem_ctx,
+                                       struct smb_krb5_context *smb_krb5_context,
+                                       krb5_keytab keytab,
+                                       krb5_principal principal,
+                                       struct ldb_context *samdb,
+                                       struct ldb_dn *dn,
+                                       bool include_historic_keys,
+                                       const char **error_string)
 {
-       krb5_error_code ret, ret2;
-       krb5_kt_cursor cursor;
-       TALLOC_CTX *mem_ctx = talloc_new(parent_ctx);
-
-       if (!mem_ctx) {
-               return ENOMEM;
-       }
-
-       *found_previous = false;
-
-       /* for each entry in the keytab */
-       ret = krb5_kt_start_seq_get(context, keytab, &cursor);
-       switch (ret) {
-       case 0:
-               break;
-#ifdef HEIM_ERR_OPNOTSUPP
-       case HEIM_ERR_OPNOTSUPP:
-#endif
-       case ENOENT:
-       case KRB5_KT_END:
-               /* no point enumerating if there isn't anything here */
-               talloc_free(mem_ctx);
-               return 0;
-       default:
-               *error_string = talloc_asprintf(parent_ctx,
-                       "failed to open keytab for read of old entries: %s\n",
-                       smb_get_krb5_error_message(context, ret, mem_ctx));
-               talloc_free(mem_ctx);
-               return ret;
+       const char *gmsa_attrs[] = {
+               "msDS-ManagedPassword",
+               "msDS-KeyVersionNumber",
+               "sAMAccountName",
+               "msDS-SupportedEncryptionTypes",
+               NULL
+       };
+
+       NTSTATUS status;
+       struct ldb_message *msg;
+       const struct ldb_val *managed_password_blob;
+       const char *managed_pw_utf8;
+       const char *previous_managed_pw_utf8;
+       const char *username;
+       const char *salt_principal;
+       uint32_t kvno = 0;
+       uint32_t supported_enctypes = 0;
+       krb5_context context = smb_krb5_context->krb5_context;
+       struct cli_credentials *cred = NULL;
+       const char *realm = NULL;
+
+       /*
+        * Search for msDS-ManagedPassword (and other attributes to
+        * avoid a race) as this was not in the original search.
+        */
+       int ret;
+
+       TALLOC_CTX *tmp_ctx = talloc_new(mem_ctx);
+       if (tmp_ctx == NULL) {
+               return NT_STATUS_NO_MEMORY;
+       }
+
+       ret = dsdb_search_one(samdb,
+                             tmp_ctx,
+                             &msg,
+                             dn,
+                             LDB_SCOPE_BASE,
+                             gmsa_attrs, 0,
+                             "(objectClass=msDS-GroupManagedServiceAccount)");
+
+       if (ret == LDB_ERR_NO_SUCH_OBJECT) {
+               /*
+                * Race condition, object has gone, or just wasn't a
+                * gMSA
+                */
+               *error_string = talloc_asprintf(mem_ctx,
+                                               "Did not find gMSA at %s",
+                                               ldb_dn_get_linearized(dn));
+               TALLOC_FREE(tmp_ctx);
+               return NT_STATUS_NO_SUCH_USER;
+       }
+
+       if (ret != LDB_SUCCESS) {
+               *error_string = talloc_asprintf(mem_ctx,
+                                               "Error looking for gMSA at %s: %s",
+                                               ldb_dn_get_linearized(dn), ldb_errstring(samdb));
+               TALLOC_FREE(tmp_ctx);
+               return NT_STATUS_UNSUCCESSFUL;
+       }
+
+       /* Extract out passwords */
+       managed_password_blob = ldb_msg_find_ldb_val(msg, "msDS-ManagedPassword");
+
+       if (managed_password_blob == NULL) {
+               /*
+                * No password set on this yet or not readable by this user
+                */
+               *error_string = talloc_asprintf(mem_ctx,
+                                               "Did not find msDS-ManagedPassword at %s",
+                                               ldb_dn_get_extended_linearized(mem_ctx, msg->dn, 1));
+               TALLOC_FREE(tmp_ctx);
+               return NT_STATUS_NO_USER_KEYS;
+       }
+
+       cred = cli_credentials_init(tmp_ctx);
+       if (cred == NULL) {
+               *error_string = talloc_asprintf(mem_ctx,
+                                               "Could not allocate cli_credentials for %s",
+                                               ldb_dn_get_linearized(msg->dn));
+               TALLOC_FREE(tmp_ctx);
+               return NT_STATUS_NO_MEMORY;
+       }
+
+       realm = smb_krb5_principal_get_realm(tmp_ctx,
+                                            context,
+                                            principal);
+       if (realm == NULL) {
+               *error_string = talloc_asprintf(mem_ctx,
+                                               "Could not allocate copy of realm for %s",
+                                               ldb_dn_get_linearized(msg->dn));
+               TALLOC_FREE(tmp_ctx);
+               return NT_STATUS_NO_MEMORY;
+       }
+
+       cli_credentials_set_realm(cred, realm, CRED_SPECIFIED);
+
+       username = ldb_msg_find_attr_as_string(msg, "sAMAccountName", NULL);
+       if (username == NULL) {
+               *error_string = talloc_asprintf(mem_ctx,
+                                               "No sAMAccountName on %s",
+                                               ldb_dn_get_linearized(msg->dn));
+               TALLOC_FREE(tmp_ctx);
+               return NT_STATUS_INVALID_ACCOUNT_NAME;
+       }
+
+       cli_credentials_set_username(cred, username, CRED_SPECIFIED);
+
+       /*
+        * Note that this value may not be correct, it is updated
+        * after the query that gives us the passwords
+        */
+       kvno = ldb_msg_find_attr_as_uint(msg, "msDS-KeyVersionNumber", 0);
+
+       cli_credentials_set_kvno(cred, kvno);
+
+       supported_enctypes = ldb_msg_find_attr_as_uint(msg,
+                                                      "msDS-SupportedEncryptionTypes",
+                                                      ENC_STRONG_SALTED_TYPES);
+       /*
+        * We trim this down to just the salted AES types, as the
+        * passwords are now wrong for rc4-hmac due to the mapping of
+        * invalid sequences in UTF16_MUNGED -> UTF8 string conversion
+        * within cli_credentials_get_password(). Users using this new
+        * feature won't be using such weak crypto anyway.  If
+        * required we could also set the NT Hash as a key directly,
+        * this is just a limitation of smb_krb5_fill_keytab() taking
+        * a simple string as input.
+        */
+       supported_enctypes &= ENC_STRONG_SALTED_TYPES;
+
+       /* Update the keytab */
+
+       status = cli_credentials_set_gmsa_passwords(cred,
+                                                   managed_password_blob,
+                                                   true /* for keytab */,
+                                                   error_string);
+
+       if (!NT_STATUS_IS_OK(status)) {
+               *error_string = talloc_asprintf(mem_ctx,
+                                               "Could not parse gMSA passwords on %s: %s",
+                                               ldb_dn_get_linearized(msg->dn),
+                                               *error_string);
+               TALLOC_FREE(tmp_ctx);
+               return status;
+       }
+
+       managed_pw_utf8 = cli_credentials_get_password(cred);
+
+       previous_managed_pw_utf8 = cli_credentials_get_old_password(cred);
+
+       salt_principal = cli_credentials_get_salt_principal(cred, tmp_ctx);
+       if (salt_principal == NULL) {
+               *error_string = talloc_asprintf(mem_ctx,
+                                               "Failed to generate salt principal for %s",
+                                               ldb_dn_get_linearized(msg->dn));
+               TALLOC_FREE(tmp_ctx);
+               return NT_STATUS_NO_MEMORY;
+       }
+
+       ret = smb_krb5_fill_keytab(tmp_ctx,
+                                  salt_principal,
+                                  kvno,
+                                  managed_pw_utf8,
+                                  previous_managed_pw_utf8,
+                                  supported_enctypes,
+                                  1,
+                                  &principal,
+                                  context,
+                                  keytab,
+                                  include_historic_keys,
+                                  error_string);
+       if (ret) {
+               *error_string = talloc_asprintf(mem_ctx,
+                                               "Failed to add keys from %s to keytab: %s",
+                                               ldb_dn_get_linearized(msg->dn),
+                                               *error_string);
+               TALLOC_FREE(tmp_ctx);
+               return NT_STATUS_UNSUCCESSFUL;
        }
 
-       while (!ret) {
-               unsigned int i;
-               bool matched = false;
-               krb5_keytab_entry entry;
-               ret = krb5_kt_next_entry(context, keytab, &entry, &cursor);
-               if (ret) {
-                       break;
-               }
-               for (i = 0; principals[i]; i++) {
-                       /* if it matches our principal */
-                       if (smb_krb5_kt_compare(context, &entry,
-                                               principals[i], 0, 0)) {
-                               matched = true;
-                               break;
-                       }
-               }
-
-               if (!matched) {
-                       /* Free the entry,
-                        * it wasn't the one we were looking for anyway */
-                       krb5_kt_free_entry(context, &entry);
-                       continue;
-               }
-
-               /* delete it, if it is not kvno -1 */
-               if (entry.vno != (kvno - 1 )) {
-                       /* Release the enumeration.  We are going to
-                        * have to start this from the top again,
-                        * because deletes during enumeration may not
-                        * always be consistent.
-                        *
-                        * Also, the enumeration locks a FILE: keytab
-                        */
-
-                       krb5_kt_end_seq_get(context, keytab, &cursor);
-
-                       ret = krb5_kt_remove_entry(context, keytab, &entry);
-                       krb5_kt_free_entry(context, &entry);
-
-                       /* Deleted: Restart from the top */
-                       ret2 = krb5_kt_start_seq_get(context, keytab, &cursor);
-                       if (ret2) {
-                               krb5_kt_free_entry(context, &entry);
-                               DEBUG(1, ("failed to restart enumeration of keytab: %s\n",
-                                         smb_get_krb5_error_message(context,
-                                                               ret, mem_ctx)));
-
-                               talloc_free(mem_ctx);
-                               return ret2;
-                       }
-
-                       if (ret) {
-                               break;
-                       }
-
-               } else {
-                       *found_previous = true;
-               }
-
-               /* Free the entry, we don't need it any more */
-               krb5_kt_free_entry(context, &entry);
-       }
-       krb5_kt_end_seq_get(context, keytab, &cursor);
-
-       switch (ret) {
-       case 0:
-               break;
-       case ENOENT:
-       case KRB5_KT_END:
-               ret = 0;
-               break;
-       default:
-               *error_string = talloc_asprintf(parent_ctx,
-                       "failed in deleting old entries for principal: %s\n",
-                       smb_get_krb5_error_message(context, ret, mem_ctx));
-       }
-       talloc_free(mem_ctx);
-       return ret;
+       TALLOC_FREE(tmp_ctx);
+       return NT_STATUS_OK;
 }
 
+/**
+ * @brief Update a Kerberos keytab and removes any obsolete keytab entries.
+ *
+ * If the keytab does not exist, this function will create one.
+ *
+ * @param[in] parent_ctx       Talloc memory context
+ * @param[in] context          Kerberos context
+ * @param[in] keytab_name      Keytab to open
+ * @param[in] samAccountName   User account to update
+ * @param[in] realm            Kerberos realm
+ * @param[in] SPNs             Service principal names to update
+ * @param[in] num_SPNs         Length of SPNs
+ * @param[in] saltPrincipal    Salt used for AES encryption.
+ *                             Required, unless delete_all_kvno is set.
+ * @param[in] old_secret       Old password
+ * @param[in] new_secret       New password
+ * @param[in] kvno             Current key version number
+ * @param[in] supp_enctypes    msDS-SupportedEncryptionTypes bit-field
+ * @param[in] delete_all_kvno  Removes all obsolete entries, without
+ *                             recreating the keytab.
+ * @param[out] _keytab         If supplied, returns the keytab
+ * @param[out] perror_string   Error string on failure
+ *
+ * @return                     0 on success, errno on failure
+ */
 krb5_error_code smb_krb5_update_keytab(TALLOC_CTX *parent_ctx,
                                krb5_context context,
                                const char *keytab_name,
@@ -463,13 +457,16 @@ krb5_error_code smb_krb5_update_keytab(TALLOC_CTX *parent_ctx,
                                uint32_t supp_enctypes,
                                bool delete_all_kvno,
                                krb5_keytab *_keytab,
-                               const char **error_string)
+                               const char **perror_string)
 {
-       krb5_keytab keytab;
+       krb5_keytab keytab = NULL;
        krb5_error_code ret;
-       bool found_previous;
-       TALLOC_CTX *tmp_ctx;
+       bool found_previous = false;
+       TALLOC_CTX *tmp_ctx = NULL;
        krb5_principal *principals = NULL;
+       uint32_t num_principals = 0;
+       char *upper_realm;
+       const char *error_string = NULL;
 
        if (keytab_name == NULL) {
                return ENOENT;
@@ -477,7 +474,7 @@ krb5_error_code smb_krb5_update_keytab(TALLOC_CTX *parent_ctx,
 
        ret = krb5_kt_resolve(context, keytab_name, &keytab);
        if (ret) {
-               *error_string = smb_get_krb5_error_message(context,
+               *perror_string = smb_get_krb5_error_message(context,
                                                           ret, parent_ctx);
                return ret;
        }
@@ -486,44 +483,73 @@ krb5_error_code smb_krb5_update_keytab(TALLOC_CTX *parent_ctx,
 
        tmp_ctx = talloc_new(parent_ctx);
        if (!tmp_ctx) {
-               return ENOMEM;
+               *perror_string = talloc_strdup(parent_ctx,
+                                             "Failed to allocate memory context");
+               ret = ENOMEM;
+               goto done;
        }
 
-       /* Get the principal we will store the new keytab entries under */
-       ret = principals_from_list(tmp_ctx,
-                                 samAccountName, realm, SPNs, num_SPNs,
-                                 context, &principals, error_string);
+       upper_realm = strupper_talloc(tmp_ctx, realm);
+       if (upper_realm == NULL) {
+               *perror_string = talloc_strdup(parent_ctx,
+                                             "Cannot allocate memory to upper case realm");
+               ret = ENOMEM;
+               goto done;
+       }
 
+       ret = smb_krb5_create_principals_array(tmp_ctx,
+                                              context,
+                                              samAccountName,
+                                              upper_realm,
+                                              num_SPNs,
+                                              SPNs,
+                                              &num_principals,
+                                              &principals,
+                                              &error_string);
        if (ret != 0) {
-               *error_string = talloc_asprintf(parent_ctx,
+               *perror_string = talloc_asprintf(parent_ctx,
                        "Failed to load principals from ldb message: %s\n",
-                       *error_string);
+                       error_string);
                goto done;
        }
 
-       ret = remove_old_entries(tmp_ctx, kvno, principals, delete_all_kvno,
-                                context, keytab, &found_previous, error_string);
+       ret = smb_krb5_remove_obsolete_keytab_entries(tmp_ctx,
+                                                     context,
+                                                     keytab,
+                                                     num_principals,
+                                                     principals,
+                                                     kvno,
+                                                     &found_previous,
+                                                     &error_string);
        if (ret != 0) {
-               *error_string = talloc_asprintf(parent_ctx,
+               *perror_string = talloc_asprintf(parent_ctx,
                        "Failed to remove old principals from keytab: %s\n",
-                       *error_string);
+                       error_string);
                goto done;
        }
 
        if (!delete_all_kvno) {
                /* Create a new keytab.  If during the cleanout we found
-                * entires for kvno -1, then don't try and duplicate them.
+                * entries for kvno -1, then don't try and duplicate them.
                 * Otherwise, add kvno, and kvno -1 */
+               if (saltPrincipal == NULL) {
+                       *perror_string = talloc_strdup(parent_ctx,
+                                                      "No saltPrincipal provided");
+                       ret = EINVAL;
+                       goto done;
+               }
 
-               ret = create_keytab(tmp_ctx,
-                                   samAccountName, realm, saltPrincipal,
+               ret = smb_krb5_fill_keytab(tmp_ctx,
+                                   saltPrincipal,
                                    kvno, new_secret, old_secret,
-                                   supp_enctypes, principals,
+                                   supp_enctypes,
+                                   num_principals,
+                                   principals,
                                    context, keytab,
                                    found_previous ? false : true,
-                                   error_string);
+                                   &error_string);
                if (ret) {
-                       talloc_steal(parent_ctx, *error_string);
+                       *perror_string = talloc_steal(parent_ctx, error_string);
                }
        }
 
@@ -533,7 +559,7 @@ krb5_error_code smb_krb5_update_keytab(TALLOC_CTX *parent_ctx,
        }
 
 done:
-       keytab_principals_free(context, principals);
+       keytab_principals_free(context, num_principals, principals);
        if (ret != 0 || _keytab == NULL) {
                krb5_kt_close(context, keytab);
        }
@@ -541,11 +567,28 @@ done:
        return ret;
 }
 
+/**
+ * @brief Wrapper around smb_krb5_update_keytab() for creating an in-memory keytab
+ *
+ * @param[in] parent_ctx       Talloc memory context
+ * @param[in] context          Kerberos context
+ * @param[in] new_secret       New password
+ * @param[in] samAccountName   User account to update
+ * @param[in] realm            Kerberos realm
+ * @param[in] salt_principal   Salt used for AES encryption.
+ *                             Required, unless delete_all_kvno is set.
+ * @param[in] kvno             Current key version number
+ * @param[out] keytab          If supplied, returns the keytab
+ * @param[out] keytab_name     Returns the created keytab name
+ *
+ * @return                     0 on success, errno on failure
+ */
 krb5_error_code smb_krb5_create_memory_keytab(TALLOC_CTX *parent_ctx,
                                krb5_context context,
                                const char *new_secret,
                                const char *samAccountName,
                                const char *realm,
+                               const char *salt_principal,
                                int kvno,
                                krb5_keytab *keytab,
                                const char **keytab_name)
@@ -553,7 +596,7 @@ krb5_error_code smb_krb5_create_memory_keytab(TALLOC_CTX *parent_ctx,
        krb5_error_code ret;
        TALLOC_CTX *mem_ctx = talloc_new(parent_ctx);
        const char *rand_string;
-       const char *error_string;
+       const char *error_string = NULL;
        if (!mem_ctx) {
                return ENOMEM;
        }
@@ -570,10 +613,9 @@ krb5_error_code smb_krb5_create_memory_keytab(TALLOC_CTX *parent_ctx,
                return ENOMEM;
        }
 
-
        ret = smb_krb5_update_keytab(mem_ctx, context,
                                     *keytab_name, samAccountName, realm,
-                                    NULL, 0, NULL, new_secret, NULL,
+                                    NULL, 0, salt_principal, new_secret, NULL,
                                     kvno, ENC_ALL_TYPES,
                                     false, keytab, &error_string);
        if (ret == 0) {