-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
keyring: fixes for keyring replication on cluster join #14987
Changes from all commits
958fab4
e8a0ec1
1c31df7
45f7352
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -155,12 +155,33 @@ const keyIDHeader = "kid" | |
// SignClaims signs the identity claim for the task and returns an | ||
// encoded JWT with both the claim and its signature | ||
func (e *Encrypter) SignClaims(claim *structs.IdentityClaims) (string, error) { | ||
e.lock.RLock() | ||
defer e.lock.RUnlock() | ||
|
||
keyset, err := e.activeKeySetLocked() | ||
getActiveKeyset := func() (*keyset, error) { | ||
e.lock.RLock() | ||
defer e.lock.RUnlock() | ||
keyset, err := e.activeKeySetLocked() | ||
return keyset, err | ||
} | ||
|
||
// If a key is rotated immediately following a leader election, plans that | ||
// are in-flight may get signed before the new leader has the key. Allow for | ||
// a short timeout-and-retry to avoid rejecting plans | ||
keyset, err := getActiveKeyset() | ||
if err != nil { | ||
return "", err | ||
ctx, cancel := context.WithTimeout(e.srv.shutdownCtx, 5*time.Second) | ||
defer cancel() | ||
for { | ||
select { | ||
case <-ctx.Done(): | ||
return "", err | ||
default: | ||
time.Sleep(50 * time.Millisecond) | ||
keyset, err = getActiveKeyset() | ||
if keyset != nil { | ||
break | ||
} | ||
} | ||
} | ||
} | ||
|
||
token := jwt.NewWithClaims(&jwt.SigningMethodEd25519{}, claim) | ||
|
@@ -435,7 +456,10 @@ START: | |
return | ||
default: | ||
// Rate limit how often we attempt replication | ||
limiter.Wait(ctx) | ||
err := limiter.Wait(ctx) | ||
if err != nil { | ||
goto ERR_WAIT // rate limit exceeded | ||
} | ||
Comment on lines
458
to
+462
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Seems like if we have our own blocking mechanism, we should be using https://pkg.go.dev/golang.org/x/time/rate#Limiter edit: actually no, There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah this API is weirdly hard to use correctly and the error case is only for exceeding the burst. I first thought we probably wanted to use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I misread the API again 😊 . Only when the rate is set to |
||
|
||
ws := store.NewWatchSet() | ||
iter, err := store.RootKeyMetas(ws) | ||
|
@@ -461,7 +485,8 @@ START: | |
getReq := &structs.KeyringGetRootKeyRequest{ | ||
KeyID: keyID, | ||
QueryOptions: structs.QueryOptions{ | ||
Region: krr.srv.config.Region, | ||
Region: krr.srv.config.Region, | ||
MinQueryIndex: keyMeta.ModifyIndex - 1, | ||
}, | ||
} | ||
getResp := &structs.KeyringGetRootKeyResponse{} | ||
|
@@ -479,7 +504,7 @@ START: | |
getReq.AllowStale = true | ||
for _, peer := range krr.getAllPeers() { | ||
err = krr.srv.forwardServer(peer, "Keyring.Get", getReq, getResp) | ||
if err == nil { | ||
if err == nil && getResp.Key != nil { | ||
break | ||
} | ||
} | ||
|
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.
Sorry for the after-the-fact drive-by review:
Looks like we could replace
activeKeySetLocked
with this func as the only other place the key is read isEncrypt
and it similarly only needs the lock for the duration of fetching the key.The fact neither reader of this field need the lock other than for this field signals to me that the lock should be encapsulated in the getter and not the caller's concern.
Not a big deal though.
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.
That's a good catch.
activeKeySetLocked
ends up holding onto the lock longer than it needs to as well -- it doesn't need to hold the lock while querying the state store. I'll open another quickie PR for that.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.
Follow-up PR: #15026