[SSSD] [PATCH] Every time return directory for krb5 cache collection.

Jakub Hrozek jhrozek at redhat.com
Fri Jun 21 18:45:55 UTC 2013


On Thu, Jun 20, 2013 at 11:11:17AM +0200, Lukas Slebodnik wrote:
> Rewritten patches are attached.
> 
> LS

Two nitpicks:

> +static char * get_ccache_name_by_principal(TALLOC_CTX *mem_ctx,
> +                                           krb5_context ctx,
> +                                           krb5_principal principal,
> +                                           const char *ccname)
> +{
> +    krb5_error_code kerr;
> +    krb5_ccache tmp_cc = NULL;
> +    char *tmp_ccname = NULL;
> +    char *ret_ccname = NULL;
> +
> +    kerr = krb5_cc_set_default_name(ctx, ccname);
> +    if (kerr != 0) {
> +        KRB5_CHILD_DEBUG(SSSDBG_MINOR_FAILURE, kerr);
> +        return NULL;
> +    }
> +
> +    kerr = krb5_cc_cache_match(ctx, principal, &tmp_cc);
> +    if (kerr != 0) {
> +        KRB5_CHILD_DEBUG(SSSDBG_TRACE_INTERNAL, kerr);
> +        return NULL;
> +    }
> +
> +    kerr = krb5_cc_get_full_name(ctx, tmp_cc, &tmp_ccname);
> +    if (kerr !=0) {
> +        KRB5_CHILD_DEBUG(SSSDBG_MINOR_FAILURE, kerr);
> +    }
> +
> +    ret_ccname = talloc_strdup(mem_ctx, tmp_ccname);
> +    if (ret_ccname == NULL) {
> +        DEBUG(SSSDBG_OP_FAILURE, ("talloc_strdup failed (ENOMEM).\n"));
> +    }
> +
> +done:

This done label is unused and GCC emits a warning over that. It should
be used if krb5_cc_get_full_name() fails.

> +    if (tmp_cc != NULL) {
> +        kerr = krb5_cc_close(ctx, tmp_cc);
> +        if (kerr != 0) {
> +            KRB5_CHILD_DEBUG(SSSDBG_MINOR_FAILURE, kerr);
> +        }
> +    }
> +    krb5_free_string(ctx, tmp_ccname);
> +
> +    return ret_ccname;
> +}
> +


> 
> +    kerr = krb5_unparse_name(ctx, princ, &full_name);
> +    if (kerr !=0) {
> +        KRB5_CHILD_DEBUG(SSSDBG_TRACE_INTERNAL, kerr);
> +        goto done;
> +    }
> +
> +    if (0 == strcmp(default_full_name, full_name)) {
> +        ret = true;
> +    }
> +

If you're changing the other patch, can you also add DEBUG message here
saying what has been compared?

> +done:
> +    if (default_cc != NULL) {
> +        kerr = krb5_cc_close(ctx, default_cc);
> +        if (kerr != 0) {
> +            KRB5_CHILD_DEBUG(SSSDBG_OP_FAILURE, kerr);
> +            goto done;
> +        }
> +    }
> +
> +    /* all functions can be safely called with NULL. */
> +    krb5_free_principal(ctx, default_princ);
> +    krb5_free_unparsed_name(ctx, default_full_name);
> +    krb5_free_unparsed_name(ctx, full_name);
> +
> +    return ret;
> +}

So far I ran a number of tests against IPA. I should still check AD
provider especially with enterprise principals to see if they still
work. If they do I'll ack.

But I will also file a ticket for 1.10.1 to move the checks to the
backend so they are all done on one place as Sumit suggested earlier.

I don't think it's necessary to delay 1.10.0 any longer for this issue,
but it's the right thing to do eventually.



More information about the sssd-devel mailing list