From fff609eaa889b45814d9242b06e2dd3f226b148c Mon Sep 17 00:00:00 2001 From: Hans Zandbelt Date: Fri, 1 Mar 2024 18:23:29 +0100 Subject: [PATCH] accept strings as well as integers in the "expires_in" claim from the token endpoint; fix for "expires_in" string values returned from the token endpoint that would be interpreted as 0; this fixes using OIDCRefreshAccessTokenBeforeExpiry or OIDCUserInfoRefreshInterval with (older) Azure AD configs that would result in a token refresh on every request since 2.4.15 or a 401 in 2.4.14.4 Signed-off-by: Hans Zandbelt --- ChangeLog | 6 ++++++ src/handle/info.c | 2 +- src/handle/refresh.c | 10 ++-------- src/handle/response.c | 14 +++----------- src/mod_auth_openidc.c | 13 +++++++------ src/mod_auth_openidc.h | 3 ++- src/proto.c | 39 ++++++++++++++++++++++++++++++--------- src/session.c | 13 +++++++++---- 8 files changed, 60 insertions(+), 40 deletions(-) diff --git a/ChangeLog b/ChangeLog index 6c13e16b..87296591 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,9 @@ +03/01/2024 +- accept strings as well as integers in the "expires_in" claim from the token endpoint +- fix for "expires_in" string values returned from the token endpoint that would be interpreted as 0 + this fixes using OIDCRefreshAccessTokenBeforeExpiry or OIDCUserInfoRefreshInterval with (older) + Azure AD configs that would result in a token refresh on every request since 2.4.15 or a 401 in 2.4.14.4 + 02/29/2024 - hash the cache key if it is larger than 512 bytes so large cache key entries (i.e. for JWT tokens) are no longer a problem in unencrypted SHM cache configs, i.e. the default shared memory cache setup; diff --git a/src/handle/info.c b/src/handle/info.c index f2f9971a..4ba3f235 100644 --- a/src/handle/info.c +++ b/src/handle/info.c @@ -143,7 +143,7 @@ int oidc_info_request(request_rec *r, oidc_cfg *c, oidc_session_t *session, apr_ /* include the access token expiry timestamp in the session info */ if (apr_hash_get(c->info_hook_data, OIDC_HOOK_INFO_ACCES_TOKEN_EXP, APR_HASH_KEY_STRING)) { - const char *access_token_expires = oidc_session_get_access_token_expires(r, session); + const char *access_token_expires = oidc_session_get_access_token_expires_str(r, session); if (access_token_expires != NULL) json_object_set_new(json, OIDC_HOOK_INFO_ACCES_TOKEN_EXP, json_string(access_token_expires)); } diff --git a/src/handle/refresh.c b/src/handle/refresh.c index 4d5168fc..996acc81 100644 --- a/src/handle/refresh.c +++ b/src/handle/refresh.c @@ -384,7 +384,6 @@ int oidc_refresh_token_request(request_rec *r, oidc_cfg *c, oidc_session_t *sess apr_byte_t oidc_refresh_access_token_before_expiry(request_rec *r, oidc_cfg *cfg, oidc_session_t *session, int ttl_minimum, apr_byte_t *needs_save) { - const char *s_access_token_expires = NULL; apr_time_t t_expires = -1; oidc_provider_t *provider = NULL; @@ -393,8 +392,8 @@ apr_byte_t oidc_refresh_access_token_before_expiry(request_rec *r, oidc_cfg *cfg if (ttl_minimum < 0) return TRUE; - s_access_token_expires = oidc_session_get_access_token_expires(r, session); - if (s_access_token_expires == NULL) { + t_expires = oidc_session_get_access_token_expires(r, session); + if (t_expires == -1) { oidc_debug(r, "no access token expires_in stored in the session (i.e. returned from in the " "authorization response), so cannot refresh the access token based on TTL requirement"); return FALSE; @@ -406,11 +405,6 @@ apr_byte_t oidc_refresh_access_token_before_expiry(request_rec *r, oidc_cfg *cfg return FALSE; } - if (sscanf(s_access_token_expires, "%" APR_TIME_T_FMT, &t_expires) != 1) { - oidc_error(r, "could not parse s_access_token_expires %s", s_access_token_expires); - return FALSE; - } - t_expires = apr_time_from_sec(t_expires - ttl_minimum); oidc_debug(r, "refresh needed in: %" APR_TIME_T_FMT " seconds", apr_time_sec(t_expires - apr_time_now())); diff --git a/src/handle/response.c b/src/handle/response.c index a2bb83a8..6779987f 100644 --- a/src/handle/response.c +++ b/src/handle/response.c @@ -467,16 +467,6 @@ static apr_byte_t oidc_response_flows(request_rec *r, oidc_cfg *c, oidc_proto_st return rc; } -/* - * parse the expiry for the access token - */ -static int oidc_response_parse_expires_in(request_rec *r, const char *expires_in) { - int number = _oidc_str_to_int(expires_in); - if (number <= 0) - oidc_warn(r, "could not parse \"expires_in\" value (%s) into a positive integer", expires_in); - return number; -} - /* * set the unique user identifier that will be propagated in the Apache r->user and REMOTE_USER variables */ @@ -609,7 +599,9 @@ static int oidc_response_process(request_rec *r, oidc_cfg *c, oidc_session_t *se return oidc_response_authorization_error(r, c, proto_state, "No id_token was provided.", NULL); } - int expires_in = oidc_response_parse_expires_in(r, apr_table_get(params, OIDC_PROTO_EXPIRES_IN)); + int expires_in = apr_table_get(params, OIDC_PROTO_EXPIRES_IN) + ? _oidc_str_to_int(apr_table_get(params, OIDC_PROTO_EXPIRES_IN)) + : -1; char *userinfo_jwt = NULL; /* diff --git a/src/mod_auth_openidc.c b/src/mod_auth_openidc.c index 30e07d66..b63bd918 100644 --- a/src/mod_auth_openidc.c +++ b/src/mod_auth_openidc.c @@ -776,7 +776,7 @@ apr_byte_t oidc_session_pass_tokens(request_rec *r, oidc_cfg *cfg, oidc_session_ } /* set the expiry timestamp in the app headers/variables */ - const char *access_token_expires = oidc_session_get_access_token_expires(r, session); + const char *access_token_expires = oidc_session_get_access_token_expires_str(r, session); if ((oidc_cfg_dir_pass_access_token(r) != 0) && access_token_expires != NULL) { /* pass it to the app in a header or environment variable */ oidc_util_set_app_info(r, OIDC_APP_INFO_ACCESS_TOKEN_EXP, access_token_expires, @@ -834,7 +834,7 @@ static apr_byte_t oidc_userinfo_create_signed_jwt(request_rec *r, oidc_cfg *cfg, oidc_jwt_t *jwt = NULL; oidc_jwk_t *jwk = NULL; oidc_jose_error_t err; - const char *access_token_expires = NULL; + apr_time_t access_token_expires = -1; char *jti = NULL; char *key = NULL; json_t *json = NULL; @@ -902,10 +902,11 @@ static apr_byte_t oidc_userinfo_create_signed_jwt(request_rec *r, oidc_cfg *cfg, } if (json_object_get(jwt->payload.value.json, OIDC_CLAIM_EXP) == NULL) { access_token_expires = oidc_session_get_access_token_expires(r, session); - json_object_set_new(jwt->payload.value.json, OIDC_CLAIM_EXP, - json_integer(access_token_expires ? _oidc_str_to_int(access_token_expires) - : apr_time_sec(apr_time_now()) + - OIDC_USERINFO_SIGNED_JWT_EXPIRE_DEFAULT)); + json_object_set_new( + jwt->payload.value.json, OIDC_CLAIM_EXP, + json_integer(access_token_expires != -1 + ? access_token_expires + : apr_time_sec(apr_time_now()) + OIDC_USERINFO_SIGNED_JWT_EXPIRE_DEFAULT)); } if (oidc_jwt_sign(r->pool, jwt, jwk, FALSE, &err) == FALSE) { diff --git a/src/mod_auth_openidc.h b/src/mod_auth_openidc.h index 0b031d23..63bbd544 100644 --- a/src/mod_auth_openidc.h +++ b/src/mod_auth_openidc.h @@ -930,7 +930,8 @@ const char *oidc_session_get_idtoken(request_rec *r, oidc_session_t *z); void oidc_session_set_access_token(request_rec *r, oidc_session_t *z, const char *access_token); const char *oidc_session_get_access_token(request_rec *r, oidc_session_t *z); void oidc_session_set_access_token_expires(request_rec *r, oidc_session_t *z, const int expires_in); -const char *oidc_session_get_access_token_expires(request_rec *r, oidc_session_t *z); +apr_time_t oidc_session_get_access_token_expires(request_rec *r, oidc_session_t *z); +const char *oidc_session_get_access_token_expires_str(request_rec *r, oidc_session_t *z); void oidc_session_set_refresh_token(request_rec *r, oidc_session_t *z, const char *refresh_token); const char *oidc_session_get_refresh_token(request_rec *r, oidc_session_t *z); void oidc_session_set_session_expires(request_rec *r, oidc_session_t *z, const apr_time_t expires); diff --git a/src/proto.c b/src/proto.c index 5cdd08ca..f03ed7a4 100644 --- a/src/proto.c +++ b/src/proto.c @@ -1331,6 +1331,19 @@ apr_byte_t oidc_proto_token_endpoint_auth(request_rec *r, oidc_cfg *cfg, const c return FALSE; } +/* + * parse the expiry for the access token + */ +static int oidc_proto_parse_expires_in(request_rec *r, const char *expires_in, const int default_value) { + int number = _oidc_str_to_int(expires_in); + if (number <= 0) { + oidc_warn(r, "could not parse \"expires_in\" value (%s) into a positive integer", expires_in); + number = default_value; + } + oidc_debug(r, "return: %d", number); + return number; +} + /* * send a code/refresh request to the token endpoint and return the parsed contents */ @@ -1341,6 +1354,7 @@ static apr_byte_t oidc_proto_token_endpoint_request(request_rec *r, oidc_cfg *cf char *response = NULL; char *basic_auth = NULL; char *bearer_auth = NULL; + json_t *j_result = NULL, *j_expires_in = NULL; /* add the token endpoint authentication credentials */ if (oidc_proto_token_endpoint_auth(r, cfg, provider->token_endpoint_auth, provider->client_id, @@ -1363,18 +1377,17 @@ static apr_byte_t oidc_proto_token_endpoint_request(request_rec *r, oidc_cfg *cf } /* check for errors, the response itself will have been logged already */ - json_t *result = NULL; - if (oidc_util_decode_json_and_check_error(r, response, &result) == FALSE) + if (oidc_util_decode_json_and_check_error(r, response, &j_result) == FALSE) return FALSE; /* get the id_token from the parsed response */ - oidc_json_object_get_string(r->pool, result, OIDC_PROTO_ID_TOKEN, id_token, NULL); + oidc_json_object_get_string(r->pool, j_result, OIDC_PROTO_ID_TOKEN, id_token, NULL); /* get the access_token from the parsed response */ - oidc_json_object_get_string(r->pool, result, OIDC_PROTO_ACCESS_TOKEN, access_token, NULL); + oidc_json_object_get_string(r->pool, j_result, OIDC_PROTO_ACCESS_TOKEN, access_token, NULL); /* get the token type from the parsed response */ - oidc_json_object_get_string(r->pool, result, OIDC_PROTO_TOKEN_TYPE, token_type, NULL); + oidc_json_object_get_string(r->pool, j_result, OIDC_PROTO_TOKEN_TYPE, token_type, NULL); /* check the new token type */ if (token_type != NULL) { @@ -1384,13 +1397,21 @@ static apr_byte_t oidc_proto_token_endpoint_request(request_rec *r, oidc_cfg *cf } } - /* get the expires_in value */ - oidc_json_object_get_int(result, OIDC_PROTO_EXPIRES_IN, expires_in, -1); + /* get the access token expires_in value */ + *expires_in = -1; + j_expires_in = json_object_get(j_result, OIDC_PROTO_EXPIRES_IN); + if (j_expires_in != NULL) { + /* cater for string values (old Azure AD) */ + if (json_is_string(j_expires_in)) + *expires_in = oidc_proto_parse_expires_in(r, json_string_value(j_expires_in), -1); + else if (json_is_integer(j_expires_in)) + *expires_in = json_integer_value(j_expires_in); + } /* get the refresh_token from the parsed response */ - oidc_json_object_get_string(r->pool, result, OIDC_PROTO_REFRESH_TOKEN, refresh_token, NULL); + oidc_json_object_get_string(r->pool, j_result, OIDC_PROTO_REFRESH_TOKEN, refresh_token, NULL); - json_decref(result); + json_decref(j_result); return TRUE; } diff --git a/src/session.c b/src/session.c index b3dfaf17..4299c238 100644 --- a/src/session.c +++ b/src/session.c @@ -594,13 +594,18 @@ const char *oidc_session_get_access_token(request_rec *r, oidc_session_t *z) { */ void oidc_session_set_access_token_expires(request_rec *r, oidc_session_t *z, const int expires_in) { if (expires_in != -1) { - oidc_session_set(r, z, OIDC_SESSION_KEY_ACCESSTOKEN_EXPIRES, - apr_psprintf(r->pool, "%" APR_TIME_T_FMT, apr_time_sec(apr_time_now()) + expires_in)); + oidc_debug(r, "storing access token expires_in in the session: %d", expires_in); + oidc_session_set_timestamp(r, z, OIDC_SESSION_KEY_ACCESSTOKEN_EXPIRES, + apr_time_now() + apr_time_from_sec(expires_in)); } } -const char *oidc_session_get_access_token_expires(request_rec *r, oidc_session_t *z) { - return oidc_session_get_key2string(r, z, OIDC_SESSION_KEY_ACCESSTOKEN_EXPIRES); +apr_time_t oidc_session_get_access_token_expires(request_rec *r, oidc_session_t *z) { + return apr_time_sec(oidc_session_get_key2timestamp(r, z, OIDC_SESSION_KEY_ACCESSTOKEN_EXPIRES)); +} + +const char *oidc_session_get_access_token_expires_str(request_rec *r, oidc_session_t *z) { + return apr_psprintf(r->pool, "%" APR_TIME_T_FMT, oidc_session_get_access_token_expires(r, z)); } /*