authentik
authentik copied to clipboard
goauthentik
Sources/Saml: Basic support for EncryptedAssertion element.
Details
This PR aims to be an initial step in expanding Authentik's SAML2 Federation capabilities to adhere more fully to the entire SAML2 specification. It does so by providing the option to use an existing signing key-pair for requesting the Assertion element to be encrypted by the SAML IdP. I've attempted to make it such that existing SAML federations should not be disturbed by this change. Currently, the only way to enable that the Assertion should be encrypted, is by updating the "request_encrypted_assertions" model field to be True, instead of the default "False" value. I'm not sure how to update Authentik's UI but I'm hoping this is a small fix.
If the field is set to "True" for a SAMLSource, the metadata will be updated with an encryption key descriptor, as well as a new attribute in the SPSSO descriptor (WantAssertionsEncrypted="True"). This will inform the IdP that assertion should be encrypted. When the SAMLResponse is recieved, the _decrypt_response() method should locate and decrypt the EncryptedData in the EncryptedAssertion element and finally replacing the EncryptedAssertion with the decrypted Assertion. In this way, once the flow proceeds the SAMLResponse will appear as if it was never encrypted to begin with.
We have tested this decryption flow against the The Danish Agency for Digitization's Municipal Access Management service, that requires the service provider to request encrypted assertions.
Checklist
- [x] Local tests pass (
ak test authentik/sources/saml) - [x] The code has been formatted (
make lint-fix) (EXCEPTION: In the metadata processor, the signing key descriptor method does not use the Union type, even though make lint-fix wants it to. I've chosen to adopt the same return type for the encryption key descriptor.)
If applicable
- [ ] The documentation has been updated
- [ ] The documentation has been formatted (
make website)
Deploy Preview for authentik-storybook ready!
| Name | Link |
|---|---|
| Latest commit | c438557a915a4d9f400314a88691596fe0a0c116 |
| Latest deploy log | https://app.netlify.com/sites/authentik-storybook/deploys/66b3b00299e082000719425c |
| Deploy Preview | https://deploy-preview-10099--authentik-storybook.netlify.app |
| Preview on mobile | Toggle QR Code...Use your smartphone camera to open QR code link. |
To edit notification comments on pull requests, go to your Netlify site configuration.
![netlify[bot] avatar](https://avatars.githubusercontent.com/in/13473?v=4 "Published by a pub.dev verified publisher"){kind=small}
Deploy Preview for authentik-docs canceled.
| Name | Link |
|---|---|
| Latest commit | c438557a915a4d9f400314a88691596fe0a0c116 |
| Latest deploy log | https://app.netlify.com/sites/authentik-docs/deploys/66b3b002ef5e0d00087715e0 |
This PR attempts to solve: https://github.com/goauthentik/authentik/issues/9172
@BeryJu: What can we do to help this PR? Its currently a blocker in our project, not being able to communicate with The Danish Agency for Digitization's Municipal Access Management service.
Sorry I'm currently travelling and haven't been able to go through a bunch of PRs, I'll review this later this week/early next week when I get back home.
Sorry I'm currently travelling and haven't been able to go through a bunch of PRs, I'll review this later this week/early next week when I get back home.
Thankyou for clearing this up! Safe travels!
Codecov Report
Attention: Patch coverage is 91.04478% with 6 lines in your changes missing coverage. Please review.
Project coverage is 92.62%. Comparing base (
134caa9) to head (c438557).
Additional details and impacted files
@@ Coverage Diff @@
## main #10099 +/- ##
==========================================
- Coverage 92.63% 92.62% -0.01%
==========================================
Files 734 734
Lines 35980 36039 +59
==========================================
+ Hits 33329 33381 +52
- Misses 2651 2658 +7
| Flag | Coverage Δ | |
|---|---|---|
| e2e | 49.41% <14.92%> (-0.09%) |
:arrow_down: |
| integration | 25.18% <1.49%> (-0.04%) |
:arrow_down: |
| unit | 90.17% <89.55%> (+<0.01%) |
:arrow_up: |
Flags with carried forward coverage won't be shown. Click here to find out more.
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
![codecov[bot] avatar](https://avatars.githubusercontent.com/in/254?v=4 "Published by a pub.dev verified publisher"){kind=small}
The lint check fails due to the Exception mentioned above.
EXCEPTION: In the metadata processor, the signing key descriptor method does not use the Union type, even though make lint-fix wants it to. I've chosen to adopt the same return type for the encryption key descriptor.
Maybe I can fix this by simply adding the # noqa comment to the line? I'm unsure as to why the container build checks fail, seems I'm getting some 403 Status code on Post requests to ghcr.io/
Any thoughts?
I can't push to the branch on the forked repo, but here's the diff for adding the option to the UI:
diff --git a/authentik/sources/saml/api/source.py b/authentik/sources/saml/api/source.py
index a3f0e9bd4..1af2a8795 100644
--- a/authentik/sources/saml/api/source.py
+++ b/authentik/sources/saml/api/source.py
@@ -32,6 +32,7 @@ class SAMLSourceSerializer(SourceSerializer):
"digest_algorithm",
"signature_algorithm",
"temporary_user_delete_after",
+ "request_encrypted_assertions",
]
diff --git a/authentik/sources/saml/processors/metadata.py b/authentik/sources/saml/processors/metadata.py
index c12a55556..0820a3159 100644
--- a/authentik/sources/saml/processors/metadata.py
+++ b/authentik/sources/saml/processors/metadata.py
@@ -46,7 +46,7 @@ class MetadataProcessor:
return key_descriptor
return None
- def get_encryption_key_descriptor(self) -> Optional[Element]: # noqa: UP007
+ def get_encryption_key_descriptor(self) -> Optional[Element]: # noqa: UP007
"""Get Encryption KeyDescriptor, if encrypted assertion is requested"""
if self.source.request_encrypted_assertions:
key_descriptor = Element(f"{{{NS_SAML_METADATA}}}KeyDescriptor")
diff --git a/authentik/sources/saml/processors/response.py b/authentik/sources/saml/processors/response.py
index 805f59ee4..5434a2c98 100644
--- a/authentik/sources/saml/processors/response.py
+++ b/authentik/sources/saml/processors/response.py
@@ -83,7 +83,6 @@ class ResponseProcessor:
def _decrypt_response(self):
"""Decrypt SAMLResponse EncryptedAssertion Element"""
-
manager = xmlsec.KeysManager()
key = xmlsec.Key.from_memory(
self._source.signing_kp.key_data,
@@ -93,9 +92,7 @@ class ResponseProcessor:
manager.add_key(key)
encryption_context = xmlsec.EncryptionContext(manager)
- encrypted_assertion = self._root.find(
- ".//{urn:oasis:names:tc:SAML:2.0:assertion}EncryptedAssertion"
- )
+ encrypted_assertion = self._root.find(f".//{{{NS_SAML_ASSERTION}}}EncryptedAssertion")
encrypted_data = xmlsec.tree.find_child(
encrypted_assertion, "EncryptedData", xmlsec.constants.EncNs
)
diff --git a/blueprints/schema.json b/blueprints/schema.json
index d32ba34e5..b321222ba 100644
--- a/blueprints/schema.json
+++ b/blueprints/schema.json
@@ -5106,6 +5106,11 @@
"minLength": 1,
"title": "Delete temporary users after",
"description": "Time offset when temporary users should be deleted. This only applies if your IDP uses the NameID Format 'transient', and the user doesn't log out manually. (Format: hours=1;minutes=2;seconds=3)."
+ },
+ "request_encrypted_assertions": {
+ "type": "boolean",
+ "title": "Request encrypted assertions",
+ "description": "When enabled, the SAML IdP will encrypt the assertion element using the public key of the SP signing keypair. The SAMLResponse will contain an EncryptedAssertion element, which will be decrypted by the private key of the service provider."
}
},
"required": []
diff --git a/schema.yml b/schema.yml
index 822318ad1..05fcb7384 100644
--- a/schema.yml
+++ b/schema.yml
@@ -43429,6 +43429,12 @@ components:
description: 'Time offset when temporary users should be deleted. This only
applies if your IDP uses the NameID Format ''transient'', and the user
doesn''t log out manually. (Format: hours=1;minutes=2;seconds=3).'
+ request_encrypted_assertions:
+ type: boolean
+ description: When enabled, the SAML IdP will encrypt the assertion element
+ using the public key of the SP signing keypair. The SAMLResponse will
+ contain an EncryptedAssertion element, which will be decrypted by the
+ private key of the service provider.
PatchedSCIMMappingRequest:
type: object
description: SCIMMapping Serializer
@@ -46114,6 +46120,12 @@ components:
description: 'Time offset when temporary users should be deleted. This only
applies if your IDP uses the NameID Format ''transient'', and the user
doesn''t log out manually. (Format: hours=1;minutes=2;seconds=3).'
+ request_encrypted_assertions:
+ type: boolean
+ description: When enabled, the SAML IdP will encrypt the assertion element
+ using the public key of the SP signing keypair. The SAMLResponse will
+ contain an EncryptedAssertion element, which will be decrypted by the
+ private key of the service provider.
required:
- component
- icon
@@ -46217,6 +46229,12 @@ components:
description: 'Time offset when temporary users should be deleted. This only
applies if your IDP uses the NameID Format ''transient'', and the user
doesn''t log out manually. (Format: hours=1;minutes=2;seconds=3).'
+ request_encrypted_assertions:
+ type: boolean
+ description: When enabled, the SAML IdP will encrypt the assertion element
+ using the public key of the SP signing keypair. The SAMLResponse will
+ contain an EncryptedAssertion element, which will be decrypted by the
+ private key of the service provider.
required:
- name
- pre_authentication_flow
diff --git a/web/src/admin/sources/saml/SAMLSourceForm.ts b/web/src/admin/sources/saml/SAMLSourceForm.ts
index c969411fb..945a74bfd 100644
--- a/web/src/admin/sources/saml/SAMLSourceForm.ts
+++ b/web/src/admin/sources/saml/SAMLSourceForm.ts
@@ -449,6 +449,25 @@ export class SAMLSourceForm extends WithCapabilitiesConfig(BaseSourceForm<SAMLSo
>
</ak-radio>
</ak-form-element-horizontal>
+ <ak-form-element-horizontal name="requestEncryptedAssertions">
+ <label class="pf-c-switch">
+ <input
+ class="pf-c-switch__input"
+ type="checkbox"
+ ?checked=${first(this.instance?.requestEncryptedAssertions, true)}
+ />
+ <span class="pf-c-switch__toggle">
+ <span class="pf-c-switch__toggle-icon">
+ <i class="fas fa-check" aria-hidden="true"></i>
+ </span>
+ </span>
+ <span class="pf-c-switch__label"
+ >${msg(
+ "Request Encrypted assertions from Identity provider.",
+ )}</span
+ >
+ </label>
+ </ak-form-element-horizontal>
</div>
</ak-form-group>
<ak-form-group>
One thing I would probably change about this is explicitly throw an error if request_encrypted_assertions is set but the assertion is not encrypted...but maybe that would warrant renaming request_encrypted_assertions to require_encrypted_assertions
Another thing, shouldn't the initial AuthnRequest request we build also be encrypted/encryptable? I assume that would be a future addition
I can't push to the branch on the forked repo..
@BeryJu: You have been added as outside collaborator with write permissions to the fork...
Thank you very much for the UI additions - that will greatly simplify the process of setting up this type of SAML source.
One thing I would probably change about this is explicitly throw an error if request_encrypted_assertions is set but the assertion is not encrypted...but maybe that would warrant renaming request_encrypted_assertions to require_encrypted_assertions
Definitely a good addition. I'm fine with renaming the model field to reflect the requirement if need be.
Another thing, shouldn't the initial AuthnRequest request we build also be encrypted/encryptable? I assume that would be a future addition
In my understanding the initial AuthnRequest to the IdP is typically not encrypted, but I'll need to check if the IdP supports decryption AuthnRequests from service providers. The encryption layer is mainly to protect the sensitive information in the Assertion element.
(Please correct me if I'm wrong though!)
Thank you very much for the UI additions - that will greatly simplify the process of setting up this type of SAML source.
One thing I would probably change about this is explicitly throw an error if request_encrypted_assertions is set but the assertion is not encrypted...but maybe that would warrant renaming request_encrypted_assertions to require_encrypted_assertions
Definitely a good addition. I'm fine with renaming the model field to reflect the requirement if need be.
One thing that I noticed in the PR is that there's no explicit error handling, I'm assuming this would throw some xmlsec Error when the encryption is missing/invalid?
Another thing, shouldn't the initial AuthnRequest request we build also be encrypted/encryptable? I assume that would be a future addition
In my understanding the initial AuthnRequest to the IdP is typically not encrypted, but I'll need to check if the IdP supports decryption AuthnRequests from service providers. The encryption layer is mainly to protect the sensitive information in the Assertion element.
(Please correct me if I'm wrong though!)
That makes sense. One other thing I'd like to see for this is some added tests (we can also add those in this PR)
One thing that I noticed in the PR is that there's no explicit error handling, I'm assuming this would throw some xmlsec Error when the encryption is missing/invalid?
Something like this?
def _decrypt_response(self):
"""Decrypt SAMLResponse EncryptedAssertion Element"""
manager = xmlsec.KeysManager()
key = xmlsec.Key.from_memory(
self._source.signing_kp.key_data,
xmlsec.constants.KeyDataFormatPem,
)
manager.add_key(key)
encryption_context = xmlsec.EncryptionContext(manager)
try:
encrypted_assertion = self._root.find(f".//{{{NS_SAML_ASSERTION}}}EncryptedAssertion")
except:
raise MissingEncryptedAssertion()
encrypted_data = xmlsec.tree.find_child(
encrypted_assertion, "EncryptedData", xmlsec.constants.EncNs
)
try:
decrypted_assertion = encryption_context.decrypt(encrypted_data)
except:
raise DecryptionError():
index_of = self._root.index(encrypted_assertion)
self._root.remove(encrypted_assertion)
self._root.insert(
index_of,
decrypted_assertion,
)
LOGGER.debug("Successfully decrypted EncryptedAssertion element")
With the following added to source/saml/exceptions.py ...
class MissingEncryptedAssertion(SAMLException):
"""Exception raised when SAMLResponse does not contain EncryptedAssertion element"""
class DecryptionError(SAMLException):
"""Exception raised when decryption of SAMLResponse could not be performed"""
If this is acceptable I can commit&push, but please let me know if it's too simple. Regards, Nicolas
yeah pretty much what you said, only thing is we usually try to prevent bare excepts (i've pushed some stuff)
one other thing I'm noticing while testing this is that I think it might be a good option to add another certificate setting to the source to configure the signature and encryption certificate separately, as I'm testing against keycloak which by default generates two different certificates for each
yeah pretty much what you said, only thing is we usually try to prevent bare excepts (i've pushed some stuff)
one other thing I'm noticing while testing this is that I think it might be a good option to add another certificate setting to the source to configure the signature and encryption certificate separately, as I'm testing against keycloak which by default generates two different certificates for each
This would be the right way to do it, I only initially planned to re-use the signing keypair for simplicity, but It would be best to have the choice between:
- No Encryption
- Encryption using signing-keypair
- Encryption using a second 'encryption-keypair'
EDIT: A simpler solution could maybe be just to replicate what is done for the signing certificate? That is imply having a drop down menu, where no selection means no Encryption, and choosing a keypair will enable the decryption flow. Then the user would just be able to choose the signing keypair for simplicity, or generate a second keypair for encryption. Thoughts?
EDITEDIT: I realize this changes the implementation we've slowly been building now, but maybe it's better this way?
Kind Regards, Nicolas
yeah pretty much what you said, only thing is we usually try to prevent bare excepts (i've pushed some stuff) one other thing I'm noticing while testing this is that I think it might be a good option to add another certificate setting to the source to configure the signature and encryption certificate separately, as I'm testing against keycloak which by default generates two different certificates for each
This would be the right way to do it, I only initially planned to re-use the signing keypair for simplicity, but It would be best to have the choice between:
- No Encryption
- Encryption using signing-keypair
- Encryption using a second 'encryption-keypair'
EDIT: A simpler solution could maybe be just to replicate what is done for the signing certificate? That is imply having a drop down menu, where no selection means no Encryption, and choosing a keypair will enable the decryption flow. Then the user would just be able to choose the signing keypair for simplicity, or generate a second keypair for encryption. Thoughts?
EDITEDIT: I realize this changes the implementation we've slowly been building now, but maybe it's better this way?
Kind Regards, Nicolas
Sorry didnt see your edits, yeah I think what you suggested in the first Edit makes most sense
yeah pretty much what you said, only thing is we usually try to prevent bare excepts (i've pushed some stuff) one other thing I'm noticing while testing this is that I think it might be a good option to add another certificate setting to the source to configure the signature and encryption certificate separately, as I'm testing against keycloak which by default generates two different certificates for each
This would be the right way to do it, I only initially planned to re-use the signing keypair for simplicity, but It would be best to have the choice between:
- No Encryption
- Encryption using signing-keypair
- Encryption using a second 'encryption-keypair'
EDIT: A simpler solution could maybe be just to replicate what is done for the signing certificate? That is imply having a drop down menu, where no selection means no Encryption, and choosing a keypair will enable the decryption flow. Then the user would just be able to choose the signing keypair for simplicity, or generate a second keypair for encryption. Thoughts? EDITEDIT: I realize this changes the implementation we've slowly been building now, but maybe it's better this way? Kind Regards, Nicolas
Sorry didnt see your edits, yeah I think what you suggested in the first Edit makes most sense
Roger that, will see if I can't figure out how to replicate it.