From a14ed22b6cfcc3b594c36462b0d07f2940ec882d Mon Sep 17 00:00:00 2001 From: Hans Zandbelt Date: Fri, 4 Oct 2024 07:44:54 +0200 Subject: [PATCH] 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 --- ChangeLog | 9 ++++ auth_openidc.conf | 6 +++ configure.ac | 2 +- src/cfg/cfg.c | 7 +++ src/cfg/cfg.h | 1 + src/cfg/cmds.c | 5 ++ src/cfg/dir.c | 11 +---- src/cfg/provider.c | 28 +++++++++++ src/cfg/provider.h | 9 ++++ src/metadata.c | 11 ++++- src/proto/id_token.c | 115 +++++++++++++++++++++++++++++++------------ src/util.c | 21 ++++++++ src/util.h | 2 + 13 files changed, 185 insertions(+), 42 deletions(-) diff --git a/ChangeLog b/ChangeLog index 4ccab73e..b2f5c47a 100644 --- 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 index 45080b84..be016e7f 100644 --- 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 + + # 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 index 4cc57922..72573ea7 100644 --- a/configure.ac +++ b/configure.ac @@ -1,4 +1,4 @@ -AC_INIT([mod_auth_openidc],[2.4.16.4],[hans.zandbelt@openidc.com]) +AC_INIT([mod_auth_openidc],[2.4.16.5rc0],[hans.zandbelt@openidc.com]) AC_SUBST(NAMEVER, AC_PACKAGE_TARNAME()-AC_PACKAGE_VERSION()) diff --git a/src/cfg/cfg.c b/src/cfg/cfg.c index de30a584..02288b14 100644 --- 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 index 3ed0eec9..776b3670 100644 --- 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 index e6c32783..ac093a7e 100644 --- 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 index f40e9044..4db9ca0b 100644 --- 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 index 9f445c16..7802c12f 100644 --- 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 index fe5653ba..79db348f 100644 --- 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 index 9a686221..709fe689 100644 --- 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 index dc00159b..ad4a3dce 100644 --- a/src/proto/id_token.c +++ b/src/proto/id_token.c @@ -96,6 +96,8 @@ 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 */ @@ -103,6 +105,10 @@ apr_byte_t oidc_proto_idtoken_validate_aud_and_azp(request_rec *r, oidc_cfg_t *c 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 index 04ced975..1375821e 100644 --- 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 index 5d45c45f..69500ffb 100644 --- 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);