From f0dff3fadab35e2bd692f52eb75700fdce151b52 Mon Sep 17 00:00:00 2001 From: Seth Hoenig Date: Mon, 14 Dec 2020 14:34:36 -0600 Subject: [PATCH] agent: revert use of http connlimit https://github.com/hashicorp/nomad/pull/9608 introduced the use of the built-in HTTP 429 response handler provided by go-connlimit. There is concern though around plausible DOS attacks that need to be addressed, so this PR reverts that functionality. It keeps a fix in the tests around the use of an HTTPS enabled client for when the server is listening on HTTPS. Previously, the tests would fail deterministically with io.EOF because that's how the TLS server terminates invalid connections. Now, the result is much less deterministic. The state of the client connection and the server socket depends on when the connection is closed and how far along the handshake was. --- command/agent/http.go | 10 ++---- command/agent/http_test.go | 74 ++++++++++++++++++++++++-------------- 2 files changed, 49 insertions(+), 35 deletions(-) diff --git a/command/agent/http.go b/command/agent/http.go index 66fed38dfc5..bbf071cbe0e 100644 --- a/command/agent/http.go +++ b/command/agent/http.go @@ -40,10 +40,6 @@ const ( // MissingRequestID is a placeholder if we cannot retrieve a request // UUID from context MissingRequestID = "" - - // HTTPConnStateFuncWriteTimeout is how long to try to write conn state errors - // before closing the connection - HTTPConnStateFuncWriteTimeout = 10 * time.Millisecond ) var ( @@ -175,19 +171,17 @@ func makeConnState(isTLS bool, handshakeTimeout time.Duration, connLimit int) fu // Still return the connection limiter return connlimit.NewLimiter(connlimit.Config{ MaxConnsPerClientIP: connLimit, - }).HTTPConnStateFuncWithDefault429Handler(HTTPConnStateFuncWriteTimeout) + }).HTTPConnStateFunc() } - return nil } if connLimit > 0 { // Return conn state callback with connection limiting and a // handshake timeout. - connLimiter := connlimit.NewLimiter(connlimit.Config{ MaxConnsPerClientIP: connLimit, - }).HTTPConnStateFuncWithDefault429Handler(HTTPConnStateFuncWriteTimeout) + }).HTTPConnStateFunc() return func(conn net.Conn, state http.ConnState) { switch state { diff --git a/command/agent/http_test.go b/command/agent/http_test.go index 68f2ac868d7..ee252ab97c5 100644 --- a/command/agent/http_test.go +++ b/command/agent/http_test.go @@ -886,8 +886,8 @@ func TestHTTPServer_Limits_OK(t *testing.T) { cafile = "../../helper/tlsutil/testdata/ca.pem" foocert = "../../helper/tlsutil/testdata/nomad-foo.pem" fookey = "../../helper/tlsutil/testdata/nomad-foo-key.pem" - maxConns = 10 // limit must be < this for testing - bufSize = 1 * 1024 // enough for 429 error message + maxConns = 10 // limit must be < this for testing + bufSize = 1 // enough to know if something was written ) cases := []struct { @@ -1055,20 +1055,16 @@ func TestHTTPServer_Limits_OK(t *testing.T) { } } - dial := func(t *testing.T, addr string, useTLS bool) net.Conn { + dial := func(t *testing.T, addr string, useTLS bool) (net.Conn, error) { if useTLS { cert, err := tls.LoadX509KeyPair(foocert, fookey) require.NoError(t, err) - conn, err := tls.Dial("tcp", addr, &tls.Config{ + return tls.Dial("tcp", addr, &tls.Config{ Certificates: []tls.Certificate{cert}, InsecureSkipVerify: true, // good enough }) - require.NoError(t, err) - return conn } else { - conn, err := net.DialTimeout("tcp", addr, 1*time.Second) - require.NoError(t, err) - return conn + return net.DialTimeout("tcp", addr, 1*time.Second) } } @@ -1079,7 +1075,9 @@ func TestHTTPServer_Limits_OK(t *testing.T) { conns := make([]net.Conn, limit) errCh := make(chan error, limit) for i := range conns { - conns[i] = dial(t, addr, useTLS) + conn, err := dial(t, addr, useTLS) + require.NoError(t, err) + conns[i] = conn go func(i int) { buf := []byte{0} @@ -1098,22 +1096,44 @@ func TestHTTPServer_Limits_OK(t *testing.T) { case <-time.After(500 * time.Millisecond): } - // Assert a new connection is dropped - conn := dial(t, addr, useTLS) - - defer func() { - require.NoError(t, conn.Close()) - }() - - deadline := time.Now().Add(10 * time.Second) - require.NoError(t, conn.SetReadDeadline(deadline)) - - buf := make([]byte, bufSize) - n, err := conn.Read(buf) - - require.NoError(t, err) - require.NotZero(t, n) - require.True(t, strings.HasPrefix(string(buf), "HTTP/1.1 429 Too Many Requests")) + // Create a new connection that will go over the connection limit. + limitConn, err := dial(t, addr, useTLS) + + // At this point, the state of the connection + handshake are up in the + // air. + // + // 1) dial failed, handshake never started + // => Conn is nil + io.EOF + // 2) dial completed, handshake failed + // => Conn is malformed + (net.OpError OR io.EOF) + // 3) dial completed, handshake succeeded + // => Conn is not nil + no error, however using the Conn should + // result in io.EOF + // + // At no point should Nomad actually write through the limited Conn. + + if limitConn == nil || err != nil { + // Case 1 or Case 2 - returned Conn is useless and the error should + // be one of: + // "EOF" + // "closed network connection" + // "connection reset by peer" + msg := err.Error() + acceptable := strings.Contains(msg, "EOF") || + strings.Contains(msg, "closed network connection") || + strings.Contains(msg, "connection reset by peer") + require.True(t, acceptable) + } else { + // Case 3 - returned Conn is usable, but Nomad should not write + // anything before closing it. + buf := make([]byte, bufSize) + deadline := time.Now().Add(10 * time.Second) + require.NoError(t, limitConn.SetReadDeadline(deadline)) + n, err := limitConn.Read(buf) + require.Equal(t, io.EOF, err) + require.Zero(t, n) + require.NoError(t, limitConn.Close()) + } // Assert existing connections are ok require.Len(t, errCh, 0) @@ -1161,7 +1181,7 @@ func TestHTTPServer_Limits_OK(t *testing.T) { if tc.assertLimit { // There's a race between assertTimeout(false) closing // its connection and the HTTP server noticing and - // untracking it. Since there's no way to coordinate + // un-tracking it. Since there's no way to coordinate // when this occurs, sleeping is the only way to avoid // asserting limits before the timed out connection is // untracked.