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.
[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.
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?
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.
[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.
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.