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

Jakub Hrozek jhrozek at redhat.com
Wed Jun 26 16:55:46 UTC 2013


On Wed, Jun 26, 2013 at 02:54:19PM +0200, Lukas Slebodnik wrote:
> On (26/06/13 10:56), Sumit Bose wrote:
> >On Wed, Jun 26, 2013 at 10:23:59AM +0200, Lukas Slebodnik wrote:
> >> On (25/06/13 11:40), Jakub Hrozek wrote:
> >> >On Mon, Jun 24, 2013 at 10:54:30PM +0200, Lukas Slebodnik wrote:
> >> >> On (24/06/13 22:06), Jakub Hrozek wrote:
> >> >> >On Sat, Jun 22, 2013 at 01:55:51PM +0200, Lukas Slebodnik wrote:
> >> >> >> On (21/06/13 20:45), Jakub Hrozek wrote:
> >> >> >> >On Thu, Jun 20, 2013 at 11:11:17AM +0200, Lukas Slebodnik wrote:
> >> >> >> >> Rewritten patches are attached.
> >> >> >> >> 
> >> >> >> >> LS
> >> >> >> >
> >> >> >> >Two nitpicks:
> >> >> >> >
> >> >> >> >> +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:
> >> >> >> >
> >> >> >> >This done label is unused and GCC emits a warning over that. It should
> >> >> >> >be used if krb5_cc_get_full_name() fails.
> >> >> >> >
> >> >> >> >> +    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;
> >> >> >> >> +}
> >> >> >> >> +
> >> >> >> >
> >> >> >> >
> >> >> >> >> 
> >> >> >> >> +    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;
> >> >> >> >> +    }
> >> >> >> >> +
> >> >> >> >
> >> >> >> >If you're changing the other patch, can you also add DEBUG message here
> >> >> >> >saying what has been compared?
> >> >> >> >
> >> >> >> >> +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;
> >> >> >> >> +}
> >> >> >> >
> >> >> >> >So far I ran a number of tests against IPA. I should still check AD
> >> >> >> >provider especially with enterprise principals to see if they still
> >> >> >> >work. If they do I'll ack.
> >> >> >> >
> >> >> >> >But I will also file a ticket for 1.10.1 to move the checks to the
> >> >> >> >backend so they are all done on one place as Sumit suggested earlier.
> >> >> >> >
> >> >> >> I removed checks in former updates. There left only mapping
> >> >> >> from collection ccache (DIR:<cc_dir_name>) to ccache
> >> >> >> (DIR::<cc_dir_name>/<ticket_name>)
> >> >> >> 
> >> >> >> I tried some modifications on backend side (with gdb), but I could not get an aim
> >> >> >> 1. store collection ccache name stored in sysdb
> >> >> >> 2. environment variable KRB5CCNAME also contains collection ccache name
> >> >> >> 3. do not have more ccaches with same principal name
> >> >> >> 
> >> >> >> >I don't think it's necessary to delay 1.10.0 any longer for this issue,
> >> >> >> >but it's the right thing to do eventually.
> >> >> >> 
> >> >> >> Updated patches are attached.
> >> >> >> 
> >> >> >> LS
> >> >> >
> >> >> >I still see two issues, sorry:
> >> >> >
> >> >> >1) The cc_residual_is_used check seems to be broken:
> >> >> >
> >> >> >[sssd[be[AD.EXAMPLE.COM]]] [cc_residual_is_used] (0x1000): User [1983201109]
> >> >> >is still active, reusing ccache [/run/user/1983201109].
> >> >> >[sssd[be[AD.EXAMPLE.COM]]] [cc_residual_is_used] (0x0200): Cache file
> >> >> >[/run/user/1983201109/primary] does not exist, it will be recreated
> >> >> >     ^^^^^^^^^^^^^^^^^
> >> >> >  This path cannot exist, I think SSSD should be checking for
> >> >> >  /run/user/1983201109/krb5cc/primary instead
> >> >> >
> >> >> >[sssd[be[AD.EXAMPLE.COM]]] [check_for_valid_tgt] (0x0020): krb5_cc_retrieve_cred failed.
> >> >> >[sssd[be[AD.EXAMPLE.COM]]] [fo_resolve_service_send] (0x0100): Trying to resolve service 'AD'
> >> >> >
> >> >> >2) When using enterprise credentials, multiple logins always get a new
> >> >> >ccache. I don't see this happening with origin/master, sorry.
> >> >> I thought, that it is aim of cache collection to have separate ccache for
> >> >> different logins.
> >> >
> >> >Yes, but these were two separate logins of the same principal. I've
> >> >demonstrated the issue to Lukas in a VM to make sure we both see the
> >> >problem.
> >> 
> >> I would like to describe situation, because I am not really sure, where is the
> >> problem.
> >> 
> >> String "ptest\\@FOO.BAR at SSSD-AD.TEST" is sent to krb5_child with enterprise
> >
> >An enterprise principal should never be send to the krb5_child. (There
> >was an issue with AD, because AD expected the canonicalization flag
> >explicitly set even for enterprise principals, which lead to enterprise
> >principals stored in SSSD's cache, but this is already fixed) Either the
> >principal from the SSSD cache or a constructed one (username at REALM)
> >should be send to krb5_child and both should not be enterprise
> >principals.
> You are right, only "ptest at FOO.BAR" is sent to krb5_child and
> stored in kr->upn. Enterprise principal is generated in k5c_setup and stored
> in kr->name and this is the string, which I meant.
> Not, it is clear to me.
> 
> LS
> >
> >The principal is then expanded to an enterprise principal in krb5_child
> >if configured.
> >
> >> principal. We have empty cache collection. New ccache is created (after login).
> >> This new ccache has principal name "ptest at SSSD-AD.TEST"
> >> 
> >> Output from klist -l
> >> #> Principal name                 Cache name
> >> #> --------------                 ----------
> >> #> ptest at SSSD-AD.TEST             DIR::/run/user/947005188/krb5cc/tktceNIlD
> >> 
> >
> >The principal form the cache (the canonicalized principal) is returned
> >to the krb5 provider together with the ccache path and should be saved
> >to the cache.
> >
> >> User decides to log on machine second time. Attribute ccacheFile has value
> >> "DIR:/run/user/947005188/krb5cc/", therefore we need find ccache in collection.
> >> With my patches, sssd uses function krb5_cc_cache_match to find ccache by
> >> principal. krb5_cc_cache_match internally uses krb5_principal_compare_flags to
> >> compare two principals and here is the problem
> >
> >The second login should pick the principal from SSSD's cache which
> >should be the one from above and hence the one from the ccache file. If
> >configured the principal is expanded to an enterprise principal quite
> >early in krb5_child. So you either make the comparison before the
> >expansion, save the original principal in the context of the krb5_child
> >and use it for the comparison or if possible do the comparison already
> >in the krb5 provider.
> >
> >HTH
> >
> >bye,
> >Sumit

With this iteration of the patches, all my testcases passed. I only see
a typo in one of the comments that will be OK to fix before pushing.

ACK.

Great work, thank you for not giving up on the issue.



More information about the sssd-devel mailing list