1.6.0rc3: improve OIDCMetadataURL handling

Author: Hans Zandbelt

ChangeLog

@@ -1,3 +1,9 @@
+10/2/2014
+- avoid crash when downloading metadata from OIDCProviderMetadataURL fails
+- set OIDCProviderMetadataURL retrieval interval to 24 hours
+- return error on configurations mixing OIDCProviderMetadataURL and OIDCMetadataDir
+- bump version number to 1.6.0rc3
+
 10/1/2014
 - support provider configuration from a metadata URL (OIDCProviderMetadataURL)
 - bump version number to 1.6.0rc2

README.md

@@ -9,8 +9,11 @@ Overview
 --------
 
 This module enables an Apache 2.x web server to operate as an [OpenID Connect]
-(http://openid.net/specs/openid-connect-core-1_0.html) *Relying Party* to an
-OpenID Connect *Provider*.
+(http://openid.net/specs/openid-connect-core-1_0.html) *Relying Party* (RP) to an
+OpenID Connect *Provider* (OP). It will enable users to authenticate at an OpenID
+Connect Provider, receive user identity information from the OP in a so called ID
+Token and pass the identity information (a.k.a. claims) in the ID Token to applications
+hosted and protected by the Apache web server.
 
 The protected content and/or applications can be served by the Apache server
 itself or it can be served from elsewhere when Apache is configured as a reverse
@@ -91,7 +94,7 @@ have to enable the `Google+ API` under `APIs & auth` in the [Google API console]
 need to specify individual provider configuration entries manually, as in:
 
     OIDCProviderIssuer accounts.google.com
-    OIDCProviderAuthorizationEndpoint https://accounts.google.com/o/oauth2/auth[?hd=<your-domain>]
+    OIDCProviderAuthorizationEndpoint https://accounts.google.com/o/oauth2/auth
     OIDCProviderTokenEndpoint https://accounts.google.com/o/oauth2/token
     OIDCProviderTokenEndpointAuth client_secret_post
     OIDCProviderUserInfoEndpoint https://www.googleapis.com/plus/v1/people/me/openIdConnect
@@ -192,15 +195,9 @@ This is also the OpenID Connect specified way of triggering 3rd party initiated
 to a specific provider when multiple OPs have been configured. In that case the callback
 may also contain a "login_hint" parameter with the login identifier the user might use to log in.
 
-An additional **mod_auth_openidc** specific parameter named `auth_request_params` may also be passed in.
-Its value would contain dynamically determined custom parameters that need to be passed in the
-authorization requests and as such it is the runtime equivalent of the static OIDCAuthRequestParams
-configuration value and the `auth_request_params` in the `.conf` file. If you use more than one of the
-previous settings you'll need to make sure they do not conflict. Note that nested URL encoding is required
-e.g. passing both prompt=consent and max_auth_age=0 would result in:
-
-    auth_request_params=prompt%3Dconsent%26max_auth_age%3D0
-
+An additional **mod_auth_openidc** specific parameter named `auth_request_params` may also be passed
+in, see the [Wiki](https://github.com/pingidentity/mod_auth_openidc/wiki#10-how-can-i-add-custom-parameters-to-the-authorization-request)
+for its usage.
 
 ###Sample Config for PingFederate OpenID Connect & OAuth 2.0
 

auth_openidc.conf

@@ -28,9 +28,9 @@
 # (Optional)
 # When using multiple OpenID Connect Providers, possibly combined with Dynamic Client
 # Registration and account-based OP Discovery.
-# Directory that holds metadata files (must be writable for the Apache process/user)
-# When not specified, it is assumed that we use a single statically configured provider
-# as described under the section "OpenID Connect Provider" below.
+# Specifies the directory that holds metadata files (must be writable for the Apache process/user).
+# When not specified, it is assumed that we use a single statically configured provider as
+# described under the section "OpenID Connect Provider" below, most likely using OIDCProviderMetadataURL.
 #OIDCMetadataDir /var/cache/apache2/mod_auth_openidc/metadata
 
 ########################################################################################
@@ -101,7 +101,7 @@
 ########################################################################################
 
 # URL where OpenID Connect Provider metadata can be found (e.g. https://accounts.google.com/.well-known/openid-configuration)
-# The obtained metadata will be cached and refreshed every 4 hours.
+# The obtained metadata will be cached and refreshed every 24 hours.
 # If set, individual entries below will not have to be configured but can be used to override
 # settings obtained from the metadata.
 # If not set, the entries below will have to be configured for a single static OP configuration

configure.ac

@@ -1,4 +1,4 @@
-AC_INIT([mod_auth_openidc],[1.6.0rc2],[[email protected]])
+AC_INIT([mod_auth_openidc],[1.6.0rc3],[[email protected]])
 
 AC_SUBST(NAMEVER, AC_PACKAGE_TARNAME()-AC_PACKAGE_VERSION())
 

src/config.c

@@ -926,6 +926,12 @@ static int oidc_check_config_openid_openidc(server_rec *s, oidc_cfg *c) {
 		// TODO: this depends on the configured OIDCResponseType now
 		if (c->provider.client_secret == NULL)
 			return oidc_check_config_error(s, "OIDCClientSecret");
+	} else {
+		if (c->provider.metadata_url != NULL) {
+			oidc_serror(s,
+					"only one of 'OIDCProviderMetadataURL' or 'OIDCMetadataDir' should be set");
+			return HTTP_INTERNAL_SERVER_ERROR;
+		}
 	}
 
 	apr_uri_parse(s->process->pconf, c->redirect_uri, &r_uri);

src/mod_auth_openidc.c

@@ -201,14 +201,16 @@ static char *oidc_get_state_cookie_name(request_rec *r, const char *state) {
 /*
  * return the static provider configuration, i.e. from a metadata URL or configuration primitives
  */
-static oidc_provider_t *oidc_provider_static_config(request_rec *r, oidc_cfg *c) {
+static apr_byte_t oidc_provider_static_config(request_rec *r, oidc_cfg *c, oidc_provider_t **provider) {
 
 	json_t *j_provider = NULL;
 	const char *s_json = NULL;
 
 	/* see if we should configure a static provider based on external (cached) metadata */
-	if ((c->metadata_dir != NULL) || (c->provider.metadata_url == NULL))
-		return &c->provider;
+	if ((c->metadata_dir != NULL) || (c->provider.metadata_url == NULL)) {
+		*provider = &c->provider;
+		return TRUE;
+	}
 
 	c->cache->get(r, OIDC_CACHE_SECTION_PROVIDER, c->provider.metadata_url,
 			&s_json);
@@ -219,7 +221,7 @@ static oidc_provider_t *oidc_provider_static_config(request_rec *r, oidc_cfg *c)
 				c->provider.metadata_url, &j_provider, &s_json) == FALSE) {
 			oidc_error(r, "could not retrieve metadata from url: %s",
 					c->provider.metadata_url);
-			return NULL;
+			return FALSE;
 		}
 
 		// TODO: make the expiry configurable
@@ -233,22 +235,20 @@ static oidc_provider_t *oidc_provider_static_config(request_rec *r, oidc_cfg *c)
 		j_provider = json_loads(s_json, 0, 0);
 	}
 
-	oidc_debug(r, " # got metadata: %s", s_json);
+	*provider = apr_pcalloc(r->pool, sizeof(oidc_provider_t));
+	memcpy(*provider, &c->provider, sizeof(oidc_provider_t));
 
-	oidc_provider_t *provider = apr_pcalloc(r->pool, sizeof(oidc_provider_t));
-	memcpy(provider, &c->provider, sizeof(oidc_provider_t));
-
-	if (oidc_metadata_provider_parse(r, j_provider, provider) == FALSE) {
+	if (oidc_metadata_provider_parse(r, j_provider, *provider) == FALSE) {
 		oidc_error(r, "could not parse metadata from url: %s",
 				c->provider.metadata_url);
 		if (j_provider)
 			json_decref(j_provider);
-		return NULL;
+		return FALSE;
 	}
 
 	json_decref(j_provider);
 
-	return provider;
+	return TRUE;
 }
 
 /*
@@ -258,7 +258,9 @@ static oidc_provider_t *oidc_get_provider_for_issuer(request_rec *r,
 		oidc_cfg *c, const char *issuer) {
 
 	/* by default we'll assume that we're dealing with a single statically configured OP */
-	oidc_provider_t *provider = oidc_provider_static_config(r, c);
+	oidc_provider_t *provider = NULL;
+	if (oidc_provider_static_config(r, c, &provider) == FALSE)
+		return NULL;
 
 	/* unless a metadata directory was configured, so we'll try and get the provider settings from there */
 	if (c->metadata_dir != NULL) {
@@ -1273,7 +1275,8 @@ static int oidc_authenticate_user(request_rec *r, oidc_cfg *c,
 			return oidc_discovery(r, c);
 
 		/* we're not using multiple OP's configured in a metadata directory, pick the statically configured OP */
-		provider = oidc_provider_static_config(r, c);
+		if (oidc_provider_static_config(r, c, &provider) == FALSE)
+			return HTTP_INTERNAL_SERVER_ERROR;
 	}
 
 	/* generate a random value to correlate request/response through browser state */

src/mod_auth_openidc.h

@@ -150,7 +150,7 @@ APLOG_USE_MODULE(auth_openidc);
 #define OIDC_REQUIRE_NAME "claim"
 
 /* defines for how long provider metadata will be cached */
-#define OIDC_CACHE_PROVIDER_METADATA_EXPIRY_DEFAULT 14400
+#define OIDC_CACHE_PROVIDER_METADATA_EXPIRY_DEFAULT 86400
 
 /* cache sections */
 #define OIDC_CACHE_SECTION_JTI "jti"