hydra icon indicating copy to clipboard operation
hydra copied to clipboard

feat: add /.well-known/jwks.json caching for HSM key manager

Open aarmam opened this issue 3 years ago • 1 comments

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.

aarmam avatar Sep 16 '22 09:09 aarmam

Codecov Report

Merging #3259 (37dce4c) into master (4eb13c9) will decrease coverage by 0.06%. The diff coverage is 32.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.

codecov[bot] avatar Sep 16 '22 10:09 codecov[bot]

  1. 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.

  1. 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

  1. No locking mechanism for concurrent access on the cache map

Locking is implemented using RWMutex on methods.

  1. 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.

aarmam avatar Dec 27 '22 12:12 aarmam