fix(api-server): register pre-auth interceptors when only GRPC_SERVICE_ACCOUNT is set#1454
Closed
markturansky wants to merge 2 commits intomainfrom
Closed
fix(api-server): register pre-auth interceptors when only GRPC_SERVICE_ACCOUNT is set#1454markturansky wants to merge 2 commits intomainfrom
markturansky wants to merge 2 commits intomainfrom
Conversation
… gRPC authz The control-plane gives runner pods OIDC tokens (SSO JWTs) for gRPC auth. These tokens pass JWT verification but carry the OIDC client username (e.g. ocm-ams-service), not the session creator's username. WatchSessionMessages then rejects the call with PERMISSION_DENIED because the JWT username doesn't match session.CreatedByUserId. Add GRPC_SERVICE_ACCOUNT env var support: when the JWT username matches this configured service account name, the pre-auth interceptor sets CallerTypeService on the context, allowing the call to bypass the per-user ownership check — same as static AMBIENT_API_TOKEN calls. The env var is sourced from the ambient-api-server secret's clientId key (same OIDC client ID the CP uses), with optional: true so deployments without OIDC continue to work. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
…E_ACCOUNT is set The init() guard required AMBIENT_API_TOKEN to be non-empty before registering gRPC pre-auth interceptors. On Stage/production with OIDC auth, AMBIENT_API_TOKEN is not set — only GRPC_SERVICE_ACCOUNT is. This meant the interceptors were never registered and OIDC service tokens were not recognized as service callers. Register interceptors when either AMBIENT_API_TOKEN or GRPC_SERVICE_ACCOUNT is set. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
✅ Deploy Preview for cheerful-kitten-f556a0 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Contributor
📝 WalkthroughWalkthroughThe authentication middleware now supports service account authentication via a new Changes
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 1 warning)
✅ Passed checks (6 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
✨ Simplify code
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/ambient-api-server/pkg/middleware/bearer_token_grpc.go`:
- Around line 32-34: The code elevates to CallerTypeService based on an
unverified JWT claim; remove the ParseUnverified-based path and replace it with
strict signature and claims validation: stop using
usernameFromJWT()/ParseUnverified(), instead validate the token signature (e.g.,
via jwt.ParseWithClaims or equivalent) and verify required claims (issuer,
audience/client_id, expiry) before matching the validated subject/username to
serviceAccountUsername, and only then call withCallerType(ctx,
CallerTypeService); keep the existing constant-time token comparison path intact
and ensure any new validation uses the configured signing key and fails closed
on errors.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 53ed6961-9628-4fff-b9fe-e799470ada2f
📒 Files selected for processing (3)
components/ambient-api-server/pkg/middleware/bearer_token.gocomponents/ambient-api-server/pkg/middleware/bearer_token_grpc.gocomponents/manifests/base/core/ambient-api-server-service.yml
Comment on lines
+32
to
+34
| if serviceAccountUsername != "" && username == serviceAccountUsername { | ||
| ctx = withCallerType(ctx, CallerTypeService) | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "1) Where service caller privilege is assigned/checked:"
rg -n -C3 --type=go '\bwithCallerType\s*\(.*CallerTypeService|\bIsServiceCaller\s*\('
echo
echo "2) Which JWT claims are currently inspected for identity/elevation:"
rg -n -C3 --type=go 'preferred_username|username|sub|azp|client_id|clientId|aud|iss|issuer'Repository: ambient-code/platform
Length of output: 50378
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Finding usernameFromJWT implementation ==="
rg -n "func usernameFromJWT" --type=go -A 20
echo ""
echo "=== Finding JWT validation/parsing ==="
rg -n "jwt\.|JWT\." --type=go | head -30
echo ""
echo "=== Checking for JWT library usage ==="
grep -r "jwt" --include="*.go" components/ambient-api-server/pkg/middleware/ | head -20Repository: ambient-code/platform
Length of output: 5098
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Full bearer_token_grpc.go context around lines 25-65 ==="
sed -n '20,70p' components/ambient-api-server/pkg/middleware/bearer_token_grpc.go
echo ""
echo "=== Check if expectedToken is required for username path ==="
sed -n '20,70p' components/ambient-api-server/pkg/middleware/bearer_token_grpc.go | grep -A 5 -B 5 "usernameFromJWT\|serviceAccountUsername"Repository: ambient-code/platform
Length of output: 3764
Remove unverified JWT claim path for service caller elevation; implement strict signed token validation.
Lines 31–34 and 57–61 elevate to CallerTypeService when an unverified JWT username claim matches serviceAccountUsername. This is a critical vulnerability: usernameFromJWT() (line 80) uses ParseUnverified(), allowing any attacker to forge a JWT with a matching username claim and bypass token signing entirely. Service elevation must require signature validation and strong claims (e.g., issuer, audience, client_id) beyond username matching. The constant-time token path (lines 28–29, 54–55) is secure; the unverified path should be removed or replaced with cryptographic verification.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@components/ambient-api-server/pkg/middleware/bearer_token_grpc.go` around
lines 32 - 34, The code elevates to CallerTypeService based on an unverified JWT
claim; remove the ParseUnverified-based path and replace it with strict
signature and claims validation: stop using usernameFromJWT()/ParseUnverified(),
instead validate the token signature (e.g., via jwt.ParseWithClaims or
equivalent) and verify required claims (issuer, audience/client_id, expiry)
before matching the validated subject/username to serviceAccountUsername, and
only then call withCallerType(ctx, CallerTypeService); keep the existing
constant-time token comparison path intact and ensure any new validation uses
the configured signing key and fails closed on errors.
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
init()guard requiredAMBIENT_API_TOKENto be non-empty before registering gRPC pre-auth interceptorsAMBIENT_API_TOKENis not set — onlyGRPC_SERVICE_ACCOUNTis — so interceptors were never registered and OIDC service tokens were not recognized as service callersTest plan
OIDC service account username: ocm-ams-serviceacpctl start, tail runner logs — noPERMISSION_DENIEDonWatchSessionMessages🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
New Features
Configuration
GRPC_SERVICE_ACCOUNTenvironment variable to enable service account credential configuration.