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-de...@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!
signature.asc
Description: This is a digitally signed message part
_______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://fedorahosted.org/mailman/listinfo/sssd-devel
