refactor: jwk aquire lock when generating #3865
Conversation
awill1988
changed the title
jwk/helper.go
Outdated
| getLock(set).Lock() | ||
| defer getLock(set).Unlock() | ||
|
|
||
| getLock(set).RLock() |
There was a problem hiding this comment.
I'm wondering why the lock is there in the first place. We're using a SQL backend for fetching the keys, and that's atomic.
There was a problem hiding this comment.
that's a good point..
There was a problem hiding this comment.
My guess is that the intention was to protect the initialization so that it's only done once within a single instance. Otherwise if there's more than one concurrent caller to this function, and each read the uninitialized state, they'd each generate and persist their own key.
There was a problem hiding this comment.
@terev if it were to keep the map[string]*RWMutex or maybe just map[string]*Mutex and lock on key generation, do you think that would work for the key set initialization condition?
There was a problem hiding this comment.
Yeah I think a plain mutex per keyset is fine. But maybe keep the rwlock for accessing the map. But actually use the read lock in the right case there. But to be clear I'm not part of ory so if aeneasr disagrees that's cool too.
There was a problem hiding this comment.
But to be clear I'm not part of ory so if aeneasr disagrees that's cool too.
thanks anyways @terev 😄!
Okay, @aeneasr, I think I'm following what you're saying about not even needing the read lock.
I'll read up on the test/conformance to grok what's going on there 🤞🏻
Also found a couple snapshots that looked like they needed a quick patch so I grabbed those just in case.
awill1988
changed the title
| if errors.Is(err, x.ErrNotFound) || keys != nil && len(keys.Keys) == 0 { | ||
| r.Logger().Warnf("JSON Web Key Set \"%s\" does not exist yet, generating new key pair...", set) | ||
|
|
||
| l := getLock(set) | ||
| defer l.Lock() |
There was a problem hiding this comment.
In case you missed it, I think you meant to lock the returned lock then defer unlock. Same thing below on line 69.
At the end of this if block it may make sense to directly return FindPrivateKey(keys). If we don't it opens up the potential for deadlock if FindPrivateKey fails and we try to generate another keyset. Also I don't really think it would help to generate another keyset if the one we just generated didn't have a private key.
|
This whole code is unsafe in the presence of multiple hydra instances. We would need to change the database code to do an |
While this is true, it doesn't really matter, because the whole thing will eventually settle. Even if we created 20 keys, it will be like if we rotated through 20 keys and just use the newest key. So the impact is really minimal in my view |
|
That is:
Postgres and other DBs which support |
|
I think that analysis is correct. So all we need really is a lock for when we generate the key and not for reading, which also solves the issue here |
Update the locking behavior of
jwk.GetOrGenerateKeysto acquire a read lock to fetch keys and only acquire the write lock when generating and persisting new keys.Related issue(s)
#3863
Checklist
introduces a new feature.
contributing code guidelines.
vulnerability. If this pull request addresses a security vulnerability, I
confirm that I got the approval (please contact
[email protected]) from the maintainers to push
the changes.
works.
Further Comments
I can attempt to write up more unit tests if that is required, but since this is not a significant change I am wondering if the existing tests can cover it?
I am also not sure what would need to be written for documentation but I am happy to add it where it is preferred.
On the related issue there are some reproducible steps that can be used to demonstrate the benefit of this refactor, but I can specify steps or update here if needed.