diff --git a/docs/DEPLOYMENT.md b/docs/DEPLOYMENT.md index 36e986c..70f0bf1 100644 --- a/docs/DEPLOYMENT.md +++ b/docs/DEPLOYMENT.md @@ -273,6 +273,12 @@ that caller's wait, not a shared native client's physical dial. This does not add immediate peer-disconnect detection during a pending TCP dial, and is not included in v1.0.1. +Source builds close any non-nil TCP connection returned alongside a custom +dialer's setup error, before replying with failure. The original error still +determines the SOCKS5 or HTTP failure response; a cleanup error does not replace +it. A nil connection with no error is also rejected. This covers SOCKS5 TCP, +HTTP CONNECT and ordinary HTTP forwarding, and is not included in v1.0.1. + For SOCKS5 TCP and HTTP CONNECT tunnels, `--idle-timeout` (default `5m`) measures inactivity across both directions: an active download or upload does not need reverse-direction application traffic to stay open. A blocked write diff --git a/internal/proxy/http.go b/internal/proxy/http.go index 9af86d8..15db51f 100644 --- a/internal/proxy/http.go +++ b/internal/proxy/http.go @@ -388,7 +388,16 @@ func (s *HTTPServer) dialContext(ctx context.Context, network, address string) ( defer cancel() conn, err = s.cfg.dialer.DialContext(dialCtx, network, address) } - if err != nil { + if err != nil || conn == nil { + // Own a custom dialer's non-nil failed result before discarding it. + // A Close error must not replace the original dial error or its + // timeout classification. + if conn != nil { + _ = conn.Close() + } + if err == nil { + err = errors.New("proxy: TCP dialer returned a nil connection") + } return nil, err } return &activityConn{Conn: conn, timeout: s.cfg.idleTimeout}, nil diff --git a/internal/proxy/socks5.go b/internal/proxy/socks5.go index 17b2348..4d80798 100644 --- a/internal/proxy/socks5.go +++ b/internal/proxy/socks5.go @@ -158,7 +158,16 @@ func (s *SOCKS5Server) serveConnect(client net.Conn, request socksRequest) { if s.cfg.handshakeTimeout > 0 { _ = client.SetWriteDeadline(time.Now().Add(s.cfg.handshakeTimeout)) } - if err != nil { + if err != nil || upstream == nil { + // A custom dialer may return a connection along with a setup error. + // Close only that failed result, never the shared dialer, and retain + // the original error for the SOCKS reply even if Close also fails. + if upstream != nil { + _ = upstream.Close() + } + if err == nil { + err = errors.New("proxy: TCP dialer returned a nil connection") + } _ = writeSOCKSReply(client, socksReplyForError(err), nil) return } diff --git a/internal/proxy/tcp_dial_error_ownership_test.go b/internal/proxy/tcp_dial_error_ownership_test.go new file mode 100644 index 0000000..6aa4be8 --- /dev/null +++ b/internal/proxy/tcp_dial_error_ownership_test.go @@ -0,0 +1,320 @@ +package proxy + +import ( + "bufio" + "context" + "errors" + "fmt" + "io" + "net" + "net/http" + "sync" + "sync/atomic" + "testing" + "time" + + "github.com/cppla/autocar/internal/transport" +) + +// Close always closes the real endpoint before returning its injected error. +// Counting calls verifies ownership independently of the peer's EOF signal. +type tcpDialErrorOwnedConn struct { + net.Conn + closeCalls atomic.Int32 + closeError error +} + +func (c *tcpDialErrorOwnedConn) Close() error { + c.closeCalls.Add(1) + _ = c.Conn.Close() + return c.closeError +} + +func tcpDialErrorOwnershipWait(t *testing.T, done <-chan struct{}, label string) bool { + t.Helper() + select { + case <-done: + return true + case <-time.After(2 * time.Second): + t.Errorf("%s did not finish", label) + return false + } +} + +// Existing HTTP Shutdown can report a duplicate listener close. Accept only +// that known close result, including wrappers; a joined unrelated failure or +// context deadline must still fail the ownership fixture. +func tcpDialErrorOwnershipOnlyClosed(err error) bool { + if joined, ok := err.(interface{ Unwrap() []error }); ok { + for _, child := range joined.Unwrap() { + if !tcpDialErrorOwnershipOnlyClosed(child) { + return false + } + } + return true + } + if wrapped, ok := err.(interface{ Unwrap() error }); ok { + return tcpDialErrorOwnershipOnlyClosed(wrapped.Unwrap()) + } + return errors.Is(err, net.ErrClosed) +} + +func TestTCPDialErrorOwnershipFrontends(t *testing.T) { + for _, frontend := range []string{"socks-connect", "http-connect", "http-forward"} { + t.Run(frontend, func(t *testing.T) { + for _, typedTimeout := range []bool{false, true} { + name := "ordinary-error-close-timeout" + setupError := errors.New("post-dial setup failed") + var closeError error = &net.OpError{Op: "close", Net: "tcp", Err: context.DeadlineExceeded} + wantSOCKS, wantHTTP := byte(socksReplyGeneralFailure), http.StatusBadGateway + if typedTimeout { + name = "typed-timeout-close-ordinary-error" + setupError = &net.OpError{Op: "dial", Net: "tcp", Err: context.DeadlineExceeded} + closeError = errors.New("closing failed result failed") + wantSOCKS, wantHTTP = socksReplyTTLExpired, http.StatusGatewayTimeout + } + t.Run(name, func(t *testing.T) { + testTCPDialErrorOwnershipFrontend(t, frontend, setupError, closeError, wantSOCKS, wantHTTP) + }) + } + }) + } +} + +func testTCPDialErrorOwnershipFrontend(t *testing.T, frontend string, setupError, closeError error, wantSOCKS byte, wantHTTP int) { + t.Helper() + originListener, err := net.ListenTCP("tcp", &net.TCPAddr{IP: net.IPv4(127, 0, 0, 1)}) + if err != nil { + t.Fatal(err) + } + t.Cleanup(func() { _ = originListener.Close() }) + if err := originListener.SetDeadline(time.Now().Add(2 * time.Second)); err != nil { + t.Fatal(err) + } + returned := make(chan *tcpDialErrorOwnedConn, 1) + dialStarted, dialDone := make(chan struct{}), make(chan struct{}) + dialer := transport.DialFunc(func(ctx context.Context, network, address string) (net.Conn, error) { + close(dialStarted) + defer close(dialDone) + conn, err := (&net.Dialer{}).DialContext(ctx, network, address) + if err != nil { + return nil, err + } + observed := &tcpDialErrorOwnedConn{Conn: conn, closeError: closeError} + returned <- observed + return observed, setupError + }) + cfg := Config{Dialer: dialer, DialTimeout: 2 * time.Second, HandshakeTimeout: time.Second, IdleTimeout: 2 * time.Second, MaxConnections: 1} + listener, err := net.Listen("tcp", "127.0.0.1:0") + if err != nil { + t.Fatal(err) + } + t.Cleanup(func() { _ = listener.Close() }) + var shutdown func(context.Context) error + var tracker *connTracker + var client, peer net.Conn + var upstream *tcpDialErrorOwnedConn + serveDone, handlerDone, handlerEntered := make(chan struct{}), make(chan struct{}), make(chan struct{}) + var serveError error + workersStarted := false + t.Cleanup(func() { + if client != nil { + _ = client.Close() + } + if shutdown != nil { + ctx, cancel := context.WithCancel(context.Background()) + cancel() + _ = shutdown(ctx) + } + _ = listener.Close() + if workersStarted { + tcpDialErrorOwnershipWait(t, serveDone, "frontend Serve cleanup") + select { + case <-handlerEntered: + tcpDialErrorOwnershipWait(t, handlerDone, "frontend handler cleanup") + default: + } + } + select { + case <-dialStarted: + tcpDialErrorOwnershipWait(t, dialDone, "custom dial cleanup") + default: + } + if upstream == nil { + select { + case upstream = <-returned: + default: + } + } + if upstream != nil { + // Cleanup must also release a regressed leaked socket, without + // adding a call to the production ownership counter. + _ = upstream.Conn.Close() + } + if peer != nil { + _ = peer.Close() + } + }) + if frontend == "socks-connect" { + server, err := NewSOCKS5Server(cfg) + if err != nil { + t.Fatal(err) + } + shutdown, tracker = server.Shutdown, server.lifecycle.tracker + managed, err := server.lifecycle.manage(listener) + if err != nil { + t.Fatal(err) + } + workersStarted = true + go func() { + defer close(serveDone) + conn, err := managed.Accept() + if err != nil { + serveError = err + return + } + close(handlerEntered) + server.serveConn(conn) + close(handlerDone) + }() + } else { + server, err := NewHTTPServer(cfg) + if err != nil { + t.Fatal(err) + } + shutdown, tracker = server.Shutdown, server.lifecycle.tracker + var startOnce, doneOnce sync.Once + server.server.Handler = http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + startOnce.Do(func() { close(handlerEntered) }) + defer doneOnce.Do(func() { close(handlerDone) }) + server.ServeHTTP(w, r) + }) + workersStarted = true + go func() { defer close(serveDone); serveError = server.Serve(listener) }() + } + client, err = net.DialTimeout("tcp", listener.Addr().String(), time.Second) + if err != nil { + t.Fatal(err) + } + if err := client.SetDeadline(time.Now().Add(2 * time.Second)); err != nil { + t.Fatal(err) + } + if frontend == "socks-connect" { + socksGreeting(t, client, nil) + host, port := splitAddress(t, originListener.Addr().String()) + mustWrite(t, client, domainSOCKSRequest(socksCommandConnect, host, port)) + if reply := readSOCKSReply(t, client); reply != wantSOCKS { + t.Errorf("SOCKS reply = %d, want original setup error reply %d", reply, wantSOCKS) + } + } else { + method := http.MethodConnect + request := fmt.Sprintf("CONNECT %s HTTP/1.1\r\nHost: %s\r\nConnection: close\r\n\r\n", originListener.Addr(), originListener.Addr()) + if frontend == "http-forward" { + method = http.MethodGet + request = fmt.Sprintf("GET http://%s/ HTTP/1.1\r\nHost: %s\r\nConnection: close\r\n\r\n", originListener.Addr(), originListener.Addr()) + } + mustWrite(t, client, []byte(request)) + response, err := http.ReadResponse(bufio.NewReader(client), &http.Request{Method: method}) + if err != nil { + t.Fatal(err) + } + _, readError := io.ReadAll(response.Body) + closeError := response.Body.Close() + if response.StatusCode != wantHTTP || readError != nil || closeError != nil { + t.Errorf("HTTP failure = status %d (want %d) read %v close %v", response.StatusCode, wantHTTP, readError, closeError) + } + } + if !tcpDialErrorOwnershipWait(t, handlerDone, "failed request handler") || !tcpDialErrorOwnershipWait(t, dialDone, "completed custom dial") { + return + } + ctx, cancel := context.WithTimeout(context.Background(), time.Second) + defer cancel() + if err := tracker.wait(ctx); err != nil { + t.Fatal("failed request retained its downstream slot: ", err) + } + if err := shutdown(ctx); err != nil { + if !tcpDialErrorOwnershipOnlyClosed(err) { + t.Fatal("shutdown after failed request: ", err) + } + t.Logf("accepted existing duplicate-listener-close Shutdown result: %v", err) + } + if !tcpDialErrorOwnershipWait(t, serveDone, "frontend after Shutdown") { + return + } + if serveError != nil { + t.Fatal(serveError) + } + select { + case upstream = <-returned: + case <-time.After(time.Second): + t.Fatal("custom dialer did not return its created TCP socket") + } + if got := upstream.closeCalls.Load(); got != 1 { + t.Errorf("frontend closed returned failed socket %d times, want exactly once", got) + } + peer, err = originListener.AcceptTCP() + if err != nil { + t.Fatal(err) + } + if err := peer.SetReadDeadline(time.Now().Add(200 * time.Millisecond)); err != nil { + t.Fatal(err) + } + n, readError := peer.Read(make([]byte, 1)) + if n != 0 || !errors.Is(readError, io.EOF) { + t.Errorf("failed returned TCP socket remained open after handler/Shutdown: %d/%v", n, readError) + } + // Independently closing the actual endpoint must yield EOF, even on a + // regressed build. It leaves the wrapper's ownership call count untouched. + _ = upstream.Conn.Close() + if err := peer.SetReadDeadline(time.Now().Add(time.Second)); err != nil { + t.Fatal(err) + } + n, readError = peer.Read(make([]byte, 1)) + if n != 0 || !errors.Is(readError, io.EOF) { + t.Errorf("explicit TCP cleanup did not yield peer EOF: %d/%v", n, readError) + } +} + +func TestHTTPDialErrorOwnershipPreservesOriginalError(t *testing.T) { + for _, setupError := range []error{errors.New("setup failed"), &net.OpError{Op: "dial", Net: "tcp", Err: context.DeadlineExceeded}} { + t.Run(setupError.Error(), func(t *testing.T) { + local, peer := net.Pipe() + t.Cleanup(func() { _ = local.Close(); _ = peer.Close() }) + owned := &tcpDialErrorOwnedConn{Conn: local, closeError: errors.New("close failed")} + server, err := NewHTTPServer(Config{Dialer: transport.DialFunc(func(context.Context, string, string) (net.Conn, error) { + return owned, setupError + })}) + if err != nil { + t.Fatal(err) + } + conn, gotError := server.dialContext(context.Background(), "tcp", "unused.invalid:443") + if conn != nil || gotError != setupError { + t.Errorf("failed dial = %T/%v, want nil and original error object %v", conn, gotError, setupError) + } + if got := owned.closeCalls.Load(); got != 1 { + t.Errorf("failed pipe close count = %d, want exactly once", got) + } + if err := peer.SetReadDeadline(time.Now().Add(200 * time.Millisecond)); err == nil { + n, err := peer.Read(make([]byte, 1)) + if n != 0 || !errors.Is(err, io.EOF) { + t.Errorf("failed pipe remained open: %d/%v", n, err) + } + } else if !errors.Is(err, io.ErrClosedPipe) { + t.Errorf("closed peer deadline: %v", err) + } + }) + } +} + +func TestHTTPDialErrorOwnershipRejectsNilSuccess(t *testing.T) { + server, err := NewHTTPServer(Config{Dialer: transport.DialFunc(func(context.Context, string, string) (net.Conn, error) { + return nil, nil + })}) + if err != nil { + t.Fatal(err) + } + conn, err := server.dialContext(context.Background(), "tcp", "unused.invalid:443") + if conn != nil || err == nil { + t.Errorf("nil-success dial = %T/%v, want nil connection and error", conn, err) + } +} diff --git a/internal/proxy/tcp_dial_result_preserve_test.go b/internal/proxy/tcp_dial_result_preserve_test.go new file mode 100644 index 0000000..d154e1f --- /dev/null +++ b/internal/proxy/tcp_dial_result_preserve_test.go @@ -0,0 +1,211 @@ +package proxy + +import ( + "bufio" + "bytes" + "context" + "fmt" + "io" + "net" + "net/http" + "net/http/httptest" + "strings" + "sync" + "sync/atomic" + "testing" + "time" + + "github.com/cppla/autocar/internal/transport" +) + +// Embed the concrete TCP connection to retain its real half-close methods. +// Only full Close is observed; reads, writes and deadlines remain unmodified. +type tcpDialPreserveObservedConn struct { + *net.TCPConn + closes atomic.Int64 + closed chan struct{} + once sync.Once +} + +func (c *tcpDialPreserveObservedConn) Close() error { + c.closes.Add(1) + err := c.TCPConn.Close() + c.once.Do(func() { close(c.closed) }) + return err +} + +func TestTCPDialSuccessfulResultPreservesTunnel(t *testing.T) { + for _, frontend := range []string{"socks", "http-connect"} { + t.Run(frontend, func(t *testing.T) { + upstream, origin := tcpConnPair(t) + defer upstream.Close() + defer origin.Close() + _ = origin.SetDeadline(time.Now().Add(3 * time.Second)) + observed := &tcpDialPreserveObservedConn{TCPConn: upstream.(*net.TCPConn), closed: make(chan struct{})} + dialer := transport.DialFunc(func(context.Context, string, string) (net.Conn, error) { + return observed, nil + }) + var client net.Conn + payload := bytes.Repeat([]byte("\x16\x03\x01 early bytes\x00\xff"), 256) + var reader io.Reader + if frontend == "socks" { + server, address, stop := startSOCKS5(t, Config{Dialer: dialer}) + defer stop(server) + client = dialTCP(t, address) + defer client.Close() + socksGreeting(t, client, nil) + request := domainSOCKSRequest(socksCommandConnect, "example.invalid", 443) + mustWrite(t, client, append(request, payload...)) + if reply := readSOCKSReply(t, client); reply != socksReplySucceeded { + t.Fatalf("healthy SOCKS reply = %d", reply) + } + reader = client + } else { + server, proxyURL, stop := startHTTPProxy(t, Config{Dialer: dialer}) + defer stop(server) + client = dialTCP(t, strings.TrimPrefix(proxyURL, "http://")) + defer client.Close() + request := []byte("CONNECT example.invalid:443 HTTP/1.1\r\nHost: example.invalid:443\r\n\r\n") + mustWrite(t, client, append(request, payload...)) + buffered := bufio.NewReader(client) + status, err := buffered.ReadString('\n') + if err != nil || !strings.Contains(status, " 200 ") { + t.Fatalf("healthy CONNECT status = %q, %v", status, err) + } + for { + line, err := buffered.ReadString('\n') + if err != nil { + t.Fatal(err) + } + if line == "\r\n" { + break + } + } + reader = buffered + } + if err := client.(*net.TCPConn).CloseWrite(); err != nil { + t.Fatal(err) + } + got, err := io.ReadAll(origin) + if err != nil || !bytes.Equal(got, payload) { + t.Fatalf("origin early payload = %d/%d bytes, equal=%t, error=%v", len(got), len(payload), bytes.Equal(got, payload), err) + } + if got := observed.closes.Load(); got != 0 { + t.Fatalf("successful dial result was fully closed %d times before the reverse reply", got) + } + reply := []byte("reverse reply after the complete early upload and EOF") + mustWrite(t, origin, reply) + if err := origin.(*net.TCPConn).CloseWrite(); err != nil { + t.Fatal(err) + } + got, err = io.ReadAll(reader) + if err != nil || !bytes.Equal(got, reply) { + t.Fatalf("reverse reply = %q, %v", got, err) + } + select { + case <-observed.closed: + case <-time.After(time.Second): + t.Fatal("completed tunnel did not release its successful dial result") + } + }) + } +} + +func TestTCPDialSuccessfulResultPreservesHTTPReuse(t *testing.T) { + origin := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + _, _ = io.WriteString(w, "origin reply "+r.URL.Path) + })) + defer origin.Close() + var dialCalls atomic.Int64 + dialed := make(chan *tcpDialPreserveObservedConn, 4) + dialer := transport.DialFunc(func(ctx context.Context, network, address string) (net.Conn, error) { + dialCalls.Add(1) + conn, err := (&net.Dialer{}).DialContext(ctx, network, address) + if err != nil { + return nil, err + } + observed := &tcpDialPreserveObservedConn{TCPConn: conn.(*net.TCPConn), closed: make(chan struct{})} + dialed <- observed + return observed, nil + }) + server, proxyURL, stop := startHTTPProxy(t, Config{Dialer: dialer}) + defer stop(server) + client := proxyHTTPClient(t, proxyURL, nil) + defer client.CloseIdleConnections() + var first *tcpDialPreserveObservedConn + for _, path := range []string{"/first", "/second"} { + response, err := client.Get(origin.URL + path) + if err != nil { + t.Fatal(err) + } + body, readErr := io.ReadAll(response.Body) + closeErr := response.Body.Close() + if response.StatusCode != http.StatusOK || string(body) != "origin reply "+path || readErr != nil || closeErr != nil { + t.Fatalf("forward response = %d/%q, read=%v close=%v", response.StatusCode, body, readErr, closeErr) + } + if first == nil { + select { + case first = <-dialed: + case <-time.After(time.Second): + t.Fatal("successful forward request has no observed upstream dial") + } + } + if got := first.closes.Load(); got != 0 { + t.Fatalf("healthy reusable origin connection was prematurely closed %d times", got) + } + } + if got := dialCalls.Load(); got != 1 { + t.Fatalf("two healthy requests used %d origin dials, want one reusable connection", got) + } +} + +func TestTCPDialNilResultFailsClosed(t *testing.T) { + dialer := transport.DialFunc(func(context.Context, string, string) (net.Conn, error) { + return nil, nil + }) + t.Run("socks", func(t *testing.T) { + server, address, stop := startSOCKS5(t, Config{Dialer: dialer}) + defer stop(server) + client := dialTCP(t, address) + defer client.Close() + socksGreeting(t, client, nil) + mustWrite(t, client, domainSOCKSRequest(socksCommandConnect, "example.invalid", 443)) + if reply := readSOCKSReply(t, client); reply != socksReplyGeneralFailure { + t.Fatalf("nil/nil dial result SOCKS reply = %d, want general failure", reply) + } + }) + t.Run("http-core", func(t *testing.T) { + server, err := NewHTTPServer(Config{Dialer: dialer}) + if err != nil { + t.Fatal(err) + } + conn, err := server.dialContext(context.Background(), "tcp", "example.invalid:443") + if conn != nil || err == nil { + t.Fatalf("nil/nil upstream result was not rejected: conn=%T error=%v", conn, err) + } + }) + for _, method := range []string{http.MethodConnect, http.MethodGet} { + t.Run("http-"+method, func(t *testing.T) { + server, proxyURL, stop := startHTTPProxy(t, Config{Dialer: dialer}) + defer stop(server) + client := dialTCP(t, strings.TrimPrefix(proxyURL, "http://")) + defer client.Close() + target := "example.invalid:443" + if method == http.MethodGet { + target = "http://example.invalid/resource" + } + mustWrite(t, client, []byte(fmt.Sprintf("%s %s HTTP/1.1\r\nHost: example.invalid\r\nConnection: close\r\n\r\n", method, target))) + response, err := http.ReadResponse(bufio.NewReader(client), &http.Request{Method: method}) + if err != nil { + t.Fatal(err) + } + defer response.Body.Close() + if response.StatusCode != http.StatusBadGateway { + t.Fatalf("nil/nil result HTTP %s status = %d, want 502", method, response.StatusCode) + } + if _, err := io.ReadAll(response.Body); err != nil { + t.Fatalf("failed %s response body: %v", method, err) + } + }) + } +}