Feat: add static-inject AuthBridge plugin for static credential injection#655
Conversation
|
Warning Review limit reached
Next review available in: 18 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughAdds the ChangesStatic credential injection
Reverse-proxy and SSE handling
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant ReverseProxy
participant Pipeline
participant Backend
Client->>ReverseProxy: Request with headers and Host
ReverseProxy->>Pipeline: Run inbound plugins
Pipeline-->>ReverseProxy: Mutated header state
ReverseProxy->>Backend: Forward rewritten Host and synchronized headers
Backend-->>ReverseProxy: SSE response with event and data fields
ReverseProxy-->>Client: Reframed SSE response preserving event fields
Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
authbridge/authlib/plugins/staticinject/plugin_test.go (1)
12-147: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winAdd a test for the
static-inject.unsafe-valuedeny path.The
OnRequestmethod denies withstatic-inject.unsafe-valuewhen the resolved credential contains CR/LF/NUL, but no plugin-level test covers this security-critical path. WhileSafeHeaderValue/SafeSetHeaderare tested directly inresolver_test.go, the integration throughOnRequestis untested.🧪 Suggested test for unsafe-value deny path
func TestDenyOnUnsafeCredentialValue(t *testing.T) { p := New() cfg := `{ "source": "mappings", "mappings": {"api.example.com": "evil\r\nX-Injected: 1"}, "key_by": "host" }` if err := p.Configure(json.RawMessage(cfg)); err != nil { t.Fatalf("Configure() error = %v", err) } pctx := &pipeline.Context{ Host: "api.example.com", Headers: http.Header{ "Authorization": []string{"Bearer PLACEHOLDER"}, }, } action := p.OnRequest(context.Background(), pctx) if action.Type != pipeline.Reject { t.Fatalf("OnRequest() action.Type = %v, want %v (Reject for unsafe value)", action.Type, pipeline.Reject) } if got := pctx.Headers.Get("Authorization"); got != "Bearer PLACEHOLDER" { t.Errorf("Authorization header = %q, want unmodified %q", got, "Bearer PLACEHOLDER") } }🤖 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 `@authbridge/authlib/plugins/staticinject/plugin_test.go` around lines 12 - 147, Add a plugin-level test named TestDenyOnUnsafeCredentialValue alongside the existing OnRequest tests. Configure a mapping whose resolved credential contains CR/LF characters, invoke OnRequest with the placeholder Authorization header, and assert it returns pipeline.Reject while leaving the Authorization header unchanged.
🤖 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 `@authbridge/authlib/plugins/staticinject/plugin_test.go`:
- Around line 186-208: Update TestConfigure_FailureLeavesZeroDenyState to
perform the invalid reconfiguration on the already-valid plugin p instead of
creating and testing a fresh p2. After the failed Configure call, invoke
p.OnRequest with the existing request context and assert it still returns the
previously configured behavior, verifying Configure atomically preserves valid
state on failure.
---
Nitpick comments:
In `@authbridge/authlib/plugins/staticinject/plugin_test.go`:
- Around line 12-147: Add a plugin-level test named
TestDenyOnUnsafeCredentialValue alongside the existing OnRequest tests.
Configure a mapping whose resolved credential contains CR/LF characters, invoke
OnRequest with the placeholder Authorization header, and assert it returns
pipeline.Reject while leaving the Authorization header unchanged.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 5477f62f-de47-45f3-84b3-69166284a8aa
📒 Files selected for processing (6)
authbridge/authlib/plugins/staticinject/plugin.goauthbridge/authlib/plugins/staticinject/plugin_test.goauthbridge/authlib/plugins/staticinject/resolver.goauthbridge/authlib/plugins/staticinject/resolver_test.goauthbridge/cmd/authbridge-proxy/plugins_staticinject.goauthbridge/docs/plugin-reference.md
|
Thanks for the review — addressed in 43debf2 (test-only):
Assisted-By: Claude Code |
huang195
left a comment
huang195
left a comment
There was a problem hiding this comment.
Summary
Bellissimo — this is a clean, self-contained plugin done right: fail-closed zero value, strict DisallowUnknownFields decode, Configure commits state only after full validation, and genuinely careful security helpers. The FileResolver path-containment (filepath.Dir(join) != Clean(Dir)) correctly rejects .., absolute, and nested keys, and SafeSetHeader closes the CWE-113 header-splitting door — both with tests. 19 tests, TDD RED→GREEN, and a thorough plugin-reference.md entry. Chef's kiss on the docs. 👨🍳
One non-blocking fail-closed edge as the single inline suggestion. Minor doc nit (not inline): the config table could state whether the key_by: host lookup key includes the port — pctx.Host is a bare field, so operators need to know whether the secret file should be named api.example.com or api.example.com:443.
Maintainer author, green CI, DCO signed. Approving — bravo! 🇮🇹
Assisted-By: Claude Code
ReadCredentialFile trims a whitespace-only secret file to "" and returns ok, and an inline mapping value may be "". The OnRequest guard only checked !ok, so an empty credential was forwarded as "Bearer " (or an empty raw header) instead of denying. Add a value == "" guard alongside !ok and lock it in with a test covering both an empty inline mapping and a whitespace-only secret file. Addresses review feedback from @huang195 on PR rossoctl#655. Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: Paolo Dettori <dettori@us.ibm.com>
…tion Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: Paolo Dettori <dettori@us.ibm.com>
…t, wantErr asserts) Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: Paolo Dettori <dettori@us.ibm.com>
Add inject_header config option to staticInjectConfig so credentials can be written to a header other than Authorization (e.g. x-api-key for direct api.anthropic.com auth). Default remains 'Authorization', preserving byte-identical legacy behavior (Bearer scheme, Authorization not deleted). Any other configured header writes the raw credential value and removes the inbound Authorization header so a stale placeholder bearer never reaches the backend. Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: Paolo Dettori <dettori@us.ibm.com>
ReadCredentialFile trims a whitespace-only secret file to "" and returns ok, and an inline mapping value may be "". The OnRequest guard only checked !ok, so an empty credential was forwarded as "Bearer " (or an empty raw header) instead of denying. Add a value == "" guard alongside !ok and lock it in with a test covering both an empty inline mapping and a whitespace-only secret file. Addresses review feedback from @huang195 on PR rossoctl#655. Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: Paolo Dettori <dettori@us.ibm.com>
452f314 to
adf5d2a
Compare
github-project-automation
Bot
moved this from New/ToDo
to Done
in Kagenti Issue Prioritization
3 participants
Summary
Adds a new
static-injectoutbound AuthBridge plugin that swaps a placeholder credential on theoutbound
Authorizationheader for a real static credential, resolved from a mounted secret — so amodel-influenced workload never holds the real secret.
This is the credential-injection mechanism for the serverless-harness RC1 AuthBridge egress
control-plane PoC (single-tenant, Kind-first). It is a self-contained plugin on the existing
pipeline.Plugin/Configurablecontract — no shared-interface change — modeled on the in-repotoken-broker/ibacplugins.What it does
source(secret_dir|mappings),secret_dir,mappings(tests only),key_by(host|static),key,placeholder.OnRequest(outbound): reads the bearer; fail-closed deny on missing bearer, placeholder mismatch,unresolved key, or an unsafe resolved value; otherwise rewrites
Authorization: Bearer <real>.FileResolveris path-contained (rejects../absolute/separator keys);SafeSetHeaderrejectsCR/LF/NUL (CWE-113).
Configurecommits state only after full validation (a failed config = deny-state).static-injectvia a build-taggedcmd/authbridge-proxy/plugins_staticinject.go(droppable with
-tags exclude_plugin_staticinject), matching the sibling plugin pattern.Testing
go test ./plugins/staticinject/...— 19 tests (incl. subtests), all pass; TDD (RED→GREEN).go build ./authlib/... ./cmd/authbridge-proxy/...,gofmt,go vet— clean.authbridge-proxyimage from this branch and verifiedstatic-injectis registered.Docs: a
static-injectentry added toauthbridge/docs/plugin-reference.md.Assisted-By: Claude Code
Summary by CodeRabbit
static-injectoutbound auth plugin to replace an approved bearer placeholder with a resolved static credential (including configurable header injection).event:lines.static-inject.