[SSSD] [PATCHES] Fix password change for trusted AD users

Sumit Bose sbose at redhat.com
Wed Nov 14 17:33:04 UTC 2012


On Wed, Nov 14, 2012 at 12:03:14PM -0500, Simo Sorce wrote:
> On Wed, 2012-11-14 at 17:27 +0100, Sumit Bose wrote:
> > Hi,
> > 
> > the following three patches make password changes for trusted AD users
> > work and fix https://fedorahosted.org/sssd/ticket/1615 . The most
> > important fix, disabling canonicalization for the AS-REQ for the
> > change
> > password ticket, is in the third patch. The other two fixes issues I
> > came across which might block password changes under specific
> > conditions.
> > 
> > bye,
> > Sumit
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > plain text
> > document
> > attachment
> > (0001-Just-use-the-service-name-with-krb5_get_init_creds_p.patch)
> 
> Besides the discard_const() issue this looks good

ok, alternative version attached.

bye,
Sumit

> 
> 
> > 
> > 
> > 
> > 
> > 
> > plain text
> > document
> > attachment
> > (0002-Fix-compare_principal_realm-check.patch)
> 
> this too
> 
> > 
> > 
> > 
> > 
> > 
> > plain text
> > document
> > attachment
> > (0003-Disable-canonicalization-during-password-changes.patch)
> 
> not tested but looks good too.
> 
> Simo.
> 
> -- 
> Simo Sorce * Red Hat, Inc * New York
> 
> _______________________________________________
> sssd-devel mailing list
> sssd-devel at lists.fedorahosted.org
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
-------------- next part --------------
From 651cdfdc7e5d57c10d0e50e9f02dd4e87fbb8d26 Mon Sep 17 00:00:00 2001
From: Sumit Bose <sbose at redhat.com>
Date: Wed, 14 Nov 2012 13:56:43 +0100
Subject: [PATCH 1/3] Just use the service name with
 krb5_get_init_creds_password()

Currently we add the realm name to change password principal but
according to the MIT Kerberos docs and the upstream usage the realm name
is just ignored.

Dropping the realm name also does not lead to confusion if the change
password request was received for a user of a trusted domain.
---
 src/providers/krb5/krb5_child.c |   26 ++------------------------
 1 files changed, 2 insertions(+), 24 deletions(-)

diff --git a/src/providers/krb5/krb5_child.c b/src/providers/krb5/krb5_child.c
index 8a68f27..34e8ac9 100644
--- a/src/providers/krb5/krb5_child.c
+++ b/src/providers/krb5/krb5_child.c
@@ -1053,7 +1053,6 @@ static errno_t changepw_child(int fd, struct krb5_req *kr)
     char *user_error_message = NULL;
     size_t user_resp_len;
     uint8_t *user_resp;
-    char *changepw_princ = NULL;
     krb5_prompter_fct prompter = sss_krb5_prompter;
     const char *realm_name;
     int realm_length;
@@ -1074,16 +1073,6 @@ static errno_t changepw_child(int fd, struct krb5_req *kr)
         goto sendresponse;
     }
 
-    changepw_princ = talloc_asprintf(kr, "%s@%s", SSSD_KRB5_CHANGEPW_PRINCIPAL,
-                                                  kr->krb5_ctx->realm);
-    if (changepw_princ == NULL) {
-        DEBUG(1, ("talloc_asprintf failed.\n"));
-        kerr = KRB5KRB_ERR_GENERIC;
-        goto sendresponse;
-    }
-    DEBUG(SSSDBG_FUNC_DATA,
-          ("Created a changepw principal [%s]\n", changepw_princ));
-
     if (kr->pd->cmd == SSS_PAM_CHAUTHTOK_PRELIM) {
         /* We do not need a password expiration warning here. */
         prompter = NULL;
@@ -1095,7 +1084,7 @@ static errno_t changepw_child(int fd, struct krb5_req *kr)
           ("Attempting kinit for realm [%s]\n",realm_name));
     kerr = krb5_get_init_creds_password(kr->ctx, kr->creds, kr->princ,
                                         pass_str, prompter, kr, 0,
-                                        changepw_princ,
+                                        SSSD_KRB5_CHANGEPW_PRINCIPAL,
                                         kr->options);
     if (kerr != 0) {
         pam_status = kerr_handle_error(kerr);
@@ -1202,7 +1191,6 @@ static errno_t tgt_req_child(int fd, struct krb5_req *kr)
     int ret;
     krb5_error_code kerr = 0;
     char *pass_str = NULL;
-    char *changepw_princ = NULL;
     int pam_status = PAM_SYSTEM_ERR;
 
     DEBUG(SSSDBG_TRACE_LIBS, ("Attempting to get a TGT\n"));
@@ -1222,16 +1210,6 @@ static errno_t tgt_req_child(int fd, struct krb5_req *kr)
         goto sendresponse;
     }
 
-    changepw_princ = talloc_asprintf(kr, "%s@%s", SSSD_KRB5_CHANGEPW_PRINCIPAL,
-                                                  kr->krb5_ctx->realm);
-    if (changepw_princ == NULL) {
-        DEBUG(1, ("talloc_asprintf failed.\n"));
-        kerr = KRB5KRB_ERR_GENERIC;
-        goto sendresponse;
-    }
-    DEBUG(SSSDBG_FUNC_DATA,
-          ("Created a changepw principal [%s]\n", changepw_princ));
-
     kerr = get_and_save_tgt(kr, pass_str);
 
     /* If the password is expired the KDC will always return
@@ -1249,7 +1227,7 @@ static errno_t tgt_req_child(int fd, struct krb5_req *kr)
         }
         kerr = krb5_get_init_creds_password(kr->ctx, kr->creds, kr->princ,
                                             pass_str, sss_krb5_prompter, kr, 0,
-                                            changepw_princ,
+                                            SSSD_KRB5_CHANGEPW_PRINCIPAL,
                                             kr->options);
         krb5_free_cred_contents(kr->ctx, kr->creds);
         if (kerr == 0) {
-- 
1.7.7.6

-------------- next part --------------
From 03b53bb73bb398ed4a489aaf940bec2a3b5c65ef Mon Sep 17 00:00:00 2001
From: Sumit Bose <sbose at redhat.com>
Date: Wed, 14 Nov 2012 16:29:14 +0100
Subject: [PATCH 2/3] Fix compare_principal_realm() check

In case of a short UPN compare_principal_realm() erroneously returns an
error.
---
 src/providers/krb5/krb5_common.c |   12 +++---------
 src/tests/krb5_utils-tests.c     |    6 ++++++
 2 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/src/providers/krb5/krb5_common.c b/src/providers/krb5/krb5_common.c
index ee3d725..ed2fffa 100644
--- a/src/providers/krb5/krb5_common.c
+++ b/src/providers/krb5/krb5_common.c
@@ -898,22 +898,16 @@ errno_t krb5_get_simple_upn(TALLOC_CTX *mem_ctx, struct krb5_ctx *krb5_ctx,
 errno_t compare_principal_realm(const char *upn, const char *realm,
                                 bool *different_realm)
 {
-    size_t upn_len;
-    size_t realm_len;
     char *at_sign;
 
-    if (upn == NULL || realm == NULL || different_realm == NULL) {
+    if (upn == NULL || realm == NULL || different_realm == NULL ||
+        *upn == '\0' || *realm == '\0') {
         return EINVAL;
     }
 
-    upn_len = strlen(upn);
-    realm_len = strlen(realm);
     at_sign = strchr(upn, '@');
 
-    /* if coming from the same realm the upn must be at least the size of the
-     * realm plus 1 for the '@' char. */
-    if (upn_len == 0 || realm_len == 0 || upn_len <= realm_len + 1 ||
-        at_sign == NULL) {
+    if (at_sign == NULL) {
         return EINVAL;
     }
 
diff --git a/src/tests/krb5_utils-tests.c b/src/tests/krb5_utils-tests.c
index 77dc27c..bc1890f 100644
--- a/src/tests/krb5_utils-tests.c
+++ b/src/tests/krb5_utils-tests.c
@@ -711,6 +711,12 @@ START_TEST(test_compare_principal_realm)
     fail_unless(ret == EOK, "Failure with different realm");
     fail_unless(different_realm == true, "Different realm but " \
                                           "different_realm is not true.");
+
+    ret = compare_principal_realm("user at ABC", "REALMNAMELONGERTHANUPN",
+                                 &different_realm);
+    fail_unless(ret == EOK, "Failure with long realm name.");
+    fail_unless(different_realm == true, "Realm name longer than UPN but "
+                                         "different_realm is not true.");
 }
 END_TEST
 
-- 
1.7.7.6

-------------- next part --------------
From 48f0eb5df8c6bc220b640af3b9ebf6ebd83e9180 Mon Sep 17 00:00:00 2001
From: Sumit Bose <sbose at redhat.com>
Date: Wed, 14 Nov 2012 14:56:47 +0100
Subject: [PATCH 3/3] Disable canonicalization during password changes

If canonicalization is enabled Active Directory KDCs return
'krbtgt/AD.DOMAIN' as service name instead of the expected
'kadmin/changepw' which causes a 'KDC reply did not match expectations'
error.

Additionally the forwardable and proxiable flags are disabled, the
renewable lifetime is set to 0 and the lifetime of the ticket is set to
5 minutes as recommended in https://fedorahosted.org/sssd/ticket/1405
and also done by the kpasswd utility.

Fixes: https://fedorahosted.org/sssd/ticket/1405
       https://fedorahosted.org/sssd/ticket/1615
---
 src/providers/krb5/krb5_child.c |   45 +++++++++++++++++++++++++++++++++++++-
 1 files changed, 43 insertions(+), 2 deletions(-)

diff --git a/src/providers/krb5/krb5_child.c b/src/providers/krb5/krb5_child.c
index 34e8ac9..66e22f4 100644
--- a/src/providers/krb5/krb5_child.c
+++ b/src/providers/krb5/krb5_child.c
@@ -101,6 +101,28 @@ struct krb5_req {
 static krb5_context krb5_error_ctx;
 #define KRB5_CHILD_DEBUG(level, error) KRB5_DEBUG(level, krb5_error_ctx, error)
 
+static krb5_error_code get_changepw_options(krb5_context ctx,
+                                            krb5_get_init_creds_opt **_options)
+{
+    krb5_get_init_creds_opt *options;
+    krb5_error_code kerr;
+
+    kerr = sss_krb5_get_init_creds_opt_alloc(ctx, &options);
+    if (kerr != 0) {
+        KRB5_CHILD_DEBUG(SSSDBG_CRIT_FAILURE, kerr);
+        return kerr;
+    }
+
+    sss_krb5_get_init_creds_opt_set_canonicalize(options, 0);
+    krb5_get_init_creds_opt_set_forwardable(options, 0);
+    krb5_get_init_creds_opt_set_proxiable(options, 0);
+    krb5_get_init_creds_opt_set_renew_life(options, 0);
+    krb5_get_init_creds_opt_set_tkt_life(options, 5*60);
+
+    *_options = options;
+
+    return 0;
+}
 
 static errno_t sss_send_pac(krb5_authdata **pac_authdata)
 {
@@ -1056,6 +1078,7 @@ static errno_t changepw_child(int fd, struct krb5_req *kr)
     krb5_prompter_fct prompter = sss_krb5_prompter;
     const char *realm_name;
     int realm_length;
+    krb5_get_init_creds_opt *chagepw_options;
 
     DEBUG(SSSDBG_TRACE_LIBS, ("Password change operation\n"));
 
@@ -1078,6 +1101,12 @@ static errno_t changepw_child(int fd, struct krb5_req *kr)
         prompter = NULL;
     }
 
+    kerr = get_changepw_options(kr->ctx, &chagepw_options);
+    if (kerr != 0) {
+        DEBUG(SSSDBG_OP_FAILURE, ("get_changepw_options failed.\n"));
+        goto sendresponse;
+    }
+
     sss_krb5_princ_realm(kr->ctx, kr->princ, &realm_name, &realm_length);
 
     DEBUG(SSSDBG_TRACE_FUNC,
@@ -1085,7 +1114,8 @@ static errno_t changepw_child(int fd, struct krb5_req *kr)
     kerr = krb5_get_init_creds_password(kr->ctx, kr->creds, kr->princ,
                                         pass_str, prompter, kr, 0,
                                         SSSD_KRB5_CHANGEPW_PRINCIPAL,
-                                        kr->options);
+                                        chagepw_options);
+    sss_krb5_get_init_creds_opt_free(kr->ctx, chagepw_options);
     if (kerr != 0) {
         pam_status = kerr_handle_error(kerr);
         goto sendresponse;
@@ -1192,6 +1222,7 @@ static errno_t tgt_req_child(int fd, struct krb5_req *kr)
     krb5_error_code kerr = 0;
     char *pass_str = NULL;
     int pam_status = PAM_SYSTEM_ERR;
+    krb5_get_init_creds_opt *chagepw_options;
 
     DEBUG(SSSDBG_TRACE_LIBS, ("Attempting to get a TGT\n"));
 
@@ -1225,10 +1256,20 @@ static errno_t tgt_req_child(int fd, struct krb5_req *kr)
             KRB5_CHILD_DEBUG(SSSDBG_CRIT_FAILURE, kerr);
             DEBUG(1, ("Failed to unset expire callback, continue ...\n"));
         }
+
+        kerr = get_changepw_options(kr->ctx, &chagepw_options);
+        if (kerr != 0) {
+            DEBUG(SSSDBG_OP_FAILURE, ("get_changepw_options failed.\n"));
+            goto sendresponse;
+        }
+
         kerr = krb5_get_init_creds_password(kr->ctx, kr->creds, kr->princ,
                                             pass_str, sss_krb5_prompter, kr, 0,
                                             SSSD_KRB5_CHANGEPW_PRINCIPAL,
-                                            kr->options);
+                                            chagepw_options);
+
+        sss_krb5_get_init_creds_opt_free(kr->ctx, chagepw_options);
+
         krb5_free_cred_contents(kr->ctx, kr->creds);
         if (kerr == 0) {
             kerr = KRB5KDC_ERR_KEY_EXP;
-- 
1.7.7.6



More information about the sssd-devel mailing list