Skip to content

Commit

Permalink
ensure backwards compatibility for multi-value ID token "aud" validation
Browse files Browse the repository at this point in the history
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 <hans.zandbelt@openidc.com>
  • Loading branch information
zandbelt committed Oct 4, 2024
1 parent 38bbedf commit a14ed22
Show file tree
Hide file tree
Showing 13 changed files with 185 additions and 42 deletions.
9 changes: 9 additions & 0 deletions ChangeLog
Original file line number Diff line number Diff line change
@@ -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

Expand Down
6 changes: 6 additions & 0 deletions auth_openidc.conf
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
2 changes: 1 addition & 1 deletion configure.ac
Original file line number Diff line number Diff line change
@@ -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())

Expand Down
7 changes: 7 additions & 0 deletions src/cfg/cfg.c
Original file line number Diff line number Diff line change
Expand Up @@ -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)

Expand Down
1 change: 1 addition & 0 deletions src/cfg/cfg.h
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
5 changes: 5 additions & 0 deletions src/cfg/cmds.c
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
11 changes: 2 additions & 9 deletions src/cfg/dir.c
Original file line number Diff line number Diff line change
Expand Up @@ -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) \
Expand Down
28 changes: 28 additions & 0 deletions src/cfg/provider.c
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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
*/
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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) {
Expand Down
9 changes: 9 additions & 0 deletions src/cfg/provider.h
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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)
Expand Down
11 changes: 10 additions & 1 deletion src/metadata.c
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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)));
Expand Down Expand Up @@ -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,
Expand Down
115 changes: 84 additions & 31 deletions src/proto/id_token.c
Original file line number Diff line number Diff line change
Expand Up @@ -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);

/*
Expand All @@ -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 {
Expand Down
21 changes: 21 additions & 0 deletions src/util.c
Original file line number Diff line number Diff line change
Expand Up @@ -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
*/
Expand Down
2 changes: 2 additions & 0 deletions src/util.h
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down

0 comments on commit a14ed22

Please sign in to comment.