fix(authbridge): Address PR #500 review feedback

Adds construction-time guards, audit logging, and tests asked for in
the PR #500 review.

Skiphost matcher (authlib/listener/skiphost):
- Reject match-all patterns ("*", "**", whitespace-only) at New() so a
  single misconfigured entry cannot silently disable the entire
  outbound enforcement pipeline. Mirrors the bypass-pattern guard
  added to ibac in #496. Empirically verified: under .-delimited
  glob "*" matches every single-label hostname (which is how every
  short Kubernetes service name reaches the listener), and "**" is
  the unambiguous match-all. Patterns like "*.svc.cluster.local" or
  "*.*" are NOT match-all and remain accepted.
- Reject patterns containing ":" so an operator typo like
  "otel-collector:8335" fails fast at startup rather than silently
  never matching (Match strips the port from the incoming host
  before comparing, so colon-bearing patterns would never fire).
- Add MatchPattern(host) (string, bool) so listeners can attribute
  audit logs / counters to the specific raw operator-supplied
  pattern that fired. Match() is now a thin wrapper around it for
  callers that only want the boolean.

Listener skip-path audit logging:
- Each successful skip in extproc (handleOutbound, handleOutboundBody)
  and forwardproxy (handleRequest, handleConnect) now emits a
  structured slog INFO line carrying host, matched pattern, and
  method/path. Without this trail, a successful self-exemption left
  no trace anywhere -- no session event, no plugin invocation --
  because that is exactly what skip means.

Trust-model documentation (authlib/config/config.go):
- ListenerConfig.SkipHosts godoc now spells out that the matched
  value is agent-influenceable on the ext_proc and HTTP-forward
  paths (Envoy :authority / r.Host header) and only safe-by-
  construction on the CONNECT-tunnel path. Operators should not
  list a host they would want IBAC / token-exchange to deny on.

Hard-guard reminder (authlib/listener/forwardproxy/transparent.go):
- HandleTransparentConn (proxy-sidecar enforce-redirect mode)
  intentionally does NOT consult SkipHosts. Add a paragraph to its
  godoc stating the omission is deliberate so a future maintainer
  does not "fix the inconsistency" by adding a skip keyed on the
  agent-controlled SNI/Host bytes and open a real bypass of the
  hard egress guard.

Tests:
- New TestForwardProxy_SkipHosts_MatchesAgainstHostHeaderNotURL
  locks the trust-model contract for the HTTP forward path: the
  skip keys on the agent-supplied Host header, not on r.URL.Host.
- New TestForwardProxy_SkipHosts_CONNECT_BypassesPipeline exercises
  the previously-uncovered CONNECT skip path end-to-end.
- New TestNew_RejectsMatchAllStar / RejectsMatchAllDoubleStar /
  RejectsWhitespaceOnly / RejectsPortInPattern lock the construction-
  time guards, plus TestNew_AcceptsLeadingStar guards against a
  future rewrite that over-rejects and breaks operator-typical FQDN
  patterns.

CLAUDE.md gotcha #11:
- Narrow the LastIntent() wording: the pin protects against FIFO
  eviction only; expired or deleted sessions can still return nil.

Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com>
Signed-off-by: Kelly Abuelsaad <kaymar@gmail.com>

Author: kellyaa

authbridge/CLAUDE.md

@@ -590,7 +590,7 @@ See [`docs/framework-architecture.md`](docs/framework-architecture.md#9-config-h
       ```
       Any change to `listener.skip_hosts` requires a pod restart (same rule as other `listener.*` fields). Do NOT add hosts here that need IBAC / token-exchange policy applied — bypass means bypass.
 
-    - **Backstop — intent pin in the eviction policy**: even with `skip_hosts` empty, the session store now pins the most-recent inbound A2A request event against FIFO eviction. If the buffer overflows, every other event evicts in normal chronological order; the protected intent stays at its original timestamp, leaving a visible time gap in the timeline. Older intents from earlier turns are NOT pinned — only the latest one — so a multi-turn conversation with huge fan-out can't pile up stale intents and starve the buffer. This means IBAC's `LastIntent()` will return non-nil as long as ANY inbound A2A request landed in the bucket since session start, regardless of how chatty the outbound traffic is. The pin is defense-in-depth; reach for `skip_hosts` first when the offending traffic is identifiable infrastructure.
+    - **Backstop — intent pin in the eviction policy**: even with `skip_hosts` empty, the session store now pins the most-recent inbound A2A request event against FIFO eviction. If the buffer overflows, every other event evicts in normal chronological order; the protected intent stays at its original timestamp, leaving a visible time gap in the timeline. Older intents from earlier turns are NOT pinned — only the latest one — so a multi-turn conversation with huge fan-out can't pile up stale intents and starve the buffer. The pin protects against FIFO eviction only: IBAC's `LastIntent()` survives buffer overflow as long as the session is still alive and an inbound A2A request has landed in it, but can still return nil after session expiry, explicit deletion, or before the first inbound request arrives. The pin is defense-in-depth; reach for `skip_hosts` first when the offending traffic is identifiable infrastructure.
 
 ## DCO Sign-Off (Mandatory)
 

authbridge/authlib/config/config.go

@@ -358,6 +358,42 @@ type ListenerConfig struct {
 	//
 	// Empty list (default) preserves current behavior: every outbound
 	// host runs the pipeline and is eligible for session recording.
+	//
+	// Trust model — the value matched against SkipHosts is the
+	// destination Host as observed at the listener boundary, which is
+	// agent-influenceable in two of the three deployment shapes:
+	//
+	//   - ext_proc / envoy-sidecar: matches Envoy's `:authority`
+	//     (fallback `host` header). The agent sets these; Envoy may
+	//     rewrite them per its config, but ultimately the value is
+	//     "what the agent told Envoy it wanted to talk to."
+	//   - HTTP forward-proxy / proxy-sidecar: matches `r.Host` from
+	//     the HTTP request. The request is then dialed against
+	//     `r.URL`, so a forged Host that diverges from the real URL
+	//     host would skip-match yet send to the actual upstream.
+	//   - CONNECT-tunnel / proxy-sidecar: safer-by-construction —
+	//     `r.Host` IS the dial target. A forged Host cannot
+	//     skip-match while dialing elsewhere.
+	//
+	// Implication: do NOT list a destination here that you'd want
+	// IBAC / token-exchange to deny on. Skip means "the operator
+	// trusts every flow to this host enough to bypass the entire
+	// outbound enforcement pipeline." Limit entries to infrastructure
+	// destinations the agent should not be making policy decisions
+	// against in the first place (collector sidecars, log shippers).
+	//
+	// Each skip is logged at INFO with the matched host and pattern
+	// so an operator reviewing logs can see when a pattern fired and
+	// catch unexpected matches early. The `transparentproxy` listener
+	// (proxy-sidecar enforce-redirect mode) intentionally does NOT
+	// consult SkipHosts — that is the hard egress guard and must not
+	// be self-exemptable via the agent's outbound destination.
+	//
+	// Match-all patterns ("*", "**", whitespace-only) and patterns
+	// containing ":port" are rejected at startup so a single
+	// misconfigured entry can't silently disable all outbound
+	// enforcement. Mirrors the bypass-pattern guard added to ibac
+	// in #496.
 	SkipHosts []string `yaml:"skip_hosts" json:"skip_hosts"`
 }
 

authbridge/authlib/listener/extproc/server.go

@@ -531,7 +531,9 @@ func (s *Server) handleOutboundBody(stream extprocv3.ExternalProcessor_ProcessSe
 	// when the headers were already passed through — without checking
 	// here, a skip-listed host whose request carries a body would still
 	// run the pipeline on the body phase.
-	if s.SkipHosts.Match(pctx.Host) {
+	if pat, matched := s.SkipHosts.MatchPattern(pctx.Host); matched {
+		slog.Info("ext_proc: skip_hosts match (body phase) — bypassing pipeline + session recording",
+			"host", pctx.Host, "pattern", pat, "path", pctx.Path)
 		return allowBodyResponse(), nil
 	}
 

authbridge/authlib/listener/forwardproxy/server.go

@@ -210,7 +210,19 @@ func (s *Server) handleRequest(w http.ResponseWriter, r *http.Request) {
 	// response-phase work. RunFinish is also skipped (no defer
 	// registered) because the pipeline never ran and has nothing to
 	// finalize. See ListenerConfig.SkipHosts for motivation.
-	skipped := s.SkipHosts.Match(pctx.Host)
+	//
+	// Audit log: Match keys on r.Host (the agent-supplied Host header
+	// at the listener boundary), and the request is then dialed against
+	// r.URL via s.Client.Do(r). A forged Host that diverges from the
+	// dial target would skip-match yet send to a different upstream —
+	// the same trust shape as ext_proc's :authority. Logging the host
+	// + matched pattern at INFO leaves a per-skip audit trail so
+	// successful self-exemption isn't invisible.
+	pat, skipped := s.SkipHosts.MatchPattern(pctx.Host)
+	if skipped {
+		slog.Info("forward-proxy: skip_hosts match — bypassing pipeline + session recording",
+			"host", pctx.Host, "pattern", pat, "method", r.Method, "path", r.URL.Path)
+	}
 
 	// Finisher dispatch runs after every exit path. RunFinish is a
 	// no-op when pctx.dispatched is empty (pre-pipeline rejects), so
@@ -707,7 +719,17 @@ func (s *Server) handleConnect(w http.ResponseWriter, r *http.Request) {
 	// destination you'd want IBAC or token-exchange to deny on, that
 	// denial does not happen — the SkipHosts list is a "trusted
 	// infrastructure" surface, not a generic per-route policy knob.
-	skipped := s.SkipHosts.Match(pctx.Host)
+	//
+	// CONNECT is safer-by-construction than the HTTP path: r.Host on
+	// CONNECT is the dial target, so a forged Host header cannot
+	// skip-match while dialing elsewhere — the proxy dials the same
+	// "host:port" it matched. We still emit an audit log so a
+	// successful skip leaves a trace.
+	pat, skipped := s.SkipHosts.MatchPattern(pctx.Host)
+	if skipped {
+		slog.Info("forward-proxy: skip_hosts match (CONNECT) — opening tunnel without pipeline + recording",
+			"host", pctx.Host, "pattern", pat)
+	}
 
 	if !skipped {
 		defer func() {

authbridge/authlib/listener/forwardproxy/skiphost_test.go

@@ -1,8 +1,10 @@
 package forwardproxy
 
 import (
+	"bufio"
 	"context"
 	"io"
+	"net"
 	"net/http"
 	"net/http/httptest"
 	"strings"
@@ -196,5 +198,169 @@ func TestForwardProxy_SkipHosts_NilMatcherPreservesBehavior(t *testing.T) {
 	}
 }
 
-// silence unused-import warning if mustParseURL ends up only used here
-var _ = strings.TrimSpace
+// TestForwardProxy_SkipHosts_MatchesAgainstHostHeaderNotURL locks the
+// trust-model contract for the HTTP forward path: the skip match keys
+// on the request `Host` header (`r.Host`), not on the dial target
+// derived from `r.URL`. This is what huang195's review called out as
+// the agent-influenceable input — the test exists to make the behavior
+// observable so future maintainers see it when reading the test, and
+// so a refactor that flips this boundary (e.g. switching to r.URL.Host)
+// breaks loudly with a named test.
+//
+// Operators reading this: do NOT add a destination to skip_hosts that
+// you'd want IBAC / token-exchange to deny on. The skip is keyed on
+// agent-influenceable headers; the audit log emitted on each skip is
+// the only after-the-fact signal.
+//
+// The test scopes itself to the trust contract — "skip fires when
+// r.Host matches" — and does NOT assert what happens to the forwarded
+// request afterward. Go's http.Transport behavior with a proxy and a
+// forged Host header is implementation-detail that the trust model
+// shouldn't depend on; what matters is that the skip decision was
+// made on the agent-supplied value.
+func TestForwardProxy_SkipHosts_MatchesAgainstHostHeaderNotURL(t *testing.T) {
+	backend := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
+		w.WriteHeader(http.StatusOK)
+	}))
+	defer backend.Close()
+
+	store := session.New(5*time.Minute, 100, 0)
+	defer store.Close()
+
+	// Skip pattern matches a hostname that is NOT the dial target.
+	// The dial target is the backend (127.0.0.1:N from backend.URL);
+	// the pattern is a fictitious hostname.
+	skip, err := skiphost.New([]string{"infrastructure-only.example"})
+	if err != nil {
+		t.Fatalf("skiphost.New: %v", err)
+	}
+
+	proxy, mp := newMarkerServer(t, store, skip)
+	defer proxy.Close()
+
+	// Send a request whose Host header is the skip-listed pattern
+	// while r.URL points at the real backend. A normal Go http.Client
+	// would set Host from URL; we override it explicitly to model the
+	// adversarial case.
+	proxyClient := &http.Client{
+		Transport: &http.Transport{
+			Proxy: http.ProxyURL(mustParseURL(proxy.URL)),
+		},
+	}
+	req, err := http.NewRequest("GET", backend.URL+"/test", nil)
+	if err != nil {
+		t.Fatalf("NewRequest: %v", err)
+	}
+	req.Host = "infrastructure-only.example" // forged Host header
+
+	// Don't assert on resp/err — the trust-model claim is solely
+	// about the skip decision. Whatever Go's transport ends up doing
+	// with the divergent Host vs URL is incidental.
+	if resp, err := proxyClient.Do(req); err == nil && resp != nil {
+		resp.Body.Close()
+	}
+
+	// The skip matched against the forged Host, so the pipeline
+	// did NOT run. That's the trust-contract assertion: the agent
+	// chose the skip-match value via the Host header, not via the
+	// real dial target.
+	if mp.calls.Load() != 0 {
+		t.Errorf("forged Host: pipeline ran %d times, want 0 — skip matched on r.Host (the agent-supplied Host header), not r.URL.Host", mp.calls.Load())
+	}
+	if sessions := store.ListSessions(); len(sessions) != 0 {
+		t.Errorf("forged Host: %d session(s) recorded, want 0 — skip path bypasses recording", len(sessions))
+	}
+}
+
+// TestForwardProxy_SkipHosts_CONNECT_BypassesPipeline asserts that the
+// CONNECT-tunnel path honors SkipHosts. CONNECT is safer-by-construction
+// than the HTTP path because r.Host IS the dial target, but skip
+// behavior must still be exercised so a regression that disables the
+// CONNECT skip path is caught.
+func TestForwardProxy_SkipHosts_CONNECT_BypassesPipeline(t *testing.T) {
+	// Minimal echo server — stand-in for an HTTPS upstream we don't
+	// want to do TLS to in tests. The skip path opens a tunnel and
+	// shuttles bytes; we just need to prove bytes flow.
+	upstream, err := net.Listen("tcp", "127.0.0.1:0")
+	if err != nil {
+		t.Fatalf("listen upstream: %v", err)
+	}
+	defer upstream.Close()
+	go func() {
+		conn, err := upstream.Accept()
+		if err != nil {
+			return
+		}
+		defer conn.Close()
+		buf := make([]byte, 64)
+		n, _ := conn.Read(buf)
+		_, _ = conn.Write(append([]byte("echo:"), buf[:n]...))
+	}()
+
+	upstreamAddr := upstream.Addr().String()
+	upstreamHost, _, _ := net.SplitHostPort(upstreamAddr)
+	store := session.New(5*time.Minute, 100, 0)
+	defer store.Close()
+
+	skip, err := skiphost.New([]string{upstreamHost})
+	if err != nil {
+		t.Fatalf("skiphost.New: %v", err)
+	}
+
+	proxy, mp := newMarkerServer(t, store, skip)
+	defer proxy.Close()
+
+	// Drive CONNECT manually so we can observe the 200 + tunnel.
+	proxyAddr := strings.TrimPrefix(proxy.URL, "http://")
+	tunnel, err := net.DialTimeout("tcp", proxyAddr, 5*time.Second)
+	if err != nil {
+		t.Fatalf("dial proxy: %v", err)
+	}
+	defer tunnel.Close()
+	if _, err := tunnel.Write([]byte("CONNECT " + upstreamAddr + " HTTP/1.1\r\nHost: " + upstreamAddr + "\r\n\r\n")); err != nil {
+		t.Fatalf("write CONNECT: %v", err)
+	}
+
+	br := bufio.NewReader(tunnel)
+	line, err := br.ReadString('\n')
+	if err != nil {
+		t.Fatalf("read CONNECT response: %v", err)
+	}
+	if !strings.Contains(line, "200") {
+		t.Fatalf("CONNECT response = %q, want 200 (skip path must still establish the tunnel)", line)
+	}
+	// Drain headers.
+	for {
+		hdr, err := br.ReadString('\n')
+		if err != nil {
+			t.Fatalf("read headers: %v", err)
+		}
+		if hdr == "\r\n" || hdr == "\n" {
+			break
+		}
+	}
+
+	// Tunnel up — write some bytes and confirm the echo round-trips.
+	if _, err := tunnel.Write([]byte("hello")); err != nil {
+		t.Fatalf("write through tunnel: %v", err)
+	}
+	got := make([]byte, 32)
+	_ = tunnel.SetReadDeadline(time.Now().Add(2 * time.Second))
+	n, err := br.Read(got)
+	if err != nil && err != io.EOF {
+		t.Fatalf("read tunnel: %v", err)
+	}
+	if string(got[:n]) != "echo:hello" {
+		t.Errorf("tunnel echo = %q, want echo:hello", got[:n])
+	}
+
+	// The skipped CONNECT must NOT have run the outbound pipeline
+	// and must NOT have appended a session event.
+	if mp.calls.Load() != 0 {
+		t.Errorf("skipped CONNECT: pipeline ran %d times, want 0", mp.calls.Load())
+	}
+	if sessions := store.ListSessions(); len(sessions) != 0 {
+		t.Errorf("skipped CONNECT: %d session(s) recorded, want 0", len(sessions))
+	}
+}
+

authbridge/authlib/listener/forwardproxy/transparent.go

@@ -42,6 +42,17 @@ import (
 // hard control against a hostile one — only the IP is ground truth. Hard
 // enforcement would need IP-set allowlists or SNI/cert cross-checks.
 //
+// SkipHosts is intentionally NOT consulted here. listener.skip_hosts is an
+// ops convenience for the cooperative-egress paths (forward proxy + ext_proc),
+// where bypassing the pipeline on infrastructure traffic is fine because the
+// agent is trusted to honor HTTP_PROXY anyway. The transparent path exists
+// precisely as the hard egress guard against agents that route around the
+// cooperative paths, and pctx.Host here is recovered from agent-controlled
+// SNI/Host bytes on the wire — making it self-exemptable would defeat the
+// reason this listener exists. If you find yourself wanting to add a
+// SkipHosts check here to "match the other listeners," don't — that's the
+// failure mode this comment is explicitly trying to prevent.
+//
 // HandleTransparentConn owns clientConn's lifecycle and always closes it.
 func (s *Server) HandleTransparentConn(clientConn net.Conn, dst string) {
 	defer func() { _ = clientConn.Close() }()