diff --git a/cmd/autocar/client.go b/cmd/autocar/client.go index 2e71595..c50a891 100644 --- a/cmd/autocar/client.go +++ b/cmd/autocar/client.go @@ -64,7 +64,7 @@ func runClient(parent context.Context, args []string) error { if err != nil { return err } - if err := validateProxyCredentials(*proxyUser, password, *socksAddress != ""); err != nil { + if err := validateProxyCredentials(*proxyUser, password, *socksAddress != "", *httpAddress != "" || *httpsAddress != ""); err != nil { return err } authenticator = proxy.StaticAuthenticator(*proxyUser, password) @@ -214,10 +214,13 @@ func runClient(parent context.Context, args []string) error { return nil } -func validateProxyCredentials(username, password string, socksEnabled bool) error { +func validateProxyCredentials(username, password string, socksEnabled, httpEnabled bool) error { if len(password) < 16 { return errors.New("local proxy password must be at least 16 bytes") } + if httpEnabled && strings.Contains(username, ":") { + return errors.New("local proxy username must not contain ':' when HTTP or HTTPS is enabled") + } // RFC 1929 encodes both lengths in one byte. HTTP Basic itself allows // longer values, so apply this compatibility bound only when SOCKS is on. if socksEnabled && (len(username) > 255 || len(password) > 255) { diff --git a/cmd/autocar/main_test.go b/cmd/autocar/main_test.go index d0988e0..4ea3e53 100644 --- a/cmd/autocar/main_test.go +++ b/cmd/autocar/main_test.go @@ -144,19 +144,19 @@ func TestEnsureProtectedPlaintextListener(t *testing.T) { } func TestValidateProxyCredentials(t *testing.T) { - if err := validateProxyCredentials("alice", strings.Repeat("x", 16), true); err != nil { + if err := validateProxyCredentials("alice", strings.Repeat("x", 16), true, true); err != nil { t.Fatal(err) } - if err := validateProxyCredentials("alice", "too-short", true); err == nil { + if err := validateProxyCredentials("alice", "too-short", true, true); err == nil { t.Fatal("short local proxy password accepted") } - if err := validateProxyCredentials(strings.Repeat("u", 256), strings.Repeat("p", 16), true); err == nil { + if err := validateProxyCredentials(strings.Repeat("u", 256), strings.Repeat("p", 16), true, true); err == nil { t.Fatal("oversized SOCKS5 username accepted") } - if err := validateProxyCredentials("alice", strings.Repeat("p", 256), true); err == nil { + if err := validateProxyCredentials("alice", strings.Repeat("p", 256), true, true); err == nil { t.Fatal("oversized SOCKS5 password accepted") } - if err := validateProxyCredentials(strings.Repeat("u", 256), strings.Repeat("p", 256), false); err != nil { + if err := validateProxyCredentials(strings.Repeat("u", 256), strings.Repeat("p", 256), false, true); err != nil { t.Fatalf("HTTP-only credentials were incorrectly limited to RFC 1929: %v", err) } } diff --git a/cmd/autocar/proxy_credentials_test.go b/cmd/autocar/proxy_credentials_test.go new file mode 100644 index 0000000..69137e9 --- /dev/null +++ b/cmd/autocar/proxy_credentials_test.go @@ -0,0 +1,97 @@ +package main + +import ( + "context" + "strings" + "testing" + "time" +) + +func TestValidateProxyCredentialsHTTPCompatibility(t *testing.T) { + const password = "diagnostic:password:with:colons" + for _, test := range []struct { + name, username string + socksEnabled, httpEnabled bool + wantErr bool + }{ + {name: "HTTP colon username", username: "user:name", httpEnabled: true, wantErr: true}, + {name: "combined colon username", username: "user:name", socksEnabled: true, httpEnabled: true, wantErr: true}, + {name: "SOCKS-only colon username", username: "user:name", socksEnabled: true}, + {name: "HTTP colon password", username: "user", httpEnabled: true}, + {name: "combined colon password", username: "user", socksEnabled: true, httpEnabled: true}, + } { + t.Run(test.name, func(t *testing.T) { + err := validateProxyCredentials(test.username, password, test.socksEnabled, test.httpEnabled) + if (err != nil) != test.wantErr { + t.Fatalf("credential validation error = %v, want error=%t", err, test.wantErr) + } + if err != nil && (strings.Contains(err.Error(), test.username) || strings.Contains(err.Error(), password)) { + t.Fatal("credential validation disclosed a credential value") + } + }) + } +} + +func TestClientRejectsHTTPColonUsernameBeforeStartup(t *testing.T) { + clearPreflightEnvironment(t) + t.Setenv("AUTOCAR_PROXY_PASSWORD", "diagnostic:password:with:colons") + files := newPreflightFiles(t) + dnsCalls := denyPreflightDNS(t) + for _, check := range []bool{false, true} { + mode := "startup" + if check { + mode = "check" + } + for _, test := range []struct { + name string + args []string + }{ + {name: "default HTTP and SOCKS"}, + {name: "HTTP only", args: []string{"--socks="}}, + {name: "HTTPS only", args: []string{"--socks=", "--http=", "--https=127.0.0.1:8443", "--proxy-cert", files.cert, "--proxy-key", files.key}}, + } { + t.Run(mode+"/"+test.name, func(t *testing.T) { + // Drop the helper's --check for the startup variant. The + // incompatible credentials must fail before binding or dialing. + args := append(files.clientArgs()[1:], "--proxy-user=user:name") + args = append(args, test.args...) + if check { + args = append(args, "--check") + } + ctx, cancel := context.WithTimeout(context.Background(), 2*time.Second) + defer cancel() + err := runClient(ctx, args) + if err == nil || err.Error() != "local proxy username must not contain ':' when HTTP or HTTPS is enabled" { + t.Fatalf("client error = %v, want HTTP username compatibility error", err) + } + }) + } + } + if dnsCalls.Load() != 0 { + t.Fatal("invalid local credentials triggered DNS") + } +} + +func TestClientCheckPreservesColonCredentialCompatibility(t *testing.T) { + clearPreflightEnvironment(t) + t.Setenv("AUTOCAR_PROXY_PASSWORD", "diagnostic:password:with:colons") + files := newPreflightFiles(t) + dnsCalls := denyPreflightDNS(t) + for _, test := range []struct { + name string + args []string + }{ + {name: "SOCKS-only colon username", args: []string{"--http=", "--proxy-user=user:name"}}, + {name: "HTTP colon password", args: []string{"--socks=", "--proxy-user=user"}}, + {name: "HTTPS colon password", args: []string{"--socks=", "--http=", "--https=127.0.0.1:8443", "--proxy-user=user", "--proxy-cert", files.cert, "--proxy-key", files.key}}, + } { + t.Run(test.name, func(t *testing.T) { + if err := runClient(context.Background(), append(files.clientArgs(), test.args...)); err != nil { + t.Fatalf("compatible local credentials rejected: %v", err) + } + }) + } + if dnsCalls.Load() != 0 { + t.Fatal("local credential checks triggered DNS") + } +} diff --git a/docs/DEPLOYMENT.md b/docs/DEPLOYMENT.md index 487e1e6..ccebaf3 100644 --- a/docs/DEPLOYMENT.md +++ b/docs/DEPLOYMENT.md @@ -420,6 +420,11 @@ those listeners on loopback or enable the local HTTPS proxy. A non-loopback plaintext listener requires an explicit override and should still be protected by a trusted private network/firewall. +When HTTP or HTTPS is enabled, `--proxy-user` must not contain `:` because HTTP +Basic uses it to separate the username and password. Startup and `--check` +reject this configuration. SOCKS-only usernames and passwords may contain `:`; +passwords containing `:` also remain valid for HTTP/HTTPS. + ## 7. systemd example `/etc/systemd/system/autocar.service`: diff --git a/internal/proxy/http.go b/internal/proxy/http.go index efbdd48..1456093 100644 --- a/internal/proxy/http.go +++ b/internal/proxy/http.go @@ -85,6 +85,9 @@ func NewHTTPServer(cfg Config) (*HTTPServer, error) { func (s *HTTPServer) Serve(listener net.Listener) error { managed, err := s.lifecycle.manage(listener) if err != nil { + if errors.Is(err, net.ErrClosed) { + return nil + } return err } err = s.server.Serve(managed) diff --git a/internal/proxy/lifecycle.go b/internal/proxy/lifecycle.go index aa93496..5e010b3 100644 --- a/internal/proxy/lifecycle.go +++ b/internal/proxy/lifecycle.go @@ -237,6 +237,7 @@ type serverLifecycle struct { mu sync.Mutex listener net.Listener tracker *connTracker + stopped bool } func newServerLifecycle(max int) *serverLifecycle { @@ -248,17 +249,26 @@ func (s *serverLifecycle) manage(listener net.Listener) (*managedListener, error return nil, errors.New("proxy: nil listener") } s.mu.Lock() - defer s.mu.Unlock() + if s.stopped { + s.mu.Unlock() + // Shutdown may finish before the Serve goroutine is scheduled. Own + // and close this late listener, but never perform I/O under the lock. + _ = listener.Close() + return nil, net.ErrClosed + } if s.listener != nil { + s.mu.Unlock() return nil, errors.New("proxy: server is already serving") } s.listener = listener + s.mu.Unlock() return &managedListener{Listener: listener, tracker: s.tracker}, nil } func (s *serverLifecycle) stopAccepting() error { - s.tracker.stopAccepting() s.mu.Lock() + s.stopped = true + s.tracker.stopAccepting() listener := s.listener s.mu.Unlock() if listener == nil { diff --git a/internal/proxy/lifecycle_test.go b/internal/proxy/lifecycle_test.go new file mode 100644 index 0000000..b91d8e3 --- /dev/null +++ b/internal/proxy/lifecycle_test.go @@ -0,0 +1,205 @@ +package proxy + +import ( + "context" + "errors" + "net" + "sync/atomic" + "testing" + "time" +) + +// An unexpected Accept returns immediately, so a shutdown regression fails +// without leaving a blocked server goroutine behind. +type lifecycleListener struct { + accepts atomic.Int32 + closes atomic.Int32 + onClose func() +} + +func (l *lifecycleListener) Accept() (net.Conn, error) { + l.accepts.Add(1) + return nil, errors.New("unexpected accept after shutdown") +} + +func (l *lifecycleListener) Close() error { + l.closes.Add(1) + if l.onClose != nil { + l.onClose() + } + return nil +} + +func (l *lifecycleListener) Addr() net.Addr { + return &net.TCPAddr{IP: net.IPv4(127, 0, 0, 1), Port: 1234} +} + +func TestServerLifecycleConcurrentManageAndShutdown(t *testing.T) { + for range 100 { + lifecycle := newServerLifecycle(1) + listener := &lifecycleListener{} + start := make(chan struct{}) + managed := make(chan error, 1) + stopped := make(chan error, 1) + go func() { + <-start + _, err := lifecycle.manage(listener) + managed <- err + }() + go func() { + <-start + stopped <- lifecycle.shutdown(context.Background()) + }() + close(start) + if err := <-managed; err != nil && !errors.Is(err, net.ErrClosed) { + t.Fatal(err) + } + if err := <-stopped; err != nil { + t.Fatal(err) + } + if listener.closes.Load() != 1 { + t.Fatalf("listener close count = %d, want 1", listener.closes.Load()) + } + lifecycle.tracker.mu.Lock() + accepting := lifecycle.tracker.accepting + lifecycle.tracker.mu.Unlock() + if accepting { + t.Fatal("tracker resumed accepting after shutdown") + } + } +} + +func TestServerLifecycleDuplicateServeKeepsActiveListener(t *testing.T) { + lifecycle := newServerLifecycle(1) + first := &lifecycleListener{} + if _, err := lifecycle.manage(first); err != nil { + t.Fatal(err) + } + second := &lifecycleListener{} + for _, duplicate := range []net.Listener{first, second} { + if _, err := lifecycle.manage(duplicate); err == nil { + t.Error("duplicate Serve was accepted") + } + } + if first.closes.Load() != 0 || second.closes.Load() != 0 { + t.Fatal("duplicate Serve closed a caller's listener") + } + if err := lifecycle.shutdown(context.Background()); err != nil { + t.Fatal(err) + } + if first.closes.Load() != 1 || second.closes.Load() != 0 { + t.Fatal("Shutdown did not close only the managed listener") + } +} + +func TestServerLifecycleClosesOutsideLocks(t *testing.T) { + for _, shutdownFirst := range []bool{false, true} { + lifecycle := newServerLifecycle(1) + listener := &lifecycleListener{onClose: func() { + if !lifecycle.mu.TryLock() { + t.Error("listener.Close called under lifecycle mutex") + } else { + lifecycle.mu.Unlock() + } + if !lifecycle.tracker.mu.TryLock() { + t.Error("listener.Close called under tracker mutex") + } else { + lifecycle.tracker.mu.Unlock() + } + }} + if shutdownFirst { + if err := lifecycle.shutdown(context.Background()); err != nil { + t.Fatal(err) + } + } + _, err := lifecycle.manage(listener) + if shutdownFirst && !errors.Is(err, net.ErrClosed) { + t.Fatalf("late manage error = %v, want closed", err) + } + if !shutdownFirst && err != nil { + t.Fatal(err) + } + if err := lifecycle.shutdown(context.Background()); err != nil { + t.Fatal(err) + } + if listener.closes.Load() != 1 { + t.Fatalf("listener close count = %d, want 1", listener.closes.Load()) + } + } +} + +func TestProxyShutdownBeforeServeClosesRealListener(t *testing.T) { + for _, kind := range []string{"socks5", "http"} { + t.Run(kind, func(t *testing.T) { + cfg := Config{Dialer: directDialer()} + var server Server + var err error + if kind == "socks5" { + server, err = NewSOCKS5Server(cfg) + } else { + server, err = NewHTTPServer(cfg) + } + if err != nil { + t.Fatal(err) + } + listener, err := net.Listen("tcp", "127.0.0.1:0") + if err != nil { + t.Fatal(err) + } + defer listener.Close() + if err := server.Shutdown(context.Background()); err != nil { + t.Fatal(err) + } + done := make(chan error, 1) + go func() { done <- server.Serve(listener) }() + select { + case err := <-done: + if err != nil { + t.Fatal(err) + } + case <-time.After(time.Second): + _ = listener.Close() + select { + case <-done: + case <-time.After(time.Second): + t.Error("Serve did not exit after cleanup") + } + t.Fatal("Serve blocked after Shutdown") + } + if err := listener.(*net.TCPListener).SetDeadline(time.Now()); !errors.Is(err, net.ErrClosed) { + t.Fatalf("listener still open after late Serve: %v", err) + } + }) + } +} + +func TestProxyServeAfterShutdownClosesListener(t *testing.T) { + for _, kind := range []string{"socks5", "http"} { + t.Run(kind, func(t *testing.T) { + var server Server + var err error + cfg := Config{Dialer: directDialer()} + if kind == "socks5" { + server, err = NewSOCKS5Server(cfg) + } else { + server, err = NewHTTPServer(cfg) + } + if err != nil { + t.Fatal(err) + } + if err := server.Shutdown(context.Background()); err != nil { + t.Fatal(err) + } + listener := &lifecycleListener{} + if err := server.Serve(listener); err != nil { + t.Errorf("Serve after Shutdown: %v", err) + } + if got := listener.accepts.Load(); got != 0 { + t.Errorf("Accept called %d times after Shutdown", got) + } + if got := listener.closes.Load(); got == 0 { + t.Error("late listener was not closed") + } + }) + } +} diff --git a/internal/proxy/socks5.go b/internal/proxy/socks5.go index 13f60cb..2c16bde 100644 --- a/internal/proxy/socks5.go +++ b/internal/proxy/socks5.go @@ -72,6 +72,9 @@ func NewSOCKS5Server(cfg Config) (*SOCKS5Server, error) { func (s *SOCKS5Server) Serve(listener net.Listener) error { managed, err := s.lifecycle.manage(listener) if err != nil { + if errors.Is(err, net.ErrClosed) { + return nil + } return err } for {