allow specific settings Strict|Lax|None|Disabled for OIDCCookieSameSite

- fix: default behaviour Lax
- fix: apply OIDCCookieSameSite Off/None properly to state cookies
- re-introduce the option to configure a Strict SameSite session cookie
- allows for a "Disabled" value that does not set any SameSite flag

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

ChangeLog

@@ -1,3 +1,13 @@
+11/14/2024
+- allow specific settings Strict|Lax|None|Disabled for OIDCCookieSameSite in addition to On(=Lax)|Off(=None)
+  - fix: default behaviour Lax
+  - fix: apply OIDCCookieSameSite Off/None properly to state cookies instead of always setting Lax
+  - re-introduces the option to configure a Strict SameSite session cookie policy, which will turn the initial
+    Lax session cookie - set upon receving the response to the Redirect URI - into a Strict session cookie
+    immediately after the first application request
+  - allows for a "Disabled" value that does not set any SameSite flag on the cookies, in which case a browser
+    falls back to its default browser behaviour (which should be Lax by spec)
+
 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")

auth_openidc.conf

@@ -555,18 +555,31 @@
 # When not defined the default is On.
 #OIDCCookieHTTPOnly [On|Off]
 
-# Defines whether the SameSite flag will be set on cookies.
-# When On the following will apply:
-#   state cookie: Lax
+# Defines the SameSite flag that will be set on cookies.
+#
+# When set to "On" (default) or "Lax" the following will apply:
 #   session cookie: Lax
-#   x_csrf discovery: Strict:
+#   state cookie: Lax
+#   x_csrf discovery: Lax
+#
+# When set to "Strict" the following will apply:
+#   session cookie: Strict (first time: Lax)
+#   state cookie: Lax
+#   x_csrf discovery: Strict
+#
+# When set to "Off" or "None" the following will apply:
+#   session cookie: None
+#   state cookie: None
+#   x_csrf discovery: None
 #
-# The default `SameSite=None` cookie appendix on `Set-Cookie` response headers can be 
+# When set to "Disabled" no SameSite flag will be appended.
+#
+# The configured SameSite cookie appendix on `Set-Cookie` response headers can be 
 # conditionally overridden using an environment variable in the Apache config as in:
 #   SetEnvIf User-Agent ".*IOS.*" OIDC_SET_COOKIE_APPEND=;
 #
-# When not defined the default is On.
-#OIDCCookieSameSite [On|Off]
+# When not defined the default is On (Lax).
+#OIDCCookieSameSite [ On | Off | Strict | Lax | None | Disabled ]
 
 # Specify the names of cookies to pickup from the browser and send along on backchannel
 # calls to the OP and AS endpoints. This can be used for load-balancing purposes.

src/cfg/cfg.c

@@ -521,8 +521,32 @@ OIDC_CFG_MEMBER_FUNC_GET(metadata_dir, const char *)
 #define OIDC_DEFAULT_COOKIE_HTTPONLY 1
 OIDC_CFG_MEMBER_FUNCS_BOOL(cookie_http_only, OIDC_DEFAULT_COOKIE_HTTPONLY)
 
-#define OIDC_DEFAULT_COOKIE_SAME_SITE 1
-OIDC_CFG_MEMBER_FUNCS_BOOL(cookie_same_site, OIDC_DEFAULT_COOKIE_SAME_SITE)
+#define OIDC_SAMESITE_COOKIE_OFF_STR "Off"
+#define OIDC_SAMESITE_COOKIE_ON_STR "On"
+#define OIDC_SAMESITE_COOKIE_DISABLED_STR "Disabled"
+#define OIDC_SAMESITE_COOKIE_NONE_STR "None"
+#define OIDC_SAMESITE_COOKIE_LAX_STR "Lax"
+#define OIDC_SAMESITE_COOKIE_STRICT_STR "Strict"
+
+/*
+ * define which header we use for calculating the fingerprint of the state during authentication
+ */
+const char *oidc_cmd_cookie_same_site_set(cmd_parms *cmd, void *m, const char *arg) {
+	oidc_cfg_t *cfg = (oidc_cfg_t *)ap_get_module_config(cmd->server->module_config, &auth_openidc_module);
+	// NB: On is made equal to Lax here and Off is equal to None (backwards compatibility)
+	static const oidc_cfg_option_t options[] = {{OIDC_SAMESITE_COOKIE_NONE, OIDC_SAMESITE_COOKIE_OFF_STR},
+						    {OIDC_SAMESITE_COOKIE_LAX, OIDC_SAMESITE_COOKIE_ON_STR},
+						    {OIDC_SAMESITE_COOKIE_DISABLED, OIDC_SAMESITE_COOKIE_DISABLED_STR},
+						    {OIDC_SAMESITE_COOKIE_NONE, OIDC_SAMESITE_COOKIE_NONE_STR},
+						    {OIDC_SAMESITE_COOKIE_LAX, OIDC_SAMESITE_COOKIE_LAX_STR},
+						    {OIDC_SAMESITE_COOKIE_STRICT, OIDC_SAMESITE_COOKIE_STRICT_STR}};
+	const char *rv = oidc_cfg_parse_option_ignore_case(cmd->pool, options, OIDC_CFG_OPTIONS_SIZE(options), arg,
+							   (int *)&cfg->cookie_same_site);
+	return OIDC_CONFIG_DIR_RV(cmd, rv);
+}
+
+#define OIDC_DEFAULT_COOKIE_SAME_SITE OIDC_SAMESITE_COOKIE_LAX
+OIDC_CFG_MEMBER_FUNC_TYPE_GET(cookie_same_site, oidc_samesite_cookie_t, OIDC_DEFAULT_COOKIE_SAME_SITE)
 
 #define OIDC_DEFAULT_SESSION_FALLBACK_TO_COOKIE 0
 OIDC_CFG_MEMBER_FUNCS_BOOL(session_cache_fallback_to_cookie, OIDC_DEFAULT_SESSION_FALLBACK_TO_COOKIE)

src/cfg/cfg.h

@@ -111,6 +111,13 @@ typedef enum {
 	OIDC_TRACE_PARENT_GENERATE = 2
 } oidc_trace_parent_t;
 
+typedef enum {
+	OIDC_SAMESITE_COOKIE_DISABLED = 0,
+	OIDC_SAMESITE_COOKIE_NONE = 1,
+	OIDC_SAMESITE_COOKIE_LAX = 2,
+	OIDC_SAMESITE_COOKIE_STRICT = 3
+} oidc_samesite_cookie_t;
+
 #define OIDC_ERROR_ENVVAR "OIDC_ERROR"
 #define OIDC_ERROR_DESC_ENVVAR "OIDC_ERROR_DESC"
 
@@ -214,7 +221,7 @@ OIDC_CFG_MEMBER_FUNCS_DECL(default_sso_url, const char *)
 OIDC_CFG_MEMBER_FUNCS_DECL(default_slo_url, const char *)
 OIDC_CFG_MEMBER_FUNCS_DECL(cookie_domain, const char *)
 OIDC_CFG_MEMBER_FUNCS_DECL(cookie_http_only, int)
-OIDC_CFG_MEMBER_FUNCS_DECL(cookie_same_site, int)
+OIDC_CFG_MEMBER_FUNCS_DECL(cookie_same_site, oidc_samesite_cookie_t)
 OIDC_CFG_MEMBER_FUNCS_DECL(claim_delimiter, const char *)
 OIDC_CFG_MEMBER_FUNCS_DECL(claim_prefix, const char *)
 OIDC_CFG_MEMBER_FUNCS_DECL(state_timeout, int)

src/cfg/cfg_int.h

@@ -150,7 +150,7 @@ struct oidc_cfg_t {
 	int session_cookie_chunk_size;
 	char *cookie_domain;
 	int cookie_http_only;
-	int cookie_same_site;
+	oidc_samesite_cookie_t cookie_same_site;
 
 	int state_timeout;
 	int max_number_of_state_cookies;

src/cfg/parse.c

@@ -106,9 +106,10 @@ char *oidc_cfg_parse_options_flatten(apr_pool_t *pool, const oidc_cfg_option_t o
 /*
  * parse an value provided as an option string into the corresponding integer/enum
  */
-char *oidc_cfg_parse_option(apr_pool_t *pool, const oidc_cfg_option_t options[], int n, const char *arg, int *v) {
+static char *oidc_cfg_parse_option_impl(apr_pool_t *pool, const oidc_cfg_option_t options[], int n, const char *arg,
+					int *v, int (*fstrcmp)(const char *, const char *)) {
 	int i = 0;
-	while ((i < n) && (_oidc_strcmp(arg, options[i].str) != 0))
+	while ((i < n) && (fstrcmp(arg, options[i].str) != 0))
 		i++;
 	if (i < n) {
 		*v = options[i].val;
@@ -118,6 +119,21 @@ char *oidc_cfg_parse_option(apr_pool_t *pool, const oidc_cfg_option_t options[],
 			    OIDC_LIST_OPTIONS_QUOTE, oidc_cfg_parse_options_flatten(pool, options, n));
 }
 
+/*
+ * parse an value provided as an option string into the corresponding integer/enum case sensitive
+ */
+char *oidc_cfg_parse_option(apr_pool_t *pool, const oidc_cfg_option_t options[], int n, const char *arg, int *v) {
+	return oidc_cfg_parse_option_impl(pool, options, n, arg, v, _oidc_strcmp);
+}
+
+/*
+ * parse an value provided as an option string into the corresponding integer/enum case insensitive
+ */
+char *oidc_cfg_parse_option_ignore_case(apr_pool_t *pool, const oidc_cfg_option_t options[], int n, const char *arg,
+					int *v) {
+	return oidc_cfg_parse_option_impl(pool, options, n, arg, v, _oidc_strnatcasecmp);
+}
+
 /*
  * check if the provided integer value is between a specified minimum and maximum
  */

src/cfg/parse.h

@@ -51,6 +51,8 @@ typedef struct oidc_cfg_option_t {
 } oidc_cfg_option_t;
 
 char *oidc_cfg_parse_option(apr_pool_t *pool, const oidc_cfg_option_t options[], int n, const char *arg, int *v);
+char *oidc_cfg_parse_option_ignore_case(apr_pool_t *pool, const oidc_cfg_option_t options[], int n, const char *arg,
+					int *v);
 char *oidc_cfg_parse_options_flatten(apr_pool_t *pool, const oidc_cfg_option_t options[], int n);
 
 char *oidc_cfg_parse_flatten_options(apr_pool_t *pool, const char *options[]);

src/handle/discovery.c

@@ -74,12 +74,29 @@ apr_byte_t oidc_is_discovery_response(request_rec *r, oidc_cfg_t *cfg) {
 	       oidc_util_request_has_parameter(r, OIDC_DISC_USER_PARAM);
 }
 
+static const char *oidc_discovery_csrf_cookie_samesite(request_rec *r, oidc_cfg_t *c) {
+	const char *rv = NULL;
+	switch (oidc_cfg_cookie_same_site_get(c)) {
+	case OIDC_SAMESITE_COOKIE_STRICT:
+		rv = OIDC_HTTP_COOKIE_SAMESITE_STRICT;
+		break;
+	case OIDC_SAMESITE_COOKIE_LAX:
+		rv = OIDC_HTTP_COOKIE_SAMESITE_LAX;
+		break;
+	case OIDC_SAMESITE_COOKIE_NONE:
+		rv = OIDC_HTTP_COOKIE_SAMESITE_NONE(c, r);
+		break;
+	case OIDC_SAMESITE_COOKIE_DISABLED:
+		break;
+	default:
+		break;
+	}
+	return rv;
+}
+
 /* define the name of the cookie/parameter for CSRF protection */
 #define OIDC_CSRF_NAME "x_csrf"
 
-#define OIDC_COOKIE_SAMESITE_STRICT(c, r)                                                                              \
-	oidc_cfg_cookie_same_site_get(c) ? OIDC_COOKIE_EXT_SAME_SITE_STRICT : OIDC_COOKIE_EXT_SAME_SITE_NONE(c, r)
-
 /*
  * present the user with an OP selection screen
  */
@@ -122,7 +139,7 @@ int oidc_discovery_request(request_rec *r, oidc_cfg_t *cfg) {
 		oidc_debug(r, "redirecting to external discovery page: %s", url);
 
 		/* set CSRF cookie */
-		oidc_http_set_cookie(r, OIDC_CSRF_NAME, csrf, -1, OIDC_COOKIE_SAMESITE_STRICT(cfg, r));
+		oidc_http_set_cookie(r, OIDC_CSRF_NAME, csrf, -1, oidc_discovery_csrf_cookie_samesite(r, cfg));
 
 		/* see if we need to preserve POST parameters through Javascript/HTML5 storage */
 		if (oidc_response_post_preserve_javascript(r, url, NULL, NULL) == TRUE)
@@ -200,7 +217,7 @@ int oidc_discovery_request(request_rec *r, oidc_cfg_t *cfg) {
 	s = apr_psprintf(r->pool, "%s<p><input type=\"submit\" value=\"Submit\"></p>\n", s);
 	s = apr_psprintf(r->pool, "%s</form>\n", s);
 
-	oidc_http_set_cookie(r, OIDC_CSRF_NAME, csrf, -1, OIDC_COOKIE_SAMESITE_STRICT(cfg, r));
+	oidc_http_set_cookie(r, OIDC_CSRF_NAME, csrf, -1, oidc_discovery_csrf_cookie_samesite(r, cfg));
 
 	char *javascript = NULL, *javascript_method = NULL;
 	char *html_head = "<style type=\"text/css\">body {text-align: center}</style>";
@@ -307,7 +324,7 @@ int oidc_discovery_response(request_rec *r, oidc_cfg_t *c) {
 	if (csrf_cookie) {
 
 		/* clean CSRF cookie */
-		oidc_http_set_cookie(r, OIDC_CSRF_NAME, "", 0, OIDC_COOKIE_EXT_SAME_SITE_NONE(c, r));
+		oidc_http_set_cookie(r, OIDC_CSRF_NAME, "", 0, OIDC_HTTP_COOKIE_SAMESITE_NONE(c, r));
 
 		/* compare CSRF cookie value with query parameter value */
 		if ((csrf_query == NULL) || _oidc_strcmp(csrf_query, csrf_cookie) != 0) {

src/handle/request.c

@@ -98,8 +98,22 @@ static int oidc_request_check_cookie_domain(request_rec *r, oidc_cfg_t *c, oidc_
 	return OK;
 }
 
-#define OIDC_COOKIE_SAMESITE_LAX(c, r)                                                                                 \
-	oidc_cfg_cookie_same_site_get(c) ? OIDC_COOKIE_EXT_SAME_SITE_LAX : OIDC_COOKIE_EXT_SAME_SITE_NONE(c, r)
+static const char *oidc_request_samesite_cookie(request_rec *r, struct oidc_cfg_t *c) {
+	const char *rv = NULL;
+	switch (oidc_cfg_cookie_same_site_get(c)) {
+	case OIDC_SAMESITE_COOKIE_STRICT:
+	case OIDC_SAMESITE_COOKIE_LAX:
+		rv = OIDC_HTTP_COOKIE_SAMESITE_LAX;
+		break;
+	case OIDC_SAMESITE_COOKIE_NONE:
+		rv = OIDC_HTTP_COOKIE_SAMESITE_NONE(c, r);
+		break;
+	case OIDC_SAMESITE_COOKIE_DISABLED:
+	default:
+		break;
+	}
+	return rv;
+}
 
 /*
  * set the state that is maintained between an authorization request and an authorization response
@@ -135,7 +149,7 @@ static int oidc_request_authorization_set_cookie(request_rec *r, oidc_cfg_t *c,
 	const char *cookieName = oidc_state_cookie_name(r, state);
 
 	/* set it as a cookie */
-	oidc_http_set_cookie(r, cookieName, cookieValue, -1, OIDC_COOKIE_SAMESITE_LAX(c, r));
+	oidc_http_set_cookie(r, cookieName, cookieValue, -1, oidc_request_samesite_cookie(r, c));
 
 	return OK;
 }

src/handle/response.c

@@ -327,6 +327,9 @@ apr_byte_t oidc_response_save_in_session(request_rec *r, oidc_cfg_t *c, oidc_ses
 		sid = id_token_jwt->payload.sub;
 	session->sid = oidc_response_make_sid_iss_unique(r, sid, oidc_cfg_provider_issuer_get(provider));
 
+	/* indicate that this is a newly created session */
+	oidc_session_set_session_new(r, session, 1);
+
 	/* store the session */
 	return oidc_session_save(r, session, TRUE);
 }
@@ -352,7 +355,7 @@ static apr_byte_t oidc_response_proto_state_restore(request_rec *r, oidc_cfg_t *
 	}
 
 	/* clear state cookie because we don't need it anymore */
-	oidc_http_set_cookie(r, cookieName, "", 0, OIDC_COOKIE_EXT_SAME_SITE_NONE(c, r));
+	oidc_http_set_cookie(r, cookieName, "", 0, OIDC_HTTP_COOKIE_SAMESITE_NONE(c, r));
 
 	*proto_state = oidc_proto_state_from_cookie(r, c, cookieValue);
 	if (*proto_state == NULL)

src/http.h

@@ -94,6 +94,10 @@
 #define OIDC_HTTP_HDR_VAL_NAVIGATE "navigate"
 #define OIDC_HTTP_HDR_VAL_DOCUMENT "document"
 
+#define OIDC_HTTP_COOKIE_SAMESITE_LAX "SameSite=Lax"
+#define OIDC_HTTP_COOKIE_SAMESITE_STRICT "SameSite=Strict"
+#define OIDC_HTTP_COOKIE_SAMESITE_NONE(c, r) oidc_util_request_is_secure(r, c) ? "SameSite=None" : NULL
+
 typedef struct oidc_http_timeout_t {
 	int request_timeout; // in seconds
 	int connect_timeout; // in seconds

src/mod_auth_openidc.c

@@ -662,6 +662,12 @@ apr_byte_t oidc_session_pass_tokens(request_rec *r, oidc_cfg_t *cfg, oidc_sessio
 		}
 	}
 
+	// if this is a newly created session, we'll write it again to update the samesite setting on the session cookie
+	if (oidc_session_get_session_new(r, session)) {
+		*needs_save = TRUE;
+		oidc_session_set_session_new(r, session, 0);
+	}
+
 	/* log message about session expiry */
 	oidc_log_session_expires(r, "session inactivity timeout", session->expiry);
 

src/mod_auth_openidc.h

@@ -132,10 +132,6 @@
 #define OIDC_APP_INFO_USERINFO_JWT "userinfo_jwt"
 #define OIDC_APP_INFO_SIGNED_JWT "signed_jwt"
 
-#define OIDC_COOKIE_EXT_SAME_SITE_LAX "SameSite=Lax"
-#define OIDC_COOKIE_EXT_SAME_SITE_STRICT "SameSite=Strict"
-#define OIDC_COOKIE_EXT_SAME_SITE_NONE(c, r) oidc_util_request_is_secure(r, c) ? "SameSite=None" : NULL
-
 int oidc_check_user_id(request_rec *r);
 int oidc_fixups(request_rec *r);
 apr_byte_t oidc_enabled(request_rec *r);