saml: Figure out the idp from SAMLResponse.

Instead of plumbing the idp to /complete/saml/ through redis, it's much
more natural to just figure it out from the SAMLResponse, because the
information is there.
This is also a preparatory step for adding IdP-initiated sign in, for
which it is important for /complete/saml/ to be able to figure out which
IdP the request is coming from.

zerver/tests/fixtures/saml/samlresponse.txt

@@ -1,4 +1,4 @@
-<?xml version="1.0" encoding="UTF-8"?><saml2p:Response Destination="http://zulip.testserver/complete/saml/" ID="id544612569720442296425226" InResponseTo="ONELOGIN_a5fde8b09598814d7af2537f865d31c2f7aea831" IssueInstant="2019-09-25T01:02:02.120Z" Version="2.0" xmlns:saml2p="urn:oasis:names:tc:SAML:2.0:protocol" xmlns:xs="http://www.w3.org/2001/XMLSchema"><saml2:Issuer Format="urn:oasis:names:tc:SAML:2.0:nameid-format:entity" xmlns:saml2="urn:oasis:names:tc:SAML:2.0:assertion">http://www.okta.com/exk1da4osrIL3Y7ip357</saml2:Issuer><ds:Signature xmlns:ds="http://www.w3.org/2000/09/xmldsig#"><ds:SignedInfo><ds:CanonicalizationMethod Algorithm="http://www.w3.org/2001/10/xml-exc-c14n#"/><ds:SignatureMethod Algorithm="http://www.w3.org/2001/04/xmldsig-more#rsa-sha256"/><ds:Reference URI="#id544612569720442296425226"><ds:Transforms><ds:Transform Algorithm="http://www.w3.org/2000/09/xmldsig#enveloped-signature"/><ds:Transform Algorithm="http://www.w3.org/2001/10/xml-exc-c14n#"><ec:InclusiveNamespaces PrefixList="xs" xmlns:ec="http://www.w3.org/2001/10/xml-exc-c14n#"/></ds:Transform></ds:Transforms><ds:DigestMethod Algorithm="http://www.w3.org/2001/04/xmlenc#sha256"/><ds:DigestValue>P/e+Gdz179UAcrrPZW2R9hzxMlSAGwbZ+Ogksp7Rzlg=</ds:DigestValue></ds:Reference></ds:SignedInfo><ds:SignatureValue>H1eepG122h3jzIqorofI6sr636xVFFtqsN0Vj5eb9YoFN3KMDH1AqzvGbzA+XEoT/1vle/D2n1A0qMv6UrnMy0EgrZlA+Mx3MgcQDhFoIqI7lV48I2aJ+G1+FvTrzt1hhfn6SBTorhc3M2+ST9z68V8mLsNXr82GveL/Ej5J4rxbQQ0Jxaic3luAkV0EhROqiSDwC7e/45II34e3sdtQ9bbnf3feDbovklb7Daa/NIqWpWX+0Y9qhHo1zx05oPiGZFtveJHiUbFXPpjR0r1juuG3HTGkORhRHCMYnpz73NsmuBTkAgYE+G0vUr0k5Sk28efS15ZZuAyiN+XCjl6SzQ==</ds:SignatureValue><ds:KeyInfo><ds:X509Data><ds:X509Certificate>MIIDojCCAoqgAwIBAgIGAW0svbVqMA0GCSqGSIb3DQEBCwUAMIGRMQswCQYDVQQGEwJVUzETMBEG
+<?xml version="1.0" encoding="UTF-8"?><saml2p:Response Destination="http://zulip.testserver/complete/saml/" ID="id544612569720442296425226" InResponseTo="ONELOGIN_a5fde8b09598814d7af2537f865d31c2f7aea831" IssueInstant="2019-09-25T01:02:02.120Z" Version="2.0" xmlns:saml2p="urn:oasis:names:tc:SAML:2.0:protocol" xmlns:xs="http://www.w3.org/2001/XMLSchema"><saml2:Issuer Format="urn:oasis:names:tc:SAML:2.0:nameid-format:entity" xmlns:saml2="urn:oasis:names:tc:SAML:2.0:assertion">https://idp.testshib.org/idp/shibboleth</saml2:Issuer><ds:Signature xmlns:ds="http://www.w3.org/2000/09/xmldsig#"><ds:SignedInfo><ds:CanonicalizationMethod Algorithm="http://www.w3.org/2001/10/xml-exc-c14n#"/><ds:SignatureMethod Algorithm="http://www.w3.org/2001/04/xmldsig-more#rsa-sha256"/><ds:Reference URI="#id544612569720442296425226"><ds:Transforms><ds:Transform Algorithm="http://www.w3.org/2000/09/xmldsig#enveloped-signature"/><ds:Transform Algorithm="http://www.w3.org/2001/10/xml-exc-c14n#"><ec:InclusiveNamespaces PrefixList="xs" xmlns:ec="http://www.w3.org/2001/10/xml-exc-c14n#"/></ds:Transform></ds:Transforms><ds:DigestMethod Algorithm="http://www.w3.org/2001/04/xmlenc#sha256"/><ds:DigestValue>P/e+Gdz179UAcrrPZW2R9hzxMlSAGwbZ+Ogksp7Rzlg=</ds:DigestValue></ds:Reference></ds:SignedInfo><ds:SignatureValue>H1eepG122h3jzIqorofI6sr636xVFFtqsN0Vj5eb9YoFN3KMDH1AqzvGbzA+XEoT/1vle/D2n1A0qMv6UrnMy0EgrZlA+Mx3MgcQDhFoIqI7lV48I2aJ+G1+FvTrzt1hhfn6SBTorhc3M2+ST9z68V8mLsNXr82GveL/Ej5J4rxbQQ0Jxaic3luAkV0EhROqiSDwC7e/45II34e3sdtQ9bbnf3feDbovklb7Daa/NIqWpWX+0Y9qhHo1zx05oPiGZFtveJHiUbFXPpjR0r1juuG3HTGkORhRHCMYnpz73NsmuBTkAgYE+G0vUr0k5Sk28efS15ZZuAyiN+XCjl6SzQ==</ds:SignatureValue><ds:KeyInfo><ds:X509Data><ds:X509Certificate>MIIDojCCAoqgAwIBAgIGAW0svbVqMA0GCSqGSIb3DQEBCwUAMIGRMQswCQYDVQQGEwJVUzETMBEG
 A1UECAwKQ2FsaWZvcm5pYTEWMBQGA1UEBwwNU2FuIEZyYW5jaXNjbzENMAsGA1UECgwET2t0YTEU
 MBIGA1UECwwLU1NPUHJvdmlkZXIxEjAQBgNVBAMMCXp1bGlwY2hhdDEcMBoGCSqGSIb3DQEJARYN
 aW5mb0Bva3RhLmNvbTAeFw0xOTA5MTMyMjI3MTNaFw0yOTA5MTMyMjI4MTNaMIGRMQswCQYDVQQG

zerver/tests/test_auth_backends.py

@@ -1505,7 +1505,7 @@ class SAMLAuthBackendTest(SocialAuthBase):
                     mock.patch('zproject.backends.logging.info') as m:
                 # This mock causes AuthFailed to be raised.
                 saml_response = self.generate_saml_response(self.email, self.name)
-                relay_state = SAMLAuthBackend.put_data_in_redis({"idp": "test_idp", "subdomain": "zulip"})
+                relay_state = SAMLAuthBackend.put_data_in_redis({"subdomain": "zulip"})
                 post_params = {"SAMLResponse": saml_response, "RelayState": relay_state}
                 result = self.client_post('/complete/saml/',  post_params)
                 self.assertEqual(result.status_code, 302)
@@ -1518,7 +1518,7 @@ class SAMLAuthBackendTest(SocialAuthBase):
                             side_effect=AuthStateForbidden('State forbidden')), \
                     mock.patch('zproject.backends.logging.warning') as m:
                 saml_response = self.generate_saml_response(self.email, self.name)
-                relay_state = SAMLAuthBackend.put_data_in_redis({"idp": "test_idp", "subdomain": "zulip"})
+                relay_state = SAMLAuthBackend.put_data_in_redis({"subdomain": "zulip"})
                 post_params = {"SAMLResponse": saml_response, "RelayState": relay_state}
                 result = self.client_post('/complete/saml/',  post_params)
                 self.assertEqual(result.status_code, 302)
@@ -1538,40 +1538,40 @@ class SAMLAuthBackendTest(SocialAuthBase):
         # Check that POSTing the RelayState, but with missing SAMLResponse,
         # doesn't cause errors either:
         with mock.patch('zproject.backends.logging.info') as m:
-            relay_state = SAMLAuthBackend.put_data_in_redis({"idp": "test_idp", "subdomain": "zulip"})
+            relay_state = SAMLAuthBackend.put_data_in_redis({"subdomain": "zulip"})
             post_params = {"RelayState": relay_state}
             result = self.client_post('/complete/saml/',  post_params)
             self.assertEqual(result.status_code, 302)
             self.assertIn('login', result.url)
-            m.assert_called_once()
+            m.assert_called_once_with('/complete/saml/: No SAMLResponse in request.')
 
         # Now test bad SAMLResponses.
         with mock.patch('zproject.backends.logging.info') as m:
-            relay_state = SAMLAuthBackend.put_data_in_redis({"idp": "test_idp", "subdomain": "zulip"})
+            relay_state = SAMLAuthBackend.put_data_in_redis({"subdomain": "zulip"})
             post_params = {"RelayState": relay_state, 'SAMLResponse': ''}
             result = self.client_post('/complete/saml/',  post_params)
             self.assertEqual(result.status_code, 302)
             self.assertIn('login', result.url)
-            m.assert_called_once()
+            m.assert_called()
 
         with mock.patch('zproject.backends.logging.info') as m:
-            relay_state = SAMLAuthBackend.put_data_in_redis({"idp": "test_idp", "subdomain": "zulip"})
+            relay_state = SAMLAuthBackend.put_data_in_redis({"subdomain": "zulip"})
             post_params = {"RelayState": relay_state, 'SAMLResponse': 'b'}
             result = self.client_post('/complete/saml/',  post_params)
             self.assertEqual(result.status_code, 302)
             self.assertIn('login', result.url)
-            m.assert_called_once()
+            m.assert_called()
 
         with mock.patch('zproject.backends.logging.info') as m:
-            relay_state = SAMLAuthBackend.put_data_in_redis({"idp": "test_idp", "subdomain": "zulip"})
+            relay_state = SAMLAuthBackend.put_data_in_redis({"subdomain": "zulip"})
             post_params = {"RelayState": relay_state, 'SAMLResponse': 'dGVzdA=='}  # base64 encoded 'test'
             result = self.client_post('/complete/saml/',  post_params)
             self.assertEqual(result.status_code, 302)
             self.assertIn('login', result.url)
-            m.assert_called_once()
+            m.assert_called()
 
         with mock.patch('zproject.backends.logging.info') as m:
-            relay_state = SAMLAuthBackend.put_data_in_redis({"idp": "test_idp", "subdomain": "zulip"})
+            relay_state = SAMLAuthBackend.put_data_in_redis({"subdomain": "zulip"})
             relay_state = relay_state[:-1]  # Break the token by removing the last character
             post_params = {"RelayState": relay_state}
             result = self.client_post('/complete/saml/',  post_params)
@@ -1579,16 +1579,54 @@ class SAMLAuthBackendTest(SocialAuthBase):
             self.assertIn('login', result.url)
             m.assert_called_with("SAML authentication failed: bad RelayState token.")
 
-    def test_social_auth_complete_no_idp_and_subdomain(self) -> None:
+    def test_social_auth_complete_no_subdomain(self) -> None:
         with mock.patch('zproject.backends.logging.info') as m:
             relay_state = SAMLAuthBackend.put_data_in_redis({})
-            post_params = {"RelayState": relay_state}
+            post_params = {"RelayState": relay_state,
+                           'SAMLResponse': self.generate_saml_response(email=self.example_email("hamlet"),
+                                                                       name="King Hamlet")}
             result = self.client_post('/complete/saml/',  post_params)
             self.assertEqual(result.status_code, 302)
             self.assertEqual('/login/', result.url)
-            self.assertIn("Missing idp or subdomain value in relayed_params",
+            self.assertIn("/complete/saml/: Missing subdomain value in relayed_params.",
                           m.call_args.args[0])
 
+    def test_social_auth_complete_wrong_issuing_idp(self) -> None:
+        relay_state = SAMLAuthBackend.put_data_in_redis({"subdomain": "zulip"})
+        saml_response = self.generate_saml_response(email=self.example_email("hamlet"),
+                                                    name="King Hamlet")
+
+        # We change the entity_id of the configured test IdP, which means it won't match
+        # the Entity ID in the SAMLResponse generated above.
+        idps_dict = copy.deepcopy(settings.SOCIAL_AUTH_SAML_ENABLED_IDPS)
+        idps_dict['test_idp']['entity_id'] = 'https://different.idp.example.com/'
+        with self.settings(SOCIAL_AUTH_SAML_ENABLED_IDPS=idps_dict):
+            with mock.patch('zproject.backends.logging.info') as m:
+                post_params = {"RelayState": relay_state,
+                               "SAMLResponse": saml_response}
+                result = self.client_post('/complete/saml/',  post_params)
+                self.assertEqual(result.status_code, 302)
+                self.assertEqual('/login/', result.url)
+                self.assertIn("/complete/saml/: No valid IdP as issuer of the SAMLResponse.",
+                              m.call_args.args[0])
+
+    def test_social_auth_complete_valid_get_idp_bad_samlresponse(self) -> None:
+        """
+        This tests for a hypothetical scenario where our basic parsing of the SAMLResponse
+        successfully returns the issuing IdP, but it fails further down the line, during proper
+        validation in the underlying libraries.
+        """
+
+        with mock.patch('zproject.backends.logging.info') as m, \
+                mock.patch.object(SAMLAuthBackend, 'get_issuing_idp', return_value='test_idp'):
+            relay_state = SAMLAuthBackend.put_data_in_redis({"subdomain": "zulip"})
+            post_params = {"RelayState": relay_state, 'SAMLResponse': 'dGVzdA=='}
+            result = self.client_post('/complete/saml/',  post_params)
+            self.assertEqual(result.status_code, 302)
+            self.assertIn('login', result.url)
+
+            m.assert_called_once()
+
     def test_social_auth_saml_bad_idp_param_on_login_page(self) -> None:
         with mock.patch('zproject.backends.logging.info') as m:
             result = self.client_get('/login/saml/')

zproject/backends.py

@@ -39,6 +39,8 @@ from django.utils.translation import ugettext as _
 from lxml.etree import XMLSyntaxError
 from requests import HTTPError
 from onelogin.saml2.errors import OneLogin_Saml2_Error
+from onelogin.saml2.response import OneLogin_Saml2_Response
+
 from social_core.backends.github import GithubOAuth2, GithubOrganizationOAuth2, \
     GithubTeamOAuth2
 from social_core.backends.azuread import AzureADOAuth2
@@ -1484,6 +1486,7 @@ class SAMLAuthBackend(SocialAuthMixin, SAMLAuth):
     standard_relay_params = ["subdomain", "multiuse_object_key", "mobile_flow_otp", "desktop_flow_otp",
                              "next", "is_signup"]
     REDIS_EXPIRATION_SECONDS = 60 * 15
+    SAMLRESPONSE_PARSING_EXCEPTIONS = (OneLogin_Saml2_Error, binascii.Error, XMLSyntaxError)
     name = "saml"
     # Organization which go through the trouble of setting up SAML are most likely
     # to have it as their main authentication method, so it seems appropriate to have
@@ -1537,7 +1540,7 @@ class SAMLAuthBackend(SocialAuthMixin, SAMLAuth):
         # RelayState, which is used as a key into our redis data store
         # for fetching the actual parameters after the IdP has
         # returned a successful authentication.
-        params_to_relay = ["idp"] + self.standard_relay_params
+        params_to_relay = self.standard_relay_params
         request_data = self.strategy.request_data().dict()
         data_to_relay = {
             key: request_data[key] for key in params_to_relay if key in request_data
@@ -1565,6 +1568,30 @@ class SAMLAuthBackend(SocialAuthMixin, SAMLAuth):
 
         return data
 
+    @classmethod
+    def get_issuing_idp(cls, SAMLResponse: str) -> Optional[str]:
+        """
+        Given a SAMLResponse, returns which of the configured IdPs is declared as the issuer.
+        This value MUST NOT be trusted as the true issuer!
+        The signatures are not validated, so it can be tampered with by the user.
+        That's not a problem for this function,
+        and true validation happens later in the underlying libraries, but it's important
+        to note this detail. The purpose of this function is merely as a helper to figure out which
+        of the configured IdPs' information to use for parsing and validating the response.
+        """
+        try:
+            resp = OneLogin_Saml2_Response(settings={}, response=SAMLResponse)
+            issuers = resp.get_issuers()
+        except cls.SAMLRESPONSE_PARSING_EXCEPTIONS as e:
+            logging.info("Error while parsing SAMLResponse: %s: %s", e.__class__.__name__, e)
+            return None
+
+        for idp_name, idp_config in settings.SOCIAL_AUTH_SAML_ENABLED_IDPS.items():
+            if idp_config['entity_id'] in issuers:
+                return idp_name
+
+        return None
+
     def auth_complete(self, *args: Any, **kwargs: Any) -> Optional[HttpResponse]:
         """
         Additional ugly wrapping on top of auth_complete in SocialAuthMixin.
@@ -1589,11 +1616,19 @@ class SAMLAuthBackend(SocialAuthMixin, SAMLAuth):
         if relayed_params is None:
             return None
 
-        idp_name = relayed_params.get("idp")
+        SAMLResponse = self.strategy.request_data().get('SAMLResponse')
+        if SAMLResponse is None:
+            logging.info("/complete/saml/: No SAMLResponse in request.")
+            return None
+
+        idp_name = self.get_issuing_idp(SAMLResponse)
+        if idp_name is None:
+            logging.info("/complete/saml/: No valid IdP as issuer of the SAMLResponse.")
+            return None
+
         subdomain = relayed_params.get("subdomain")
-        if idp_name is None or subdomain is None:
+        if subdomain is None:
-            error_msg = "Missing idp or subdomain value in relayed_params in SAML auth_complete: %s"
+            logging.info("/complete/saml/: Missing subdomain value in relayed_params.")
-            logging.info(error_msg, dict(subdomain=subdomain, idp=idp_name))
             return None
 
         idp_valid = self.validate_idp_for_subdomain(idp_name, subdomain)
@@ -1617,7 +1652,7 @@ class SAMLAuthBackend(SocialAuthMixin, SAMLAuth):
 
             # Call the auth_complete method of SocialAuthMixIn
             result = super().auth_complete(*args, **kwargs)
-        except (OneLogin_Saml2_Error, binascii.Error, XMLSyntaxError) as e:
+        except self.SAMLRESPONSE_PARSING_EXCEPTIONS as e:
             # These can be raised if SAMLResponse is missing or badly formatted.
             logging.info("/complete/saml/: %s: %s", e.__class__.__name__, e)
             # Fall through to returning None.