ensure backwards compatibility for multi-value ID token "aud" validation

ensure backwards compatibility with versions <2.4.16.x when a JSON array of string values is provided in the "aud" claim of the ID token; required by (at least) Oracle IDCS see #1272 and #1273; thanks @lufik and @tydalforce add OIDCIDTokenAudValues configuration primitive that allows for explicit (and exhaustive) configuration of the list of accepted values in the "aud" claim of the ID token e.g. as required for passing FAPI 2 conformance testing bump to 2.4.16.5rc0 Signed-off-by: Hans Zandbelt <[email protected]>
OpenIDC · Oct 4, 2024 · a14ed22 · a14ed22
1 parent 38bbedf
commit a14ed22
Show file tree

Hide file tree

Showing 13 changed files with 185 additions and 42 deletions.
diff --git a/ChangeLog b/ChangeLog
@@ -1,3 +1,12 @@
+10/04/2024
+- ensure backwards compatibility with versions <2.4.16.x when a JSON array of string values
+  is provided in the "aud" claim of the ID token; required by (at least) Oracle IDCS
+  see #1272 and #1273; thanks @lufik and @tydalforce
+- add OIDCIDTokenAudValues configuration primitive that allows for explicit (and exhaustive)
+  configuration of the list of accepted values in the "aud" claim of the ID token
+  e.g. as required for passing FAPI 2 conformance testing
+- bump to 2.4.16.5rc0
+
 09/29/2024
 - release 2.4.16.4
 

diff --git a/auth_openidc.conf b/auth_openidc.conf
@@ -347,6 +347,12 @@
 # NB: this can be overridden on a per-OP basis in the .conf file using the key: id_token_encrypted_response_enc
 #OIDCIDTokenEncryptedResponseEnc [A128CBC-HS256|A256CBC-HS512|A256GCM]
 
+# The accepted value(s) of the "aud" claim in the ID token, restricted to only those values that have been defined here.
+# The convienience value "@" can be used to refer to the configured client id (i.e. in case of dynamic client registration).
+# When not defined the default is to accept any list of values (or a single string value) that includes value of OIDCClientID.
+# NB: this can be overridden on a per-OP basis in the .conf file using the key: id_token_aud_values with the value set to a JSON array of strings.
+#OIDCIDTokenAudValues <value>+
+
 # The algorithm that the OP should use to sign the UserInfo response
 # When not defined the default (by spec) is that the OP does not sign the response.
 # (ES??? algorithms only supported when using OpenSSL >= 1.0)

diff --git a/configure.ac b/configure.ac
@@ -1,4 +1,4 @@
-AC_INIT([mod_auth_openidc],[2.4.16.4],[[email protected]])
+AC_INIT([mod_auth_openidc],[2.4.16.5rc0],[[email protected]])
 
 AC_SUBST(NAMEVER, AC_PACKAGE_TARNAME()-AC_PACKAGE_VERSION())
 

diff --git a/src/cfg/cfg.c b/src/cfg/cfg.c
@@ -52,6 +52,13 @@
 #include "session.h"
 #include "util.h"
 
+const char *oidc_cfg_string_list_add(apr_pool_t *pool, apr_array_header_t **list, const char *arg) {
+	if (*list == NULL)
+		*list = apr_array_make(pool, 1, sizeof(const char *));
+	APR_ARRAY_PUSH(*list, const char *) = arg;
+	return NULL;
+}
+
 #define OIDC_DEFAULT_ACTION_ON_USERINFO_ERROR OIDC_ON_ERROR_502
 OIDC_CFG_MEMBER_FUNC_TYPE_GET(action_on_userinfo_error, oidc_on_error_action_t, OIDC_DEFAULT_ACTION_ON_USERINFO_ERROR)
 

diff --git a/src/cfg/cfg.h b/src/cfg/cfg.h
@@ -186,6 +186,7 @@ void *oidc_cfg_server_merge(apr_pool_t *pool, void *BASE, void *ADD);
 int oidc_cfg_post_config(oidc_cfg_t *cfg, server_rec *s);
 void oidc_cfg_child_init(apr_pool_t *pool, oidc_cfg_t *cfg, server_rec *s);
 void oidc_cfg_cleanup_child(oidc_cfg_t *cfg, server_rec *s);
+const char *oidc_cfg_string_list_add(apr_pool_t *pool, apr_array_header_t **list, const char *arg);
 
 #define OIDC_CFG_MEMBER_FUNC_NAME(member, type, method) oidc_##type##_##member##_##method
 

diff --git a/src/cfg/cmds.c b/src/cfg/cmds.c
@@ -465,6 +465,11 @@ const command_rec oidc_cfg_cmds[] = {
 		OIDCIDTokenEncryptedResponseEnc,
 		id_token_encrypted_response_enc,
 		"The algorithm that the OP should use to encrypt to the id_token with the Content Encryption Key (used only in dynamic client registration); must be one of [A128CBC-HS256|A256CBC-HS512|A256GCM]"),
+	OIDC_CFG_CMD_PROVIDER(
+		AP_INIT_ITERATE,
+		OIDCIDTokenAudValues,
+		id_token_aud_values,
+		"Accepted \"aud\" claim values in the ID token."),
 	OIDC_CFG_CMD_PROVIDER(
 		AP_INIT_TAKE1,
 		OIDCUserInfoSignedResponseAlg,

diff --git a/src/cfg/dir.c b/src/cfg/dir.c
@@ -244,21 +244,14 @@ const char *oidc_cmd_dir_accept_oauth_token_in_set(cmd_parms *cmd, void *m, cons
 /*
  * specify cookies names to pass/strip
  */
-static const char *oidc_cfg_dir_cookie_names_set(apr_pool_t *pool, apr_array_header_t **cookie_names, const char *arg) {
-	if (*cookie_names == NULL)
-		*cookie_names = apr_array_make(pool, 3, sizeof(const char *));
-	APR_ARRAY_PUSH(*cookie_names, const char *) = arg;
-	return NULL;
-}
-
 const char *oidc_cmd_dir_strip_cookies_set(cmd_parms *cmd, void *m, const char *arg) {
 	oidc_dir_cfg_t *dir_cfg = (oidc_dir_cfg_t *)m;
-	return oidc_cfg_dir_cookie_names_set(cmd->pool, &dir_cfg->strip_cookies, arg);
+	return oidc_cfg_string_list_add(cmd->pool, &dir_cfg->strip_cookies, arg);
 }
 
 const char *oidc_cmd_dir_pass_cookies_set(cmd_parms *cmd, void *m, const char *arg) {
 	oidc_dir_cfg_t *dir_cfg = (oidc_dir_cfg_t *)m;
-	return oidc_cfg_dir_cookie_names_set(cmd->pool, &dir_cfg->pass_cookies, arg);
+	return oidc_cfg_string_list_add(cmd->pool, &dir_cfg->pass_cookies, arg);
 }
 
 #define OIDC_CFG_DIR_MEMBER_FUNC_GET(member, type, def_val, unset_val)                                                 \

diff --git a/src/cfg/provider.c b/src/cfg/provider.c
@@ -89,6 +89,7 @@ struct oidc_provider_t {
 	char *id_token_signed_response_alg;
 	char *id_token_encrypted_response_alg;
 	char *id_token_encrypted_response_enc;
+	apr_array_header_t *id_token_aud_values;
 	char *userinfo_signed_response_alg;
 	char *userinfo_encrypted_response_alg;
 	char *userinfo_encrypted_response_enc;
@@ -240,6 +241,28 @@ OIDC_PROVIDER_TYPE_MEMBER_FUNCS_PASSPHRASE(token_endpoint_tls_client_key_pwd)
 OIDC_PROVIDER_MEMBER_FUNCS_KEYS(verify_public_keys)
 OIDC_PROVIDER_MEMBER_FUNCS_KEYS(client_keys)
 
+/*
+ * string list
+ */
+
+#define OIDC_PROVIDER_MEMBER_FUNCS_STR_LIST(member)                                                                    \
+                                                                                                                       \
+	const char *oidc_cfg_provider_##member##_set_str_list(apr_pool_t *pool, oidc_provider_t *provider,             \
+							      apr_array_header_t *arg) {                               \
+		provider->member = arg;                                                                                \
+		return NULL;                                                                                           \
+	}                                                                                                              \
+                                                                                                                       \
+	const char *oidc_cfg_provider_##member##_set(apr_pool_t *pool, oidc_provider_t *provider, const char *arg) {   \
+		return oidc_cfg_string_list_add(pool, &provider->member, arg);                                         \
+	}                                                                                                              \
+	OIDC_PROVIDER_MEMBER_FUNCS_TYPE_DEF(member, const apr_array_header_t *, NULL)
+
+/*
+ * id token aud values
+ */
+OIDC_PROVIDER_MEMBER_FUNCS_STR_LIST(id_token_aud_values)
+
 /*
  * PKCE
  */
@@ -701,6 +724,8 @@ static void oidc_cfg_provider_init(oidc_provider_t *provider) {
 	provider->request_object = NULL;
 
 	provider->response_require_iss = OIDC_CONFIG_POS_INT_UNSET;
+
+	provider->id_token_aud_values = NULL;
 }
 
 void oidc_cfg_provider_merge(apr_pool_t *pool, oidc_provider_t *dst, const oidc_provider_t *base,
@@ -812,6 +837,9 @@ void oidc_cfg_provider_merge(apr_pool_t *pool, oidc_provider_t *dst, const oidc_
 
 	dst->response_require_iss = add->response_require_iss != OIDC_CONFIG_POS_INT_UNSET ? add->response_require_iss
 											   : base->response_require_iss;
+
+	dst->id_token_aud_values =
+	    add->id_token_aud_values != NULL ? add->id_token_aud_values : base->id_token_aud_values;
 }
 
 oidc_provider_t *oidc_cfg_provider_create(apr_pool_t *pool) {

diff --git a/src/cfg/provider.h b/src/cfg/provider.h
@@ -122,6 +122,7 @@ typedef struct oidc_jwks_uri_t {
 #define OIDCIDTokenSignedResponseAlg "OIDCIDTokenSignedResponseAlg"
 #define OIDCIDTokenEncryptedResponseAlg "OIDCIDTokenEncryptedResponseAlg"
 #define OIDCIDTokenEncryptedResponseEnc "OIDCIDTokenEncryptedResponseEnc"
+#define OIDCIDTokenAudValues "OIDCIDTokenAudValues"
 #define OIDCUserInfoSignedResponseAlg "OIDCUserInfoSignedResponseAlg"
 #define OIDCUserInfoEncryptedResponseAlg "OIDCUserInfoEncryptedResponseAlg"
 #define OIDCUserInfoEncryptedResponseEnc "OIDCUserInfoEncryptedResponseEnc"
@@ -178,6 +179,11 @@ typedef struct oidc_jwks_uri_t {
 	OIDC_CFG_PROVIDER_MEMBER_FUNCS_TYPE_DECL(member, type)                                                         \
 	void OIDC_CFG_MEMBER_FUNC_NAME(member, cfg_provider, int_set)(oidc_provider_t * provider, type arg);
 
+#define OIDC_CFG_PROVIDER_MEMBER_FUNCS_STR_LIST_DECL(member)                                                           \
+	OIDC_CFG_PROVIDER_MEMBER_FUNCS_TYPE_DECL(member, const apr_array_header_t *)                                   \
+	const char *OIDC_CFG_MEMBER_FUNC_NAME(member, cfg_provider, set_str_list)(apr_pool_t *, oidc_provider_t *,     \
+										  apr_array_header_t *);
+
 OIDC_CFG_PROVIDER_MEMBER_FUNCS_STR_DECL(metadata_url)
 OIDC_CFG_PROVIDER_MEMBER_FUNCS_STR_DECL(issuer)
 OIDC_CFG_PROVIDER_MEMBER_FUNCS_STR_DECL(authorization_endpoint_url);
@@ -212,6 +218,9 @@ OIDC_CFG_PROVIDER_MEMBER_FUNCS_STR_DECL(userinfo_encrypted_response_alg)
 OIDC_CFG_PROVIDER_MEMBER_FUNCS_STR_DECL(userinfo_encrypted_response_enc)
 OIDC_CFG_PROVIDER_MEMBER_FUNCS_STR_DECL(request_object)
 
+// string list
+OIDC_CFG_PROVIDER_MEMBER_FUNCS_STR_LIST_DECL(id_token_aud_values)
+
 // keys
 OIDC_CFG_PROVIDER_MEMBER_FUNCS_KEYS_DECL(verify_public_keys)
 OIDC_CFG_PROVIDER_MEMBER_FUNCS_KEYS_DECL(client_keys)

diff --git a/src/metadata.c b/src/metadata.c
@@ -85,6 +85,7 @@
 #define OIDC_METADATA_ID_TOKEN_SIGNED_RESPONSE_ALG "id_token_signed_response_alg"
 #define OIDC_METADATA_ID_TOKEN_ENCRYPTED_RESPONSE_ALG "id_token_encrypted_response_alg"
 #define OIDC_METADATA_ID_TOKEN_ENCRYPTED_RESPONSE_ENC "id_token_encrypted_response_enc"
+#define OIDC_METADATA_ID_TOKEN_AUD_VALUES "id_token_aud_values"
 #define OIDC_METADATA_USERINFO_SIGNED_RESPONSE_ALG "userinfo_signed_response_alg"
 #define OIDC_METADATA_USERINFO_ENCRYPTED_RESPONSE_ALG "userinfo_encrypted_response_alg"
 #define OIDC_METADATA_USERINFO_ENCRYPTED_RESPONSE_ENC "userinfo_encrypted_response_enc"
@@ -1228,7 +1229,7 @@ apr_byte_t oidc_metadata_conf_parse(request_rec *r, oidc_cfg_t *cfg, json_t *j_c
 	const char *rv = NULL;
 	char *value = NULL;
 	int ivalue = OIDC_CONFIG_POS_INT_UNSET;
-	apr_array_header_t *keys = NULL;
+	apr_array_header_t *keys = NULL, *auds = NULL;
 
 	oidc_util_json_object_get_string(r->pool, j_conf, OIDC_METADATA_CLIENT_JWKS_URI, &value,
 					 oidc_cfg_provider_client_jwks_uri_get(oidc_cfg_provider_get(cfg)));
@@ -1263,6 +1264,14 @@ apr_byte_t oidc_metadata_conf_parse(request_rec *r, oidc_cfg_t *cfg, json_t *j_c
 	    oidc_cfg_provider_id_token_encrypted_response_enc_get(oidc_cfg_provider_get(cfg)));
 	OIDC_METADATA_PROVIDER_SET(id_token_encrypted_response_enc, value, rv)
 
+	oidc_util_json_object_get_string_array(r->pool, j_conf, OIDC_METADATA_ID_TOKEN_AUD_VALUES, &auds,
+					       oidc_cfg_provider_id_token_aud_values_get(oidc_cfg_provider_get(cfg)));
+	if (auds != NULL) {
+		rv = oidc_cfg_provider_id_token_aud_values_set_str_list(r->pool, provider, auds);
+		if (rv != NULL)
+			oidc_error(r, "oidc_cfg_provider_aud_values_set: %s", rv);
+	}
+
 	/* get the (optional) signing & encryption settings for the userinfo response */
 	oidc_util_json_object_get_string(
 	    r->pool, j_conf, OIDC_METADATA_USERINFO_SIGNED_RESPONSE_ALG, &value,

diff --git a/src/proto/id_token.c b/src/proto/id_token.c
@@ -96,13 +96,19 @@ apr_byte_t oidc_proto_idtoken_validate_nonce(request_rec *r, oidc_cfg_t *cfg, oi
 	return TRUE;
 }
 
+#define OIDC_PROTO_IDTOKEN_AUD_CLIENT_ID_SPECIAL_VALUE "@"
+
 /*
  * validate the "aud" and "azp" claims in the id_token payload
  */
 apr_byte_t oidc_proto_idtoken_validate_aud_and_azp(request_rec *r, oidc_cfg_t *cfg, oidc_provider_t *provider,
 						   oidc_jwt_payload_t *id_token_payload) {
 
 	char *azp = NULL;
+	const char *s_aud = NULL;
+	const apr_array_header_t *arr = NULL;
+	int i = 0;
+
 	oidc_jose_get_string(r->pool, id_token_payload->value.json, OIDC_CLAIM_AZP, FALSE, &azp, NULL);
 
 	/*
@@ -122,45 +128,92 @@ apr_byte_t oidc_proto_idtoken_validate_aud_and_azp(request_rec *r, oidc_cfg_t *c
 	json_t *aud = json_object_get(id_token_payload->value.json, OIDC_CLAIM_AUD);
 	if (aud != NULL) {
 
+		arr = oidc_cfg_provider_id_token_aud_values_get(provider);
+
 		/* check if it is a single-value */
 		if (json_is_string(aud)) {
 
-			/* a single-valued audience must be equal to our client_id */
-			if (_oidc_strcmp(json_string_value(aud), oidc_cfg_provider_client_id_get(provider)) != 0) {
-				oidc_error(r,
-					   "the configured client_id (%s) did not match the \"%s\" claim value (%s) in "
-					   "the id_token",
-					   oidc_cfg_provider_client_id_get(provider), OIDC_CLAIM_AUD,
-					   json_string_value(aud));
-				return FALSE;
+			if (arr == NULL) {
+
+				/* a single-valued audience must be equal to our client_id */
+				if (_oidc_strcmp(json_string_value(aud), oidc_cfg_provider_client_id_get(provider)) !=
+				    0) {
+					oidc_error(r,
+						   "the configured client_id (%s) did not match the \"%s\" claim value "
+						   "(%s) in "
+						   "the id_token",
+						   oidc_cfg_provider_client_id_get(provider), OIDC_CLAIM_AUD,
+						   json_string_value(aud));
+					return FALSE;
+				}
+
+			} else {
+
+				for (i = 0; i < arr->nelts; i++) {
+					s_aud = APR_ARRAY_IDX(arr, i, const char *);
+					if (_oidc_strcmp(s_aud, OIDC_PROTO_IDTOKEN_AUD_CLIENT_ID_SPECIAL_VALUE) == 0)
+						s_aud = oidc_cfg_provider_client_id_get(provider);
+					if (_oidc_strcmp(json_string_value(aud), s_aud) == 0)
+						break;
+				}
+
+				if (i == arr->nelts) {
+					oidc_error(
+					    r, "none of our configured audience values could be found in \"%s\" claim",
+					    OIDC_CLAIM_AUD);
+					return FALSE;
+				}
 			}
 
 			/* check if this is a multi-valued audience */
 		} else if (json_is_array(aud)) {
 
-			if ((json_array_size(aud) > 1) && (azp == NULL)) {
-				oidc_warn(r,
-					  "the \"%s\" claim value in the id_token is an array with more than 1 "
-					  "element, but \"%s\" claim is not present (a SHOULD in the spec...)",
-					  OIDC_CLAIM_AUD, OIDC_CLAIM_AZP);
-			}
-
-			if (oidc_util_json_array_has_value(r, aud, oidc_cfg_provider_client_id_get(provider)) ==
-			    FALSE) {
-				oidc_error(r,
-					   "our configured client_id (%s) could not be found in the array of values "
-					   "for \"%s\" claim",
-					   oidc_cfg_provider_client_id_get(provider), OIDC_CLAIM_AUD);
-				return FALSE;
-			}
-
-			if (json_array_size(aud) > 1) {
-				oidc_error(
-				    r,
-				    "our configured client_id (%s) was found in the array of values "
-				    "for \"%s\" claim, but there are other unknown/untrusted values included as well",
-				    oidc_cfg_provider_client_id_get(provider), OIDC_CLAIM_AUD);
-				return FALSE;
+			if (arr == NULL) {
+
+				if ((json_array_size(aud) > 1) && (azp == NULL)) {
+					oidc_warn(r,
+						  "the \"%s\" claim value in the id_token is an array with more than 1 "
+						  "element, but \"%s\" claim is not present (a SHOULD in the spec...)",
+						  OIDC_CLAIM_AUD, OIDC_CLAIM_AZP);
+				}
+
+				if (oidc_util_json_array_has_value(r, aud, oidc_cfg_provider_client_id_get(provider)) ==
+				    FALSE) {
+					oidc_error(
+					    r,
+					    "our configured client_id (%s) could not be found in the array of values "
+					    "for \"%s\" claim",
+					    oidc_cfg_provider_client_id_get(provider), OIDC_CLAIM_AUD);
+					return FALSE;
+				}
+
+			} else {
+
+				/* handle explicit and exhaustive configuration of acceptable audience values */
+
+				for (i = 0; i < arr->nelts; i++) {
+					s_aud = APR_ARRAY_IDX(arr, i, const char *);
+					if (_oidc_strcmp(s_aud, OIDC_PROTO_IDTOKEN_AUD_CLIENT_ID_SPECIAL_VALUE) == 0)
+						s_aud = oidc_cfg_provider_client_id_get(provider);
+					if (oidc_util_json_array_has_value(r, aud, s_aud) == FALSE) {
+						oidc_error(r,
+							   "our configured audience value (%s) could not be found in "
+							   "the array of values "
+							   "for \"%s\" claim",
+							   APR_ARRAY_IDX(arr, i, const char *), OIDC_CLAIM_AUD);
+						return FALSE;
+					}
+				}
+
+				if (json_array_size(aud) > arr->nelts) {
+					oidc_error(
+					    r,
+					    "our configured audience values are all present in the array of values "
+					    "for \"%s\" claim, but there are other unknown/untrusted values included "
+					    "as well",
+					    OIDC_CLAIM_AUD);
+					return FALSE;
+				}
 			}
 
 		} else {

diff --git a/src/util.c b/src/util.c
@@ -1703,6 +1703,27 @@ apr_byte_t oidc_util_json_object_get_string(apr_pool_t *pool, json_t *json, cons
 	return TRUE;
 }
 
+/*
+ * get (optional) string array from a JSON object
+ */
+apr_byte_t oidc_util_json_object_get_string_array(apr_pool_t *pool, json_t *json, const char *name,
+						  apr_array_header_t **value, const apr_array_header_t *default_value) {
+	json_t *v = NULL, *arr = NULL;
+	size_t i = 0;
+	*value = (default_value != NULL) ? apr_array_copy(pool, default_value) : NULL;
+	if (json != NULL) {
+		arr = json_object_get(json, name);
+		if ((arr != NULL) && (json_is_array(arr))) {
+			*value = apr_array_make(pool, json_array_size(arr), sizeof(const char *));
+			for (i = 0; i < json_array_size(arr); i++) {
+				v = json_array_get(arr, i);
+				APR_ARRAY_PUSH(*value, const char *) = apr_pstrdup(pool, json_string_value(v));
+			}
+		}
+	}
+	return TRUE;
+}
+
 /*
  * get (optional) int from a JSON object
  */

diff --git a/src/util.h b/src/util.h
@@ -92,6 +92,8 @@ apr_byte_t oidc_util_spaced_string_equals(apr_pool_t *pool, const char *a, const
 apr_byte_t oidc_util_spaced_string_contains(apr_pool_t *pool, const char *str, const char *match);
 apr_byte_t oidc_util_json_object_get_string(apr_pool_t *pool, json_t *json, const char *name, char **value,
 					    const char *default_value);
+apr_byte_t oidc_util_json_object_get_string_array(apr_pool_t *pool, json_t *json, const char *name,
+						  apr_array_header_t **value, const apr_array_header_t *default_value);
 apr_byte_t oidc_util_json_object_get_int(const json_t *json, const char *name, int *value, const int default_value);
 apr_byte_t oidc_util_json_object_get_bool(const json_t *json, const char *name, int *value, const int default_value);
 char *oidc_util_html_escape(apr_pool_t *pool, const char *input);