Combine obtaining resource, host, method into one operation (#6465)

This also adds a reduced timeout for errant connections, to
be quickly closed if they can't even send HTTP headers properly.

Fixes #6459

cmd/http/listener.go

@@ -73,11 +73,40 @@ func isHTTPMethod(s string) bool {
 	return false
 }
 
-func getResourceHost(bufConn *BufConn, maxHeaderBytes, methodLen int) (resource string, host string, err error) {
+func getPlainText(bufConn *BufConn) (bool, error) {
+	defer bufConn.setReadTimeout()
+
+	if bufConn.canSetReadDeadline() {
+		// Set deadline such that we close the connection quickly
+		// of no data was received from the Peek()
+		bufConn.SetReadDeadline(time.Now().UTC().Add(time.Second * 3))
+	}
+	b, err := bufConn.Peek(1)
+	if err != nil {
+		return false, err
+	}
+	for _, method := range methods {
+		if b[0] == method[0] {
+			return true, nil
+		}
+	}
+	return false, nil
+}
+
+func getResourceHost(bufConn *BufConn, maxHeaderBytes int) (resource string, method string, host string, err error) {
+	defer bufConn.setReadTimeout()
+
 	var data []byte
-	for dataLen := 0; methodLen+dataLen < maxHeaderBytes; dataLen += 8 {
+	for dataLen := 1; dataLen < maxHeaderBytes; dataLen++ {
-		if data, err = bufConn.Peek(methodLen + dataLen); err != nil {
+		if bufConn.canSetReadDeadline() {
-			return "", "", err
+			// Set deadline such that we close the connection quickly
+			// of no data was received from the Peek()
+			bufConn.SetReadDeadline(time.Now().UTC().Add(time.Second * 3))
+		}
+
+		data, err = bufConn.bufReader.Peek(dataLen)
+		if err != nil {
+			return "", "", "", err
 		}
 
 		tokens := strings.Split(string(data), "\n")
@@ -85,8 +114,17 @@ func getResourceHost(bufConn *BufConn, maxHeaderBytes, methodLen int) (resource
 			continue
 		}
 
-		if resource == "" {
+		if method == "" && resource == "" {
-			resource = strings.SplitN(tokens[0][methodLen:], " ", 2)[0]
+			if i := strings.IndexByte(tokens[0], ' '); i == -1 {
+				return "", "", "", fmt.Errorf("malformed HTTP request %s, from %s", tokens[0], bufConn.LocalAddr())
+			}
+			httpTokens := strings.SplitN(tokens[0], " ", 3)
+			if len(httpTokens) < 3 {
+				return "", "", "", fmt.Errorf("malformed HTTP request %s, from %s", tokens[0], bufConn.LocalAddr())
+			}
+
+			method = httpTokens[0]
+			resource = httpTokens[1]
 		}
 
 		for _, token := range tokens[1:] {
@@ -99,7 +137,7 @@ func getResourceHost(bufConn *BufConn, maxHeaderBytes, methodLen int) (resource
 			token = strings.ToLower(token)
 			if strings.HasPrefix(token, "host: ") {
 				host = strings.TrimPrefix(strings.TrimSuffix(token, "\r"), "host: ")
-				return resource, host, nil
+				return resource, method, host, nil
 			}
 		}
 
@@ -108,7 +146,7 @@ func getResourceHost(bufConn *BufConn, maxHeaderBytes, methodLen int) (resource
 		}
 	}
 
-	return resource, host, nil
+	return "", "", "", fmt.Errorf("malformed HTTP request from %s", bufConn.LocalAddr())
 }
 
 type acceptResult struct {
@@ -175,9 +213,44 @@ func (listener *httpListener) start() {
 		tcpConn.SetKeepAlivePeriod(listener.tcpKeepAliveTimeout)
 
 		bufconn := newBufConn(tcpConn, listener.readTimeout, listener.writeTimeout)
+		if listener.tlsConfig != nil {
+			ok, err := getPlainText(bufconn)
+			if err != nil {
+				// Peek could fail legitimately when clients abruptly close
+				// connection. E.g. Chrome browser opens connections speculatively to
+				// speed up loading of a web page. Peek may also fail due to network
+				// saturation on a transport with read timeout set. All other kind of
+				// errors should be logged for further investigation. Thanks @brendanashworth.
+				if !isRoutineNetErr(err) {
+					reqInfo := (&logger.ReqInfo{}).AppendTags("remoteAddr", bufconn.RemoteAddr().String())
+					reqInfo.AppendTags("localAddr", bufconn.LocalAddr().String())
+					ctx := logger.SetReqInfo(context.Background(), reqInfo)
+					logger.LogIf(ctx, err)
+				}
+				bufconn.Close()
+				return
+			}
+			if ok {
+				// As TLS is configured and we got plain text HTTP request,
+				// return 403 (forbidden) error.
+				bufconn.Write(sslRequiredErrMsg)
+				bufconn.Close()
+				return
+			}
+			// As the listener is configured with TLS, try to do TLS handshake, drop the connection if it fails.
+			tlsConn := tls.Server(bufconn, listener.tlsConfig)
+			if err := tlsConn.Handshake(); err != nil {
+				reqInfo := (&logger.ReqInfo{}).AppendTags("remoteAddr", bufconn.RemoteAddr().String())
+				reqInfo.AppendTags("localAddr", bufconn.LocalAddr().String())
+				ctx := logger.SetReqInfo(context.Background(), reqInfo)
+				logger.LogIf(ctx, err)
+				bufconn.Close()
+				return
+			}
+			bufconn = newBufConn(tlsConn, listener.readTimeout, listener.writeTimeout)
+		}
 
-		// Peek bytes of maximum length of all HTTP methods.
+		resource, method, host, err := getResourceHost(bufconn, listener.maxHeaderBytes)
-		data, err := bufconn.Peek(methodMaxLen)
 		if err != nil {
 			// Peek could fail legitimately when clients abruptly close
 			// connection. E.g. Chrome browser opens connections speculatively to
@@ -195,110 +268,29 @@ func (listener *httpListener) start() {
 		}
 
 		// Return bufconn if read data is a valid HTTP method.
-		tokens := strings.SplitN(string(data), " ", 2)
+		if !isHTTPMethod(method) {
-		if isHTTPMethod(tokens[0]) {
+			reqInfo := (&logger.ReqInfo{}).AppendTags("remoteAddr", bufconn.RemoteAddr().String())
-			if listener.tlsConfig != nil {
+			reqInfo.AppendTags("localAddr", bufconn.LocalAddr().String())
-				// As TLS is configured and we got plain text HTTP request,
+			ctx := logger.SetReqInfo(context.Background(), reqInfo)
-				// return 403 (forbidden) error.
+			logger.LogIf(ctx, fmt.Errorf("malformed HTTP invalid HTTP method %s", method))
-				bufconn.Write(sslRequiredErrMsg)
-				bufconn.Close()
-				return
-			}
 
-			var resource, host string
+			bufconn.Close()
-			if resource, host, err = getResourceHost(bufconn, listener.maxHeaderBytes, len(tokens[0])+1); err != nil {
-				if !isRoutineNetErr(err) {
-					reqInfo := (&logger.ReqInfo{}).AppendTags("remoteAddr", bufconn.RemoteAddr().String())
-					reqInfo.AppendTags("localAddr", bufconn.LocalAddr().String())
-					ctx := logger.SetReqInfo(context.Background(), reqInfo)
-					logger.LogIf(ctx, err)
-				}
-				bufconn.Close()
-				return
-			}
-
-			header := make(http.Header)
-			if host != "" {
-				header.Add("Host", host)
-			}
-			bufconn.setRequest(&http.Request{
-				Method: tokens[0],
-				URL:    &url.URL{Path: resource},
-				Host:   bufconn.LocalAddr().String(),
-				Header: header,
-			})
-			bufconn.setUpdateFuncs(listener.updateBytesReadFunc, listener.updateBytesWrittenFunc)
-
-			send(acceptResult{bufconn, nil}, doneCh)
 			return
 		}
 
-		if listener.tlsConfig != nil {
+		header := make(http.Header)
-			// As the listener is configured with TLS, try to do TLS handshake, drop the connection if it fails.
+		if host != "" {
-			tlsConn := tls.Server(bufconn, listener.tlsConfig)
+			header.Add("Host", host)
-			if err = tlsConn.Handshake(); err != nil {
-				reqInfo := (&logger.ReqInfo{}).AppendTags("remoteAddr", bufconn.RemoteAddr().String())
-				reqInfo.AppendTags("localAddr", bufconn.LocalAddr().String())
-				ctx := logger.SetReqInfo(context.Background(), reqInfo)
-				logger.LogIf(ctx, err)
-				bufconn.Close()
-				return
-			}
-
-			// Check whether the connection contains HTTP request or not.
-			bufconn = newBufConn(tlsConn, listener.readTimeout, listener.writeTimeout)
-
-			// Peek bytes of maximum length of all HTTP methods.
-			data, err = bufconn.Peek(methodMaxLen)
-			if err != nil {
-				if !isRoutineNetErr(err) {
-					reqInfo := (&logger.ReqInfo{}).AppendTags("remoteAddr", bufconn.RemoteAddr().String())
-					reqInfo.AppendTags("localAddr", bufconn.LocalAddr().String())
-					ctx := logger.SetReqInfo(context.Background(), reqInfo)
-					logger.LogIf(ctx, err)
-				}
-				bufconn.Close()
-				return
-			}
-
-			// Return bufconn if read data is a valid HTTP method.
-			tokens := strings.SplitN(string(data), " ", 2)
-			if isHTTPMethod(tokens[0]) {
-				var resource, host string
-				if resource, host, err = getResourceHost(bufconn, listener.maxHeaderBytes, len(tokens[0])+1); err != nil {
-					if !isRoutineNetErr(err) {
-						reqInfo := (&logger.ReqInfo{}).AppendTags("remoteAddr", bufconn.RemoteAddr().String())
-						reqInfo.AppendTags("localAddr", bufconn.LocalAddr().String())
-						ctx := logger.SetReqInfo(context.Background(), reqInfo)
-						logger.LogIf(ctx, err)
-					}
-					bufconn.Close()
-					return
-				}
-
-				header := make(http.Header)
-				if host != "" {
-					header.Add("Host", host)
-				}
-				bufconn.setRequest(&http.Request{
-					Method: tokens[0],
-					URL:    &url.URL{Path: resource},
-					Host:   bufconn.LocalAddr().String(),
-					Header: header,
-				})
-				bufconn.setUpdateFuncs(listener.updateBytesReadFunc, listener.updateBytesWrittenFunc)
-
-				send(acceptResult{bufconn, nil}, doneCh)
-				return
-			}
 		}
-		reqInfo := (&logger.ReqInfo{}).AppendTags("remoteAddr", bufconn.RemoteAddr().String())
+		bufconn.setRequest(&http.Request{
-		reqInfo.AppendTags("localAddr", bufconn.LocalAddr().String())
+			Method: method,
-		ctx := logger.SetReqInfo(context.Background(), reqInfo)
+			URL:    &url.URL{Path: resource},
-		logger.LogIf(ctx, err)
+			Host:   bufconn.LocalAddr().String(),
+			Header: header,
+		})
+		bufconn.setUpdateFuncs(listener.updateBytesReadFunc, listener.updateBytesWrittenFunc)
 
-		bufconn.Close()
+		send(acceptResult{bufconn, nil}, doneCh)
-		return
 	}
 
 	// Closure to handle TCPListener until done channel is closed.

cmd/http/listener_test.go

@@ -393,11 +393,11 @@ func TestHTTPListenerAccept(t *testing.T) {
 		{[]string{"localhost:0"}, nil, "GET / HTTP/1.0\r\nHost: example.org\r\n\r\n", "200 OK\r\n", "GET / HTTP/1.0\r\n"},
 		{[]string{nonLoopBackIP + ":0"}, nil, "POST / HTTP/1.0\r\nHost: example.org\r\n\r\n", "200 OK\r\n", "POST / HTTP/1.0\r\n"},
 		{[]string{nonLoopBackIP + ":0"}, nil, "HEAD / HTTP/1.0\r\nhost: example.org\r\n\r\n", "200 OK\r\n", "HEAD / HTTP/1.0\r\n"},
-		{[]string{"127.0.0.1:0", nonLoopBackIP + ":0"}, nil, "CONNECT \r\nHost: www.example.org\r\n\r\n", "200 OK\r\n", "CONNECT \r\n"},
+		{[]string{"127.0.0.1:0", nonLoopBackIP + ":0"}, nil, "CONNECT / HTTP/1.0\r\nHost: www.example.org\r\n\r\n", "200 OK\r\n", "CONNECT / HTTP/1.0\r\n"},
 		{[]string{"localhost:0"}, tlsConfig, "GET / HTTP/1.0\r\nHost: example.org\r\n\r\n", "200 OK\r\n", "GET / HTTP/1.0\r\n"},
 		{[]string{nonLoopBackIP + ":0"}, tlsConfig, "POST / HTTP/1.0\r\nHost: example.org\r\n\r\n", "200 OK\r\n", "POST / HTTP/1.0\r\n"},
 		{[]string{nonLoopBackIP + ":0"}, tlsConfig, "HEAD / HTTP/1.0\r\nhost: example.org\r\n\r\n", "200 OK\r\n", "HEAD / HTTP/1.0\r\n"},
-		{[]string{"127.0.0.1:0", nonLoopBackIP + ":0"}, tlsConfig, "CONNECT \r\nHost: www.example.org\r\n\r\n", "200 OK\r\n", "CONNECT \r\n"},
+		{[]string{"127.0.0.1:0", nonLoopBackIP + ":0"}, tlsConfig, "CONNECT / HTTP/1.0\r\nHost: www.example.org\r\n\r\n", "200 OK\r\n", "CONNECT / HTTP/1.0\r\n"},
 	}
 
 	for i, testCase := range testCases {
@@ -643,11 +643,25 @@ func TestHTTPListenerAcceptError(t *testing.T) {
 				}
 			}()
 
+			if !testCase.secureClient && testCase.tlsConfig != nil {
+				buf := make([]byte, len(sslRequiredErrMsg))
+				var n int
+				n, err = io.ReadFull(conn, buf)
+				if err != nil {
+					t.Fatalf("Test %d: reply read: expected = <nil> got = %v", i+1, err)
+				} else if n != len(buf) {
+					t.Fatalf("Test %d: reply length: expected = %v got = %v", i+1, len(buf), n)
+				} else if !bytes.Equal(buf, sslRequiredErrMsg) {
+					t.Fatalf("Test %d: reply: expected = %v got = %v", i+1, string(sslRequiredErrMsg), string(buf))
+				}
+				continue
+			}
+
 			_, err = bufio.NewReader(conn).ReadString('\n')
 			if err == nil {
-				t.Fatalf("Test %d: reply read: expected = EOF got = <nil>", i+1)
+				t.Errorf("Test %d: reply read: expected = EOF got = <nil>", i+1)
 			} else if err.Error() != "EOF" {
-				t.Fatalf("Test %d: reply read: expected = EOF got = %v", i+1, err)
+				t.Errorf("Test %d: reply read: expected = EOF got = %v", i+1, err)
 			}
 
 			conn.Close()