fix(proxy): extract SNI before dial for SNI-deferred connections (#14)

Restructure handleConnect so that SNI-deferred connections extract the
TLS ClientHello SNI BEFORE dialing through the MITM proxy. Previously,
dial was called first with the raw IP as the CONNECT target, causing
goproxy to use the IP for the upstream TLS ServerName. The real server's
cert has DNS SANs (e.g. *.telegram.org), not IP SANs, so TLS verification
failed with "cannot validate certificate for <IP>".

New flow for SNI-deferred connections:
1. Send SOCKS5 CONNECT success (so client starts TLS handshake)
2. Peek ClientHello to extract SNI hostname
3. Update context FQDN with recovered hostname
4. Evaluate policy with hostname
5. Dial through MITM with hostname (not IP)

Also: refactor relay logic into reusable relayData method, extract
sniSaveRule helper to reduce duplication.

Also: revised ECH DNS hostname recovery plan from review feedback.

Author: nnemirovsky

docs/plans/20260409-ech-dns-hostname-recovery.md

@@ -0,0 +1,104 @@
+# ECH-aware hostname recovery via HTTPS/SVCB DNS records
+
+## Overview
+
+Add HTTPS/SVCB DNS record parsing to the DNS interceptor. When a DNS response contains HTTPS/SVCB records with `ipv4hint`/`ipv6hint` SvcParams, extract the IP addresses and store hostname -> IP mappings in a separate SVCB cache. This cache is always populated but only used as a hostname source when SNI disagrees with it (ECH connections where the real SNI is encrypted and the outer SNI is a dummy).
+
+The hostname recovery priority becomes:
+1. FQDN from SOCKS5 CONNECT (if client sends hostname)
+2. SNI from TLS ClientHello (happy path for most TLS)
+3. SVCB DNS hint (when SNI disagrees with SVCB cache for the same IP, indicating ECH)
+4. A/AAAA DNS reverse cache (existing, for non-TLS)
+5. Raw IP (last resort)
+
+## Context
+
+- DNS interceptor: `internal/proxy/dns.go` (HandleQuery, forwards queries, gates PopulateFromResponse on A/AAAA types)
+- DNS reverse cache: `internal/proxy/dns_reverse.go` (ReverseDNSCache, PopulateFromResponse, parseDNSName)
+- SNI policy check: `internal/proxy/server.go` (sniPolicyCheck, handleConnect)
+- DNS constants: `internal/proxy/dns_reverse.go` (dnsTypeA, dnsTypeAAAA, dnsHeaderLen)
+
+## Development Approach
+
+- **Testing approach**: Regular (code first, then tests)
+- CRITICAL: every task MUST include new/updated tests
+- CRITICAL: all tests must pass before starting next task
+
+## Testing Strategy
+
+- Unit tests for SVCB record parsing: `internal/proxy/dns_test.go`
+- Unit tests for SVCB cache lookup: `internal/proxy/dns_reverse_test.go` (new file)
+- Integration tests for ECH fallback flow: `internal/proxy/server_test.go`
+
+## Solution Overview
+
+1. Add a separate `svcbEntries` map to `ReverseDNSCache` with `StoreSVCB`/`LookupSVCB` methods
+2. Extend `PopulateFromResponse` to parse HTTPS (type 65) and SVCB (type 64) records, using `parseDNSName` for the target name field (DNS name compression applies per RFC 9460)
+3. Modify `HandleQuery` in `dns.go` to call `PopulateFromResponse` for HTTPS/SVCB query types (currently gated to A/AAAA only)
+4. In `sniPolicyCheck`: after extracting SNI, compare it against the SVCB-cached hostname for the destination IP. If they differ, prefer the SVCB hostname (the SNI is likely a dummy ECH outer SNI)
+
+**ECH detection approach**: no explicit ECH detection. Simply compare extracted SNI against SVCB-cached hostname for the same IP. If the SVCB cache says `149.154.167.220 -> example.com` but SNI says `cloudflare-ech.com`, prefer `example.com`. This works because SVCB hints are authoritative for the queried domain. No `HasECH` method needed.
+
+## Progress Tracking
+
+- Mark completed items with `[x]` immediately when done
+- Add newly discovered tasks with + prefix
+
+## Implementation Steps
+
+### Task 1: Add SVCB cache and parse HTTPS/SVCB records
+
+**Files:**
+- Modify: `internal/proxy/dns_reverse.go`
+- Modify: `internal/proxy/dns.go`
+- Create: `internal/proxy/dns_reverse_test.go`
+
+- [ ] Add constants: `dnsTypeHTTPS = 65`, `dnsTypeSVCB = 64`, `svcKeyIPv4Hint = 4`, `svcKeyIPv6Hint = 6`
+- [ ] Add `svcbEntries` map to `ReverseDNSCache` (separate from existing `entries`) with same TTL/bounds
+- [ ] Add `StoreSVCB(ip, hostname)` and `LookupSVCB(ip) string` methods
+- [ ] In `PopulateFromResponse`, add cases for HTTPS and SVCB record types
+- [ ] Parse SVCB RDATA: priority (2 bytes), target name (use `parseDNSName` for DNS compression), then SvcParam key-value pairs
+- [ ] Extract `ipv4hint` (key 4, addresses are 4 bytes each) and `ipv6hint` (key 6, 16 bytes each)
+- [ ] Store each hint IP -> hostname in `svcbEntries`
+- [ ] In `HandleQuery` (`dns.go`): extend the `PopulateFromResponse` gate to also include HTTPS and SVCB query types
+- [ ] Write tests: parse HTTPS record with ipv4hint and ipv6hint
+- [ ] Write tests: parse SVCB record, verify StoreSVCB/LookupSVCB
+- [ ] Write tests: malformed SVCB RDATA does not crash
+- [ ] Write tests: HTTPS record with no hints is a no-op
+- [ ] Run tests - must pass before next task
+
+### Task 2: Use SVCB cache as ECH fallback in SNI policy check
+
+**Files:**
+- Modify: `internal/proxy/server.go`
+- Test: `internal/proxy/server_test.go`
+
+- [ ] In `sniPolicyCheck`: after extracting SNI, look up the destination IP in SVCB cache via `dnsInterceptor.LookupSVCB(ipStr)`
+- [ ] If SVCB hostname exists AND differs from extracted SNI, prefer the SVCB hostname (ECH detected)
+- [ ] Log `[SNI-ECH] <ip>:<port> SNI=<outer> SVCB=<real> (using SVCB hostname)` for observability
+- [ ] Add comment block in `sniPolicyCheck` documenting the hostname recovery priority chain
+- [ ] Write test: SNI differs from SVCB cache -> SVCB hostname used
+- [ ] Write test: SNI matches SVCB cache -> SNI used (normal case)
+- [ ] Write test: no SVCB cache entry -> SNI used as-is
+- [ ] Write test: empty SNI with SVCB cache hit -> SVCB hostname used
+- [ ] Run tests - must pass before next task
+
+### Task 3: Verify acceptance criteria
+
+- [ ] Verify HTTPS/SVCB records populate the SVCB cache
+- [ ] Verify ECH connections recover hostname via SVCB cache
+- [ ] Verify non-ECH connections still use SNI
+- [ ] Run full test suite: `go test ./... -v -timeout 30s`
+- [ ] Run linter: `golangci-lint run ./...`
+
+### Task 4: [Final] Update documentation
+
+- [ ] Add one-line note to CLAUDE.md DNS section about HTTPS/SVCB parsing for ECH fallback
+- [ ] Move this plan to `docs/plans/completed/`
+
+## Post-Completion
+
+**Manual verification:**
+- Deploy to knuth and verify SVCB record parsing with a Cloudflare-protected domain
+- Test with an ECH-enabled server (e.g., `crypto.cloudflare.com`)
+- Verify sluice logs show `[SNI-ECH]` entries for ECH connections

…cs/plans/20260408-sni-defer-mitm-cert.md …ompleted/20260408-sni-defer-mitm-cert.mddocs/plans/20260408-sni-defer-mitm-cert.md renamed to docs/plans/completed/20260408-sni-defer-mitm-cert.md docs/plans/20260408-sni-defer-mitm-cert.md renamed to docs/plans/completed/20260408-sni-defer-mitm-cert.md

@@ -220,16 +220,20 @@ func (r *policyRuleSet) Allow(ctx context.Context, req *socks5.Request) (context
 	}
 
 	dest := req.DestAddr.FQDN
-	ipOnly := false // true when SOCKS5 CONNECT had no FQDN and DNS cache missed
+	ipOnly := false // true when SOCKS5 CONNECT had no FQDN
 	if dest == "" {
 		if req.DestAddr.IP != nil {
 			ipStr := req.DestAddr.IP.String()
-			// Recover hostname from DNS reverse cache. tun2proxy operates
-			// at the network level and sends IP-only CONNECT requests.
-			// The DNS interceptor caches IP -> hostname mappings from
-			// responses, so we can show hostnames in approval messages
-			// and evaluate hostname-based policy rules.
-			if r.dnsInterceptor != nil {
+			port := req.DestAddr.Port
+
+			// For TLS ports, always defer to SNI extraction (happy path).
+			// SNI from the TLS ClientHello is more reliable than the DNS
+			// reverse cache because it comes directly from the client and
+			// doesn't expire. DNS cache is used only for non-TLS protocols.
+			if isTLSPort(port) {
+				dest = ipStr
+				ipOnly = true
+			} else if r.dnsInterceptor != nil {
 				if hostname := r.dnsInterceptor.ReverseLookup(ipStr); hostname != "" {
 					dest = hostname
 				} else {
@@ -1082,6 +1086,51 @@ func dialThroughInjector(injectorAddr, host string, port int, authToken, pinID s
 // the TLS ClientHello SNI, re-evaluates policy with the recovered hostname,
 // and blocks on the approval flow if needed before relaying data.
 func (s *Server) handleConnect(ctx context.Context, writer io.Writer, request *socks5.Request) error {
+	// SNI-deferred connections: extract SNI BEFORE dialing so the MITM proxy
+	// uses the recovered hostname (not the raw IP) for the upstream TLS
+	// ServerName. Without this, the upstream TLS handshake fails because the
+	// real server's cert has DNS SANs (e.g. *.telegram.org) but not IP SANs.
+	//
+	// Hostname recovery priority:
+	//   1. FQDN from SOCKS5 CONNECT (if client sends hostname)
+	//   2. SNI from TLS ClientHello (this code path)
+	//   3. DNS reverse cache (fallback for non-TLS)
+	//   4. Raw IP (last resort)
+	clientReader := request.Reader
+	if deferred, _ := ctx.Value(ctxKeySNIDeferred).(bool); deferred {
+		// Send SOCKS5 CONNECT success early so the client starts the TLS
+		// handshake, allowing us to peek the ClientHello for SNI.
+		// Use the destination address as bind address. The client expects
+		// a valid address in the SOCKS5 reply, not 0.0.0.0:0.
+		bindAddr := &net.TCPAddr{IP: request.DestAddr.IP, Port: request.DestAddr.Port}
+		if sendErr := socks5.SendReply(writer, statute.RepSuccess, bindAddr); sendErr != nil {
+			return fmt.Errorf("failed to send reply: %w", sendErr)
+		}
+
+		// Set a read deadline so SNI peeking doesn't block forever if the
+		// client is slow to send the TLS ClientHello.
+		if conn, ok := writer.(net.Conn); ok {
+			conn.SetReadDeadline(time.Now().Add(10 * time.Second)) //nolint:errcheck
+			defer conn.SetReadDeadline(time.Time{})                //nolint:errcheck
+		}
+
+		var allow bool
+		clientReader, ctx, allow = s.sniPolicyCheckBeforeDial(ctx, request)
+		if !allow {
+			return nil
+		}
+
+		// Dial with the updated context (FQDN now contains the SNI hostname).
+		target, err := s.dial(ctx, "tcp", request.DestAddr.String())
+		if err != nil {
+			return fmt.Errorf("connect to %v failed: %w", request.RawDestAddr, err)
+		}
+		defer target.Close() //nolint:errcheck
+
+		return s.relayData(clientReader, writer, target)
+	}
+
+	// Normal (non-deferred) path: dial first, then relay.
 	target, err := s.dial(ctx, "tcp", request.DestAddr.String())
 	if err != nil {
 		msg := err.Error()
@@ -1102,15 +1151,11 @@ func (s *Server) handleConnect(ctx context.Context, writer io.Writer, request *s
 		return fmt.Errorf("failed to send reply: %w", sendErr)
 	}
 
-	// SNI-deferred policy check: peek client bytes for TLS ClientHello.
-	clientReader := request.Reader
-	if deferred, _ := ctx.Value(ctxKeySNIDeferred).(bool); deferred {
-		clientReader = s.sniPolicyCheck(ctx, request, target)
-		if clientReader == nil {
-			return nil // connection closed by policy check
-		}
-	}
+	return s.relayData(clientReader, writer, target)
+}
 
+// relayData bidirectionally copies data between the client and target.
+func (s *Server) relayData(clientReader io.Reader, writer io.Writer, target net.Conn) error {
 	errCh := make(chan error, 2)
 	go func() {
 		_, cpErr := io.Copy(target, clientReader)
@@ -1135,18 +1180,25 @@ func (s *Server) handleConnect(ctx context.Context, writer io.Writer, request *s
 	return nil
 }
 
-// sniPolicyCheck peeks the first bytes from the client to extract TLS SNI,
-// then re-evaluates policy with the recovered hostname. Returns a reader
-// that replays the peeked bytes followed by the rest of the client stream.
-// Returns nil if the connection should be closed (policy denied or error).
-func (s *Server) sniPolicyCheck(ctx context.Context, request *socks5.Request, target net.Conn) io.Reader {
-	buf, sni, err := peekSNI(request.Reader, 4096)
+// sniPolicyCheckBeforeDial peeks the first bytes from the client to extract
+// TLS SNI, re-evaluates policy with the recovered hostname, and updates the
+// context FQDN so that the subsequent dial uses the hostname (not the raw IP)
+// for the MITM upstream connection. Called BEFORE dial for SNI-deferred
+// connections. Returns the client reader (with peeked bytes prepended), the
+// updated context, and whether the connection should proceed.
+func (s *Server) sniPolicyCheckBeforeDial(ctx context.Context, request *socks5.Request) (io.Reader, context.Context, bool) {
+	buf, sni, err := peekSNI(request.Reader, 8192)
 	if err != nil || sni == "" {
-		// Not TLS or no SNI. Fall through with original data.
+		// Not TLS or no SNI. Fall through with original data and IP-based context.
+		hexPrefix := ""
+		if len(buf) >= 6 {
+			hexPrefix = fmt.Sprintf(" first6=%02x", buf[:6])
+		}
+		log.Printf("[SNI-PEEK] no SNI extracted (err=%v, bufLen=%d, sni=%q%s)", err, len(buf), sni, hexPrefix)
 		if len(buf) > 0 {
-			return io.MultiReader(bytes.NewReader(buf), request.Reader)
+			return io.MultiReader(bytes.NewReader(buf), request.Reader), ctx, true
 		}
-		return request.Reader
+		return request.Reader, ctx, true
 	}
 
 	sni = strings.TrimRight(sni, ".")
@@ -1156,6 +1208,9 @@ func (s *Server) sniPolicyCheck(ctx context.Context, request *socks5.Request, ta
 
 	log.Printf("[SNI] %s -> %s:%d (recovered hostname via TLS ClientHello)", ipStr, sni, port)
 
+	// Update context FQDN so dial() uses the hostname for the MITM upstream.
+	ctx = context.WithValue(ctx, ctxKeyFQDN, sni)
+
 	// Populate DNS reverse cache for future connections.
 	if s.dnsInterceptor != nil {
 		s.dnsInterceptor.StoreReverse(ipStr, sni)
@@ -1167,83 +1222,65 @@ func (s *Server) sniPolicyCheck(ctx context.Context, request *socks5.Request, ta
 		eng = s.rules.engine.Load()
 	}
 	verdict := eng.Evaluate(sni, port)
+	reader := io.MultiReader(bytes.NewReader(buf), request.Reader)
 
 	switch verdict {
 	case policy.Allow:
 		log.Printf("[SNI->ALLOW] %s:%d (hostname %s matched allow rule)", ipStr, port, sni)
-		return io.MultiReader(bytes.NewReader(buf), request.Reader)
+		return reader, ctx, true
 	case policy.Deny:
 		log.Printf("[SNI->DENY] %s:%d (hostname %s matched deny rule)", ipStr, port, sni)
-		_ = target.Close()
-		return nil
+		return nil, ctx, false
 	case policy.Ask:
 		if s.rules.broker == nil {
 			log.Printf("[SNI->DENY] %s:%d (hostname %s: ask treated as deny, no broker)", ipStr, port, sni)
-			_ = target.Close()
-			return nil
+			return nil, ctx, false
 		}
 		log.Printf("[SNI->ASK] %s:%d (hostname %s: waiting for approval)", ipStr, port, sni)
 		timeout := time.Duration(eng.TimeoutSec) * time.Second
 		proto := DetectProtocol(port)
 		resp, reqErr := s.rules.broker.Request(sni, port, proto.String(), timeout)
 		if reqErr != nil {
 			log.Printf("[SNI->DENY] %s:%d (hostname %s: approval timeout: %v)", ipStr, port, sni, reqErr)
-			_ = target.Close()
-			return nil
+			return nil, ctx, false
 		}
 		switch resp {
 		case channel.ResponseAllowOnce:
 			log.Printf("[SNI->ALLOW] %s:%d (hostname %s: user approved once)", ipStr, port, sni)
-			return io.MultiReader(bytes.NewReader(buf), request.Reader)
+			return reader, ctx, true
 		case channel.ResponseAlwaysAllow:
 			log.Printf("[SNI->ALLOW+SAVE] %s:%d (hostname %s: user approved always)", ipStr, port, sni)
-			s.rules.reloadMu.Lock()
-			func() {
-				defer s.rules.reloadMu.Unlock()
-				if s.rules.store != nil {
-					if _, storeErr := s.rules.store.AddRule("allow", store.RuleOpts{Destination: sni, Ports: []int{port}, Source: "approval"}); storeErr != nil {
-						log.Printf("[WARN] failed to persist allow rule for %s:%d: %v", sni, port, storeErr)
-					}
-					if newEng, recompErr := policy.LoadFromStore(s.rules.store); recompErr != nil {
-						log.Printf("[WARN] failed to recompile engine after SNI always-allow: %v", recompErr)
-					} else if valErr := newEng.Validate(); valErr != nil {
-						log.Printf("[WARN] engine validation failed after SNI always-allow: %v", valErr)
-					} else {
-						s.rules.engine.Store(newEng)
-					}
-				}
-			}()
-			return io.MultiReader(bytes.NewReader(buf), request.Reader)
+			s.sniSaveRule("allow", sni, port)
+			return reader, ctx, true
 		case channel.ResponseAlwaysDeny:
 			log.Printf("[SNI->DENY+SAVE] %s:%d (hostname %s: user denied always)", ipStr, port, sni)
-			s.rules.reloadMu.Lock()
-			func() {
-				defer s.rules.reloadMu.Unlock()
-				if s.rules.store != nil {
-					if _, storeErr := s.rules.store.AddRule("deny", store.RuleOpts{Destination: sni, Ports: []int{port}, Source: "approval"}); storeErr != nil {
-						log.Printf("[WARN] failed to persist deny rule for %s:%d: %v", sni, port, storeErr)
-					}
-					if newEng, recompErr := policy.LoadFromStore(s.rules.store); recompErr != nil {
-						log.Printf("[WARN] failed to recompile engine after SNI always-deny: %v", recompErr)
-					} else if valErr := newEng.Validate(); valErr != nil {
-						log.Printf("[WARN] engine validation failed after SNI always-deny: %v", valErr)
-					} else {
-						s.rules.engine.Store(newEng)
-					}
-				}
-			}()
-			_ = target.Close()
-			return nil
+			s.sniSaveRule("deny", sni, port)
+			return nil, ctx, false
 		default:
 			log.Printf("[SNI->DENY] %s:%d (hostname %s: user denied)", ipStr, port, sni)
-			_ = target.Close()
-			return nil
+			return nil, ctx, false
 		}
 	default:
-		// Default verdict (typically deny). Use it.
 		log.Printf("[SNI->DENY] %s:%d (hostname %s: default deny)", ipStr, port, sni)
-		_ = target.Close()
-		return nil
+		return nil, ctx, false
+	}
+}
+
+// sniSaveRule persists an allow or deny rule from an SNI-based approval.
+func (s *Server) sniSaveRule(verdict, dest string, port int) {
+	s.rules.reloadMu.Lock()
+	defer s.rules.reloadMu.Unlock()
+	if s.rules.store != nil {
+		if _, storeErr := s.rules.store.AddRule(verdict, store.RuleOpts{Destination: dest, Ports: []int{port}, Source: "approval"}); storeErr != nil {
+			log.Printf("[WARN] failed to persist %s rule for %s:%d: %v", verdict, dest, port, storeErr)
+		}
+		if newEng, recompErr := policy.LoadFromStore(s.rules.store); recompErr != nil {
+			log.Printf("[WARN] failed to recompile engine after SNI %s: %v", verdict, recompErr)
+		} else if valErr := newEng.Validate(); valErr != nil {
+			log.Printf("[WARN] engine validation failed after SNI %s: %v", verdict, valErr)
+		} else {
+			s.rules.engine.Store(newEng)
+		}
 	}
 }