[SSSD] [PATCHES] KRB5: Add support for KEYRING:persistent

Jakub Hrozek jhrozek at redhat.com
Tue Aug 27 15:21:34 UTC 2013


On Tue, Aug 27, 2013 at 10:47:21AM -0400, Stephen Gallagher wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
> 
> On 08/27/2013 10:45 AM, Jakub Hrozek wrote:
> > On Mon, Aug 26, 2013 at 07:10:35AM -0400, Stephen Gallagher wrote:
> >> -----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
> >> 
> >> On 08/26/2013 05:19 AM, Jakub Hrozek wrote:
> >>> On Fri, Aug 23, 2013 at 02:37:48PM -0400, Stephen Gallagher
> >>> wrote:
> >>>> -----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
> >>>> 
> >>>> Patch 0001: Adds a couple debug messages I found useful to
> >>>> figure out if a bug was in SSSD or libkrb5. Feel free to skip
> >>>> its inclusion if you feel it's unnecessary (it's at log level
> >>>> 9)
> >>> 
> >>> ACK
> >>> 
> >>>> 
> >>>> Patch 0002: KRB5: Remove unnecessary call to become_user() By
> >>>> the time that the create_ccache_in_dir() routine is called,
> >>>> we are already guaranteed to have dropped privileges. This
> >>>> has either happened because we dropped them before the exec()
> >>>> in the normal operation case or because we dropped them
> >>>> explicitly after we completed the TGT validation step if that
> >>>> or FAST is configured. This code is actually completely
> >>>> harmless, since it checks internally to see if we've already
> >>>> dropped privileges before it does so, but it's unnecessary.
> >>> 
> >>> ACK. There is one patch that I thought wouldn't be handled at 
> >>> first (validate==true && offline==true) but in that case the
> >>> child is spawned as the user already.
> >>> 
> >>>> 
> >>>> Patch 0003: KRB5: Add support for KEYRING cache type This is
> >>>> the Big One. This adds support for KEYRING types but notes in
> >>>>  sssd-krb5(5) that we only support KEYRING:persistent (this
> >>>> is because the other keyring types may break if validation or
> >>>> FAST are used). Other than that, this patch should be
> >>>> reasonably straightforward. I have tested this code against
> >>>> Simo's preliminary Kerberos patches along with David Howells'
> >>>> kernel patch for persistent keyrings. We hit (and fixed) a
> >>>> couple bugs in krb5. This patch should be safe to put into
> >>>> the upstream master immediately; it does not change any
> >>>> behavior with the existing FILE or DIR cache types.
> >>> 
> >>> Where can I get the dependencies to test these patches? So far
> >>> I only read the code and it looks OK to me. One question:
> >>> 
> >> 
> >> I'm attaching a repo file to point at Simo's experimental
> >> repository. It requires an updated kernel, keyutils and krb5
> >> packages.
> >> 
> >> 
> >>>> +errno_t +cc_keyring_check_existing(const char *location,
> >>>> uid_t uid, +                          const char *realm,
> >>>> const char *princ, +                          const char
> >>>> *cc_template, bool *_active, +                          bool
> >>>> *_valid) +{ + errno_t ret; +    bool active; +    bool valid;
> >>>> +    const char *residual; + +    residual = 
> >>>> sss_krb5_residual_check_type(location,
> >>>> SSS_KRB5_TYPE_KEYRING); + if (!residual) { +
> >>>> DEBUG(SSSDBG_CRIT_FAILURE, + ("%s is not of type KEYRING:\n",
> >>>> location)); +        return EINVAL; +    } + +    /* The
> >>>> keyring cache is always active */ + active = true; + +    /*
> >>>> Check if any user is actively using this cache */
> >>> 
> >>> The comment sounds like you got the two (active and valid)
> >>> mixed up.
> >>> 
> >>> check_cc_validity checks if the ccache contains a credential
> >>> that is not expired yet, not that a user is actively using it.
> >>> 
> >> 
> >> Are you saying I go the comments wrong or the functionality? I'm
> >> not clear here.
> > 
> > Just the comment (now that I actually tested the code).
> > 
> > I tested that: * logins work * parallel logins reuse the same
> > ccache and extend it (with the right krb5 package) * new ccache is
> > correctly generated when the old one goes away (I only tested with
> > kdestroy, I didn't remove the key from the keyring per se) *
> > renewals work
> > 
> > So ACK to the code. I can either remove the comment before pushing
> > or resend it with the comment removed.
> > 
> 
> Thanks very much for testing, Jakub. Feel free to remove the comment
> before pushing. Also, while you're in there, would you mind adding
> that it resolves https://fedorahosted.org/sssd/ticket/2036 to the
> commit message as well?

Pushed to master and sssd-1-10



More information about the sssd-devel mailing list