[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