Support multiple certificates for DigiD/eHerkenning #79
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
As these will need to be updated for the multi-certificate feature to be supported, it was checked if they are used in the first place. DevOps says it's not used, so removing them is far easier. The metadata itself is still accessible through URLs/views and the admin interface from each configuration page, it's just the CLI integration that is removed.
* Ensure migration tests use temporary private media root * Addressed some type checker issues in update_metadata management command implementation
sergei-maertens
force-pushed
the
feature/74-multi-certificate
branch
from
0bc596e to
0ba2f45
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #79 +/- ##
==========================================
+ Coverage 90.91% 91.97% +1.05%
==========================================
Files 51 48 -3
Lines 1674 1645 -29
Branches 156 153 -3
==========================================
- Hits 1522 1513 -9
+ Misses 110 96 -14
+ Partials 42 36 -6
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Added a proxy model so that we can expose/introspect the valid_from of a given certificate, which will be needed to select the appropriate certificate for authentication requests. The model to link certificate and config deliberately uses a simple constant - a foreign key or m2m doesn't make much sense because we're dealing with singleton/django-solo models of which there will only ever be a single instance. A flatter table layout is therefore possible. Next steps will be adding some tests for the admin and a data migration to copy the single certificate FK field over to the new structure.
Certificates for DigiD/eHerkenning SAMLv2 must satisfy: * validity from * validity until * must be a private key / public cert pair * private key / public cert files must be loadable
sergei-maertens
force-pushed
the
feature/74-multi-certificate
branch
from
5805166 to
da167c7
Compare
This copies over the respective certificate that may be in use. If no certificate is set up or no config exists yet, it's a no-op result.
Added to the migration file with the data migration so everything runs in a single transaction.
Updated the admin pages for DigiD/EH configuration to now point to the place to add new/extra certificates (or manage existing ones). The validation before saving now requires that at least one certificate is defined beforehand, which matches the existing validation rules that the certificate had to be selected.
The SAML client will still grab the configuration from our config dicts, however the config dicts are now populated from the candidate selection helper (to be implemented). This allows us to add tests for edge cases to the certificate selection, and de-duplicates the logic for certificate selection. We are limited by python3-saml which only has hooks for the current and new certificate, and it doesn't support an arbitrary amount of certificates. Subclassing *could* be an option, but then we're forced to copy and modify a lot of code that we actually don't care about and is too risky to forget to update (and the same considerations apply with making the changes in our fork). Given the use case that we want to prepare for certificate replacements, there doesn't seem to be a pressing urgency to support more than 2 certificates in the metadata.
When a next certificate to be used for signing is available/known, ensure that it is passed along to the metadata generation. That way, the new metadata can be exchanged, informing the identity provider of our future certificate, allowing us to seamlessy transition once the 'current' certificate expires. python3-saml supports this through the x509certNew settings dict property.
* Check that the metadata signing certificates match our provided certificates * Check that the singing certificates are distinct from each other
Refactored to mutate the config from the parent function rather than overwriting/replacing it and having to duplicate a bunch of code.
Added a test to ensure that the certificates management link is displayed.
sergei-maertens
commented
Jul 22, 2024
Comment on lines
+14
to
+17
| **Features** | ||
|
|
||
| ... | ||
|
|
There was a problem hiding this comment.
I'll amend this in the release branch once this is done and merged :)
SilviaAmAm
reviewed
Jul 23, 2024
Comment on lines
+38
to
+40
| The certificates are used for signing authentication requests (and metadata) | ||
| itself, and the possible certificates that perform signing are included in the | ||
| metadata. For zero-downtime/gradual replacement, a current and next certificate |
There was a problem hiding this comment.
In the metadata it is possible to include certificates/key for signing and certificates/key for encryption. We don't have separate certificates for encryption (since up to now we only had one certificate).
So if I understand correctly we only add the ability to add multiple certificates/key ONLY for signing.
There was a problem hiding this comment.
Correct - we only support multi-certificate for signing use. I saw indeed that python3-saml has the option to use a different key/cert to sign the metadata itself, but so far there has not been a need for that apparently.
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.
Closes #74