From cb58351a39384c109755f7c394a9dcc89bd4e070 Mon Sep 17 00:00:00 2001 From: Andrew Morgan Date: Thu, 1 Apr 2021 16:35:44 +0100 Subject: [PATCH 1/5] Allow OIDC cookies to work on non-root public baseurls --- synapse/config/server.py | 8 ++++---- synapse/handlers/oidc_handler.py | 23 ++++++++++++++++++----- 2 files changed, 22 insertions(+), 9 deletions(-) diff --git a/synapse/config/server.py b/synapse/config/server.py index 02b86b11a5ee..21ca7b33e38d 100644 --- a/synapse/config/server.py +++ b/synapse/config/server.py @@ -235,7 +235,11 @@ def read_config(self, config, **kwargs): self.print_pidfile = config.get("print_pidfile") self.user_agent_suffix = config.get("user_agent_suffix") self.use_frozen_dicts = config.get("use_frozen_dicts", False) + self.public_baseurl = config.get("public_baseurl") + if self.public_baseurl is not None: + if self.public_baseurl[-1] != "/": + self.public_baseurl += "/" # Whether to enable user presence. presence_config = config.get("presence") or {} @@ -407,10 +411,6 @@ def read_config(self, config, **kwargs): config_path=("federation_ip_range_blacklist",), ) - if self.public_baseurl is not None: - if self.public_baseurl[-1] != "/": - self.public_baseurl += "/" - # (undocumented) option for torturing the worker-mode replication a bit, # for testing. The value defines the number of milliseconds to pause before # sending out any replication updates. diff --git a/synapse/handlers/oidc_handler.py b/synapse/handlers/oidc_handler.py index b156196a70b7..2985d835f3be 100644 --- a/synapse/handlers/oidc_handler.py +++ b/synapse/handlers/oidc_handler.py @@ -15,7 +15,7 @@ import inspect import logging from typing import TYPE_CHECKING, Dict, Generic, List, Optional, TypeVar, Union -from urllib.parse import urlencode +from urllib.parse import urlencode, urlparse import attr import pymacaroons @@ -71,8 +71,8 @@ # # Here we have the names of the cookies, and the options we use to set them. _SESSION_COOKIES = [ - (b"oidc_session", b"Path=/_synapse/client/oidc; HttpOnly; Secure; SameSite=None"), - (b"oidc_session_no_samesite", b"Path=/_synapse/client/oidc; HttpOnly"), + (b"oidc_session", b"HttpOnly; Secure; SameSite=None"), + (b"oidc_session_no_samesite", b"HttpOnly"), ] #: A token exchanged from the token endpoint, as per RFC6749 sec 5.1. and @@ -282,6 +282,13 @@ def __init__( self._config = provider self._callback_url = hs.config.oidc_callback_url # type: str + # TODO: This is pretty much a hack to get the path specified by public_baseurl. + # It'd probably be nicer to have a config option that lets you specify a custom + # path, which we'd then use here. + self._callback_path_prefix = urlparse(hs.config.public_baseurl).path + if self._callback_path_prefix.endswith("/"): + self._callback_path_prefix = self._callback_path_prefix[:-1] + self._oidc_attribute_requirements = provider.attribute_requirements self._scopes = provider.scopes self._user_profile_method = provider.user_profile_method @@ -782,8 +789,14 @@ async def handle_redirect_request( for cookie_name, options in _SESSION_COOKIES: request.cookies.append( - b"%s=%s; Max-Age=3600; %s" - % (cookie_name, cookie.encode("utf-8"), options) + b"%s=%s; Max-Age=3600; Path=%s; %s" + % ( + cookie_name, + cookie.encode("utf-8"), + self._callback_path_prefix.encode("utf-8") + + b"/_synapse/client/oidc", + options, + ) ) metadata = await self.load_metadata() From 02ba232d04c89932ce6546f4b7e1ae7c6cc00e50 Mon Sep 17 00:00:00 2001 From: Andrew Morgan Date: Thu, 1 Apr 2021 16:40:38 +0100 Subject: [PATCH 2/5] Changelog --- changelog.d/9726.bugfix | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/9726.bugfix diff --git a/changelog.d/9726.bugfix b/changelog.d/9726.bugfix new file mode 100644 index 000000000000..4ba0b24327cb --- /dev/null +++ b/changelog.d/9726.bugfix @@ -0,0 +1 @@ +Fixes the OIDC SSO flow when using a `public_baseurl` value including a non-root URL path. \ No newline at end of file From a8ea1a3ee0dc587ca60c895415b1e500aba8441e Mon Sep 17 00:00:00 2001 From: Andrew Morgan Date: Mon, 19 Apr 2021 15:59:34 +0100 Subject: [PATCH 3/5] Append standard OIDC callback path in the init function --- synapse/handlers/oidc_handler.py | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/synapse/handlers/oidc_handler.py b/synapse/handlers/oidc_handler.py index 2985d835f3be..8dc680c62972 100644 --- a/synapse/handlers/oidc_handler.py +++ b/synapse/handlers/oidc_handler.py @@ -285,9 +285,15 @@ def __init__( # TODO: This is pretty much a hack to get the path specified by public_baseurl. # It'd probably be nicer to have a config option that lets you specify a custom # path, which we'd then use here. - self._callback_path_prefix = urlparse(hs.config.public_baseurl).path - if self._callback_path_prefix.endswith("/"): - self._callback_path_prefix = self._callback_path_prefix[:-1] + public_baseurl_path = urlparse(hs.config.public_baseurl).path + if public_baseurl_path.endswith("/"): + public_baseurl_path = public_baseurl_path[:-1] + + # Calculate the prefix for OIDC callback paths based on the public_baseurl. + # We'll insert this into the Path= parameter of any session cookies we set. + self._callback_path_prefix = ( + public_baseurl_path.encode("utf-8") + b"/_synapse/client/oidc" + ) self._oidc_attribute_requirements = provider.attribute_requirements self._scopes = provider.scopes @@ -793,8 +799,7 @@ async def handle_redirect_request( % ( cookie_name, cookie.encode("utf-8"), - self._callback_path_prefix.encode("utf-8") - + b"/_synapse/client/oidc", + self._callback_path_prefix, options, ) ) From 67334d78a2145a5af4a2d526e449430c391120eb Mon Sep 17 00:00:00 2001 From: Andrew Morgan Date: Mon, 19 Apr 2021 18:27:18 +0100 Subject: [PATCH 4/5] Remove TODO and replace with existing comment --- synapse/handlers/oidc_handler.py | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/synapse/handlers/oidc_handler.py b/synapse/handlers/oidc_handler.py index 8dc680c62972..36eae71021e0 100644 --- a/synapse/handlers/oidc_handler.py +++ b/synapse/handlers/oidc_handler.py @@ -282,15 +282,13 @@ def __init__( self._config = provider self._callback_url = hs.config.oidc_callback_url # type: str - # TODO: This is pretty much a hack to get the path specified by public_baseurl. - # It'd probably be nicer to have a config option that lets you specify a custom - # path, which we'd then use here. - public_baseurl_path = urlparse(hs.config.public_baseurl).path + # Calculate the prefix for OIDC callback paths based on the public_baseurl. + # We'll insert this into the Path= parameter of any session cookies we set. + + public_baseurl_path = urlparse(hs.config.server.public_baseurl).path if public_baseurl_path.endswith("/"): public_baseurl_path = public_baseurl_path[:-1] - # Calculate the prefix for OIDC callback paths based on the public_baseurl. - # We'll insert this into the Path= parameter of any session cookies we set. self._callback_path_prefix = ( public_baseurl_path.encode("utf-8") + b"/_synapse/client/oidc" ) From 1bde9d3d796fccd7cd3ade38450063501f0d3e7a Mon Sep 17 00:00:00 2001 From: Andrew Morgan Date: Mon, 19 Apr 2021 18:29:08 +0100 Subject: [PATCH 5/5] public_baseurl must end with / --- synapse/handlers/oidc_handler.py | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/synapse/handlers/oidc_handler.py b/synapse/handlers/oidc_handler.py index 36eae71021e0..a5894550f59f 100644 --- a/synapse/handlers/oidc_handler.py +++ b/synapse/handlers/oidc_handler.py @@ -284,13 +284,9 @@ def __init__( # Calculate the prefix for OIDC callback paths based on the public_baseurl. # We'll insert this into the Path= parameter of any session cookies we set. - public_baseurl_path = urlparse(hs.config.server.public_baseurl).path - if public_baseurl_path.endswith("/"): - public_baseurl_path = public_baseurl_path[:-1] - self._callback_path_prefix = ( - public_baseurl_path.encode("utf-8") + b"/_synapse/client/oidc" + public_baseurl_path.encode("utf-8") + b"_synapse/client/oidc" ) self._oidc_attribute_requirements = provider.attribute_requirements