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=ldapc...
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.