2.4.14.4rc6: improve behaviour for parallel refresh token requests

Signed-off-by: Hans Zandbelt <[email protected]>

ChangeLog

@@ -1,3 +1,9 @@
+10/11/2023
+- improve behaviour when parallel refresh token grant requests occur on the same Apache server/host
+  and rolling refresh tokens are issued; synchronize using a global refresh token lock and avoid
+  corrupting the session by storing/overwriting an expired refresh token
+- bump to 2.4.14.4rc6
+
 09/22/2023
 - performance: store userinfo refresh interval in session to avoid parsing JSON on each request
 - fix memory leak in oidc_refresh_token_grant: free the parsed id_token that is returned

configure.ac

@@ -1,4 +1,4 @@
-AC_INIT([mod_auth_openidc],[2.4.14.4rc5],[[email protected]])
+AC_INIT([mod_auth_openidc],[2.4.14.4rc6],[[email protected]])
 
 AC_SUBST(NAMEVER, AC_PACKAGE_TARNAME()-AC_PACKAGE_VERSION())
 

src/cache/cache.h

@@ -96,6 +96,7 @@ apr_byte_t oidc_cache_set(request_rec *r, const char *section, const char *key,
 #define OIDC_CACHE_SECTION_NONCE             "n"
 #define OIDC_CACHE_SECTION_JWKS              "j"
 #define OIDC_CACHE_SECTION_ACCESS_TOKEN      "a"
+#define OIDC_CACHE_SECTION_REFRESH_TOKEN     "e"
 #define OIDC_CACHE_SECTION_PROVIDER          "p"
 #define OIDC_CACHE_SECTION_OAUTH_PROVIDER    "o"
 #define OIDC_CACHE_SECTION_JTI               "t"
@@ -111,6 +112,7 @@ apr_byte_t oidc_cache_set(request_rec *r, const char *section, const char *key,
 #define oidc_cache_get_nonce(r, key, value) oidc_cache_get(r, OIDC_CACHE_SECTION_NONCE, key, value)
 #define oidc_cache_get_jwks(r, key, value) oidc_cache_get(r, OIDC_CACHE_SECTION_JWKS, key, value)
 #define oidc_cache_get_access_token(r, key, value) oidc_cache_get(r, OIDC_CACHE_SECTION_ACCESS_TOKEN, key, value)
+#define oidc_cache_get_refresh_token(r, key, value) oidc_cache_get(r, OIDC_CACHE_SECTION_REFRESH_TOKEN, key, value)
 #define oidc_cache_get_provider(r, key, value) oidc_cache_get(r, OIDC_CACHE_SECTION_PROVIDER, key, value)
 #define oidc_cache_get_oauth_provider(r, key, value) oidc_cache_get(r, OIDC_CACHE_SECTION_OAUTH_PROVIDER, key, value)
 #define oidc_cache_get_jti(r, key, value) oidc_cache_get(r, OIDC_CACHE_SECTION_JTI, key, value)
@@ -123,6 +125,7 @@ apr_byte_t oidc_cache_set(request_rec *r, const char *section, const char *key,
 #define oidc_cache_set_nonce(r, key, value, expiry) oidc_cache_set(r, OIDC_CACHE_SECTION_NONCE, key, value, expiry)
 #define oidc_cache_set_jwks(r, key, value, expiry) oidc_cache_set(r, OIDC_CACHE_SECTION_JWKS, key, value, expiry)
 #define oidc_cache_set_access_token(r, key, value, expiry) oidc_cache_set(r, OIDC_CACHE_SECTION_ACCESS_TOKEN, key, value, expiry)
+#define oidc_cache_set_refresh_token(r, key, value, expiry) oidc_cache_set(r, OIDC_CACHE_SECTION_REFRESH_TOKEN, key, value, expiry)
 #define oidc_cache_set_provider(r, key, value, expiry) oidc_cache_set(r, OIDC_CACHE_SECTION_PROVIDER, key, value, expiry)
 #define oidc_cache_set_oauth_provider(r, key, value, expiry) oidc_cache_set(r, OIDC_CACHE_SECTION_OAUTH_PROVIDER, key, value, expiry)
 #define oidc_cache_set_jti(r, key, value, expiry) oidc_cache_set(r, OIDC_CACHE_SECTION_JTI, key, value, expiry)

src/config.c

@@ -1862,6 +1862,7 @@ void* oidc_create_server_config(apr_pool_t *pool, server_rec *svr) {
 	c->logout_x_frame_options = NULL;
 	c->x_forwarded_headers = OIDC_DEFAULT_X_FORWARDED_HEADERS;
 	c->action_on_userinfo_error = OIDC_ON_ERROR_CONTINUE;
+	c->refresh_mutex = oidc_cache_mutex_create(pool, TRUE);
 
 	return c;
 }
@@ -2111,8 +2112,8 @@ void* oidc_merge_server_config(apr_pool_t *pool, void *BASE, void *ADD) {
 			add->cookie_domain != NULL ?
 					add->cookie_domain : base->cookie_domain;
 	c->claim_delimiter =
-			_oidc_strcmp(add->claim_delimiter, OIDC_DEFAULT_CLAIM_DELIMITER)
-			!= 0 ? add->claim_delimiter : base->claim_delimiter;
+			_oidc_strcmp(add->claim_delimiter, OIDC_DEFAULT_CLAIM_DELIMITER) != 0 ?
+					add->claim_delimiter : base->claim_delimiter;
 	c->claim_prefix =
 			add->claim_prefix != NULL ? add->claim_prefix : base->claim_prefix;
 	c->remote_user_claim.claim_name =
@@ -2206,6 +2207,9 @@ void* oidc_merge_server_config(apr_pool_t *pool, void *BASE, void *ADD) {
 					add->action_on_userinfo_error :
 					base->action_on_userinfo_error;
 
+	c->refresh_mutex =
+			c->refresh_mutex != NULL ? add->refresh_mutex : base->refresh_mutex;
+
 	return c;
 }
 
@@ -2505,8 +2509,7 @@ void* oidc_merge_dir_config(apr_pool_t *pool, void *BASE, void *ADD) {
 			add->unautz_action != OIDC_CONFIG_POS_INT_UNSET ?
 					add->unautz_action : base->unautz_action;
 	c->unauthz_arg =
-			add->unauthz_arg != NULL ?
-					add->unauthz_arg : base->unauthz_arg;
+			add->unauthz_arg != NULL ? add->unauthz_arg : base->unauthz_arg;
 
 	c->pass_cookies =
 			add->pass_cookies != NULL ? add->pass_cookies : base->pass_cookies;
@@ -2805,6 +2808,12 @@ static apr_status_t oidc_cleanup_child(void *data) {
 				oidc_serror(sp, "cache destroy function failed");
 			}
 		}
+		if (cfg->refresh_mutex != NULL) {
+			if (oidc_cache_mutex_destroy(sp, cfg->refresh_mutex) != TRUE) {
+				oidc_serror(sp,
+						"oidc_cache_mutex_destroy on refresh mutex failed");
+			}
+		}
 		sp = sp->next;
 	}
 
@@ -2919,6 +2928,11 @@ static int oidc_post_config(apr_pool_t *pool, apr_pool_t *p1, apr_pool_t *p2,
 			if (cfg->cache->post_config(sp) != OK)
 				return HTTP_INTERNAL_SERVER_ERROR;
 		}
+		if (cfg->refresh_mutex != NULL) {
+			if (oidc_cache_mutex_post_config(sp, cfg->refresh_mutex,
+					"refresh") != TRUE)
+				return HTTP_INTERNAL_SERVER_ERROR;
+		}
 		sp = sp->next;
 	}
 
@@ -2966,8 +2980,7 @@ static const authz_provider oidc_authz_claim_provider = {
 #ifdef USE_LIBJQ
 static const authz_provider oidc_authz_claims_expr_provider = {
 		&oidc_authz_checker_claims_expr,
-		NULL,
-};
+		NULL, };
 #endif
 
 #endif
@@ -2985,6 +2998,13 @@ static void oidc_child_init(apr_pool_t *p, server_rec *s) {
 				oidc_serror(sp, "cfg->cache->child_init failed");
 			}
 		}
+		if (cfg->refresh_mutex != NULL) {
+			if (oidc_cache_mutex_child_init(p, sp,
+					cfg->refresh_mutex) != APR_SUCCESS) {
+				oidc_serror(sp,
+						"oidc_cache_mutex_child_init on refresh mutex failed");
+			}
+		}
 		sp = sp->next;
 	}
 	apr_pool_cleanup_register(p, s, oidc_cleanup_child, apr_pool_cleanup_null);
@@ -3062,7 +3082,8 @@ static apr_status_t oidc_filter_in_filter(ap_filter_t *f,
 
 				if (oidc_util_hdr_in_content_length_get(f->r) != NULL)
 					oidc_util_hdr_in_set(f->r, OIDC_HTTP_HDR_CONTENT_LENGTH,
-							apr_psprintf(f->r->pool, "%ld", (long)ctx->nbytes));
+							apr_psprintf(f->r->pool, "%ld",
+									(long) ctx->nbytes));
 
 				apr_pool_userdata_set(NULL, OIDC_USERDATA_POST_PARAMS_KEY,
 						NULL, f->r->pool);

src/mod_auth_openidc.c

@@ -1061,31 +1061,50 @@ static apr_byte_t oidc_refresh_token_grant(request_rec *r, oidc_cfg *c,
 		oidc_session_t *session, oidc_provider_t *provider,
 		char **new_access_token, char **new_id_token) {
 
+	apr_byte_t rc = FALSE;
+	char *s_id_token = NULL;
+	int expires_in = -1;
+	char *s_token_type = NULL;
+	char *s_access_token = NULL;
+	char *s_refresh_token = NULL;
+	oidc_jwt_t *id_token_jwt = NULL;
+	oidc_jose_error_t err;
+	char *value = NULL;
+	const char *refresh_token = NULL;
+
 	oidc_debug(r, "enter");
 
+	oidc_cache_mutex_lock(r->pool, r->server, c->refresh_mutex);
+
 	/* get the refresh token that was stored in the session */
-	const char *refresh_token = oidc_session_get_refresh_token(r, session);
+	refresh_token = oidc_session_get_refresh_token(r, session);
 	if (refresh_token == NULL) {
 		oidc_warn(r,
 				"refresh token routine called but no refresh_token found in the session");
-		return FALSE;
+		goto end;
 	}
 
-	/* elements returned in the refresh response */
-	char *s_id_token = NULL;
-	int expires_in = -1;
-	char *s_token_type = NULL;
-	char *s_access_token = NULL;
-	char *s_refresh_token = NULL;
-	oidc_jwt_t *id_token_jwt = NULL;
-	oidc_jose_error_t err;
+	// check existing refresh is going on
+	oidc_cache_get_refresh_token(r, refresh_token, &value);
+	if (value != NULL) {
+		oidc_warn(r,
+				"refresh token routine called but existing parallel refresh is in progress");
+		goto end;
+	}
+	// "lock" the refresh token best effort; this does not work failsafe in a clustered setup...
+	oidc_cache_set_refresh_token(r, refresh_token, refresh_token,
+			apr_time_now() + apr_time_from_sec(c->http_timeout_long));
+	oidc_debug(r, "refreshing refresh_token: %s", refresh_token);
+	// don't unlock after this since other processes may be waiting for the lock to refresh the same refresh token
 
 	/* refresh the tokens by calling the token endpoint */
 	if (oidc_proto_refresh_request(r, c, provider, refresh_token, &s_id_token,
 			&s_access_token, &s_token_type, &expires_in,
 			&s_refresh_token) == FALSE) {
-		oidc_error(r, "access_token could not be refreshed");
-		return FALSE;
+		oidc_error(r,
+				"access_token could not be refreshed with refresh_token: %s",
+				refresh_token);
+		goto end;
 	}
 
 	/* store the new access_token in the session and discard the old one */
@@ -1139,7 +1158,16 @@ static apr_byte_t oidc_refresh_token_grant(request_rec *r, oidc_cfg *c,
 			oidc_jwt_destroy(id_token_jwt);
 	}
 
-	return TRUE;
+	oidc_debug(r, "refreshed refresh_token: %s into %s", refresh_token,
+			s_refresh_token);
+
+	rc = TRUE;
+
+end:
+
+	oidc_cache_mutex_unlock(r->pool, r->server, c->refresh_mutex);
+
+	return rc;
 }
 
 /*
@@ -1274,10 +1302,13 @@ static apr_byte_t oidc_refresh_claims_from_userinfo_endpoint(request_rec *r,
 				oidc_store_userinfo_claims(r, cfg, session, provider, claims,
 						userinfo_jwt);
 
-				/* indicated something changed */
-				*needs_save = TRUE;
-
-				rc = (claims != NULL);
+				if (claims == NULL) {
+					*needs_save = FALSE;
+					rc = FALSE;
+				} else {
+					/* indicated something changed */
+					*needs_save = TRUE;
+				}
 			}
 		}
 	}
@@ -1423,6 +1454,7 @@ static apr_byte_t oidc_refresh_access_token_before_expiry(request_rec *r,
 	if (oidc_refresh_token_grant(r, cfg, session, provider,
 			NULL, NULL) == FALSE) {
 		oidc_warn(r, "access_token could not be refreshed");
+		*needs_save = FALSE;
 		return FALSE;
 	}
 
@@ -1694,6 +1726,7 @@ static int oidc_handle_existing_session(request_rec *r, oidc_cfg *cfg,
 	/* check if the maximum session duration was exceeded */
 	if (oidc_check_max_session_duration(r, cfg, session, &rc) == FALSE) {
 		*needs_save = FALSE;
+		// NB: rc was set (e.g. to a 302 auth redirect) by the call to oidc_check_max_session_duration
 		return rc;
 	}
 
@@ -1712,12 +1745,16 @@ static int oidc_handle_existing_session(request_rec *r, oidc_cfg *cfg,
 			oidc_session_kill(r, session);
 			return oidc_handle_unauthenticated_user(r, cfg);
 		}
+		*needs_save = FALSE;
+		return HTTP_UNAUTHORIZED;
 	}
 
 	/* if needed, refresh claims from the user info endpoint */
-	rv = oidc_refresh_claims_from_userinfo_endpoint(r, cfg, session, needs_save);
+	rv = oidc_refresh_claims_from_userinfo_endpoint(r, cfg, session,
+			needs_save);
 	if (rv == FALSE) {
-		oidc_debug(r, "action_on_userinfo_error: %d", cfg->action_on_userinfo_error);
+		oidc_debug(r, "action_on_userinfo_error: %d",
+				cfg->action_on_userinfo_error);
 		if (cfg->action_on_userinfo_error == OIDC_ON_ERROR_LOGOUT) {
 			*needs_save = FALSE;
 			return oidc_handle_logout_request(r, cfg, session,
@@ -1728,6 +1765,8 @@ static int oidc_handle_existing_session(request_rec *r, oidc_cfg *cfg,
 			oidc_session_kill(r, session);
 			return oidc_handle_unauthenticated_user(r, cfg);
 		}
+		*needs_save = FALSE;
+		return HTTP_UNAUTHORIZED;
 	}
 
 	/* set the user authentication HTTP header if set and required */
@@ -3978,10 +4017,11 @@ static int oidc_handle_info_request(request_rec *r, oidc_cfg *c,
 
 				/* execute the actual refresh grant */
 				if (oidc_refresh_token_grant(r, c, session, provider,
-						NULL, NULL) == FALSE)
+						NULL, NULL) == FALSE) {
 					oidc_warn(r, "access_token could not be refreshed");
-				else
-					needs_save = TRUE;
+					return HTTP_INTERNAL_SERVER_ERROR;
+				}
+				needs_save = TRUE;
 			}
 		}
 	}
@@ -4001,8 +4041,13 @@ static int oidc_handle_info_request(request_rec *r, oidc_cfg *c,
 	 * side-effect is that this may refresh the access token if not already done
 	 * note that OIDCUserInfoRefreshInterval should be set to control the refresh policy
 	 */
-	if (b_extend_session)
-		oidc_refresh_claims_from_userinfo_endpoint(r, c, session, &needs_save);
+	if (b_extend_session) {
+		if (oidc_refresh_claims_from_userinfo_endpoint(r, c, session,
+				&needs_save) == FALSE) {
+			rc = HTTP_INTERNAL_SERVER_ERROR;
+			goto end;
+		}
+	}
 
 	/* include the access token in the session info */
 	if (apr_hash_get(c->info_hook_data, OIDC_HOOK_INFO_ACCES_TOKEN,
@@ -4101,6 +4146,7 @@ static int oidc_handle_info_request(request_rec *r, oidc_cfg *c,
 		if (oidc_session_save(r, session, FALSE) == FALSE) {
 			oidc_warn(r, "error saving session");
 			rc = HTTP_INTERNAL_SERVER_ERROR;
+			goto end;
 		}
 	}
 
@@ -4118,6 +4164,8 @@ static int oidc_handle_info_request(request_rec *r, oidc_cfg *c,
 				apr_psprintf(r->pool, "<pre>%s</pre>", r_value), OK);
 	}
 
+end:
+
 	/* free the allocated resources */
 	json_decref(json);
 

src/mod_auth_openidc.h

@@ -505,6 +505,7 @@ typedef struct oidc_cfg {
 	char *logout_x_frame_options;
 	apr_byte_t x_forwarded_headers;
 	int action_on_userinfo_error;
+	oidc_cache_mutex_t *refresh_mutex;
 } oidc_cfg;
 
 void oidc_pre_config_init();