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....@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@REALM) > >should be send to krb5_child and both should not be enterprise > >principals. > You are right, only "pt...@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 "pt...@sssd-ad.test" > >> > >> Output from klist -l > >> #> Principal name Cache name > >> #> -------------- ---------- > >> #> pt...@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. _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel