Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 9 additions & 8 deletions pkg/tlsx/ztls/ztls.go
Original file line number Diff line number Diff line change
Expand Up @@ -323,17 +323,18 @@ func (c *Client) getConfig(hostname, ip, port string, options clients.ConnectOpt
// tlsHandshakeWithCtx attempts tls handshake with given timeout
func (c *Client) tlsHandshakeWithTimeout(tlsConn *tls.Conn, ctx context.Context) error {
errChan := make(chan error, 1)
defer close(errChan)

go func() {
errChan <- tlsConn.Handshake()
}()
Comment on lines +327 to +329
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why spawn a goroutine?


select {
case <-ctx.Done():
return errorutil.NewWithTag("ztls", "timeout while attempting handshake") //nolint
case errChan <- tlsConn.Handshake():
}

err := <-errChan
if err == tls.ErrCertsOnly {
err = nil
case err := <-errChan:
if err == tls.ErrCertsOnly {
return nil
}
return err
}
return err
}
77 changes: 77 additions & 0 deletions pkg/tlsx/ztls/ztls_timeout_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,77 @@
package ztls

import (
"context"
"net"
"strings"
"testing"
"time"

"github.com/zmap/zcrypto/tls"
)

// hangingConn is a net.Conn that blocks on Read and Write until context is cancelled
type hangingConn struct {
net.Conn
ctx context.Context
}

func (h hangingConn) Read(b []byte) (n int, err error) {
<-h.ctx.Done()
return 0, h.ctx.Err()
}

func (h hangingConn) Write(b []byte) (n int, err error) {
<-h.ctx.Done()
return 0, h.ctx.Err()
}

func TestTLSHandshakeHang(t *testing.T) {
// create a pipe to simulate a connection
// we wrap client side to ensure it hangs if it tries to read/write
clientConn, _ := net.Pipe()
defer clientConn.Close()

// Context to control the hanging connection's lifecycle
// This ensures the goroutine spawned by Handshake eventually exits
connCtx, connCancel := context.WithCancel(context.Background())
defer connCancel()

hanging := hangingConn{
Conn: clientConn,
ctx: connCtx,
}

// create a tls connection using the hanging connection
// we don't need a real server because we want to test the client-side timeout
// when the "network" hangs during handshake hello or similar.
config := &tls.Config{
InsecureSkipVerify: true,
}
tlsConn := tls.Client(hanging, config)

// Create a dummy client just to call the method
client := &Client{}

// context with short timeout for the handshake operation
ctx, cancel := context.WithTimeout(context.Background(), 200*time.Millisecond)
defer cancel()

start := time.Now()
err := client.tlsHandshakeWithTimeout(tlsConn, ctx)
duration := time.Since(start)

// Check if it respected timeout
if duration > 1*time.Second {
t.Errorf("Handshake took too long: %v, expected ~200ms", duration)
}

if err == nil {
t.Error("Expected timeout error, got nil")
} else {
// verify it's the right error
if !strings.Contains(err.Error(), "timeout") {
t.Errorf("Expected timeout error, got: %v", err)
}
}
Comment on lines +29 to +76
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot Feb 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Goroutine leak in test: the spawned handshake goroutine can never be unblocked.

hangingConn.Read/Write use a bare select {} which blocks forever — even after clientConn.Close() is called. This means the goroutine spawned inside tlsHandshakeWithTimeout will permanently leak for each test run. In production code the caller closes the real net.Conn, which unblocks the zcrypto handshake; the test should simulate this too.

Also, the server side of net.Pipe() (line 29, assigned to _) is never closed.

A simple fix: use a channel to make hangingConn cancellable so the goroutine can exit after the test.

Suggested fix
-type hangingConn struct {
-	net.Conn
+type hangingConn struct {
+	net.Conn
+	done chan struct{}
 }
 
-func (h hangingConn) Read(b []byte) (n int, err error) {
-	select {}
+func (h hangingConn) Read(b []byte) (n int, err error) {
+	<-h.done
+	return 0, net.ErrClosed
 }
 
-func (h hangingConn) Write(b []byte) (n int, err error) {
-	select {}
+func (h hangingConn) Write(b []byte) (n int, err error) {
+	<-h.done
+	return 0, net.ErrClosed
 }
 
 func TestTLSHandshakeHang(t *testing.T) {
-	clientConn, _ := net.Pipe()
+	clientConn, serverConn := net.Pipe()
 	defer clientConn.Close()
+	defer serverConn.Close()
 
-	hanging := hangingConn{Conn: clientConn}
+	done := make(chan struct{})
+	defer close(done) // unblocks the goroutine after test assertions
+	hanging := hangingConn{Conn: clientConn, done: done}
🧰 Tools
🪛 ast-grep (0.40.5)

[warning] 36-38: MinVersionis missing from this TLS configuration. By default, TLS 1.2 is currently used as the minimum when acting as a client, and TLS 1.0 when acting as a server. General purpose web applications should default to TLS 1.3 with all other protocols disabled. Only where it is known that a web server must support legacy clients with unsupported an insecure browsers (such as Internet Explorer 10), it may be necessary to enable TLS 1.0 to provide support. AddMinVersion: tls.VersionTLS13' to the TLS configuration to bump the minimum version to TLS 1.3.
Context: tls.Config{
InsecureSkipVerify: true,
}
Note: [CWE-327]: Use of a Broken or Risky Cryptographic Algorithm [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures [REFERENCES]
https://owasp.org/Top10/A02_2021-Cryptographic_Failures

(missing-ssl-minversion-go)

🤖 Prompt for AI Agents
In `@pkg/tlsx/ztls/ztls_timeout_test.go` around lines 26 - 65, The test leaks a
goroutine because hangingConn.Read/Write block forever (they use a bare select)
and the server side net.Pipe() was never closed; update the test and hangingConn
so reads/writes are cancellable: create the pipe as clientConn, serverConn :=
net.Pipe() and defer serverConn.Close(), make hangingConn hold a done channel
(or use a sync.Once-closeable signal) that is closed in hangingConn.Close() (and
call clientConn.Close()/hangingConn.Close() in defer), and change
hangingConn.Read and hangingConn.Write to select on the done channel (return
net.ErrClosed or a timeout-like error) as well as the real underlying Conn so
the goroutine spawned by Client.tlsHandshakeWithTimeout can unblock and exit
when the test cancels the context or closes the conn.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dogancanbakir I have refactored the test case (ztls_timeout_test.go) to address the goroutine leak noted by CodeRabbit. The test still passes successfully. Ready for final review.
proof43

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!

}
Loading