From 31441ef39ae18483aa7b7f14e84761577628fb28 Mon Sep 17 00:00:00 2001 From: Alexandre Fayolle Date: Tue, 9 Jan 2024 12:27:34 +0100 Subject: [PATCH 1/2] [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 | 6 +- 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 | 5 +- auth_saml/tests/test_pysaml.py | 59 ++++++++++++++++- auth_saml/views/auth_saml.xml | 15 +++++ 8 files changed, 207 insertions(+), 15 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 1dcf66b922..fdc0367123 100644 --- a/auth_saml/README.rst +++ b/auth_saml/README.rst @@ -7,7 +7,7 @@ SAML2 Authentication !! This file is generated by oca-gen-addon-readme !! !! changes will be overwritten. !! !!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!! - !! source digest: sha256:139e220611ae66b5caca3b9586fb543ebc5de97db011e239ec48e389950beba5 + !! source digest: sha256:56a6042e204ca8c553db8eb36de4b1ad7ae8e1e9d5abe598a8398f5e17da7e7f !!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!! .. |badge1| image:: https://img.shields.io/badge/maturity-Beta-yellow.png @@ -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 ccd36b1e92..3a668b9065 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) + 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..3eb7d6c67f 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}}) diff --git a/auth_saml/tests/test_pysaml.py b/auth_saml/tests/test_pysaml.py index 7549e2546f..1de8ea275a 100644 --- a/auth_saml/tests/test_pysaml.py +++ b/auth_saml/tests/test_pysaml.py @@ -2,12 +2,16 @@ import base64 import html import os +import os.path as osp +from copy import deepcopy from unittest.mock import patch +import responses + from odoo.exceptions import AccessDenied, UserError, ValidationError from odoo.tests import HttpCase, tagged -from .fake_idp import FakeIDP +from .fake_idp import CONFIG, FakeIDP @tagged("saml", "post_install", "-at_install") @@ -359,3 +363,56 @@ 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_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() diff --git a/auth_saml/views/auth_saml.xml b/auth_saml/views/auth_saml.xml index 960905213b..1c6b75b38b 100644 --- a/auth_saml/views/auth_saml.xml +++ b/auth_saml/views/auth_saml.xml @@ -76,6 +76,21 @@ + +