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(a)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 "ptest(a)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(a)SSSD-AD.TEST"
>>
>> Output from klist -l
>> #> Principal name Cache name
>> #> -------------- ----------
>> #> ptest(a)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.