deprecate OIDCHTMLErrorTemplate

rely on standard Apache error handling capabilities; by default environment variable strings REDIRECT_OIDC_ERROR and REDIRECT_OIDC_ERROR_DESC are available in ErrorDocument backwards compatibility is retained by setting "OIDCHTMLErrorTemplate deprecated"; bump to 2.4.14rc0 Signed-off-by: Hans Zandbelt <[email protected]>
OpenIDC · Apr 7, 2023 · 996cc08 · 996cc08
1 parent ee4efbe
commit 996cc08
Show file tree

Hide file tree

Showing 6 changed files with 147 additions and 82 deletions.
diff --git a/ChangeLog b/ChangeLog
@@ -1,3 +1,9 @@
+04/07/2023
+- deprecate OIDCHTMLErrorTemplate and rely on standard Apache error handling capabilities by default
+  environment variable strings REDIRECT_OIDC_ERROR and REDIRECT_OIDC_ERROR_DESC are available in ErrorDocument
+  backwards compatibility is retained by setting "OIDCHTMLErrorTemplate deprecated"
+- bump to 2.4.14rc0
+
 04/07/2023
 - add support for passing on claims resolved from the userinfo endpoint in a JWT signed by
   mod_auth_openidc using `OIDCPassUserInfoAs signed_jwt[:<name>]`  with the keys configured

diff --git a/configure.ac b/configure.ac
@@ -1,4 +1,4 @@
-AC_INIT([mod_auth_openidc],[2.4.13.3rc3],[[email protected]])
+AC_INIT([mod_auth_openidc],[2.4.14rc0],[[email protected]])
 
 AC_SUBST(NAMEVER, AC_PACKAGE_TARNAME()-AC_PACKAGE_VERSION())
 

diff --git a/src/config.c b/src/config.c
@@ -2150,6 +2150,15 @@ int oidc_cfg_session_cache_fallback_to_cookie(request_rec *r) {
 	return cfg->session_cache_fallback_to_cookie;
 }
 
+static const char* oidc_set_html_error_template(cmd_parms *cmd,
+		void *struct_ptr, const char *arg) {
+	oidc_cfg *cfg = (oidc_cfg*) ap_get_module_config(cmd->server->module_config,
+			&auth_openidc_module);
+	oidc_swarn(cmd->server,
+			OIDCHTMLErrorTemplate" is deprecated; please use the standard Apache features to deal with the "OIDC_ERROR_ENVVAR" and "OIDC_ERROR_DESC_ENVVAR" environment variables set by this module, see: https://httpd.apache.org/docs/2.4/custom-error.html");
+	return ap_set_string_slot(cmd, cfg, arg);
+}
+
 /*
  * create a new directory config record with defaults
  */
@@ -3550,7 +3559,7 @@ const command_rec oidc_config_cmds[] = {
 				"Timeout waiting for a response of the Redis servers."),
 #endif
 		AP_INIT_TAKE1(OIDCHTMLErrorTemplate,
-				oidc_set_string_slot,
+				oidc_set_html_error_template,
 				(void*)APR_OFFSETOF(oidc_cfg, error_template),
 				RSRC_CONF,
 				"Name of a HTML error template: needs to contain two \"%s\" characters, one for the error message, one for the description."),

diff --git a/src/mod_auth_openidc.c b/src/mod_auth_openidc.c
@@ -611,8 +611,8 @@ static int oidc_clean_expired_state_cookies(request_rec *r, oidc_cfg *c,
 /*
  * restore the state that was maintained between authorization request and response in an encrypted cookie
  */
-static apr_byte_t oidc_restore_proto_state(request_rec *r, oidc_cfg *c, const char *state,
-		oidc_proto_state_t **proto_state) {
+static apr_byte_t oidc_restore_proto_state(request_rec *r, oidc_cfg *c,
+		const char *state, oidc_proto_state_t **proto_state) {
 
 	oidc_debug(r, "enter");
 
@@ -624,12 +624,15 @@ static apr_byte_t oidc_restore_proto_state(request_rec *r, oidc_cfg *c, const ch
 	/* get the state cookie value first */
 	char *cookieValue = oidc_util_get_cookie(r, cookieName);
 	if (cookieValue == NULL) {
-		oidc_error(r, "no \"%s\" state cookie found: check domain and samesite cookie settings", cookieName);
+		oidc_error(r,
+				"no \"%s\" state cookie found: check domain and samesite cookie settings",
+				cookieName);
 		return FALSE;
 	}
 
 	/* clear state cookie because we don't need it anymore */
-	oidc_util_set_cookie(r, cookieName, "", 0, OIDC_COOKIE_EXT_SAME_SITE_NONE(c, r));
+	oidc_util_set_cookie(r, cookieName, "", 0,
+			OIDC_COOKIE_EXT_SAME_SITE_NONE(c, r));
 
 	*proto_state = oidc_proto_state_from_cookie(r, c, cookieValue);
 	if (*proto_state == NULL)
@@ -641,7 +644,9 @@ static apr_byte_t oidc_restore_proto_state(request_rec *r, oidc_cfg *c, const ch
 	char *calc = oidc_get_browser_state_hash(r, c, nonce);
 	/* compare the calculated hash with the value provided in the authorization response */
 	if (_oidc_strcmp(calc, state) != 0) {
-		oidc_error(r, "calculated state from cookie does not match state parameter passed back in URL: \"%s\" != \"%s\"", state, calc);
+		oidc_error(r,
+				"calculated state from cookie does not match state parameter passed back in URL: \"%s\" != \"%s\"",
+				state, calc);
 		oidc_proto_state_destroy(*proto_state);
 		return FALSE;
 	}
@@ -652,14 +657,14 @@ static apr_byte_t oidc_restore_proto_state(request_rec *r, oidc_cfg *c, const ch
 	if (apr_time_now() > ts + apr_time_from_sec(c->state_timeout)) {
 		oidc_error(r, "state has expired");
 		if ((c->default_sso_url == NULL)
-				|| (apr_table_get(r->subprocess_env, "OIDC_NO_DEFAULT_URL_ON_STATE_TIMEOUT") != NULL)) {
-			oidc_util_html_send_error(r, c->error_template, "Invalid Authentication Response", apr_psprintf(r->pool, "This is due to a timeout; please restart your authentication session by re-entering the URL/bookmark you originally wanted to access: %s", oidc_proto_state_get_original_url(*proto_state)),
-									  OK);
-			/*
-			 * a hack for Apache 2.4 to prevent it from writing its own 500/400/302 HTML document
-			 * text by making ap_send_error_response in http_protocol.c return early...
-			 */
-			r->header_only = 1;
+				|| (apr_table_get(r->subprocess_env,
+						"OIDC_NO_DEFAULT_URL_ON_STATE_TIMEOUT") != NULL)) {
+			oidc_util_html_send_error(r, c->error_template,
+					"Invalid Authentication Response",
+					apr_psprintf(r->pool,
+							"This is due to a timeout; please restart your authentication session by re-entering the URL/bookmark you originally wanted to access: %s",
+							oidc_proto_state_get_original_url(*proto_state)),
+							OK);
 		}
 		oidc_proto_state_destroy(*proto_state);
 		return FALSE;
@@ -669,7 +674,8 @@ static apr_byte_t oidc_restore_proto_state(request_rec *r, oidc_cfg *c, const ch
 	oidc_proto_state_set_state(*proto_state, state);
 
 	/* log the restored state object */
-	oidc_debug(r, "restored state: %s", oidc_proto_state_to_string(r, *proto_state));
+	oidc_debug(r, "restored state: %s",
+			oidc_proto_state_to_string(r, *proto_state));
 
 	/* we've made it */
 	return TRUE;
@@ -1997,8 +2003,8 @@ static int oidc_handle_authorization_response(request_rec *r, oidc_cfg *c,
 
 	/* match the returned state parameter against the state stored in the browser */
 	if (oidc_authorization_response_match_state(r, c,
-			apr_table_get(params, OIDC_PROTO_STATE), &provider, &proto_state)
-			== FALSE) {
+			apr_table_get(params, OIDC_PROTO_STATE), &provider,
+			&proto_state) == FALSE) {
 		if (c->default_sso_url != NULL) {
 			oidc_warn(r,
 					"invalid authorization response state; a default SSO URL is set, sending the user there: %s",
@@ -2009,11 +2015,21 @@ static int oidc_handle_authorization_response(request_rec *r, oidc_cfg *c,
 		}
 		oidc_error(r,
 				"invalid authorization response state and no default SSO URL is set, sending an error...");
-		// if content was already returned via html/http send then don't return 500
-		// but send 200 to avoid extraneous internal error document text to be sent
-		return ((r->user) && (_oidc_strncmp(r->user, "", 1) == 0)) ?
-				OK :
-				HTTP_BAD_REQUEST;
+
+		// detect if we've set a (timeout) error message in oidc_authorization_response_match_state
+		if (apr_table_get(r->subprocess_env, OIDC_ERROR_ENVVAR) != NULL) {
+			// overrides the OK from the timeout detection
+			return HTTP_BAD_REQUEST;
+		} else if (c->error_template) {
+			// for backwards compatibility
+			if ((r->user) && (_oidc_strncmp(r->user, "", 1) == 0))
+				return HTTP_BAD_REQUEST;
+		}
+
+		return oidc_util_html_send_error(r, c->error_template,
+				"Invalid Authorization Response",
+				"Could not match the authorization response to an earlier request via the state parameter and corresponding state cookie",
+				HTTP_BAD_REQUEST);
 	}
 
 	/* see if the response is an error response */
@@ -2088,7 +2104,8 @@ static int oidc_handle_authorization_response(request_rec *r, oidc_cfg *c,
 			return HTTP_INTERNAL_SERVER_ERROR;
 		}
 
-		oidc_debug(r, "set remote_user to \"%s\" in new session \"%s\"", r->user, session->uuid);
+		oidc_debug(r, "set remote_user to \"%s\" in new session \"%s\"",
+				r->user, session->uuid);
 
 	} else {
 		oidc_error(r, "remote user could not be set");
@@ -4186,57 +4203,71 @@ static authz_status oidc_handle_unauthorized_user24(request_rec *r) {
 
 	oidc_debug(r, "enter");
 
-	oidc_cfg *c = ap_get_module_config(r->server->module_config, &auth_openidc_module);
+	oidc_cfg *c = ap_get_module_config(r->server->module_config,
+			&auth_openidc_module);
 	char *html_head = NULL;
 
 	if (apr_strnatcasecmp((const char*) ap_auth_type(r),
-						  OIDC_AUTH_TYPE_OPENID_OAUTH20) == 0) {
-		oidc_debug(r, "setting environment variable %s to \"%s\" for usage in mod_headers", OIDC_OAUTH_BEARER_SCOPE_ERROR, OIDC_OAUTH_BEARER_SCOPE_ERROR_VALUE);
-		apr_table_set(r->subprocess_env, OIDC_OAUTH_BEARER_SCOPE_ERROR, OIDC_OAUTH_BEARER_SCOPE_ERROR_VALUE);
+			OIDC_AUTH_TYPE_OPENID_OAUTH20) == 0) {
+		oidc_debug(r,
+				"setting environment variable %s to \"%s\" for usage in mod_headers",
+				OIDC_OAUTH_BEARER_SCOPE_ERROR,
+				OIDC_OAUTH_BEARER_SCOPE_ERROR_VALUE);
+		apr_table_set(r->subprocess_env, OIDC_OAUTH_BEARER_SCOPE_ERROR,
+				OIDC_OAUTH_BEARER_SCOPE_ERROR_VALUE);
 		return AUTHZ_DENIED;
 	}
 
 	/* see if we've configured OIDCUnAutzAction for this path */
 	switch (oidc_dir_cfg_unautz_action(r)) {
-		case OIDC_UNAUTZ_RETURN403:
-		case OIDC_UNAUTZ_RETURN401:
-			if (oidc_dir_cfg_unauthz_arg(r)) {
-				oidc_util_html_send(r, "Authorization Error", NULL, NULL, oidc_dir_cfg_unauthz_arg(r),
-									HTTP_UNAUTHORIZED);
-				r->header_only = 1;
-			}
-			return AUTHZ_DENIED;
-		case OIDC_UNAUTZ_RETURN302:
-			html_head =
-					apr_psprintf(r->pool, "<meta http-equiv=\"refresh\" content=\"0; url=%s\">", oidc_dir_cfg_unauthz_arg(r));
-			oidc_util_html_send(r, "Authorization Error Redirect", html_head, NULL, NULL,
+	case OIDC_UNAUTZ_RETURN403:
+	case OIDC_UNAUTZ_RETURN401:
+		if (oidc_dir_cfg_unauthz_arg(r)) {
+			oidc_util_html_send_error(r, c->error_template,
+					"Authorization Error", oidc_dir_cfg_unauthz_arg(r),
 					HTTP_UNAUTHORIZED);
-			r->header_only = 1;
+			if (c->error_template)
+				r->header_only = 1;
+		}
+		return AUTHZ_DENIED;
+	case OIDC_UNAUTZ_RETURN302:
+		html_head = apr_psprintf(r->pool,
+				"<meta http-equiv=\"refresh\" content=\"0; url=%s\">",
+				oidc_dir_cfg_unauthz_arg(r));
+		oidc_util_html_send(r, "Authorization Error Redirect", html_head, NULL,
+				NULL,
+				HTTP_UNAUTHORIZED);
+		r->header_only = 1;
+		return AUTHZ_DENIED;
+	case OIDC_UNAUTZ_AUTHENTICATE:
+		/*
+		 * exception handling: if this looks like an HTTP request that cannot
+		 * complete an authentication round trip to the provider, we
+		 * won't redirect the user and thus avoid creating a state cookie
+		 */
+		if (oidc_is_auth_capable_request(r) == FALSE)
 			return AUTHZ_DENIED;
-		case OIDC_UNAUTZ_AUTHENTICATE:
-			/*
-			 * exception handling: if this looks like an HTTP request that cannot
-			 * complete an authentication round trip to the provider, we
-			 * won't redirect the user and thus avoid creating a state cookie
-			 */
-			if (oidc_is_auth_capable_request(r) == FALSE)
-				return AUTHZ_DENIED;
-			break;
+		break;
 	}
 
-	oidc_authenticate_user(r, c, NULL, oidc_get_current_url(r, c->x_forwarded_headers), NULL,
-			NULL, NULL, oidc_dir_cfg_path_auth_request_params(r), oidc_dir_cfg_path_scope(r));
+	oidc_authenticate_user(r, c, NULL,
+			oidc_get_current_url(r, c->x_forwarded_headers), NULL,
+			NULL, NULL, oidc_dir_cfg_path_auth_request_params(r),
+			oidc_dir_cfg_path_scope(r));
 
 	const char *location = oidc_util_hdr_out_location_get(r);
 
-	if ((oidc_request_state_get(r, OIDC_REQUEST_STATE_KEY_DISCOVERY) != NULL) && (location == NULL))
+	if ((oidc_request_state_get(r, OIDC_REQUEST_STATE_KEY_DISCOVERY) != NULL)
+			&& (location == NULL))
 		return AUTHZ_GRANTED;
 
 	if (location != NULL) {
-		oidc_debug(r, "send HTML refresh with authorization redirect: %s", location);
+		oidc_debug(r, "send HTML refresh with authorization redirect: %s",
+				location);
 
-		html_head =
-				apr_psprintf(r->pool, "<meta http-equiv=\"refresh\" content=\"0; url=%s\">", location);
+		html_head = apr_psprintf(r->pool,
+				"<meta http-equiv=\"refresh\" content=\"0; url=%s\">",
+				location);
 		oidc_util_html_send(r, "Stepup Authentication", html_head, NULL, NULL,
 				HTTP_UNAUTHORIZED);
 		/*

diff --git a/src/mod_auth_openidc.h b/src/mod_auth_openidc.h
@@ -248,6 +248,9 @@ APLOG_USE_MODULE(auth_openidc);
 #define OIDC_COOKIE_SAMESITE_LAX(c, r) \
 		c->cookie_same_site ? OIDC_COOKIE_EXT_SAME_SITE_LAX : OIDC_COOKIE_EXT_SAME_SITE_NONE(c, r)
 
+#define OIDC_ERROR_ENVVAR "OIDC_ERROR"
+#define OIDC_ERROR_DESC_ENVVAR "OIDC_ERROR_DESC"
+
 /* https://tools.ietf.org/html/draft-ietf-tokbind-ttrp-01 */
 #define OIDC_TB_CFG_PROVIDED_ENV_VAR     "Sec-Provided-Token-Binding-ID"
 /* https://www.ietf.org/id/draft-ietf-oauth-mtls-12 */

diff --git a/src/util.c b/src/util.c
@@ -1735,39 +1735,55 @@ int oidc_util_html_send_error(request_rec *r, const char *html_template,
 
 	if (html_template != NULL) {
 
-		html_template = oidc_util_get_full_path(r->pool, html_template);
-
-		if (html_error_template_contents == NULL) {
-			int rc = oidc_util_file_read(r, html_template,
-					r->server->process->pool, &html_error_template_contents);
-			if (rc == FALSE) {
-				oidc_error(r, "could not read HTML error template: %s",
-						html_template);
-				html_error_template_contents = NULL;
+		if (_oidc_strcmp(html_template, "deprecated") != 0) {
+
+			html_template = oidc_util_get_full_path(r->pool, html_template);
+
+			if (html_error_template_contents == NULL) {
+				int rc = oidc_util_file_read(r, html_template,
+						r->server->process->pool,
+						&html_error_template_contents);
+				if (rc == FALSE) {
+					oidc_error(r, "could not read HTML error template: %s",
+							html_template);
+					html_error_template_contents = NULL;
+				}
 			}
-		}
 
-		if (html_error_template_contents) {
-			html = apr_psprintf(r->pool, html_error_template_contents,
-					oidc_util_html_escape(r->pool, error ? error : ""),
-					oidc_util_html_escape(r->pool,
-							description ? description : ""));
+			if (html_error_template_contents) {
+				html = apr_psprintf(r->pool, html_error_template_contents,
+						oidc_util_html_escape(r->pool, error ? error : ""),
+						oidc_util_html_escape(r->pool,
+								description ? description : ""));
 
-			return oidc_util_http_send(r, html, _oidc_strlen(html),
-					OIDC_CONTENT_TYPE_TEXT_HTML, status_code);
+				return oidc_util_http_send(r, html, _oidc_strlen(html),
+						OIDC_CONTENT_TYPE_TEXT_HTML, status_code);
+			}
 		}
-	}
 
-	if (error != NULL) {
-		html = apr_psprintf(r->pool, "%s<p>Error: <pre>%s</pre></p>", html,
-				oidc_util_html_escape(r->pool, error));
-	}
-	if (description != NULL) {
-		html = apr_psprintf(r->pool, "%s<p>Description: <pre>%s</pre></p>",
-				html, oidc_util_html_escape(r->pool, description));
+		if (error != NULL) {
+			html = apr_psprintf(r->pool, "%s<p>Error: <pre>%s</pre></p>", html,
+					oidc_util_html_escape(r->pool, error));
+		}
+		if (description != NULL) {
+			html = apr_psprintf(r->pool, "%s<p>Description: <pre>%s</pre></p>",
+					html, oidc_util_html_escape(r->pool, description));
+		}
+
+		return oidc_util_html_send(r, "Error", NULL, NULL, html, status_code);
 	}
 
-	return oidc_util_html_send(r, "Error", NULL, NULL, html, status_code);
+	oidc_debug(r, "setting "OIDC_ERROR_ENVVAR" environment variable to: %s",
+			error);
+	apr_table_set(r->subprocess_env, OIDC_ERROR_ENVVAR, error ? error : "");
+
+	oidc_debug(r,
+			"setting "OIDC_ERROR_DESC_ENVVAR" environment variable to: %s",
+			description);
+	apr_table_set(r->subprocess_env, OIDC_ERROR_DESC_ENVVAR,
+			description ? description : "");
+
+	return status_code;
 }
 
 /*