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.