Fix: remediate security audit findings in authbridge#496
Merged
huang195 merged 2 commits intoJun 12, 2026
Conversation
Address HIGH/CRITICAL findings from the agents-operator security audit
that apply to the authbridge code in this repo:
- tokenbroker: replace case-sensitive extractBearer with canonical
auth.ExtractBearer (RFC 6750 compliance)
- bypass: reject overly broad patterns ("*", "/*", "") and trim
whitespace from patterns before storing
- redact: new package to strip sensitive fields (client_secret,
judge_bearer, passwords, tokens) from JSON config payloads
- statserver: redact /config endpoint output, fix Content-Type on
error responses, restore trailing newline consistency
- sessionapi: redact /v1/pipeline plugin config output
- cache: replace full-clear eviction with random-sample eviction
(~25%) to prevent cache-miss storms under pressure
- extproc: call RunFinish on rejected requests so pipeline Finisher
contract is honored even on deny paths
Signed-off-by: Varsha Prasad Narsing <vnarsing@redhat.com>
Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com>
Signed-off-by: Varsha Prasad Narsing <varshaprasad96@gmail.com>
github-project-automation
Bot
moved this to New /:ToDo
in Rossoctl Issue Prioritization
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughConsolidates and hardens authlib: adds JSON redaction for sensitive fields, tightens bypass pattern validation, improves cache eviction, ensures pipeline finish hooks run on early rejection, and centralizes bearer extraction to a shared utility. ChangesAuthlib Enhancements
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" 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 (2)
authbridge/authlib/redact/redact_test.go (1)
38-52: ⚡ Quick winAdd a regression case for sensitive keys that wrap objects or arrays.
The suite only exercises sensitive keys at string leaves. It doesn’t cover the currently broken shape where the sensitive key itself owns a container, e.g.
{"token":{"access_token":"..."}}, so this leak can slip through without a failing test.Also applies to: 103-119
🤖 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/redact/redact_test.go` around lines 38 - 52, Test suite only checks redaction when sensitive keys map to string leaves; add regression cases in redact_test.go that exercise sensitive keys whose values are objects and arrays (e.g., {"token": {"access_token":"..."}} and {"token":[{"access_token":"..."}]}) to ensure JSON(in) / JSON() redacts the entire value to "[REDACTED]". Modify or add tests (e.g., extend TestJSON_RedactsNestedKeys or add new TestJSON_RedactsContainerValues) to unmarshal JSON(out) and assert that the sensitive key (e.g., "token", "jwt_password" if used as a container in tests) equals "[REDACTED]" and that other sibling keys remain unchanged.authbridge/authlib/plugins/tokenexchange/cache/cache_test.go (1)
68-90: 💤 Low valueSolid test coverage for random eviction behavior.
The test correctly validates:
- Cache size falls to ~75% capacity (target=75) + the new overflow entry
- The overflow entry is present after eviction (critical check)
The margin of 70-80 appropriately accounts for the non-deterministic map iteration order.
Optional readability suggestion:
Consider using `fmt.Sprintf` for clearer test keys
Line 72 uses
string(rune(i))to generate unique keys, which produces Unicode characters U+0000 through U+0063. While this works correctly, it's less readable than:+import "fmt" + func TestMaxSize_RandomEviction(t *testing.T) { const maxSize = 100 c := New(WithMaxSize(maxSize)) for i := range maxSize { - c.Set("token"+string(rune(i)), "aud", "val", 5*time.Minute) + c.Set(fmt.Sprintf("token%d", i), "aud", "val", 5*time.Minute) }The current approach avoids the fmt import, so this is purely a style preference.
🤖 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/tokenexchange/cache/cache_test.go` around lines 68 - 90, In TestMaxSize_RandomEviction replace the key generation using string(rune(i)) with a clearer formatted string (e.g. fmt.Sprintf("%d", i)) when calling c.Set inside the loop so test keys are readable and unambiguous; update imports to include fmt. Locate the loop that invokes c.Set("token"+string(rune(i)), "aud", "val", 5*time.Minute) and change the concatenation to use a formatted integer key (e.g. "token"+fmt.Sprintf("%d", i)) and add fmt to the test file imports.
🤖 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/redact/redact.go`:
- Around line 36-40: The loop currently short-circuits on sensitive keys
(isSensitiveKey(k)) and only replaces direct string values, then continues,
which skips recursion into container-valued sensitive fields and leaks nested
secrets; change the branch in the redact/map-walking logic so that when
isSensitiveKey(k) is true you still redact if v is a string (m[k] =
"[REDACTED]") but do NOT "continue" for non-string values — instead detect
container types (map[string]interface{} and []interface{}) and recursively call
the existing redact helper(s) (e.g., redactValue/redactMap/redactSlice or
whatever recursion functions exist) to descend and redact nested entries,
leaving other primitive non-string types untouched. Ensure you reference the
same symbols used in the file (isSensitiveKey, the map iteration block, and the
recursive redact helpers) so callers behave correctly for payloads like
{"token": {"access_token":"secret"}}.
---
Nitpick comments:
In `@authbridge/authlib/plugins/tokenexchange/cache/cache_test.go`:
- Around line 68-90: In TestMaxSize_RandomEviction replace the key generation
using string(rune(i)) with a clearer formatted string (e.g. fmt.Sprintf("%d",
i)) when calling c.Set inside the loop so test keys are readable and
unambiguous; update imports to include fmt. Locate the loop that invokes
c.Set("token"+string(rune(i)), "aud", "val", 5*time.Minute) and change the
concatenation to use a formatted integer key (e.g. "token"+fmt.Sprintf("%d", i))
and add fmt to the test file imports.
In `@authbridge/authlib/redact/redact_test.go`:
- Around line 38-52: Test suite only checks redaction when sensitive keys map to
string leaves; add regression cases in redact_test.go that exercise sensitive
keys whose values are objects and arrays (e.g., {"token":
{"access_token":"..."}} and {"token":[{"access_token":"..."}]}) to ensure
JSON(in) / JSON() redacts the entire value to "[REDACTED]". Modify or add tests
(e.g., extend TestJSON_RedactsNestedKeys or add new
TestJSON_RedactsContainerValues) to unmarshal JSON(out) and assert that the
sensitive key (e.g., "token", "jwt_password" if used as a container in tests)
equals "[REDACTED]" and that other sibling keys remain 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: fb6c31ba-455b-47d5-b94c-a51a58f4241d
📒 Files selected for processing (11)
authbridge/authlib/bypass/matcher.goauthbridge/authlib/bypass/matcher_test.goauthbridge/authlib/listener/extproc/server.goauthbridge/authlib/observe/statserver.goauthbridge/authlib/plugins/tokenbroker/plugin.goauthbridge/authlib/plugins/tokenbroker/plugin_edge_test.goauthbridge/authlib/plugins/tokenexchange/cache/cache.goauthbridge/authlib/plugins/tokenexchange/cache/cache_test.goauthbridge/authlib/redact/redact.goauthbridge/authlib/redact/redact_test.goauthbridge/authlib/sessionapi/server.go
huang195
changed the title
huang195
approved these changes
Jun 12, 2026
huang195
left a comment
huang195
left a comment
Member
There was a problem hiding this comment.
Review Summary
Solid, well-scoped remediation of the audit findings, with good test coverage across the board (footgun-pattern rejection, whitespace trim, random-eviction bounds, 8 redaction cases incl. nested arrays, RFC-6750 bearer edge cases). A few notes:
- Genuine wins: redacting the unauthenticated session API +
/configis a real improvement (closes a known info-exposure gap);RunFinish-on-reject is applied symmetrically across all four ext_proc paths (honors the Finisher contract — worth a quick confirm that finishers are idempotent and the Outcome reflects "rejected"); the statserver error path correctly preservesapplication/jsonsince Content-Type is set beforeWriteHeader. - Bypass matcher: rejecting
*//*/empty is good hardening, but note it's a denylist (a pattern like/v1/*is still broad yet allowed) and it's a behavior change — any existing deployment withbypass_paths: ["*"]or["/*"]will now fail atNewMatcher(fail-closed, which is the intent, but worth a release-note line). - Cache eviction (nit):
evictRandomrelies on Go's map-iteration order, which is randomized but bucket-biased (not uniform) and doesn't prefer expired/LRU entries — fine for breaking the full-clear storm, just not truly "random."
The two inline suggestions (redaction key coverage; bearer trim semantics) are the only things I'd act on, neither blocking.
Areas reviewed: Go (bypass, redact, cache, extproc, statserver, tokenbroker, sessionapi) + tests; cross-checked the auth.ExtractBearer impl · Commits: 1, signed-off ✅ (the PR body has a "Singed-off-by" typo, but the commit trailer is correct — DCO passed) · CI: green
No blockers — approving.
Assisted-By: Claude Code
Member
|
@varshaprasad96 I'll hold off merging in case you want to make any of the suggested changes. We can also merge as is as these are not must-fix. |
- redact: add "key" and "credential" to sensitive suffix list for
defense-in-depth (api_key, private_key, signing_key, etc.)
- redact: descend into container-valued sensitive keys instead of
skipping them — a field like {"token": {"access_token": "..."}}
now has its nested secrets redacted
- redact: add best-effort doc noting inline secrets should use
*_file paths; this layer is defense-in-depth
- tokenbroker: TrimSpace the extracted bearer token so whitespace-only
tokens ("Bearer ") are treated as missing rather than forwarded
to the broker
Signed-off-by: Varsha Prasad Narsing <vnarsing@redhat.com>
Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com>
Signed-off-by: Varsha Prasad Narsing <varshaprasad96@gmail.com>
Contributor
Author
|
@huang195 addressed the changes suggested in the review. Thanks! |
5 tasks
github-project-automation
Bot
moved this from New /:ToDo
to Done
in Rossoctl Issue Prioritization
kellyaa
added a commit
to kellyaa/kagenti-extensions
that referenced
this pull request
Jun 12, 2026
Adds construction-time guards, audit logging, and tests asked for in the PR rossoctl#500 review. Skiphost matcher (authlib/listener/skiphost): - Reject match-all patterns ("*", "**", whitespace-only) at New() so a single misconfigured entry cannot silently disable the entire outbound enforcement pipeline. Mirrors the bypass-pattern guard added to ibac in rossoctl#496. Empirically verified: under .-delimited glob "*" matches every single-label hostname (which is how every short Kubernetes service name reaches the listener), and "**" is the unambiguous match-all. Patterns like "*.svc.cluster.local" or "*.*" are NOT match-all and remain accepted. - Reject patterns containing ":" so an operator typo like "otel-collector:8335" fails fast at startup rather than silently never matching (Match strips the port from the incoming host before comparing, so colon-bearing patterns would never fire). - Add MatchPattern(host) (string, bool) so listeners can attribute audit logs / counters to the specific raw operator-supplied pattern that fired. Match() is now a thin wrapper around it for callers that only want the boolean. Listener skip-path audit logging: - Each successful skip in extproc (handleOutbound, handleOutboundBody) and forwardproxy (handleRequest, handleConnect) now emits a structured slog INFO line carrying host, matched pattern, and method/path. Without this trail, a successful self-exemption left no trace anywhere -- no session event, no plugin invocation -- because that is exactly what skip means. Trust-model documentation (authlib/config/config.go): - ListenerConfig.SkipHosts godoc now spells out that the matched value is agent-influenceable on the ext_proc and HTTP-forward paths (Envoy :authority / r.Host header) and only safe-by- construction on the CONNECT-tunnel path. Operators should not list a host they would want IBAC / token-exchange to deny on. Hard-guard reminder (authlib/listener/forwardproxy/transparent.go): - HandleTransparentConn (proxy-sidecar enforce-redirect mode) intentionally does NOT consult SkipHosts. Add a paragraph to its godoc stating the omission is deliberate so a future maintainer does not "fix the inconsistency" by adding a skip keyed on the agent-controlled SNI/Host bytes and open a real bypass of the hard egress guard. Tests: - New TestForwardProxy_SkipHosts_MatchesAgainstHostHeaderNotURL locks the trust-model contract for the HTTP forward path: the skip keys on the agent-supplied Host header, not on r.URL.Host. - New TestForwardProxy_SkipHosts_CONNECT_BypassesPipeline exercises the previously-uncovered CONNECT skip path end-to-end. - New TestNew_RejectsMatchAllStar / RejectsMatchAllDoubleStar / RejectsWhitespaceOnly / RejectsPortInPattern lock the construction- time guards, plus TestNew_AcceptsLeadingStar guards against a future rewrite that over-rejects and breaks operator-typical FQDN patterns. CLAUDE.md gotcha rossoctl#11: - Narrow the LastIntent() wording: the pin protects against FIFO eviction only; expired or deleted sessions can still return nil. Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: Kelly Abuelsaad <kaymar@gmail.com>
This was referenced Jun 15, 2026
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
3 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
extractBearerwith canonicalauth.ExtractBearer(case-insensitive per RFC 6750 §2.1)authlib/redactpackage stripsclient_secret,judge_bearer, passwords, and token values from/configand/v1/pipelineendpointsbypass.NewMatchernow rejects overly broad patterns (*,/*, empty) and trims whitespace from patterns before storingRunFinishis now called on rejected requests in all four ext_proc handler paths, honoring the pipeline Finisher contractapplication/jsonContent-Type on error path, restored trailing newline consistency with/statsContext
A security audit of
opendatahub-io/agents-operator(commitb6489069) produced 85 must-fix findings (8 CRITICAL, 77 HIGH). Since authbridge originated in this repo, many findings apply here. This PR addresses the verified, fixable issues. False positives (TLS InsecureSkipVerify with custom VerifyPeerCertificate,$EDITORexec pattern in abctl, SSE client timeout, init container USER root, pre-existing shellcheck disables) were triaged out.Test plan
go vet ./...passes (verified locally)go test ./...passes — all 36 packages green (verified locally)TestNewMatcher_RejectsFootgunPatterns,TestNewMatcher_TrimsWhitespace,TestMaxSize_RandomEviction,TestJSON_*(8 redaction tests including nested arrays)TestExtractBearer_EdgeCases(now RFC 6750 case-insensitive),TestTokenBroker_OnRequest_DifferentAuthSchemes(lowercase bearer accepted)Singed-off-by: Varsha Prasad Narsing varshaprasad96@gmail.com
Assisted-By: Claude (Anthropic AI) noreply@anthropic.com
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Tests