feat(proxy): add destination-scoped credential brokering - #667
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds credential proxy configuration and schema support, a loopback HTTPS proxy with scoped credential substitution and redaction, ChangesCredential Proxy
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant ChildProcess
participant RunningProxy
participant ProxyPolicy
participant UpstreamHTTPS
ChildProcess->>RunningProxy: CONNECT destination:443
RunningProxy->>ProxyPolicy: authorize request and inject placeholder
ProxyPolicy-->>RunningProxy: approved request
RunningProxy->>UpstreamHTTPS: forward HTTPS request
UpstreamHTTPS-->>RunningProxy: response body and headers
RunningProxy-->>ChildProcess: redacted response
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
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 |
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Greptile SummaryAdds destination-scoped credential brokering through a local TLS interception proxy.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains; all three previously reported defects are corrected in the current code. Important Files Changed
Reviews (8): Last reviewed commit: "fix(proxy): return HTTP errors over TLS" | Re-trigger Greptile |
Instruction counts
2 benchmark(s) above the 1% gate: Only instruction counts gate. Wall clock is shown for context — on identical hardware it moves 4-20% run to run. Measured by tak — instruction-counted CLI benchmarks, stored in this repository's git notes.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (3)
crates/fnox-core/src/config.rs (1)
917-922: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd a merge test for the proxy replace-as-unit behavior.
The rationale for replacing
proxywholesale (rather than merging field-by-field) is sound, but unlike the analogousmcp.secretsreplace-as-unit logic — which has bothmcp_secrets_overlay_replaces_base_not_appendsandmcp_secrets_overlay_without_secrets_preserves_basetests — there's no test verifying that an overlay's[proxy]section fully replaces (not merges with) a base config's rules, nor one verifying the baseproxyis preserved when the overlay omits[proxy]entirely. Given this is a security-relevant merge path, it's worth locking in with a regression test.♻️ Suggested test (mirrors the existing `mcp_secrets_overlay_*` tests)
#[test] fn proxy_overlay_replaces_base_rules_not_appends() { let base = Config { proxy: Some(ProxyConfig { rules: vec![ProxyRule { secret: "A".into(), domain: "a.example.com".into(), env: None, header: default_proxy_header(), methods: vec![], paths: vec![], placeholder: None, }], ..ProxyConfig::default() }), ..Config::new() }; let overlay = Config { proxy: Some(ProxyConfig { rules: vec![ProxyRule { secret: "B".into(), domain: "b.example.com".into(), env: None, header: default_proxy_header(), methods: vec![], paths: vec![], placeholder: None, }], ..ProxyConfig::default() }), ..Config::new() }; let merged = Config::merge_configs(base, overlay).unwrap(); let rules = merged.proxy.unwrap().rules; assert_eq!(rules.len(), 1, "overlay must replace, not append, base rules"); assert_eq!(rules[0].secret, "B"); }🤖 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 `@crates/fnox-core/src/config.rs` around lines 917 - 922, Add merge regression tests near the existing mcp_secrets_overlay_* tests for Config::merge_configs: verify an overlay with proxy rules replaces the base rules entirely, and verify an overlay that omits proxy preserves the base proxy. Reuse the existing ProxyConfig, ProxyRule, default_proxy_header, and Config::new patterns, asserting both rule count/content and preservation behavior.src/proxy.rs (2)
550-554: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winBuild the upstream
reqwest::Clientonce, not per request.A fresh client per proxied request rebuilds the TLS config and discards the connection pool, adding a full TCP+TLS handshake to every call — noticeable for agent workloads that issue many API requests. Construct it once (e.g. in
RunningProxy::startor aOnceCell) and share it via the policy/connection context. It also gives you a natural place to set a request timeout, which is currently absent on this outbound call.🤖 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 `@src/proxy.rs` around lines 550 - 554, Move upstream reqwest::Client construction out of the per-request path around the proxy request handler and initialize it once during RunningProxy::start or equivalent shared context setup. Pass and reuse that client through the policy/connection context for all proxied requests, preserving the existing HTTP/redirect configuration, and configure an appropriate outbound request timeout when constructing it.
823-905: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a negative test for path scoping.
The suite covers wrong method and wrong header but never asserts that a placeholder is rejected for a path outside
paths. That is the case most likely to regress (see theliteral_separatornote oncompile_paths) — e.g.authorize("api.github.com", "GET", "/repos/other/private", ...)should error, and withliteral_separator(true)a/repos/example/*rule should not match/repos/example/a/b.🤖 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 `@src/proxy.rs` around lines 823 - 905, Add a negative path-scoping test alongside rejects_placeholder_on_wrong_method and rejects_placeholder_in_another_header, using the existing policy and Authorization placeholder to assert authorize rejects an allowed method on an outside path such as /repos/other/private. Also cover that a wildcard path rule with literal_separator(true) does not match a nested path like /repos/example/a/b, reusing the relevant policy/config symbols.
🤖 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 `@docs/guide/proxy.md`:
- Around line 54-55: Update the proxy description near the startup flow to call
it a loopback-only HTTPS (CONNECT) proxy rather than an HTTP/HTTPS proxy, and
add a Current Limits bullet stating that plain http:// proxy requests are
rejected; keep the existing CONNECT behavior and terminology consistent with
handle_connection.
In `@src/commands/proxy.rs`:
- Around line 181-190: Ensure the proxy cleanup always executes when
command.status() fails, including spawn errors: capture the result of
command.status().await with its existing FnoxError::CommandExecutionFailed
mapping, call running.shutdown().await before propagating the result, then
return the command status outcome.
- Around line 134-150: Extend the credential scrub list in the proxy command’s
environment-removal loop to include DOPPLER_TOKEN, FNOX_DOPPLER_TOKEN,
FOKS_BOT_TOKEN, and FNOX_FOKS_BOT_TOKEN; also add
PROTON_PASS_PERSONAL_ACCESS_TOKEN and FNOX_PROTON_PASS_PERSONAL_ACCESS_TOKEN so
these ambient credentials are removed before execution.
In `@src/proxy.rs`:
- Around line 695-714: The proxy’s header and body size checks occur only after
full buffering, so oversized inputs can exceed the intended memory bounds. In
src/proxy.rs lines 695-714, update read_line_limited to enforce remaining during
reading via a bounded or incremental read, preserving the existing errors and
return behavior. In src/proxy.rs lines 583-591, replace response.bytes() with
response.chunk() streaming, track accumulated bytes, and stop immediately once
MAX_BODY_BYTES is exceeded.
- Around line 386-408: Update the accept loop in the spawned task around
listener.accept so transient accept errors are logged and the loop continues
instead of breaking. Preserve shutdown handling and only terminate the loop when
the shutdown receiver completes; include the accept error in the diagnostic log.
- Around line 512-520: Update proxy_http_request and the surrounding
handle_connection TLS error paths to flush or shut down the AsyncWrite stream
before returning. After successful write_error or response-body writes, await
AsyncWriteExt::shutdown(stream), and ensure TLS-path errors also flush/shut down
before the connection is dropped so all buffered response data is sent.
- Around line 278-297: Update compile_paths to construct each glob with
GlobBuilder configured with literal_separator(true) before building it, so
wildcard patterns do not match across directory boundaries. Preserve the
existing validation, error propagation, and GlobSetBuilder flow; do not enable
recursive matching unless explicitly documented and tested.
---
Nitpick comments:
In `@crates/fnox-core/src/config.rs`:
- Around line 917-922: Add merge regression tests near the existing
mcp_secrets_overlay_* tests for Config::merge_configs: verify an overlay with
proxy rules replaces the base rules entirely, and verify an overlay that omits
proxy preserves the base proxy. Reuse the existing ProxyConfig, ProxyRule,
default_proxy_header, and Config::new patterns, asserting both rule
count/content and preservation behavior.
In `@src/proxy.rs`:
- Around line 550-554: Move upstream reqwest::Client construction out of the
per-request path around the proxy request handler and initialize it once during
RunningProxy::start or equivalent shared context setup. Pass and reuse that
client through the policy/connection context for all proxied requests,
preserving the existing HTTP/redirect configuration, and configure an
appropriate outbound request timeout when constructing it.
- Around line 823-905: Add a negative path-scoping test alongside
rejects_placeholder_on_wrong_method and rejects_placeholder_in_another_header,
using the existing policy and Authorization placeholder to assert authorize
rejects an allowed method on an outside path such as /repos/other/private. Also
cover that a wildcard path rule with literal_separator(true) does not match a
nested path like /repos/example/a/b, reusing the relevant policy/config symbols.
🪄 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: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 73d6adc5-95f9-4ce0-8d53-82379f03355b
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (18)
Cargo.tomlcrates/fnox-core/src/config.rsdocs/.vitepress/config.mjsdocs/cli/commands.jsondocs/cli/index.mddocs/cli/proxy.mddocs/cli/proxy/rules.mddocs/cli/proxy/run.mddocs/guide/how-it-works.mddocs/guide/proxy.mddocs/public/schema.jsondocs/reference/configuration.mdfnox.usage.kdlsrc/commands/mod.rssrc/commands/proxy.rssrc/daemon.rssrc/lib.rssrc/proxy.rs
| "DENO_CERT", | ||
| ] { | ||
| command.env(key, &ca_path); | ||
| } |
There was a problem hiding this comment.
Permissive egress TLS trust broken
Medium Severity
proxy run sets SSL_CERT_FILE, CURL_CA_BUNDLE, REQUESTS_CA_BUNDLE, and similar variables to only the ephemeral fnox CA. That trust store works for MITM’d rule domains, but permissive egress tunnels other hosts with end-to-end TLS, so clients need public CAs and commonly fail verification or refuse connections.
Reviewed by Cursor Bugbot for commit 56e5a81. Configure here.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
There are 2 total unresolved issues (including 1 from previous review).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit f8ccf05. Configure here.
1 participant
Summary
[proxy]configuration with strict/permissive egress and exact domain, method, path, and header rulesfnox proxy rulesandfnox proxy run -- <command>with session placeholders and an ephemeral loopback TLS interception proxyCurrent limits
Testing
mise run test:cargomise run lintSKIP_KEYCHAIN_TESTS=1 mise exec -- fnox exec -- bats test(707 tests; keychain integration skipped because this container has no Secret Service)curlsmoke test against httpbin verified upstream substitution and downstream redactionAI-assisted — Tool: Codex; model: OpenAI/GPT-5; version: unavailable.
Note
High Risk
Adds TLS interception and in-memory credential injection with a broad attack surface; misconfiguration or proxy bypass could leak secrets despite placeholders and scrubbing.
Overview
Introduces a credential proxy so agent workloads can call external APIs without receiving real secret values in their environment.
Configuration and CLI: New
[proxy]block with strict/permissive egress, audit logging, and per-rule matching on domain, HTTP method, path globs, and header.fnox proxy rulesprints effective policy;fnox proxy run -- <cmd>resolves rule secrets (plus provider auth dependencies), starts a session-local loopback CONNECT proxy with an ephemeral CA, and runs the child with placeholders and standard proxy/CA env vars. Overlay configs replace[proxy]wholesale so layered rules cannot silently broaden authority.Proxy behavior: For matched HTTPS destinations on port 443, fnox terminates TLS, substitutes placeholders in allowed headers only when route rules match, forwards via a no-redirect upstream client, and redacts reflected secrets in responses. Unmatched destinations are blocked in strict mode or tunneled in permissive mode. The child environment is scrubbed of profile secrets and a large set of ambient provider credential env vars.
Docs and deps: User guide, CLI reference, schema, and usage spec are updated;
rcgen,rustls-native-certs,tokio-rustls, andreqwestsupport the TLS stack.Reviewed by Cursor Bugbot for commit 466cdcb. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by CodeRabbit
proxyCLI tooling, includingproxy rules(inspect effective rules) andproxy run(run a command through the local proxy with placeholders).proxyconfiguration options (egress strict/permissive, auditing, and per-rule matching/substitution).