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
