Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ModuleRouter: support paths in BASE #405

Open
wants to merge 11 commits into
base: master
Choose a base branch
from
2 changes: 1 addition & 1 deletion doc/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,7 @@ 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 |
Expand Down
2 changes: 1 addition & 1 deletion src/satosa/backends/base.py
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ def __init__(self, auth_callback_func, internal_attributes, base_url, name):
self.auth_callback_func = auth_callback_func
self.internal_attributes = internal_attributes
self.converter = AttributeMapper(internal_attributes)
self.base_url = base_url
self.base_url = base_url.rstrip("/") if base_url else ""
bajnokk marked this conversation as resolved.
Show resolved Hide resolved
self.name = name

def start_auth(self, context, internal_request):
Expand Down
9 changes: 7 additions & 2 deletions src/satosa/base.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
import uuid

from saml2.s_utils import UnknownSystemEntity
from urllib.parse import urlparse

from satosa import util
from .context import Context
Expand Down Expand Up @@ -38,6 +39,8 @@ def __init__(self, config):
"""
self.config = config

base_path = urlparse(self.config["BASE"]).path.lstrip("/")

logger.info("Loading backend modules...")
backends = load_backends(self.config, self._auth_resp_callback_func,
self.config["INTERNAL_ATTRIBUTES"])
Expand All @@ -63,8 +66,10 @@ def __init__(self, config):
self.config["BASE"]))
self._link_micro_services(self.response_micro_services, self._auth_resp_finish)

self.module_router = ModuleRouter(frontends, backends,
self.request_micro_services + self.response_micro_services)
self.module_router = ModuleRouter(frontends,
backends,
self.request_micro_services + self.response_micro_services,
base_path)

def _link_micro_services(self, micro_services, finisher):
if not micro_services:
Expand Down
4 changes: 0 additions & 4 deletions src/satosa/context.py
Original file line number Diff line number Diff line change
Expand Up @@ -76,10 +76,6 @@ def path(self, p):
raise ValueError("path can't start with '/'")
self._path = p

def target_entity_id_from_path(self):
target_entity_id = self.path.split("/")[1]
return target_entity_id

def decorate(self, key, value):
"""
Add information to the context
Expand Down
10 changes: 9 additions & 1 deletion src/satosa/frontends/base.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,9 @@
"""
from ..attribute_mapping import AttributeMapper

import os.path
from urllib.parse import urlparse


class FrontendModule(object):
"""
Expand All @@ -14,17 +17,22 @@ 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
self.base_url = base_url or ""
bajnokk marked this conversation as resolved.
Show resolved Hide resolved
self.name = name
self.endpoint_baseurl = os.path.join(self.base_url, self.name)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

os.path.join will not work on certain platforms (see Windows..).

To join URLs with paths using a function, use urllib.parse.urljoin; but it also has caveats (paths that begin with / will be considered the root, and bases that do not end with a / will be considered a file and will be truncated).

The simplest approach is to just concatenate with a / (ie, f"{self.base_url}/{self.name}").

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I always forget about Windows, good catch, thanks.

The biggest advantage of os.path.join that it handles double slashes and empty strings intelligently. I'm thinking about adding a path_join function to util.py which would save the work of working around the empty base_path with "/".join([foo, bar]) all the time. (And I didn't want to add Python >=3.9 dependency with str.removesuffix())

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would probably name the function join_url_paths (verb first).

Note that the latest pysaml2 already requires Python 3.9 and SATOSA will be updated to require it too. IdentityPython projects try to be compatible with the python that ships on the latest Debian stable release (which is now Python 3.9).
So, requiring Python 3.9 is fine; but no newer atm.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The updated patch contains a join_paths implementation, which tries to handle separators a little bit more intelligently than a simple concatenation. I've replaced all erroneous os.path.join calls to join_paths, but didn't replace {}/{}.format(foo, bar) all over the code, since this appears way too many times.

self.endpoint_basepath = urlparse(self.endpoint_baseurl).path.lstrip("/")

def handle_authn_response(self, context, internal_resp):
"""
Expand Down
45 changes: 33 additions & 12 deletions src/satosa/frontends/openid_connect.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

import json
import logging
import os.path
from collections import defaultdict
from urllib.parse import urlencode, urlparse

Expand Down Expand Up @@ -97,7 +98,6 @@ def __init__(self, auth_req_callback_func, internal_attributes, conf, base_url,
else:
cdb = {}

self.endpoint_baseurl = "{}/{}".format(self.base_url, self.name)
self.provider = _create_provider(
provider_config,
self.endpoint_baseurl,
Expand Down Expand Up @@ -173,6 +173,18 @@ 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
"""
# 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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OpenID Providers supporting Discovery MUST make a JSON document available at the path formed by concatenating the string /.well-known/openid-configuration to the Issuer.

The issuer is discovered through a WebFinger request for resources of http://openid.net/specs/connect/1.0/issuer relation. The response contains one or more href properties with the Issuer URL, which is allowed to contain a path.

What we define is that the frontend contains the frontend name as a path component and under that you can query the well-known documents.

With that in mind we can have multiple frontends each with its own discovery.

The problem is that atm, the base_url is used instead of endpoint_baseurl.

We can introduce a configuration option to select between the two behaviours, or (even better) introduce a configuration to set the discovery URL for a frontend.


At some point I would like to invert this logic; instead of a component defining paths of URLs internally that mapped to functionality (which the routing module has to match to based on some rules), there should be URLs as entrypoints mapped to functionality directly (as it happens within most web frameworks - flask, django, fastapi, etc).

Copy link
Contributor Author

@bajnokk bajnokk Mar 15, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem is that atm, the base_url is used instead of endpoint_baseurl.
We can introduce a configuration option to select between the two behaviours, or (even better) introduce a configuration to set the discovery URL for a frontend.

Would you agree to add a use_module_name_in_issuer option (default False for backward compatibility, but the examples changed to True)?

A more subtle change but also harder to document alternative would be to make the assignment in

provider_config["issuer"] = base_url
optional, so that one could set the issuer manually under the provider dict.

Is any of the two OK with you, or am I misunderstanding the problem?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, both are ok with me. As long as we provide a way to configure things to work as before, it is fine to introduce such changes.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The updated patchset contains a commit, which does not perform the above assignment if the provider config has the issuer set. I did some research in git log, but I think it was just in this way forever.
I added a brief explanation to the example configuration, too.
Note that this could be a breaking change for those, who had a lurking "issuer" in their provider config, but since it's never been supported, I'm inclined to go this way rather than adding a new "fix-something-but-dont-break-old-config" type of configuration option.

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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
jwks_uri = ("^{}/jwks$".format(self.endpoint_basepath), self.jwks)
jwks_uri = ("^{}$".format(join_paths(self.endpoint_basepath, "jwks")), self.jwks)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are not equivalents. The equivalent form would be
jwks_uri = ("^{}$".format(join_paths("/", self.endpoint_basepath, "jwks")), self.jwks)
Or is it what you are suggesting that the leading '/' should not be present when endpoint_basepath is the empty string?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh, I didn't think that self.endpoint_basepath would be empty.
Previously, self.name was used and was always filled; in general, we do not match routes that start with /.

So, I still think this is fine, but let me know if I'm skipping over anything.


backend_name = None
if len(backend_names) != 1:
# only supports one backend since there currently is no way to publish multiple authorization endpoints
Expand All @@ -189,40 +201,49 @@ def register_endpoints(self, backend_names):
else:
backend_name = backend_names[0]

provider_config = ("^.well-known/openid-configuration$", self.provider_config)
jwks_uri = ("^{}/jwks$".format(self.name), self.jwks)

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.name, 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.name, 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.name, 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.name, RegistrationEndpoint.url),
"^{}".format(
os.path.join(self.endpoint_basepath, RegistrationEndpoint.url)
),
self.client_registration,
)
url_map.append(client_registration)
Expand Down
3 changes: 2 additions & 1 deletion src/satosa/frontends/ping.py
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import logging
import os.path

import satosa.logging_util as lu
from satosa.frontends.base import FrontendModule
Expand Down Expand Up @@ -43,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(self.name), self.ping_endpoint)]
url_map = [("^{}".format(os.path.join(self.endpoint_basepath, self.name)), self.ping_endpoint)]

return url_map

Expand Down
42 changes: 28 additions & 14 deletions src/satosa/frontends/saml2.py
Original file line number Diff line number Diff line change
Expand Up @@ -117,7 +117,7 @@ def register_endpoints(self, backend_names):

if self.enable_metadata_reload():
url_map.append(
("^%s/%s$" % (self.name, "reload-metadata"), self._reload_metadata))
("^%s/%s$" % (self.endpoint_basepath, "reload-metadata"), self._reload_metadata))

self.idp_config = self._build_idp_config_endpoints(
self.config[self.KEY_IDP_CONFIG], backend_names)
Expand Down Expand Up @@ -511,15 +511,19 @@ def _register_endpoints(self, providers):
"""
url_map = []

backend_providers = "|".join(providers)
base_path = urlparse(self.base_url).path.lstrip("/")
if base_path:
base_path = base_path + "/"
bajnokk marked this conversation as resolved.
Show resolved Hide resolved
for endp_category in self.endpoints:
for binding, endp in self.endpoints[endp_category].items():
valid_providers = ""
for provider in providers:
valid_providers = "{}|^{}".format(valid_providers, provider)
valid_providers = valid_providers.lstrip("|")
parsed_endp = urlparse(endp)
url_map.append(("(%s)/%s$" % (valid_providers, parsed_endp.path),
functools.partial(self.handle_authn_request, binding_in=binding)))
endp_path = urlparse(endp).path
url_map.append(
(
"^{}({})/{}$".format(base_path, backend_providers, endp_path),
bajnokk marked this conversation as resolved.
Show resolved Hide resolved
functools.partial(self.handle_authn_request, binding_in=binding)
)
)

if self.expose_entityid_endpoint():
logger.debug("Exposing frontend entity endpoint = {}".format(self.idp.config.entityid))
Expand Down Expand Up @@ -675,11 +679,18 @@ def _load_idp_dynamic_endpoints(self, context):
:param context:
:return: An idp server
"""
target_entity_id = context.target_entity_id_from_path()
target_entity_id = self._target_entity_id_from_path(context.path)
idp_conf_file = self._load_endpoints_to_config(context.target_backend, target_entity_id)
idp_config = IdPConfig().load(idp_conf_file)
return Server(config=idp_config)

def _target_entity_id_from_path(self, request_path):
c00kiemon5ter marked this conversation as resolved.
Show resolved Hide resolved
path = request_path.lstrip("/")
base_path = urlparse(self.base_url).path.lstrip("/")
if base_path and path.startswith(base_path):
path = path[len(base_path):].lstrip("/")
return path.split("/")[1]

def _load_idp_dynamic_entity_id(self, state):
"""
Loads an idp server with the entity id saved in state
Expand All @@ -705,7 +716,7 @@ def handle_authn_request(self, context, binding_in):
:type binding_in: str
:rtype: satosa.response.Response
"""
target_entity_id = context.target_entity_id_from_path()
target_entity_id = self._target_entity_id_from_path(context.path)
target_entity_id = urlsafe_b64decode(target_entity_id).decode()
context.decorate(Context.KEY_TARGET_ENTITYID, target_entity_id)

Expand All @@ -723,7 +734,7 @@ def _create_state_data(self, context, resp_args, relay_state):
:rtype: dict[str, dict[str, str] | str]
"""
state = super()._create_state_data(context, resp_args, relay_state)
state["target_entity_id"] = context.target_entity_id_from_path()
state["target_entity_id"] = self._target_entity_id_from_path(context.path)
return state

def handle_backend_error(self, exception):
Expand Down Expand Up @@ -758,13 +769,16 @@ def _register_endpoints(self, providers):
"""
url_map = []

backend_providers = "|".join(providers)
base_path = urlparse(self.base_url).path.lstrip("/")
if base_path:
base_path = base_path + "/"
bajnokk marked this conversation as resolved.
Show resolved Hide resolved
for endp_category in self.endpoints:
for binding, endp in self.endpoints[endp_category].items():
valid_providers = "|^".join(providers)
parsed_endp = urlparse(endp)
endp_path = urlparse(endp).path
url_map.append(
(
r"(^{})/\S+/{}".format(valid_providers, parsed_endp.path),
"^{}({})/\S+/{}$".format(base_path, backend_providers, endp_path),
bajnokk marked this conversation as resolved.
Show resolved Hide resolved
functools.partial(self.handle_authn_request, binding_in=binding)
)
)
Expand Down
12 changes: 11 additions & 1 deletion src/satosa/micro_services/account_linking.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
"""
import json
import logging
import os.path

import requests
from jwkest.jwk import rsa_load, RSAKey
Expand Down Expand Up @@ -161,4 +162,13 @@ def register_endpoints(self):

:return: A list of endpoints bound to a function
"""
return [("^account_linking%s$" % self.endpoint, self._handle_al_response)]
return [
(
"^{}$".format(
os.path.join(
self.base_path, "account_linking", self.endpoint.lstrip("/")
)
),
self._handle_al_response,
)
]
2 changes: 2 additions & 0 deletions src/satosa/micro_services/base.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
Micro service for SATOSA
"""
import logging
from urllib.parse import urlparse

logger = logging.getLogger(__name__)

Expand All @@ -14,6 +15,7 @@ class MicroService(object):
def __init__(self, name, base_url, **kwargs):
self.name = name
self.base_url = base_url
self.base_path = urlparse(base_url).path.lstrip("/")
bajnokk marked this conversation as resolved.
Show resolved Hide resolved
self.next = None

def process(self, context, data):
Expand Down
12 changes: 11 additions & 1 deletion src/satosa/micro_services/consent.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
import hashlib
import json
import logging
import os.path
from base64 import urlsafe_b64encode

import requests
Expand Down Expand Up @@ -238,4 +239,13 @@ def register_endpoints(self):

:return: A list of endpoints bound to a function
"""
return [("^consent%s$" % self.endpoint, self._handle_consent_response)]
return [
(
"^{}$".format(
os.path.join(
self.base_path, "consent", self.endpoint.lstrip("/")
)
),
self._handle_consent_response,
)
]
Loading