ModuleRouter: support paths in BASE #405

bajnokk · 2022-07-20T13:36:10Z

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.

Additionally, DEBUG logging was configured for the tests so that the
debug logs are accessible during testing.

Fixes #404

All Submissions:

Have you checked to ensure there aren't other open Pull Requests for the same update/change?
Have you added an explanation of what problem you are trying to solve with this PR?
Have you added information on what your changes do and why you chose this as your solution?
Have you written new tests for your changes?
Does your submission pass tests?
This project follows PEP8 style guide. Have you run your code against the 'flake8' linter?

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. Additionally, DEBUG logging was configured for the tests so that the debug logs are accessible during testing.

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.

Avoid messing README.md with an unwanted line break.

c00kiemon5ter · 2023-03-14T11:51:07Z

src/satosa/frontends/openid_connect.py

+        # 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.


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).

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

SATOSA/src/satosa/frontends/openid_connect.py

Line 66 in fc9374c

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?

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.

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.

c00kiemon5ter · 2023-03-14T12:06:32Z

src/satosa/frontends/base.py

        self.name = name
+        self.endpoint_baseurl = os.path.join(self.base_url, self.name)


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}").

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())

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.

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.

tests/conftest.py

tests/satosa/frontends/test_openid_connect.py

c00kiemon5ter · 2023-03-14T12:24:07Z

src/satosa/routing.py


-        if backend in self.backends:
+        backend = self._find_backend(context.path)


there was an idea here to reuse the method that is invoked to find a frontend;

see #279

I must say that I can't fully understand all the _find_* methods. My motives for yet another implementation is that it is looking at the very specific part of the request path and tries to find the backend name. I'm probably mistaken but I think that the reused version would try to match the backend endpoint, which is not present in a frontend request.

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.

Even though the OIDC provider configuration has an element for setting the issuer, for some reason it was rewritten to BASE unconditionally, but this has broken provider endpoint discovery when multiple OIDC frontends were in use.

src/satosa/backends/base.py

c00kiemon5ter · 2023-06-11T20:10:57Z

src/satosa/frontends/openid_connect.py

+            "^{}$".format(join_paths(urlparse(issuer).path.lstrip("/"), autoconf_path)),
+            self.provider_config,
+        )
+        jwks_uri = ("^{}/jwks$".format(self.endpoint_basepath), self.jwks)


Suggested change

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

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

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?

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.

src/satosa/frontends/openid_connect.py

example/plugins/frontends/openid_connect_frontend.yaml.example

src/satosa/micro_services/base.py

src/satosa/frontends/saml2.py

src/satosa/routing.py

Add base_path and endpoint_basepath to backend and micro_services Co-authored-by: Ivan Kanakarakis <ivan.kanak@gmail.com>

c00kiemon5ter

Simplify the join_paths function

src/satosa/util.py

Setting an alternative issuer should not be an encouraged setup, although provider discovery should work either way. The recommended setting is to use the BASE as the issuer, and we can leverage the agressive configuration value replacement logic, which rewrites all occurences of <base_url> to the value of BASE. The unit test was modified to guarantee this behaviour, though.

Add base_path and endpoint_basepath to backend and micro_services Co-authored-by: Ivan Kanakarakis <ivan.kanak@gmail.com>

bajnokk and others added 2 commits March 8, 2023 12:38

bajnokk force-pushed the base-path branch from 862c400 to b253b30 Compare March 13, 2023 08:49

fixup! ModuleRouter: support paths in BASE

fc9374c

Avoid messing README.md with an unwanted line break.

c00kiemon5ter reviewed Mar 14, 2023

View reviewed changes

tests/conftest.py Show resolved Hide resolved

c00kiemon5ter reviewed Mar 14, 2023

View reviewed changes

tests/satosa/frontends/test_openid_connect.py Show resolved Hide resolved

c00kiemon5ter reviewed Mar 14, 2023

View reviewed changes

bajnokk added 3 commits March 17, 2023 21:52

fixup! frontends/openid_connect: support issuer override via provider

0c8ab4f

c00kiemon5ter requested changes Jun 11, 2023

View reviewed changes

bajnokk added a commit to bajnokk/SATOSA that referenced this pull request Jun 12, 2023

PR IdentityPython#405: apply changes from review

91c0ac2

Add base_path and endpoint_basepath to backend and micro_services Co-authored-by: Ivan Kanakarakis <ivan.kanak@gmail.com>

c00kiemon5ter requested changes Jun 12, 2023

View reviewed changes

src/satosa/util.py Outdated Show resolved Hide resolved

src/satosa/util.py Outdated Show resolved Hide resolved

bajnokk added a commit to bajnokk/SATOSA that referenced this pull request Jun 13, 2023

fixup! PR IdentityPython#405: apply changes from review

73f632d

bajnokk requested a review from c00kiemon5ter June 15, 2023 07:14

c00kiemon5ter self-assigned this Sep 26, 2023

c00kiemon5ter added the enhancement label Nov 7, 2023

bajnokk and others added 4 commits November 24, 2023 14:57

PR IdentityPython#405: apply changes from review

740ba28

Add base_path and endpoint_basepath to backend and micro_services Co-authored-by: Ivan Kanakarakis <ivan.kanak@gmail.com>

fixup! PR IdentityPython#405: apply changes from review

f975308

fixup! amend! fixup! ModuleRouter: support paths in BASE

f166869

fixup! fixup! amend! fixup! ModuleRouter: support paths in BASE

e25c9e0

bajnokk force-pushed the base-path branch from da0e4c6 to e25c9e0 Compare November 24, 2023 13:58

bajnokk added a commit to OneIdentity/SATOSA that referenced this pull request Nov 24, 2023

PR IdentityPython#405: apply changes from review

5878cbd

Add base_path and endpoint_basepath to backend and micro_services Co-authored-by: Ivan Kanakarakis <ivan.kanak@gmail.com>

bajnokk mentioned this pull request Jan 3, 2024

Support paths in BASE during routing #451

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ModuleRouter: support paths in BASE #405

ModuleRouter: support paths in BASE #405

bajnokk commented Jul 20, 2022

c00kiemon5ter Mar 14, 2023

bajnokk Mar 15, 2023 •

edited

Loading

c00kiemon5ter Mar 15, 2023

bajnokk Mar 17, 2023

c00kiemon5ter Mar 14, 2023

bajnokk Mar 15, 2023

c00kiemon5ter Mar 15, 2023

bajnokk Mar 17, 2023

c00kiemon5ter Mar 14, 2023

bajnokk Mar 16, 2023

c00kiemon5ter Jun 11, 2023

bajnokk Jun 12, 2023

c00kiemon5ter Jun 12, 2023

c00kiemon5ter left a comment

		self.name = name
		self.endpoint_baseurl = os.path.join(self.base_url, self.name)


		if backend in self.backends:
		backend = self._find_backend(context.path)

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

ModuleRouter: support paths in BASE #405

Are you sure you want to change the base?

ModuleRouter: support paths in BASE #405

Conversation

bajnokk commented Jul 20, 2022

All Submissions:

Choose a reason for hiding this comment

bajnokk Mar 15, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

c00kiemon5ter left a comment

Choose a reason for hiding this comment

bajnokk Mar 15, 2023 •

edited

Loading