Feat: Outbound TLS bridge (Phase 1) — decrypt agent egress HTTPS into the pipeline#522
Merged
Merged
Conversation
Self-contained design + implementation plan for the AuthBridge outbound TLS bridge (terminate agent egress TLS, run the existing pipeline on plaintext, re-originate verified TLS). Phase 1 is AuthBridge-only and test-only; Phase 2 (cert-manager CA) drops in via the CASource seam. Includes the empty tlsbridge package (doc.go). Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: Hai Huang <huang195@gmail.com>
Signed-off-by: Hai Huang <huang195@gmail.com>
… cache Signed-off-by: Hai Huang <huang195@gmail.com>
Signed-off-by: Hai Huang <huang195@gmail.com>
Signed-off-by: Hai Huang <huang195@gmail.com>
…cision branches Signed-off-by: Hai Huang <huang195@gmail.com>
Signed-off-by: Hai Huang <huang195@gmail.com>
Signed-off-by: Hai Huang <huang195@gmail.com>
…acade + sniff Peek Signed-off-by: Hai Huang <huang195@gmail.com>
http.Server.Serve dispatches the accepted conn to a goroutine and calls Accept again immediately; the old oneConnListener returned net.ErrClosed on that second Accept, so Serve — and ServeConn — returned before the request was handled, tearing the conn down mid-response. Make the second Accept block until the served conn is closed (via a close-notifying conn wrapper), so ServeConn serves the whole connection synchronously like a tunnel. The T5 keep-alive unit test masked this by running ServeConn in a goroutine; the synchronous transparent-bridge path (T7) needs it to block. Signed-off-by: Hai Huang <huang195@gmail.com>
Add the bridge branch to HandleTransparentConn: peek the sniffed ClientHello, classify, and on Terminate verify the upstream origin first (reversibility) then forge a leaf, terminate the agent TLS, and run the unchanged outbound pipeline via serveOutbound, relaying over the verified upstream client. Falls open to a re-dialed tunnel when upstream verify fails; auto-skips a host whose minted leaf the client rejects. Integration test drives the full decrypt -> pipeline -> re-originate loop end to end. Signed-off-by: Hai Huang <huang195@gmail.com>
Signed-off-by: Hai Huang <huang195@gmail.com>
Signed-off-by: Hai Huang <huang195@gmail.com>
Signed-off-by: Hai Huang <huang195@gmail.com>
…lidation An unknown ca_source (e.g. typo'd 'vault') previously fell through to the ephemeral CA silently; fail fast so a misconfigured file-CA deployment is a loud startup error rather than silent ephemeral mode. Signed-off-by: Hai Huang <huang195@gmail.com>
|
Warning Review limit reached
More reviews will be available in 34 minutes and 53 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?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 credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate. 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, the refill rate gradually slows as usage increases. The highest same-day bursts are limited more strictly. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughImplements a complete outbound TLS bridge (MITM) feature for AuthBridge: adds a new ChangesAuthBridge Phase 1 TLS Bridge
Sequence Diagram(s)sequenceDiagram
participant Agent as TLS Agent
participant Proxy as Forward Proxy
participant Decision
participant BridgeServe
participant Upstream
participant Terminator
participant Handler as Outbound Pipeline
participant Origin
rect rgba(100, 180, 100, 0.5)
note over Agent,Origin: Bridge-eligible TLS (port 443/8443, ClientHello detected)
Agent->>Proxy: raw TLS bytes (CONNECT or transparent)
Proxy->>Proxy: peekedConn.Peek(5 bytes)
Proxy->>Decision: Classify(host, port, first 5 bytes)
Decision-->>Proxy: Terminate verdict
Proxy->>Proxy: Check TLSBridge.Skip.Contains(host)
Proxy->>BridgeServe: verify and bridge
BridgeServe->>Upstream: HEAD https://authority (bounded timeout)
Upstream-->>BridgeServe: 200 OK (upstream TLS verified)
BridgeServe->>Terminator: Terminate(conn, host)
Terminator-->>BridgeServe: tls.Conn (forged leaf cert via Minter)
Agent->>Proxy: decrypted HTTP request (over tls.Conn)
Proxy->>Handler: serveOutbound(isBridge=true)
Handler->>Handler: plugins, auth normalization, session events
Handler->>Upstream: re-originate plaintext request
Upstream->>Origin: GET https://origin (verified TLS, system+injected roots)
Origin-->>Upstream: HTTP response
Handler-->>Agent: response
end
rect rgba(255, 150, 0, 0.5)
note over Proxy,Origin: Unverifiable upstream or pinned client → fail-open
alt Upstream TLS verification fails OR client rejects forged leaf
BridgeServe-->>Proxy: error
Proxy->>Proxy: close pre-dial, re-dial dst
Proxy->>Proxy: SkipSet.Add(host) if pinned
Proxy->>Agent: bidirectional raw tunnel (transparent or CONNECT)
end
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested reviewers
🚥 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)
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 |
The tlsbridge work promoted golang.org/x/net to a direct authlib dep and bumped it v0.51.0 -> v0.56.0. go.work masks the resulting cmd-module go.mod staleness locally, but CI builds each cmd binary with GOWORK=off, where 'go fmt/vet ./...' aborts with 'updates to go.mod needed'. Tidy the three cmd modules (proxy/envoy/lite) so their go.mod/go.sum reflect x/net v0.56.0. Signed-off-by: Hai Huang <huang195@gmail.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (3)
authbridge/authlib/config/config_test.go (1)
733-751: ⚡ Quick winExpand TLS bridge tests to pin validation failures.
Line 733 currently validates only the happy decode path. Please add negative cases for invalid
scope, invalidca_source, andca_source=filewithoutca_cert_path/ca_key_pathso Lines 60-68 inconfig.gostay regression-proof.🧪 Suggested test shape
+func TestConfig_TLSBridgeValidationErrors(t *testing.T) { + cases := []struct { + name string + yaml string + }{ + { + name: "invalid scope", + yaml: "mode: proxy-sidecar\ntls_bridge:\n enabled: true\n scope: internet\n", + }, + { + name: "invalid ca_source", + yaml: "mode: proxy-sidecar\ntls_bridge:\n enabled: true\n ca_source: vault\n", + }, + { + name: "file source missing paths", + yaml: "mode: proxy-sidecar\ntls_bridge:\n enabled: true\n ca_source: file\n", + }, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + p := filepath.Join(t.TempDir(), "cfg.yaml") + if err := os.WriteFile(p, []byte(tc.yaml), 0o600); err != nil { + t.Fatal(err) + } + if _, err := Load(p); err == nil { + t.Fatalf("expected Load error for %s", tc.name) + } + }) + } +}🤖 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/config/config_test.go` around lines 733 - 751, The test TestConfig_TLSBridgeBlockDecodes currently only validates the happy path for TLS bridge configuration decoding. Add three additional test functions to cover negative validation cases: create TestConfig_TLSBridgeInvalidScope to test rejected scope values, TestConfig_TLSBridgeInvalidCASource to test rejected ca_source values, and TestConfig_TLSBridgeFileMissingPaths to test ca_source set to file without providing ca_cert_path and ca_key_path. Each test should follow the same pattern as the existing test by writing invalid YAML configurations to a temporary file, calling Load() on the config path, and using t.Fatalf to assert that Load() returns an error (not nil) to ensure the validation logic in config.go lines 60-68 properly rejects these invalid configurations.authbridge/authlib/listener/forwardproxy/tlsbridge_integration_test.go (1)
71-80: 💤 Low valueConsider using the
listenSniffablehelper.This inline port-binding logic duplicates the
listenSniffable()helper defined at lines 406-417. Refactoring to use the helper would improve consistency with the other tests.♻️ Suggested refactor
- var ln net.Listener - for _, p := range []string{"8443", "8080"} { - if l, e := net.Listen("tcp", "127.0.0.1:"+p); e == nil { - ln = l - break - } - } - if ln == nil { + ln := listenSniffable() + if ln == nil { t.Skip("transparent bridge test needs a free sniffable port (8443 or 8080)") }🤖 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/listener/forwardproxy/tlsbridge_integration_test.go` around lines 71 - 80, The inline port-binding logic in the test that iterates through ports 8443 and 8080 to find a free listener duplicates functionality already provided by the listenSniffable() helper function. Replace the entire for loop and conditional check (the loop over ports and the ln == nil check) with a single call to listenSniffable(), which will handle finding a free sniffable port and skipping the test appropriately if none is available. This eliminates code duplication and improves consistency with other tests in the file.authbridge/authlib/tlsbridge/ca_test.go (1)
42-79: ⚡ Quick winAdd an RSA PKCS#1 case to cover the advertised key-format support.
Lines 42-79 validate EC PKCS#8 and SEC1, but not RSA PKCS#1. Since
parsePrivateKeyclaims PKCS#1 support, a dedicated case here would prevent regressions in that path.🧪 Example test extension
func TestFileSource_LoadsPKCS1AndPKCS8(t *testing.T) { @@ } + + t.Run("rsa-pkcs1", func(t *testing.T) { + key, err := rsa.GenerateKey(rand.Reader, 2048) + if err != nil { + t.Fatalf("rsa.GenerateKey: %v", err) + } + tmpl := &x509.Certificate{ + SerialNumber: big.NewInt(2), Subject: pkix.Name{CommonName: "t-rsa"}, + NotBefore: time.Now().Add(-time.Minute), NotAfter: time.Now().Add(time.Hour), + IsCA: true, KeyUsage: x509.KeyUsageCertSign, BasicConstraintsValid: true, + } + der, err := x509.CreateCertificate(rand.Reader, tmpl, tmpl, key.Public(), key) + if err != nil { + t.Fatalf("CreateCertificate: %v", err) + } + dir := t.TempDir() + certP := filepath.Join(dir, "tls.crt") + keyP := filepath.Join(dir, "tls.key") + if err := os.WriteFile(certP, pem.EncodeToMemory(&pem.Block{Type: "CERTIFICATE", Bytes: der}), 0o600); err != nil { + t.Fatalf("write cert: %v", err) + } + keyDER := x509.MarshalPKCS1PrivateKey(key) + if err := os.WriteFile(keyP, pem.EncodeToMemory(&pem.Block{Type: "RSA PRIVATE KEY", Bytes: keyDER}), 0o600); err != nil { + t.Fatalf("write key: %v", err) + } + if _, err := NewFileSource(certP, keyP); err != nil { + t.Fatalf("NewFileSource(rsa-pkcs1): %v", err) + } + }) }🤖 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/tlsbridge/ca_test.go` around lines 42 - 79, The table-driven test TestFileSource_LoadsPKCS1AndPKCS8 currently only tests EC key formats (PKCS#8 and SEC1) but does not test RSA PKCS#1 format despite parsePrivateKey claiming to support it. Add a new test case to the test table for RSA PKCS#1 format by generating an RSA private key using rsa.GenerateKey instead of ecdsa.GenerateKey, marshaling it using x509.MarshalPKCS1PrivateKey, and using "RSA PRIVATE KEY" as the PEM block type to ensure the RSA PKCS#1 code path is properly tested and prevent regressions.
🤖 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/tlsbridge/ca.go`:
- Around line 3-15: The NewFileSource function parses certificate and private
key files without validating that the certificate is suitable for CA operations
or that the keys match, which allows invalid configurations to pass
initialization silently. Add validation checks in the NewFileSource function
after parsing the certificate to verify that cert.IsCA is true and that
cert.KeyUsage includes x509.KeyUsageCertSign, then use the public key's Equal()
method (the idiomatic Go approach) to verify that the public key embedded in the
certificate matches the provided private key. These checks should return an
error immediately if any validation fails, catching configuration errors at
startup rather than during certificate signing operations.
In `@authbridge/authlib/tlsbridge/decision.go`:
- Around line 95-110: The SkipSet struct's map field m can grow unbounded as
hosts are added via the Add method, leading to memory accumulation in long-lived
processes. Implement a size cap mechanism by adding a maximum capacity field to
the SkipSet struct and checking the map size in the Add method before inserting
new entries. When the map reaches its capacity limit, either reject new
additions, evict the least recently used entry, or implement a TTL-based
eviction policy by tracking entry timestamps and periodically removing stale
entries. Update the NewSkipSet constructor to initialize the capacity limit.
In `@authbridge/authlib/tlsbridge/engine.go`:
- Around line 26-35: The code does not validate that caPEM is non-empty before
performing substring matching in the for loop. If caPEM is empty or contains
only whitespace, strings.Contains(string(b), want) will always return true since
an empty string is contained in all strings, causing a false positive "OK" trust
result to be logged. Add an early non-empty validation check on the want
variable immediately after the line want := strings.TrimSpace(string(caPEM)) and
before the for loop that iterates over the SSL_CERT_FILE, NODE_EXTRA_CA_CERTS,
and REQUESTS_CA_BUNDLE environment variables to ensure want has actual content
before proceeding with the substring matching logic.
In `@authbridge/authlib/tlsbridge/minter.go`:
- Around line 48-50: The ecdsa.GenerateKey call in the Minter constructor is
discarding the error return value, which allows nil to be assigned to the
leafKey field if key generation fails. This causes a panic later when
leafKey.Public() is called. Instead of ignoring the error with the underscore
operator, capture both the key and error from ecdsa.GenerateKey, check if an
error occurred, and return that error early from the constructor. This requires
adding an error return type to the constructor function if it doesn't already
have one, and returning an error value when key generation fails to fail fast at
construction time.
In `@authbridge/authlib/tlsbridge/terminator.go`:
- Around line 28-31: The conn.Handshake() call in the TLS Server handshake block
can block indefinitely if the client stalls during the handshake process. Before
calling conn.Handshake(), set a read deadline on the conn object using
SetReadDeadline() with a short timeout value. After the handshake completes
successfully (after the error check passes), clear the deadline by calling
SetReadDeadline() again with a zero time value to allow normal operation without
the timeout constraint.
In `@authbridge/authlib/tlsbridge/upstream.go`:
- Around line 25-35: The http.Client returned from this function is missing
timeout protections for upstream verification calls. Add a ResponseHeaderTimeout
field to the http.Transport configuration to prevent blocking when an upstream
server completes the TLS handshake but stalls on sending response headers, and
also add a Timeout field to the http.Client itself to ensure overall request
completion bounds. These timeouts should be set to reasonable values (e.g.,
ResponseHeaderTimeout around 10 seconds similar to TLSHandshakeTimeout, and
client Timeout slightly higher to accommodate the handshake and header
timeouts).
In `@authbridge/docs/superpowers/plans/2026-06-17-authbridge-tlsbridge-phase1.md`:
- Around line 28-29: Correct the logging guidance in the plan document at lines
28-29. The current text states that main.go uses slog and os.Exit rather than
log.Fatalf, but this conflicts with the actual current convention where
log.Fatal* is intentionally used in cmd-entrypoint bootstrap paths
(authbridge-cpex, authbridge-proxy, authbridge-envoy, authbridge-lite). Update
the documentation to accurately reflect that log.Fatal* is used in the main
wiring section, and add a note that this convention is intentional and will be
refactored during the shared Run() extraction phase mentioned in Task 6.
In `@authbridge/docs/superpowers/specs/2026-06-12-authbridge-tlsbridge-design.md`:
- Around line 68-76: Update the specification document to align package and
configuration names with the actual implementation. Replace all occurrences of
"authlib/mitm/" with "authlib/tlsbridge/" throughout the document. Additionally,
replace all instances of "mitm:" with "tls_bridge" and rename "MITMConfig" and
"isMITM" to "TLSBridgeConfig" to match the shipped interface. These changes need
to be applied across the table starting at line 68 and any other sections that
reference these identifiers (including lines 207 and 252) to maintain
consistency between the specification and the actual implementation.
---
Nitpick comments:
In `@authbridge/authlib/config/config_test.go`:
- Around line 733-751: The test TestConfig_TLSBridgeBlockDecodes currently only
validates the happy path for TLS bridge configuration decoding. Add three
additional test functions to cover negative validation cases: create
TestConfig_TLSBridgeInvalidScope to test rejected scope values,
TestConfig_TLSBridgeInvalidCASource to test rejected ca_source values, and
TestConfig_TLSBridgeFileMissingPaths to test ca_source set to file without
providing ca_cert_path and ca_key_path. Each test should follow the same pattern
as the existing test by writing invalid YAML configurations to a temporary file,
calling Load() on the config path, and using t.Fatalf to assert that Load()
returns an error (not nil) to ensure the validation logic in config.go lines
60-68 properly rejects these invalid configurations.
In `@authbridge/authlib/listener/forwardproxy/tlsbridge_integration_test.go`:
- Around line 71-80: The inline port-binding logic in the test that iterates
through ports 8443 and 8080 to find a free listener duplicates functionality
already provided by the listenSniffable() helper function. Replace the entire
for loop and conditional check (the loop over ports and the ln == nil check)
with a single call to listenSniffable(), which will handle finding a free
sniffable port and skipping the test appropriately if none is available. This
eliminates code duplication and improves consistency with other tests in the
file.
In `@authbridge/authlib/tlsbridge/ca_test.go`:
- Around line 42-79: The table-driven test TestFileSource_LoadsPKCS1AndPKCS8
currently only tests EC key formats (PKCS#8 and SEC1) but does not test RSA
PKCS#1 format despite parsePrivateKey claiming to support it. Add a new test
case to the test table for RSA PKCS#1 format by generating an RSA private key
using rsa.GenerateKey instead of ecdsa.GenerateKey, marshaling it using
x509.MarshalPKCS1PrivateKey, and using "RSA PRIVATE KEY" as the PEM block type
to ensure the RSA PKCS#1 code path is properly tested and prevent regressions.
🪄 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: 79ac147c-25e7-4feb-a981-2209d8186763
⛔ Files ignored due to path filters (1)
authbridge/authlib/go.sumis excluded by!**/*.sum
📒 Files selected for processing (23)
authbridge/authlib/config/config.goauthbridge/authlib/config/config_test.goauthbridge/authlib/go.modauthbridge/authlib/listener/forwardproxy/server.goauthbridge/authlib/listener/forwardproxy/sniff.goauthbridge/authlib/listener/forwardproxy/tlsbridge_integration_test.goauthbridge/authlib/listener/forwardproxy/transparent.goauthbridge/authlib/tlsbridge/ca.goauthbridge/authlib/tlsbridge/ca_test.goauthbridge/authlib/tlsbridge/decision.goauthbridge/authlib/tlsbridge/decision_test.goauthbridge/authlib/tlsbridge/doc.goauthbridge/authlib/tlsbridge/engine.goauthbridge/authlib/tlsbridge/minter.goauthbridge/authlib/tlsbridge/minter_test.goauthbridge/authlib/tlsbridge/serve.goauthbridge/authlib/tlsbridge/terminator.goauthbridge/authlib/tlsbridge/terminator_test.goauthbridge/authlib/tlsbridge/upstream.goauthbridge/authlib/tlsbridge/upstream_test.goauthbridge/cmd/authbridge-proxy/main.goauthbridge/docs/superpowers/plans/2026-06-17-authbridge-tlsbridge-phase1.mdauthbridge/docs/superpowers/specs/2026-06-12-authbridge-tlsbridge-design.md
…eview rossoctl#522) - TLSBridgeConfig.Validate now rejects malformed internal_cidrs entries so an operator CIDR typo fails loudly at load instead of being silently dropped by Decision (which would stop excluding an intended in-cluster range under scope=external). Adds TestTLSBridgeConfig_Validate covering scope/ca_source/ file-paths/CIDR cases. - Update the TLSBridgeConfig doc comment: 'MITM termination' -> 'TLS termination (formerly MITM)' for grep-consistency with the rename. Signed-off-by: Hai Huang <huang195@gmail.com>
…f-check; fail-fast keygen (review rossoctl#522) Address CodeRabbit findings on PR rossoctl#522: - Terminator.Terminate now sets a 10s handshake deadline (cleared on success so it doesn't bound the kept-alive served conn). Nothing upstream bounded the client conn — sniffHost clears its read deadline and the CONNECT peek has none — so a stalled client could pin the serving goroutine indefinitely. - bridgeServe bounds the pre-forge HEAD reachability/cert probe with a 10s context timeout (on the probe ONLY — NOT the relay, which must stay unbounded for streaming responses; adding ResponseHeaderTimeout to the shared upstream client would reintroduce the streaming-MCP 502 the main client's comment warns against). CodeRabbit's 'Critical' on upstream.go was not a TLS-bypass — RootCAs/ MinVersion/no-skip-verify are correct; the real issue was an unbounded verify. - RunTrustSelfCheck returns early on empty CA PEM (strings.Contains(b,"") would otherwise log a false 'trust OK'). - NewMinter no longer discards the ecdsa.GenerateKey error (latent nil-deref in mint); panics at construction on the effectively-impossible failure. - Plan doc: correct the main-logging guidance (cmd entrypoint uses log.Fatalf, not slog+os.Exit). Deferred (per reviewer / Phase 2): bound SkipSet + per-host verify cache (LRU/TTL); NewFileSource CA IsCA/key-match validation (cert-manager path). Spec mitm-identifier drift is covered by the spec's existing terminology disclaimer. Signed-off-by: Hai Huang <huang195@gmail.com>
pdettori
approved these changes
Jun 18, 2026
pdettori
left a comment
pdettori
left a comment
Member
There was a problem hiding this comment.
Solid Phase 1 implementation of the outbound TLS bridge. The security posture is sound: crypto/rand throughout, P-256 keys, upstream TLS verification never disabled, fall-open design thoroughly tested, and no secrets logged. The package decomposition is clean with well-defined boundaries.
Three hardening suggestions for Phase 2 consideration below. No blockers.
Areas reviewed: Go (security, concurrency, resource management), Docs
Commits: 18 commits, all signed-off
CI status: all passing
|
|
||
| // SkipSet is the runtime auto-skip set (hosts whose minted leaf the client | ||
| // rejected). Concurrent-safe; augments the static skip list. | ||
| type SkipSet struct { |
Member
There was a problem hiding this comment.
suggestion: SkipSet grows unboundedly as hosts are auto-skipped on handshake failure. Under adversarial conditions (many unique hosts failing verification), this map could exhaust memory. Consider adding a max-size cap or TTL-based expiration for Phase 2.
| // Terminate completes the server-side TLS handshake against the agent. host is | ||
| // the dialed name/IP, used to mint when the ClientHello carries no SNI. | ||
| func (t *Terminator) Terminate(client net.Conn, host string) (*tls.Conn, error) { | ||
| cfg := &tls.Config{ |
Member
There was a problem hiding this comment.
suggestion: The server-side tls.Config doesn't explicitly set MinVersion. Go defaults to TLS 1.2 since 1.18, but an explicit MinVersion: tls.VersionTLS12 would make the security posture visible and guard against future Go default changes.
| // keep-alive, negotiating h2 when ALPN selected it. It blocks until the | ||
| // connection is closed (like a tunnel), so callers can serve it synchronously. | ||
| func ServeConn(tconn *tls.Conn, handler http.Handler) { | ||
| srv := &http.Server{Handler: handler} |
Member
There was a problem hiding this comment.
suggestion: The per-conn http.Server has no IdleTimeout or ReadHeaderTimeout. If an agent holds a bridged connection open without sending data, the goroutine is pinned indefinitely. Consider IdleTimeout: 5*time.Minute for the h1.1 keep-alive path to prevent resource accumulation.
Add an explicit 'Protocol & port coverage' section to the design spec: the bridge is HTTPS-inspection (TLS + configured port + trusted CA), not general egress inspection. Tabulates the no-visibility categories (non-TLS TCP like SSH/DBs, TLS on non-standard ports, STARTTLS, non-TCP/QUIC, un-MITM-able pinned/mTLS/ECH), documents why the port gate is load-bearing (ServeConn assumes HTTP; bridging non-HTTP TLS like LDAPS/SMTPS would break the conn), and names connection-level egress policy as the backstop. Records envoy-sidecar bridging and non-HTTP protocols as non-goals. Signed-off-by: Hai Huang <huang195@gmail.com>
…,8443)
Adds tls_bridge.ports to TLSBridgeConfig (empty => {443,8443}), threads it
into Decision via main.go, and unifies the transparent listener's sniff gate
with the bridge's port set: shouldSniff() keeps its host-recovery heuristic
{80,443,8080,8443}, but the transparent path now ALSO sniffs whatever ports
the bridge intercepts (Decision.HandlesPort), so a configured non-standard
port (e.g. 9443) gets the peekable conn the bridge needs — no drift between
the two lists. Validation rejects out-of-range ports. New
TestTransparentBridge_CustomPort proves the bridge engages on a random
ephemeral port (outside shouldSniff's set) when configured.
Keep ports HTTP(S)-only: the bridge serves the decrypted stream as HTTP/1.1
or h2, so non-HTTP TLS (LDAPS/SMTPS/DB-over-TLS) must NOT be added here.
Signed-off-by: Hai Huang <huang195@gmail.com>
The tls_bridge schema is new/unreleased — tighten it now, before the operator renders it and an alpha pins it: - Collapse enabled+scope -> mode: disabled|enabled (matches operator tlsBridgeMode 1:1). Drop the external/all distinction and the whole Scope/isInternal/internal_cidrs machinery in Decision — the bridge intercepts everything eligible on the configured ports, period. Classify loses its now-unused ip param. - Drop ca_source + ca_cert_path + ca_key_path -> single ca_dir (cert-manager Secret key conventions tls.crt/tls.key/ca.crt). ephemeral was never usable in production (a real agent can't trust an unexported in-memory CA); it survives only as NewEphemeralSource for in-process tests, not a config knob. Production CA is always the operator-mounted file. - Rename skip_hosts -> passthrough_hosts to disambiguate from the existing listener.skip_hosts (full pipeline bypass vs bridge tunnel). Net: 6 fields -> mode + ca_dir + passthrough_hosts (+ existing ports, upstream_ca_bundle). No migration — nothing consumes the schema yet. Signed-off-by: Hai Huang <huang195@gmail.com>
As-built note + corrected YAML: mode|ca_dir|passthrough_hosts|ports| upstream_ca_bundle (dropped enabled/scope/internal_cidrs/ca_source/cert+key paths/Name-Constraints; ephemeral is test-only). Signed-off-by: Hai Huang <huang195@gmail.com>
…Secret Two operator-path hardenings, both load-time: - When tls_bridge.mode=enabled, Load() rewrites listener.session_api_addr to 127.0.0.1 (preserving the port) so the session API — which now carries decrypted bodies — isn't reachable by other pods. kubectl port-forward (abctl) still works (it targets the pod's loopback). No auth, no redaction yet; this just shrinks the reachable surface. - NewFileSource now fails loud on a misissued cert-manager Secret: rejects a non-CA cert (IsCA=false / missing KeyUsageCertSign) and a cert/key public-key mismatch. Previously these loaded 'fine' and silently failed at signing time → every call tunnels with no error. Turns a misconfigured Secret into a clear startup failure. Tests: TestForceLocalhost, TestLoad_TLSBridgeHardensSessionAPI, TestFileSource_RejectsNonCAOrMismatch. Signed-off-by: Hai Huang <huang195@gmail.com>
…idual MITM comment - SkipSet was an unbounded map. With scope/CIDR removed, all eligible egress flows through the bridge, so a flood of distinct SNIs each rejecting the forged leaf could grow it without bound. Add a 10m TTL (self-healing: a transiently-pinned/rotated client gets re-attempted) and a 4096 cap with oldest-expiry eviction on the cold Add path. Contains stays an RLock read (expired entries read as absent; Add reclaims them). Tests cover TTL expiry and the size bound. - main.go: drop a residual '(MITM)' in a comment (rename completed elsewhere). Defers (per reviewer): caching upstream-verify success per host is a pure optimization that trades verify-freshness for fewer probes — left for Phase 2. Signed-off-by: Hai Huang <huang195@gmail.com>
github-project-automation
Bot
moved this from New/ToDo
to Done
in Rossoctl Issue Prioritization
This was referenced Jun 18, 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
Adds an outbound TLS bridge to AuthBridge's Go forward proxy (
proxy-sidecar/lite): it terminates the agent's egress TLS by forging a per-origin leaf signed by an in-process CA, runs the existing, unchanged outbound pipeline on the decrypted request, and re-originates a separately-verified TLS connection to the real origin. This closes the gap where every content plugin (token-exchange, MCP/A2A parsing, IBAC, guardrails) silently did nothing on HTTPS egress because the proxy blind-tunneled TLS.The feature was previously prototyped under the name "MITM"; it is now the TLS bridge (terminate one TLS connection, run logic on plaintext, originate a fresh verified TLS — a bridge between two TLS connections). Package
tlsbridge, config blocktls_bridge:.Phase 1 is AuthBridge-only and test-only. It has no operator dependency and is off by default (
tls_bridge.enabled: false). A real operator-deployed agent does not yet trust the bridge CA, so its egress safely tunnels — the decrypt→pipeline→re-originate loop is proven by in-process integration tests. Live-agent trust (mountingca.crt+ trust env via the operator's cert-manager CA) is Phase 2 (sketched in the plan); Phase 1 is built so that path drops in via theCASourceseam with no changes to the bridge core.Design:
authbridge/docs/superpowers/specs/2026-06-12-authbridge-tlsbridge-design.mdPlan:
authbridge/docs/superpowers/plans/2026-06-17-authbridge-tlsbridge-phase1.mdWhat's included
authlib/tlsbridge/:CASource(ephemeral self-signed + file, PKCS#8/PKCS#1/SEC1 key parsing for cert-manager compatibility);Minter(per-host leaf, one shared P-256 leaf key, LRU+TTL cache);Decision(5-byte TLS-record-header validation, port/scope/CIDR/skip gates) +SkipSet;UpstreamClient(system + injected roots, neverInsecureSkipVerify);Terminator(ALPNh2+http/1.1);ServeConn(h2 + HTTP/1.1 keep-alive, blocks for the connection lifetime);Enginefacade + best-effort trust self-check.serveOutbound(re-originates via the dedicated upstream client when bridging — never the mesh-mTLS dialer that presents the agent SVID);bridgeServe(verify upstream before forging — reversibility); reversible bridge branch on both the transparent and CONNECT capture paths;peekedConn.Peek.tls_bridge:config block with validation;main.goconstructs the engine when enabled and runs the trust self-check.No-broken-calls guarantee
The headline safety property is that no bridge configuration ever turns a working agent call into a hard failure. Encoded as integration tests through the real capture path:
Test plan
go test ./...green inauthbridge/authlib(14tlsbridgeunit tests + 6 forward-proxy integration tests).go test ./tlsbridge/ ./listener/forwardproxy/ -raceclean.authbridge-proxybinary builds.go vetclean; no newgolangci-lintfindings;gofmtclean./secret) → real verified origin → response byte-intact, on both transparent and CONNECT paths.upstream_ca_bundle; system-roots-only rejects.Follow-ups (not blocking; tracked for Phase 2)
SkipSet(LRU/TTL) — currently unbounded.FileSource), agent-sideca.crthard-mount + trust env,:9094session-API hardening when bridging is enabled, CA rotation.Assisted-By: Claude (Anthropic AI) noreply@anthropic.com
Summary by CodeRabbit
tls_bridge) that can terminate and re-originate traffic for eligible HTTPS/TLS ports on both transparent andCONNECTflows.tls_bridgemode enablement, required CA directory, port allow-list validation, passthrough/skip hosts, and optional upstream CA bundle injection.golang.org/x/*module versions.