Skip to content

Commit

Permalink
🐛 [open-formulieren/open-forms#4785] Ensure the key descriptors don't…
Browse files Browse the repository at this point in the history
… 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.
  • Loading branch information
sergei-maertens committed Dec 19, 2024
1 parent 8a7c03e commit a86e325
Showing 1 changed file with 14 additions and 0 deletions.
14 changes: 14 additions & 0 deletions digid_eherkenning/saml2/eherkenning.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down

0 comments on commit a86e325

Please sign in to comment.