From 6d53e3c2d7462cbfecb03b68208b1e6e8c0f7903 Mon Sep 17 00:00:00 2001 From: Harshavardhana Date: Mon, 1 Nov 2021 08:04:03 -0700 Subject: [PATCH] reduce number of middleware handlers (#13546) - combine similar looking functionalities into single handlers, and remove unnecessary proxying of the requests at handler layer. - remove bucket forwarding handler as part of default setup add it only if bucket federation is enabled. Improvements observed for 1kiB object reads. ``` ------------------- Operation: GET Operations: 4538555 -> 4595804 * Average: +1.26% (+0.2 MiB/s) throughput, +1.26% (+190.2) obj/s * Fastest: +4.67% (+0.7 MiB/s) throughput, +4.67% (+739.8) obj/s * 50% Median: +1.15% (+0.2 MiB/s) throughput, +1.15% (+173.9) obj/s ``` --- cmd/auth-handler.go | 20 +++ cmd/crossdomain-xml-handler.go | 30 ++--- cmd/endpoint.go | 69 ---------- cmd/gateway-main.go | 2 +- cmd/generic-handlers.go | 238 +++++++++++---------------------- cmd/generic-handlers_test.go | 2 +- cmd/healthcheck-handler.go | 4 + cmd/routers.go | 29 ++-- cmd/server-main.go | 2 +- cmd/test-utils_test.go | 2 +- 10 files changed, 125 insertions(+), 273 deletions(-) diff --git a/cmd/auth-handler.go b/cmd/auth-handler.go index 26052c2c2..9022bb645 100644 --- a/cmd/auth-handler.go +++ b/cmd/auth-handler.go @@ -487,6 +487,26 @@ func setAuthHandler(h http.Handler) http.Handler { // handler for validating incoming authorization headers. return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { aType := getRequestAuthType(r) + if aType == authTypeSigned || aType == authTypeSignedV2 || aType == authTypeStreamingSigned { + // Verify if date headers are set, if not reject the request + amzDate, errCode := parseAmzDateHeader(r) + if errCode != ErrNone { + // All our internal APIs are sensitive towards Date + // header, for all requests where Date header is not + // present we will reject such clients. + writeErrorResponse(r.Context(), w, errorCodes.ToAPIErr(errCode), r.URL) + atomic.AddUint64(&globalHTTPStats.rejectedRequestsTime, 1) + return + } + // Verify if the request date header is shifted by less than globalMaxSkewTime parameter in the past + // or in the future, reject request otherwise. + curTime := UTCNow() + if curTime.Sub(amzDate) > globalMaxSkewTime || amzDate.Sub(curTime) > globalMaxSkewTime { + writeErrorResponse(r.Context(), w, errorCodes.ToAPIErr(ErrRequestTimeTooSkewed), r.URL) + atomic.AddUint64(&globalHTTPStats.rejectedRequestsTime, 1) + return + } + } if isSupportedS3AuthType(aType) || aType == authTypeJWT || aType == authTypeSTS { h.ServeHTTP(w, r) return diff --git a/cmd/crossdomain-xml-handler.go b/cmd/crossdomain-xml-handler.go index ddadb9809..d97ffab0c 100644 --- a/cmd/crossdomain-xml-handler.go +++ b/cmd/crossdomain-xml-handler.go @@ -25,29 +25,21 @@ const crossDomainXML = `= 0 { - proxyRequest(context.TODO(), w, r, globalProxyEndpoints[idx]) - return - } - h.ServeHTTP(w, r) - }) -} - func guessIsBrowserReq(r *http.Request) bool { aType := getRequestAuthType(r) return strings.Contains(r.Header.Get("User-Agent"), "Mozilla") && @@ -174,10 +140,6 @@ func setBrowserRedirectHandler(h http.Handler) http.Handler { }) } -func shouldProxy() bool { - return newObjectLayerFn() == nil -} - // Fetch redirect location if urlPath satisfies certain // criteria. Some special names are considered to be // redirectable, this is purely internal function and @@ -261,31 +223,21 @@ func isAdminReq(r *http.Request) bool { return strings.HasPrefix(r.URL.Path, adminPathPrefix) } -// Adds verification for incoming paths. -func setReservedBucketHandler(h http.Handler) http.Handler { - return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - // For all other requests reject access to reserved buckets - bucketName, _ := request2BucketObjectName(r) - if isMinioReservedBucket(bucketName) || isMinioMetaBucket(bucketName) { - if !guessIsRPCReq(r) && !guessIsBrowserReq(r) && !guessIsHealthCheckReq(r) && !guessIsMetricsReq(r) && !isAdminReq(r) { - writeErrorResponse(r.Context(), w, errorCodes.ToAPIErr(ErrAllAccessDisabled), r.URL) - return - } - } - h.ServeHTTP(w, r) - }) -} - -// Supported Amz date formats. +// Supported amz date formats. var amzDateFormats = []string{ + // Do not change this order, x-amz-date format is usually in + // iso8601Format rest are meant for relaxed handling of other + // odd SDKs that might be out there. + iso8601Format, time.RFC1123, time.RFC1123Z, - iso8601Format, // Add new AMZ date formats here. } // Supported Amz date headers. var amzDateHeaders = []string{ + // Do not chane this order, x-amz-date value should be + // validated first. "x-amz-date", "date", } @@ -314,34 +266,6 @@ func parseAmzDateHeader(req *http.Request) (time.Time, APIErrorCode) { return time.Time{}, ErrMissingDateHeader } -// setTimeValidityHandler to validate parsable time over http header -func setTimeValidityHandler(h http.Handler) http.Handler { - return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - aType := getRequestAuthType(r) - if aType == authTypeSigned || aType == authTypeSignedV2 || aType == authTypeStreamingSigned { - // Verify if date headers are set, if not reject the request - amzDate, errCode := parseAmzDateHeader(r) - if errCode != ErrNone { - // All our internal APIs are sensitive towards Date - // header, for all requests where Date header is not - // present we will reject such clients. - writeErrorResponse(r.Context(), w, errorCodes.ToAPIErr(errCode), r.URL) - atomic.AddUint64(&globalHTTPStats.rejectedRequestsTime, 1) - return - } - // Verify if the request date header is shifted by less than globalMaxSkewTime parameter in the past - // or in the future, reject request otherwise. - curTime := UTCNow() - if curTime.Sub(amzDate) > globalMaxSkewTime || amzDate.Sub(curTime) > globalMaxSkewTime { - writeErrorResponse(r.Context(), w, errorCodes.ToAPIErr(ErrRequestTimeTooSkewed), r.URL) - atomic.AddUint64(&globalHTTPStats.rejectedRequestsTime, 1) - return - } - } - h.ServeHTTP(w, r) - }) -} - // setHttpStatsHandler sets a http Stats handler to gather HTTP statistics func setHTTPStatsHandler(h http.Handler) http.Handler { return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { @@ -420,6 +344,23 @@ func setRequestValidityHandler(h http.Handler) http.Handler { atomic.AddUint64(&globalHTTPStats.rejectedRequestsInvalid, 1) return } + // For all other requests reject access to reserved buckets + bucketName, _ := request2BucketObjectName(r) + if isMinioReservedBucket(bucketName) || isMinioMetaBucket(bucketName) { + if !guessIsRPCReq(r) && !guessIsBrowserReq(r) && !guessIsHealthCheckReq(r) && !guessIsMetricsReq(r) && !isAdminReq(r) { + writeErrorResponse(r.Context(), w, errorCodes.ToAPIErr(ErrAllAccessDisabled), r.URL) + return + } + } + // Deny SSE-C requests if not made over TLS + if !globalIsTLS && (crypto.SSEC.IsRequested(r.Header) || crypto.SSECopy.IsRequested(r.Header)) { + if r.Method == http.MethodHead { + writeErrorResponseHeadersOnly(w, errorCodes.ToAPIErr(ErrInsecureSSECustomerRequest)) + } else { + writeErrorResponse(r.Context(), w, errorCodes.ToAPIErr(ErrInsecureSSECustomerRequest), r.URL) + } + return + } h.ServeHTTP(w, r) }) } @@ -429,8 +370,7 @@ func setRequestValidityHandler(h http.Handler) http.Handler { // is obtained from centralized etcd configuration service. func setBucketForwardingHandler(h http.Handler) http.Handler { return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - if globalDNSConfig == nil || len(globalDomainNames) == 0 || !globalBucketFederation || - guessIsHealthCheckReq(r) || guessIsMetricsReq(r) || + if guessIsHealthCheckReq(r) || guessIsMetricsReq(r) || guessIsRPCReq(r) || guessIsLoginSTSReq(r) || isAdminReq(r) { h.ServeHTTP(w, r) return @@ -491,29 +431,23 @@ func setBucketForwardingHandler(h http.Handler) http.Handler { }) } -// customHeaderHandler sets x-amz-request-id header. -// Previously, this value was set right before a response was sent to -// the client. So, logger and Error response XML were not using this -// value. This is set here so that this header can be logged as -// part of the log entry, Error response XML and auditing. -func addCustomHeaders(h http.Handler) http.Handler { - return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - // Set custom headers such as x-amz-request-id for each request. - w.Header().Set(xhttp.AmzRequestID, mustGetRequestID(UTCNow())) - h.ServeHTTP(logger.NewResponseWriter(w), r) - }) -} - -// addSecurityHeaders adds various HTTP(S) response headers. +// addCustomHeaders adds various HTTP(S) response headers. // Security Headers enable various security protections behaviors in the client's browser. -func addSecurityHeaders(h http.Handler) http.Handler { +func addCustomHeaders(h http.Handler) http.Handler { return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { header := w.Header() header.Set("X-XSS-Protection", "1; mode=block") // Prevents against XSS attacks header.Set("Content-Security-Policy", "block-all-mixed-content") // prevent mixed (HTTP / HTTPS content) header.Set("X-Content-Type-Options", "nosniff") // Prevent mime-sniff header.Set("Strict-Transport-Security", "max-age=31536000; includeSubDomains") // HSTS mitigates variants of MITM attacks - h.ServeHTTP(w, r) + + // Previously, this value was set right before a response was sent to + // the client. So, logger and Error response XML were not using this + // value. This is set here so that this header can be logged as + // part of the log entry, Error response XML and auditing. + // Set custom headers such as x-amz-request-id for each request. + w.Header().Set(xhttp.AmzRequestID, mustGetRequestID(UTCNow())) + h.ServeHTTP(logger.NewResponseWriter(w), r) }) } @@ -521,32 +455,16 @@ func addSecurityHeaders(h http.Handler) http.Handler { // `panic(logger.ErrCritical)` as done by `logger.CriticalIf`. // // It should be always the first / highest HTTP handler. -type criticalErrorHandler struct{ handler http.Handler } - -func (h criticalErrorHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) { - defer func() { - if err := recover(); err == logger.ErrCritical { // handle - writeErrorResponse(r.Context(), w, errorCodes.ToAPIErr(ErrInternalError), r.URL) - return - } else if err != nil { - panic(err) // forward other panic calls - } - }() - h.handler.ServeHTTP(w, r) -} - -// sseTLSHandler enforces certain rules for SSE requests which are made / must be made over TLS. -func setSSETLSHandler(h http.Handler) http.Handler { +func setCriticalErrorHandler(h http.Handler) http.Handler { return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - // Deny SSE-C requests if not made over TLS - if !globalIsTLS && (crypto.SSEC.IsRequested(r.Header) || crypto.SSECopy.IsRequested(r.Header)) { - if r.Method == http.MethodHead { - writeErrorResponseHeadersOnly(w, errorCodes.ToAPIErr(ErrInsecureSSECustomerRequest)) - } else { - writeErrorResponse(r.Context(), w, errorCodes.ToAPIErr(ErrInsecureSSECustomerRequest), r.URL) + defer func() { + if err := recover(); err == logger.ErrCritical { // handle + writeErrorResponse(r.Context(), w, errorCodes.ToAPIErr(ErrInternalError), r.URL) + return + } else if err != nil { + panic(err) // forward other panic calls } - return - } + }() h.ServeHTTP(w, r) }) } diff --git a/cmd/generic-handlers_test.go b/cmd/generic-handlers_test.go index 098507c5f..85bc5ad96 100644 --- a/cmd/generic-handlers_test.go +++ b/cmd/generic-handlers_test.go @@ -155,7 +155,7 @@ func TestSSETLSHandler(t *testing.T) { r.Header = test.Header r.URL = test.URL - h := setSSETLSHandler(okHandler) + h := setRequestValidityHandler(okHandler) h.ServeHTTP(w, r) switch { diff --git a/cmd/healthcheck-handler.go b/cmd/healthcheck-handler.go index 30eaeadad..fc59f14dd 100644 --- a/cmd/healthcheck-handler.go +++ b/cmd/healthcheck-handler.go @@ -28,6 +28,10 @@ import ( const unavailable = "offline" +func shouldProxy() bool { + return newObjectLayerFn() == nil +} + // ClusterCheckHandler returns if the server is ready for requests. func ClusterCheckHandler(w http.ResponseWriter, r *http.Request) { if globalIsGateway { diff --git a/cmd/routers.go b/cmd/routers.go index e812f0d9e..62a7b869f 100644 --- a/cmd/routers.go +++ b/cmd/routers.go @@ -40,41 +40,24 @@ func registerDistErasureRouters(router *mux.Router, endpointServerPools Endpoint // List of some generic handlers which are applied for all incoming requests. var globalHandlers = []mux.MiddlewareFunc{ - // filters HTTP headers which are treated as metadata and are reserved - // for internal use only. - filterReservedMetadata, - // Enforce rules specific for TLS requests - setSSETLSHandler, // Auth handler verifies incoming authorization headers and // routes them accordingly. Client receives a HTTP error for // invalid/unsupported signatures. - setAuthHandler, + // // Validates all incoming requests to have a valid date header. - setTimeValidityHandler, - // Validates if incoming request is for restricted buckets. - setReservedBucketHandler, + setAuthHandler, // Redirect some pre-defined browser request paths to a static location prefix. setBrowserRedirectHandler, // Adds 'crossdomain.xml' policy handler to serve legacy flash clients. setCrossDomainPolicy, - // Limits all header sizes to a maximum fixed limit - setRequestHeaderSizeLimitHandler, - // Limits all requests size to a maximum fixed limit - setRequestSizeLimitHandler, + // Limits all body and header sizes to a maximum fixed limit + setRequestLimitHandler, // Network statistics setHTTPStatsHandler, // Validate all the incoming requests. setRequestValidityHandler, - // Forward path style requests to actual host in a bucket federated setup. - setBucketForwardingHandler, - // set HTTP security headers such as Content-Security-Policy. - addSecurityHeaders, // set x-amz-request-id header. addCustomHeaders, - // add redirect handler to redirect - // requests when object layer is not - // initialized. - setRedirectHandler, // Add new handlers here. } @@ -104,6 +87,10 @@ func configureServerHandler(endpointServerPools EndpointServerPools) (http.Handl // Add API router registerAPIRouter(router) + // Enable bucket forwarding handler only if bucket federation is enabled. + if globalDNSConfig != nil && globalBucketFederation { + globalHandlers = append(globalHandlers, setBucketForwardingHandler) + } router.Use(globalHandlers...) return router, nil diff --git a/cmd/server-main.go b/cmd/server-main.go index d2d8075e5..2a8dcf9f9 100644 --- a/cmd/server-main.go +++ b/cmd/server-main.go @@ -511,7 +511,7 @@ func serverMain(ctx *cli.Context) { addrs = append(addrs, globalMinioAddr) } - httpServer := xhttp.NewServer(addrs, criticalErrorHandler{corsHandler(handler)}, getCert) + httpServer := xhttp.NewServer(addrs, setCriticalErrorHandler(corsHandler(handler)), getCert) httpServer.BaseContext = func(listener net.Listener) context.Context { return GlobalContext } diff --git a/cmd/test-utils_test.go b/cmd/test-utils_test.go index a1c06cba0..250d5f987 100644 --- a/cmd/test-utils_test.go +++ b/cmd/test-utils_test.go @@ -334,7 +334,7 @@ func UnstartedTestServer(t TestErrHandler, instanceType string) TestServer { } // Run TestServer. - testServer.Server = httptest.NewUnstartedServer(criticalErrorHandler{corsHandler(httpHandler)}) + testServer.Server = httptest.NewUnstartedServer(setCriticalErrorHandler(corsHandler(httpHandler))) globalObjLayerMutex.Lock() globalObjectAPI = objLayer