fix(backend): API keys access user integrations via SA annotation#901
Merged
Gkrumbach07 merged 3 commits intomainfrom Mar 13, 2026
Merged
fix(backend): API keys access user integrations via SA annotation#901Gkrumbach07 merged 3 commits intomainfrom
Gkrumbach07 merged 3 commits intomainfrom
Conversation
API keys (ServiceAccount JWTs) couldn't access user integrations because userID was only read from OAuth proxy headers. Now store creating user's ID as SA annotation and read it in middleware fallback. Fixes: RHOAIENG-52967 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
WalkthroughRequires an authenticated user ID when creating a project key and records it on the created ServiceAccount; adds middleware fallback to recover that user ID from a ServiceAccount annotation via Kubernetes TokenReview / ServiceAccount lookup when the context lacks it. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Middleware as forwardedIdentityMiddleware
participant K8s as Kubernetes API
participant Handler
Client->>Middleware: HTTP request (may include Authorization / X-Remote-User)
Middleware->>Middleware: Check context for userID
alt userID missing
Middleware->>K8s: TokenReview (validate Bearer token)
K8s-->>Middleware: TokenReview result (authenticated, user info)
Middleware->>K8s: Get ServiceAccount (namespace, name)
K8s-->>Middleware: ServiceAccount
Middleware->>Middleware: Read ambient-code.io/created-by-user-id annotation
Middleware->>Middleware: Set userID in context
end
Middleware->>Handler: Forward request (with userID)
Handler-->>Client: Response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@components/backend/handlers/permissions.go`:
- Around line 393-397: The code currently writes an empty
"ambient-code.io/created-by-user-id" annotation using c.GetString("userID");
change the API key creation flow to reject requests when the creator user ID is
unresolved: read userID := c.GetString("userID") before building annotations and
if userID == "" return an HTTP error (e.g. 400/422) with a clear message instead
of proceeding; only set "ambient-code.io/created-by-user-id" in the annotation
map when userID is non-empty. Ensure this check is added in the function
handling the key creation (the code that uses req.Name, req.Description, role
and the annotations map) so no API key is persisted with an empty
created-by-user-id.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 73b8995d-cb57-4578-ac0e-188730107f5c
📒 Files selected for processing (2)
components/backend/handlers/permissions.gocomponents/backend/server/server.go
…ver package Move the canonical implementation to server.ExtractServiceAccountFromAuth (where it is needed by forwardedIdentityMiddleware) and keep a thin forwarding wrapper in handlers for backward compatibility. This eliminates the duplicate extractServiceAccountFromBearer that was introduced in the PR. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
… key creation Replace insecure raw JWT parsing with K8s TokenReview API to verify the bearer token before trusting the ServiceAccount identity. This prevents forged tokens from spoofing SA annotations. Also reject key creation when userID is empty to avoid persisting keys that can never resolve integration credentials. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@components/backend/handlers/middleware.go`:
- Around line 266-269: The compatibility wrapper ExtractServiceAccountFromAuth
should continue to handle bearer-token API-key serviceAccount extraction instead
of delegating entirely to server.ExtractServiceAccountFromAuth; restore or
reimplement the prior bearer-token logic here by first calling
server.ExtractServiceAccountFromAuth to handle X-Remote-User, and if that
returns empty/false then perform the existing bearer-token extraction (the same
path used by getUserSubjectFromContext) to reconstruct the
"system:serviceaccount:<ns>:<name>" subject; update
ExtractServiceAccountFromAuth to return the correct subject and boolean so
callers in this package continue to receive service account subjects.
In `@components/backend/server/server.go`:
- Around line 169-176: The code currently trusts the ServiceAccount annotation
"ambient-code.io/created-by-user-id" and sets c.Set("userID", ...) directly (in
the block using K8sClient and resolveServiceAccountFromToken), which allows
caller-controlled SA annotations to impersonate users; change this by requiring
a platform-managed marker on the ServiceAccount before accepting its created-by
annotation (e.g. check for a trusted annotation/label such as an
"ambient-code.io/platform-managed" marker or other server-controlled flag on
sa.Annotations/sa.Labels), and if that marker is absent do not set
c.Set("userID") from the SA annotation — instead record the mapping in
server-owned/tamper-evident state (or reject/ignore the annotation) and only
populate c.Set("userID") after verifying the SA is platform-managed or after
consulting the server-owned mapping.
- Around line 188-193: The Authorization header parsing in server.go only checks
c.GetHeader("Authorization") (rawAuth) and thus misses query-string ?token=
values used by websocket/agent callers; update the logic in the same function
that computes token (using rawAuth, parts, token) to fallback to the query param
(e.g., c.Query("token") or r.URL.Query().Get("token")) when rawAuth is empty or
parts length != 2, so token gets populated from the ?token= value (this keeps
behavior consistent with ValidateProjectContext and
forwardedIdentityMiddleware); ensure you still trim/validate the token string
before returning.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 28d31053-6221-43f3-b6f8-909d213fe650
📒 Files selected for processing (3)
components/backend/handlers/middleware.gocomponents/backend/handlers/permissions.gocomponents/backend/server/server.go
Comment on lines
+266
to
+269
| // ExtractServiceAccountFromAuth delegates to server.ExtractServiceAccountFromAuth. | ||
| // Kept as a forwarding function for backward compatibility with callers in this package. | ||
| func ExtractServiceAccountFromAuth(c *gin.Context) (string, string, bool) { | ||
| // Check X-Remote-User header (OpenShift OAuth proxy format) | ||
| // This is a production feature, not just for tests | ||
| remoteUser := c.GetHeader("X-Remote-User") | ||
| if remoteUser != "" { | ||
| const prefix = "system:serviceaccount:" | ||
| if strings.HasPrefix(remoteUser, prefix) { | ||
| rest := strings.TrimPrefix(remoteUser, prefix) | ||
| parts := strings.SplitN(rest, ":", 2) | ||
| if len(parts) == 2 { | ||
| return parts[0], parts[1], true | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Standard Authorization Bearer JWT parsing | ||
| rawAuth := c.GetHeader("Authorization") | ||
| parts := strings.SplitN(rawAuth, " ", 2) | ||
| if len(parts) != 2 || !strings.EqualFold(parts[0], "Bearer") { | ||
| return "", "", false | ||
| } | ||
| token := strings.TrimSpace(parts[1]) | ||
| if token == "" { | ||
| return "", "", false | ||
| } | ||
| segs := strings.Split(token, ".") | ||
| if len(segs) < 2 { | ||
| return "", "", false | ||
| } | ||
| payloadB64 := segs[1] | ||
| if m := len(payloadB64) % 4; m != 0 { | ||
| payloadB64 += strings.Repeat("=", 4-m) | ||
| } | ||
| data, err := base64.URLEncoding.DecodeString(payloadB64) | ||
| if err != nil { | ||
| return "", "", false | ||
| } | ||
| var payload map[string]interface{} | ||
| if err := json.Unmarshal(data, &payload); err != nil { | ||
| return "", "", false | ||
| } | ||
| sub, _ := payload["sub"].(string) | ||
| const prefix = "system:serviceaccount:" | ||
| if !strings.HasPrefix(sub, prefix) { | ||
| return "", "", false | ||
| } | ||
| rest := strings.TrimPrefix(sub, prefix) | ||
| parts2 := strings.SplitN(rest, ":", 2) | ||
| if len(parts2) != 2 { | ||
| return "", "", false | ||
| } | ||
| return parts2[0], parts2[1], true | ||
| return server.ExtractServiceAccountFromAuth(c) |
There was a problem hiding this comment.
Keep bearer-token ServiceAccount extraction in this compatibility wrapper.
server.ExtractServiceAccountFromAuth only handles X-Remote-User, but callers in this package still rely on ExtractServiceAccountFromAuth for bearer-token API-key requests (getUserSubjectFromContext), and the existing handler tests cover that path. After this delegation, those requests no longer round-trip to system:serviceaccount:<ns>:<name> and instead fall back to the creator userID, which changes behavior for any subject-based logic.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@components/backend/handlers/middleware.go` around lines 266 - 269, The
compatibility wrapper ExtractServiceAccountFromAuth should continue to handle
bearer-token API-key serviceAccount extraction instead of delegating entirely to
server.ExtractServiceAccountFromAuth; restore or reimplement the prior
bearer-token logic here by first calling server.ExtractServiceAccountFromAuth to
handle X-Remote-User, and if that returns empty/false then perform the existing
bearer-token extraction (the same path used by getUserSubjectFromContext) to
reconstruct the "system:serviceaccount:<ns>:<name>" subject; update
ExtractServiceAccountFromAuth to return the correct subject and boolean so
callers in this package continue to receive service account subjects.
Comment on lines
+169
to
+176
| if c.GetString("userID") == "" && K8sClient != nil { | ||
| if ns, saName, ok := resolveServiceAccountFromToken(c); ok { | ||
| sa, err := K8sClient.CoreV1().ServiceAccounts(ns).Get(c.Request.Context(), saName, v1.GetOptions{}) | ||
| if err == nil && sa.Annotations != nil { | ||
| if uid := sa.Annotations["ambient-code.io/created-by-user-id"]; uid != "" { | ||
| c.Set("userID", uid) | ||
| } | ||
| } |
There was a problem hiding this comment.
Do not treat a mutable ServiceAccount annotation as authoritative user identity.
Once the token is verified, this block copies ambient-code.io/created-by-user-id straight into userID without checking that the ServiceAccount is platform-managed. These ServiceAccounts are created in-project with the caller-scoped Kubernetes client in components/backend/handlers/permissions.go, so this annotation is not a strong trust boundary. Any annotated ServiceAccount token can become a user-integration identity; at minimum gate this on a platform-managed marker, and preferably move the mapping to server-owned or tamper-evident state.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@components/backend/server/server.go` around lines 169 - 176, The code
currently trusts the ServiceAccount annotation
"ambient-code.io/created-by-user-id" and sets c.Set("userID", ...) directly (in
the block using K8sClient and resolveServiceAccountFromToken), which allows
caller-controlled SA annotations to impersonate users; change this by requiring
a platform-managed marker on the ServiceAccount before accepting its created-by
annotation (e.g. check for a trusted annotation/label such as an
"ambient-code.io/platform-managed" marker or other server-controlled flag on
sa.Annotations/sa.Labels), and if that marker is absent do not set
c.Set("userID") from the SA annotation — instead record the mapping in
server-owned/tamper-evident state (or reject/ignore the annotation) and only
populate c.Set("userID") after verifying the SA is platform-managed or after
consulting the server-owned mapping.
Comment on lines
+188
to
+193
| rawAuth := c.GetHeader("Authorization") | ||
| parts := strings.SplitN(rawAuth, " ", 2) | ||
| if len(parts) != 2 || !strings.EqualFold(parts[0], "Bearer") { | ||
| return "", "", false | ||
| } | ||
| token := strings.TrimSpace(parts[1]) |
There was a problem hiding this comment.
Query-string API-key auth still misses this fallback.
This resolver only reads Authorization, but ?token= is promoted into that header later in components/backend/handlers/middleware.go inside ValidateProjectContext. Because forwardedIdentityMiddleware() runs first, websocket/agent callers using query-token auth will still reach this path with empty userID, so the new integration fallback never runs for them.
Possible fix
func Run(registerRoutes RouterFunc) error {
// Setup Gin router with custom logger that redacts tokens
r := gin.New()
r.Use(gin.Recovery())
+ r.Use(func(c *gin.Context) {
+ if c.GetHeader("Authorization") == "" && c.GetHeader("X-Forwarded-Access-Token") == "" {
+ if qp := strings.TrimSpace(c.Query("token")); qp != "" {
+ c.Request.Header.Set("Authorization", "Bearer "+qp)
+ }
+ }
+ c.Next()
+ })
r.Use(gin.LoggerWithFormatter(func(param gin.LogFormatterParams) string {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@components/backend/server/server.go` around lines 188 - 193, The
Authorization header parsing in server.go only checks
c.GetHeader("Authorization") (rawAuth) and thus misses query-string ?token=
values used by websocket/agent callers; update the logic in the same function
that computes token (using rawAuth, parts, token) to fallback to the query param
(e.g., c.Query("token") or r.URL.Query().Get("token")) when rawAuth is empty or
parts length != 2, so token gets populated from the ?token= value (this keeps
behavior consistent with ValidateProjectContext and
forwardedIdentityMiddleware); ensure you still trim/validate the token string
before returning.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Test plan
Fixes: RHOAIENG-52967
Co-Authored-By: Claude Opus 4.6 noreply@anthropic.com