Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix eherkenning metadata #83

Merged
merged 8 commits into from
Dec 20, 2024
Merged

Conversation

sergei-maertens
Copy link
Member

@sergei-maertens sergei-maertens commented Dec 18, 2024

Partly closes open-formulieren/open-forms#4785

This should bring the library back into line with version 1.24a of the AfsprakenStelsel.

Patches up some findings reported by the KPN broker:

  • Do not include EntityConcernedID requested attributes -> removed the defaults and amended the documentation
  • Removed the forbidden <NameIDFormat> element
  • Mark one ACS as default if multiple are present
  • Default to SHA256 / drop SHA1 support in EH metadata
  • Drop the use="signing" attribute if the certificate is used for both signing and encryption

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.
…ributes

... 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.
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
…enning 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...
Copy link

codecov bot commented Dec 18, 2024

Codecov Report

Attention: Patch coverage is 98.00000% with 1 line in your changes missing coverage. Please review.

Project coverage is 92.31%. Comparing base (5d6df08) to head (a86e325).
Report is 9 commits behind head on master.

Files with missing lines Patch % Lines
digid_eherkenning/models/base.py 91.66% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master      #83      +/-   ##
==========================================
+ Coverage   91.99%   92.31%   +0.32%     
==========================================
  Files          48       48              
  Lines        1648     1691      +43     
  Branches      154      107      -47     
==========================================
+ Hits         1516     1561      +45     
  Misses         96       96              
+ Partials       36       34       -2     
Flag Coverage Δ
base 91.07% <98.00%> (+0.41%) ⬆️
oidc 100.00% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Remove the SHA1 choices in the eherkenning configuration.
…tribute

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
… 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.
@sergei-maertens sergei-maertens merged commit ad0b349 into master Dec 20, 2024
17 checks passed
@sergei-maertens sergei-maertens deleted the issue/4785-eherkenning-metadata branch December 20, 2024 16:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

feedback Signicat/kpn on generated eHerkenning metadata.xml
2 participants