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.
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
>
> By default, krb5_principal_compare_flags is called with flags argument 0.
> In this case following principals are compared
>
> principal->data principal->realm
> ------------------------------
> ptest SSSD-AD.TEST //Principal obtained from existing ccache
> pt...@foo.bar SSSD-AD.TEST //Principal created by parsing string
>
> I tried to call krb5_principal_compare_flags with argument
> flags = KRB5_PRINCIPAL_COMPARE_ENTERPRISE and in this case following
> principals
> were compared.
>
> principal->data principal->realm
> ------------------------------
> ptest SSSD-AD.TEST //Principal obtained from existing ccache
> ptest FOO.BAR //Principal created by parsing string
>
>
> This is a reason, why ccache can't be found in collection and
> new ccache is created every time.
>
> LS
> _______________________________________________
> sssd-devel mailing list
> sssd-devel@lists.fedorahosted.org
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
_______________________________________________
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
