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