feat(security): credential binding enforcement#1671
Merged
Conversation
Hierarchical credential binding authorization per credential-binding-enforcement spec: - Relax project check from project:owner to project:editor+ for credential bindings - Add agent-level validation (agent_id requires project_id, agent must belong to project) - Global credential bindings require platform:admin - Allow platform:admin to create internal role bindings (credential:token-reader) - Asymmetric unbind: project:editor can remove credential bindings without credential:owner - Non-credential delete restricted to project:owner/platform:admin - Scope enum validation rejects invalid scope values - Empty-string FK validation prevents invalid DB state - Nil-guard all nullable field dereferences in PATCH handler - Replace raw DB error messages with generic authorization failure responses - Row-level locking (SELECT FOR UPDATE) on last-owner count queries - Add credential:token-reader to RoleLevel hierarchy map Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Migration adds role_binding:delete permission to project:editor so the RBAC middleware allows editors to reach the Delete handler for credential binding asymmetric unbind. The handler's secondary authorization restricts editors to credential-scope bindings only. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…lifecycle Replace legacy resolveCredentialIDs (which listed ALL credentials) with hierarchical binding resolution: agent → project → global precedence. Earliest created_at wins for same-provider duplicates at the same tier. Add credential:token-reader grant lifecycle: - grantTokenReaderBindings creates scoped bindings at session provision - revokeTokenReaderBindings cleans up at deprovision/cleanup with error surfacing - ServiceIdentity config field from GRPC_SERVICE_ACCOUNT env var Security hardening: - TSL injection prevention via validateTSLValue regex before all fmt.Sprintf - Remove legacy fallback that leaked credentials across tenants - Revocation errors logged at Error level instead of silently swallowed Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Integration tests (Go, testcontainers): - 10 tests covering Create handler authorization scenarios - 7 tests for hierarchical credential resolver with mock API server - Tests cover: editor bind, viewer reject, agent validation, global binding, asymmetric unbind, internal role creation, agent-project check E2e tests (bash, Kind cluster): - Phase 26 added to rbac_e2e_test.sh with 12 credential binding scenarios - Tests real Keycloak JWT auth against running API server - Covers bind, unbind, agent validation, global binding, internal roles Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
|
Warning Review limit reached
More reviews will be available in 16 minutes and 45 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR implements credential binding enforcement across the API and control plane. It adds credential-scoped role authorization rules (create/patch/delete), enables credential resolution via hierarchical role bindings instead of simple listing, manages ChangesCredential Binding Enforcement
Possibly related PRs
Suggested labels
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (2 errors, 1 warning)
✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
components/ambient-api-server/plugins/roleBindings/handler.go (1)
246-283: 💤 Low valueConsider extracting scope FK validation to reduce duplication.
The pattern for preventing scope FK changes is repeated 4 times (ProjectId, AgentId, SessionId, CredentialId) with identical logic. Consider extracting to a helper function.
However, the current explicit approach is clear and low-risk, so this is optional.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@components/ambient-api-server/plugins/roleBindings/handler.go` around lines 246 - 283, The repeated "prevent scope widening" checks for ProjectId, AgentId, SessionId, and CredentialId duplicate the same logic; extract that logic into a helper like validateNoScopeFKChange(callerLevel int, isSet func() bool, get func() *T, foundVal *T) (or a typed variant) and call it for each field from the patch handling block so the pattern in handler.go (the callerLevel != 0 branch that checks patch.<Field>.IsSet(), patchVal := patch.<Field>.Get(), found.<Field> == nil || patchVal == nil || *patchVal != *found.<Field>) is centralized and reused for ProjectId, AgentId, SessionId, and CredentialId validation.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@components/ambient-control-plane/internal/config/config.go`:
- Line 95: The ServiceIdentity field in config.go is set from
os.Getenv("GRPC_SERVICE_ACCOUNT") without normalization; update the config
initialization to mirror the middleware by trimming whitespace/newlines so the
service identity matches the API's caller context. Locate the ServiceIdentity
assignment in the config construction (the ServiceIdentity symbol) and replace
the raw env read with a normalized value (e.g., strings.TrimSpace of the env
var) so trailing spaces/newlines do not create mismatched user_id values.
In `@components/ambient-control-plane/internal/reconciler/kube_reconciler.go`:
- Around line 206-208: The call to r.grantTokenReaderBindings currently swallows
partial failures and only logs a top-level error, which can cause ensurePod to
receive credentialIDs the identity wasn't granted; update
grantTokenReaderBindings (and other similar usages) to either return a non-nil
error on any individual grant failure or return a filtered map/slice of
credential IDs that were actually bound successfully, and then pass that
filtered set into ensurePod instead of the original credentialIDs (refer to
r.grantTokenReaderBindings, credentialIDs, session.ID, and ensurePod to locate
the callsite and implementation). Ensure the chosen approach propagates errors
or explicitly collects and returns successful IDs so no partial failures are
silently ignored.
- Around line 228-235: The revokeTokenReaderBindings error is being logged and
swallowed in the session cleanup path; change the logic in the block that calls
r.factory.ForProject(ctx, session.ProjectID) and
r.revokeTokenReaderBindings(ctx, sdk, session.ID) so that if
revokeTokenReaderBindings returns an error you return it (or wrap and return it)
instead of only logging, ensuring the reconciler requeues; do the same for the
second occurrence around r.revokeTokenReaderBindings at the later branch. Locate
the calls to r.factory.ForProject and r.revokeTokenReaderBindings in
kube_reconciler.go and replace the logging-only error handling with propagation
(return err or fmt.Errorf/contextual wrap) so partial revocation failures are
not silently ignored.
---
Nitpick comments:
In `@components/ambient-api-server/plugins/roleBindings/handler.go`:
- Around line 246-283: The repeated "prevent scope widening" checks for
ProjectId, AgentId, SessionId, and CredentialId duplicate the same logic;
extract that logic into a helper like validateNoScopeFKChange(callerLevel int,
isSet func() bool, get func() *T, foundVal *T) (or a typed variant) and call it
for each field from the patch handling block so the pattern in handler.go (the
callerLevel != 0 branch that checks patch.<Field>.IsSet(), patchVal :=
patch.<Field>.Get(), found.<Field> == nil || patchVal == nil || *patchVal !=
*found.<Field>) is centralized and reused for ProjectId, AgentId, SessionId, and
CredentialId validation.
🪄 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: 5268f141-74a2-4b45-8e61-da3138460711
📒 Files selected for processing (12)
components/ambient-api-server/pkg/rbac/hierarchy.gocomponents/ambient-api-server/plugins/roleBindings/handler.gocomponents/ambient-api-server/plugins/roles/migration.gocomponents/ambient-api-server/plugins/roles/plugin.gocomponents/ambient-api-server/test/e2e/rbac_e2e_test.shcomponents/ambient-api-server/test/integration/credential_binding_test.gocomponents/ambient-api-server/test/integration/integration_test.gocomponents/ambient-api-server/test/integration/rbac_test.gocomponents/ambient-control-plane/cmd/ambient-control-plane/main.gocomponents/ambient-control-plane/internal/config/config.gocomponents/ambient-control-plane/internal/reconciler/credential_resolver_test.gocomponents/ambient-control-plane/internal/reconciler/kube_reconciler.go
The credential:owner auto-binding (created when a credential is created) has the same shape as a global injection binding: scope=credential, credential_id set, project_id=NULL, agent_id=NULL. The resolver was treating ownership bindings as injection intent, causing unbound credentials to be injected into sessions. Fix: look up the credential:owner role ID and filter out ownership bindings from all three resolution tiers (agent, project, global). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- TrimSpace on GRPC_SERVICE_ACCOUNT to match API server's middleware normalization - grantTokenReaderBindings returns only successfully granted credential IDs; ensurePod receives the filtered map so sidecars are only created for credentials the service identity can actually read - Propagate revokeTokenReaderBindings errors from deprovisionSession and cleanupSession so the reconciler requeues on failure instead of leaving orphaned token-reader grants Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
markturansky
reviewed
Jun 9, 2026
| RolePlatformViewer: 2, | ||
| RoleProjectEditor: 2, | ||
| RoleAgentOperator: 2, | ||
| "agent:editor": 2, |
Contributor
Merge Queue Status
This pull request spent 1 minute 27 seconds in the queue, including 16 seconds running CI. Required conditions to merge |
✅ Deploy Preview for cheerful-kitten-f556a0 canceled.
|
✅ Deploy Preview for cheerful-kitten-f556a0 canceled.
|
1 similar comment
✅ Deploy Preview for cheerful-kitten-f556a0 canceled.
|
✅ Deploy Preview for cheerful-kitten-f556a0 canceled.
|
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
2 participants
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
Implements the credential binding enforcement spec (
specs/security/credential-binding-enforcement.spec.md):Files changed
plugins/roleBindings/handler.gopkg/rbac/hierarchy.goplugins/roles/migration.go,plugin.goreconciler/kube_reconciler.goconfig/config.go,main.goTest plan
rbac_e2e_test.shon Kind cluster) — all pass🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
New Features
Improvements