On (21/06/13 20:45), Jakub Hrozek wrote:
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 removed checks in former updates. There left only mapping
from collection ccache (DIR:<cc_dir_name>) to ccache
(DIR::<cc_dir_name>/<ticket_name>)
I tried some modifications on backend side (with gdb), but I could not get an aim
1. store collection ccache name stored in sysdb
2. environment variable KRB5CCNAME also contains collection ccache name
3. do not have more ccaches with same principal name
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.
Updated patches are attached.
LS