From 95baad3342aa7ef7172ad4922eb9f0d605dc469b Mon Sep 17 00:00:00 2001
From: Hans Zandbelt <hans.zandbelt@zmartzone.eu>
Date: Thu, 6 Dec 2018 14:55:44 +0100
Subject: [PATCH 12/13] optionally delete the oldest state cookie(s); see #399
bump to 2.3.10rc3
Signed-off-by: Hans Zandbelt <hans.zandbelt@zmartzone.eu>
(cherry picked from commit 46758a75eef8e3c91f2917fc7c6302136eb18809)
---
auth_openidc.conf | 8 +++--
src/config.c | 27 +++++++++++++----
src/mod_auth_openidc.c | 66 +++++++++++++++++++++++++++++++++++++++---
src/mod_auth_openidc.h | 2 ++
src/parse.c | 9 ++++--
src/parse.h | 2 +-
6 files changed, 100 insertions(+), 14 deletions(-)
diff --git a/auth_openidc.conf b/auth_openidc.conf
index 48dd027..33cea64 100644
--- a/auth_openidc.conf
+++ b/auth_openidc.conf
@@ -450,8 +450,12 @@
# authentication requests. See: https://github.com/zmartzone/mod_auth_openidc/issues/331
# Setting this to 0 means unlimited, until the browser or server gives up which is the
# behavior of mod_auth_openidc < 2.3.8, which did not have this configuration option.
-# When not defined, the default is 7.
-#OIDCStateMaxNumberOfCookies <number>
+#
+# The optional second boolean parameter if the oldest state cookie(s) will be deleted,
+# even if still valid; see #399.
+#
+# When not defined, the default is 7 and "false", thus the oldest cookie(s) will not be deleted.
+#OIDCStateMaxNumberOfCookies <number> [false|true]
########################################################################################
#
diff --git a/src/config.c b/src/config.c
index 6fa6227..8e56716 100644
--- a/src/config.c
+++ b/src/config.c
@@ -106,6 +106,8 @@
#define OIDC_DEFAULT_STATE_TIMEOUT 300
/* maximum number of parallel state cookies; 0 means unlimited, until the browser or server gives up */
#define OIDC_DEFAULT_MAX_NUMBER_OF_STATE_COOKIES 7
+/* default setting for deleting the oldest state cookies */
+#define OIDC_DEFAULT_DELETE_OLDEST_STATE_COOKIES 0
/* default session inactivity timeout */
#define OIDC_DEFAULT_SESSION_INACTIVITY_TIMEOUT 300
/* default session max duration */
@@ -1001,11 +1003,12 @@ static const char *oidc_set_client_auth_bearer_token(cmd_parms *cmd,
* set the maximun number of parallel state cookies
*/
static const char *oidc_set_max_number_of_state_cookies(cmd_parms *cmd,
- void *struct_ptr, const char *arg) {
+ void *struct_ptr, const char *arg1, const char *arg2) {
oidc_cfg *cfg = (oidc_cfg *) ap_get_module_config(
cmd->server->module_config, &auth_openidc_module);
- const char *rv = oidc_parse_max_number_of_state_cookies(cmd->pool, arg,
- &cfg->max_number_of_state_cookies);
+ const char *rv = oidc_parse_max_number_of_state_cookies(cmd->pool, arg1,
+ arg2, &cfg->max_number_of_state_cookies,
+ &cfg->delete_oldest_state_cookies);
return OIDC_CONFIG_DIR_RV(cmd, rv);
}
@@ -1018,6 +1021,15 @@ int oidc_cfg_max_number_of_state_cookies(oidc_cfg *cfg) {
return cfg->max_number_of_state_cookies;
}
+/*
+ * return the number of oldest state cookies that need to be deleted
+ */
+int oidc_cfg_delete_oldest_state_cookies(oidc_cfg *cfg) {
+ if (cfg->delete_oldest_state_cookies == OIDC_CONFIG_POS_INT_UNSET)
+ return OIDC_DEFAULT_DELETE_OLDEST_STATE_COOKIES;
+ return cfg->delete_oldest_state_cookies;
+}
+
/*
* create a new server config record with defaults
*/
@@ -1127,6 +1139,7 @@ void *oidc_create_server_config(apr_pool_t *pool, server_rec *svr) {
c->http_timeout_short = OIDC_DEFAULT_HTTP_TIMEOUT_SHORT;
c->state_timeout = OIDC_DEFAULT_STATE_TIMEOUT;
c->max_number_of_state_cookies = OIDC_CONFIG_POS_INT_UNSET;
+ c->delete_oldest_state_cookies = OIDC_CONFIG_POS_INT_UNSET;
c->session_inactivity_timeout = OIDC_DEFAULT_SESSION_INACTIVITY_TIMEOUT;
c->cookie_domain = NULL;
@@ -1445,6 +1458,10 @@ void *oidc_merge_server_config(apr_pool_t *pool, void *BASE, void *ADD) {
add->max_number_of_state_cookies != OIDC_CONFIG_POS_INT_UNSET ?
add->max_number_of_state_cookies :
base->max_number_of_state_cookies;
+ c->delete_oldest_state_cookies =
+ add->delete_oldest_state_cookies != OIDC_CONFIG_POS_INT_UNSET ?
+ add->delete_oldest_state_cookies :
+ base->delete_oldest_state_cookies;
c->session_inactivity_timeout =
add->session_inactivity_timeout
!= OIDC_DEFAULT_SESSION_INACTIVITY_TIMEOUT ?
@@ -2656,11 +2673,11 @@ const command_rec oidc_config_cmds[] = {
(void*)APR_OFFSETOF(oidc_cfg, state_timeout),
RSRC_CONF,
"Time to live in seconds for state parameter (cq. interval in which the authorization request and the corresponding response need to be completed)."),
- AP_INIT_TAKE1(OIDCStateMaxNumberOfCookies,
+ AP_INIT_TAKE12(OIDCStateMaxNumberOfCookies,
oidc_set_max_number_of_state_cookies,
(void*)APR_OFFSETOF(oidc_cfg, max_number_of_state_cookies),
RSRC_CONF,
- "Maximun number of parallel state cookies i.e. outstanding authorization requests."),
+ "Maximun number of parallel state cookies i.e. outstanding authorization requests and whether to delete the oldest cookie(s)."),
AP_INIT_TAKE1(OIDCSessionInactivityTimeout,
oidc_set_session_inactivity_timeout,
(void*)APR_OFFSETOF(oidc_cfg, session_inactivity_timeout),
diff --git a/src/mod_auth_openidc.c b/src/mod_auth_openidc.c
index 897a449..8740e02 100644
--- a/src/mod_auth_openidc.c
+++ b/src/mod_auth_openidc.c
@@ -686,15 +686,53 @@ static apr_byte_t oidc_unsolicited_proto_state(request_rec *r, oidc_cfg *c,
return TRUE;
}
+typedef struct oidc_state_cookies_t {
+ char *name;
+ apr_time_t timestamp;
+ struct oidc_state_cookies_t *next;
+} oidc_state_cookies_t;
+
+static int oidc_delete_oldest_state_cookies(request_rec *r,
+ int number_of_valid_state_cookies, int max_number_of_state_cookies,
+ oidc_state_cookies_t *first) {
+ oidc_state_cookies_t *cur = NULL, *prev = NULL, *prev_oldest = NULL,
+ *oldest = NULL;
+ while (number_of_valid_state_cookies >= max_number_of_state_cookies) {
+ oldest = first;
+ prev_oldest = NULL;
+ prev = first;
+ cur = first->next;
+ while (cur) {
+ if ((cur->timestamp < oldest->timestamp)) {
+ oldest = cur;
+ prev_oldest = prev;
+ }
+ prev = cur;
+ cur = cur->next;
+ }
+ oidc_warn(r,
+ "deleting oldest state cookie: %s (time until expiry " APR_TIME_T_FMT " seconds)",
+ oldest->name, apr_time_sec(oldest->timestamp - apr_time_now()));
+ oidc_util_set_cookie(r, oldest->name, "", 0, NULL);
+ if (prev_oldest)
+ prev_oldest->next = oldest->next;
+ else
+ first = first->next;
+ number_of_valid_state_cookies--;
+ }
+ return number_of_valid_state_cookies;
+}
+
/*
* clean state cookies that have expired i.e. for outstanding requests that will never return
* successfully and return the number of remaining valid cookies/outstanding-requests while
* doing so
*/
static int oidc_clean_expired_state_cookies(request_rec *r, oidc_cfg *c,
- const char *currentCookieName) {
+ const char *currentCookieName, int delete_oldest) {
int number_of_valid_state_cookies = 0;
- char *cookie, *tokenizerCtx;
+ oidc_state_cookies_t *first = NULL, *last = NULL;
+ char *cookie, *tokenizerCtx = NULL;
char *cookies = apr_pstrdup(r->pool, oidc_util_hdr_in_cookie_get(r));
if (cookies != NULL) {
cookie = apr_strtok(cookies, OIDC_STR_SEMI_COLON, &tokenizerCtx);
@@ -722,6 +760,18 @@ static int oidc_clean_expired_state_cookies(request_rec *r, oidc_cfg *c,
oidc_util_set_cookie(r, cookieName, "", 0,
NULL);
} else {
+ if (first == NULL) {
+ first = apr_pcalloc(r->pool,
+ sizeof(oidc_state_cookies_t));
+ last = first;
+ } else {
+ last->next = apr_pcalloc(r->pool,
+ sizeof(oidc_state_cookies_t));
+ last = last->next;
+ }
+ last->name = cookieName;
+ last->timestamp = ts;
+ last->next = NULL;
number_of_valid_state_cookies++;
}
oidc_proto_state_destroy(proto_state);
@@ -732,6 +782,12 @@ static int oidc_clean_expired_state_cookies(request_rec *r, oidc_cfg *c,
cookie = apr_strtok(NULL, OIDC_STR_SEMI_COLON, &tokenizerCtx);
}
}
+
+ if (delete_oldest > 0)
+ number_of_valid_state_cookies = oidc_delete_oldest_state_cookies(r,
+ number_of_valid_state_cookies, c->max_number_of_state_cookies,
+ first);
+
return number_of_valid_state_cookies;
}
@@ -746,7 +802,7 @@ static apr_byte_t oidc_restore_proto_state(request_rec *r, oidc_cfg *c,
const char *cookieName = oidc_get_state_cookie_name(r, state);
/* clean expired state cookies to avoid pollution */
- oidc_clean_expired_state_cookies(r, c, cookieName);
+ oidc_clean_expired_state_cookies(r, c, cookieName, FALSE);
/* get the state cookie value first */
char *cookieValue = oidc_util_get_cookie(r, cookieName);
@@ -820,10 +876,12 @@ static int oidc_authorization_request_set_cookie(request_rec *r, oidc_cfg *c,
* clean expired state cookies to avoid pollution and optionally
* try to avoid the number of state cookies exceeding a max
*/
- int number_of_cookies = oidc_clean_expired_state_cookies(r, c, NULL);
+ int number_of_cookies = oidc_clean_expired_state_cookies(r, c, NULL,
+ oidc_cfg_delete_oldest_state_cookies(c));
int max_number_of_cookies = oidc_cfg_max_number_of_state_cookies(c);
if ((max_number_of_cookies > 0)
&& (number_of_cookies >= max_number_of_cookies)) {
+
oidc_warn(r,
"the number of existing, valid state cookies (%d) has exceeded the limit (%d), no additional authorization request + state cookie can be generated, aborting the request",
number_of_cookies, max_number_of_cookies);
diff --git a/src/mod_auth_openidc.h b/src/mod_auth_openidc.h
index f89f392..c3a0a23 100644
--- a/src/mod_auth_openidc.h
+++ b/src/mod_auth_openidc.h
@@ -380,6 +380,7 @@ typedef struct oidc_cfg {
int http_timeout_short;
int state_timeout;
int max_number_of_state_cookies;
+ int delete_oldest_state_cookies;
int session_inactivity_timeout;
int session_cache_fallback_to_cookie;
@@ -691,6 +692,7 @@ int oidc_cfg_session_cache_fallback_to_cookie(request_rec *r);
const char *oidc_parse_pkce_type(apr_pool_t *pool, const char *arg, oidc_proto_pkce_t **type);
const char *oidc_cfg_claim_prefix(request_rec *r);
int oidc_cfg_max_number_of_state_cookies(oidc_cfg *cfg);
+int oidc_cfg_delete_oldest_state_cookies(oidc_cfg *cfg);
// oidc_util.c
int oidc_strnenvcmp(const char *a, const char *b, int len);
diff --git a/src/parse.c b/src/parse.c
index 0f986fd..2d09584 100644
--- a/src/parse.c
+++ b/src/parse.c
@@ -1245,7 +1245,12 @@ const char *oidc_parse_auth_request_method(apr_pool_t *pool, const char *arg,
* parse the maximum number of parallel state cookies
*/
const char *oidc_parse_max_number_of_state_cookies(apr_pool_t *pool,
- const char *arg, int *int_value) {
- return oidc_parse_int_valid(pool, arg, int_value,
+ const char *arg1, const char *arg2, int *int_value, int *bool_value) {
+ const char *rv = NULL;
+
+ rv = oidc_parse_int_valid(pool, arg1, int_value,
oidc_valid_max_number_of_state_cookies);
+ if ((rv == NULL) && (arg2 != NULL))
+ rv = oidc_parse_boolean(pool, arg2, bool_value);
+ return rv;
}
diff --git a/src/parse.h b/src/parse.h
index 6355db4..bdf5651 100644
--- a/src/parse.h
+++ b/src/parse.h
@@ -117,7 +117,7 @@ const char *oidc_parse_info_hook_data(apr_pool_t *pool, const char *arg, apr_has
const char *oidc_parse_token_binding_policy(apr_pool_t *pool, const char *arg, int *int_value);
const char *oidc_token_binding_policy2str(apr_pool_t *pool, int v);
const char *oidc_parse_auth_request_method(apr_pool_t *pool, const char *arg, int *method);
-const char *oidc_parse_max_number_of_state_cookies(apr_pool_t *pool, const char *arg, int *int_value);
+const char *oidc_parse_max_number_of_state_cookies(apr_pool_t *pool, const char *arg1, const char *arg2, int *int_value, int *bool_value);
typedef const char *(*oidc_valid_int_function_t)(apr_pool_t *, int);
typedef const char *(*oidc_valid_function_t)(apr_pool_t *, const char *);
--
2.26.2