[SSSD] [PATCH] https://fedorahosted.org/sssd/ticket/2410

Jakub Hrozek jhrozek at redhat.com
Tue Aug 26 11:45:11 UTC 2014


On Tue, Aug 26, 2014 at 12:06:53PM +0200, Sumit Bose wrote:
> On Tue, Aug 26, 2014 at 10:46:59AM +0200, Jakub Hrozek wrote:
> > On Fri, Aug 22, 2014 at 05:24:46PM +0200, Jakub Hrozek wrote:
> > > On Fri, Aug 22, 2014 at 05:21:11PM +0200, Pavel Březina wrote:
> > > > 2) the ccache name is stored in KRB5CCACHE (or something like that)
> > > > environment variable so actually only one ccache may be used. Possible
> > > > solution would be to create two files and than keep only one of them.
> > > 
> > > If I understood Sumit on the call yesterday, his proposal was to create
> > > a randomized name, but then rename it to a predictable name. Would that
> > > solve the problem?
> > 
> > Hi,
> > 
> > in my testing (using a test that our QE colleagues kindly provided), the
> > attached patch solves the problem as well and in my opinion is a bit
> > more lightweight.
> 
> I like your approach better and do not expect any issues since rename()
> is expected to be atomic.
> 
> Please find comments below.
> 
> bye,
> Sumit

[..]

> > --- a/src/providers/ldap/ldap_child.c
> > +++ b/src/providers/ldap/ldap_child.c
> > @@ -168,7 +168,9 @@ static krb5_error_code ldap_child_get_tgt_sync(TALLOC_CTX *memctx,
> >                                                 const char **ccname_out,
> >                                                 time_t *expire_time_out)
> >  {
> > +    int fd;
> >      char *ccname;
> > +    char *ccname_dummy;
> >      char *realm_name = NULL;
> >      char *full_princ = NULL;
> >      char *default_realm = NULL;
> > @@ -184,6 +186,8 @@ static krb5_error_code ldap_child_get_tgt_sync(TALLOC_CTX *memctx,
> >      int canonicalize = 0;
> >      int kdc_time_offset_usec;
> >      int ret;
> > +    char *ccname_file_dummy;
> > +    char *ccname_file;
> 
> You are leaking ccname_dummy, ccname_file_dummy and ccname_file. Since
> the LDAO child is a short running process it might be ok but I would
> recommend to free() them to not distract tools like valgrind when
> searching for other memory leaks.

OK, I was relying on owner of mem_ctx to just free the context, but I
can add the free() calls, no problems.

> 
> >  
> >      krberr = krb5_init_context(&context);
> >      if (krberr) {
> > @@ -283,14 +287,34 @@ static krb5_error_code ldap_child_get_tgt_sync(TALLOC_CTX *memctx,
> >          goto done;
> >      }
> >  
> > -    ccname = talloc_asprintf(memctx, "FILE:%s/ccache_%s", DB_PATH, realm_name);
> > -    if (!ccname) {
> > -        krberr = KRB5KRB_ERR_GENERIC;
> > +    ccname_file_dummy = talloc_asprintf(memctx, "%s/ccache_%s_XXXXXX",
> > +                                        DB_PATH, realm_name);
> 
> This is working as expected, but using krb5_cc_new_unique() and
> krb5_cc_get_name() from libkrb5 might even result in more simple code?

In theory yes, I have a patch that I tested already:
    https://fedorapeople.org/cgit/jhrozek/public_git/sssd.git/commit/?h=ldapchild&id=b692d625202d7ee512f0affb2a38c49ec02403d6o

But I'm not sure this approach will work well for us, because:
    1) The ccache is always created in /tmp when using
    krb5_cc_new_unique(). In the krb5 code it looks like
    krb5_cc_new_unique calls krb5_fcc_generate_new() which
    unconditionally uses a templatized file in /tmp. Because on many
    systems, tmp is a tmpfs, the rename errored out with error 18
    (Invalid cross-device link).

    2) Rename from one directory to another is not friedly to SELinux.

So while the code is way cleaner than handling the templates ourselves,
I think we would have a way to tell krb5_cc_new_unique() to accept a
directory to create the ccache at..or copy the file ourselves.



More information about the sssd-devel mailing list