diff options
| -rw-r--r-- | changelog.d/9376.feature | 1 | ||||
| -rw-r--r-- | synapse/handlers/oidc_handler.py | 74 | ||||
| -rw-r--r-- | synapse/rest/synapse/client/oidc/callback_resource.py | 13 | ||||
| -rw-r--r-- | tests/handlers/test_oidc.py | 26 |
4 files changed, 78 insertions, 36 deletions
diff --git a/changelog.d/9376.feature b/changelog.d/9376.feature new file mode 100644 index 0000000000..68ea21dbdd --- /dev/null +++ b/changelog.d/9376.feature @@ -0,0 +1 @@ +Add support for receiving OpenID Connect authentication responses via form `POST`s rather than `GET`s. diff --git a/synapse/handlers/oidc_handler.py b/synapse/handlers/oidc_handler.py index c00b9c57c6..07db1e31e4 100644 --- a/synapse/handlers/oidc_handler.py +++ b/synapse/handlers/oidc_handler.py @@ -48,7 +48,26 @@ if TYPE_CHECKING: logger = logging.getLogger(__name__) -SESSION_COOKIE_NAME = b"oidc_session" +# we want the cookie to be returned to us even when the request is the POSTed +# result of a form on another domain, as is used with `response_mode=form_post`. +# +# Modern browsers will not do so unless we set SameSite=None; however *older* +# browsers (including all versions of Safari on iOS 12?) don't support +# SameSite=None, and interpret it as SameSite=Strict: +# https://bugs.webkit.org/show_bug.cgi?id=198181 +# +# As a rather painful workaround, we set *two* cookies, one with SameSite=None +# and one with no SameSite, in the hope that at least one of them will get +# back to us. +# +# Secure is necessary for SameSite=None (and, empirically, also breaks things +# on iOS 12.) +# +# 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"), +] #: A token exchanged from the token endpoint, as per RFC6749 sec 5.1. and #: OpenID.Core sec 3.1.3.3. @@ -149,26 +168,33 @@ class OidcHandler: # otherwise, it is presumably a successful response. see: # https://tools.ietf.org/html/rfc6749#section-4.1.2 - # Fetch the session cookie - session = request.getCookie(SESSION_COOKIE_NAME) # type: Optional[bytes] - if session is None: + # Fetch the session cookie. See the comments on SESSION_COOKIES for why there + # are two. + + for cookie_name, _ in _SESSION_COOKIES: + session = request.getCookie(cookie_name) # type: Optional[bytes] + if session is not None: + break + else: logger.info("Received OIDC callback, with no session cookie") self._sso_handler.render_error( request, "missing_session", "No session cookie found" ) return - # Remove the cookie. There is a good chance that if the callback failed + # Remove the cookies. There is a good chance that if the callback failed # once, it will fail next time and the code will already be exchanged. - # Removing it early avoids spamming the provider with token requests. - request.addCookie( - SESSION_COOKIE_NAME, - b"", - path="/_synapse/oidc", - expires="Thu, Jan 01 1970 00:00:00 UTC", - httpOnly=True, - sameSite="lax", - ) + # Removing the cookies early avoids spamming the provider with token requests. + # + # we have to build the header by hand rather than calling request.addCookie + # because the latter does not support SameSite=None + # (https://twistedmatrix.com/trac/ticket/10088) + + for cookie_name, options in _SESSION_COOKIES: + request.cookies.append( + b"%s=; Expires=Thu, Jan 01 1970 00:00:00 UTC; %s" + % (cookie_name, options) + ) # Check for the state query parameter if b"state" not in request.args: @@ -722,14 +748,18 @@ class OidcProvider: ui_auth_session_id=ui_auth_session_id, ), ) - request.addCookie( - SESSION_COOKIE_NAME, - cookie, - path="/_synapse/client/oidc", - max_age="3600", - httpOnly=True, - sameSite="lax", - ) + + # Set the cookies. See the comments on _SESSION_COOKIES for why there are two. + # + # we have to build the header by hand rather than calling request.addCookie + # because the latter does not support SameSite=None + # (https://twistedmatrix.com/trac/ticket/10088) + + for cookie_name, options in _SESSION_COOKIES: + request.cookies.append( + b"%s=%s; Max-Age=3600; %s" + % (cookie_name, cookie.encode("utf-8"), options) + ) metadata = await self.load_metadata() authorization_endpoint = metadata.get("authorization_endpoint") diff --git a/synapse/rest/synapse/client/oidc/callback_resource.py b/synapse/rest/synapse/client/oidc/callback_resource.py index f7a0bc4bdb..1af33f0a45 100644 --- a/synapse/rest/synapse/client/oidc/callback_resource.py +++ b/synapse/rest/synapse/client/oidc/callback_resource.py @@ -12,19 +12,30 @@ # WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. # See the License for the specific language governing permissions and # limitations under the License. + import logging +from typing import TYPE_CHECKING from synapse.http.server import DirectServeHtmlResource +if TYPE_CHECKING: + from synapse.server import HomeServer + logger = logging.getLogger(__name__) class OIDCCallbackResource(DirectServeHtmlResource): isLeaf = 1 - def __init__(self, hs): + def __init__(self, hs: "HomeServer"): super().__init__() self._oidc_handler = hs.get_oidc_handler() async def _async_render_GET(self, request): await self._oidc_handler.handle_oidc_callback(request) + + async def _async_render_POST(self, request): + # the auth response can be returned via an x-www-form-urlencoded form instead + # of GET params, as per + # https://openid.net/specs/oauth-v2-form-post-response-mode-1_0.html. + await self._oidc_handler.handle_oidc_callback(request) diff --git a/tests/handlers/test_oidc.py b/tests/handlers/test_oidc.py index bdd2e02eae..cf1de28fa9 100644 --- a/tests/handlers/test_oidc.py +++ b/tests/handlers/test_oidc.py @@ -327,7 +327,9 @@ class OidcHandlerTestCase(HomeserverTestCase): def test_redirect_request(self): """The redirect request has the right arguments & generates a valid session cookie.""" - req = Mock(spec=["addCookie"]) + req = Mock(spec=["cookies"]) + req.cookies = [] + url = self.get_success( self.provider.handle_redirect_request(req, b"http://client/redirect") ) @@ -346,19 +348,16 @@ class OidcHandlerTestCase(HomeserverTestCase): self.assertEqual(len(params["state"]), 1) self.assertEqual(len(params["nonce"]), 1) - # Check what is in the cookie - # note: python3.5 mock does not have the .called_once() method - calls = req.addCookie.call_args_list - self.assertEqual(len(calls), 1) # called once - # For some reason, call.args does not work with python3.5 - args = calls[0][0] - kwargs = calls[0][1] + # Check what is in the cookies + self.assertEqual(len(req.cookies), 2) # two cookies + cookie_header = req.cookies[0] # The cookie name and path don't really matter, just that it has to be coherent # between the callback & redirect handlers. - self.assertEqual(args[0], b"oidc_session") - self.assertEqual(kwargs["path"], "/_synapse/client/oidc") - cookie = args[1] + parts = [p.strip() for p in cookie_header.split(b";")] + self.assertIn(b"Path=/_synapse/client/oidc", parts) + name, cookie = parts[0].split(b"=") + self.assertEqual(name, b"oidc_session") macaroon = pymacaroons.Macaroon.deserialize(cookie) state = self.handler._token_generator._get_value_from_macaroon( @@ -489,7 +488,7 @@ class OidcHandlerTestCase(HomeserverTestCase): def test_callback_session(self): """The callback verifies the session presence and validity""" - request = Mock(spec=["args", "getCookie", "addCookie"]) + request = Mock(spec=["args", "getCookie", "cookies"]) # Missing cookie request.args = {} @@ -943,13 +942,14 @@ def _build_callback_request( spec=[ "args", "getCookie", - "addCookie", + "cookies", "requestHeaders", "getClientIP", "getHeader", ] ) + request.cookies = [] request.getCookie.return_value = session request.args = {} request.args[b"code"] = [code.encode("utf-8")] |