-
Notifications
You must be signed in to change notification settings - Fork 0
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
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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...
Codecov ReportAttention: Patch coverage is
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
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
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.
This was referenced Dec 19, 2024
SilviaAmAm
approved these changes
Dec 20, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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:
EntityConcernedID
requested attributes -> removed the defaults and amended the documentation<NameIDFormat>
elementuse="signing"
attribute if the certificate is used for both signing and encryption