[SSSD] [PATCH] Every time return directory for krb5 cache collection.

Lukas Slebodnik lslebodn at redhat.com
Thu Jun 20 09:11:17 UTC 2013


On (19/06/13 12:20), Sumit Bose wrote:
>On Mon, Jun 17, 2013 at 12:02:02PM +0200, Lukas Slebodnik wrote:
>> On (12/06/13 16:05), Jakub Hrozek wrote:
>> >On Wed, Jun 05, 2013 at 09:37:28AM +0200, Lukas Slebodnik wrote:
>> >> On (02/06/13 23:14), Jakub Hrozek wrote:
>> >> >On Sat, 2013-06-01 at 11:43 +0200, Lukas Slebodnik wrote:
>> >> >> On (31/05/13 21:29), Jakub Hrozek wrote:
>> >> >> >On Wed, 2013-05-29 at 16:09 +0200, Lukas Slebodnik wrote:
>> >> >> >> On (29/05/13 08:44), Simo Sorce wrote:
>> >> >> >> >On Wed, 2013-05-29 at 11:30 +0200, Lukas Slebodnik wrote:
>> >> >> >> >> On (29/05/13 11:07), Lukas Slebodnik wrote:
>> >> >> >> >> >ehlo,
>> >> >> >> >> >
>> >> >> >> >> >Function krb5_cc_get_full_name is called only as a way to validate that,
>> >> >> >> >> >we have the right cache. Instead of returned name, location will be returned
>> >> >> >> >> >from function cc_dir_cache_for_princ.
>> >> >> >> >> >
>> >> >> >> >> >https://fedorahosted.org/sssd/ticket/1936
>> >> >> >> >> >
>> >> >> >> >> >Patch is attached.
>> >> >> >> >> >
>> >> >> >> >> >LS
>> >> >> >> >> 
>> >> >> >> >> self NACK
>> >> >> >> >> 
>> >> >> >> >> this patch store to cache DIR:/run/user/325600000/krb5cc
>> >> >> >> >>                             ^^^^
>> >> >> >> >>                      missing colon?
>> >> >> >> >
>> >> >> >> >No, this is the correct form.
>> >> >> >> I found out, that it is a correct form.
>> >> >> >> Problem was with checking ccname in function sss_krb5_cc_file_path.
>> >> >> >> 
>> >> >> >> New patches attached.
>> >> >> >> 
>> >> >> >> LS
>> >> >> >
>> >> >> >Sorry for the reply from gmail. My OTP token decided the best password
>> >> >> >for me on a Friday evening is "Err", so I can't access my redhat.com
>> >> >> >account at the moment.
>> >> >> >
>> >> >> >These patches break one assumption we want to keep -- if there is a user
>> >> >> >logged in and the same user logs in for example from another terminal,
>> >> >> >they should have the same ccache. With your patches, I'm getting a new
>> >> >> >one when I log in simultaneously.
>> >> >> >
>> >> >> >I haven't tested that, but I guess this is because path to collection is
>> >> >> >always passed to the krb5_child now. I think that in the case user is
>> >> >> >already logged in (in krb5 code we denote this with "ccache is active"),
>> >> >> >then you should pass the full path to the ccache to the krb5_child.
>> >> >> >
>> >> >> Simo wrote in ticket comment (https://fedorahosted.org/sssd/ticket/1936#comment:10)
>> >> >> 
>> >> >>    > Do we really want to store only DIR:/run/user/$uid/krb5cc/ to cache?
>> >> >>    Yes, this is exactly what we want as a ccache.
>> >> >> 
>> >> >> LS
>> >> >
>> >> >Yes, I saw that comment and I agree with Simo.
>> >> >
>> >> >But I think we should examine the ccache collection and in the case
>> >> >there already is a valid cache present there, we should reuse it just
>> >> >like we did before.
>> >> >
>> >> 
>> >> I did not realize, that new ccache is created after another login.
>> >> Thank you for review.
>> >> 
>> >> Updated patches are attached.
>> >> 
>> >> LS
>> >
>> >I'm afraid the detection works too well :-)
>> >
>> >I tested with an expired ccache:
>> >$ sudo klist /run/user/208800000/ccdir/tktGihEUV
>> >Ticket cache: FILE:/run/user/208800000/ccdir/tktGihEUV
>> >Default principal: admin at EXAMPLE.COM
>> >
>> >Valid starting       Expires              Service principal
>> >06/07/2013 14:57:16  06/08/2013 14:57:16  krbtgt/EXAMPLE.COM at EXAMPLE.COM
>> >                     ^^^^^^^^^^^^^^^^^^
>> >                     only valid until 8th of July
>> >
>> >    renew until 06/14/2013 14:57:16
>> >
>> >And SSSD assigned me this ccache. I would expect that if there already
>> >is a ccache for the user, but it is expired (there is a function in the
>> >krb5 provider that checks if ccache is valid and active), then it would
>> >be overwritten with a fresh one.
>> 
>> With new patches you will get only valid cache.
>> 
>> LS
>
>I would prefer if the checks can be done in the responder and not in the
>krb5_child. The current logic is documented and evaluated in
>krb5_auth_prepare_ccache_file(), do you think it would be possible to
>enhance krb5_auth_prepare_ccache_file() with the needed new checks?
>Additionally there is already code in the responder e.g. to check for a
>valid TGT which might help to simplify your patch.
>
>The responder then can send either
>
>DIR::/run/user/325600000/krb5cc/ to indicate that a new ccache should be
>created in the collection, and that existing ccaches in the collection
>for the principal can be removed, because they are invalid and not used
>
>or
>
>DIR::/run/user/325600000/krb5cc/tktt1yNP8 to indicated that the
>specified ccache in the collection should be reused
>
>to the krb5_child. Does this make sense?
>
>
>
>> @@ -1100,13 +1257,19 @@ static krb5_error_code get_and_save_tgt(struct krb5_req *kr,
>>  
>>      DEBUG(SSSDBG_TRACE_FUNC,
>>            ("Attempting kinit for realm [%s]\n",realm_name));
>> -    kerr = krb5_get_init_creds_password(kr->ctx, kr->creds, kr->princ,
>> -                                        discard_const(password),
>> -                                        sss_krb5_prompter, kr, 0,
>> -                                        NULL, kr->options);
>> -    if (kerr != 0) {
>> -        KRB5_CHILD_DEBUG(SSSDBG_CRIT_FAILURE, kerr);
>> -        return kerr;
>> +
>> +    have_old_cache = retrieved_valid_cred_from_cache(kr->ctx, kr->princ,
>> +                                                     realm_name, kr->ccname,
>> +                                                     &kr->creds);
>> +    if (!have_old_cache) {
>
>we always want to get a new TGT and update the existing cache to
>increase the use time of the ticket, otherwise we would be limited by
>the lifetime of the ticket and the
>use-the-screensaver-to-get-a-new-valid-ticket trick wouldn't work
>anymore.
>
At first, I wanted to use krb5_get_renewed_creds to renew credential, but it
became too complicated. Therefore I inspired in kinit code (new ccache
is created with the same name as old ccname)

>> +        kerr = krb5_get_init_creds_password(kr->ctx, kr->creds, kr->princ,
>> +                                            discard_const(password),
>> +                                            sss_krb5_prompter, kr, 0,
>> +                                            NULL, kr->options);
>> +        if (kerr != 0) {
>> +            KRB5_CHILD_DEBUG(SSSDBG_CRIT_FAILURE, kerr);
>> +            return kerr;
>> +        }
>>      }
>>  
>>      if (kr->validate) {
>
>bye,
>Sumit

Rewritten patches are attached.

LS
-------------- next part --------------
>From cfb9863fa096c49bfa57fb533a420a7006113434 Mon Sep 17 00:00:00 2001
From: Lukas Slebodnik <lslebodn at redhat.com>
Date: Wed, 29 May 2013 13:15:58 +0200
Subject: [PATCH 1/3] Fix wrong detection of krb5 ccname

DIR:/run/user/1000/krb5cc is valid ccname, but function sss_krb5_cc_file_path
returned NULL in this case.
---
 src/util/sss_krb5.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/src/util/sss_krb5.c b/src/util/sss_krb5.c
index 74db98fe9ee4cba858de5b459f0a5540003c63f8..9adb8ff2647fd04a86abef3ef58c015ba3dd21b2 100644
--- a/src/util/sss_krb5.c
+++ b/src/util/sss_krb5.c
@@ -996,7 +996,10 @@ sss_krb5_cc_file_path(const char *full_location)
 #ifdef HAVE_KRB5_DIRCACHE
         case SSS_KRB5_TYPE_DIR:
             /* DIR::/run/user/tkt_foo */
-            if (residual[0] == ':') return residual+1;
+            if (residual[0] == ':') {
+                ++residual;
+            }
+            return residual;
 #endif
         case SSS_KRB5_TYPE_UNKNOWN:
             break;
-- 
1.8.1.4

-------------- next part --------------
>From 7455717ba4fb10c35ec99fabfbc4910af8f5dadb Mon Sep 17 00:00:00 2001
From: Lukas Slebodnik <lslebodn at redhat.com>
Date: Wed, 29 May 2013 09:57:38 +0200
Subject: [PATCH 2/3] Every time return directory for krb5 cache collection.

Function krb5_cc_get_full_name is called only as a way to validate that,
we have the right cache. Instead of returned name, location will be returned
from function cc_dir_cache_for_princ.

https://fedorahosted.org/sssd/ticket/1936
---
 src/providers/krb5/krb5_child.c | 58 ++++++++++++++++++++++++++++++++++++++++-
 src/providers/krb5/krb5_utils.c |  5 +++-
 2 files changed, 61 insertions(+), 2 deletions(-)

diff --git a/src/providers/krb5/krb5_child.c b/src/providers/krb5/krb5_child.c
index e9fbfbeafe774bc82f8be4976cc1b5a67a6acc5b..f0ad6fe9e9befbc22f78bd82888d9100a62c4584 100644
--- a/src/providers/krb5/krb5_child.c
+++ b/src/providers/krb5/krb5_child.c
@@ -1080,12 +1080,57 @@ done:
 
 }
 
+static char * get_ccache_name_by_principal(TALLOC_CTX *mem_ctx,
+                                           krb5_context ctx,
+                                           krb5_principal principal,
+                                           const char *ccname)
+{
+    krb5_error_code kerr;
+    krb5_ccache tmp_cc = NULL;
+    char *tmp_ccname = NULL;
+    char *ret_ccname = NULL;
+
+    kerr = krb5_cc_set_default_name(ctx, ccname);
+    if (kerr != 0) {
+        KRB5_CHILD_DEBUG(SSSDBG_MINOR_FAILURE, kerr);
+        return NULL;
+    }
+
+    kerr = krb5_cc_cache_match(ctx, principal, &tmp_cc);
+    if (kerr != 0) {
+        KRB5_CHILD_DEBUG(SSSDBG_TRACE_INTERNAL, kerr);
+        return NULL;
+    }
+
+    kerr = krb5_cc_get_full_name(ctx, tmp_cc, &tmp_ccname);
+    if (kerr !=0) {
+        KRB5_CHILD_DEBUG(SSSDBG_MINOR_FAILURE, kerr);
+    }
+
+    ret_ccname = talloc_strdup(mem_ctx, tmp_ccname);
+    if (ret_ccname == NULL) {
+        DEBUG(SSSDBG_OP_FAILURE, ("talloc_strdup failed (ENOMEM).\n"));
+    }
+
+done:
+    if (tmp_cc != NULL) {
+        kerr = krb5_cc_close(ctx, tmp_cc);
+        if (kerr != 0) {
+            KRB5_CHILD_DEBUG(SSSDBG_MINOR_FAILURE, kerr);
+        }
+    }
+    krb5_free_string(ctx, tmp_ccname);
+
+    return ret_ccname;
+}
+
 static krb5_error_code get_and_save_tgt(struct krb5_req *kr,
                                         const char *password)
 {
     const char *realm_name;
     int realm_length;
     krb5_error_code kerr;
+    char *cc_name;
 
 
     kerr = sss_krb5_get_init_creds_opt_set_expire_callback(kr->ctx, kr->options,
@@ -1100,6 +1145,17 @@ static krb5_error_code get_and_save_tgt(struct krb5_req *kr,
 
     DEBUG(SSSDBG_TRACE_FUNC,
           ("Attempting kinit for realm [%s]\n",realm_name));
+
+    /* If kr->ccname is cache collection (DIR:/...), we want to work
+     * with directly with file ccache (DIR::/...), but cache collection
+     * should be returned bactk to back end.
+     */
+    cc_name = get_ccache_name_by_principal(kr->pd, kr->ctx, kr->princ,
+                                           kr->ccname);
+    if (cc_name == NULL) {
+        cc_name = kr->ccname;
+    }
+
     kerr = krb5_get_init_creds_password(kr->ctx, kr->creds, kr->princ,
                                         discard_const(password),
                                         sss_krb5_prompter, kr, 0,
@@ -1134,7 +1190,7 @@ static krb5_error_code get_and_save_tgt(struct krb5_req *kr,
     /* Use the updated principal in the creds in case canonicalized */
     kerr = create_ccache(kr->uid, kr->gid, kr->ctx,
                          kr->creds ? kr->creds->client : kr->princ,
-                         kr->ccname, kr->creds);
+                         cc_name, kr->creds);
     if (kerr != 0) {
         KRB5_CHILD_DEBUG(SSSDBG_CRIT_FAILURE, kerr);
         goto done;
diff --git a/src/providers/krb5/krb5_utils.c b/src/providers/krb5/krb5_utils.c
index 1883d785e6746c45e51c8b0fabe01afbad688d6d..3f16faa7fb238bbb9801029beed66293cd873c15 100644
--- a/src/providers/krb5/krb5_utils.c
+++ b/src/providers/krb5/krb5_utils.c
@@ -1164,6 +1164,9 @@ cc_dir_cache_for_princ(TALLOC_CTX *mem_ctx, const char *location,
         return NULL;
     }
 
+    /* This function is called only as a way to validate that,
+     * we have the right cache
+     */
     krberr = krb5_cc_get_full_name(context, ccache, &name);
     if (ccache) krb5_cc_close(context, ccache);
     krb5_free_context(context);
@@ -1173,7 +1176,7 @@ cc_dir_cache_for_princ(TALLOC_CTX *mem_ctx, const char *location,
         return NULL;
     }
 
-    return talloc_strdup(mem_ctx, name);
+    return talloc_strdup(mem_ctx, location);
 }
 
 errno_t
-- 
1.8.1.4

-------------- next part --------------
>From 370b19eab0cdb266703d5f08fb45c9eb49bfe9f7 Mon Sep 17 00:00:00 2001
From: Lukas Slebodnik <lslebodn at redhat.com>
Date: Wed, 19 Jun 2013 18:47:25 +0200
Subject: [PATCH 3/3] Do not switch to credentials everytime.

If user decide to kinit as another user we do not want to switch back
to user ccache at another login. We will switch to new ccache if and only
if default principal name is the same as current principal name, or there is
not any default ccache.

https://fedorahosted.org/sssd/ticket/1936
---
 src/providers/krb5/krb5_child.c | 70 ++++++++++++++++++++++++++++++++++++++---
 1 file changed, 66 insertions(+), 4 deletions(-)

diff --git a/src/providers/krb5/krb5_child.c b/src/providers/krb5/krb5_child.c
index f0ad6fe9e9befbc22f78bd82888d9100a62c4584..c27692ba6b0fd90ad15011184a2daede1f63220b 100644
--- a/src/providers/krb5/krb5_child.c
+++ b/src/providers/krb5/krb5_child.c
@@ -436,6 +436,66 @@ done:
     return kerr;
 }
 
+#ifdef HAVE_KRB5_DIRCACHE
+static bool need_switch_to_principal(krb5_context ctx, krb5_principal princ)
+{
+    krb5_error_code kerr;
+    krb5_ccache default_cc = NULL;
+    krb5_principal default_princ = NULL;
+    char *default_full_name = NULL;
+    char *full_name = NULL;
+    bool ret = false;
+
+    kerr = krb5_cc_default(ctx, &default_cc);
+    if (kerr !=0) {
+        KRB5_CHILD_DEBUG(SSSDBG_TRACE_INTERNAL, kerr);
+        goto done;
+    }
+
+    kerr = krb5_cc_get_principal(ctx, default_cc, &default_princ);
+    if (kerr == KRB5_FCC_NOFILE) {
+        /* There is not any default cache. */
+        ret = true;
+        goto done;
+    } else if (kerr != 0) {
+        KRB5_CHILD_DEBUG(SSSDBG_TRACE_INTERNAL, kerr);
+        goto done;
+    }
+
+    kerr = krb5_unparse_name(ctx, default_princ, &default_full_name);
+    if (kerr !=0) {
+        KRB5_CHILD_DEBUG(SSSDBG_TRACE_INTERNAL, kerr);
+        goto done;
+    }
+
+    kerr = krb5_unparse_name(ctx, princ, &full_name);
+    if (kerr !=0) {
+        KRB5_CHILD_DEBUG(SSSDBG_TRACE_INTERNAL, kerr);
+        goto done;
+    }
+
+    if (0 == strcmp(default_full_name, full_name)) {
+        ret = true;
+    }
+
+done:
+    if (default_cc != NULL) {
+        kerr = krb5_cc_close(ctx, default_cc);
+        if (kerr != 0) {
+            KRB5_CHILD_DEBUG(SSSDBG_OP_FAILURE, kerr);
+            goto done;
+        }
+    }
+
+    /* all functions can be safely called with NULL. */
+    krb5_free_principal(ctx, default_princ);
+    krb5_free_unparsed_name(ctx, default_full_name);
+    krb5_free_unparsed_name(ctx, full_name);
+
+    return ret;
+}
+#endif /* HAVE_KRB5_DIRCACHE */
+
 static krb5_error_code
 store_creds_in_ccache(krb5_context ctx, krb5_principal princ,
                       krb5_ccache cc, krb5_creds *creds)
@@ -466,10 +526,12 @@ store_creds_in_ccache(krb5_context ctx, krb5_principal princ,
     }
 
 #ifdef HAVE_KRB5_DIRCACHE
-    kerr = krb5_cc_switch(ctx, cc);
-    if (kerr != 0) {
-        KRB5_CHILD_DEBUG(SSSDBG_OP_FAILURE, kerr);
-        goto done;
+    if (need_switch_to_principal(ctx, princ)) {
+        kerr = krb5_cc_switch(ctx, cc);
+        if (kerr != 0) {
+            KRB5_CHILD_DEBUG(SSSDBG_OP_FAILURE, kerr);
+            goto done;
+        }
     }
 #endif /* HAVE_KRB5_DIRCACHE */
 
-- 
1.8.1.4



More information about the sssd-devel mailing list