fix(a2a): redact transport errors, reject mixed sidecar config (lr-890fae) - #31
Merged
clagentic-merger[bot] merged 7 commits intoAug 17, 2026
Conversation
…lr-890fae)
OpenBao's {"errors":[...]} envelope is parsed and surfaced verbatim (the
existing comment reasoning OpenBao's own error bodies carry no secrets is
preserved). A body that does NOT match that shape -- unbounded content an
intervening proxy could substitute -- is truncated via a new TransportError
type instead of interpolated raw, at both call sites (jwt auth login,
identity/oidc/token read).
MILLER adjudication (lr-890fae comment #8, item F1): true at the code
level that a2atoken.go interpolated string(respBody) verbatim; the claimed
echo path was unproven but the missing bound was real. TransportError is a
distinct type so a2amint can target redaction at this error class without
degrading diagnosability of its own internal errors.
Tests: envelope parsing, non-OpenBao-shaped body truncation (both call
sites), full suite green.
…(lr-890fae) AuditEvent.Reason previously took err.Error() verbatim, which -- when the error is an a2atoken.TransportError -- now carries an already-bounded message, and main.go's stderr Fprintf inherits that bound with no extra work needed there. Every other a2amint error class (attestation, entitlement, missing broker/config, key-parse failure) keeps its full message; none of those carry third-party response content and blanket redaction would have degraded their diagnosability for no benefit. Cross-boundary regression test drives Service.Mint with the real a2atoken.Issue against a stub HTTP server standing in for a proxy returning an oversized non-OpenBao-shaped error body, and asserts the bounded value -- not the raw body -- is what lands in the audit record. Per MILLER's adjudication (lr-890fae comment #8): prior passes verified per-package invariants and never traced a value across the a2atoken -> a2amint boundary, which is why this gap existed.
…t (lr-890fae) ResolveSidecars' prepend ordering (legacy first) was already correct and stays unchanged -- config_test.go's BackCompat test still asserts it. The defect was downstream: cmd/gatekeeper passes chainSidecars[0] alone into the domain-scoped PerSpawn resolver. If an operator's legacy sidecar block names a SESSION namespace while sidecars[0] names the per-spawn namespace, the merge silently puts the session entry first and it gets installed as PerSpawn -- DomainA2A then fail-closes correctly against the WRONG namespace, a confused-deputy outcome. Reject over warn: docs/SIDECAR-READ-CONTRACT.md section 2 mandates spawn-first, and config.example.yaml already deprecates the legacy block in favor of sidecars, so nothing is lost by refusing the combination outright -- consistent with this repo's fail-closed posture. New tests: internal/config unit-level rejection/non-conflict cases, plus a cmd-level cross-boundary test (TestRunMintA2A_MixedLegacyAndNewSidecarConfigRejected) that wires a mixed config through runMintA2A and asserts the rejection -- not just the isolated merge -- per MILLER's finding that no prior test traced this value across the config -> cmd boundary. MILLER adjudication (lr-890fae comment #8, item F2): the reviewer's claimed mechanism (session sidecar satisfying DomainA2A via fallthrough) was refuted -- no such fallthrough exists, DomainA2A has no Chain fallback. The real defect underneath is the wrong-namespace-as-PerSpawn wiring described above, reachable only via misconfiguration, on an off-by-default path.
SETUP.md said no A2A mint command existed yet in cmd/gatekeeper, while the same file documents 'gatekeeper mint-a2a' extensively a few hundred lines later -- self-contradicting since the PR that added the command. AC7 miss, confirmed by MILLER adjudication (lr-890fae comment #8, NIT 2).
… (lr-890fae) Two gaps in the error-redaction bound left by the earlier pass in this PR: 1. parseOpenBaoErrors' parsed-envelope branch returned strings.Join(envelope.Errors, "; ") with no bound at all, while the raw-body fallback branch was truncated to maxRawBodyExcerpt (200 bytes). A deployment could return an oversized errors[] array and the bound would never apply. 2. Both exchangeAssertion and readOIDCToken called io.ReadAll(resp.Body) with no read cap, so the full body materialized in memory BEFORE either parse branch ran - the 200-byte string bound was never a memory bound on any path. Added maxResponseBodyBytes (64KiB), applied via io.LimitReader at both read sites: comfortably larger than any legitimate OpenBao response this package parses (both envelopes are well under 4KiB), small enough to be a real memory bound. Corrected stale comments in this file that asserted a bound the code did not actually have on the parsed-envelope branch. Regression coverage: TestIssueLoginFailureTruncatesOversizedOpenBaoEnvelope (envelope-shaped body >200 bytes) and TestIssueLoginFailureCapsResponseBodyRead (body > maxResponseBodyBytes, proves the read itself is capped, not just the rendered string). TASK: lr-890fae
…(lr-890fae)
auditReason's doc comment claimed a2atoken.Issue's errors were already
bounded/redacted at the source for third-party response content - true
of the raw-fallback branch before this PR's a2atoken fix, but the
parsed-OpenBao-envelope branch was unbounded until the prior commit.
Corrected the comment to name both a2atoken.maxRawBodyExcerpt and
maxResponseBodyBytes explicitly, so it describes what the code actually
guarantees now rather than what a reviewer had to independently verify.
Added TestMintAuditRedactsOversizedOpenBaoErrorEnvelope: drives the
REAL a2atoken.Issue (not a stub IssueFunc) against a stub OpenBao
server returning an oversized {"errors":[...]} envelope on the
jwt-auth-login leg, asserting AuditEvent.Reason - what a real
deployment's log sink would see - is bounded on this branch, tracing
the value across the a2atoken -> a2amint package boundary rather than
asserting only in-package.
TASK: lr-890fae
…90fae)
Corrected main.go's audit-hook comment, which asserted ev.Reason
carries no additional filtering concern without accounting for the %q
verb's expansion: strconv.Quote semantics on control-heavy input can
run roughly 4x the input length, so a 200-byte-bounded Reason can
still print up to ~800 bytes at this sink. Documented that expansion
explicitly rather than leaving the comment silent on it. No code
change to the %q usage itself - MILLER's adjudication refuted the
injection concern for this sink (%q already escapes newlines/control
bytes), so this is a documentation correction, not a new escaping
layer.
Added TestRunMintA2A_StderrAuditBoundedForOversizedOpenBaoErrorEnvelope:
the full cross-boundary sink test - a2atoken -> a2amint -> main.go's
os.Stderr Fprintf - driving an oversized OpenBao {"errors":[...]}
envelope through runMintA2A end to end and asserting the actual bytes
written to stderr are bounded, not an unbounded echo of a
proxy/OpenBao-controlled body. This is the layer the prior in-package
and package-boundary tests did not reach.
TASK: lr-890fae
|
PEACHES — clean All three critical fixes verified: F1: parseOpenBaoErrors envelope-join truncation F2: io.LimitReader memory bound at both response-read sites F3: Three stale comments now describe actual guarantee Data-flow trace: a2atoken → a2amint → main.go stderr sink
Cross-boundary test coverage: Protected tests unchanged: No issues found. All bounds verified at sink. |
|
BOBBIE — clean Re-audit of PR #31 at head eb22003 (moved from 99d346b per 3 fix commits closing the prior findings). Traced an OpenBao error body end-to-end through internal/a2atoken -> internal/a2amint -> cmd/gatekeeper/main.go under the stated threat model (hostile/intervening proxy shaping content to satisfy the {"errors":[...]} envelope parse). Verified fixed:
Six protected pre-existing tests confirmed unchanged: internal/attestation/ (the two DomainA2A fail-closed tests) has zero diff in this PR; internal/config/config_test.go and internal/a2atoken/a2atoken_test.go show additions only -- TestAttestationConfig_ResolveSidecars_BackCompat, TestIssueUntrustedKeyRejected, TestIssueBogusSubjectRejected all untouched. go.mod/go.sum untouched by this diff -- osv-scanner stdlib findings are pre-existing to the go directive, not introduced here. scanners_run: gitleaks (2 hits, both env-var names in test fixtures -- GATEKEEPER_TEST_A2A_KEY_LR890FAE / GATEKEEPER_TEST_A2A_SINK_KEY_LR890FAE -- dropped consistent with PR #30 and the prior #31 pass), semgrep p/golang+p/secrets+p/security-audit (0), osv-scanner (0 new). 0 findings. |
Contributor
|
Merged via clagentic-loadout v0.2.0
|
clagentic-merger
Bot
deleted the
fix/lr-890fae-a2a-error-redaction-sidecar-guard
branch
August 17, 2026 18:45
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
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.
Fold-in unit on already-merged PR #30 (lr-890fae), addressing MILLER's post-merge adjudication (lr-890fae comment #8). Both blocking items confirmed by MILLER as real (though PEACHES's stated F2 mechanism was refuted), plus the stale-doc nit. Built to MILLER's corrected analysis, not the original reviewer framing.
What changed
AUDIT-PATH REDACTION (F1, primary). internal/a2atoken now returns a distinct *a2atoken.TransportError for any OpenBao HTTP-call failure, carrying an already-bounded/redacted message. internal/a2amint.Service.Mint routes AuditEvent.Reason through a new auditReason helper that applies this bound only to the TransportError class -- every other error (attestation, entitlement, missing broker/config, key-parse failure) keeps its full message, since none of those carry third-party response content. cmd/gatekeeper/main.go's stderr Fprintf inherits the bound with no separate change needed there.
ERROR ENVELOPE PARSING. internal/a2atoken/a2atoken.go's two raw string(respBody) interpolation sites (jwt auth login, identity/oidc/token read) now parse OpenBao's documented errors-list envelope (errors: [ ... ]) and surface only those strings; a body that does not match that shape (e.g. an intervening proxy's own error page) is truncated to a fixed excerpt rather than echoed verbatim. The existing comment reasoning that OpenBao's own error bodies carry no secrets is preserved and updated, not deleted -- the fix targets unbounded third-party proxy content, not OpenBao itself.
MIXED LEGACY+NEW SIDECAR CONFIG (F2, hardening). MILLER refuted the reviewer's claimed mechanism (a session sidecar satisfying DomainA2A via fallthrough -- no such fallthrough exists) while confirming a different real defect: cmd/gatekeeper passes chainSidecars[0] alone into the domain-scoped PerSpawn resolver, so if an operator's legacy attestation.sidecar names a session namespace while sidecars[0] names the per-spawn namespace, the deliberate legacy-first merge order (unchanged, still asserted by config_test.go's BackCompat test) puts the session entry at index 0 and it gets installed as PerSpawn -- a confused-deputy outcome. Fixed by rejecting the combination outright at config.Load (AttestationConfig.validate), chosen over warn because docs/SIDECAR-READ-CONTRACT.md section 2 mandates spawn-first and config.example.yaml already deprecates the legacy block -- nothing is lost by refusing it. Added the missing cmd-level wiring test (TestRunMintA2A_MixedLegacyAndNewSidecarConfigRejected) that traces a mixed config through runMintA2A and asserts which namespace the rejection covers, per MILLER's finding that no prior test crossed the config -> cmd boundary.
STALE DOC. docs/SETUP.md's Status-in-this-repository paragraph said no A2A mint command existed yet in cmd/gatekeeper, self-contradicting the same file's extensive mint-a2a documentation a few hundred lines later. Corrected; no other stale A2A-absent statements found elsewhere in the file.
Explicitly out of scope
The --json subject field (a2atoken.go:240-244, main.go:259-262) is documented-deliberate per MILLER's adjudication; left untouched.
Standing constraints preserved
Roster-agnostic throughout. No signing-key handling for the peer-facing token added or touched. Fail-closed ordering unchanged (refusal before any OpenBao call). Additive/off-by-default: a2a_provider absent still leaves the GitHub-domain path byte-identical -- the new config.Load validation only fires on the sidecar-mixing combination, which is unrelated to a2a_provider being configured.
Tests
go build / go vet / go test ./... all pass. New tests: a2atoken envelope-parsing + non-OpenBao-body truncation at both call sites; a2amint cross-boundary redaction test driving the real a2atoken.Issue against a stub proxy response; config unit-level mixed-sidecar rejection/non-conflict cases; cmd-level TestRunMintA2A_MixedLegacyAndNewSidecarConfigRejected wiring test. Both pre-existing negative-control tests and both pre-existing DomainA2A fail-closed tests preserved unchanged.
TASK: lr-890fae