Add armed Listener with TLS handshake timeout by Choraden · Pull Request #637 · saucelabs/forwarder

Choraden · 2024-01-05T13:23:16Z

This patch adds server side TLS handshake timeout. It will protect forwarders listener and additionally a handshake in mitm.

Due to the lack of a convenient solution upstream connect is given a constant 1m timeout to complete.

mmatczuk · 2024-01-05T14:04:47Z

http_proxy.go

 		return listener, nil
 	case HTTPSScheme, HTTP2Scheme:
-		return tls.NewListener(listener, hp.tlsConfig), nil
+		l := NewTLSListener(listener, hp.tlsConfig)


How about we passed TLSServerConfig to NewTLSListener?

I decided to leave it as it is, to have logging in the same place where we load certs.

func (hp *HTTPProxy) configureHTTPS() error { if hp.config.CertFile == "" && hp.config.KeyFile == "" { hp.log.Infof("no TLS certificate provided, using self-signed certificate") } else { hp.log.Debugf("loading TLS certificate from %s and %s", hp.config.CertFile, hp.config.KeyFile) } hp.tlsConfig = httpsTLSConfigTemplate() return hp.config.ConfigureTLSConfig(hp.tlsConfig) }

Eventually, I changed it.

mmatczuk · 2024-01-05T14:09:46Z

It feels like tlshandshake could be inlined given that we have the TLSListener

mmatczuk · 2024-01-05T14:10:06Z

Some more context it the commit messages would be helpful

Choraden · 2024-01-05T15:31:12Z

It feels like tlshandshake could be inlined given that we have the TLSListener

Listener is one thing. The other is handling handshake in mitm or SC.
It can be later extended with proper error handling.

mmatczuk · 2024-01-09T14:43:18Z

Now we can inline handshake in the listener.

Choraden · 2024-01-10T15:58:29Z

I've came to a conclusion adding handshake in the listener might not be the best option.

The logic in the martian Proxy.Serve would cause Proxy to exit on a failed handshake.

conn, err := l.Accept()
nosigpipe.IgnoreSIGPIPE(conn)
if err != nil {
	var nerr net.Error
	if ok := errors.As(err, &nerr); ok && nerr.Temporary() {
		if delay == 0 {
			delay = 5 * time.Millisecond
		} else {
			delay *= 2
		}
		if max := time.Second; delay > max {
			delay = max
		}

		log.Debugf(context.TODO(), "temporary error on accept: %v", err)
		time.Sleep(delay)
		continue
	}

	if errors.Is(err, net.ErrClosed) {
		log.Debugf(context.TODO(), "listener closed, returning")
		return err
	}

	log.Errorf(context.TODO(), "failed to accept: %v", err)
	return err
}

mmatczuk · 2024-01-16T10:47:00Z

net.go

 	return defaultListenConfig().Listen(context.Background(), network, address)
 }
+
+type Listener struct {


Let's use exported variables to configure the listener like we do with http servers for instance.

mmatczuk · 2024-01-16T10:48:23Z

net.go

+	handshakeTimeout time.Duration
+
+	// OnAccept is called when a new connection is successfully accepted.
+	OnAccept func(net.Conn)


An interface with the callback would be better if we are planning to have more of them.

mmatczuk · 2024-01-16T11:01:42Z

http_proxy.go

 	hp.proxy.ReadTimeout = hp.config.ReadTimeout
 	hp.proxy.ReadHeaderTimeout = hp.config.ReadHeaderTimeout
 	hp.proxy.WriteTimeout = hp.config.WriteTimeout
+	hp.proxy.MITMTLSHandshakeTimeout = hp.config.TLSServerConfig.HandshakeTimeout


Move it to mitm setup

mmatczuk · 2024-01-16T11:02:45Z

internal/martian/proxy.go


+	// MITMTLSHandshakeTimeout specifies the maximum amount of time to wait for a TLS handshake for a MITMed connection.
+	// Zero means no timeout.
+	MITMTLSHandshakeTimeout time.Duration


Move it adjanct to MITMFilter

mmatczuk · 2024-01-16T11:04:18Z

internal/martian/proxy.go

-	res, conn, err = d.DialContextR(req.Context(), "tcp", req.URL.Host)
+
+	ctx, cancel := context.WithTimeout(req.Context(), 60*time.Second)
+	res, conn, err = d.DialContextR(ctx, "tcp", req.URL.Host)


Let's have ConnectTimeout field.

Let's open new issue to add context to socks.

Choraden · 2024-01-17T08:49:41Z

@mmatczuk I've addressed your comments

net.go

mmatczuk · 2024-01-18T09:22:15Z

net.go

+}
+
+func (l *Listener) withTLS(c net.Conn) (net.Conn, error) {
+	tc := tls.Server(c, l.TLSConfig)


use Martian/std naming tconn

mmatczuk · 2024-01-18T09:22:47Z

net.go

+	Address          string
+	Log              log.Logger
+	TLSConfig        *tls.Config
+	HandshakeTimeout time.Duration


TLSHandshakeTimeout

net.go

mmatczuk · 2024-01-18T09:24:22Z

net_test.go

+		t.Fatal(err)
+	}
+
+	donec := make(chan struct{})


use done as it's more popular variant.

mmatczuk · 2024-01-18T09:25:14Z

net.go

+	}
+}
+
+func (l *Listener) withTLS(c net.Conn) (net.Conn, error) {


rename c to conn and return tls.Conn ptr

mmatczuk · 2024-01-18T09:28:24Z

net_test.go

+	donec := make(chan struct{})
+
+	l := Listener{
+		Address:          "[::]:0",


Why bind to IPv6?

Tests should run on localhost as much as possible.
This way you can avoid the macOS banner to allow listening from the test process.
Please one an issue to fix that across the board i.e. in Martian tests.

Why bind to IPv6?

I guess, I took it from martian tests.

mmatczuk · 2024-01-18T09:30:33Z

internal/martian/proxy.go

+		var hctx context.Context
+		if p.MITMTLSHandshakeTimeout > 0 {
+			var hcancel context.CancelFunc
+			hctx, hcancel = context.WithTimeout(req.Context(), p.MITMTLSHandshakeTimeout)
+			defer hcancel()
+		} else {
+			hctx = req.Context()
+		}
+		if err := tlsconn.HandshakeContext(hctx); err != nil {


Can we have var error - less ugly code.

mmatczuk · 2024-01-18T09:31:50Z

internal/martian/proxy.go

 	// Implementations can return ErrConnectFallback to indicate that the CONNECT request should be handled by martian.
 	ConnectFunc ConnectFunc

+	// ConnectTimeout specifies the maximum amount of time to wait for a dial to complete.


It's not dial it's connect with upstream.

Choraden · 2024-01-18T09:54:52Z

@mmatczuk I've addressed your comments and opened two issues.

The goal of this patch is to move listening related tasks to custom Listener type. The Listener supports: - tls connections with handshake timeout - per connection read and write limits - callbacks to easily attach metrics gatherer

The timeout is 60s by default.

mmatczuk

LGTM

mmatczuk reviewed Jan 5, 2024

View reviewed changes

Choraden force-pushed the hg/tls_handshake branch from 8123ab5 to add8ce9 Compare January 9, 2024 08:25

Choraden changed the title ~~Add server TLS handshake timeout~~ Add TLS handshake timeout Jan 9, 2024

Choraden requested a review from mmatczuk January 9, 2024 08:32

Choraden force-pushed the hg/tls_handshake branch from add8ce9 to 5388a6d Compare January 15, 2024 11:52

Choraden requested a review from a team as a code owner January 15, 2024 11:52

Choraden changed the title ~~Add TLS handshake timeout~~ Add armed Listener with TLS handshake timeout Jan 15, 2024

mmatczuk reviewed Jan 16, 2024

View reviewed changes

Choraden force-pushed the hg/tls_handshake branch 3 times, most recently from 28cd997 to 62e72b4 Compare January 16, 2024 17:19

Choraden force-pushed the hg/tls_handshake branch from 62e72b4 to b956763 Compare January 17, 2024 11:02

Choraden mentioned this pull request Jan 17, 2024

http_proxy: support multi-addresses listening #654

Closed

mmatczuk reviewed Jan 18, 2024

View reviewed changes

net.go Show resolved Hide resolved

mmatczuk reviewed Jan 18, 2024

View reviewed changes

net.go Show resolved Hide resolved

mmatczuk reviewed Jan 18, 2024

View reviewed changes

Choraden force-pushed the hg/tls_handshake branch from b956763 to c81ba63 Compare January 18, 2024 09:44

Choraden added 4 commits January 18, 2024 11:03

martian: add TLSHandshakeTimeout for mitm

6bd9ee8

bind: add server tls handshake timeout

e64efac

martian: add timeout for upstream HTTP connect

f9efa12

The timeout is 60s by default.

Choraden force-pushed the hg/tls_handshake branch from c81ba63 to f9efa12 Compare January 18, 2024 10:16

mmatczuk approved these changes Jan 18, 2024

View reviewed changes

mmatczuk merged commit fb9e70a into main Jan 18, 2024

mmatczuk deleted the hg/tls_handshake branch January 18, 2024 10:36

Conversation

Choraden commented Jan 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mmatczuk commented Jan 5, 2024

Uh oh!

mmatczuk commented Jan 5, 2024

Uh oh!

Choraden commented Jan 5, 2024

Uh oh!

mmatczuk commented Jan 9, 2024

Uh oh!

Choraden commented Jan 10, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choraden commented Jan 17, 2024

Uh oh!

Uh oh!

mmatczuk Jan 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mmatczuk Jan 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mmatczuk Jan 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choraden commented Jan 18, 2024

Uh oh!

mmatczuk left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Choraden commented Jan 5, 2024 •

edited

Loading

mmatczuk Jan 18, 2024 •

edited

Loading

mmatczuk Jan 18, 2024 •

edited

Loading

mmatczuk Jan 18, 2024 •

edited

Loading