feat: add /.well-known/jwks.json caching for HSM key manager
This PR adds caching for well known keys when using HSM key manager. Operations per sec on HSM are limited and caching well known keys would make sense. Also if I understand https://github.com/ThalesIgnite/crypto11/blob/master/crypto11.go correctly, then crypto.Signer is thread-safe and hsm.GetKey/hsm.GetKeySet could be cached also, but I will create another PR for this.
Checklist
- [x] I have read the contributing guidelines.
- [x] I have referenced an issue containing the design document if my change introduces a new feature.
- [x] I am following the contributing code guidelines.
- [x] I have read the security policy.
- [x] I confirm that this pull request does not address a security vulnerability. If this pull request addresses a security. vulnerability, I confirm that I got green light (please contact [email protected]) from the maintainers to push the changes.
- [x] I have added tests that prove my fix is effective or that my feature works.
- [ ] I have added or changed the documentation.
Codecov Report
Merging #3259 (37dce4c) into master (4eb13c9) will decrease coverage by
0.06%. The diff coverage is32.50%.
:exclamation: Current head 37dce4c differs from pull request most recent head 741c6b5. Consider uploading reports for the commit 741c6b5 to get more accurate results
@@ Coverage Diff @@
## master #3259 +/- ##
==========================================
- Coverage 76.75% 76.68% -0.07%
==========================================
Files 123 123
Lines 9076 8866 -210
==========================================
- Hits 6966 6799 -167
+ Misses 1668 1641 -27
+ Partials 442 426 -16
| Impacted Files | Coverage Δ | |
|---|---|---|
| driver/registry_sql.go | 64.94% <0.00%> (-2.05%) |
:arrow_down: |
| hsm/manager_nohsm.go | 0.00% <0.00%> (ø) |
|
| jwk/manager.go | 100.00% <ø> (ø) |
|
| jwk/manager_strategy.go | 88.88% <0.00%> (-11.12%) |
:arrow_down: |
| jwk/handler.go | 67.08% <40.00%> (+2.64%) |
:arrow_up: |
| persistence/sql/persister_jwk.go | 76.33% <61.11%> (-2.43%) |
:arrow_down: |
| cmd/server/helper_cert.go | 13.95% <0.00%> (-12.14%) |
:arrow_down: |
| cmd/cliclient/client.go | 37.50% <0.00%> (-5.36%) |
:arrow_down: |
| cmd/cmd_create_jwks.go | 77.77% <0.00%> (-2.23%) |
:arrow_down: |
| cmd/root.go | 92.72% <0.00%> (-1.40%) |
:arrow_down: |
| ... and 119 more |
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.
- Cache invalidation across multiple nodes is unsolved (what happenes to the cache on container A when you rotate the key on container B?)
Right this is a problem in current implementation.
- Cache key collision is possible because the cache key is the set id which is not guaranteed to be unique for two distinct keys
Yes keySetCache contains all keys with set id. When any of these keys are deleted by DeleteKey or DeleteKeySet, then the cache key is evicted
- No locking mechanism for concurrent access on the cache map
Locking is implemented using RWMutex on methods.
- No limit on cache size and thus memory consumption
As this feature was intended for GetWellKnownKeys only I assumed there is reasonable amount of keys.
Before introducing caching in Ory Hydra, I would recommend to write a design document outlining the challenges and approaches of how to solve caching for this component at scale. We are generally very tough on merging features that introduce caches, because caches have been the no 1 offender when it comes to security CVEs at Ory. It is quite likely that a cache feature such as this one will never be merged to master.
Thanks even caching was intended for GetWellKnownKeys only im not sure if we will pursue implementing it then. We initially thought caching well known keys would be useful for deployments using cloud HSM where each operation costs.