[SSSD] [PATCH] Support DIR: credential caches

Stephen Gallagher sgallagh at redhat.com
Thu Jun 14 19:49:38 UTC 2012


On Thu, 2012-06-14 at 21:22 +0200, Jakub Hrozek wrote:
> On Thu, Jun 14, 2012 at 07:07:55PM +0200, Jakub Hrozek wrote:
> > On Wed, Jun 13, 2012 at 03:48:50PM -0400, Stephen Gallagher wrote:
> > > On Wed, 2012-06-13 at 20:32 +0200, Jakub Hrozek wrote:
> > > > I'm going to combine the replies into one (thanks for sending them
> > > > separately, though).
> > > > 
> > > > The unit tests were fixed.
> > > > 
> > > > > > [PATCH 01/11] Two small krb5_child fixes
> > > > > > ssia
> > > > > >
> > > > > 
> > > > > Nack. You only fixed the errno overwrite in one of the two debug
> > > > > messages you changed.
> > > > 
> > > > Actually, one of three :-) Fixed.
> > > 
> > > Ack.
> > > 
> > > > 
> > > > > > [PATCH 02/11] Provide more debugging in krb5_child and ldap_child
> > > > > > I started this patch before Nick did, maybe it would still be useful, at
> > > > > > least the parts that get rid of level-9 DEBUG messages.
> > > > > >
> > > > > 
> > > > > Nack.
> > > > > 
> > > > > Can we change the debug log message "Created ccache file:" to just
> > > > > "Created ccache:"? Since we're now printing the whole CCNAME, complete
> > > > > with type.
> > > > 
> > > > It's only printed in create_ccache_file() which only created FILE:
> > > > ccaches.
> > > > 
> > > 
> > > 
> > > In that case, Ack.
> > > 
> > > > > 
> > > > > Speling misteak (it was there before, but we should fix it while we're
> > > > > touching the code:
> > > > > "successfull" should be "successful" in changepw_child()
> > > > 
> > > > Done
> > > > 
> > > 
> > > Ack
> > > 
> > > > > > [PATCH 03/11] Allow redefining the KRB5_CHILD path
> > > > > > The krb5-child-test will want to run the child from the build directory.
> > > > > >
> > > > > 
> > > > > Ack
> > > > > 
> > > > > > [PATCH 04/11] Split parse_krb5_child_response so it can be reused
> > > > > > krb5-child-test will be another consumer. It also makes the code more
> > > > > > readable by splitting a huge function.
> > > > > >
> > > > > 
> > > > > Ack
> > > > > 
> > > > > > [PATCH 05/11] Add a krb5_child test tool
> > > > > > https://fedorahosted.org/sssd/ticket/1127
> > > > > >
> > > > > 
> > > > > Nack
> > > > > 
> > > > > Typo in help: secods -> seconds.
> > > > 
> > > > Fixed
> > > > 
> > > > > 
> > > > > Typo in DEBUG: unkown -> unknown
> > > > 
> > > > Sorry, I don't see that?
> > > > 
> > > 
> > > I can't find it anymore either. My eyes must be playing tricks on you.
> > > 
> > > 
> > > > > On Wed, 2012-06-13 at 15:48 +0200, Jakub Hrozek wrote:
> > > > > ...
> > > > > > [PATCH 06/11] Residual util functions
> > > > > > Kerberos credential caches can be specified by TYPE:RESIDUAL. This pa
> > > > > > tch adds a couple of utilities to support parsing if ccache locations,
> > > > > > checking types etc.
> > > > > > 
> > > > > 
> > > > > Ack
> > > > > 
> > > > > > [PATCH 07/11] Handle trailing slash in the ccname template
> > > > > > With the DIR cache support, it's perfectly legal to specify a ccname
> > > > > > directory that ends with a slash. The create_dir function did not han dle
> > > > > > that situation correctly. The unit test is included in the DIR: cache
> > > > > > patch, because it uses the cc_dir_create() function.
> > > > > > 
> > > > > 
> > > > > Nack
> > > > > 
> > > > > Please add comments explaining that you're walking back from the end and
> > > > > removing any trailing slashes.
> > > > > 
> > > > 
> > > > Done.
> > > > 
> > > 
> > > Nack
> > > 
> > > Your comment needs work:
> > > we only pass /some/path to find_ccdir_parent_data, not /some/path
> > > 
> > > That looks like the same thing to me :)
> > > 
> > > > > 
> > > > > > [PATCH 08/11] Add a credential cache back end structure
> > > > > > To be able to add support for new credential cache types easily, this
> > > > > > patch creates a new structure sss_krb5_cc_be that defines common with
> > > > > > a credential cache, such as create, check if used or remove.
> > > > > > 
> > > > > 
> > > > > Nack.
> > > > > 
> > > > > Why did you add a tmp_ctx to safe_remove_old_ccache_file()? You don't
> > > > > allocate anything atop it.
> > > > > 
> > > > 
> > > > I used to, then I changed safe_remove_old_ccache_file() and forgot to
> > > > remove the context. Fixed.
> > > > 
> > > 
> > > Ack
> > > 
> > > > > Otherwise I think this looks sane. It's a good approach to modularizing
> > > > > the approaches.
> > > > > 
> > > > > > [PATCH 09/11] Add support for storing credential caches in the DIR: back end
> > > > > > https://fedorahosted.org/sssd/ticket/974
> > > > > > 
> > > > > > Please note that only the TGTs acquired by the krb5_child have changed,
> > > > > > the ldap_child still puts its ccache into /var/lib/sss/db.
> > > > > > 
> > > > > 
> > > > > Yes, this is as it should be. We're unlikely to need multiple TGTs for
> > > > > GSSAPI.
> > > > > 
> > > > > > The cc_dir_remove() function is a little odd, I tried to use the krb5
> > > > > > API directly, but I think I found a bug in libkrb5. For a subsidiary
> > > > > > cache that does not exist (DIR::/no/such/path), the following code would
> > > > > > segfault:
> > > > > > 
> > > > > >     krb5_cc_resolve(context, location, &ccache); // returns EOK
> > > > > >     krberr = krb5_cc_destroy(context, ccache);   // KRB5_FCC_NOFILE
> > > > > >     if (krberr) {
> > > > > >         if (ccache) krb5_cc_close(context, ccache); // SIGSEGV
> > > > > >     }
> > > > > 
> > > > > Maybe I'm confused, but shouldn't you be closing the cache before
> > > > > destroying it?
> > > > > 
> > > > 
> > > > It was my impression that krb5_cc_destroy() does both destroy and
> > > > close and the krb5_ccache "object" is invalid after krb5_cc_close().
> > > > 
> > > 
> > > Ok, Ack.
> > > 
> > > > > 
> > > > > In general, I don't see anything obviously wrong with this code, so I'm
> > > > > probably okay with committing it for the beta and fixing any issues as
> > > > > they come up, provided that the testing I'm doing continues to meet my
> > > > > expectations.
> > > > > 
> > > > > I'll continue to test (and retest) until the requested revisions are
> > > > > complete.
> > > > > 
> > > > 
> > > > There's one thing I forgot that I'm going to write a separate patch for
> > > > - we should probably limit the substitutions used in the templates in
> > > > case DIR is used. I think that if DIR is used, the last template must be
> > > > "%d", the file-specific substitutions make no sense if DIR is used.
> > > > 
> > > 
> > > Simo, Jakub and I had a long discussion on #sssd about how to handle
> > > this. The end result is that we're going to maintain the existing
> > > expansions (regardless of their usefulness).
> > > 
> > > We also decided that our plan would be to attempt to mkdir() the
> > > requested patch and fail if we could not do so. We would also fail if
> > > the path currently exists but has the wrong ownership and/or
> > > permissions.
> > > 
> > > 
> > > > > > 
> > > > > > [PATCH 10/11] Use Kerberos context in KRB5_DEBUG
> > > > > > Passing Kerberos context to sss_krb5_get_error_message will allow us to
> > > > > > get better error messages. This patch technically belong earlier, but
> > > > > > rebasing would be hard at this point.
> > > > > > 
> > > > > 
> > > > > Ack
> > > > > 
> > > > > > [PATCH 11/11] Switch Kerberos cache default to DIR
> > > > > > Just switches the defaults.
> > > > > 
> > > > > Nack
> > > > > 
> > > > > Instead of hard-coding the defaults, can we make this into a configure
> > > > > flag? Not all distributions are yet using systemd, so on those platforms
> > > > > we'll still want to default to /tmp.
> > > > > 
> > > > 
> > > > Done, patch #11 is a different one.
> > > > 
> > > > New patches are attached
> > > 
> > > Nack
> > > 
> > > The specfile needs to be conditionalized to only set /run/user by
> > > default on F17 and higher. This will break our nightlies for F16, RHEL 5
> > > and RHEL 6.
> > > 
> > 
> > Done
> > 
> > > Building sssd-krb5.5 appears to be broken when building from a parallel
> > > build directory:
> > > 
> > > po4a --option doctype=docbook --package-name sssd-docs --variable
> > > builddir=/home/sgallagh/workspace/sssd/x86_64/src/man --package-version
> > > 1.8.92 --msgid-bugs-address sssd-devel at redhat.com --copyright-holder
> > > "Red Hat" --no-backups po/po4a.cfg
> > > po/po4a.cfg:13: The 'sssd-krb5.5.xml' master file does not exist.
> > 
> > OK, I switched po4a.cfg to using absolute paths and sssd-krb5.xml is now
> > referenced from builddir because it's generated.
> > 
> > New patches are attached.
> 
> As discussed on IRC, I reverted the changes that include the
> configure-time default. We will include those as part of
> https://fedorahosted.org/sssd/ticket/1378 because doing it right proved
> to be tricky and out of scope.


Patches compile and behave as expected.

Ack!
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 198 bytes
Desc: This is a digitally signed message part
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20120614/db2b1117/attachment.sig>


More information about the sssd-devel mailing list