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.