Skip to content
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.

Sign up for GitHub

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

refactor: jwk aquire lock when generating #3865

Closed
wants to merge 4 commits into from
Closed

refactor: jwk aquire lock when generating #3865

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
c0f8901
refactor(jwk): aquire read lock unless generating
awill1988 Oct 28, 2024
d1101c7
rm lock
awill1988 Oct 28, 2024
a353c38
lock for persisting
awill1988 Oct 28, 2024
ee7a2ce
restore map lock but acquire once
awill1988 Oct 29, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 10 additions & 3 deletions jwk/helper.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,12 +44,16 @@ func EnsureAsymmetricKeypairExists(ctx context.Context, r InternalRegistry, alg,
}

func GetOrGenerateKeys(ctx context.Context, r InternalRegistry, m Manager, set, kid, alg string) (private *jose.JSONWebKey, err error) {
getLock(set).Lock()
defer getLock(set).Unlock()

getLock(set).RLock()
Copy link
Member

@aeneasr aeneasr Oct 28, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that's a good point..

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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?

Copy link
Contributor

@terev terev Oct 28, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

keys, err := m.GetKeySet(ctx, set)
getLock(set).RUnlock()

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)

getLock(set).Lock()
defer getLock(set).Unlock()

keys, err = m.GenerateAndPersistKeySet(ctx, set, kid, alg, "sig")
if err != nil {
return nil, err
Expand All @@ -64,6 +68,9 @@ func GetOrGenerateKeys(ctx context.Context, r InternalRegistry, m Manager, set,
} else {
r.Logger().WithField("jwks", set).Warnf("JSON Web Key not found in JSON Web Key Set %s, generating new key pair...", set)

getLock(set).Lock()
defer getLock(set).Unlock()

keys, err = m.GenerateAndPersistKeySet(ctx, set, kid, alg, "sig")
if err != nil {
return nil, err
Expand Down
Loading