From 2c9207bdd0954ed74871d3100448abd69860a80b Mon Sep 17 00:00:00 2001 From: Alexandre Fayolle Date: Tue, 9 Jan 2024 12:27:34 +0100 Subject: [PATCH] [IMP] auth_saml: download the provider metadata On Office365, what you get when configuring an application for SAML authentication is the URL of the federation metadata document. This URL is stable, but the content of the document is not. I suspect some of the encryption keys can be updated / renewed over time. The result is that the configured provider in Odoo suddenly stops working, because the messages sent by the Office365 provider can no longer be validated by Odoo (because the federation document is out of date). Downloading the new version and updating the auth.saml.provider record fixes the issue. This PR adds a new field to store the URL of the metadata document. When this field is set on a provider, you get a button next to it in the form view to download the document from the URL. The button will not update the document if it has not changed. Additionally, when a SignatureError happens, we check if downloading the document again fixes the issue. --- auth_saml/README.rst | 4 + auth_saml/models/auth_saml_provider.py | 81 ++++++++++++-- auth_saml/readme/CONFIGURE.rst | 4 + auth_saml/tests/data/cert_idp_expired.pem | 24 ++++ auth_saml/tests/data/key_idp_expired.pem | 28 +++++ auth_saml/tests/fake_idp.py | 15 ++- auth_saml/tests/test_pysaml.py | 130 +++++++++++++++++++++- auth_saml/views/auth_saml.xml | 15 +++ 8 files changed, 287 insertions(+), 14 deletions(-) create mode 100644 auth_saml/tests/data/cert_idp_expired.pem create mode 100644 auth_saml/tests/data/key_idp_expired.pem diff --git a/auth_saml/README.rst b/auth_saml/README.rst index ac6877f56e..024fe35a38 100644 --- a/auth_saml/README.rst +++ b/auth_saml/README.rst @@ -81,6 +81,10 @@ by using the query parameter ``disable_autoredirect``, as in ``https://example.com/web/login?disable_autoredirect=`` The login is also displayed if there is an error with SAML login, in order to display any error message. +If you are using Office365 as identity provider, set up the federation metadata document +rather than the document itself. This will allow the module to refresh the document when +needed. + Usage ===== diff --git a/auth_saml/models/auth_saml_provider.py b/auth_saml/models/auth_saml_provider.py index 78e6c96f33..6850f6a934 100644 --- a/auth_saml/models/auth_saml_provider.py +++ b/auth_saml/models/auth_saml_provider.py @@ -9,13 +9,17 @@ import tempfile import urllib.parse +import requests + # dependency name is pysaml2 # pylint: disable=W7936 import saml2 import saml2.xmldsig as ds from saml2.client import Saml2Client from saml2.config import Config as Saml2Config +from saml2.sigver import SignatureError from odoo import api, fields, models +from odoo.exceptions import UserError _logger = logging.getLogger(__name__) @@ -42,6 +46,14 @@ class AuthSamlProvider(models.Model): ), required=True, ) + idp_metadata_url = fields.Char( + string="Identity Provider Metadata URL", + help="Some SAML providers, notably Office365 can have a metadata " + "document which changes over time, and they provide a URL to the " + "document instead. When this field is set, the metadata can be " + "fetched from the provided URL.", + ) + sp_baseurl = fields.Text( string="Override Base URL", help="""Base URL sent to Odoo with this, rather than automatically @@ -232,10 +244,19 @@ def _get_config_for_provider(self, base_url: str = None) -> Saml2Config: "cert_file": self._get_cert_key_path("sp_pem_public"), "key_file": self._get_cert_key_path("sp_pem_private"), } - sp_config = Saml2Config() - sp_config.load(settings) - sp_config.allow_unknown_attributes = True - return sp_config + try: + sp_config = Saml2Config() + sp_config.load(settings) + sp_config.allow_unknown_attributes = True + return sp_config + except saml2.SAMLError: + if self.env.context.get("saml2_retry_after_refresh_metadata", False): + raise + # Retry after refresh metadata + self.action_refresh_metadata_from_url() + return self.with_context( + saml2_retry_after_refresh_metatata=1 + )._get_config_for_provider(base_url) def _get_client_for_provider(self, base_url: str = None) -> Saml2Client: sp_config = self._get_config_for_provider(base_url) @@ -280,13 +301,26 @@ def _get_auth_request(self, extra_state=None, url_root=None): def _validate_auth_response(self, token: str, base_url: str = None): """return the validation data corresponding to the access token""" self.ensure_one() - - client = self._get_client_for_provider(base_url) - response = client.parse_authn_request_response( - token, - saml2.entity.BINDING_HTTP_POST, - self._get_outstanding_requests_dict(), - ) + try: + client = self._get_client_for_provider(base_url) + response = client.parse_authn_request_response( + token, + saml2.entity.BINDING_HTTP_POST, + self._get_outstanding_requests_dict(), + ) + except SignatureError: + # we have a metadata url: try to refresh the metadata document + if self.idp_metadata_url: + self.action_refresh_metadata_from_url() + # retry: if it fails again, we let the exception flow + client = self._get_client_for_provider(base_url) + response = client.parse_authn_request_response( + token, + saml2.entity.BINDING_HTTP_POST, + self._get_outstanding_requests_dict(), + ) + else: + raise matching_value = None if self.matching_attribute == "subject.nameId": @@ -370,3 +404,28 @@ def _hook_validate_auth_response(self, response, matching_value): vals[attribute.field_name] = attribute_value return {"mapped_attrs": vals} + + def action_refresh_metadata_from_url(self): + providers = self.search( + [("idp_metadata_url", "ilike", "http%"), ("id", "in", self.ids)] + ) + if not providers: + return False + # lock the records we might update, so that multiple simultaneous login + # attempts will not cause concurrent updates + self.env.cr.execute( + "SELECT id FROM auth_saml_provider WHERE id in %s FOR UPDATE", + (tuple(providers.ids),), + ) + updated = False + for provider in providers: + document = requests.get(provider.idp_metadata_url, timeout=5) + if document.status_code != 200: + raise UserError( + f"Unable to download the metadata for {provider.name}: {document.reason}" + ) + if document.text != provider.idp_metadata: + provider.idp_metadata = document.text + _logger.info("Updated provider metadata for %s", provider.name) + updated = True + return updated diff --git a/auth_saml/readme/CONFIGURE.rst b/auth_saml/readme/CONFIGURE.rst index 270886b630..b08476110c 100644 --- a/auth_saml/readme/CONFIGURE.rst +++ b/auth_saml/readme/CONFIGURE.rst @@ -15,3 +15,7 @@ with the highest priority. It is still possible to access the login without redi by using the query parameter ``disable_autoredirect``, as in ``https://example.com/web/login?disable_autoredirect=`` The login is also displayed if there is an error with SAML login, in order to display any error message. + +If you are using Office365 as identity provider, set up the federation metadata document +rather than the document itself. This will allow the module to refresh the document when +needed. diff --git a/auth_saml/tests/data/cert_idp_expired.pem b/auth_saml/tests/data/cert_idp_expired.pem new file mode 100644 index 0000000000..d2c320a4b9 --- /dev/null +++ b/auth_saml/tests/data/cert_idp_expired.pem @@ -0,0 +1,24 @@ +-----BEGIN CERTIFICATE----- +MIID7TCCAtWgAwIBAgIUDBX/LJ1BPZOhb2vrDnwIasyEi+AwDQYJKoZIhvcNAQEL +BQAwgYUxCzAJBgNVBAYTAkFVMRMwEQYDVQQIDApTb21lLVN0YXRlMQ4wDAYDVQQH +DAVQYXJpczEMMAoGA1UECgwDT0NBMQwwCgYDVQQLDANPQ0ExFDASBgNVBAMMC2V4 +YW1wbGUuY29tMR8wHQYJKoZIhvcNAQkBFhB0ZXN0QGV4YW1wbGUuY29tMB4XDTIz +MDEwMTExMDAyN1oXDTIzMDEzMTExMDAyN1owgYUxCzAJBgNVBAYTAkFVMRMwEQYD +VQQIDApTb21lLVN0YXRlMQ4wDAYDVQQHDAVQYXJpczEMMAoGA1UECgwDT0NBMQww +CgYDVQQLDANPQ0ExFDASBgNVBAMMC2V4YW1wbGUuY29tMR8wHQYJKoZIhvcNAQkB +FhB0ZXN0QGV4YW1wbGUuY29tMIIBIjANBgkqhkiG9w0BAQEFAAOCAQ8AMIIBCgKC +AQEAvgeLRr1Q9aS/t8ZuC7/pZRHTB6sqamVwXyR7zh0v51yH7xBy9xs4zJWKneRn +OJw46IogYhY+dyNWElbY+Ckcc6z1eJONiHNtOKAy07VtfhisGviRv1kLE56SHGgW +fIXrOuFqj6F1yTfKyLtq2RZBzmbMTNG7z89rO2hqdTWqhyof9OGWtecrM7Ei9PnL +tqULhQyh6n47KnIXfBMLIeQG7d/WyGU5CnO7yhHkS/51E9gI6g5G0VoueBVFErCl +rjo0clMJrFVpanOG2USGgLfPkomSIv9ZL4SreFN27sbhTbkVWxbk7AOCFCQcaBIv +RThpRrA9YRv2dB/X4yIi7UrrPwIDAQABo1MwUTAdBgNVHQ4EFgQU4WFoM/SL6qvT +jV4YUwH3rggBqyIwHwYDVR0jBBgwFoAU4WFoM/SL6qvTjV4YUwH3rggBqyIwDwYD +VR0TAQH/BAUwAwEB/zANBgkqhkiG9w0BAQsFAAOCAQEAYG+CTENdcmiKmyYg/n6H +59RRgaLHSszjsKYdM5Uy/PmwfvPTRuxP38UhF0gWJTsLxWRjI0ejzdy5vzvxpoYl +3pEyYxPzl+jfGA6AUSOfzTT/ksi7SZWak3cqJDFCdnfaCSjYyi+nKz5y6Bm1/fMy +/3zJX92ELIWc8cTUItUIWjlITmxgLhIGr1wYaCinxkaEopGqkX85RYFaWKyYa5ok +8MnoYbPrh/i9EekHUMBMPKWN3tWMMEROTtX9hmxSSTtgdQCahBaOCCU+m8PSNKEc +UA8nSStaolv8t6aOyEb/Kzs7WSbd7v1ovZsy2FYmIRn0eHz8fpMAw2qk7mol6iao +GQ== +-----END CERTIFICATE----- diff --git a/auth_saml/tests/data/key_idp_expired.pem b/auth_saml/tests/data/key_idp_expired.pem new file mode 100644 index 0000000000..496f309a12 --- /dev/null +++ b/auth_saml/tests/data/key_idp_expired.pem @@ -0,0 +1,28 @@ +-----BEGIN PRIVATE KEY----- +MIIEvgIBADANBgkqhkiG9w0BAQEFAASCBKgwggSkAgEAAoIBAQC+B4tGvVD1pL+3 +xm4Lv+llEdMHqypqZXBfJHvOHS/nXIfvEHL3GzjMlYqd5Gc4nDjoiiBiFj53I1YS +Vtj4KRxzrPV4k42Ic204oDLTtW1+GKwa+JG/WQsTnpIcaBZ8hes64WqPoXXJN8rI +u2rZFkHOZsxM0bvPz2s7aGp1NaqHKh/04Za15yszsSL0+cu2pQuFDKHqfjsqchd8 +Ewsh5Abt39bIZTkKc7vKEeRL/nUT2AjqDkbRWi54FUUSsKWuOjRyUwmsVWlqc4bZ +RIaAt8+SiZIi/1kvhKt4U3buxuFNuRVbFuTsA4IUJBxoEi9FOGlGsD1hG/Z0H9fj +IiLtSus/AgMBAAECggEBAKuXUFJeHL7TNzMRAMmnT28uOypPiwtr8Z5X6Vtiy6DU +0wIyDj3H3PAPkI2mcvaRSmngYAFyKJGX3N7OgTkElmZ1pWptgn3WDKf3MC4vQ2F7 +kd0A20q3cuMSaskvzC5BFvmiFoD/wMYjlP7RDVhdWqqv9IbhVAAAQcnxLUANZ6CH +/xrieGuYavs62pSu5fnke7zRozdD1Mb7/oolAnycaLuoi1eZBh8wW8EJyFSxcZ5A +pYF5kNqbwAdOZ22Tygxwu7lnh8PUOKxf9pTmO6uUYAJcn/Z3ZHtnBYsjU/LkfNPV +hYLu1bKftm6UEZYwCXE3/ygop1q648NvCvtJB+Gbj9ECgYEA8nB+hS+7MLgi/dv8 +FCMJ9HBN76/nlwjOCTZIyIhCs5Jc6zJQGiDNLUFM/1mpBKUWWAss3g0dmJq32ish +apsCUxabzWuKi44fDMEterJrGDWquyJK+jNPqfqOORLdMf0edNfZbjUxev7D52Ak +4Ej3Ggy/fENd8QWLK6PZHV5X1MUCgYEAyKiWlawh7l8eBrba8UFQ4n1HiK/2uEud +yQOLceSRmW/xC6ZCiR0ILinrtZWRxqQg+ZSS24hjnHhcdnRw8TRXx22TkTwGfAXW +wKesPrtGJrn0ADuZwPkGewyeHPsisXNSiuGLPcLiOCoNNYgbIWJ2RknM1Xw+2p8C +qYU8Si6l6DMCgYEA20v4ld7sExCszjZ72XcsXQhs5v+Vm9/iByEsSwA+XZJqLHFx +VYEQNvxXeq8OnN37zR4msqDogY6J+XWEH5shSiksO28ofj3LRk1DJzZWeyqoSeem +LJXXXKkAlw3COaJ9NzG8Qt0o6dmjORqVoK8/nTekyfFh+0+JaKsoDFG3XwUCgYBN +tq2Ljj0d+wzAAPXO1kMjVO3tjGj7e53CinLpS2LwkCBFKMFAJVRTvLyjeSgaTNrQ +jrBKAgrCQQNehT5wzJrqjA/JAfxo8EH6H3ZgXVuQCBjuNicYS9ossfhStRj8rPNd +AnlRFDdVFUREZVBMn7u7AT4puJMHTOpVCVsOR/7NbQKBgApyR1WfsxZYi8vzVosQ +jnMIW18lnZN3s6auyEvmpVowx0U0yd9QU3HHX1j8Kfq7D9uERCwBtIfn9fzZrZnu +Xgbi9LMUT1z1jFXxJNgzaNmm0JHW9cD24BWNeQ60uxaRiGGmCyfmgqrGOXSn2R8w +KoWEnnunZ9nehcD9dkWcH5zG +-----END PRIVATE KEY----- diff --git a/auth_saml/tests/fake_idp.py b/auth_saml/tests/fake_idp.py index 1d958233bd..e2043f468e 100644 --- a/auth_saml/tests/fake_idp.py +++ b/auth_saml/tests/fake_idp.py @@ -105,8 +105,9 @@ def _unpack(self, ver="SAMLResponse"): class FakeIDP(Server): - def __init__(self, metadatas=None): - settings = CONFIG + def __init__(self, metadatas=None, settings=None): + if settings is None: + settings = CONFIG if metadatas: settings.update({"metadata": {"inline": metadatas}}) @@ -164,3 +165,13 @@ def authn_request_endpoint(self, req, binding, relay_state): ) return DummyResponse(**_dict) + + +class UnsignedFakeIDP(FakeIDP): + def create_authn_response( + self, + *args, + **kwargs, + ): + kwargs["sign_assertion"] = False + return super().create_authn_response(*args, **kwargs) diff --git a/auth_saml/tests/test_pysaml.py b/auth_saml/tests/test_pysaml.py index 1660eb44fe..5246b50a37 100644 --- a/auth_saml/tests/test_pysaml.py +++ b/auth_saml/tests/test_pysaml.py @@ -2,12 +2,18 @@ import base64 import html import os +import os.path as osp +from copy import deepcopy from unittest.mock import patch +import responses +from saml2.sigver import SignatureError + from odoo.exceptions import AccessDenied, UserError, ValidationError from odoo.tests import HttpCase, tagged +from odoo.tools import mute_logger -from .fake_idp import FakeIDP +from .fake_idp import CONFIG, FakeIDP, UnsignedFakeIDP @tagged("saml", "post_install", "-at_install") @@ -358,3 +364,125 @@ def test_disallow_user_password_when_changing_settings(self): self.authenticate( user="user@example.com", password="NesTNSte9340D720te>/-A" ) + + @responses.activate + def test_download_metadata(self): + expected_metadata = self.idp.get_metadata() + responses.add( + responses.GET, + "http://localhost:8000/metadata", + status=200, + content_type="text/xml", + body=expected_metadata, + ) + self.saml_provider.idp_metadata_url = "http://localhost:8000/metadata" + self.saml_provider.idp_metadata = "" + self.saml_provider.action_refresh_metadata_from_url() + self.assertEqual(self.saml_provider.idp_metadata, expected_metadata) + + @responses.activate + def test_download_metadata_no_provider(self): + self.saml_provider.idp_metadata_url = "http://localhost:8000/metadata" + self.saml_provider.idp_metadata = "" + self.saml_provider.active = False + self.saml_provider.action_refresh_metadata_from_url() + self.assertFalse(self.saml_provider.idp_metadata) + + @responses.activate + def test_download_metadata_error(self): + responses.add( + responses.GET, + "http://localhost:8000/metadata", + status=500, + content_type="text/xml", + ) + self.saml_provider.idp_metadata_url = "http://localhost:8000/metadata" + self.saml_provider.idp_metadata = "" + with self.assertRaises(UserError): + self.saml_provider.action_refresh_metadata_from_url() + self.assertFalse(self.saml_provider.idp_metadata) + + @responses.activate + def test_download_metadata_no_update(self): + expected_metadata = self.idp.get_metadata() + responses.add( + responses.GET, + "http://localhost:8000/metadata", + status=200, + content_type="text/xml", + body=expected_metadata, + ) + self.saml_provider.idp_metadata_url = "http://localhost:8000/metadata" + self.saml_provider.idp_metadata = expected_metadata + self.saml_provider.action_refresh_metadata_from_url() + self.assertEqual(self.saml_provider.idp_metadata, expected_metadata) + + @responses.activate + def test_login_with_saml_metadata_empty(self): + self.saml_provider.idp_metadata_url = "http://localhost:8000/metadata" + self.saml_provider.idp_metadata = "" + expected_metadata = self.idp.get_metadata() + responses.add( + responses.GET, + "http://localhost:8000/metadata", + status=200, + content_type="text/xml", + body=expected_metadata, + ) + self.test_login_with_saml() + self.assertEqual(self.saml_provider.idp_metadata, expected_metadata) + + @responses.activate + def test_login_with_saml_metadata_key_changed(self): + settings = deepcopy(CONFIG) + settings["key_file"] = osp.join( + osp.dirname(__file__), "data", "key_idp_expired.pem" + ) + settings["cert"] = osp.join( + osp.dirname(__file__), "data", "key_idp_expired.pem" + ) + expired_idp = FakeIDP(settings=settings) + self.saml_provider.idp_metadata = expired_idp.get_metadata() + self.saml_provider.idp_metadata_url = "http://localhost:8000/metadata" + up_to_date_metadata = self.idp.get_metadata() + self.assertNotEqual(self.saml_provider.idp_metadata, up_to_date_metadata) + responses.add( + responses.GET, + "http://localhost:8000/metadata", + status=200, + content_type="text/xml", + body=up_to_date_metadata, + ) + self.test_login_with_saml() + + @responses.activate + def test_login_with_saml_unsigned_response(self): + self.add_provider_to_user() + self.saml_provider.idp_metadata_url = "http://localhost:8000/metadata" + unsigned_idp = UnsignedFakeIDP([self.saml_provider._metadata_string()]) + redirect_url = self.saml_provider._get_auth_request() + self.assertIn("http://localhost:8000/sso/redirect?SAMLRequest=", redirect_url) + + response = unsigned_idp.fake_login(redirect_url) + self.assertEqual(200, response.status_code) + unpacked_response = response._unpack() + + responses.add( + responses.GET, + "http://localhost:8000/metadata", + status=200, + content_type="text/xml", + body=self.saml_provider.idp_metadata, + ) + with ( + self.assertRaises(SignatureError), + mute_logger("saml2.entity"), + mute_logger("saml2.client_base"), + ): + (database, login, token) = ( + self.env["res.users"] + .sudo() + .auth_saml( + self.saml_provider.id, unpacked_response.get("SAMLResponse"), None + ) + ) diff --git a/auth_saml/views/auth_saml.xml b/auth_saml/views/auth_saml.xml index a0f4e1b855..9bda039d93 100644 --- a/auth_saml/views/auth_saml.xml +++ b/auth_saml/views/auth_saml.xml @@ -76,6 +76,21 @@ + +