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 <[email protected]>
OpenIDC · Nov 7, 2024 · fed0f2f · fed0f2f
1 parent bddff12
commit fed0f2f
Show file tree

Hide file tree

Showing 5 changed files with 82 additions and 62 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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);