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.
> [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.
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
> [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?
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.
> [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.
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().
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.
>
> [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
Also, I'm going to open a bug to have pam_systemd.so create a
subdirectory for us, so we're not polluting the /run/user/username
directory with cryptically-named files.