Support for unwrapping key via an HSM when decrypting the SAML assertion.#290
Support for unwrapping key via an HSM when decrypting the SAML assertion.#290pitbulk merged 3 commits intoSAML-Toolkits:masterfrom web-frankenstein:master
Conversation
…e SAML assertion.
2. Added 2 optional dependencies to integrate this library with the Azure Key Vault.
3. Modified the decryptAssertion() method in the SamlResponse class in order to check whether an HSM has been configured or not. If yes, it will call the HSM in order to unwrap the key.
4. Modified the settings validations in the Saml2Settings class in order to validate the following scenarios:
a. If the library is configured with both a private key and an HSM, an error (use_either_hsm_or_private_key) is thrown.
b. If the library is configured to use an HSM, it does not need to check for the SP certificate.
5. Added a new function decryptUsingHsm() in the Util class and moved the validation on the encrypted data to a new function in order to be used both from the decryptUsingHsm() and decryptElement() functions.
6. Added respective tests for the Saml2Settings class and added also the config.hsm.properties file in order to test the scenario when you have the library configured with both the private key and the HSM.
7. Added a new abstract class HSM, which must be used whenever a new HSM needs to be integrated with this library.
8. Added a new class AzureKeyVault which extends the abstract class and implement its abstract methods.
2. Removed any local suppressions since these false positives have been addressed in the above mentioned version.
|
I don't want and need Azure dependencies in my artifacts. |
Hi @ArjanSchouten, that's why I have imported the 2 new azure libraries as optional dependencies so that the library won't include any new dependencies unless you specifically specify them yourself. |
| this.getWantAssertionsEncrypted() == true || | ||
| this.getWantNameIdEncrypted() == true) | ||
| && this.checkSPCerts() == false) { | ||
| if (this.getHsm() == null && (this.getAuthnRequestsSigned() || this.getLogoutRequestSigned() |
There was a problem hiding this comment.
Replace by:
if ((this.getAuthnRequestsSigned() == true ||
this.getLogoutRequestSigned() == true ||
this.getLogoutResponseSigned() == true ||
this.getWantAssertionsEncrypted() == true ||
this.getWantNameIdEncrypted() == true)
&& !this.checkSPCerts() && this.getHsm() == null) {
There was a problem hiding this comment.
Hi @pitbulk
First of all, sorry for my late reply! I have committed the requested changes on my forked repository. Shall you re-open this pull request or shall I open a new one in order to merge these changes?
| Util.decryptElement(encryptedData, key); | ||
|
|
||
| if (hsm != null) { | ||
| Util.decryptUsingHsm(encryptedData, hsm); |
There was a problem hiding this comment.
can you rename to decryptElementUsingHsm ?
|
I left 2 minor changes, and I believe we are ready to go. @ArjanSchouten, As @RobertButtigieg said. The feature dependencies are marked as optional so if you don't require them, not gonna be part of your project. That said, @ArjanSchouten do you see an easy approach to implement this feature as a module? |
|
@pitbulk first of all, thanks for your quick response. I think the HSM abstract class (IMHO it can be an interface) is a core thing. Different implementations such as AzureKeyVault can be in a different module. Since its an optional dependency, my original concern has been addressed. Creating a module for 1 class and 2 dependencies is maybe not worth it 😄 . Thanks 👍 |
This pull request addresses #257
Currently, the library has no support for HSM integration, in particular when it comes to unwrap the key as part of the SAML decryption process.
Hence, this solution will support HSM key unwrapping by setting an HSM object as part of the SAML settings. My requirement was to integrate this library specifically to the Azure Key Vault. However, I have implemented an HSM abstract class in order to allow the possibility of integrating it with other HSMs as well. I have also imported 2 new libraries (for the Azure Key Vault) as optional dependencies since whoever is going to use this library might not require this HSM support.
I have also upgraded the dependency check plugin to the current latest version (v6.0.3) since it includes fixes of the false positives which were flagged when those 2 new libraries were imported.