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

Sumit Bose sbose at redhat.com
Tue Aug 26 10:06:53 UTC 2014


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

> From 58347c89c9d2ee788e0f6ccf6becc44537423d0e Mon Sep 17 00:00:00 2001
> From: Jakub Hrozek <jhrozek at redhat.com>
> Date: Tue, 26 Aug 2014 09:43:09 +0200
> Subject: [PATCH] LDAP: Use randomized ccname for storing credentials
> 
> https://fedorahosted.org/sssd/ticket/2410
> 
> If two ldap_child processes attempt to prime the ccache at the same time
> for the same domain, the ldap_child might fail with:
>     [ldap_child_get_tgt_sync] (0x0040): Failed to init ccache: Internal credentials cache error
>     [main] (0x0020): ldap_child_get_tgt_sync failed.
> 
> To avoid the race-condition, the ldap_child process now creates the
> ccache randomized and before returning to the caller, renames the
> randomized ccache to a permanent one.
> ---
>  src/providers/ldap/ldap_child.c | 42 ++++++++++++++++++++++++++++++++++++-----
>  1 file changed, 37 insertions(+), 5 deletions(-)
> 
> diff --git a/src/providers/ldap/ldap_child.c b/src/providers/ldap/ldap_child.c
> index e2fe3f232b3f051bf1d94617c2c711b7087130c7..677d61792b867cf489598f67fb8a61e0bace1cfa 100644
> --- 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.

>  
>      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?

> +    ccname_file = talloc_asprintf(memctx, "%s/ccache_%s",
> +                                  DB_PATH, realm_name);
> +    if (ccname_file_dummy == NULL || ccname_file == NULL) {
> +        ret = ENOMEM;
>          goto done;
>      }
> -    DEBUG(SSSDBG_TRACE_INTERNAL, "keytab ccname: [%s]\n", ccname);
>  
> -    krberr = krb5_cc_resolve(context, ccname, &ccache);
> +    fd = mkstemp(ccname_file_dummy);
> +    if (fd == -1) {
> +        ret = errno;
> +        goto done;
> +    }
> +    /* We only care about creating a unique file name here, we don't
> +     * need the fd
> +     */
> +    close(fd);
> +
> +    ccname_dummy = talloc_asprintf(memctx, "FILE:%s", ccname_file_dummy);
> +    ccname = talloc_asprintf(memctx, "FILE:%s", ccname_file);
> +    if (ccname_dummy == NULL || ccname == NULL) {
> +        krberr = ENOMEM;
> +        goto done;
> +    }
> +    DEBUG(SSSDBG_TRACE_INTERNAL, "keytab ccname: [%s]\n", ccname_dummy);
> +
> +    krberr = krb5_cc_resolve(context, ccname_dummy, &ccache);
>      if (krberr) {
>          DEBUG(SSSDBG_OP_FAILURE, "Failed to set cache name: %s\n",
>                    sss_krb5_get_error_message(context, krberr));
> @@ -361,6 +385,14 @@ static krb5_error_code ldap_child_get_tgt_sync(TALLOC_CTX *memctx,
>      kdc_time_offset = 0;
>  #endif
>  
> +    ret = rename(ccname_file_dummy, ccname_file);
> +    if (ret == -1) {
> +        ret = errno;
> +        DEBUG(SSSDBG_CRIT_FAILURE,
> +              "rename failed [%d][%s].\n", ret, strerror(ret));
> +        goto done;
> +    }

I think adding a corresponding SSSDBG_TRACE_INTERNAL message saying that
ccname_dummy is now ccname_file would be useful to follow what
ldap_child is doing.

> +
>      krberr = 0;
>      *ccname_out = ccname;
>      *expire_time_out = my_creds.times.endtime - kdc_time_offset;
> -- 
> 1.9.3
> 

> _______________________________________________
> 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