From b253b3043cb7c0fb3806710b216184a54de7c9c7 Mon Sep 17 00:00:00 2001 From: Kristof Bajnok Date: Mon, 13 Mar 2023 09:21:01 +0100 Subject: [PATCH] fixup! ModuleRouter: support paths in BASE Rebased to current master. When composing the paths, use os.path.join primarily, since it handles empty strings and duplicate separators logically. As long as we use the BASE_URL in the OpenID Connect frontend as an issuer, it's not possible to create multiple provider discovery URLs. Add documentation and a comment to explain this limitation. --- doc/README.md | 3 +- src/satosa/frontends/base.py | 5 +- src/satosa/frontends/openid_connect.py | 40 +++++++++++---- tests/flows/test_oidc-saml.py | 17 +++++-- tests/satosa/frontends/test_openid_connect.py | 51 +++++++++++++++---- 5 files changed, 90 insertions(+), 26 deletions(-) diff --git a/doc/README.md b/doc/README.md index 8d001847e..6fff65c7b 100644 --- a/doc/README.md +++ b/doc/README.md @@ -78,7 +78,8 @@ bind_password: !ENVFILE LDAP_BIND_PASSWORD_FILE | Parameter name | Data type | Example value | Description | | -------------- | --------- | ------------- | ----------- | -| `BASE` | string | `https://proxy.example.com` | base url of the proxy | +| `BASE` | string | `https://proxy.example.com` | The base url of the proxy. For the OIDC Frontend, this is used to set the issuer as well, and due to implementation constraints, avoid +using trailing slashes in this case. | | `COOKIE_STATE_NAME` | string | `satosa_state` | name of the cookie SATOSA uses for preserving state between requests | | `CONTEXT_STATE_DELETE` | bool | `True` | controls whether SATOSA will delete the state cookie after receiving the authentication response from the upstream IdP| | `STATE_ENCRYPTION_KEY` | string | `52fddd3528a44157` | key used for encrypting the state cookie, will be overridden by the environment variable `SATOSA_STATE_ENCRYPTION_KEY` if it is set | diff --git a/src/satosa/frontends/base.py b/src/satosa/frontends/base.py index 6eb15d2e1..d5a611995 100644 --- a/src/satosa/frontends/base.py +++ b/src/satosa/frontends/base.py @@ -17,16 +17,19 @@ def __init__(self, auth_req_callback_func, internal_attributes, base_url, name): :type auth_req_callback_func: (satosa.context.Context, satosa.internal.InternalData) -> satosa.response.Response :type internal_attributes: dict[str, dict[str, str | list[str]]] + :type base_url: str :type name: str :param auth_req_callback_func: Callback should be called by the module after the authorization response has been processed. + :param internal_attributes: attribute mapping + :param base_url: base url of the proxy :param name: name of the plugin """ self.auth_req_callback_func = auth_req_callback_func self.internal_attributes = internal_attributes self.converter = AttributeMapper(internal_attributes) - self.base_url = base_url.rstrip("/") if base_url else "" + self.base_url = base_url or "" self.name = name self.endpoint_baseurl = os.path.join(self.base_url, self.name) self.endpoint_basepath = urlparse(self.endpoint_baseurl).path.lstrip("/") diff --git a/src/satosa/frontends/openid_connect.py b/src/satosa/frontends/openid_connect.py index 75573b4fe..3c04437b2 100644 --- a/src/satosa/frontends/openid_connect.py +++ b/src/satosa/frontends/openid_connect.py @@ -4,6 +4,7 @@ import json import logging +import os.path from collections import defaultdict from urllib.parse import urlencode, urlparse @@ -172,7 +173,16 @@ def register_endpoints(self, backend_names): :rtype: list[(str, ((satosa.context.Context, Any) -> satosa.response.Response, Any))] :raise ValueError: if more than one backend is configured """ - provider_config = ("^.well-known/openid-configuration$", self.provider_config) + # See https://openid.net/specs/openid-connect-discovery-1_0.html#ProviderConfig + # Unfortunately since the issuer is always `base_url` for all OIDC frontend instances, + # the discovery endpoint will be the same for every instance. + # This means that only one frontend will be usable for autodiscovery. + autoconf_path = ".well-known/openid-configuration" + base_path = urlparse(self.base_url).path.lstrip("/") + provider_config = ( + "^{}$".format(os.path.join(base_path, autoconf_path)), + self.provider_config, + ) jwks_uri = ("^{}/jwks$".format(self.endpoint_basepath), self.jwks) backend_name = None @@ -193,35 +203,47 @@ def register_endpoints(self, backend_names): if backend_name: # if there is only one backend, include its name in the path so the default routing can work - auth_endpoint = "{}/{}/{}/{}".format(self.base_url, backend_name, self.name, AuthorizationEndpoint.url) + auth_endpoint = os.path.join( + self.base_url, + backend_name, + self.name, + AuthorizationEndpoint.url, + ) self.provider.configuration_information["authorization_endpoint"] = auth_endpoint auth_path = urlparse(auth_endpoint).path.lstrip("/") else: - auth_path = "{}/{}".format(self.endpoint_basepath, AuthorizationEndpoint.url) + auth_path = os.path.join(self.endpoint_basepath, AuthorizationRequest.url) authentication = ("^{}$".format(auth_path), self.handle_authn_request) url_map = [provider_config, jwks_uri, authentication] if any("code" in v for v in self.provider.configuration_information["response_types_supported"]): - self.provider.configuration_information["token_endpoint"] = "{}/{}".format( - self.endpoint_baseurl, TokenEndpoint.url + self.provider.configuration_information["token_endpoint"] = os.path.join( + self.endpoint_baseurl, + TokenEndpoint.url, ) token_endpoint = ( - "^{}/{}".format(self.endpoint_basepath, TokenEndpoint.url), self.token_endpoint + "^{}".format(os.path.join(self.endpoint_basepath, TokenEndpoint.url)), + self.token_endpoint, ) url_map.append(token_endpoint) self.provider.configuration_information["userinfo_endpoint"] = ( - "{}/{}".format(self.endpoint_baseurl, UserinfoEndpoint.url) + os.path.join(self.endpoint_baseurl, UserinfoEndpoint.url) ) userinfo_endpoint = ( - "^{}/{}".format(self.endpoint_basepath, UserinfoEndpoint.url), self.userinfo_endpoint + "^{}".format( + os.path.join(self.endpoint_basepath, UserinfoEndpoint.url) + ), + self.userinfo_endpoint, ) url_map.append(userinfo_endpoint) if "registration_endpoint" in self.provider.configuration_information: client_registration = ( - "^{}/{}".format(self.endpoint_basepath, RegistrationEndpoint.url), + "^{}".format( + os.path.join(self.endpoint_basepath, RegistrationEndpoint.url) + ), self.client_registration, ) url_map.append(client_registration) diff --git a/tests/flows/test_oidc-saml.py b/tests/flows/test_oidc-saml.py index 2a299bfef..656a70a1f 100644 --- a/tests/flows/test_oidc-saml.py +++ b/tests/flows/test_oidc-saml.py @@ -52,7 +52,6 @@ def oidc_frontend_config(signing_key_path): "module": "satosa.frontends.openid_connect.OpenIDConnectFrontend", "name": "OIDCFrontend", "config": { - "issuer": "https://proxy-op.example.com", "signing_key_path": signing_key_path, "provider": {"response_types_supported": ["id_token"]}, "client_db_uri": DB_URI, # use mongodb for integration testing @@ -69,7 +68,6 @@ def oidc_stateless_frontend_config(signing_key_path, client_db_path): "module": "satosa.frontends.openid_connect.OpenIDConnectFrontend", "name": "OIDCFrontend", "config": { - "issuer": "https://proxy-op.example.com", "signing_key_path": signing_key_path, "client_db_path": client_db_path, "db_uri": "stateless://user:abc123@localhost", @@ -94,6 +92,12 @@ def _client_setup(self): "response_types": ["id_token"] } + def _discover_provider(self, client, provider): + discovery_path = ( + os.path.join(urlparse(provider).path, ".well-known/openid-configuration") + ) + return json.loads(client.get(discovery_path).data.decode("utf-8")) + def test_full_flow(self, satosa_config_dict, oidc_frontend_config, saml_backend_config, idp_conf): self._client_setup() subject_id = "testuser1" @@ -110,7 +114,8 @@ def test_full_flow(self, satosa_config_dict, oidc_frontend_config, saml_backend_ test_client = Client(make_app(SATOSAConfig(satosa_config_dict)), Response) # get frontend OP config info - provider_config = json.loads(test_client.get("/.well-known/openid-configuration").data.decode("utf-8")) + issuer = satosa_config_dict["BASE"] + provider_config = self._discover_provider(test_client, issuer) # create auth req claims_request = ClaimsRequest(id_token=Claims(**{k: None for k in USERS[subject_id]})) @@ -168,7 +173,8 @@ def test_full_stateless_id_token_flow(self, satosa_config_dict, oidc_stateless_f test_client = Client(make_app(SATOSAConfig(satosa_config_dict)), Response) # get frontend OP config info - provider_config = json.loads(test_client.get("/.well-known/openid-configuration").data.decode("utf-8")) + issuer = satosa_config_dict["BASE"] + provider_config = self._discover_provider(test_client, issuer) # create auth req claims_request = ClaimsRequest(id_token=Claims(**{k: None for k in USERS[subject_id]})) @@ -226,7 +232,8 @@ def test_full_stateless_code_flow(self, satosa_config_dict, oidc_stateless_front test_client = Client(make_app(SATOSAConfig(satosa_config_dict)), Response) # get frontend OP config info - provider_config = json.loads(test_client.get("/.well-known/openid-configuration").data.decode("utf-8")) + issuer = satosa_config_dict["BASE"] + provider_config = self._discover_provider(test_client, issuer) # create auth req claims_request = ClaimsRequest(id_token=Claims(**{k: None for k in USERS[subject_id]})) diff --git a/tests/satosa/frontends/test_openid_connect.py b/tests/satosa/frontends/test_openid_connect.py index f769b2c66..4331e59a0 100644 --- a/tests/satosa/frontends/test_openid_connect.py +++ b/tests/satosa/frontends/test_openid_connect.py @@ -28,7 +28,7 @@ INTERNAL_ATTRIBUTES = { "attributes": {"mail": {"saml": ["email"], "openid": ["email"]}} } -BASE_URL = "https://op.example.com" +BASE_URL = "https://op.example.com/satosa" CLIENT_ID = "client1" CLIENT_SECRET = "client_secret" EXTRA_CLAIMS = { @@ -86,10 +86,10 @@ def frontend_config_with_extra_id_token_claims(self, signing_key_path): return config - def create_frontend(self, frontend_config): + def create_frontend(self, frontend_config, issuer=BASE_URL): # will use in-memory storage instance = OpenIDConnectFrontend(lambda ctx, req: None, INTERNAL_ATTRIBUTES, - frontend_config, BASE_URL, "oidc_frontend") + frontend_config, issuer, "oidc_frontend") instance.register_endpoints(["foo_backend"]) return instance @@ -369,10 +369,30 @@ def test_jwks(self, context, frontend): jwks = json.loads(http_response.message) assert jwks == {"keys": [frontend.signing_key.serialize()]} - def test_register_endpoints_token_and_userinfo_endpoint_is_published_if_necessary(self, frontend): + @pytest.mark.parametrize("issuer", [ + "https://example.com", + "https://example.com/some/op", + "https://example.com/" + ]) + def test_register_endpoints_handles_path_in_issuer(self, frontend_config, issuer): + frontend = self.create_frontend(frontend_config, issuer) + issuer_path = urlparse(issuer).path[1:] + if issuer_path: + issuer_path += "/" urls = frontend.register_endpoints(["test"]) - assert ("^{}/{}".format(frontend.name, TokenEndpoint.url), frontend.token_endpoint) in urls - assert ("^{}/{}".format(frontend.name, UserinfoEndpoint.url), frontend.userinfo_endpoint) in urls + assert ( + "^{}{}$".format(issuer_path, ".well-known/openid-configuration"), + frontend.provider_config, + ) in urls + + assert ( + "^{}/{}".format(frontend.endpoint_basepath, TokenEndpoint.url), + frontend.token_endpoint, + ) in urls + assert ( + "^{}/{}".format(frontend.endpoint_basepath, UserinfoEndpoint.url), + frontend.userinfo_endpoint, + ) in urls def test_register_endpoints_token_and_userinfo_endpoint_is_not_published_if_only_implicit_flow( self, frontend_config, context): @@ -380,8 +400,14 @@ def test_register_endpoints_token_and_userinfo_endpoint_is_not_published_if_only frontend = self.create_frontend(frontend_config) urls = frontend.register_endpoints(["test"]) - assert ("^{}/{}".format("test", TokenEndpoint.url), frontend.token_endpoint) not in urls - assert ("^{}/{}".format("test", UserinfoEndpoint.url), frontend.userinfo_endpoint) not in urls + assert ( + "^{}/{}".format(frontend.endpoint_basepath, TokenEndpoint.url), + frontend.token_endpoint, + ) not in urls + assert ( + "^{}/{}".format(frontend.endpoint_basepath, UserinfoEndpoint.url), + frontend.userinfo_endpoint, + ) not in urls http_response = frontend.provider_config(context) provider_config = ProviderConfigurationResponse().deserialize(http_response.message, "json") @@ -397,8 +423,13 @@ def test_register_endpoints_dynamic_client_registration_is_configurable( frontend = self.create_frontend(frontend_config) urls = frontend.register_endpoints(["test"]) - assert (("^{}/{}".format(frontend.name, RegistrationEndpoint.url), - frontend.client_registration) in urls) == client_registration_enabled + assert ( + ( + "^{}/{}".format(frontend.endpoint_basepath, RegistrationEndpoint.url), + frontend.client_registration, + ) + in urls + ) == client_registration_enabled provider_info = ProviderConfigurationResponse().deserialize(frontend.provider_config(None).message, "json") assert ("registration_endpoint" in provider_info) == client_registration_enabled