ROX-33256: add console user to requester claim#19683
ROX-33256: add console user to requester claim#19683
Conversation
|
Skipping CI for Draft Pull Request. |
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- The
tokenCacheKey.cacheKeyStringimplementation (scope + "|" + username) can collide if either field contains"|"; consider using a delimiter that cannot appear in either value (e.g.,"\x00") or a more robust encoding (e.g., JSON orfmt.Sprintf("%q|%q", ...)) to ensure unique coalescer keys.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `tokenCacheKey.cacheKeyString` implementation (`scope + "|" + username`) can collide if either field contains `"|"`; consider using a delimiter that cannot appear in either value (e.g., `"\x00"`) or a more robust encoding (e.g., JSON or `fmt.Sprintf("%q|%q", ...)`) to ensure unique coalescer keys.
## Individual Comments
### Comment 1
<location path="sensor/common/centralproxy/transport.go" line_range="186-187" />
<code_context>
+}
+
+// cacheKeyString returns a string representation for coalescer keys.
+func (k tokenCacheKey) cacheKeyString() string {
+ return k.scope + "|" + k.username
+}
+
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Consider making the coalescer key construction robust against delimiter collisions or future changes in username format.
The current `scope + "|" + username` key relies on `"|"` never appearing in `username`, but this isn’t enforced. If that assumption changes, distinct `(scope, username)` pairs could collide in the coalescer. To avoid this, either pick a delimiter that’s guaranteed invalid for both fields, encode each component before joining (e.g., URL-escape/base64), or use a structured key type in the coalescer instead of flattening to a string.
Suggested implementation:
```golang
// cacheKeyString returns a string representation for coalescer keys.
// We URL-escape each component to ensure the "|" delimiter cannot be
// produced by the encoded values, avoiding collisions even if scope
// or username formats change in the future.
func (k tokenCacheKey) cacheKeyString() string {
return url.QueryEscape(k.scope) + "|" + url.QueryEscape(k.username)
}
// Buffer the request body upfront so we can replay it on retry.
var bodyBytes []byte
req.Body = io.NopCloser(bytes.NewBuffer(bodyBytes))
```
To compile successfully, ensure that `net/url` is imported at the top of `sensor/common/centralproxy/transport.go`:
1. Add `net/url` to the existing import block, for example:
- From:
`import ( "bytes" "io" ... )`
- To:
`import ( "bytes" "io" "net/url" ... )`
No other changes should be required, assuming `cacheKeyString()` is only used as a string key and not relied upon to be human-readable.
</issue_to_address>
### Comment 2
<location path="sensor/common/centralproxy/transport.go" line_range="250" />
<code_context>
}
- log.Debugf("Token cache miss for namespace scope %q, requesting from Central", namespaceScope)
+ log.Debugf("Token cache miss for namespace scope %q (user %q), requesting from Central", key.scope, key.username)
// Use a background context with timeout to ensure the shared function is independent
</code_context>
<issue_to_address>
**🚨 suggestion (security):** Revisit logging of the raw username in debug logs to avoid leaking potentially sensitive identifiers.
This now logs the console username for every cache miss, which may end up in long‑retention aggregated logs. To reduce privacy/compliance risk, consider hashing or redacting `username`, or omitting it and logging only the scope or a non-identifying token for correlation.
```suggestion
log.Debugf("Token cache miss for namespace scope %q, requesting from Central", key.scope)
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| func (k tokenCacheKey) cacheKeyString() string { | ||
| return k.scope + "|" + k.username |
There was a problem hiding this comment.
suggestion (bug_risk): Consider making the coalescer key construction robust against delimiter collisions or future changes in username format.
The current scope + "|" + username key relies on "|" never appearing in username, but this isn’t enforced. If that assumption changes, distinct (scope, username) pairs could collide in the coalescer. To avoid this, either pick a delimiter that’s guaranteed invalid for both fields, encode each component before joining (e.g., URL-escape/base64), or use a structured key type in the coalescer instead of flattening to a string.
Suggested implementation:
// cacheKeyString returns a string representation for coalescer keys.
// We URL-escape each component to ensure the "|" delimiter cannot be
// produced by the encoded values, avoiding collisions even if scope
// or username formats change in the future.
func (k tokenCacheKey) cacheKeyString() string {
return url.QueryEscape(k.scope) + "|" + url.QueryEscape(k.username)
}
// Buffer the request body upfront so we can replay it on retry.
var bodyBytes []byte
req.Body = io.NopCloser(bytes.NewBuffer(bodyBytes))To compile successfully, ensure that net/url is imported at the top of sensor/common/centralproxy/transport.go:
- Add
net/urlto the existing import block, for example:- From:
import ( "bytes" "io" ... ) - To:
import ( "bytes" "io" "net/url" ... )
- From:
No other changes should be required, assuming cacheKeyString() is only used as a string key and not relied upon to be human-readable.
| } | ||
|
|
||
| log.Debugf("Token cache miss for namespace scope %q, requesting from Central", namespaceScope) | ||
| log.Debugf("Token cache miss for namespace scope %q (user %q), requesting from Central", key.scope, key.username) |
There was a problem hiding this comment.
🚨 suggestion (security): Revisit logging of the raw username in debug logs to avoid leaking potentially sensitive identifiers.
This now logs the console username for every cache miss, which may end up in long‑retention aggregated logs. To reduce privacy/compliance risk, consider hashing or redacting username, or omitting it and logging only the scope or a non-identifying token for correlation.
| log.Debugf("Token cache miss for namespace scope %q (user %q), requesting from Central", key.scope, key.username) | |
| log.Debugf("Token cache miss for namespace scope %q, requesting from Central", key.scope) |
|
Images are ready for the commit at 56e55bd. To use with deploy scripts, first |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #19683 +/- ##
==========================================
+ Coverage 49.38% 49.65% +0.27%
==========================================
Files 2743 2747 +4
Lines 207037 207278 +241
==========================================
+ Hits 102236 102921 +685
+ Misses 97218 96701 -517
- Partials 7583 7656 +73
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
e105ffb to
ff09221
Compare
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey - I've found 1 issue
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="sensor/common/centralproxy/transport_test.go" line_range="434-443" />
<code_context>
+
+ provider := newTestTokenProvider(fakeClient, "test-cluster-id")
+
+ keyA := tokenCacheKey{scope: "shared-scope", username: "user-a"}
+ keyB := tokenCacheKey{scope: "shared-scope", username: "user-b"}
+
+ // Request for user-a.
+ tokenA, err := provider.getTokenForScope(context.Background(), keyA)
+ require.NoError(t, err)
+ assert.Equal(t, "token-user-a", tokenA)
+
+ // Request for user-b (same scope, different user, should get new token).
+ tokenB, err := provider.getTokenForScope(context.Background(), keyB)
+ require.NoError(t, err)
+ assert.Equal(t, "token-user-b", tokenB)
+
+ // Request for user-a again (should use cached).
+ tokenACached, err := provider.getTokenForScope(context.Background(), keyA)
+ require.NoError(t, err)
</code_context>
<issue_to_address>
**suggestion (testing):** Consider adding an invalidateToken test that differentiates between users with the same scope.
To fully validate the new per-user invalidation in `invalidateToken(tokenCacheKey)`, please add a corresponding test in `TestTokenProvider_InvalidateToken` that:
- Caches tokens for two keys with the same scope but different `username` values.
- Calls `invalidateToken` for only one key.
- Asserts that the invalidated user triggers a new RPC on the next call, while the other user still uses the cached token.
This complements the existing "invalidate only specific scope" test by covering the user dimension as well.
Suggested implementation:
```golang
require.NotNil(t, resp)
assert.Equal(t, "console-admin", fakeClient.lastRequest.GetRequester())
})
}
// Verifies that invalidating a token for one user in a shared scope does not
// affect other users with the same scope, and that only the invalidated user
// triggers a new RPC.
func TestTokenProvider_InvalidateToken_DifferentUsersSameScope(t *testing.T) {
ctx := context.Background()
tokenIndex := 0
tokens := []string{
"token-user-a-1", // first token for user-a
"token-user-b-1", // first token for user-b
"token-user-a-2", // refreshed token for user-a after invalidation
}
fakeClient := &dynamicFakeTokenServiceClient{
getToken: func() string {
token := tokens[tokenIndex]
tokenIndex++
return token
},
}
provider := newTestTokenProvider(fakeClient, "test-cluster-id")
keyA := tokenCacheKey{scope: "shared-scope", username: "user-a"}
keyB := tokenCacheKey{scope: "shared-scope", username: "user-b"}
// Initial requests populate the cache for both users.
tokenA1, err := provider.getTokenForScope(ctx, keyA)
require.NoError(t, err)
assert.Equal(t, "token-user-a-1", tokenA1)
tokenB1, err := provider.getTokenForScope(ctx, keyB)
require.NoError(t, err)
assert.Equal(t, "token-user-b-1", tokenB1)
// Invalidate only user-a's token.
provider.invalidateToken(keyA)
// Next request for user-a should trigger a new RPC and return a new token.
tokenA2, err := provider.getTokenForScope(ctx, keyA)
require.NoError(t, err)
assert.Equal(t, "token-user-a-2", tokenA2)
// User-b should still use the cached token and not trigger a new RPC.
tokenB2, err := provider.getTokenForScope(ctx, keyB)
require.NoError(t, err)
assert.Equal(t, "token-user-b-1", tokenB2)
// We expect exactly three RPCs: initial A, initial B, refreshed A.
assert.Equal(t, 3, tokenIndex)
}
t.Run("different users with same scope get separate cache entries", func(t *testing.T) {
tokenIndex := 0
tokens := []string{"token-user-a", "token-user-b"}
fakeClient := &dynamicFakeTokenServiceClient{
getToken: func() string {
```
If `context` is not already imported in this file, add:
```go
import "context"
```
to the import block. The new test assumes `newTestTokenProvider`, `dynamicFakeTokenServiceClient`, `tokenCacheKey`, `require`, and `assert` are already defined and imported as in the existing tests. Adjust the token strings or assertion style if your existing tests follow a different naming or assertion convention.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
ff09221 to
56e55bd
Compare
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Central YAML (base), Organization UI (inherited) Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdded console username context propagation through request handling and token caching layers. Introduces context key helpers in handler to attach authenticated username, derives username in transport's RoundTrip method, uses composite cache keys combining namespace scope and username for token caching, and includes requester field in token generation requests to Central. Changes
Sequence DiagramsequenceDiagram
participant Client
participant Handler
participant Transport
participant TokenProvider
participant Central
Client->>Handler: HTTP Request with Bearer Token
Handler->>Handler: Authenticate & Extract Username
Handler->>Handler: Create Context with Username
Handler->>Transport: ServeHTTP with Updated Context
Transport->>Transport: Derive Username from Context
Transport->>Transport: Build Composite Cache Key<br/>(Namespace + Username)
Transport->>TokenProvider: Check Token Cache<br/>for Composite Key
alt Cache Hit
TokenProvider-->>Transport: Return Cached Token
else Cache Miss
Transport->>TokenProvider: Request Token with Username
TokenProvider->>Central: GenerateTokenForPermissionsAndScopeRequest<br/>(Requester: username)
Central-->>TokenProvider: Token Response
TokenProvider->>TokenProvider: Store in Cache<br/>(Composite Key)
TokenProvider-->>Transport: Return New Token
end
Transport->>Transport: Strip Internal Headers
Transport->>Central: Forward Request with Token
Central-->>Transport: Response
Transport-->>Client: Response
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes 🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Description
change me!
User-facing documentation
Testing and quality
Automated testing
How I validated my change
change me!