search

spring-projects / spring-security

Spring Security
http://spring.io/projects/spring-security
Apache License 2.0
8.75k stars 5.88k forks source link

Add support for HSM #8349

Open jzheaux opened 4 years ago

jzheaux commented 4 years ago

Many applications will not deal with keys at all but will instead send data to a service like Vault to be encrypted, decrypted, signed, and verified.

Currently, an application needs to implement their own Saml2AuthenticationRequestFactory and AuthenticationProvider to achieve this. It would be nice if applications could implement something more targeted to cryptographic operations.

ryan13mt commented 4 years ago

+1

This would be very useful to have supported due to security requirements on where the private keys are stored.

Koshux commented 4 years ago

+1 Would appreciate this due to a particular use case!

jnunqui commented 4 years ago

+1 would be very useful

cvenkatreddy commented 4 years ago

would be very useful enhancement

ghost commented 4 years ago

+1 This would be useful for us

nafidhaque commented 4 years ago

+1 Would be useful to a particular use case

briannmic commented 4 years ago

I need this for my project

duncanportelli commented 4 years ago

@jzheaux,

Thanks for opening this issue. I am currently working on an IDP-initiated solution and can confirm that a custom AuthenticationProvider must be implemented from scratch in order to achieve this.

The problem is that the OpenSamlAuthenticationProvider is a final class and cannot be extended. If at least, this was not final, this would have been so much easier as only the decrypt(Saml2AuthenticationToken token, EncryptedAssertion assertion) would need to be overridden in order to provide a custom Decrypter

ryan13mt commented 4 years ago

Hi @jzheaux by any chance was there any progress on this or at least a plan to include it in an upcoming release?

jzheaux commented 4 years ago

Hi, @ryan13mt, there is no plan or progress on this issue.

One thing that might help, though, is to begin with a sample.

@duncanportelli, is your implementation something that you can share via GitHub? This could at least get the conversation started about what is the appropriate thing to expose to simplify this. While introducing an inheritance-based solution is a possibility, I wonder if there is a sensible composition-based one.

duncanportelli commented 4 years ago

Hi @jzheaux,

Sure. I uploaded a sample spring boot application in which you can see the number of overridden classes in order to create a custom decrypter to include the decryption logic (in this case via the Azure Key Vault).

ryan13mt commented 4 years ago

Hi @jzheaux and @duncanportelli,

Our use case (having private key on an HSM) would only require to handle the decryption part since that is the only time we need the actual private key to be used. @duncanportelli i'm not sure about your use case, but it seems like ours since in your sample application you mentioned a custom decrypter as well. Since the other keys are all 'public' they can technically be stored locally on the machine.

@jzheaux the company i work at is ready to invest resources in contributing to this issue and develop the code needed to handle a private key stored on an HSM. Would you be willing to accept a pull request of this feature (decryption by HSM)? Or do you require that the contribution includes a full HSM integration that includes encryption, signing and verification as well? Maybe we can create a separate issue that handles the decryption specifically.

Thanks and looking forward to contribute to this open-source community

duncanportelli commented 4 years ago

@ryan13mt, thanks for your message. We are currently working on an IDP initiated SSO solution so signing and encryption is not required in our case. With regards to verification, we are doing it using a public key which doesn't need to be stored in an HSM. That being said, only decryption is required in our scenario as well.

ryan13mt commented 4 years ago

Hey @jzheaux

i just saw that the getDecrypter method has been changed into a DecrypterConverter. If we add a setter for this converter, could we technically just pass it our own custom decrypter to fix this issue?

jzheaux commented 4 years ago

@ryan13mt, yes, that was the idea I was playing with while preparing the 5.4 release.

Yes, a pull request would be welcome. While I understand that your use case is for decryption only, would you be able to provide a PR that handles signature validation as well? I believe it will be the same conceptually, returning a SignatureTrustEngine instead.

ryan13mt commented 4 years ago

@jzheaux wouldn't the signature validation be done using the public certificate provided by the asserting party in their metadata?

Or have i misunderstood your use case?

jzheaux commented 4 years ago

No, @ryan13mt, I just got my wires crossed. The ticket here calls for support in OpenSamlAuthenticationProvider to decrypt assertions as well as OpenSamlAuthenticationRequestFactory to sign requests, and I was thinking it would be nice to address both of these in the same PR.

However, thinking more about this, it would be better to wait on OpenSamlAuthenticationRequestFactory until a contributor has a concrete use case, so I've updated the ticket's description to break things down into individual tasks.

I've got some additional thoughts about decryption. Now that these tasks are separated, I've added those thoughts to #9044.