fix(tlsbridge): bound handshake + upstream-verify; guard empty-CA self-check; fail-fast keygen (review #522)

Address CodeRabbit findings on PR #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>

Author: huang195

authbridge/authlib/listener/forwardproxy/server.go

@@ -42,6 +42,11 @@ const maxBodySize = 1 << 20 // 1MB — matches Envoy's default per_stream_buffer
 // are for.
 const streamReadIdleTimeout = 5 * time.Minute
 
+// upstreamVerifyTimeout bounds the pre-forge HEAD reachability/cert probe in
+// bridgeServe. It applies to that probe only — never to the relay, which must
+// stay unbounded so streaming responses aren't cut off.
+const upstreamVerifyTimeout = 10 * time.Second
+
 // Server is an HTTP forward proxy that performs token exchange on outbound requests.
 //
 // OutboundPipeline is a holder so the bound pipeline can be hot-swapped
@@ -451,8 +456,18 @@ func (s *Server) serveOutbound(w http.ResponseWriter, r *http.Request, isBridge
 func (s *Server) bridgeServe(client net.Conn, authority, host string) bool {
 	// 1) Verify upstream reachability + cert via the dedicated client, BEFORE forging.
 	//    HEAD avoids GET side-effects; a non-2xx status still returns err==nil (cert
-	//    verified), which is all we need. Only a transport/TLS error fails here.
-	resp, err := s.TLSBridge.Upstream.Head("https://" + authority)
+	//    verified), which is all we need. Only a transport/TLS error fails here. The
+	//    verify is bounded by its own context timeout so a slow/stalled origin can't
+	//    pin the bridging goroutine — the timeout is on this probe ONLY, not on the
+	//    relay (which must stay unbounded for streaming responses).
+	ctx, cancel := context.WithTimeout(context.Background(), upstreamVerifyTimeout)
+	defer cancel()
+	req, err := http.NewRequestWithContext(ctx, http.MethodHead, "https://"+authority, nil)
+	if err != nil {
+		slog.Info("tls-bridge passthrough", "host", host, "reason", "upstream-verify", "error", err)
+		return false
+	}
+	resp, err := s.TLSBridge.Upstream.Do(req)
 	if err != nil {
 		slog.Info("tls-bridge passthrough", "host", host, "reason", "upstream-verify", "error", err)
 		return false // fall back to plain tunnel — agent's own e2e TLS still reaches origin

authbridge/authlib/tlsbridge/engine.go

@@ -24,6 +24,12 @@ type Engine struct {
 // env is set, so this simply notes that egress will safely tunnel.
 func RunTrustSelfCheck(caPEM []byte) {
 	want := strings.TrimSpace(string(caPEM))
+	if want == "" {
+		// An empty CA would make strings.Contains below always true → a false
+		// "trust self-check OK". Guard so an empty/misconstructed CA is visible.
+		slog.Warn("tls-bridge trust self-check skipped: empty CA PEM")
+		return
+	}
 	for _, env := range []string{"SSL_CERT_FILE", "NODE_EXTRA_CA_CERTS", "REQUESTS_CA_BUNDLE"} {
 		p := os.Getenv(env)
 		if p == "" {

authbridge/authlib/tlsbridge/minter.go

@@ -45,7 +45,12 @@ func NewMinter(src CASource, o MinterOpts) *Minter {
 	if o.LeafTTL <= 0 {
 		o.LeafTTL = 24 * time.Hour
 	}
-	key, _ := ecdsa.GenerateKey(elliptic.P256(), rand.Reader)
+	key, err := ecdsa.GenerateKey(elliptic.P256(), rand.Reader)
+	if err != nil {
+		// P-256 keygen from crypto/rand effectively never fails; if it does, fail
+		// fast at construction rather than nil-deref later in mint().
+		panic(fmt.Errorf("tlsbridge: generate leaf key: %w", err))
+	}
 	return &Minter{
 		src: src, max: o.CacheMax, ttl: o.LeafTTL, leafKey: key,
 		ll: list.New(), items: make(map[string]*list.Element),

authbridge/authlib/tlsbridge/terminator.go

@@ -3,8 +3,13 @@ package tlsbridge
 import (
 	"crypto/tls"
 	"net"
+	"time"
 )
 
+// handshakeTimeout bounds the server-side TLS handshake against the agent so a
+// stalled/malicious client cannot pin the serving goroutine indefinitely.
+const handshakeTimeout = 10 * time.Second
+
 // Terminator wraps a sniffed client conn as a tls.Server, using the Minter to
 // forge a per-SNI leaf. ALPN offers h2 + http/1.1.
 type Terminator struct {
@@ -26,8 +31,12 @@ func (t *Terminator) Terminate(client net.Conn, host string) (*tls.Conn, error)
 		},
 	}
 	conn := tls.Server(client, cfg)
+	// Bound the handshake; clear the deadline on success so it does not leak
+	// into the served-connection (keep-alive) lifetime.
+	_ = conn.SetDeadline(time.Now().Add(handshakeTimeout))
 	if err := conn.Handshake(); err != nil {
 		return nil, err
 	}
+	_ = conn.SetDeadline(time.Time{})
 	return conn, nil
 }

authbridge/docs/superpowers/plans/2026-06-17-authbridge-tlsbridge-phase1.md

@@ -25,7 +25,7 @@
 - `(s *Server) HandleTransparentConn(clientConn net.Conn, dst string)` (`transparent.go:46`): `name, wrapped := sniffHost(clientConn); clientConn = wrapped` (`transparent.go:69-70`); local var `host` = `net.JoinHostPort(name, port)` or `dst` (`transparent.go:67-78`); dials `upstream` against `dst` with `defer upstream.Close()` (`transparent.go:115-120`); `tunnel(clientConn, upstream)` (`transparent.go:125`).
 - `sniffHost(conn net.Conn) (string, net.Conn)` (`sniff.go`): returns the recovered host string and a replayable `*peekedConn{net.Conn; r *bufio.Reader}` (`sniff.go:148-153`). It uses `br.Peek(...)` (non-consuming) internally but **discards the peeked bytes** and has **no exported peek method**. Task 6 adds `(c *peekedConn) Peek(n int) ([]byte, error)`.
 - `config.Config` (`config/config.go`): optional pointer blocks `MTLS *MTLSConfig \`yaml:"mtls,omitempty"\`` (`:33`), `SPIFFE *SPIFFEConfig \`yaml:"spiffe,omitempty"\`` (`:39`), each with a `Validate()` method (`:86`, `:133`) called from the top-level loader (`cfg.MTLS.Validate()` `:441`, `cfg.SPIFFE.Validate()` `:460`). The loader is **`func Load(path string) (*Config, error)`** (`:415`) — it takes a **file path, not bytes**. `config.go` already imports `fmt` and `os`.
-- main wiring (`cmd/authbridge-proxy/main.go`): `fpMTLS = &forwardproxy.MTLSOptions{...}` (`:243`); `fpSrv, err := forwardproxy.NewServer(outboundH, sessions, fpMTLS)` (`:256`); then `fpSrv.SkipHosts = …` (`:268`), `fpSrv.Shared = sharedStore` (`:272`); `transparentproxy.NewServer(fp.HandleTransparentConn)` (`:424`). main uses **`log/slog` + `os.Exit`** (imports `log/slog` `:17`, `os` `:20`) — **not** `log.Fatalf`.
+- main wiring (`cmd/authbridge-proxy/main.go`): `fpMTLS = &forwardproxy.MTLSOptions{...}` (`:243`); `fpSrv, err := forwardproxy.NewServer(outboundH, sessions, fpMTLS)` (`:256`); then `fpSrv.SkipHosts = …` (`:268`), `fpSrv.Shared = sharedStore` (`:272`); `transparentproxy.NewServer(fp.HandleTransparentConn)` (`:424`). NOTE: `cmd/authbridge-proxy/main.go` imports `"log"` and uses **`log.Fatalf`** for boot-fatal errors (no `os.Exit` in the boot path) — match that convention, not `slog`+`os.Exit`.
 - `pipeline`: `Action{Type ActionType, Violation *Violation}` with `Reject` const (`pipeline/action.go:11-24`); `SharedStore` is an interface (`pipeline/context.go:35`); `Context` (`pipeline/context.go:93`). The response phase (`RunResponse`/`RunResponseFrame`) lives inside the `handleRequest` body that Task 6 moves wholesale — no new response-phase code is authored.
 
 ---
@@ -1436,7 +1436,7 @@ func RunTrustSelfCheck(caPEM []byte) {
 }
 ```
 
-- [ ] **Step 6: Wire `main.go`** — beside the `fpMTLS` construction (`main.go:243`), build the engine when enabled; set the field after `NewServer` (mirroring `fpSrv.SkipHosts`/`fpSrv.Shared`). main uses `slog`+`os.Exit`, not `log.Fatalf`:
+- [ ] **Step 6: Wire `main.go`** — beside the `fpMTLS` construction (`main.go:243`), build the engine when enabled; set the field after `NewServer` (mirroring `fpSrv.SkipHosts`/`fpSrv.Shared`). main uses `log.Fatalf` for boot-fatal errors — match it (the sketch below shows `slog.Error`+`os.Exit`; use `log.Fatalf` to fit the file):
 
 ```go
 	var bridge *tlsbridge.Engine
@@ -1528,6 +1528,6 @@ Phase 2 (operator) is intentionally **not** bite-sized here. When Phase 1 lands,
 ## Self-review notes
 
 - **Spec coverage:** every Phase-1 spec item maps to a task — `tlsbridge/` units (T1–T5), serveOutbound + full-pipeline reuse (T6), reversible decision + upstream-verify-first (T7/T8 `bridgeServe`), auto-skip (T7/T9), interception scope gate (T3/T10), upstream client with injected roots (T4), h2 + keep-alive (T5), config + wiring + self-check (T10), no-broken-calls (T9). Operator items → Phase 2.
-- **Fixes folded in from the end-to-end audit (2026-06-17):** `Load(path)` not `Load(bytes)` (T10 temp file); `slog`+`os.Exit` not `log.Fatalf` (T10); `(*peekedConn).Peek` added since `sniffHost` discards the ClientHello (T6) — replaces the undefined `peekFirstBytes`/`peek(5)`; `hostOnly`/`portOf`/`nameOrIP` helpers defined (T6/T7); CONNECT `peekedConn` constructed **with** its `bufio.Reader` (T8); `serveOutbound` selects `s.TLSBridge.Upstream` for bridge re-origination, never the mesh-mTLS `s.Client` (T6, **core correctness**); authority (`host:port`) carried so non-443 origins re-originate and verify on the right port (T7/T8); upstream verify uses `HEAD` to avoid `GET /` side-effects (T7); `FileSource` multi-format key parsing for 2c (T1); pre-dialed upstream closed and re-dialed only on fall-open (T7/T8).
+- **Fixes folded in from the end-to-end audit (2026-06-17):** `Load(path)` not `Load(bytes)` (T10 temp file); main `log.Fatalf` for boot-fatal (T10 — corrected: the cmd entrypoint uses `log.Fatalf`, not `slog`+`os.Exit`); `(*peekedConn).Peek` added since `sniffHost` discards the ClientHello (T6) — replaces the undefined `peekFirstBytes`/`peek(5)`; `hostOnly`/`portOf`/`nameOrIP` helpers defined (T6/T7); CONNECT `peekedConn` constructed **with** its `bufio.Reader` (T8); `serveOutbound` selects `s.TLSBridge.Upstream` for bridge re-origination, never the mesh-mTLS `s.Client` (T6, **core correctness**); authority (`host:port`) carried so non-443 origins re-originate and verify on the right port (T7/T8); upstream verify uses `HEAD` to avoid `GET /` side-effects (T7); `FileSource` multi-format key parsing for 2c (T1); pre-dialed upstream closed and re-dialed only on fall-open (T7/T8).
 - **Known minor limitations (documented, not bugs):** CONNECT-path `scope=external` keys on a hostname (no IP) so it won't classify a hostname-addressed in-cluster service as internal — the transparent path (which has the SO_ORIGINAL_DST IP) does; the bridge pays one upstream HEAD per agent connection (reversibility cost) plus the relay handshake.
 - **Type consistency:** `GetCertificateForHost(host string)`, `Terminate(client net.Conn, host string)`, `Classify(host, ip string, port int, first []byte) (Verdict, string)`, `Engine{Decision,Term,Skip,Upstream,CAPEM}`, `bridgeServe(client net.Conn, authority, host string) bool`, `serveOutbound(w, r, isBridge bool)` are used consistently across tasks.