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

Sumit Bose sbose at redhat.com
Tue Aug 26 12:21:00 UTC 2014


On Tue, Aug 26, 2014 at 01:45:11PM +0200, Jakub Hrozek wrote:
> 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.

ah, sorry, I didn't realized that the hint option is unused. I agree
that you original approach is better. But please free the temporary
strings.

bye,
Sumit
> _______________________________________________
> sssd-devel mailing list
> sssd-devel at lists.fedorahosted.org
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel



More information about the sssd-devel mailing list