Support for form_post in OIDC responses

changelog.d/9376.feature

@@ -0,0 +1 @@
+Add support for receiving OpenID Connect authentication responses via form `POST`s rather than `GET`s.

synapse/handlers/oidc_handler.py

@@ -48,6 +48,7 @@
 logger = logging.getLogger(__name__)
 
 SESSION_COOKIE_NAME = b"oidc_session"
+SESSION_COOKIE_NAME_IOS_HACK = b"oidc_session_no_samesite"
 
 #: A token exchanged from the token endpoint, as per RFC6749 sec 5.1. and
 #: OpenID.Core sec 3.1.3.3.
@@ -149,26 +150,29 @@ async def handle_oidc_callback(self, request: SynapseRequest) -> None:
         # otherwise, it is presumably a successful response. see:
         #   https://tools.ietf.org/html/rfc6749#section-4.1.2
 
-        # Fetch the session cookie
+        # Fetch the session cookie. See notes in handle_redirect_request about why
+        # we have two of these.
         session = request.getCookie(SESSION_COOKIE_NAME)  # type: Optional[bytes]
+        if session is None:
+            session = request.getCookie(SESSION_COOKIE_NAME_IOS_HACK)
         if session is None:
             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.
+        for cookie_name, options in [
+            (SESSION_COOKIE_NAME, b"; SameSite=None"),
+            (SESSION_COOKIE_NAME_IOS_HACK, b""),
+        ]:
+            request.cookies.append(
+                b"%s=; Path=/_synapse/client/oidc; Expires=Thu, Jan 01 1970 00:00:00 UTC; "
+                b"HttpOnly; Secure%s" % (cookie_name, options)
+            )
 
         # Check for the state query parameter
         if b"state" not in request.args:
@@ -693,14 +697,33 @@ async def handle_redirect_request(
                 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",
-        )
+
+        # 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.)
+        #
+        # we have to build the cookie by hand rather than calling request.addCookie
+        # to work around https://twistedmatrix.com/trac/ticket/10088
+        #
+        for cookie_name, options in [
+            (SESSION_COOKIE_NAME, b"; Secure; SameSite=None"),
+            (SESSION_COOKIE_NAME_IOS_HACK, b""),
+        ]:
+            request.cookies.append(
+                b"%s=%s; Path=/_synapse/client/oidc; Max-Age=3600; HttpOnly%s"
+                % (cookie_name, cookie.encode("utf-8"), options)
+            )
 
         metadata = await self.load_metadata()
         authorization_endpoint = metadata.get("authorization_endpoint")

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)

tests/handlers/test_oidc.py

@@ -308,7 +308,9 @@ def test_skip_verification(self):
 
     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")
         )
@@ -327,19 +329,16 @@ def test_redirect_request(self):
         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(
@@ -470,7 +469,7 @@ def test_callback(self):
 
     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 = {}
@@ -910,13 +909,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")]