From 8fb0fe96c6909cc34c0160381ac27498a08d30a5 Mon Sep 17 00:00:00 2001 From: Mateusz Mandera Date: Thu, 8 Dec 2022 20:06:30 +0100 Subject: [PATCH] saml: Save SessionIndex in session and use when making a LogoutRequest. This is a useful improvement in general for making correct LogoutRequests to Idps and a necessary one to make SP-initiated logout fully work properly in the desktop application. During desktop auth flow, the user goes through the browser, where they log in through their IdP. This gives them a logged in browser session at the IdP. However, SAML SP-initiated logout is fully conducted within the desktop application. This means that proper information needs to be given to the the IdP in the LogoutRequest to let it associate the LogoutRequest with that logged in session that was established in the browser. SessionIndex is exactly the tool for that in the SAML spec. --- docs/production/authentication-methods.md | 3 -- zerver/tests/test_auth_backends.py | 59 ++++++++++++++++++++++- zproject/backends.py | 51 +++++++++++++++++++- 3 files changed, 106 insertions(+), 7 deletions(-) diff --git a/docs/production/authentication-methods.md b/docs/production/authentication-methods.md index 82b1247be8..420204b4e2 100644 --- a/docs/production/authentication-methods.md +++ b/docs/production/authentication-methods.md @@ -721,9 +721,6 @@ API key. - This implementation doesn't support using `SessionIndex` to limit which sessions are affected; in IdP-initiated Logout it always terminates all logged-in sessions for the user identified in the `NameID`. - In SP-initiated Logout this simply means that Zulip does not include - `SessionIndex` in the `LogoutRequest` to the IdP - however, that doesn't - seem to cause any undesired behavior with Keycloak. - SAML Logout in a configuration where your IdP handles authentication for multiple organizations is not yet supported. diff --git a/zerver/tests/test_auth_backends.py b/zerver/tests/test_auth_backends.py index c13d55f9a5..faf4e8d6cb 100644 --- a/zerver/tests/test_auth_backends.py +++ b/zerver/tests/test_auth_backends.py @@ -1990,7 +1990,11 @@ class SAMLAuthBackendTest(SocialAuthBase): return result def generate_saml_response( - self, email: str, name: str, extra_attributes: Mapping[str, List[str]] = {} + self, + email: str, + name: str, + extra_attributes: Mapping[str, List[str]] = {}, + include_session_index: bool = True, ) -> str: """ The samlresponse.txt fixture has a pre-generated SAMLResponse, @@ -2025,6 +2029,14 @@ class SAMLAuthBackendTest(SocialAuthBase): last_name=last_name, extra_attrs=extra_attrs, ) + + if not include_session_index: + # The SessionIndex value is hard-coded in the fixture, so we can remove it + # in a very hacky, but easy way, without needing to write regexes or parsing XML. + unencoded_saml_response = unencoded_saml_response.replace( + ' SessionIndex="ONELOGIN_a5fde8b09598814d7af2537f865d31c2f7aea831"', "" + ) + # SAMLResponse needs to be base64-encoded. saml_response: str = base64.b64encode(unencoded_saml_response.encode()).decode() @@ -2084,6 +2096,11 @@ class SAMLAuthBackendTest(SocialAuthBase): self.assert_logged_in_user_id(hamlet.id) + # Obtain the SessionIndex value saved in the session, for ensuring it's sent in + # the LogoutRequest later. + session_index = self.client.session["saml_session_index"] + self.assertNotEqual(session_index, None) + result = self.client_post("/accounts/logout/") # A redirect to the IdP is returned. self.assertEqual(result.status_code, 302) @@ -2100,6 +2117,7 @@ class SAMLAuthBackendTest(SocialAuthBase): saml_request = OneLogin_Saml2_Utils.decode_base64_and_inflate(saml_request_encoded).decode() self.assertIn("{hamlet.delivery_email}", saml_request) + self.assertIn(f"{session_index}", saml_request) unencoded_logout_response = self.fixture_data("logoutresponse.txt", type="saml") logout_response: str = base64.b64encode(unencoded_logout_response.encode()).decode() @@ -2142,6 +2160,10 @@ class SAMLAuthBackendTest(SocialAuthBase): self.client.session.flush() self.verify_desktop_flow_end_page(result, self.email, desktop_flow_otp) + # Ensure the SessionIndex gets plumbed through to the final session in the app. + session_index = self.client.session["saml_session_index"] + self.assertNotEqual(session_index, None) + # Now we have a desktop app logged in session. Verify that the logout # request will trigger the SAML SLO flow - it should if we correctly # plumbed through the information about the SAML authentication to the @@ -2584,6 +2606,39 @@ class SAMLAuthBackendTest(SocialAuthBase): ], ) + def test_social_auth_complete_samlresponse_without_session_index_logging(self) -> None: + """ + Tests submitting a valid SAMLResponse to /complete/saml/ without the SessionIndex attribute, + which is optional - to verify we log that unusual situation appropriately. + """ + + # We don't need to test full flow, so just generate the data to POST to /complete/saml/ + # as the last step of the SAML flow. + relay_state = orjson.dumps( + dict( + state_token=SAMLAuthBackend.put_data_in_redis({"subdomain": "zulip"}), + ) + ).decode() + saml_response = self.generate_saml_response( + email=self.example_email("hamlet"), name="King Hamlet", include_session_index=False + ) + + with self.assertLogs(self.logger_string, level="INFO") as m: + post_params = {"RelayState": relay_state, "SAMLResponse": saml_response} + with mock.patch.object(OneLogin_Saml2_Response, "is_valid", return_value=True): + result = self.client_post("/complete/saml/", post_params) + # Redirect to /accounts/login/subdomain/ indicating auth success. + self.assertEqual(result.status_code, 302) + self.assertIn("/accounts/login/subdomain/", result["Location"]) + + # Verify the expected logline exists. + self.assertIn( + self.logger_output( + "/complete/saml/: IdP did not provide SessionIndex in the SAMLResponse.", "info" + ), + m.output, + ) + def test_social_auth_complete_wrong_issuing_idp(self) -> None: relay_state = orjson.dumps( dict( @@ -2636,7 +2691,7 @@ class SAMLAuthBackendTest(SocialAuthBase): self.assertEqual(result.status_code, 302) self.assertIn("login", result["Location"]) - self.assert_length(m.output, 1) + self.assert_length(m.output, 3) def test_social_auth_saml_bad_idp_param_on_login_page(self) -> None: with self.assertLogs(self.logger_string, level="INFO") as m: diff --git a/zproject/backends.py b/zproject/backends.py index 722dbd856d..35ca613600 100644 --- a/zproject/backends.py +++ b/zproject/backends.py @@ -2358,6 +2358,22 @@ class SAMLResponse(SAMLDocument): self.logger.error("Error parsing SAMLResponse: %s", str(e)) return [] + def get_session_index(self) -> Optional[str]: + """ + Returns the SessionIndex from the SAMLResponse. + """ + config = self.backend.generate_saml_config() + saml_settings = OneLogin_Saml2_Settings(config, sp_validation_only=True) + + try: + resp = OneLogin_Saml2_Response( + settings=saml_settings, response=self.encoded_saml_message + ) + return resp.get_session_index() + except self.SAML_PARSING_EXCEPTIONS as e: + self.logger.error("Error parsing SAMLResponse: %s", str(e)) + return None + def is_logout_response(self) -> bool: """ Checks whether the SAMLResponse is a LogoutResponse based on some @@ -2676,6 +2692,8 @@ class SAMLAuthBackend(SocialAuthMixin, SAMLAuth): # or an authentication response. return SAMLSPInitiatedLogout.process_logout_response(saml_document, idp_name) + assert isinstance(saml_document, SAMLResponse) + result = None try: params = relayed_params.copy() @@ -2686,6 +2704,14 @@ class SAMLAuthBackend(SocialAuthMixin, SAMLAuth): # We want the IdP name to be accessible from the social pipeline. self.strategy.session_set("saml_idp_name", idp_name) + session_index = saml_document.get_session_index() + if session_index is None: + # In general IdPs will always provide a SessionIndex, but we can't know + # if some providers might not send it, so we allow it but log the event. + self.logger.info( + "/complete/saml/: IdP did not provide SessionIndex in the SAMLResponse." + ) + self.strategy.session_set("saml_session_index", session_index) # super().auth_complete expects to have RelayState set to the idp_name, # so we need to replace this param. @@ -2769,8 +2795,12 @@ class SAMLAuthBackend(SocialAuthMixin, SAMLAuth): def get_params_to_store_in_authenticated_session(self) -> Dict[str, str]: idp_name = self.strategy.session_get("saml_idp_name") + saml_session_index = self.strategy.session_get("saml_session_index") - return {"social_auth_backend": f"saml:{idp_name}"} + return { + "social_auth_backend": f"saml:{idp_name}", + "saml_session_index": saml_session_index, + } def patch_saml_auth_require_messages_signed(auth: OneLogin_Saml2_Auth) -> None: @@ -2877,6 +2907,20 @@ class SAMLSPInitiatedLogout: return authentication_method.split("saml:")[1] + @classmethod + def get_logged_in_user_session_index(cls, request: HttpRequest) -> Optional[str]: + """ + During SAML authentication, we obtain the SessionIndex value provided + by the IdP and save it in the session. This function can be used + to retrieve it. + """ + # Some asserts to ensure this doesn't get called incorrectly: + assert hasattr(request, "user") + assert isinstance(request.user, UserProfile) + + session_index = request.session.get("saml_session_index") + return session_index + @classmethod def slo_request_to_idp( cls, request: HttpRequest, return_to: Optional[str] = None @@ -2899,10 +2943,13 @@ class SAMLSPInitiatedLogout: idp_name = cls.get_logged_in_user_idp(request) if idp_name is None: raise AssertionError("User not logged in via SAML") + session_index = cls.get_logged_in_user_session_index(request) idp = saml_backend.get_idp(idp_name) auth = saml_backend._create_saml_auth(idp) - slo_url = auth.logout(name_id=user_profile.delivery_email, return_to=return_to) + slo_url = auth.logout( + name_id=user_profile.delivery_email, return_to=return_to, session_index=session_index + ) return HttpResponseRedirect(slo_url)