krb5 commit: Fix LDAP policy enforcement of pw_expiration

https://github.com/krb5/krb5/commit/6b004dd5739bded71be4290c11e7ac3a816c7e09
commit 6b004dd5739bded71be4290c11e7ac3a816c7e09
Author: Robbie Harwood <[email protected]>
Date:   Tue Dec 17 17:37:41 2019 -0500

    Fix LDAP policy enforcement of pw_expiration
    
    In the LDAP backend, the change mask is used to determine what LDAP
    attributes to update.  As a result, password expiration was not set
    from policy when running during addprinc, among other issues.
    However, when the mask did not contain KADM5_PRINCIPAL, pw_expiration
    would be applied regardless, which meant that (for instance) changing
    the password would cause the password application to be applied.
    
    Remove the check for KADM5_PRINCIPAL, and fix the mask to contain
    KADM5_PW_EXPIRATION where appropriate.  Add a regression test to
    t_kdb.py.
    
    [[email protected]: also set KADM5_ATTRIBUTES for randkey and setkey
    since they both unset KRB5_KDB_REQUIRES_PWCHANGE; edited comments and
    commit message]
    
    ticket: 8861 (new)
    tags: pullup
    target_version: 1.17-next

 src/lib/kadm5/srv/svr_principal.c                  |   92 +++++++++-----------
 src/plugins/kdb/ldap/libkdb_ldap/ldap_principal2.c |   13 ---
 src/tests/t_kdb.py                                 |   17 ++++
 3 files changed, 60 insertions(+), 62 deletions(-)

diff --git a/src/lib/kadm5/srv/svr_principal.c b/src/lib/kadm5/srv/svr_principal.c
index 6ee262e..53ecbe1 100644
--- a/src/lib/kadm5/srv/svr_principal.c
+++ b/src/lib/kadm5/srv/svr_principal.c
@@ -356,6 +356,11 @@ kadm5_create_principal_3(void *server_handle,
     kdb = calloc(1, sizeof(*kdb));
     if (kdb == NULL)
         return ENOMEM;
+
+    /* In all cases the principal entry is new and key data is set; let the
+     * database provider know. */
+    kdb->mask = mask | KADM5_KEY_DATA | KADM5_PRINCIPAL;
+
     memset(&adb, 0, sizeof(osa_princ_ent_rec));
 
     /*
@@ -405,14 +410,12 @@ kadm5_create_principal_3(void *server_handle,
         kdb->expiration = handle->params.expiration;
 
     kdb->pw_expiration = 0;
-    if (have_polent) {
-        if(polent.pw_max_life)
-            kdb->pw_expiration = ts_incr(now, polent.pw_max_life);
-        else
-            kdb->pw_expiration = 0;
-    }
-    if ((mask & KADM5_PW_EXPIRATION))
+    if (mask & KADM5_PW_EXPIRATION) {
         kdb->pw_expiration = entry->pw_expiration;
+    } else if (have_polent && polent.pw_max_life) {
+        kdb->mask |= KADM5_PW_EXPIRATION;
+        kdb->pw_expiration = ts_incr(now, polent.pw_max_life);
+    }
 
     kdb->last_success = 0;
     kdb->last_failed = 0;
@@ -503,9 +506,6 @@ kadm5_create_principal_3(void *server_handle,
         adb.policy = entry->policy;
     }
 
-    /* In all cases key and the principal data is set, let the database provider know */
-    kdb->mask = mask | KADM5_KEY_DATA | KADM5_PRINCIPAL ;
-
     /* store the new db entry */
     ret = kdb_put_entry(handle, kdb, &adb);
 
@@ -601,6 +601,9 @@ kadm5_modify_principal(void *server_handle,
     if (ret)
         return(ret);
 
+    /* Let the mask propagate to the database provider. */
+    kdb->mask = mask;
+
     /*
      * This is pretty much the same as create ...
      */
@@ -616,11 +619,15 @@ kadm5_modify_principal(void *server_handle,
             free(adb.policy);
         adb.policy = strdup(entry->policy);
     }
-    if (have_pol) {
+
+    if (mask & KADM5_PW_EXPIRATION) {
+        kdb->pw_expiration = entry->pw_expiration;
+    } else if (have_pol) {
         /* set pw_max_life based on new policy */
+        kdb->mask |= KADM5_PW_EXPIRATION;
         if (pol.pw_max_life) {
             ret = krb5_dbe_lookup_last_pwd_change(handle->context, kdb,
-                                                  &(kdb->pw_expiration));
+                                                  &kdb->pw_expiration);
             if (ret)
                 goto done;
             kdb->pw_expiration = ts_incr(kdb->pw_expiration, pol.pw_max_life);
@@ -642,8 +649,6 @@ kadm5_modify_principal(void *server_handle,
         kdb->max_life = entry->max_life;
     if ((mask & KADM5_PRINC_EXPIRE_TIME))
         kdb->expiration = entry->princ_expire_time;
-    if (mask & KADM5_PW_EXPIRATION)
-        kdb->pw_expiration = entry->pw_expiration;
     if (mask & KADM5_MAX_RLIFE)
         kdb->max_renewable_life = entry->max_renewable_life;
 
@@ -682,9 +687,6 @@ kadm5_modify_principal(void *server_handle,
         kdb->fail_auth_count = 0;
     }
 
-    /* let the mask propagate to the database provider */
-    kdb->mask = mask;
-
     ret = k5_kadm5_hook_modify(handle->context, handle->hook_handles,
                                KADM5_HOOK_STAGE_PRECOMMIT, entry, mask);
     if (ret)
@@ -1362,6 +1364,11 @@ kadm5_chpass_principal_3(void *server_handle,
     if ((ret = kdb_get_entry(handle, principal, &kdb, &adb)))
         return(ret);
 
+    /* We will always be changing the key data, attributes, auth failure count,
+     * and password expiration time. */
+    kdb->mask = KADM5_KEY_DATA | KADM5_ATTRIBUTES | KADM5_FAIL_AUTH_COUNT |
+        KADM5_PW_EXPIRATION;
+
     ret = apply_keysalt_policy(handle, adb.policy, n_ks_tuple, ks_tuple,
                                &new_n_ks_tuple, &new_ks_tuple);
     if (ret)
@@ -1407,6 +1414,7 @@ kadm5_chpass_principal_3(void *server_handle,
     if (ret)
         goto done;
 
+    kdb->pw_expiration = 0;
     if ((adb.aux_attributes & KADM5_POLICY)) {
         /* the policy was loaded before */
 
@@ -1439,10 +1447,6 @@ kadm5_chpass_principal_3(void *server_handle,
 
         if (pol.pw_max_life)
             kdb->pw_expiration = ts_incr(now, pol.pw_max_life);
-        else
-            kdb->pw_expiration = 0;
-    } else {
-        kdb->pw_expiration = 0;
     }
 
 #ifdef USE_PASSWORD_SERVER
@@ -1481,11 +1485,6 @@ kadm5_chpass_principal_3(void *server_handle,
     /* unlock principal on this KDC */
     kdb->fail_auth_count = 0;
 
-    /* key data and attributes changed, let the database provider know */
-    kdb->mask = KADM5_KEY_DATA | KADM5_ATTRIBUTES |
-        KADM5_FAIL_AUTH_COUNT;
-    /* | KADM5_CPW_FUNCTION */
-
     if (hist_added)
         kdb->mask |= KADM5_KEY_HIST;
 
@@ -1560,6 +1559,11 @@ kadm5_randkey_principal_3(void *server_handle,
     if ((ret = kdb_get_entry(handle, principal, &kdb, &adb)))
         return(ret);
 
+    /* We will always be changing the key data, attributes, auth failure count,
+     * and password expiration time. */
+    kdb->mask = KADM5_KEY_DATA | KADM5_ATTRIBUTES | KADM5_FAIL_AUTH_COUNT |
+        KADM5_PW_EXPIRATION;
+
     ret = apply_keysalt_policy(handle, adb.policy, n_ks_tuple, ks_tuple,
                                &new_n_ks_tuple, &new_ks_tuple);
     if (ret)
@@ -1599,14 +1603,10 @@ kadm5_randkey_principal_3(void *server_handle,
         if (ret)
             goto done;
     }
-    if (have_pol) {
-        if (pol.pw_max_life)
-            kdb->pw_expiration = ts_incr(now, pol.pw_max_life);
-        else
-            kdb->pw_expiration = 0;
-    } else {
-        kdb->pw_expiration = 0;
-    }
+
+    kdb->pw_expiration = 0;
+    if (have_pol && pol.pw_max_life)
+        kdb->pw_expiration = ts_incr(now, pol.pw_max_life);
 
     ret = krb5_dbe_update_last_pwd_change(handle->context, kdb, now);
     if (ret)
@@ -1624,10 +1624,6 @@ kadm5_randkey_principal_3(void *server_handle,
             goto done;
     }
 
-    /* key data changed, let the database provider know */
-    kdb->mask = KADM5_KEY_DATA | KADM5_FAIL_AUTH_COUNT;
-    /* | KADM5_RANDKEY_USED */;
-
     ret = k5_kadm5_hook_chpass(handle->context, handle->hook_handles,
                                KADM5_HOOK_STAGE_PRECOMMIT, principal, keepold,
                                new_n_ks_tuple, new_ks_tuple, NULL);
@@ -1763,6 +1759,11 @@ kadm5_setkey_principal_4(void *server_handle, krb5_principal principal,
     if (ret)
         return ret;
 
+    /* We will always be changing the key data, attributes, auth failure count,
+     * and password expiration time. */
+    kdb->mask = KADM5_KEY_DATA | KADM5_ATTRIBUTES | KADM5_FAIL_AUTH_COUNT |
+        KADM5_PW_EXPIRATION;
+
     if (kvno == 0) {
         /* Pick the next kvno. */
         for (i = 0; i < kdb->n_key_data; i++) {
@@ -1864,14 +1865,10 @@ kadm5_setkey_principal_4(void *server_handle, krb5_principal principal,
         if (ret)
             goto done;
     }
-    if (have_pol) {
-        if (pol.pw_max_life)
-            kdb->pw_expiration = ts_incr(now, pol.pw_max_life);
-        else
-            kdb->pw_expiration = 0;
-    } else {
-        kdb->pw_expiration = 0;
-    }
+
+    kdb->pw_expiration = 0;
+    if (have_pol && pol.pw_max_life)
+        kdb->pw_expiration = ts_incr(now, pol.pw_max_life);
 
     ret = krb5_dbe_update_last_pwd_change(handle->context, kdb, now);
     if (ret)
@@ -1880,9 +1877,6 @@ kadm5_setkey_principal_4(void *server_handle, krb5_principal principal,
     /* Unlock principal on this KDC. */
     kdb->fail_auth_count = 0;
 
-    /* key data changed, let the database provider know */
-    kdb->mask = KADM5_KEY_DATA | KADM5_FAIL_AUTH_COUNT;
-
     ret = kdb_put_entry(handle, kdb, &adb);
     if (ret)
         goto done;
diff --git a/src/plugins/kdb/ldap/libkdb_ldap/ldap_principal2.c b/src/plugins/kdb/ldap/libkdb_ldap/ldap_principal2.c
index 564093f..1d07267 100644
--- a/src/plugins/kdb/ldap/libkdb_ldap/ldap_principal2.c
+++ b/src/plugins/kdb/ldap/libkdb_ldap/ldap_principal2.c
@@ -1230,19 +1230,6 @@ krb5_ldap_put_principal(krb5_context context, krb5_db_entry *entry,
                 goto cleanup;
         }
 
-        if (!(entry->mask & KADM5_PRINCIPAL)) {
-            memset(strval, 0, sizeof(strval));
-            if ((strval[0]=getstringtime(entry->pw_expiration)) == NULL)
-                goto cleanup;
-            if ((st=krb5_add_str_mem_ldap_mod(&mods,
-                                              "krbpasswordexpiration",
-                                              LDAP_MOD_REPLACE, strval)) != 0) {
-                free (strval[0]);
-                goto cleanup;
-            }
-            free (strval[0]);
-        }
-
         /* Update last password change whenever a new key is set */
         {
             krb5_timestamp last_pw_changed;
diff --git a/src/tests/t_kdb.py b/src/tests/t_kdb.py
index 9fff204..03ee70f 100755
--- a/src/tests/t_kdb.py
+++ b/src/tests/t_kdb.py
@@ -495,6 +495,23 @@ else:
     realm.run([kadminl, 'modprinc', '-pwexpire', '2040-02-03', 'user'])
     realm.run([kadminl, 'getprinc', 'user'], expected_msg=' 2040\n')
 
+# Regression test for #8861 (pw_expiration policy enforcement).
+mark('pw_expiration propogation')
+# Create a policy with a max life and verify its application.
+realm.run([kadminl, 'addpol', '-maxlife', '1s', 'pw_e'])
+realm.run([kadminl, 'addprinc', '-policy', 'pw_e', '-pw', 'password',
+           'pwuser'])
+out = realm.run([kadminl, 'getprinc', 'pwuser'],
+                expected_msg='Password expiration date: ')
+if 'Password expiration date: [never]' in out:
+    fail('pw_expiration not applied at principal creation')
+# Unset the policy max life and verify its application during password
+# change.
+realm.run([kadminl, 'modpol', '-maxlife', '0', 'pw_e'])
+realm.run([kadminl, 'cpw', '-pw', 'password_', 'pwuser'])
+realm.run([kadminl, 'getprinc', 'pwuser'],
+          expected_msg='Password expiration date: [never]')
+
 realm.stop()
 
 # Briefly test dump and load.