-
Notifications
You must be signed in to change notification settings - Fork 10
fix(control-plane): stop Keycloak event storms #230
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
Changes from all commits
9ac4354
6ab0e62
074915f
8dc6a76
ba52838
2d0ebfd
454629e
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 |
|---|---|---|
|
|
@@ -88,7 +88,10 @@ func (tp *TokenProvider) Token() (string, error) { | |
|
|
||
| tp.token = token | ||
| // Refresh at 80% of TTL to avoid using an expired token. | ||
| tp.expiry = time.Now().Add(time.Duration(float64(expiresIn) * 0.8)) | ||
| ttl := time.Duration(expiresIn) * time.Second | ||
| refreshAfter := ttl * 8 / 10 | ||
| tp.expiry = time.Now().Add(refreshAfter) | ||
| log.Printf("INFO got OIDC access token for client %q; refresh in %s", tp.clientID, refreshAfter) | ||
|
Collaborator
Author
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. Minor (observability / security). The grant log records only
Collaborator
Author
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. Root-cause fix confirmed. The previous |
||
|
|
||
| return tp.token, nil | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,169 @@ | ||
| package auth | ||
|
|
||
| import ( | ||
| "bytes" | ||
| "encoding/json" | ||
| "fmt" | ||
| "log" | ||
| "net/http" | ||
| "net/http/httptest" | ||
| "strings" | ||
| "sync/atomic" | ||
| "testing" | ||
| "time" | ||
| ) | ||
|
|
||
| func TestTokenGrantLogContainsSafeRefreshDetails(t *testing.T) { | ||
| var output bytes.Buffer | ||
| priorWriter := log.Writer() | ||
| priorFlags := log.Flags() | ||
| priorPrefix := log.Prefix() | ||
| log.SetOutput(&output) | ||
| log.SetFlags(0) | ||
| log.SetPrefix("") | ||
| t.Cleanup(func() { | ||
| log.SetOutput(priorWriter) | ||
| log.SetFlags(priorFlags) | ||
| log.SetPrefix(priorPrefix) | ||
| }) | ||
|
|
||
| var grants atomic.Int32 | ||
| server := newTokenServer(t, &grants) | ||
| provider := NewTokenProvider("https://issuer.invalid", "client-id\nforged-entry", "client-secret") | ||
| provider.SetTokenEndpoint(server.URL) | ||
|
|
||
| if _, err := provider.Token(); err != nil { | ||
| t.Fatalf("Token() failed: %v", err) | ||
| } | ||
|
|
||
| message := output.String() | ||
| if !strings.Contains(message, `client "client-id\nforged-entry"; refresh in 4m0s`) { | ||
| t.Fatalf("token grant log = %q, want quoted client ID and refresh interval", message) | ||
| } | ||
| for _, forbidden := range []string{"token-1", "client-secret", "\nforged-entry"} { | ||
| if strings.Contains(message, forbidden) { | ||
| t.Fatalf("token grant log contains unsafe value %q: %q", forbidden, message) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| func TestTokenReusesCachedToken(t *testing.T) { | ||
| t.Parallel() | ||
|
|
||
| var grants atomic.Int32 | ||
| server := newTokenServer(t, &grants) | ||
|
|
||
| provider := NewTokenProvider("https://issuer.invalid", "client-id", "client-secret") | ||
| provider.SetTokenEndpoint(server.URL) | ||
|
|
||
| first, err := provider.Token() | ||
| if err != nil { | ||
| t.Fatalf("first Token() call failed: %v", err) | ||
| } | ||
| second, err := provider.Token() | ||
| if err != nil { | ||
| t.Fatalf("second Token() call failed: %v", err) | ||
| } | ||
|
|
||
| if first != second { | ||
| t.Fatalf("Token() returned %q and %q; both calls must return the same token", first, second) | ||
| } | ||
| if got := grants.Load(); got != 1 { | ||
| t.Fatalf("token endpoint received %d grants; it must receive 1", got) | ||
| } | ||
| } | ||
|
|
||
| func TestConcurrentTokenCallsShareCachedToken(t *testing.T) { | ||
| t.Parallel() | ||
|
|
||
| var grants atomic.Int32 | ||
| server := newTokenServer(t, &grants) | ||
|
|
||
| provider := NewTokenProvider("https://issuer.invalid", "client-id", "client-secret") | ||
| provider.SetTokenEndpoint(server.URL) | ||
|
|
||
| const callCount = 16 | ||
| type result struct { | ||
| token string | ||
| err error | ||
| } | ||
|
|
||
| start := make(chan struct{}) | ||
| results := make(chan result, callCount) | ||
| for range callCount { | ||
| go func() { | ||
| <-start | ||
| token, err := provider.Token() | ||
| results <- result{token: token, err: err} | ||
| }() | ||
| } | ||
| close(start) | ||
|
|
||
| for range callCount { | ||
| result := <-results | ||
| if result.err != nil { | ||
| t.Fatalf("Token() failed: %v", result.err) | ||
| } | ||
| if result.token != "token-1" { | ||
| t.Fatalf("Token() returned %q; it must return %q", result.token, "token-1") | ||
| } | ||
| } | ||
|
|
||
| if got := grants.Load(); got != 1 { | ||
| t.Fatalf("token endpoint received %d grants; it must receive 1", got) | ||
| } | ||
| } | ||
|
|
||
| func TestTokenRefreshesAfterThreshold(t *testing.T) { | ||
| t.Parallel() | ||
|
|
||
| var grants atomic.Int32 | ||
| server := newTokenServer(t, &grants) | ||
|
|
||
| provider := NewTokenProvider("https://issuer.invalid", "client-id", "client-secret") | ||
| provider.SetTokenEndpoint(server.URL) | ||
|
|
||
| requestStarted := time.Now() | ||
| first, err := provider.Token() | ||
| requestFinished := time.Now() | ||
| if err != nil { | ||
| t.Fatalf("first Token() call failed: %v", err) | ||
| } | ||
|
|
||
| provider.mu.Lock() | ||
| refreshAt := provider.expiry | ||
| provider.expiry = time.Now().Add(-time.Second) | ||
| provider.mu.Unlock() | ||
|
|
||
| const refreshDelay = 4 * time.Minute | ||
| if refreshAt.Before(requestStarted.Add(refreshDelay)) || refreshAt.After(requestFinished.Add(refreshDelay)) { | ||
| t.Fatalf("refresh time = %v; want 80 percent of a 300-second lifetime", refreshAt) | ||
| } | ||
|
|
||
| second, err := provider.Token() | ||
| if err != nil { | ||
| t.Fatalf("second Token() call failed: %v", err) | ||
| } | ||
| if first != "token-1" || second != "token-2" { | ||
| t.Fatalf("Token() returned %q and %q; want %q and %q", first, second, "token-1", "token-2") | ||
| } | ||
| if got := grants.Load(); got != 2 { | ||
| t.Fatalf("token endpoint received %d grants; it must receive 2", got) | ||
| } | ||
| } | ||
|
|
||
| func newTokenServer(t *testing.T, grants *atomic.Int32) *httptest.Server { | ||
| t.Helper() | ||
|
|
||
| server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { | ||
| grant := grants.Add(1) | ||
| w.Header().Set("Content-Type", "application/json") | ||
| _ = json.NewEncoder(w).Encode(tokenResponse{ | ||
| AccessToken: fmt.Sprintf("token-%d", grant), | ||
| ExpiresIn: 300, | ||
| TokenType: "Bearer", | ||
| }) | ||
| })) | ||
| t.Cleanup(server.Close) | ||
| return server | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -31,6 +31,7 @@ const ( | |
| clientRefreshTokenAttribute = "client_credentials.use_refresh_token" | ||
| deviceGrantAttribute = "oauth2.device.authorization.grant.enabled" | ||
| cibaGrantAttribute = "oidc.ciba.grant.enabled" | ||
| builtInServiceAccountScope = "service_account" | ||
| defaultAccessTokenLifetimeSecs = 300 | ||
| ) | ||
|
|
||
|
|
@@ -276,7 +277,7 @@ func (c *Client) reconcileConverged(ctx context.Context, spec ServiceAccountSpec | |
| return false, nil | ||
| } | ||
| if len(client.RedirectURIs) > 0 || len(client.WebOrigins) > 0 || | ||
| len(client.DefaultClientScopes) > 0 || len(client.OptionalClientScopes) > 0 { | ||
| !defaultClientScopesConverged(client.DefaultClientScopes) || len(client.OptionalClientScopes) > 0 { | ||
| return false, nil | ||
| } | ||
| lifetime := spec.AccessTokenLifetimeSeconds | ||
|
|
@@ -305,6 +306,13 @@ func (c *Client) reconcileConverged(ctx context.Context, spec ServiceAccountSpec | |
| return c.protocolMappersConverged(ctx, client.ID, spec.GatewayClientID) | ||
| } | ||
|
|
||
| // defaultClientScopesConverged accepts the built-in scope that Keycloak adds | ||
| // when service accounts are enabled. The repair payload stays empty because | ||
| // Keycloak owns this scope. All other scopes are drift. | ||
| func defaultClientScopesConverged(scopes []string) bool { | ||
|
Collaborator
Author
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. The predicate loosening is safe and self-terminating: |
||
| return len(scopes) == 0 || (len(scopes) == 1 && scopes[0] == builtInServiceAccountScope) | ||
|
Collaborator
Author
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. Good fail-closed predicate: only an empty list or exactly |
||
| } | ||
|
|
||
| // roleMappingSet is the shape Keycloak returns for both user role-mappings and | ||
| // client scope-mappings. | ||
| type roleMappingSet struct { | ||
|
|
||
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.
Root cause confirmed and correctly fixed. The prior
time.Duration(float64(expiresIn) * 0.8)treatedexpiresIn(a value in seconds) as nanoseconds, so a 300s token was cached for ~240ns and effectively everyToken()call re-ran the client_credentials grant — that is the KeycloakCLIENT_LOGINevent storm.time.Duration(expiresIn) * time.Secondrestores the intended unit and the 80% threshold. Confidence: High.The integer form
ttl * 8 / 10avoids float rounding and cannot overflow for any realisticexpires_in(max 900s), so this is a clean choice.