broker: passthrough unmatched hosts by default with per-vault deny opt-in#146
broker: passthrough unmatched hosts by default with per-vault deny opt-in#146dangtony98 merged 7 commits intomainfrom
Conversation
…t-in
The MITM proxy used to deny every request whose target host wasn't in the
vault's service allowlist, which made a fresh install reject all traffic
until an operator hand-curated services. Flip the default: unmatched
hosts now forward as plain proxy traffic, and a new per-vault
`unmatched_host_policy=deny` setting restores the strict allowlist for
operators who want it.
Adds a generic vault_settings key/value table (migration 043), a
GET/PATCH /v1/vaults/{name}/settings endpoint gated on vault admin or
instance owner, a Strict deny mode toggle in the vault Settings tab, and
paired updates to skill_cli.md / skill_http.md so agents understand 403
forbidden only fires under deny mode now. The matched-host hot path adds
zero work; the policy lookup only fires on the unmatched-host branch and
fails closed on store errors.
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Workflows to automatically generate PRs for you. |
|
💬 Discussion in Slack: #pr-review-agent-vault-146-broker-passthrough-unmatched-hosts-by-default-with-per Posted by Review Police — reviews, comments, new commits, and CI failures will stream into this channel. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
- credential.go: a transient GetBrokerConfig error was silently treated as "no row" and fell through to the unmatched-host policy, which under the new passthrough default would forward an upstream-credentialed request without injection — a fail-closed -> fail-open regression. Distinguish sql.ErrNoRows from real errors and fail closed on the latter, mirroring the policy-lookup branch a few lines below. - handle_vaults.go (PATCH): echo the validated input on the write path instead of unconditionally re-reading. A transient read failure after a committed write would 500, which the optimistic UI rolls back — desyncing the displayed policy from the persisted one. The read still fires for no-op PATCHes. - handle_vaults.go (GET): relax to vault-member scope so non-admin members see the real policy in the Settings tab. Previously the GET required admin/owner and the frontend silently fell back to passthrough on the resulting 403, misrepresenting an enforcement control. PATCH stays admin/owner-only. - Tests: assert fail-closed on a non-ErrNoRows GetBrokerConfig error, PATCH-response echo, and the member-can-GET / member-cannot-PATCH split.
Pulls the unmatched-host policy toggle into the same bordered card as the rename row, separated by a horizontal rule so the two controls share visual width and the page reads as one config card. Description trimmed to a single sentence — the wording belongs in the docs, not the chrome.
The rename row is bounded by its input's max-w-xs plus the Rename button (~max-w-md total). Wrap the toggle row in a max-w-md inner box so the description and Toggle sit within the same visual band, instead of stretching to the full card width.
…-deny label Wraps both the rename row and the strict-deny row in a single max-w-md box so the Rename button and Toggle align flush at the right edge. Switches the strict-deny heading to the same uppercase label style as the FormField above it for visual consistency.
|
@claude review |
There was a problem hiding this comment.
Fixes in cbc40cd look good — all three concerns addressed correctly with regression tests. Deferring to a human for final approval since this changes the default proxy behavior (existing vaults flip to passthrough on upgrade) and touches security-sensitive credential broker paths.
Extended reasoning...
Overview
PR flips the MITM proxy default from "deny unmatched hosts" to "passthrough", with a new per-vault unmatched_host_policy setting (passthrough/deny) exposed via GET/PATCH /v1/vaults/{name}/settings and a Strict deny mode toggle. New vault_settings table (migration 043), new InjectResult.Passthrough flag plumbed through the audit log, and an extracted resolveVaultForAdminOrOwner auth prelude shared with rename/delete handlers.
Security risks
This is a fail-closed → fail-open default change for a credential broker — exactly the class of decision that benefits from a human signing off. Specific risks I scrutinized:
- Inject() error handling: now correctly distinguishes
sql.ErrNoRows(fall through to policy) from real errors (ErrServiceNotFound), with a new regression testTestInject_GetBrokerConfigError_FailsClosed. Matches my prior concern. - Settings PATCH: write path echoes the validated input rather than re-reading post-write — eliminates the UI/DB desync window on transient read failures.
- Settings GET: relaxed from admin-only to vault-member, so non-admin members see the real policy (toggle stays disabled via
canManage); PATCH retains admin/owner gating. New test asserts both halves. - Substitution and credential resolution paths are untouched on the matched-host hot path; passthrough leaves
Headers=nilso no header injection happens for unmatched hosts.
Level of scrutiny
High. The PR is operator-visible (default behavior flips on upgrade), modifies the MITM credential injection path, adds a new admin-gated mutation endpoint, and introduces a new auth helper that several handlers now depend on. Not a candidate for shadow approval even with my prior concerns resolved.
Other factors
- All three of my previous review comments resolved with corresponding tests; no outstanding feedback in the timeline.
- Test coverage looks proportional: brokercore unit tests for both policies, MITM integration test for passthrough forwarding, server tests for GET/PATCH including the non-admin member case.
1 participant
Summary
403 no_match), so a fresh install denied all traffic until an operator hand-curated services. This flips the default: unmatched hosts now forward as plain proxy traffic, with credentials only injected when a service matches.unmatched_host_policysetting (passthroughdefault,denyopt-in) lets vault admins restore the strict allowlist behavior — exposed viaGET/PATCH /v1/vaults/{name}/settingsand a Strict deny mode toggle in the Settings tab.Implementation notes
vault_settings, migration 043) so future toggles fit without schema churn.brokercore.UnmatchedHostPolicytyped enum +IsPassthroughflag onInjectResult; the existingAuth.Type=="passthrough"matched-service branch already produced the same shape, soforward.goonly needed a one-liner to thread the flag into the audit log.resolveVaultForAdminOrOwner; setting read-validate-default extracted toreadUnmatchedHostPolicy.cmd/skill_{cli,http}.md) updated as a pair — agents now know403 forbiddenonly fires under deny mode.Operator-visible behavior change: existing vaults flip to passthrough on upgrade. Set
unmatched_host_policy=denyper vault to restore the previous strict behavior.Test plan
make test— all packages green, including new brokercore policy tests, mitm passthrough integration test, and settings-handler testsgo build ./...andtsc --noEmitclean./agent-vault run --vault defaultwith no services configured —curl --proxy https://localhost:14322 https://example.comsucceeds (was 403)proposal_hint; toggle off and confirm passthrough resumespassthrough=trueslog field