-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
feat: Hardware Security Module support #2625
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you! This looks nice already. I think the three big things we need to address are:
- How do we write tests for HSM that work cross-platform
- Implementing HSM as a different
KeyManager
as opposed to havingif c.HSMEnabled
everywhere - Which library to choose for HSM
Another topic will probably be supporting HSM from cloud vendors (e.g. https://cloud.google.com/kms/docs/hsm) as probably most companies today rely on cloud HSM versus having their own HSM deployed.
While the PR is being worked on I will mark it as a draft. That declutters our review backlog :) Once you're done with your changes and would like someone to review them, mark the PR as ready and request a review from one of the maintainers. Thank you! |
Back from vacation! Working on it! :) |
3f4d527
to
0356a40
Compare
Codecov Report
@@ Coverage Diff @@
## master #2625 +/- ##
==========================================
+ Coverage 78.26% 79.78% +1.51%
==========================================
Files 110 113 +3
Lines 7731 8054 +323
==========================================
+ Hits 6051 6426 +375
+ Misses 1265 1222 -43
+ Partials 415 406 -9
Continue to review full report at Codecov.
|
fb43075
to
5298b1c
Compare
Added quickstart-hsm for testing Lines 88 to 90 in 5298b1c
hydra/.docker/Dockerfile-build Lines 17 to 43 in 5298b1c
but I guess we should add circle-ci step with similar setup to run tests with hsm enabled?
Implemented hsm key manager. See other comments for questions.
Library is HSM vendor specific. So you install/setup the HSM client
Any cloud HSM that supports pkcs11 works exactly the same. For example https://docs.aws.amazon.com/cloudhsm/latest/userguide/pkcs11-library.html |
@aeneasr, how can I help moving this pull request forward? |
Hey @aarmam - due to the security sensitive nature and my lack of knowledge around HSM I will probably need a full week to work on this. It might be helpful if we jump on a call to review it together. I am available again next week - you can ping me on Slack :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Verify that cross compilation works, you can use: https://github.com/ory/xgoreleaser#testing-builds
- We need to ensure that HSM does not require CGO. If it does require CGO, we need to have a build flag (similar to
-tags sqlite
) that enables HSM and thus also the dependency on CGO. - Exclude "real" HSM tests with a go build tag to ensure you can run the full tests without having a soft HSM library installed
- See if soft HSM works on macOS
- Write a guide for "developing the HSM feature". i.e. how to install soft HSM (e.g.
apt-get install softhsm
) and how to set up test keys (e.g.pkcs-tool ...
)
Clean up tests with a helper func:
Table tests
Or use testify.Suite: https://github.com/stretchr/testify#suite-package |
Running CircleCI locally: https://circleci.com/docs/2.0/local-cli/#run-a-job-in-a-container-on-your-machine Add HSM set up to (e.g. Line 68 in 8373bba
|
4be3076
to
32dfda3
Compare
hsm/manager_hsm.go
Outdated
switch k := key.Public().(type) { | ||
case *rsa.PublicKey: | ||
alg = "RS256" | ||
// TODO: Should we validate minimal key length by checking CKA_MODULUS_BITS? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@aarmam do you want to address this one? Otherwise we can remove the TODO item I think
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When HSM mode is enabled and no keys are found then RSA keys are generated with key length 4096. But if keys are generated on HSM beforehand then there is no key limit set. We could throw error here if key length is too small. I'm on vacation right now and not able to write serious code :) So I would remove this TODO right now if you think its not important or leave this in and I will do separate PR for this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, that sounds good to me! I will create a follow up issue for it and link it here :)
@aarmam this looks basically good to merge! However, I still have one question left. As far as I can tell, the Go code does not actually test HSM support with integration? I saw that we use quite a lot of mocking (which is awesome 😎 ) but I realized that we do not actually use the HSM library in the Go test. I think this is ok as you have covered all the cases, but I wanted to check in if this is intentional or not. I also saw Another thing I noticed is that we introduce the From what I can tell, Ory Hydra would also build without that tag normally on all systems. I don't have HSM support on my mac installed yet, but it runs fine. Otherwise, from a code perspective, this is good to go. Sorry for not reviewing earlier :( But I have time these next days to review the PR and get it merged before NYE :) |
Unit tests that use mocks are executed always (HSM enabled/disabled).
I used the cross compilation tool you recommended and it seemed that ARM based goarch targets did not compile. I might be completely wrong but It seemed so? So I had to use build tags -> https://github.com/ory/hydra/blob/2fab67a444051ca15b04b35941bec1fcb0941d91/.goreleaser.yml I'm on vacation and might respond slowly, but I will rebase this PR later in the evening! And yes if possible lets try to merge it before NYE :) Let me know if you have more questions! |
Cross compile on ARM might indeed be an issue, I will check it out. Awesome! I’m also on vacation, I hope you enjoy yours and have time to spend with your family & loved ones and don’t get too busy reading GitHub 😇 |
You're right, cross compilation does indeed not work out of the box. So yes, the HSM build tags do make sense. Unfortunately my internet is incredibly slow, but I'll try to check wether cross compile works in our cross compile image. If not, we have a small problem 😅 |
# Conflicts: # driver/config/provider.go # driver/registry.go # driver/registry_sql.go # go.mod # persistence/sql/persister_test.go
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the clarifications and very, very hard work. I fixed the tests and ensured cross compilation works (it seems to work as far as I can tell). This is now good to merge if the CI passes!
Hm, not sure what's going on, but the HSM tests fail with an unrelated test: This test though passes on master and also in the main pipeline (without HSM enabled). Could you maybe take a look when you have a bit of time @aarmam ? Once resolved, we can merge this! |
These tests are related to #2384 that was just merged. I'm not sure how to resolve this right now. As I understand #2384 involves trusting public keys. If HSM is enabled hardware key manager is the default key manager. Keys API uses hardware key manager to Find/Generate/Delete keys, but the newly added Trust API uses software key manager. It uses AddKey method to store the public key. This means that this key is not found when using Keys API. There is a possibility to store x509 certificates to HSM using crypto11 so that public key could be stored and retrieved. Importing public key only is not supported by crypto11 at this moment. How many keys are expected to be trusted with #2384 - hundreds/thousands/more? If nr. of keys are in a range of HSM capabilities (I don't know what's reasonable) I could figure something with crypto11 import certificate functionality. Otherwise I'm not sure at the moment how to resolve this. And this might be problem with future features also, that need to import public keys or keypairs. I guess it would be possible to refactor code so that software/hardware key manager can be selected when accessing Keys API etc. If you have ideas please let me know! |
I see, I am glad we were able to catch this!
Depending on the use case it could be up to millions as this feature will be used similar to SAML assertions. Basically you exchange a JWT for an access token.
Wouldn't it suffice to use the software key manager in this scenario, and use HSM only for system-level keys? |
An alternative could also be to have two lookup strategies. When HSM is enabled, we try to look up in HSM first, then in software. When keys are added, we always add it to software. So it's not an either/or question, but something "on top". |
Thanks I implemented this variant! I hope I was not too inattentive, I'm still in vacation mood :) |
Great work man! |
# Conflicts: # driver/registry_base.go # driver/registry_sql.go # go.mod
Hello @aarmam |
Back from vacation and really nice news to kickstart the work mode!:tada::relaxed: Thank you @aeneasr and thanks for all the guidance! |
This change introduces support for Hardware Security Modules, a physical computing device that safeguards and manages digital keys, performs encryption and decryption functions for digital signatures, strong authentication, and other cryptographic functions.
If enabled, the Hardware Security Module is used to look up any keys. If no key is found, the software module is used as a fallback for lookup. This allows you to use the HSM for privileged keys, and the software module to manage lifecycle keys (e.g. for Token Exchange).
For more information, please read the guide.
Thank you to aarmam for this great contribution!
Proposed changes
Hardware Security Module support for keys hydra.openid.id-token, hydra.jwt.access-token using PKCS#11 Cryptographic Token Interface Standard
Checklist
contributing code guidelines.
vulnerability. If this pull request addresses a security. vulnerability, I
confirm that I got green light (please contact
security@ory.sh) from the maintainers to push
the changes.
works.
Further comments
Related PR in ory/fosite