From e3777b1dd9975d55a2d0a4998f245001f414c04d Mon Sep 17 00:00:00 2001 From: Harshavardhana Date: Thu, 13 Sep 2018 05:47:03 -0700 Subject: [PATCH] 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 | 204 ++++++++++++++++++-------------------- cmd/http/listener_test.go | 22 +++- 2 files changed, 116 insertions(+), 110 deletions(-) diff --git a/cmd/http/listener.go b/cmd/http/listener.go index 7647f0345..f7b86d741 100644 --- a/cmd/http/listener.go +++ b/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 { - if data, err = bufConn.Peek(methodLen + dataLen); err != nil { - return "", "", err + for dataLen := 1; dataLen < maxHeaderBytes; dataLen++ { + 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)) + } + + 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 == "" { - resource = strings.SplitN(tokens[0][methodLen:], " ", 2)[0] + if method == "" && resource == "" { + 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,38 +213,14 @@ func (listener *httpListener) start() { tcpConn.SetKeepAlivePeriod(listener.tcpKeepAliveTimeout) bufconn := newBufConn(tcpConn, listener.readTimeout, listener.writeTimeout) - - // Peek bytes of maximum length of all HTTP methods. - 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 - // 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 - } - - // Return bufconn if read data is a valid HTTP method. - tokens := strings.SplitN(string(data), " ", 2) - if isHTTPMethod(tokens[0]) { - if listener.tlsConfig != nil { - // As TLS is configured and we got plain text HTTP request, - // return 403 (forbidden) error. - bufconn.Write(sslRequiredErrMsg) - bufconn.Close() - return - } - - var resource, host string - if resource, host, err = getResourceHost(bufconn, listener.maxHeaderBytes, len(tokens[0])+1); err != nil { + 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()) @@ -216,27 +230,16 @@ func (listener *httpListener) start() { bufconn.Close() return } - - header := make(http.Header) - if host != "" { - header.Add("Host", host) + if ok { + // As TLS is configured and we got plain text HTTP request, + // return 403 (forbidden) error. + bufconn.Write(sslRequiredErrMsg) + bufconn.Close() + return } - 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 { // 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 { + 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) @@ -244,61 +247,50 @@ func (listener *httpListener) start() { 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 + resource, method, host, err := getResourceHost(bufconn, listener.maxHeaderBytes) + 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 + } - // 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 - } + // Return bufconn if read data is a valid HTTP method. + if !isHTTPMethod(method) { + reqInfo := (&logger.ReqInfo{}).AppendTags("remoteAddr", bufconn.RemoteAddr().String()) + reqInfo.AppendTags("localAddr", bufconn.LocalAddr().String()) + ctx := logger.SetReqInfo(context.Background(), reqInfo) + logger.LogIf(ctx, fmt.Errorf("malformed HTTP invalid HTTP method %s", method)) - 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 - } + bufconn.Close() + return } - 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: method, + URL: &url.URL{Path: resource}, + Host: bufconn.LocalAddr().String(), + Header: header, + }) + bufconn.setUpdateFuncs(listener.updateBytesReadFunc, listener.updateBytesWrittenFunc) + + send(acceptResult{bufconn, nil}, doneCh) } // Closure to handle TCPListener until done channel is closed. diff --git a/cmd/http/listener_test.go b/cmd/http/listener_test.go index eef072438..abbff321d 100644 --- a/cmd/http/listener_test.go +++ b/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 = 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 = ", i+1) + t.Errorf("Test %d: reply read: expected = EOF got = ", 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()