From a86e3250cc1da68691ad31ba198564485f4a4ac7 Mon Sep 17 00:00:00 2001 From: Sergei Maertens Date: Thu, 19 Dec 2024 12:59:29 +0100 Subject: [PATCH] :bug: [open-formulieren/open-forms#4785] Ensure the key descriptors don't have 'use' attributes The 'use' attribute is relevant to specify if a key is used for signing authentication requests or used for encryption (when encrypted name attributes are returned this key is used to encrypt them). The DV metadata for HM specifies that either: * at least one key descriptor with use=signing and at least one with use=encryption must be present * OR one key descriptor without use attribute is present, which implies the default behaviour that it's used for both signing and encryption This parameter is controlled by python3-saml in the metadata, based on the security settings, namely: 'wantAssertionsEncrypted' (or 'wantNameIdEncrypted'). We offer a configuration option for this in the model/admin through a checkbox, but it turns out that this is never checked in real environments, which results in the 'use' parameter being included. The 'easy' way to fix the metadata is to require this checkbox to be checked, but this sucks because: * if it always has to be checked, then what's the point of having a configuration option? And how does it affect the DigiD flow? * checking it is not without risks because the option is used at runtime when the SAML response is being processed. If it is checked, but the identity provider did not encrypt any name attributes, then the python3-saml library will stop in its tracks. That's quite a big risk for existing installations that are now functional. Upon checking the SAML specification it doesn't look like there's a mechanism to 'demand' that the identity provider encrypts the attributes, and on the Eherkenning Documentation ( https://afsprakenstelsel.etoegang.nl/Startpagina/v3/interface-specifications-dv-hm\) I also can't find any clear indication that identity providers are required to encrypt attributes. It may very well depend on whether a key descriptor suitable for encryption is present in the metadata or not, and that they then decide to encrypt if those conditions are met. So, the workaround applied here only affects the metadata generation and not the runtime behaviour. It is unclear if we properly support decrypting assertions in the response, but it looks like it's built into the core of the library. --- digid_eherkenning/saml2/eherkenning.py | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/digid_eherkenning/saml2/eherkenning.py b/digid_eherkenning/saml2/eherkenning.py index ee8e30c..fcd334d 100644 --- a/digid_eherkenning/saml2/eherkenning.py +++ b/digid_eherkenning/saml2/eherkenning.py @@ -489,6 +489,20 @@ def make_attribute_consuming_services(service_provider: dict): return result + @staticmethod + def _add_x509_key_descriptors(root, cert: str, use=None): + """ + Override the usage of the 'use' attribute. + + This patch is a hack on top of the python3-saml library. We deliberately ignore + any "use" attribute in the generated metadata so that we don't affect the + runtime behaviour. + """ + fixed_use = None # ignore the use parameter entirely. + super( + CustomOneLogin_Saml2_Metadata, CustomOneLogin_Saml2_Metadata + )._add_x509_key_descriptors(root, cert=cert, use=fixed_use) + class CustomOneLogin_Saml2_Settings(OneLogin_Saml2_Settings): metadata_class = CustomOneLogin_Saml2_Metadata