From e950db4c8ecdacf4299aabee9f793f1328d3162d Mon Sep 17 00:00:00 2001 From: Sergei Maertens Date: Wed, 18 Dec 2024 15:44:49 +0100 Subject: [PATCH 1/8] :bento: Update XSD for service catalog to v1.24 This version went into effect on October 14th 2024. The previous version was 1.14. Schema is taken from https://afsprakenstelsel.etoegang.nl/Startpagina/v3/service-catalog As far as I can tell, there don't exist any XSDs for the eHerkenning/EIDAS specific requirements on top of SAML v2.0 and validating against the SAML metadata XSD will not catch the problems pointed out. --- digid_eherkenning/xsd/eherkenning-dc.xml | 48 ++++++++++++------------ 1 file changed, 24 insertions(+), 24 deletions(-) diff --git a/digid_eherkenning/xsd/eherkenning-dc.xml b/digid_eherkenning/xsd/eherkenning-dc.xml index df478bc..d45abd3 100644 --- a/digid_eherkenning/xsd/eherkenning-dc.xml +++ b/digid_eherkenning/xsd/eherkenning-dc.xml @@ -1,22 +1,16 @@ - + xmlns:md="urn:oasis:names:tc:SAML:2.0:metadata" + xmlns:ds="http://www.w3.org/2000/09/xmldsig#" + xmlns:saml2="urn:oasis:names:tc:SAML:2.0:assertion" + xmlns:esc="urn:etoegang:1.13:service-catalog" + targetNamespace="urn:etoegang:1.13:service-catalog" + elementFormDefault="qualified" + attributeFormDefault="unqualified"> - + + @@ -39,6 +33,7 @@ + @@ -52,13 +47,16 @@ + + + @@ -82,14 +80,14 @@ - - - + + + - - - - + + + + @@ -135,8 +133,9 @@ - - + + + @@ -229,5 +228,6 @@ + From f6bdf849e0a58c0b2ed552cf472dee5efbe0ae77 Mon Sep 17 00:00:00 2001 From: Sergei Maertens Date: Wed, 18 Dec 2024 16:10:32 +0100 Subject: [PATCH 2/8] :sparkles: [open-formulieren/open-forms#4785] Remove the default requested attributes ... and make the fields in the admin not-required. The requested attributes are documented (vaguely) on the service provider metadata page: https://afsprakenstelsel.etoegang.nl/Startpagina/v3/dv-metadata-for-hm and in more detail on the attribute catalogue page: https://afsprakenstelsel.etoegang.nl/Startpagina/v3/attribuutcatalogus These attributes are *additional* attributes you can request from the eHerkenning/EIDAS flow, on top of the identifier (KVK number) which you will always get and may not specify as requested attribute. See https://afsprakenstelsel.etoegang.nl/Startpagina/v3/interface-specifications-dv-hm I've opted to *keep* the defaults for EIDAS because typically you only get a PseudoID back from that service, which doesn't give us much information to work with and there are open issues/requests to use the retrieved information from EIDAS for authentication/identification already. --- ...ngconfiguration_service_description_url.py | 2 +- ...ration_eh_requested_attributes_and_more.py | 33 +++++++++++++++++++ digid_eherkenning/models/eherkenning.py | 19 +++-------- docs/metadata.rst | 28 ++++++++++++---- 4 files changed, 60 insertions(+), 22 deletions(-) create mode 100644 digid_eherkenning/migrations/0013_alter_eherkenningconfiguration_eh_requested_attributes_and_more.py diff --git a/digid_eherkenning/migrations/0007_eherkenningconfiguration_service_description_url.py b/digid_eherkenning/migrations/0007_eherkenningconfiguration_service_description_url.py index 52e31bf..7bbb063 100644 --- a/digid_eherkenning/migrations/0007_eherkenningconfiguration_service_description_url.py +++ b/digid_eherkenning/migrations/0007_eherkenningconfiguration_service_description_url.py @@ -26,7 +26,7 @@ class Migration(migrations.Migration): model_name="eherkenningconfiguration", name="eh_requested_attributes", field=models.JSONField( - default=digid_eherkenning.models.eherkenning.get_default_requested_attributes_eherkenning, + default=list, help_text="A list of additional requested attributes. A single requested attribute can be a string (the name of the attribute) or an object with keys 'name' and 'required', where 'name' is a string and 'required' a boolean'.", verbose_name="requested attributes", ), diff --git a/digid_eherkenning/migrations/0013_alter_eherkenningconfiguration_eh_requested_attributes_and_more.py b/digid_eherkenning/migrations/0013_alter_eherkenningconfiguration_eh_requested_attributes_and_more.py new file mode 100644 index 0000000..825a2b7 --- /dev/null +++ b/digid_eherkenning/migrations/0013_alter_eherkenningconfiguration_eh_requested_attributes_and_more.py @@ -0,0 +1,33 @@ +# Generated by Django 4.2.13 on 2024-12-18 14:58 + +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ("digid_eherkenning", "0012_move_config_certificate"), + ] + + operations = [ + migrations.AlterField( + model_name="eherkenningconfiguration", + name="eh_requested_attributes", + field=models.JSONField( + blank=True, + default=list, + help_text="A list of additional requested attributes. A single requested attribute can be a string (the name of the attribute) or an object with keys 'name' and 'required', where 'name' is a string and 'required' a boolean'.", + verbose_name="requested attributes", + ), + ), + migrations.AlterField( + model_name="eherkenningconfiguration", + name="eidas_requested_attributes", + field=models.JSONField( + blank=True, + default=list, + help_text="A list of additional requested attributes. A single requested attribute can be a string (the name of the attribute) or an object with keys 'name' and 'required', where 'name' is a string and 'required' a boolean'.", + verbose_name="requested attributes", + ), + ), + ] diff --git a/digid_eherkenning/models/eherkenning.py b/digid_eherkenning/models/eherkenning.py index e67cda1..78f1ab1 100644 --- a/digid_eherkenning/models/eherkenning.py +++ b/digid_eherkenning/models/eherkenning.py @@ -9,19 +9,6 @@ from .base import BaseConfiguration -def get_default_requested_attributes_eherkenning(): - return [ - { - "name": "urn:etoegang:1.11:attribute-represented:CompanyName", - "required": True, - "purpose_statements": { - "en": "For testing purposes.", - "nl": "Voor testdoeleinden.", - }, - } - ] - - def get_default_requested_attributes_eidas(): return [ { @@ -76,7 +63,8 @@ class EherkenningConfiguration(BaseConfiguration): ) eh_requested_attributes = models.JSONField( _("requested attributes"), - default=get_default_requested_attributes_eherkenning, + default=list, + blank=True, help_text=_( "A list of additional requested attributes. A single requested attribute " "can be a string (the name of the attribute) or an object with keys 'name' " @@ -115,7 +103,8 @@ class EherkenningConfiguration(BaseConfiguration): ) eidas_requested_attributes = models.JSONField( _("requested attributes"), - default=get_default_requested_attributes_eidas, + default=list, + blank=True, help_text=_( "A list of additional requested attributes. A single requested attribute " "can be a string (the name of the attribute) or an object with keys 'name' " diff --git a/docs/metadata.rst b/docs/metadata.rst index 4e4eb47..8c783dc 100644 --- a/docs/metadata.rst +++ b/docs/metadata.rst @@ -1,3 +1,5 @@ +.. _metadata: + =================== Metadata generation =================== @@ -21,17 +23,19 @@ If you wish, you can still use :ref:`management commands` to generate the m eHerkenning / eIDAS ------------------- +.. _metadata_requested_attributes: + Configuring RequestedAttribute ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ -In the field ``RequestedAttribute`` one can specify all the attributes that may be requested by the service -when a company/person logs in with eHerkenning or eIDAS. +In the field ``RequestedAttribute`` one can specify all the attributes that may be +requested by the service when a company/person logs in with eHerkenning or eIDAS. -The values specified need to come from the "`Attribuutcatalogus `_" -(there are multiple catalogues: 'generiek', 'natuurlijke personen' and 'non-natuurlijke personen'). +The values specified need to come from the Attribuutcatalogus_ (there are multiple +catalogues: 'generiek', 'natuurlijke personen' and 'non-natuurlijke personen'). -In the admin, these can be specified as a list of dictionaries. For example, for eIDAS one could use the following JSON -to request the first name of the person who logged in: +In the admin, these can be specified as a list of dictionaries. For example, for eIDAS +one could use the following JSON to request the first name of the person who logged in: .. code-block:: json @@ -45,3 +49,15 @@ to request the first name of the person who logged in: } } ] + +.. warning:: YOU MAY NOT REQUEST ATTRIBUTES LISTED IN "Identificerende kenmerken". If + you do so, the metadata will be rejected by the broker. In practice this means: + + * don't request the ``KVKNr`` attribute + * don't request the ``Pseudo`` attribute + * don't request the ``RSIN`` attribute + * don't request the ``BSN`` attribute + + These attributes are pre-configured and will be returned without asking for them. + +.. _Attribuutcatalogus: https://afsprakenstelsel.etoegang.nl/Startpagina/v3/attribuutcatalogus From c554cbd69f9b786a5d72c9bb51be27386501cabb Mon Sep 17 00:00:00 2001 From: Sergei Maertens Date: Wed, 18 Dec 2024 16:43:44 +0100 Subject: [PATCH 3/8] :white_check_mark: Run XSD validation in eherkenning metadata test --- tests/test_eherkenning_metadata.py | 13 +++++++++++++ tests/utils.py | 22 ++++++++++++++++++++++ 2 files changed, 35 insertions(+) diff --git a/tests/test_eherkenning_metadata.py b/tests/test_eherkenning_metadata.py index c2fe4d9..44742c2 100644 --- a/tests/test_eherkenning_metadata.py +++ b/tests/test_eherkenning_metadata.py @@ -1,3 +1,5 @@ +from pathlib import Path + from django.test import TestCase import pytest @@ -12,6 +14,13 @@ ) from .mixins import EherkenningMetadataMixin +from .utils import validate_against_xsd + +_repo_root = Path(__file__).parent.parent.resolve() + +SAML_METADATA_XSD = ( + _repo_root / "digid_eherkenning" / "xsd" / "saml-schema-metadata-2.0.xsd" +) NAME_SPACES = { "md": "urn:oasis:names:tc:SAML:2.0:metadata", @@ -162,6 +171,10 @@ def test_generate_metadata_all_options_specified(self): self.eherkenning_config.save() eherkenning_metadata = generate_eherkenning_metadata() + + with self.subTest("passes XSD validation"): + validate_against_xsd(eherkenning_metadata, SAML_METADATA_XSD) + self.assertEqual(eherkenning_metadata[:5], b" None: + """ + Validate the XML against a schema. + + See https://lxml.de/validation.html + """ + xmlschema = etree.XMLSchema(_load_schema(xsd_schema)) + doc = etree.parse(BytesIO(xml)) + xmlschema.assertValid(doc) From edeedfdd7ec64a6ff520450f55479b2356c989c9 Mon Sep 17 00:00:00 2001 From: Sergei Maertens Date: Wed, 18 Dec 2024 16:52:43 +0100 Subject: [PATCH 4/8] :bug: [open-formulieren/open-forms#4785] Remove the NameIDFormat element While having this element present passes XSD validation against the SAML 2.0 metadata schema, this is not accepted by brokers anymore because of the line in the AS1.24a specification saying that unlisted elements must not be included in the metadata. I've opted to drop this key/element in the eHerkenning SAML client implementation rather than the base class because I don't know if removing it entirely will cause the DigiD metadata to break. It would probably be wise to *not* share a common base class anymore for DigiD and eHerkenning as it proves to be quite a maintenance nightmare. Documentation at the time of writing: https://afsprakenstelsel.etoegang.nl/Startpagina/v3/dv-metadata-for-hm --- digid_eherkenning/saml2/eherkenning.py | 5 +++++ digid_eherkenning/types.py | 2 +- 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/digid_eherkenning/saml2/eherkenning.py b/digid_eherkenning/saml2/eherkenning.py index 5f5f96a..a64d0b5 100644 --- a/digid_eherkenning/saml2/eherkenning.py +++ b/digid_eherkenning/saml2/eherkenning.py @@ -469,6 +469,11 @@ def create_config_dict(self, conf: EHerkenningConfig) -> EHerkenningSAMLConfig: config_dict: EHerkenningSAMLConfig = super().create_config_dict(conf) sp_config = config_dict["sp"] + # may not be included for eHerkenning/EIDAS since AS1.24a, see: + # https://afsprakenstelsel.etoegang.nl/Startpagina/v3/dv-metadata-for-hm + # + # ... Elements not listed in this table MUST NOT be included in the metadata. + del sp_config["NameIDFormat"] # we have multiple services, so delete the config for the "single service" variant attribute_consuming_services = create_attribute_consuming_services(conf) diff --git a/digid_eherkenning/types.py b/digid_eherkenning/types.py index 36ef8c8..f55e1f4 100644 --- a/digid_eherkenning/types.py +++ b/digid_eherkenning/types.py @@ -48,7 +48,7 @@ class ServiceProviderSAMLConfig(TypedDict): assertionConsumerService: dict singleLogoutService: dict attributeConsumingServices: list[dict] - NameIDFormat: str + NameIDFormat: str # may not be included for eHerkenning x509cert: str privateKey: str privateKeyPassphrase: Optional[str] From 0d0990605977c4a80426ae1cff89fb2a83200b02 Mon Sep 17 00:00:00 2001 From: Sergei Maertens Date: Wed, 18 Dec 2024 18:00:35 +0100 Subject: [PATCH 5/8] :alien: [open-formulieren/open-forms#4785] Mark one ACS as default in eHerkenning metadata If multiple assertion consumer services are present in the metadata (which is the case if you do both eherkenning AND eidas), then one MUST be marked as default according to the eherkenning specification: https://afsprakenstelsel.etoegang.nl/Startpagina/v3/dv-metadata-for-hm The implementation of this sucks. python3-saml has no way of providing this and is also not that easy to extend to only override a small bit (it would actually be really useful if everything worked with lxml nodes instead of strings), so our SAML client now dynamically uses a different settings class/namespace from python3-saml, which allows us to use a different metadata class so that we can override the static method that actually generates the XML string, to finally use string replacement to change the element and add an additional attribute. The configuration for this flows from an entirely different place and uses magic configuration dicts that somehow end up all in the same place allowing us to figure out which index needs to be replaced in the XML, because the index value is user-input from the admin interface. And it gets worse, because the method that is overridden here seems to be only present in our fork of the library? Though I suppose that does offer options to solve this in python3-saml rather than this dirty hack, but on the other hand I'd like to not have to maintain a fork at all... --- digid_eherkenning/models/eherkenning.py | 4 ++ digid_eherkenning/saml2/base.py | 5 +- digid_eherkenning/saml2/eherkenning.py | 46 +++++++++++++- digid_eherkenning/types.py | 1 + tests/test_eherkenning_metadata.py | 79 ++++++++++++++----------- 5 files changed, 97 insertions(+), 38 deletions(-) diff --git a/digid_eherkenning/models/eherkenning.py b/digid_eherkenning/models/eherkenning.py index 78f1ab1..a0b2101 100644 --- a/digid_eherkenning/models/eherkenning.py +++ b/digid_eherkenning/models/eherkenning.py @@ -203,6 +203,10 @@ def as_dict(self) -> EHerkenningConfig: "service_uuid": str(self.eh_service_uuid), "service_name": self.service_name, "attribute_consuming_service_index": self.eh_attribute_consuming_service_index, + # always mark EH as default and EIDAS as not the default. If we ever support + # more assertion consumer services than these two, then we need to expand on + # this logic/configuration. + "mark_default": True, "service_instance_uuid": str(self.eh_service_instance_uuid), "service_description": self.service_description, "service_description_url": self.service_description_url, diff --git a/digid_eherkenning/saml2/base.py b/digid_eherkenning/saml2/base.py index 15baa93..d9d5a41 100644 --- a/digid_eherkenning/saml2/base.py +++ b/digid_eherkenning/saml2/base.py @@ -84,6 +84,8 @@ class BaseSaml2Client: "custom_base_path": None, } + settings_cls = OneLogin_Saml2_Settings + def __init__(self, conf=None): self.authn_storage = AuthnRequestStorage( self.cache_key_prefix, self.cache_timeout @@ -203,7 +205,8 @@ def create_config(self, config_dict): """ Convert to the format expected by the OneLogin SAML2 library. """ - return OneLogin_Saml2_Settings(config_dict, **self.saml2_setting_kwargs) + cls = self.settings_cls + return cls(config_dict, **self.saml2_setting_kwargs) def create_config_dict(self, conf): """ diff --git a/digid_eherkenning/saml2/eherkenning.py b/digid_eherkenning/saml2/eherkenning.py index a64d0b5..ee8e30c 100644 --- a/digid_eherkenning/saml2/eherkenning.py +++ b/digid_eherkenning/saml2/eherkenning.py @@ -11,6 +11,7 @@ from cryptography.x509 import load_pem_x509_certificate from lxml.builder import ElementMaker from lxml.etree import Element, tostring +from onelogin.saml2.metadata import OneLogin_Saml2_Metadata from onelogin.saml2.settings import OneLogin_Saml2_Settings from ..models import EherkenningConfiguration @@ -406,7 +407,7 @@ def get_metadata_eherkenning_requested_attributes( conf: ServiceConfig, service_id: str ) -> list[dict]: # There needs to be a RequestedAttribute element where the name is the ServiceID - # https://afsprakenstelsel.etoegang.nl/display/as/DV+metadata+for+HM + # https://afsprakenstelsel.etoegang.nl/Startpagina/v3/dv-metadata-for-hm requested_attributes = [{"name": service_id, "isRequired": False}] for requested_attribute in conf.get("requested_attributes", []): if isinstance(requested_attribute, dict): @@ -447,15 +448,58 @@ def create_attribute_consuming_services(conf: EHerkenningConfig) -> list[dict]: "serviceDescription": service_description, "requestedAttributes": requested_attributes, "language": service.get("language", "nl"), + "mark_default": service.get("mark_default", False), } ) return attribute_consuming_services +class CustomOneLogin_Saml2_Metadata(OneLogin_Saml2_Metadata): + """ + Modify the generated metadata to comply with AfsprakenStelsel 1.24a + """ + + @staticmethod + def make_attribute_consuming_services(service_provider: dict): + """ + Add an attribute to the default AttributeConsumingService element. + + .. note:: the upstream master branch has refactored this interface, so once we + rebase on master (quite a task I think), we will have to deal with this too. + """ + result = super( + CustomOneLogin_Saml2_Metadata, CustomOneLogin_Saml2_Metadata + ).make_attribute_consuming_services(service_provider) + + attribute_consuming_services = service_provider["attributeConsumingServices"] + if len(attribute_consuming_services) > 1: + # find the ACS that's marked as default - there *must* be one otherwise we + # don't comply with AfsprakenStelsel 1.24a requirements + default_service_index = next( + acs["index"] + for acs in attribute_consuming_services + if acs["mark_default"] + ) + + # do string replacement, because we can't pass any options to the metadata + # generation to modify this behaviour :/ + needle = f'' + replacement = f'' + result = result.replace(needle, replacement, 1) + + return result + + +class CustomOneLogin_Saml2_Settings(OneLogin_Saml2_Settings): + metadata_class = CustomOneLogin_Saml2_Metadata + + class eHerkenningClient(BaseSaml2Client): cache_key_prefix = "eherkenning" cache_timeout = 60 * 60 # 1 hour + settings_cls = CustomOneLogin_Saml2_Settings + @property def conf(self) -> EHerkenningConfig: if not hasattr(self, "_conf"): diff --git a/digid_eherkenning/types.py b/digid_eherkenning/types.py index f55e1f4..7e9730b 100644 --- a/digid_eherkenning/types.py +++ b/digid_eherkenning/types.py @@ -20,6 +20,7 @@ class ServiceConfig(TypedDict): entity_concerned_types_allowed: list[dict] language: str classifiers: Optional[list[str]] + mark_default: bool class EHerkenningConfig(TypedDict): diff --git a/tests/test_eherkenning_metadata.py b/tests/test_eherkenning_metadata.py index 44742c2..f9c0242 100644 --- a/tests/test_eherkenning_metadata.py +++ b/tests/test_eherkenning_metadata.py @@ -235,42 +235,49 @@ def test_generate_metadata_all_options_specified(self): eh_attribute_consuming_service_node = attribute_consuming_service_nodes[0] eidas_attribute_consuming_service_node = attribute_consuming_service_nodes[1] - self.assertEqual( - "urn:etoegang:DV:00000000000000000011:services:9050", - eh_attribute_consuming_service_node.find( - ".//md:RequestedAttribute", namespaces=NAME_SPACES - ).attrib["Name"], - ) - self.assertEqual( - "Test Service Name", - eh_attribute_consuming_service_node.find( - ".//md:ServiceName", namespaces=NAME_SPACES - ).text, - ) - self.assertEqual( - "Test Service Description", - eh_attribute_consuming_service_node.find( - ".//md:ServiceDescription", namespaces=NAME_SPACES - ).text, - ) - self.assertEqual( - "urn:etoegang:DV:00000000000000000011:services:9051", - eidas_attribute_consuming_service_node.find( - ".//md:RequestedAttribute", namespaces=NAME_SPACES - ).attrib["Name"], - ) - self.assertEqual( - "Test Service Name (eIDAS)", - eidas_attribute_consuming_service_node.find( - ".//md:ServiceName", namespaces=NAME_SPACES - ).text, - ) - self.assertEqual( - "Test Service Description", - eidas_attribute_consuming_service_node.find( - ".//md:ServiceDescription", namespaces=NAME_SPACES - ).text, - ) + with self.subTest("eh attribute consuming service"): + self.assertEqual( + eh_attribute_consuming_service_node.attrib["isDefault"], + "true", + ) + self.assertEqual( + "urn:etoegang:DV:00000000000000000011:services:9050", + eh_attribute_consuming_service_node.find( + ".//md:RequestedAttribute", namespaces=NAME_SPACES + ).attrib["Name"], + ) + self.assertEqual( + "Test Service Name", + eh_attribute_consuming_service_node.find( + ".//md:ServiceName", namespaces=NAME_SPACES + ).text, + ) + self.assertEqual( + "Test Service Description", + eh_attribute_consuming_service_node.find( + ".//md:ServiceDescription", namespaces=NAME_SPACES + ).text, + ) + + with self.subTest("eidas attribute consuming service"): + self.assertEqual( + "urn:etoegang:DV:00000000000000000011:services:9051", + eidas_attribute_consuming_service_node.find( + ".//md:RequestedAttribute", namespaces=NAME_SPACES + ).attrib["Name"], + ) + self.assertEqual( + "Test Service Name (eIDAS)", + eidas_attribute_consuming_service_node.find( + ".//md:ServiceName", namespaces=NAME_SPACES + ).text, + ) + self.assertEqual( + "Test Service Description", + eidas_attribute_consuming_service_node.find( + ".//md:ServiceDescription", namespaces=NAME_SPACES + ).text, + ) organisation_name_node = entity_descriptor_node.find( ".//md:OrganizationName", From e28f228adde5bc680d43dba5c89958a6ff28efbc Mon Sep 17 00:00:00 2001 From: Sergei Maertens Date: Thu, 19 Dec 2024 11:11:40 +0100 Subject: [PATCH 6/8] :alien: SHA1 is no longer allowed for EHerkenning Remove the SHA1 choices in the eherkenning configuration. --- digid_eherkenning/choices.py | 17 ++++++- ...configuration_digest_algorithm_and_more.py | 48 +++++++++++++++++++ digid_eherkenning/models/base.py | 47 +++++++++++++++--- digid_eherkenning/models/eherkenning.py | 14 +++++- 4 files changed, 117 insertions(+), 9 deletions(-) create mode 100644 digid_eherkenning/migrations/0014_alter_eherkenningconfiguration_digest_algorithm_and_more.py diff --git a/digid_eherkenning/choices.py b/digid_eherkenning/choices.py index d2f9dda..3a85d93 100644 --- a/digid_eherkenning/choices.py +++ b/digid_eherkenning/choices.py @@ -19,13 +19,28 @@ class SectorType(models.TextChoices): class DigestAlgorithms(models.TextChoices): - sha1 = OneLogin_Saml2_Constants.SHA1, "SHA1" sha256 = OneLogin_Saml2_Constants.SHA256, "SHA256" sha384 = OneLogin_Saml2_Constants.SHA384, "SHA384" sha512 = OneLogin_Saml2_Constants.SHA512, "SHA512" class SignatureAlgorithms(models.TextChoices): + # Deprecated because of the SHA1 options, which appear to still be used with DigiD + rsa_sha256 = OneLogin_Saml2_Constants.RSA_SHA256, "RSA_SHA256" + rsa_sha384 = OneLogin_Saml2_Constants.RSA_SHA384, "RSA_SHA384" + rsa_sha512 = OneLogin_Saml2_Constants.RSA_SHA512, "RSA_SHA512" + + +class DeprecatedDigestAlgorithms(models.TextChoices): + # Deprecated because of the SHA1 options, which appear to still be used with DigiD + sha1 = OneLogin_Saml2_Constants.SHA1, "SHA1" + sha256 = OneLogin_Saml2_Constants.SHA256, "SHA256" + sha384 = OneLogin_Saml2_Constants.SHA384, "SHA384" + sha512 = OneLogin_Saml2_Constants.SHA512, "SHA512" + + +class DeprecatedSignatureAlgorithms(models.TextChoices): + # Deprecated because of the SHA1 options, which appear to still be used with DigiD dsa_sha1 = OneLogin_Saml2_Constants.DSA_SHA1, "DSA_SHA1" rsa_sha1 = OneLogin_Saml2_Constants.RSA_SHA1, "RSA_SHA1" rsa_sha256 = OneLogin_Saml2_Constants.RSA_SHA256, "RSA_SHA256" diff --git a/digid_eherkenning/migrations/0014_alter_eherkenningconfiguration_digest_algorithm_and_more.py b/digid_eherkenning/migrations/0014_alter_eherkenningconfiguration_digest_algorithm_and_more.py new file mode 100644 index 0000000..cbd3b04 --- /dev/null +++ b/digid_eherkenning/migrations/0014_alter_eherkenningconfiguration_digest_algorithm_and_more.py @@ -0,0 +1,48 @@ +# Generated by Django 4.2.13 on 2024-12-19 10:11 + +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ( + "digid_eherkenning", + "0013_alter_eherkenningconfiguration_eh_requested_attributes_and_more", + ), + ] + + operations = [ + migrations.AlterField( + model_name="eherkenningconfiguration", + name="digest_algorithm", + field=models.CharField( + blank=True, + choices=[ + ("http://www.w3.org/2001/04/xmlenc#sha256", "SHA256"), + ("http://www.w3.org/2001/04/xmldsig-more#sha384", "SHA384"), + ("http://www.w3.org/2001/04/xmlenc#sha512", "SHA512"), + ], + default="http://www.w3.org/2001/04/xmlenc#sha256", + help_text="Digest algorithm. Note that SHA1 is deprecated, but still the default value in the SAMLv2 standard. Warning: there are known issues with single-logout functionality if using anything other than SHA1 due to some hardcoded algorithm.", + max_length=100, + verbose_name="digest algorithm", + ), + ), + migrations.AlterField( + model_name="eherkenningconfiguration", + name="signature_algorithm", + field=models.CharField( + blank=True, + choices=[ + ("http://www.w3.org/2001/04/xmldsig-more#rsa-sha256", "RSA_SHA256"), + ("http://www.w3.org/2001/04/xmldsig-more#rsa-sha384", "RSA_SHA384"), + ("http://www.w3.org/2001/04/xmldsig-more#rsa-sha512", "RSA_SHA512"), + ], + default="http://www.w3.org/2001/04/xmldsig-more#rsa-sha256", + help_text="Signature algorithm. Note that DSA_SHA1 and RSA_SHA1 are deprecated, but RSA_SHA1 is still the default value in the SAMLv2 standard. Warning: there are known issues with single-logout functionality if using anything other than SHA1 due to some hardcoded algorithm.", + max_length=100, + verbose_name="signature algorithm", + ), + ), + ] diff --git a/digid_eherkenning/models/base.py b/digid_eherkenning/models/base.py index ddda5ca..2ef9a6f 100644 --- a/digid_eherkenning/models/base.py +++ b/digid_eherkenning/models/base.py @@ -1,3 +1,5 @@ +from typing import TypeVar + from django.core.exceptions import ValidationError from django.core.files.base import ContentFile from django.db import models @@ -12,13 +14,46 @@ from ..choices import ( ConfigTypes, - DigestAlgorithms, - SignatureAlgorithms, + DeprecatedDigestAlgorithms, + DeprecatedSignatureAlgorithms, XMLContentTypes, ) from ..exceptions import CertificateProblem from .certificates import ConfigCertificate +M = TypeVar("M", bound=type[models.Model]) + + +def override_choices( + field: str, + new_choices: type[models.TextChoices], + new_default: models.TextChoices | None = None, +): + """ + Decorator to override the field choices and default for a concrete model. + + The :class:`BaseConfiguration` allows choice selection that can be wider than desired + for specific subclasses. Use this decorator on the subclass to narrow them. + + :arg field: field name to override. The field must exist on the model. + :arg new_choices: the new choices class to use. + :arg new_default: the new default value to use, optional. + """ + + def decorator(cls: M) -> M: + model_field = cls._meta.get_field(field) + assert isinstance(model_field, models.Field) + assert model_field.choices + + # replace the choices and default + model_field.choices = new_choices.choices + if new_default is not None: + model_field.default = new_default + + return cls + + return decorator + class BaseConfiguration(SingletonModel): idp_metadata_file = PrivateMediaFileField( @@ -76,8 +111,8 @@ class BaseConfiguration(SingletonModel): signature_algorithm = models.CharField( _("signature algorithm"), blank=True, - choices=SignatureAlgorithms.choices, - default=SignatureAlgorithms.rsa_sha1, + choices=DeprecatedSignatureAlgorithms.choices, + default=DeprecatedSignatureAlgorithms.rsa_sha1, help_text=_( "Signature algorithm. Note that DSA_SHA1 and RSA_SHA1 are deprecated, but " "RSA_SHA1 is still the default value in the SAMLv2 standard. Warning: " @@ -89,8 +124,8 @@ class BaseConfiguration(SingletonModel): digest_algorithm = models.CharField( _("digest algorithm"), blank=True, - choices=DigestAlgorithms.choices, - default=DigestAlgorithms.sha1, + choices=DeprecatedDigestAlgorithms.choices, + default=DeprecatedDigestAlgorithms.sha1, help_text=_( "Digest algorithm. Note that SHA1 is deprecated, but still the default " "value in the SAMLv2 standard. Warning: " diff --git a/digid_eherkenning/models/eherkenning.py b/digid_eherkenning/models/eherkenning.py index a0b2101..b8fda7c 100644 --- a/digid_eherkenning/models/eherkenning.py +++ b/digid_eherkenning/models/eherkenning.py @@ -3,10 +3,10 @@ from django.db import models from django.utils.translation import gettext_lazy as _ -from ..choices import AssuranceLevels +from ..choices import AssuranceLevels, DigestAlgorithms, SignatureAlgorithms from ..types import EHerkenningConfig from ..validators import oin_validator -from .base import BaseConfiguration +from .base import BaseConfiguration, override_choices def get_default_requested_attributes_eidas(): @@ -46,6 +46,16 @@ def get_default_requested_attributes_eidas(): ] +@override_choices( + "signature_algorithm", + new_choices=SignatureAlgorithms, + new_default=SignatureAlgorithms.rsa_sha256, +) +@override_choices( + "digest_algorithm", + new_choices=DigestAlgorithms, + new_default=DigestAlgorithms.sha256, +) class EherkenningConfiguration(BaseConfiguration): eh_loa = models.CharField( _("eHerkenning LoA"), From 8a7c03ec47dd094836f1e9587b5aef0cd88222f6 Mon Sep 17 00:00:00 2001 From: Sergei Maertens Date: Thu, 19 Dec 2024 11:29:17 +0100 Subject: [PATCH 7/8] :test_tube: [open-formulieren/open-forms#4785] Add test for unexpected 'use' attribute Per the standard, key descriptors elements must contain at least one key that is used for signing and at least one key used for encryption, OR, if both are done with a single key, the 'use' attribute must not be specified to imply the default behaviour. See https://afsprakenstelsel.etoegang.nl/Startpagina/v3/dv-metadata-for-hm version AS1.24a --- tests/test_eherkenning_metadata.py | 142 ++++++++++++++++------------- 1 file changed, 80 insertions(+), 62 deletions(-) diff --git a/tests/test_eherkenning_metadata.py b/tests/test_eherkenning_metadata.py index f9c0242..a50d147 100644 --- a/tests/test_eherkenning_metadata.py +++ b/tests/test_eherkenning_metadata.py @@ -182,40 +182,56 @@ def test_generate_metadata_all_options_specified(self): "http://test-entity.id", entity_descriptor_node.attrib["entityID"] ) - sspo_descriptor_node = entity_descriptor_node.find( - ".//md:SPSSODescriptor", - namespaces=NAME_SPACES, - ) + with self.subTest("metadata signature"): + certificate_node = entity_descriptor_node.find( + ".//ds:X509Certificate", + namespaces=NAME_SPACES, + ) + self.assertIn( + "MIIC0DCCAbigAwIBAgIUEjGmfCGa1cOiTi+UKtDQVtySOHUwDQYJKoZIhvcNAQEL", + certificate_node.text, + ) - self.assertEqual("true", sspo_descriptor_node.attrib["AuthnRequestsSigned"]) - self.assertEqual("true", sspo_descriptor_node.attrib["WantAssertionsSigned"]) + signature_algorithm_node = entity_descriptor_node.find( + ".//ds:SignatureMethod", + namespaces=NAME_SPACES, + ) + self.assertEqual( + "http://www.w3.org/2001/04/xmldsig-more#rsa-sha256", + signature_algorithm_node.attrib["Algorithm"], + ) - certificate_node = entity_descriptor_node.find( - ".//ds:X509Certificate", - namespaces=NAME_SPACES, - ) - self.assertIn( - "MIIC0DCCAbigAwIBAgIUEjGmfCGa1cOiTi+UKtDQVtySOHUwDQYJKoZIhvcNAQEL", - certificate_node.text, - ) + digest_algorithm_node = entity_descriptor_node.find( + ".//ds:DigestMethod", + namespaces=NAME_SPACES, + ) + self.assertEqual( + "http://www.w3.org/2001/04/xmlenc#sha256", + digest_algorithm_node.attrib["Algorithm"], + ) - signature_algorithm_node = entity_descriptor_node.find( - ".//ds:SignatureMethod", - namespaces=NAME_SPACES, - ) - self.assertEqual( - "http://www.w3.org/2001/04/xmldsig-more#rsa-sha256", - signature_algorithm_node.attrib["Algorithm"], - ) + with self.subTest("SPSSODescriptor"): + sspo_descriptor_node = entity_descriptor_node.find( + ".//md:SPSSODescriptor", + namespaces=NAME_SPACES, + ) + assert sspo_descriptor_node is not None - digest_algorithm_node = entity_descriptor_node.find( - ".//ds:DigestMethod", - namespaces=NAME_SPACES, - ) - self.assertEqual( - "http://www.w3.org/2001/04/xmlenc#sha256", - digest_algorithm_node.attrib["Algorithm"], - ) + self.assertEqual("true", sspo_descriptor_node.attrib["AuthnRequestsSigned"]) + self.assertEqual( + "true", sspo_descriptor_node.attrib["WantAssertionsSigned"] + ) + + with self.subTest("key descriptors"): + key_descriptor_nodes = sspo_descriptor_node.findall( + ".//md:KeyDescriptor", namespaces=NAME_SPACES + ) + self.assertEqual(len(key_descriptor_nodes), 1) + + key_descriptor_node = key_descriptor_nodes[0] + # use attribute should not be specified if it's used for both signing and + # encryption + self.assertNotIn("use", key_descriptor_node.attrib) assertion_consuming_service_node = entity_descriptor_node.find( ".//md:AssertionConsumerService", @@ -279,41 +295,43 @@ def test_generate_metadata_all_options_specified(self): ).text, ) - organisation_name_node = entity_descriptor_node.find( - ".//md:OrganizationName", - namespaces=NAME_SPACES, - ) - self.assertEqual("Test organisation", organisation_name_node.text) + with self.subTest("organization details"): + organisation_name_node = entity_descriptor_node.find( + ".//md:OrganizationName", + namespaces=NAME_SPACES, + ) + self.assertEqual("Test organisation", organisation_name_node.text) - organisation_display_node = entity_descriptor_node.find( - ".//md:OrganizationDisplayName", - namespaces=NAME_SPACES, - ) - self.assertEqual("Test organisation", organisation_display_node.text) + organisation_display_node = entity_descriptor_node.find( + ".//md:OrganizationDisplayName", + namespaces=NAME_SPACES, + ) + self.assertEqual("Test organisation", organisation_display_node.text) - organisation_url_node = entity_descriptor_node.find( - ".//md:OrganizationURL", - namespaces=NAME_SPACES, - ) - self.assertEqual("http://test-organisation.nl", organisation_url_node.text) + organisation_url_node = entity_descriptor_node.find( + ".//md:OrganizationURL", + namespaces=NAME_SPACES, + ) + self.assertEqual("http://test-organisation.nl", organisation_url_node.text) - contact_person_node = entity_descriptor_node.find( - ".//md:ContactPerson", - namespaces=NAME_SPACES, - ) - self.assertEqual("technical", contact_person_node.attrib["contactType"]) + with self.subTest("technical contact person details"): + contact_person_node = entity_descriptor_node.find( + ".//md:ContactPerson", + namespaces=NAME_SPACES, + ) + self.assertEqual("technical", contact_person_node.attrib["contactType"]) - contact_email_node = entity_descriptor_node.find( - ".//md:EmailAddress", - namespaces=NAME_SPACES, - ) - self.assertEqual("test@test.nl", contact_email_node.text) + contact_email_node = entity_descriptor_node.find( + ".//md:EmailAddress", + namespaces=NAME_SPACES, + ) + self.assertEqual("test@test.nl", contact_email_node.text) - contact_telephone_node = entity_descriptor_node.find( - ".//md:TelephoneNumber", - namespaces=NAME_SPACES, - ) - self.assertEqual("06123123123", contact_telephone_node.text) + contact_telephone_node = entity_descriptor_node.find( + ".//md:TelephoneNumber", + namespaces=NAME_SPACES, + ) + self.assertEqual("06123123123", contact_telephone_node.text) def test_contact_telephone_no_email(self): self.eherkenning_config.technical_contact_person_telephone = "06123123123" @@ -418,8 +436,8 @@ def test_current_and_next_certificate_in_metadata( key_nodes = metadata_node.findall("md:KeyDescriptor", namespaces=NAME_SPACES) assert len(key_nodes) == 2 # we expect current + next key key1_node, key2_node = key_nodes - assert key1_node.attrib["use"] == "signing" - assert key2_node.attrib["use"] == "signing" + assert "use" not in key1_node.attrib + assert "use" not in key2_node.attrib with ( eherkenning_certificate.public_certificate.open("r") as _current, From a86e3250cc1da68691ad31ba198564485f4a4ac7 Mon Sep 17 00:00:00 2001 From: Sergei Maertens Date: Thu, 19 Dec 2024 12:59:29 +0100 Subject: [PATCH 8/8] :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