On Mon, Feb 23, 2015 at 05:54:59PM +0100, Pavel Reichl wrote:
>
> On 02/23/2015 02:54 PM, Jakub Hrozek wrote:
> >On Mon, Feb 23, 2015 at 01:51:36PM +0100, Pavel Reichl wrote:
> >>Your patch adds at 3 places another empty line. Is there a reason for that?
> >>I suppose it's just a typo...
> >Yes, it's a copy-n-paste error, thanks for catching that. Attached is a
> >new patch.
> >
> >btw please note that every KRB5_DEBUG call also means there's a
> >sss_log() call that emits to syslog, I hope that's acceptable as well
> >since those errors are fatal..
> >
> >
> >_______________________________________________
> >sssd-devel mailing list
> >sssd-devel@lists.fedorahosted.org
> >https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
> I am Sorry I didn't notice it before but adding KRB5_CHILD_DEBUG() might be
> duplicit as It's called every time the create_ccache() call fails - as you
> can see at attached snippet.
>
> >static krb5_error_code get_and_save_tgt_with_keytab(krb5_context ctx,
> >--
> > }
> >
> > /* Use the updated principal in the creds in case canonicalized */
> > kerr = create_ccache(ccname, &creds);
> > if (kerr != 0) {
> > KRB5_CHILD_DEBUG(SSSDBG_CRIT_FAILURE, kerr);
> Here.
> > goto done;
> > }
> > kerr = 0;
> >
> >done:
> > krb5_free_cred_contents(ctx, &creds);
> >
> > return kerr;
> >--
> >static krb5_error_code get_and_save_tgt(struct krb5_req *kr,
> >--
> > }
> >
> > /* Use the updated principal in the creds in case canonicalized */
> > kerr = create_ccache(cc_name, kr->creds);
> > if (kerr != 0) {
> > KRB5_CHILD_DEBUG(SSSDBG_CRIT_FAILURE, kerr);
> Here.
> > goto done;
> > }
> >
> > /* Successfull authentication! Check if ccache contains the
> > * right principal...
> > */
> > kerr = sss_krb5_check_ccache_princ(kr->ctx, kr->ccname,
> >kr->creds->client);
> > if (kerr) {
> >--
> >static errno_t create_empty_ccache(struct krb5_req *kr)
> >--
> > DEBUG(SSSDBG_TRACE_LIBS, "Creating empty ccache\n");
> > kerr = create_empty_cred(kr->ctx, kr->princ, &creds);
> > if (kerr == 0) {
> > kerr = create_ccache(kr->ccname, creds);
> > }
> > } else {
> > DEBUG(SSSDBG_TRACE_LIBS, "Existing ccache still valid,
> >reusing\n");
> > kerr = 0;
> > }
> >
> > if (kerr != 0) {
> > KRB5_CHILD_DEBUG(SSSDBG_CRIT_FAILURE, kerr);
> Here.
> > } else {
> > kerr = k5c_attach_ccname_msg(kr);
> >(END)
>
> If I may propose solution I would remove KRB5_CHILD_DEBUG() from the code
> pasted above and I would move KRB5_CHILD_DEBUG(
I did this.
> ) in create_ccache() after
> 'done:' to save pasting it explicitly into 6 if statement.
>
I didn't do this, because KRB5_CHILD_DEBUG output contains the line
number and moving the statement to DEBUG would make it useless.
> > kerr = handle_randomized(ccname);
> > if (kerr != 0) {
> > DEBUG(SSSDBG_CRIT_FAILURE,
> > "Cannot create randomized ccache name: %d\n", kerr);
> > goto done;
> > }
> The only if statement that doesn't contain in KRB5_CHILD_DEBUG() can return
> directly, which would be IMO good thing as it returns errno value but
> function create_ccache() is supposed to return krb5_error_code, so I think
> that explicit conversion would be nice.
I explicitly print the error value in handle_randomized now, but I don't
think we need to do any converstion, it's perfectly safe to mix krb5
error codes and errno code, they have a different base.
>From 57d71ec270286c1eb5e284d866b1aa04bbd8b772 Mon Sep 17 00:00:00 2001
From: Jakub Hrozek <jhro...@redhat.com>
Date: Tue, 11 Nov 2014 14:04:30 +0100
Subject: [PATCH] KRB5: More debugging for create_ccache()
It was difficult to find where the problem was without advanced
techniques like strace.
---
src/providers/krb5/krb5_child.c | 53 +++++++++++++++++++++++++++++++----------
1 file changed, 40 insertions(+), 13 deletions(-)
diff --git a/src/providers/krb5/krb5_child.c b/src/providers/krb5/krb5_child.c
index
8b3f10d8244f483e6f99a79b01964b0018fa3ee4..10b4e8c948ff9a394910a0f7b7006950963e7ec3
100644
--- a/src/providers/krb5/krb5_child.c
+++ b/src/providers/krb5/krb5_child.c
@@ -556,7 +556,8 @@ static errno_t handle_randomized(char *in)
umask(old_umask);
if (fd == -1) {
ret = errno;
- DEBUG(SSSDBG_CRIT_FAILURE, "mkstemp(\"%s\") failed!\n", ccname);
+ DEBUG(SSSDBG_CRIT_FAILURE,
+ "mkstemp(\"%s\") failed: %d!\n", ccname, ret);
return ret;
}
}
@@ -592,43 +593,73 @@ static krb5_error_code create_ccache(char *ccname,
krb5_creds *creds)
}
kerr = handle_randomized(ccname);
- if (kerr) goto done;
+ if (kerr) {
+ DEBUG(SSSDBG_CRIT_FAILURE, "handle_randomized failed: %d\n", kerr);
+ goto done;
+ }
kerr = krb5_cc_resolve(kctx, ccname, &kcc);
- if (kerr) goto done;
+ if (kerr) {
+ KRB5_CHILD_DEBUG(SSSDBG_CRIT_FAILURE, kerr);
+ goto done;
+ }
type = krb5_cc_get_type(kctx, kcc);
DEBUG(SSSDBG_TRACE_ALL, "Initializing ccache of type [%s]\n", type);
#ifdef HAVE_KRB5_CC_COLLECTION
if (krb5_cc_support_switch(kctx, type)) {
+ DEBUG(SSSDBG_TRACE_ALL, "CC supports switch\n");
kerr = krb5_cc_set_default_name(kctx, ccname);
- if (kerr) goto done;
+ if (kerr) {
+ DEBUG(SSSDBG_TRACE_ALL, "Cannot set default name!\n");
+ KRB5_CHILD_DEBUG(SSSDBG_CRIT_FAILURE, kerr);
+ goto done;
+ }
kerr = krb5_cc_cache_match(kctx, creds->client, &cckcc);
if (kerr == KRB5_CC_NOTFOUND) {
+ DEBUG(SSSDBG_TRACE_ALL, "Match not found\n");
kerr = krb5_cc_new_unique(kctx, type, NULL, &cckcc);
switch_to_cc = true;
}
- if (kerr) goto done;
+ if (kerr) {
+ DEBUG(SSSDBG_TRACE_ALL, "krb5_cc_cache_match failed\n");
+ KRB5_CHILD_DEBUG(SSSDBG_CRIT_FAILURE, kerr);
+ goto done;
+ }
krb5_cc_close(kctx, kcc);
kcc = cckcc;
}
#endif
kerr = krb5_cc_initialize(kctx, kcc, creds->client);
- if (kerr) goto done;
+ if (kerr) {
+ DEBUG(SSSDBG_TRACE_ALL, "krb5_cc_initialize failed\n");
+ KRB5_CHILD_DEBUG(SSSDBG_CRIT_FAILURE, kerr);
+ goto done;
+ }
kerr = krb5_cc_store_cred(kctx, kcc, creds);
- if (kerr) goto done;
+ if (kerr) {
+ DEBUG(SSSDBG_TRACE_ALL, "krb5_cc_store_cred failed");
+ KRB5_CHILD_DEBUG(SSSDBG_CRIT_FAILURE, kerr);
+ goto done;
+ }
#ifdef HAVE_KRB5_CC_COLLECTION
if (switch_to_cc) {
+ DEBUG(SSSDBG_TRACE_ALL, "switch_to_cc");
kerr = krb5_cc_switch(kctx, kcc);
- if (kerr) goto done;
+ if (kerr) {
+ DEBUG(SSSDBG_TRACE_ALL, "krb5_cc_switch\n");
+ KRB5_CHILD_DEBUG(SSSDBG_CRIT_FAILURE, kerr);
+ goto done;
+ }
}
#endif
+ DEBUG(SSSDBG_TRACE_ALL, "returning: %d\n", kerr);
done:
if (kcc) {
/* FIXME: should we krb5_cc_destroy in case of error ? */
@@ -955,7 +986,6 @@ static krb5_error_code
get_and_save_tgt_with_keytab(krb5_context ctx,
/* Use the updated principal in the creds in case canonicalized */
kerr = create_ccache(ccname, &creds);
if (kerr != 0) {
- KRB5_CHILD_DEBUG(SSSDBG_CRIT_FAILURE, kerr);
goto done;
}
kerr = 0;
@@ -1026,7 +1056,6 @@ static krb5_error_code get_and_save_tgt(struct krb5_req
*kr,
/* Use the updated principal in the creds in case canonicalized */
kerr = create_ccache(cc_name, kr->creds);
if (kerr != 0) {
- KRB5_CHILD_DEBUG(SSSDBG_CRIT_FAILURE, kerr);
goto done;
}
@@ -1490,9 +1519,7 @@ static errno_t create_empty_ccache(struct krb5_req *kr)
kerr = 0;
}
- if (kerr != 0) {
- KRB5_CHILD_DEBUG(SSSDBG_CRIT_FAILURE, kerr);
- } else {
+ if (kerr == 0) {
kerr = k5c_attach_ccname_msg(kr);
}
--
2.1.0
_______________________________________________
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
