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.
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(a)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.