diff --git a/README.md b/README.md index 570a088..097f316 100644 --- a/README.md +++ b/README.md @@ -179,6 +179,9 @@ Unix 目录权限为 `0700`、文件为 `0600`；Windows 需另行限制 ACL。 ./bin/autocar cert --hosts relay.example.com,203.0.113.10 --cert server.crt --key server.key ``` +证书和私钥必须使用不同输出文件；同一目录中的文件名也不能仅大小写不同。 +相对/绝对路径或父目录符号链接指向同一输出文件时，会在写入前拒绝。 + 服务端的 UDP 与 TCP 可以使用相同端口号；这里先使用非特权端口： ```bash diff --git a/internal/proxy/lifecycle.go b/internal/proxy/lifecycle.go index 9fa863a..f92c8d6 100644 --- a/internal/proxy/lifecycle.go +++ b/internal/proxy/lifecycle.go @@ -103,8 +103,9 @@ type trackedConn struct { // waiting for upstream setup need not be reading the socket; net/http's reader // can also stop after caching a pipelined byte. Socket closure alone therefore // need not cancel that work. The frontend calls this once per accepted -// connection, from HTTP ConnContext or SOCKS TCP setup. Registration and Close -// may race during shutdown; either ordering must return a canceled context. +// connection, from HTTP ConnContext or before SOCKS negotiation. Registration +// and Close may race during shutdown; either ordering must return a canceled +// context. func (c *trackedConn) connectionContext(parent context.Context) context.Context { ctx, cancel := context.WithCancel(parent) c.closeMu.Lock() diff --git a/internal/proxy/socks5.go b/internal/proxy/socks5.go index e04d57d..03b7df8 100644 --- a/internal/proxy/socks5.go +++ b/internal/proxy/socks5.go @@ -97,11 +97,18 @@ func (s *SOCKS5Server) Shutdown(ctx context.Context) error { func (s *SOCKS5Server) serveConn(client net.Conn) { defer client.Close() + owner := context.Background() + if tracked, ok := client.(*trackedConn); ok { + // Own authentication, setup, and the established TCP relay from the + // start. Closing the socket must also interrupt work that is not doing + // socket I/O, without coupling it to a stage-specific deadline. + owner = tracked.connectionContext(owner) + } if s.cfg.handshakeTimeout > 0 { _ = client.SetDeadline(time.Now().Add(s.cfg.handshakeTimeout)) } - if err := s.negotiate(client); err != nil { + if err := s.negotiate(owner, client); err != nil { return } request, err := readSOCKSRequest(client) @@ -132,17 +139,10 @@ func (s *SOCKS5Server) serveConn(client net.Conn) { s.serveUDPAssociate(client, request) return } - s.serveConnect(client, request) + s.serveConnect(owner, client, request) } -func (s *SOCKS5Server) serveConnect(client net.Conn, request socksRequest) { - owner := context.Background() - if tracked, ok := client.(*trackedConn); ok { - // Bind this caller's setup wait and established relay to full socket - // close, not to a half-close or the independent setup deadline. Do not - // add a payload-consuming reader or close the shared upstream dialer. - owner = tracked.connectionContext(owner) - } +func (s *SOCKS5Server) serveConnect(owner context.Context, client net.Conn, request socksRequest) { ctx := owner var cancel context.CancelFunc if s.cfg.dialTimeout > 0 { @@ -623,7 +623,7 @@ wait: } } -func (s *SOCKS5Server) negotiate(conn net.Conn) error { +func (s *SOCKS5Server) negotiate(owner context.Context, conn net.Conn) error { methods, err := readSOCKSGreeting(conn) if err != nil { return err @@ -647,7 +647,7 @@ func (s *SOCKS5Server) negotiate(conn net.Conn) error { _ = writeFull(conn, []byte{userPasswordVersion, 0x01}) return err } - ctx := context.Background() + ctx := owner if s.cfg.handshakeTimeout > 0 { var cancel context.CancelFunc ctx, cancel = context.WithTimeout(ctx, s.cfg.handshakeTimeout) diff --git a/internal/proxy/socks5_auth_shutdown_cancel_test.go b/internal/proxy/socks5_auth_shutdown_cancel_test.go new file mode 100644 index 0000000..506b199 --- /dev/null +++ b/internal/proxy/socks5_auth_shutdown_cancel_test.go @@ -0,0 +1,140 @@ +package proxy + +import ( + "context" + "errors" + "io" + "net" + "sync" + "sync/atomic" + "testing" + "time" + + "github.com/cppla/autocar/internal/transport" +) + +func TestSOCKS5ForcedShutdownCancelsAuthentication(t *testing.T) { + entered := make(chan context.Context, 1) + authDone := make(chan struct{}) + release := make(chan struct{}) + var releaseOnce sync.Once + var dialCalls atomic.Int64 + server, err := NewSOCKS5Server(Config{ + Dialer: transport.DialFunc(func(context.Context, string, string) (net.Conn, error) { + dialCalls.Add(1) + return nil, errors.New("authentication must finish before dialing") + }), + HandshakeTimeout: time.Hour, + Authenticator: AuthFunc(func(ctx context.Context, _, _ string) bool { + defer close(authDone) + entered <- ctx + select { + case <-ctx.Done(): + case <-release: // Independent cleanup only, never evidence of cancellation. + } + return false + }), + }) + if err != nil { + t.Fatal(err) + } + listener, err := net.Listen("tcp", "127.0.0.1:0") + if err != nil { + t.Fatal(err) + } + managed, err := server.lifecycle.manage(listener) + if err != nil { + _ = listener.Close() + t.Fatal(err) + } + accepted := make(chan error, 1) + handlerDone := make(chan struct{}) + var client net.Conn + t.Cleanup(func() { + releaseOnce.Do(func() { close(release) }) + if client != nil { + _ = client.Close() + } + ctx, cancel := context.WithCancel(context.Background()) + cancel() + _ = server.Shutdown(ctx) + select { + case <-handlerDone: + t.Log("independent cleanup joined the production authentication handler") + case <-time.After(2 * time.Second): + t.Error("authentication handler did not finish after independent cleanup") + } + }) + // Keep the production connection tracking and serveConn implementation; + // expose only the worker's completion so socket closure cannot mask it. + go func() { + defer close(handlerDone) + conn, err := managed.Accept() + accepted <- err + if err == nil { + server.serveConn(conn) + } + }() + client = dialTCP(t, listener.Addr().String()) + select { + case err := <-accepted: + if err != nil { + t.Fatal(err) + } + case <-time.After(2 * time.Second): + t.Fatal("managed listener did not accept the authentication client") + } + socksGreeting(t, client, []byte{socksMethodUserPassword}) + mustWrite(t, client, []byte{userPasswordVersion, 1, 'u', 1, 'p'}) + var authCtx context.Context + select { + case authCtx = <-entered: + case <-time.After(2 * time.Second): + t.Fatal("production negotiation did not enter the authenticator") + } + if err := authCtx.Err(); err != nil { + t.Fatalf("authentication was canceled before shutdown: %v", err) + } + + shutdownCtx, cancel := context.WithCancel(context.Background()) + cancel() // Force the close path without a timer or a scheduling delay. + shutdownErr := server.Shutdown(shutdownCtx) + if !errors.Is(shutdownErr, context.Canceled) { + t.Errorf("forced Shutdown = %v, want context canceled", shutdownErr) + } + server.lifecycle.tracker.mu.Lock() + held := len(server.lifecycle.tracker.conns) + server.lifecycle.tracker.mu.Unlock() + if held != 0 { + t.Errorf("forced Shutdown retained %d tracked connections", held) + } + if err := client.SetReadDeadline(time.Now().Add(2 * time.Second)); err != nil { + t.Fatal(err) + } + var one [1]byte + n, readErr := client.Read(one[:]) + if n != 0 || !errors.Is(readErr, io.EOF) { + t.Errorf("client after forced Shutdown = %d/%v, want 0/EOF", n, readErr) + } + if dialCalls.Load() != 0 { + t.Errorf("unfinished authentication made %d upstream dial calls", dialCalls.Load()) + } + if err := authCtx.Err(); !errors.Is(err, context.Canceled) { + // With neither ctx.Done nor cleanup release closed, the authenticator + // cannot finish. This is a context ownership assertion, not a sleep. + t.Errorf("forced Shutdown left authentication context live: %v; tracked=%d client=%d/%v", err, held, n, readErr) + select { + case <-authDone: + t.Error("authenticator unexpectedly finished without cancellation or cleanup") + default: + t.Log("before independent cleanup: authenticator and serveConn are still blocked") + } + return + } + select { + case <-handlerDone: + t.Logf("forced Shutdown=%v; context canceled; tracker=%d; client EOF; authentication handler joined", shutdownErr, held) + case <-time.After(2 * time.Second): + t.Error("authentication handler did not finish after its context was canceled") + } +} diff --git a/internal/proxy/socks5_tcp_setup_cancel_test.go b/internal/proxy/socks5_tcp_setup_cancel_test.go index 7c6cf5f..07c4e78 100644 --- a/internal/proxy/socks5_tcp_setup_cancel_test.go +++ b/internal/proxy/socks5_tcp_setup_cancel_test.go @@ -199,7 +199,8 @@ func TestSOCKSTCPSetupAlreadyClosedSkipsDial(t *testing.T) { if err := tracked.Close(); err != nil { t.Fatal(err) } - server.serveConnect(tracked, socksRequest{command: socksCommandConnect, address: "127.0.0.1:443"}) + owner := tracked.connectionContext(context.Background()) + server.serveConnect(owner, tracked, socksRequest{command: socksCommandConnect, address: "127.0.0.1:443"}) if dialCalls != 0 { t.Fatalf("already closed SOCKS TCP setup started %d remote dials", dialCalls) } diff --git a/internal/security/cert.go b/internal/security/cert.go index 85438c5..fb36d0d 100644 --- a/internal/security/cert.go +++ b/internal/security/cert.go @@ -119,13 +119,14 @@ func GenerateSelfSignedCertificate(opts CertificateOptions) (certPEM, keyPEM []b // WriteSelfSignedCertificate generates and atomically writes a certificate and // private key. The key is always installed with mode 0600, even when replacing -// a pre-existing file; the certificate is installed with mode 0644. +// a pre-existing file; the certificate is installed with mode 0644. Destinations +// in the same directory must not have equal or case-only-different filenames. func WriteSelfSignedCertificate(certFile, keyFile string, opts CertificateOptions) error { if strings.TrimSpace(certFile) == "" || strings.TrimSpace(keyFile) == "" { return errors.New("security: certificate and key paths are required") } - if filepath.Clean(certFile) == filepath.Clean(keyFile) { - return errors.New("security: certificate and key paths must be different") + if err := validateCertificateDestinations(certFile, keyFile); err != nil { + return err } certPEM, keyPEM, err := GenerateSelfSignedCertificate(opts) @@ -159,6 +160,41 @@ func WriteSelfSignedCertificate(certFile, keyFile string, opts CertificateOption return nil } +func validateCertificateDestinations(certFile, keyFile string) error { + // Compare the directories the filesystem actually resolves, not just path + // spellings: relative paths and symlinked parents can name the same entry. + // Split preserves symlink/.. semantics that cleaning the path could change. + certDir, certName := filepath.Split(certFile) + keyDir, keyName := filepath.Split(keyFile) + if certDir == "" { + certDir = "." + } + if keyDir == "" { + keyDir = "." + } + certParent, err := os.Stat(certDir) + if err != nil { + return fmt.Errorf("security: certificate directory: %w", err) + } + if !certParent.IsDir() { + return errors.New("security: certificate parent must be a directory") + } + keyParent, err := os.Stat(keyDir) + if err != nil { + return fmt.Errorf("security: private key directory: %w", err) + } + if !keyParent.IsDir() { + return errors.New("security: private key parent must be a directory") + } + // Conservatively disallow case-only names in one directory, including on + // case-sensitive hosts, so a pair is safe on case-insensitive filesystems. + // Do not follow the final component: Rename replaces that directory entry. + if os.SameFile(certParent, keyParent) && strings.EqualFold(certName, keyName) { + return errors.New("security: certificate and key paths must be different") + } + return nil +} + func prepareAtomicFile(destination string, data []byte, mode os.FileMode) (path string, err error) { dir := filepath.Dir(destination) base := filepath.Base(destination) diff --git a/internal/security/cert_path_alias_test.go b/internal/security/cert_path_alias_test.go new file mode 100644 index 0000000..d2bd613 --- /dev/null +++ b/internal/security/cert_path_alias_test.go @@ -0,0 +1,89 @@ +package security + +import ( + "bytes" + "os" + "path/filepath" + "runtime" + "testing" +) + +func TestWriteSelfSignedCertificateRejectsAliasedPaths(t *testing.T) { + workingDirectory, err := os.Getwd() + if err != nil { + t.Fatal(err) + } + for _, aliasKind := range []string{"absolute-relative", "symlink-parent", "case-only"} { + for _, existing := range []bool{false, true} { + state := "missing" + if existing { + state = "existing" + } + t.Run(aliasKind+"/"+state, func(t *testing.T) { + directory := t.TempDir() + target := filepath.Join(directory, "shared.pem") + var alias string + switch aliasKind { + case "absolute-relative": + alias, err = filepath.Rel(workingDirectory, target) + if err != nil { + t.Skipf("cannot express temporary target relative to working directory: %v", err) + } + case "symlink-parent": + link := filepath.Join(t.TempDir(), "parent-link") + if err := os.Symlink(directory, link); err != nil { + if runtime.GOOS == "windows" { + t.Skipf("directory symlink unavailable: %v", err) + } + t.Fatal(err) + } + alias = filepath.Join(link, "shared.pem") + case "case-only": + alias = filepath.Join(directory, "SHARED.PEM") + } + original := []byte("existing destination must stay unchanged\n") + if existing { + if err := os.WriteFile(target, original, 0o600); err != nil { + t.Fatal(err) + } + } + if err := WriteSelfSignedCertificate(target, alias, CertificateOptions{Hosts: []string{"node.example"}}); err == nil { + t.Error("aliased certificate and private-key destinations were accepted") + } + if existing { + got, err := os.ReadFile(target) + if err != nil { + t.Fatal(err) + } + if !bytes.Equal(got, original) { + t.Error("existing destination was overwritten") + } + } else if _, err := os.Lstat(target); !os.IsNotExist(err) { + t.Errorf("rejected aliases must not create a destination; stat error: %v", err) + } + entries, err := os.ReadDir(directory) + if err != nil { + t.Fatal(err) + } + wantEntries := 0 + if existing { + wantEntries = 1 + } + if len(entries) != wantEntries { + t.Errorf("rejected aliases left %d directory entries, want %d", len(entries), wantEntries) + } + }) + } + } +} + +func TestWriteSelfSignedCertificateAllowsSameBasenameInDifferentDirectories(t *testing.T) { + certFile := filepath.Join(t.TempDir(), "same.pem") + keyFile := filepath.Join(t.TempDir(), "same.pem") + if err := WriteSelfSignedCertificate(certFile, keyFile, CertificateOptions{Hosts: []string{"node.example"}}); err != nil { + t.Fatal(err) + } + if _, err := LoadKeyPair(certFile, keyFile); err != nil { + t.Fatalf("different destinations must contain a usable certificate and key: %v", err) + } +}