[SSSD] [PATCH] Support DIR: credential caches

Stephen Gallagher sgallagh at redhat.com
Wed Jun 13 16:18:21 UTC 2012


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.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 198 bytes
Desc: This is a digitally signed message part
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20120613/6c182b8c/attachment.sig>


More information about the sssd-devel mailing list