From 92f60fd695cce58e9693e8edc0611f8ab389cd6b Mon Sep 17 00:00:00 2001 From: Hans Zandbelt Date: Thu, 5 Jun 2014 11:09:57 +0200 Subject: [PATCH] add warning/errors when configured hosts/domains do not match match the URL being accessed against the OIDCRedirectURI and OIDCCookieDomain --- ChangeLog | 1 + src/config.c | 18 ++++++++++++++++++ src/metadata.c | 2 +- src/mod_auth_openidc.c | 25 +++++++++++++++++++++++++ 4 files changed, 45 insertions(+), 1 deletion(-) diff --git a/ChangeLog b/ChangeLog index eee48bca..d9ef4b72 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,5 +1,6 @@ 6/5/2014 - do not set Secure cookies for plain HTTP +- add warning/errors when configured hosts/domains do not match 6/4/2014 - fix passing integer claims on non-Mac OS X systems diff --git a/src/config.c b/src/config.c index 7aea945d..8f12e189 100644 --- a/src/config.c +++ b/src/config.c @@ -835,6 +835,24 @@ static int oidc_check_config_openid_openidc(server_rec *s, oidc_cfg *c) { return oidc_check_config_error(s, "OIDCClientSecret"); } + apr_uri_t r_uri; + apr_uri_parse(s->process->pconf, c->redirect_uri, &r_uri); + if (apr_strnatcmp(r_uri.scheme, "https") != 0) { + ap_log_error(APLOG_MARK, APLOG_WARNING, 0, s, + "oidc_check_config_openid_openidc: the URL scheme (%s) of the configured OIDCRedirectURI SHOULD be \"https\" for security reasons (moreover: some Providers may reject non-HTTPS URLs)", + r_uri.scheme); + } + + if (c->cookie_domain != NULL) { + char *p = strstr(r_uri.hostname, c->cookie_domain); + if ((p == NULL) || (apr_strnatcmp(c->cookie_domain, p) != 0)) { + ap_log_error(APLOG_MARK, APLOG_ERR, 0, s, + "oidc_check_config_openid_openidc: the domain (%s) configured in OIDCCookieDomain does not match the URL hostname (%s) of the configured OIDCRedirectURI (%s): setting \"state\" and \"session\" cookies will not work!", + c->cookie_domain, r_uri.hostname, c->redirect_uri); + return HTTP_INTERNAL_SERVER_ERROR; + } + } + return OK; } diff --git a/src/metadata.c b/src/metadata.c index bdcee7c0..3eb2977c 100644 --- a/src/metadata.c +++ b/src/metadata.c @@ -770,7 +770,7 @@ static apr_byte_t oidc_metadata_client_get(request_rec *r, oidc_cfg *cfg, /* at this point we have no valid client metadata, see if there's a registration endpoint for this provider */ if (provider->registration_endpoint_url == NULL) { ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, - "oidc_metadata_client_get: no (valid) client metadata exists and provider JSON object did not contain a (valid) \"registration_endpoint\" string"); + "oidc_metadata_client_get: no (valid) client metadata exists for provider (%s) and provider JSON object did not contain a (valid) \"registration_endpoint\" string", issuer); return FALSE; } diff --git a/src/mod_auth_openidc.c b/src/mod_auth_openidc.c index bdf983ef..18529e9f 100644 --- a/src/mod_auth_openidc.c +++ b/src/mod_auth_openidc.c @@ -1134,6 +1134,31 @@ static int oidc_authenticate_user(request_rec *r, oidc_cfg *c, provider->response_type, "code token")))) proto_state.nonce = NULL; + /* + * printout errors if Cookie settings are not going to work + */ + apr_uri_t o_uri; + apr_uri_t r_uri; + apr_uri_parse(r->pool, original_url, &o_uri); + apr_uri_parse(r->pool, c->redirect_uri, &r_uri); + if ( (apr_strnatcmp(o_uri.scheme, r_uri.scheme) != 0) && (apr_strnatcmp(r_uri.scheme, "https") == 0) ) { + ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, "oidc_authenticate_user: the URL scheme (%s) of the configured OIDCRedirectURI does not match the URL scheme of the URL being accessed (%s): the \"state\" and \"session\" cookies will not be shared between the two!", r_uri.scheme, o_uri.scheme); + } + + if (c->cookie_domain == NULL) { + if (apr_strnatcmp(o_uri.hostname, r_uri.hostname) != 0) { + char *p = strstr(o_uri.hostname, r_uri.hostname); + if ( (p == NULL) || (apr_strnatcmp(r_uri.hostname, p) != 0)) { + ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, "oidc_authenticate_user: the URL hostname (%s) of the configured OIDCRedirectURI does not match the URL hostname of the URL being accessed (%s): the \"state\" and \"session\" cookies will not be shared between the two!", r_uri.hostname, o_uri.hostname); + } + } + } else { + char *p = strstr(o_uri.hostname, c->cookie_domain); + if ( (p == NULL) || (apr_strnatcmp(c->cookie_domain, p) != 0)) { + ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, "oidc_authenticate_user: the domain (%s) configured in OIDCCookieDomain does not match the URL hostname (%s) of the URL being accessed (%s): setting \"state\" and \"session\" cookies will not work!!", c->cookie_domain, o_uri.hostname, original_url); + } + } + /* send off to the OpenID Connect Provider */ // TODO: maybe show intermediate/progress screen "redirecting to" return oidc_proto_authorization_request(r, provider, c->redirect_uri, state, &proto_state);