amend! fixup! ModuleRouter: support paths in BASE

bajnokk · bajnokk · commit 8cb44d6d4fba · 2023-03-17T21:52:30.000+01:00
If Satosa is installed under a path which is not the root of the webserver (ie. "https://example.com/satosa"), then endpoint routing must take the base path into consideration. Some modules registered some of their endpoints with the base path included, but other times the base path was omitted, thus it made the routing fail. Now all endpoint registrations include the base path in their endpoint map. Provide a simple implementation for joining path components, since we don't want to add the separator for empty strings and when any of the path components already have it. Additionally, DEBUG logging was configured for the tests so that the debug logs are accessible during testing.
diff --git a/doc/README.md b/doc/README.md
@@ -78,7 +78,7 @@ bind_password: !ENVFILE LDAP_BIND_PASSWORD_FILE
 
 | Parameter name | Data type | Example value | Description |
 | -------------- | --------- | ------------- | ----------- |
-| `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.  |
+| `BASE` | string | `https://proxy.example.com` | base url of the proxy |
 | `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
@@ -2,8 +2,8 @@
 Holds a base class for frontend modules used in the SATOSA proxy.
 """
 from ..attribute_mapping import AttributeMapper
+from ..util import join_paths
 
-import os.path
 from urllib.parse import urlparse
 
 
@@ -31,7 +31,7 @@ def __init__(self, auth_req_callback_func, internal_attributes, base_url, name):
         self.converter = AttributeMapper(internal_attributes)
         self.base_url = base_url or ""
         self.name = name
-        self.endpoint_baseurl = os.path.join(self.base_url, self.name)
+        self.endpoint_baseurl = join_paths(self.base_url, self.name)
         self.endpoint_basepath = urlparse(self.endpoint_baseurl).path.lstrip("/")
 
     def handle_authn_response(self, context, internal_resp):
diff --git a/src/satosa/frontends/openid_connect.py b/src/satosa/frontends/openid_connect.py
@@ -4,7 +4,6 @@
 
 import json
 import logging
-import os.path
 from collections import defaultdict
 from urllib.parse import urlencode, urlparse
 
@@ -38,7 +37,7 @@
 from ..response import BadRequest, Created
 from ..response import SeeOther, Response
 from ..response import Unauthorized
-from ..util import rndstr
+from ..util import join_paths, rndstr
 
 import satosa.logging_util as lu
 from satosa.internal import InternalData
@@ -174,13 +173,14 @@ def register_endpoints(self, backend_names):
         :raise ValueError: if more than one backend is configured
         """
         # 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.
+        #
+        # We skip the scheme + host + port of the issuer URL, because we can only map the
+        # path for the provider config endpoint. We are safe to use urlparse().path here,
+        # because for issuer OIDC allows only https URLs without query and fragment parts.
+        issuer = self.provider.configuration_information["issuer"]
         autoconf_path = ".well-known/openid-configuration"
-        base_path = urlparse(self.base_url).path.lstrip("/")
         provider_config = (
-            "^{}$".format(os.path.join(base_path, autoconf_path)),
+            "^{}$".format(join_paths(urlparse(issuer).path.lstrip("/"), autoconf_path)),
             self.provider_config,
         )
         jwks_uri = ("^{}/jwks$".format(self.endpoint_basepath), self.jwks)
@@ -203,7 +203,7 @@ 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 = os.path.join(
+            auth_endpoint = join_paths(
                 self.base_url,
                 backend_name,
                 self.name,
@@ -212,28 +212,28 @@ def register_endpoints(self, backend_names):
             self.provider.configuration_information["authorization_endpoint"] = auth_endpoint
             auth_path = urlparse(auth_endpoint).path.lstrip("/")
         else:
-            auth_path = os.path.join(self.endpoint_basepath, AuthorizationRequest.url)
+            auth_path = join_paths(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"] = os.path.join(
+            self.provider.configuration_information["token_endpoint"] = join_paths(
                 self.endpoint_baseurl,
                 TokenEndpoint.url,
             )
             token_endpoint = (
-                "^{}".format(os.path.join(self.endpoint_basepath, TokenEndpoint.url)),
+                "^{}".format(join_paths(self.endpoint_basepath, TokenEndpoint.url)),
                 self.token_endpoint,
             )
             url_map.append(token_endpoint)
 
             self.provider.configuration_information["userinfo_endpoint"] = (
-                os.path.join(self.endpoint_baseurl, UserinfoEndpoint.url)
+                join_paths(self.endpoint_baseurl, UserinfoEndpoint.url)
             )
             userinfo_endpoint = (
                 "^{}".format(
-                    os.path.join(self.endpoint_basepath, UserinfoEndpoint.url)
+                    join_paths(self.endpoint_basepath, UserinfoEndpoint.url)
                 ),
                 self.userinfo_endpoint,
             )
@@ -242,7 +242,7 @@ def register_endpoints(self, backend_names):
         if "registration_endpoint" in self.provider.configuration_information:
             client_registration = (
                 "^{}".format(
-                    os.path.join(self.endpoint_basepath, RegistrationEndpoint.url)
+                    join_paths(self.endpoint_basepath, RegistrationEndpoint.url)
                 ),
                 self.client_registration,
             )
diff --git a/src/satosa/frontends/ping.py b/src/satosa/frontends/ping.py
@@ -1,9 +1,9 @@
 import logging
-import os.path
 
 import satosa.logging_util as lu
 from satosa.frontends.base import FrontendModule
 from satosa.response import Response
+from satosa.util import join_paths
 
 
 logger = logging.getLogger(__name__)
@@ -44,7 +44,7 @@ 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
         """
-        url_map = [("^{}".format(os.path.join(self.endpoint_basepath, self.name)), self.ping_endpoint)]
+        url_map = [("^{}".format(join_paths(self.endpoint_basepath, self.name)), self.ping_endpoint)]
 
         return url_map
 
diff --git a/src/satosa/micro_services/account_linking.py b/src/satosa/micro_services/account_linking.py
@@ -3,7 +3,6 @@
 """
 import json
 import logging
-import os.path
 
 import requests
 from jwkest.jwk import rsa_load, RSAKey
@@ -13,6 +12,7 @@
 from ..exception import SATOSAAuthenticationError
 from ..micro_services.base import ResponseMicroService
 from ..response import Redirect
+from ..util import join_paths
 
 import satosa.logging_util as lu
 logger = logging.getLogger(__name__)
@@ -165,8 +165,8 @@ def register_endpoints(self):
         return [
             (
                 "^{}$".format(
-                    os.path.join(
-                        self.base_path, "account_linking", self.endpoint.lstrip("/")
+                    join_paths(
+                        self.base_path, "account_linking", self.endpoint
                     )
                 ),
                 self._handle_al_response,
diff --git a/src/satosa/micro_services/consent.py b/src/satosa/micro_services/consent.py
@@ -4,7 +4,6 @@
 import hashlib
 import json
 import logging
-import os.path
 from base64 import urlsafe_b64encode
 
 import requests
@@ -17,6 +16,7 @@
 from satosa.internal import InternalData
 from satosa.micro_services.base import ResponseMicroService
 from satosa.response import Redirect
+from satosa.util import join_paths
 
 
 logger = logging.getLogger(__name__)
@@ -242,8 +242,8 @@ def register_endpoints(self):
         return [
             (
                 "^{}$".format(
-                    os.path.join(
-                        self.base_path, "consent", self.endpoint.lstrip("/")
+                    join_paths(
+                        self.base_path, "consent", self.endpoint
                     )
                 ),
                 self._handle_consent_response,
diff --git a/src/satosa/util.py b/src/satosa/util.py
@@ -89,3 +89,29 @@ def rndstr(size=16, alphabet=""):
     if not alphabet:
         alphabet = string.ascii_letters[0:52] + string.digits
     return type(alphabet)().join(rng.choice(alphabet) for _ in range(size))
+
+
+def join_paths(base, *paths):
+    """
+    Joins strings with a "/" separator, like they were path components, but
+    tries to avoid adding an unnecessary separator.  Note that the contents of
+    the strings are not sanitized in any way. If any of the components begins or
+    ends with a "/", the separator is not inserted, and any number of empty
+    strings at the beginning would not add a leading slash.  Any number of empty
+    strings at the end only add a single trailing slash.
+
+    Raises TypeError if any of the components are not strings.
+    """
+    sep = "/"
+
+    path = base
+    try:
+        for p in paths:
+            if not path or path.endswith(sep) or p.startswith(sep):
+                path += p
+            else:
+                path += sep + p
+    except (AttributeError, TypeError) as err:
+        raise TypeError("Arguments must be strings") from err
+
+    return path
diff --git a/tests/flows/test_account_linking.py b/tests/flows/test_account_linking.py
@@ -1,11 +1,11 @@
-import os.path
 import responses
 from urllib.parse import urlparse
 from werkzeug.test import Client
 from werkzeug.wrappers import Response
 
 from satosa.proxy_server import make_app
 from satosa.satosa_config import SATOSAConfig
+from satosa.util import join_paths
 
 
 class TestAccountLinking:
@@ -15,7 +15,7 @@ def test_full_flow(self, satosa_config_dict, account_linking_module_config):
         account_linking_module_config["config"]["api_url"] = api_url
         account_linking_module_config["config"]["redirect_url"] = redirect_url
         satosa_config_dict["MICRO_SERVICES"].insert(0, account_linking_module_config)
-        base_path = urlparse(satosa_config_dict["BASE"]).path.lstrip("\n")
+        base_path = urlparse(satosa_config_dict["BASE"]).path.lstrip("\n/")
 
         # application
         test_client = Client(make_app(SATOSAConfig(satosa_config_dict)), Response)
@@ -39,5 +39,5 @@ def test_full_flow(self, satosa_config_dict, account_linking_module_config):
             rsps.add(responses.GET, "{}/get_id".format(api_url), "test_userid", status=200)
 
             # incoming account linking response
-            http_resp = test_client.get(os.path.join("/", base_path, "account_linking/handle_account_linking"))
+            http_resp = test_client.get(join_paths("/", base_path, "account_linking/handle_account_linking"))
             assert http_resp.status_code == 200
diff --git a/tests/flows/test_consent.py b/tests/flows/test_consent.py
@@ -1,5 +1,4 @@
 import json
-import os.path
 import re
 
 import responses
@@ -9,6 +8,7 @@
 
 from satosa.proxy_server import make_app
 from satosa.satosa_config import SATOSAConfig
+from satosa.util import join_paths
 
 
 class TestConsent:
@@ -18,7 +18,7 @@ def test_full_flow(self, satosa_config_dict, consent_module_config):
         consent_module_config["config"]["api_url"] = api_url
         consent_module_config["config"]["redirect_url"] = redirect_url
         satosa_config_dict["MICRO_SERVICES"].append(consent_module_config)
-        base_path = urlparse(satosa_config_dict["BASE"]).path.lstrip("\n")
+        base_path = urlparse(satosa_config_dict["BASE"]).path.lstrip("\n/")
 
         # application
         test_client = Client(make_app(SATOSAConfig(satosa_config_dict)), Response)
@@ -46,5 +46,5 @@ def test_full_flow(self, satosa_config_dict, consent_module_config):
             rsps.add(responses.GET, verify_url_re, json.dumps({"foo": "bar"}), status=200)
 
             # incoming consent response
-            http_resp = test_client.get(os.path.join("/", base_path, "consent/handle_consent"))
+            http_resp = test_client.get(join_paths("/", base_path, "consent/handle_consent"))
             assert http_resp.status_code == 200
diff --git a/tests/flows/test_oidc-saml.py b/tests/flows/test_oidc-saml.py
@@ -17,6 +17,7 @@
 from satosa.metadata_creation.saml_metadata import create_entity_descriptors
 from satosa.proxy_server import make_app
 from satosa.satosa_config import SATOSAConfig
+from satosa.util import join_paths
 from tests.users import USERS
 from tests.users import OIDC_USERS
 from tests.util import FakeIdP
@@ -94,7 +95,7 @@ def _client_setup(self):
 
     def _discover_provider(self, client, provider):
         discovery_path = (
-            os.path.join(urlparse(provider).path, ".well-known/openid-configuration")
+            join_paths(urlparse(provider).path, ".well-known/openid-configuration")
         )
         return json.loads(client.get(discovery_path).data.decode("utf-8"))
 
diff --git a/tests/satosa/test_util.py b/tests/satosa/test_util.py
@@ -0,0 +1,39 @@
+import pytest
+from satosa.util import join_paths
+
+
+@pytest.mark.parametrize(
+    "args, expected",
+    [
+        (["/foo", "baz", "bar"], "/foo/baz/bar"),
+        (["foo", "baz", "bar"], "foo/baz/bar"),
+        (["https://foo.baz", "bar"], "https://foo.baz/bar"),
+        (["https://foo.baz/", "bar"], "https://foo.baz/bar"),
+        (["foo", "/bar"], "foo/bar"),
+        (["/foo", "baz", "/bar"], "/foo/baz/bar"),
+        (["", "foo", "bar"], "foo/bar"),
+        (["", "/foo", "bar"], "/foo/bar"),
+        (["", "/foo/", "bar"], "/foo/bar"),
+        (["", "", "", "/foo", "bar"], "/foo/bar"),
+        (["", "", "/foo/", "", "bar"], "/foo/bar"),
+        (["", "", "/foo/", "", "", "bar/"], "/foo/bar/"),
+        (["/foo", ""], "/foo/"),
+        (["/foo", "", "", ""], "/foo/"),
+        (["/foo//", "bar"], "/foo//bar"),
+        (["foo"], "foo"),
+        ([""], ""),
+        (["", ""], ""),
+        (["'not ", "sanitized'\0/; rm -rf *"], "'not /sanitized'\0/; rm -rf *"),
+        (["foo/", "/bar"], "foo//bar"),
+        (["foo", "", "/bar"], "foo//bar"),
+        ([b"foo", "bar"], TypeError),
+        (["foo", b"bar"], TypeError),
+        ([None, "foo"], TypeError),
+    ],
+)
+def test_join_paths(args, expected):
+    if isinstance(expected, str):
+        assert join_paths(*args) == expected
+    else:
+        with pytest.raises(expected):
+            _ = join_paths(*args)
