From fed0f2f71bde4143cb0d48bc792265f5dfdec0de Mon Sep 17 00:00:00 2001 From: Hans Zandbelt Date: Thu, 7 Nov 2024 15:21:23 +0100 Subject: [PATCH] fix requests to the info hook with extend_session=false; see #1279 - properly reflect the (unmodified) inactivity timeout in the response ("timeout") - avoid refreshing an access token (since the session is not saved) - avoid refreshing claims from the user info endpoint, and possibly refreshing the access token thanks @fnieri-cdp Signed-off-by: Hans Zandbelt --- ChangeLog | 6 ++ src/handle/info.c | 6 +- src/handle/refresh.c | 2 +- src/mod_auth_openidc.c | 125 +++++++++++++++++++++++------------------ src/mod_auth_openidc.h | 5 +- 5 files changed, 82 insertions(+), 62 deletions(-) diff --git a/ChangeLog b/ChangeLog index 9e7737a1..c207fab1 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,9 @@ +11/07/2024 +- info: fix requests to the info hook with extend_session=false; see #1279; thanks @fnieri-cdp + - properly reflect the (unmodified) inactivity timeout in the response ("timeout") + - avoid refreshing an access token (since the session is not saved) + - avoid refreshing claims from the user info endpoint, and possibly refreshing the access token + 10/23/2024 - metadata: allow plain HTTP URLs in metadata elements `jwks_uri` and `signed_jwks_uri` to ensure backwards compatibility with <=2.4.15.7 and to support private/test deployments diff --git a/src/handle/info.c b/src/handle/info.c index 8a98e476..3dd81dea 100644 --- a/src/handle/info.c +++ b/src/handle/info.c @@ -45,7 +45,6 @@ #include "util.h" #define OIDC_INFO_PARAM_ACCESS_TOKEN_REFRESH_INTERVAL "access_token_refresh_interval" -#define OIDC_INFO_PARAM_EXTEND_SESSION "extend_session" #define OIDC_HOOK_INFO_FORMAT_JSON "json" #define OIDC_HOOK_INFO_FORMAT_HTML "html" @@ -209,9 +208,8 @@ int oidc_info_request(request_rec *r, oidc_cfg_t *c, oidc_session_t *session, ap } /* pass the tokens to the application and save the session, possibly updating the expiry */ - if (b_extend_session) - if (oidc_session_pass_tokens(r, c, session, &needs_save) == FALSE) - oidc_warn(r, "error passing tokens"); + if (oidc_session_pass_tokens(r, c, session, b_extend_session, &needs_save) == FALSE) + oidc_warn(r, "error passing tokens"); /* check if something was updated in the session and we need to save it again */ if (b_extend_session && needs_save) { diff --git a/src/handle/refresh.c b/src/handle/refresh.c index 150ec125..ce68a508 100644 --- a/src/handle/refresh.c +++ b/src/handle/refresh.c @@ -363,7 +363,7 @@ int oidc_refresh_token_request(request_rec *r, oidc_cfg_t *c, oidc_session_t *se } /* pass the tokens to the application, possibly updating the expiry */ - if (oidc_session_pass_tokens(r, c, session, &needs_save) == FALSE) { + if (oidc_session_pass_tokens(r, c, session, TRUE, &needs_save) == FALSE) { error_code = "session_corruption"; goto end; } diff --git a/src/mod_auth_openidc.c b/src/mod_auth_openidc.c index d96d9b6d..c13b6abf 100644 --- a/src/mod_auth_openidc.c +++ b/src/mod_auth_openidc.c @@ -599,7 +599,8 @@ static void oidc_copy_tokens_to_request_state(request_rec *r, oidc_session_t *se /* * pass refresh_token, access_token and access_token_expires as headers/environment variables to the application */ -apr_byte_t oidc_session_pass_tokens(request_rec *r, oidc_cfg_t *cfg, oidc_session_t *session, apr_byte_t *needs_save) { +apr_byte_t oidc_session_pass_tokens(request_rec *r, oidc_cfg_t *cfg, oidc_session_t *session, apr_byte_t extend_session, + apr_byte_t *needs_save) { oidc_appinfo_pass_in_t pass_in = oidc_cfg_dir_pass_info_in_get(r); oidc_appinfo_encoding_t encoding = oidc_cfg_dir_pass_info_encoding_get(r); @@ -636,27 +637,29 @@ apr_byte_t oidc_session_pass_tokens(request_rec *r, oidc_cfg_t *cfg, oidc_sessio OIDC_DEFAULT_HEADER_PREFIX, pass_in, encoding); } - /* - * reset the session inactivity timer - * but only do this once per 10% of the inactivity timeout interval (with a max to 60 seconds) - * for performance reasons - * - * now there's a small chance that the session ends 10% (or a minute) earlier than configured/expected - * cq. when there's a request after a recent save (so no update) and then no activity happens until - * a request comes in just before the session should expire - * ("recent" and "just before" refer to 10%-with-a-max-of-60-seconds of the inactivity interval after - * the start/last-update and before the expiry of the session respectively) - * - * this is be deemed acceptable here because of performance gain - */ - apr_time_t interval = apr_time_from_sec(oidc_cfg_session_inactivity_timeout_get(cfg)); - apr_time_t now = apr_time_now(); - apr_time_t slack = interval / 10; - if (slack > apr_time_from_sec(60)) - slack = apr_time_from_sec(60); - if (session->expiry - now < interval - slack) { - session->expiry = now + interval; - *needs_save = TRUE; + if (extend_session) { + /* + * reset the session inactivity timer + * but only do this once per 10% of the inactivity timeout interval (with a max to 60 seconds) + * for performance reasons + * + * now there's a small chance that the session ends 10% (or a minute) earlier than configured/expected + * cq. when there's a request after a recent save (so no update) and then no activity happens until + * a request comes in just before the session should expire + * ("recent" and "just before" refer to 10%-with-a-max-of-60-seconds of the inactivity interval after + * the start/last-update and before the expiry of the session respectively) + * + * this is be deemed acceptable here because of performance gain + */ + apr_time_t interval = apr_time_from_sec(oidc_cfg_session_inactivity_timeout_get(cfg)); + apr_time_t now = apr_time_now(); + apr_time_t slack = interval / 10; + if (slack > apr_time_from_sec(60)) + slack = apr_time_from_sec(60); + if (session->expiry - now < interval - slack) { + session->expiry = now + interval; + *needs_save = TRUE; + } } /* log message about session expiry */ @@ -669,7 +672,7 @@ apr_byte_t oidc_session_pass_tokens(request_rec *r, oidc_cfg_t *cfg, oidc_sessio * handle the case where we have identified an existing authentication session for a user */ static int oidc_handle_existing_session(request_rec *r, oidc_cfg_t *cfg, oidc_session_t *session, - apr_byte_t *needs_save) { + apr_byte_t extend_session, apr_byte_t *needs_save) { apr_byte_t rv = FALSE; int rc = OK; @@ -710,39 +713,44 @@ static int oidc_handle_existing_session(request_rec *r, oidc_cfg_t *cfg, oidc_se return rc; } - /* if needed, refresh the access token */ - rv = oidc_refresh_access_token_before_expiry( - r, cfg, session, oidc_cfg_dir_refresh_access_token_before_expiry_get(r), needs_save); - if (rv == FALSE) { - *needs_save = FALSE; - oidc_debug(r, "dir_action_on_error_refresh: %d", oidc_cfg_dir_action_on_error_refresh_get(r)); - OIDC_METRICS_COUNTER_INC(r, cfg, OM_SESSION_ERROR_REFRESH_ACCESS_TOKEN); - if (oidc_cfg_dir_action_on_error_refresh_get(r) == OIDC_ON_ERROR_LOGOUT) { - return oidc_logout_request( - r, cfg, session, oidc_util_absolute_url(r, cfg, oidc_cfg_default_slo_url_get(cfg)), FALSE); - } - if (oidc_cfg_dir_action_on_error_refresh_get(r) == OIDC_ON_ERROR_AUTH) { - oidc_session_kill(r, session); - return oidc_handle_unauthenticated_user(r, cfg); - } - return HTTP_BAD_GATEWAY; - } + if (extend_session) { - /* if needed, refresh claims from the user info endpoint */ - rv = oidc_userinfo_refresh_claims(r, cfg, session, needs_save); - if (rv == FALSE) { - *needs_save = FALSE; - oidc_debug(r, "action_on_userinfo_error: %d", oidc_cfg_action_on_userinfo_error_get(cfg)); - OIDC_METRICS_COUNTER_INC(r, cfg, OM_SESSION_ERROR_REFRESH_USERINFO); - if (oidc_cfg_action_on_userinfo_error_get(cfg) == OIDC_ON_ERROR_LOGOUT) { - return oidc_logout_request( - r, cfg, session, oidc_util_absolute_url(r, cfg, oidc_cfg_default_slo_url_get(cfg)), FALSE); + /* if needed, refresh the access token */ + rv = oidc_refresh_access_token_before_expiry( + r, cfg, session, oidc_cfg_dir_refresh_access_token_before_expiry_get(r), needs_save); + if (rv == FALSE) { + *needs_save = FALSE; + oidc_debug(r, "dir_action_on_error_refresh: %d", oidc_cfg_dir_action_on_error_refresh_get(r)); + OIDC_METRICS_COUNTER_INC(r, cfg, OM_SESSION_ERROR_REFRESH_ACCESS_TOKEN); + if (oidc_cfg_dir_action_on_error_refresh_get(r) == OIDC_ON_ERROR_LOGOUT) { + return oidc_logout_request( + r, cfg, session, oidc_util_absolute_url(r, cfg, oidc_cfg_default_slo_url_get(cfg)), + FALSE); + } + if (oidc_cfg_dir_action_on_error_refresh_get(r) == OIDC_ON_ERROR_AUTH) { + oidc_session_kill(r, session); + return oidc_handle_unauthenticated_user(r, cfg); + } + return HTTP_BAD_GATEWAY; } - if (oidc_cfg_action_on_userinfo_error_get(cfg) == OIDC_ON_ERROR_AUTH) { - oidc_session_kill(r, session); - return oidc_handle_unauthenticated_user(r, cfg); + + /* if needed, refresh claims from the user info endpoint */ + rv = oidc_userinfo_refresh_claims(r, cfg, session, needs_save); + if (rv == FALSE) { + *needs_save = FALSE; + oidc_debug(r, "action_on_userinfo_error: %d", oidc_cfg_action_on_userinfo_error_get(cfg)); + OIDC_METRICS_COUNTER_INC(r, cfg, OM_SESSION_ERROR_REFRESH_USERINFO); + if (oidc_cfg_action_on_userinfo_error_get(cfg) == OIDC_ON_ERROR_LOGOUT) { + return oidc_logout_request( + r, cfg, session, oidc_util_absolute_url(r, cfg, oidc_cfg_default_slo_url_get(cfg)), + FALSE); + } + if (oidc_cfg_action_on_userinfo_error_get(cfg) == OIDC_ON_ERROR_AUTH) { + oidc_session_kill(r, session); + return oidc_handle_unauthenticated_user(r, cfg); + } + return HTTP_BAD_GATEWAY; } - return HTTP_BAD_GATEWAY; } /* set the user authentication HTTP header if set and required */ @@ -777,7 +785,7 @@ static int oidc_handle_existing_session(request_rec *r, oidc_cfg_t *cfg, oidc_se } /* pass the at, rt and at expiry to the application, possibly update the session expiry */ - if (oidc_session_pass_tokens(r, cfg, session, needs_save) == FALSE) + if (oidc_session_pass_tokens(r, cfg, session, extend_session, needs_save) == FALSE) return HTTP_INTERNAL_SERVER_ERROR; oidc_userinfo_pass_as(r, cfg, session, s_claims, pass_in, encoding); @@ -971,6 +979,7 @@ static int oidc_javascript_implicit(request_rec *r, oidc_cfg_t *c) { int oidc_handle_redirect_uri_request(request_rec *r, oidc_cfg_t *c, oidc_session_t *session) { apr_byte_t needs_save = FALSE; + char *s_extend_session = NULL; int rc = OK; OIDC_METRICS_TIMING_START(r, c); @@ -1098,8 +1107,12 @@ int oidc_handle_redirect_uri_request(request_rec *r, oidc_cfg_t *c, oidc_session OIDC_METRICS_COUNTER_INC(r, c, OM_REDIRECT_URI_REQUEST_INFO); + oidc_util_request_parameter_get(r, OIDC_INFO_PARAM_EXTEND_SESSION, &s_extend_session); + // need to establish user/claims for authorization purposes - rc = oidc_handle_existing_session(r, c, session, &needs_save); + rc = oidc_handle_existing_session( + r, c, session, (s_extend_session == NULL) || (_oidc_strcmp(s_extend_session, "false") != 0), + &needs_save); // retain this session across the authentication and content handler phases // by storing it in the request state @@ -1233,7 +1246,7 @@ static int oidc_check_userid_openidc(request_rec *r, oidc_cfg_t *c) { } else if (session->remote_user != NULL) { /* this is initial request and we already have a session */ - rc = oidc_handle_existing_session(r, c, session, &needs_save); + rc = oidc_handle_existing_session(r, c, session, TRUE, &needs_save); if (rc == OK) { /* check if something was updated in the session and we need to save it again */ diff --git a/src/mod_auth_openidc.h b/src/mod_auth_openidc.h index 4dac1429..46422cfc 100644 --- a/src/mod_auth_openidc.h +++ b/src/mod_auth_openidc.h @@ -99,6 +99,8 @@ #define OIDC_REDIRECT_URI_REQUEST_SID "sid" #define OIDC_REDIRECT_URI_REQUEST_ISS "iss" +#define OIDC_INFO_PARAM_EXTEND_SESSION "extend_session" + #define OIDC_CLAIM_ISS "iss" #define OIDC_CLAIM_AUD "aud" #define OIDC_CLAIM_AZP "azp" @@ -145,7 +147,8 @@ apr_byte_t oidc_get_remote_user(request_rec *r, const char *claim_name, const ch json_t *json, char **request_user); apr_byte_t oidc_get_provider_from_session(request_rec *r, oidc_cfg_t *c, oidc_session_t *session, oidc_provider_t **provider); -apr_byte_t oidc_session_pass_tokens(request_rec *r, oidc_cfg_t *cfg, oidc_session_t *session, apr_byte_t *needs_save); +apr_byte_t oidc_session_pass_tokens(request_rec *r, oidc_cfg_t *cfg, oidc_session_t *session, apr_byte_t extend_session, + apr_byte_t *needs_save); void oidc_log_session_expires(request_rec *r, const char *msg, apr_time_t session_expires); apr_byte_t oidc_provider_static_config(request_rec *r, oidc_cfg_t *c, oidc_provider_t **provider); const char *oidc_original_request_method(request_rec *r, oidc_cfg_t *cfg, apr_byte_t handle_discovery_response);