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.
_______________________________________________
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
