Make gss_store_cred*() work

krb5_cc_cache_match() searches all ccache collections for a ccache that
has credentials for a given principal name.  This includes MEMORY
ccaches, which means it can find the same ccache as is referenced by a
GSS cred handle given to gss_store_cred(), which means that
gss_store_cred() can fail.

For now we work around this by including a private variant of
krb5_cc_cache_match() that only searches the default ccache, not all
collections.  Eventually we should ensure that krb5_cc_default() also
searches all collection-type (other than MEMORY) ccaches for a default
credential, then we can go back to using krb5_cc_cache_match() (though
we'll need to make sure that MEMORY is searched last or not at all).

lib/gssapi/krb5/store_cred.c

@@ -33,6 +33,72 @@
 
 #include "gsskrb5_locl.h"
 
+static int
+same_princ(krb5_context context, krb5_ccache id1, krb5_ccache id2)
+{
+    krb5_error_code ret;
+    krb5_principal p1 = NULL;
+    krb5_principal p2 = NULL;
+    int same = 0;
+
+    ret = krb5_cc_get_principal(context, id1, &p1);
+    if (ret == 0)
+        ret = krb5_cc_get_principal(context, id2, &p2);
+    if (ret == 0)
+        same = krb5_principal_compare(context, p1, p2);
+    krb5_free_principal(context, p1);
+    krb5_free_principal(context, p2);
+    return same;
+}
+
+/*
+ * Like krb5_cc_cache_match(), but only looking in the default collection.
+ *
+ * We need this to avoid looking for MEMORY ccaches, which risks matching the
+ * same credential that we're storing.  We could make sure that MEMORY ccaches
+ * are searched for last in krb5_cc_cache_match(), then ignore any MEMORY
+ * ccaches we find there, but, if we might then store in a ccache that will not
+ * be found later as the default ccache, then it's not worth it.
+ *
+ * XXX In order to remove this, we'll first need to make sure that
+ *     krb5_cc_default() searches all collections when KRB5CCNAME is not set,
+ *     then we'll need to make sure that krb5_cc_cache_match() searches MEMORY
+ *     ccaches last (or else introduce a new ccache type like MEMORY but which
+ *     is never searched or searchable), then make sure that the caller below
+ *     treat finding a MEMORY the same as not finding a ccache at all.
+ */
+static krb5_error_code
+ccache_match(krb5_context context,
+             krb5_principal princ,
+             const char *cctype,
+             krb5_ccache *id)
+{
+    krb5_cc_cache_cursor cursor = NULL;
+    krb5_error_code ret;
+
+    *id = NULL;
+    ret = krb5_cc_cache_get_first(context, cctype, &cursor);
+    if (ret)
+        return ret;
+
+    while (krb5_cc_cache_next(context, cursor, id) == 0) {
+        krb5_principal p = NULL;
+
+        ret = krb5_cc_get_principal(context, *id, &p);
+        if (ret == 0 &&
+            krb5_principal_compare(context, princ, p)) {
+            krb5_free_principal(context, p);
+            krb5_cc_cache_end_seq_get(context, cursor);
+            return 0;
+        }
+        if (*id)
+            krb5_cc_close(context, *id);
+        *id = NULL;
+    }
+    krb5_cc_cache_end_seq_get(context, cursor);
+    return KRB5_CC_END;
+}
+
 OM_uint32 GSSAPI_CALLCONV
 _gsskrb5_store_cred_into(OM_uint32         *minor_status,
 			 gss_const_cred_id_t input_cred_handle,
@@ -46,9 +112,8 @@ _gsskrb5_store_cred_into(OM_uint32         *minor_status,
 {
     krb5_context context;
     krb5_error_code ret;
-    gsskrb5_cred cred;
+    gsskrb5_cred input_cred;
     krb5_ccache id = NULL;
-    const char *def_type = NULL;
     time_t exp_current;
     time_t exp_new;
     const char *cs_ccache_name = NULL;
@@ -56,41 +121,30 @@ _gsskrb5_store_cred_into(OM_uint32         *minor_status,
 
     *minor_status = 0;
 
+    /* Sanity check inputs */
     if (cred_usage != GSS_C_INITIATE) {
+        /* It'd be nice if we could also do accept, writing a keytab */
 	*minor_status = GSS_KRB5_S_G_BAD_USAGE;
 	return GSS_S_FAILURE;
     }
-
     if (desired_mech != GSS_C_NO_OID &&
         gss_oid_equal(desired_mech, GSS_KRB5_MECHANISM) == 0)
 	return GSS_S_BAD_MECH;
+    if (input_cred_handle == GSS_C_NO_CREDENTIAL)
+	return GSS_S_CALL_INACCESSIBLE_READ;
+    input_cred = (gsskrb5_cred)input_cred_handle;
 
-    cred = (gsskrb5_cred)input_cred_handle;
+    /* Sanity check the input_cred */
-    if (cred == NULL)
+    if (input_cred->usage != cred_usage && input_cred->usage != GSS_C_BOTH) {
-	return GSS_S_NO_CRED;
-
-    GSSAPI_KRB5_INIT (&context);
-
-    HEIMDAL_MUTEX_lock(&cred->cred_id_mutex);
-    if (cred->usage != cred_usage && cred->usage != GSS_C_BOTH) {
-	HEIMDAL_MUTEX_unlock(&cred->cred_id_mutex);
 	*minor_status = GSS_KRB5_S_G_BAD_USAGE;
-	return GSS_S_FAILURE;
+	return GSS_S_NO_CRED;
     }
-
+    if (input_cred->principal == NULL) {
-    ret = krb5_cc_get_lifetime(context, cred->ccache, &exp_new);
+	*minor_status = GSS_KRB5_S_KG_TGT_MISSING;
-    if (ret) {
-	HEIMDAL_MUTEX_unlock(&cred->cred_id_mutex);
-	*minor_status = ret;
 	return GSS_S_NO_CRED;
     }
 
-    if (cred->principal == NULL) {
+    /* Extract the ccache name from the store if given */
-	HEIMDAL_MUTEX_unlock(&cred->cred_id_mutex);
-	*minor_status = GSS_KRB5_S_KG_TGT_MISSING;
-	return GSS_S_FAILURE;
-    }
-
     if (cred_store != GSS_C_NO_CRED_STORE) {
 	major_status = __gsskrb5_cred_store_find(minor_status, cred_store,
 						 "ccache", &cs_ccache_name);
@@ -98,44 +152,81 @@ _gsskrb5_store_cred_into(OM_uint32         *minor_status,
 	    *minor_status = GSS_KRB5_S_G_UNKNOWN_CRED_STORE_ELEMENT;
 	    major_status = GSS_S_NO_CRED;
 	}
-	if (GSS_ERROR(major_status)) {
+	if (GSS_ERROR(major_status))
-	    HEIMDAL_MUTEX_unlock(&cred->cred_id_mutex);
 	    return major_status;
-	}
     }
 
-    if (cs_ccache_name)
+    GSSAPI_KRB5_INIT (&context);
+    HEIMDAL_MUTEX_lock(&input_cred->cred_id_mutex);
+
+    /* More sanity checking of the input_cred (good to fail early) */
+    ret = krb5_cc_get_lifetime(context, input_cred->ccache, &exp_new);
+    if (ret) {
+	HEIMDAL_MUTEX_unlock(&input_cred->cred_id_mutex);
+	*minor_status = ret;
+	return GSS_S_NO_CRED;
+    }
+
+    if (cs_ccache_name) {
+        /*
+         * Not the default ccache.
+         *
+         * Therefore not a collection type cache.
+         *
+         * Therefore there's no question of switching the primary ccache.
+         *
+         * Therefore we reset default_cred.
+         *
+         * XXX Perhaps we should fail in this case if default_cred is true.
+         */
+        default_cred = 0;
 	ret = krb5_cc_resolve(context, cs_ccache_name, &id);
-    else {
+    } else {
-	krb5_ccache def_ccache = NULL;
+        const char *cctype = NULL;
 
-	if (krb5_cc_default(context, &def_ccache) == 0) {
+        /*
-	    def_type = krb5_cc_get_type(context, def_ccache);
+         * Use the default ccache, and if it's a collection, switch it if
-	    krb5_cc_close(context, def_ccache);
+         * default_cred is true.
-	}
+         */
+        ret = krb5_cc_default(context, &id);
+        if (ret == 0) {
+	    cctype = krb5_cc_get_type(context, id);
+            if (krb5_cc_support_switch(context, cctype)) {
+                /* The default ccache is a collection type */
 
-	/* write out cred to credential cache */
+                krb5_cc_close(context, id);
-	ret = krb5_cc_cache_match(context, cred->principal, &id);
+                id = NULL;
-	if (ret) {
+
-	    if (default_cred)
+                /* Find a matching ccache or create a new one */
-		ret = krb5_cc_default(context, &id);
+                ret = ccache_match(context, input_cred->principal,
-	    else if (def_type &&
+                                   cctype, &id);
-		     krb5_cc_support_switch(context, def_type)) {
+                if (ret || id == NULL) {
-		ret = krb5_cc_new_unique(context, def_type, NULL, &id);
+                    /* Since the ccache is new, just store unconditionally */
-		overwrite_cred = 1;
+                    overwrite_cred = 1;
-	    } else
+                    ret = krb5_cc_new_unique(context, cctype, NULL, &id);
-		ret = 0; /* == GSS_C_NO_CRED */
+                }
-	}
+            }
+        }
     }
 
     if (ret || id == NULL) {
-	HEIMDAL_MUTEX_unlock(&cred->cred_id_mutex);
+	HEIMDAL_MUTEX_unlock(&input_cred->cred_id_mutex);
 	*minor_status = ret;
 	return ret == 0 ? GSS_S_NO_CRED : GSS_S_FAILURE;
     }
 
+    /*
+     * If the new creds are for a different principal than we had before,
+     * overwrite.
+     */
+    if (!overwrite_cred && !same_princ(context, id, input_cred->ccache))
+        overwrite_cred = 1;
+
     if (!overwrite_cred) {
-        /* If current creds are expired or near it, overwrite */
+        /*
+         * If current creds are for the same princ as we already had creds for,
+         * and the new creds live longer than the old, overwrite.
+         */
         ret = krb5_cc_get_lifetime(context, id, &exp_current);
         if (ret != 0 || exp_new > exp_current)
             overwrite_cred = 1;
@@ -143,28 +234,21 @@ _gsskrb5_store_cred_into(OM_uint32         *minor_status,
 
     if (!overwrite_cred) {
         /* Nothing to do */
+        HEIMDAL_MUTEX_unlock(&input_cred->cred_id_mutex);
         krb5_cc_close(context, id);
-        HEIMDAL_MUTEX_unlock(&cred->cred_id_mutex);
         *minor_status = 0;
         return GSS_S_DUPLICATE_ELEMENT;
     }
 
-    ret = krb5_cc_initialize(context, id, cred->principal);
+    ret = krb5_cc_initialize(context, id, input_cred->principal);
     if (ret == 0)
-	ret = krb5_cc_copy_match_f(context, cred->ccache, id, NULL, NULL, NULL);
+        ret = krb5_cc_copy_match_f(context, input_cred->ccache, id, NULL, NULL,
-    if (ret) {
+                                   NULL);
-        krb5_cc_close(context, id);
+    if (ret == 0 && default_cred)
-	HEIMDAL_MUTEX_unlock(&cred->cred_id_mutex);
+        krb5_cc_switch(context, id);
-	*minor_status = ret;
+    (void) krb5_cc_close(context, id);
-	return(GSS_S_FAILURE);
-    }
 
-    if (default_cred && def_type != NULL &&
+    HEIMDAL_MUTEX_unlock(&input_cred->cred_id_mutex);
-        krb5_cc_support_switch(context, def_type))
+    *minor_status = ret;
-	krb5_cc_switch(context, id);
+    return ret ? GSS_S_FAILURE : GSS_S_COMPLETE;
-
-    krb5_cc_close(context, id);
-    HEIMDAL_MUTEX_unlock(&cred->cred_id_mutex);
-    *minor_status = 0;
-    return GSS_S_COMPLETE;
 }

lib/gssapi/mech/gss_store_cred.c

@@ -33,6 +33,7 @@
 
 #include "mech_locl.h"
 
+/* See RFC5588 */
 GSSAPI_LIB_FUNCTION OM_uint32 GSSAPI_LIB_CALL
 gss_store_cred(OM_uint32         *minor_status,
 	       gss_cred_id_t     input_cred_handle,

lib/gssapi/mech/gss_store_cred_into.c

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2009 Kungliga Tekniska H<EFBFBD>gskolan
+ * Copyright (c) 2009 Kungliga Tekniska Högskolan
  * (Royal Institute of Technology, Stockholm, Sweden).
  * All rights reserved.
  *
@@ -61,6 +61,13 @@ store_mech_cred(OM_uint32 *minor_status,
     return major_status;
 }
 
+/*
+ * See RFC5588 for gss_store_cred().  This function is a variant that takes a
+ * const key/value hashmap-like thing that specifies a credential store in a
+ * mechanism- and implementation-specific way, though Heimdal and MIT agree on
+ * at least the following keys for the Kerberos mechanism: ccache, keytab, and
+ * client_keytab.
+ */
 GSSAPI_LIB_FUNCTION OM_uint32 GSSAPI_LIB_CALL
 gss_store_cred_into(OM_uint32 *minor_status,
 		    gss_const_cred_id_t input_cred_handle,