From fdf691fdccd5e922ebab5aaccf150a622b17c983 Mon Sep 17 00:00:00 2001 From: Andreas Auernhammer Date: Wed, 17 Oct 2018 04:22:09 +0200 Subject: [PATCH] move SSE-C TLS enforcement into generic handler (#6639) This commit moves the check that SSE-C requests must be made over TLS into a generic HTTP handler. Since the HTTP server uses custom TCP connection handling it is not possible to use `http.Request.TLS` to check for TLS connections. So using `globalIsSSL` is the only option to detect whether the request is made over TLS. By extracting this check into a separate handler it's possible to refactor other parts of the SSE handling code further. --- cmd/api-errors.go | 2 - cmd/api-errors_test.go | 1 - cmd/encryption-v1.go | 15 --- cmd/encryption-v1_test.go | 223 ----------------------------------- cmd/generic-handlers.go | 15 +++ cmd/generic-handlers_test.go | 37 ++++++ cmd/routers.go | 2 + 7 files changed, 54 insertions(+), 241 deletions(-) diff --git a/cmd/api-errors.go b/cmd/api-errors.go index 89946cf43..4ba957d6d 100644 --- a/cmd/api-errors.go +++ b/cmd/api-errors.go @@ -1469,8 +1469,6 @@ func toAPIErrorCode(err error) (apiErr APIErrorCode) { apiErr = ErrInvalidEncryptionParameters case crypto.ErrInvalidEncryptionMethod: apiErr = ErrInvalidEncryptionMethod - case errInsecureSSERequest: - apiErr = ErrInsecureSSECustomerRequest case crypto.ErrInvalidCustomerAlgorithm: apiErr = ErrInvalidSSECustomerAlgorithm case crypto.ErrInvalidCustomerKey: diff --git a/cmd/api-errors_test.go b/cmd/api-errors_test.go index acfe16735..723580f0f 100644 --- a/cmd/api-errors_test.go +++ b/cmd/api-errors_test.go @@ -52,7 +52,6 @@ var toAPIErrorCodeTests = []struct { {err: errSignatureMismatch, errCode: ErrSignatureDoesNotMatch}, // SSE-C errors - {err: errInsecureSSERequest, errCode: ErrInsecureSSECustomerRequest}, {err: crypto.ErrInvalidCustomerAlgorithm, errCode: ErrInvalidSSECustomerAlgorithm}, {err: crypto.ErrMissingCustomerKey, errCode: ErrMissingSSECustomerKey}, {err: crypto.ErrInvalidCustomerKey, errCode: ErrInvalidSSECustomerKey}, diff --git a/cmd/encryption-v1.go b/cmd/encryption-v1.go index 4e8759baf..8cc5d0842 100644 --- a/cmd/encryption-v1.go +++ b/cmd/encryption-v1.go @@ -37,7 +37,6 @@ import ( var ( // AWS errors for invalid SSE-C requests. - errInsecureSSERequest = errors.New("SSE-C requests require TLS connections") errEncryptedObject = errors.New("The object was stored using a form of SSE") errInvalidSSEParameters = errors.New("The SSE-C key for key-rotation is not correct") // special access denied errKMSNotConfigured = errors.New("KMS not configured for a server side encrypted object") @@ -105,13 +104,6 @@ func isEncryptedMultipart(objInfo ObjectInfo) bool { // ParseSSECopyCustomerRequest parses the SSE-C header fields of the provided request. // It returns the client provided key on success. func ParseSSECopyCustomerRequest(h http.Header, metadata map[string]string) (key []byte, err error) { - if !globalIsSSL { // minio only supports HTTP or HTTPS requests not both at the same time - // we cannot use r.TLS == nil here because Go's http implementation reflects on - // the net.Conn and sets the TLS field of http.Request only if it's an tls.Conn. - // Minio uses a BufConn (wrapping a tls.Conn) so the type check within the http package - // will always fail -> r.TLS is always nil even for TLS requests. - return nil, errInsecureSSERequest - } if crypto.S3.IsEncrypted(metadata) && crypto.SSECopy.IsRequested(h) { return nil, crypto.ErrIncompatibleEncryptionMethod } @@ -128,13 +120,6 @@ func ParseSSECustomerRequest(r *http.Request) (key []byte, err error) { // ParseSSECustomerHeader parses the SSE-C header fields and returns // the client provided key on success. func ParseSSECustomerHeader(header http.Header) (key []byte, err error) { - if !globalIsSSL { // minio only supports HTTP or HTTPS requests not both at the same time - // we cannot use r.TLS == nil here because Go's http implementation reflects on - // the net.Conn and sets the TLS field of http.Request only if it's an tls.Conn. - // Minio uses a BufConn (wrapping a tls.Conn) so the type check within the http package - // will always fail -> r.TLS is always nil even for TLS requests. - return nil, errInsecureSSERequest - } if crypto.S3.IsRequested(header) && crypto.SSEC.IsRequested(header) { return key, crypto.ErrIncompatibleEncryptionMethod } diff --git a/cmd/encryption-v1_test.go b/cmd/encryption-v1_test.go index cd8121c9f..f2af329f6 100644 --- a/cmd/encryption-v1_test.go +++ b/cmd/encryption-v1_test.go @@ -18,7 +18,6 @@ package cmd import ( "bytes" - "encoding/base64" "net/http" "testing" @@ -107,228 +106,6 @@ func TesthasSSECustomerHeader(t *testing.T) { } } -var parseSSECustomerRequestTests = []struct { - headers map[string]string - useTLS bool - err error -}{ - { - headers: map[string]string{ - crypto.SSECAlgorithm: "AES256", - crypto.SSECKey: "XAm0dRrJsEsyPb1UuFNezv1bl9hxuYsgUVC/MUctE2k=", // 0 - crypto.SSECKeyMD5: "bY4wkxQejw9mUJfo72k53A==", - }, - useTLS: true, err: nil, - }, - { - headers: map[string]string{ - crypto.SSECAlgorithm: "AES256", - crypto.SSECKey: "XAm0dRrJsEsyPb1UuFNezv1bl9hxuYsgUVC/MUctE2k=", // 1 - crypto.SSECKeyMD5: "bY4wkxQejw9mUJfo72k53A==", - }, - useTLS: false, err: errInsecureSSERequest, - }, - { - headers: map[string]string{ - crypto.SSECAlgorithm: "AES 256", - crypto.SSECKey: "XAm0dRrJsEsyPb1UuFNezv1bl9hxuYsgUVC/MUctE2k=", // 2 - crypto.SSECKeyMD5: "bY4wkxQejw9mUJfo72k53A==", - }, - useTLS: true, err: crypto.ErrInvalidCustomerAlgorithm, - }, - { - headers: map[string]string{ - crypto.SSECAlgorithm: "AES256", - crypto.SSECKey: "NjE0SL87s+ZhYtaTrg5eI5cjhCQLGPVMKenPG2bCJFw=", // 3 - crypto.SSECKeyMD5: "H+jq/LwEOEO90YtiTuNFVw==", - }, - useTLS: true, err: crypto.ErrCustomerKeyMD5Mismatch, - }, - { - headers: map[string]string{ - crypto.SSECAlgorithm: "AES256", - crypto.SSECKey: " jE0SL87s+ZhYtaTrg5eI5cjhCQLGPVMKenPG2bCJFw=", // 4 - crypto.SSECKeyMD5: "H+jq/LwEOEO90YtiTuNFVw==", - }, - useTLS: true, err: crypto.ErrInvalidCustomerKey, - }, - { - headers: map[string]string{ - crypto.SSECAlgorithm: "AES256", - crypto.SSECKey: "NjE0SL87s+ZhYtaTrg5eI5cjhCQLGPVMKenPG2bCJFw=", // 5 - crypto.SSECKeyMD5: " +jq/LwEOEO90YtiTuNFVw==", - }, - useTLS: true, err: crypto.ErrCustomerKeyMD5Mismatch, - }, - { - headers: map[string]string{ - crypto.SSECAlgorithm: "AES256", - crypto.SSECKey: "vFQ9ScFOF6Tu/BfzMS+rVMvlZGJHi5HmGJenJfrfKI45", // 6 - crypto.SSECKeyMD5: "9KPgDdZNTHimuYCwnJTp5g==", - }, - useTLS: true, err: crypto.ErrInvalidCustomerKey, - }, - { - headers: map[string]string{ - crypto.SSECAlgorithm: "AES256", - crypto.SSECKey: "", // 7 - crypto.SSECKeyMD5: "9KPgDdZNTHimuYCwnJTp5g==", - }, - useTLS: true, err: crypto.ErrMissingCustomerKey, - }, - { - headers: map[string]string{ - crypto.SSECAlgorithm: "AES256", - crypto.SSECKey: "vFQ9ScFOF6Tu/BfzMS+rVMvlZGJHi5HmGJenJfrfKI45", // 8 - crypto.SSECKeyMD5: "", - }, - useTLS: true, err: crypto.ErrMissingCustomerKeyMD5, - }, - { - headers: map[string]string{ - crypto.SSECAlgorithm: "AES256", - crypto.SSECKey: "vFQ9ScFOF6Tu/BfzMS+rVMvlZGJHi5HmGJenJfrfKI45", // 8 - crypto.SSECKeyMD5: "", - crypto.SSEHeader: "", - }, - useTLS: true, err: crypto.ErrIncompatibleEncryptionMethod, - }, -} - -func TestParseSSECustomerRequest(t *testing.T) { - defer func(flag bool) { globalIsSSL = flag }(globalIsSSL) - for i, test := range parseSSECustomerRequestTests { - headers := http.Header{} - for k, v := range test.headers { - headers.Set(k, v) - } - request := &http.Request{} - request.Header = headers - globalIsSSL = test.useTLS - - _, err := ParseSSECustomerRequest(request) - if err != test.err { - t.Errorf("Test %d: Parse returned: %v want: %v", i, err, test.err) - } - } -} - -var parseSSECopyCustomerRequestTests = []struct { - headers map[string]string - metadata map[string]string - useTLS bool - err error -}{ - { - headers: map[string]string{ - crypto.SSECopyAlgorithm: "AES256", - crypto.SSECopyKey: "XAm0dRrJsEsyPb1UuFNezv1bl9hxuYsgUVC/MUctE2k=", // 0 - crypto.SSECopyKeyMD5: "bY4wkxQejw9mUJfo72k53A==", - }, - metadata: map[string]string{}, - useTLS: true, err: nil, - }, - { - headers: map[string]string{ - crypto.SSECopyAlgorithm: "AES256", - crypto.SSECopyKey: "XAm0dRrJsEsyPb1UuFNezv1bl9hxuYsgUVC/MUctE2k=", // 0 - crypto.SSECopyKeyMD5: "bY4wkxQejw9mUJfo72k53A==", - }, - metadata: map[string]string{"X-Minio-Internal-Server-Side-Encryption-S3-Sealed-Key": base64.StdEncoding.EncodeToString(make([]byte, 64))}, - useTLS: true, err: crypto.ErrIncompatibleEncryptionMethod, - }, - { - headers: map[string]string{ - crypto.SSECopyAlgorithm: "AES256", - crypto.SSECopyKey: "XAm0dRrJsEsyPb1UuFNezv1bl9hxuYsgUVC/MUctE2k=", // 1 - crypto.SSECopyKeyMD5: "bY4wkxQejw9mUJfo72k53A==", - }, - metadata: map[string]string{}, - useTLS: false, err: errInsecureSSERequest, - }, - { - headers: map[string]string{ - crypto.SSECopyAlgorithm: "AES 256", - crypto.SSECopyKey: "XAm0dRrJsEsyPb1UuFNezv1bl9hxuYsgUVC/MUctE2k=", // 2 - crypto.SSECopyKeyMD5: "bY4wkxQejw9mUJfo72k53A==", - }, - metadata: map[string]string{}, - useTLS: true, err: crypto.ErrInvalidCustomerAlgorithm, - }, - { - headers: map[string]string{ - crypto.SSECopyAlgorithm: "AES256", - crypto.SSECopyKey: "NjE0SL87s+ZhYtaTrg5eI5cjhCQLGPVMKenPG2bCJFw=", // 3 - crypto.SSECopyKeyMD5: "H+jq/LwEOEO90YtiTuNFVw==", - }, - metadata: map[string]string{}, - useTLS: true, err: crypto.ErrCustomerKeyMD5Mismatch, - }, - { - headers: map[string]string{ - crypto.SSECopyAlgorithm: "AES256", - crypto.SSECopyKey: " jE0SL87s+ZhYtaTrg5eI5cjhCQLGPVMKenPG2bCJFw=", // 4 - crypto.SSECopyKeyMD5: "H+jq/LwEOEO90YtiTuNFVw==", - }, - metadata: map[string]string{}, - useTLS: true, err: crypto.ErrInvalidCustomerKey, - }, - { - headers: map[string]string{ - crypto.SSECopyAlgorithm: "AES256", - crypto.SSECopyKey: "NjE0SL87s+ZhYtaTrg5eI5cjhCQLGPVMKenPG2bCJFw=", // 5 - crypto.SSECopyKeyMD5: " +jq/LwEOEO90YtiTuNFVw==", - }, - metadata: map[string]string{}, - useTLS: true, err: crypto.ErrCustomerKeyMD5Mismatch, - }, - { - headers: map[string]string{ - crypto.SSECopyAlgorithm: "AES256", - crypto.SSECopyKey: "vFQ9ScFOF6Tu/BfzMS+rVMvlZGJHi5HmGJenJfrfKI45", // 6 - crypto.SSECopyKeyMD5: "9KPgDdZNTHimuYCwnJTp5g==", - }, - metadata: map[string]string{}, - useTLS: true, err: crypto.ErrInvalidCustomerKey, - }, - { - headers: map[string]string{ - crypto.SSECopyAlgorithm: "AES256", - crypto.SSECopyKey: "", // 7 - crypto.SSECopyKeyMD5: "9KPgDdZNTHimuYCwnJTp5g==", - }, - metadata: map[string]string{}, - useTLS: true, err: crypto.ErrMissingCustomerKey, - }, - { - headers: map[string]string{ - crypto.SSECopyAlgorithm: "AES256", - crypto.SSECopyKey: "vFQ9ScFOF6Tu/BfzMS+rVMvlZGJHi5HmGJenJfrfKI45", // 8 - crypto.SSECopyKeyMD5: "", - }, - metadata: map[string]string{}, - useTLS: true, err: crypto.ErrMissingCustomerKeyMD5, - }, -} - -func TestParseSSECopyCustomerRequest(t *testing.T) { - defer func(flag bool) { globalIsSSL = flag }(globalIsSSL) - for i, test := range parseSSECopyCustomerRequestTests { - headers := http.Header{} - for k, v := range test.headers { - headers.Set(k, v) - } - request := &http.Request{} - request.Header = headers - globalIsSSL = test.useTLS - - _, err := ParseSSECopyCustomerRequest(request.Header, test.metadata) - if err != test.err { - t.Errorf("Test %d: Parse returned: %v want: %v", i, err, test.err) - } - } -} - var encryptRequestTests = []struct { header map[string]string metadata map[string]string diff --git a/cmd/generic-handlers.go b/cmd/generic-handlers.go index 1498e33a7..2b781c74c 100644 --- a/cmd/generic-handlers.go +++ b/cmd/generic-handlers.go @@ -28,6 +28,7 @@ import ( "github.com/minio/minio-go/pkg/set" humanize "github.com/dustin/go-humanize" + "github.com/minio/minio/cmd/crypto" "github.com/minio/minio/cmd/logger" "github.com/minio/minio/pkg/dns" "github.com/minio/minio/pkg/handlers" @@ -773,3 +774,17 @@ func (h criticalErrorHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) }() h.handler.ServeHTTP(w, r) } + +func setSSETLSHandler(h http.Handler) http.Handler { return sseTLSHandler{h} } + +// sseTLSHandler enforces certain rules for SSE requests which are made / must be made over TLS. +type sseTLSHandler struct{ handler http.Handler } + +func (h sseTLSHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) { + // Deny SSE-C requests if not made over TLS + if !globalIsSSL && (crypto.SSEC.IsRequested(r.Header) || crypto.SSECopy.IsRequested(r.Header)) { + writeErrorResponseHeadersOnly(w, ErrInsecureSSECustomerRequest) + return + } + h.handler.ServeHTTP(w, r) +} diff --git a/cmd/generic-handlers_test.go b/cmd/generic-handlers_test.go index 18be8fe69..b014aa7a6 100644 --- a/cmd/generic-handlers_test.go +++ b/cmd/generic-handlers_test.go @@ -18,6 +18,7 @@ package cmd import ( "net/http" + "net/http/httptest" "strconv" "testing" @@ -181,3 +182,39 @@ func TestContainsReservedMetadata(t *testing.T) { } } } + +var sseTLSHandlerTests = []struct { + Header http.Header + IsTLS, ShouldFail bool +}{ + {Header: http.Header{}, IsTLS: false, ShouldFail: false}, // 0 + {Header: http.Header{crypto.SSECAlgorithm: []string{"AES256"}}, IsTLS: false, ShouldFail: true}, // 1 + {Header: http.Header{crypto.SSECAlgorithm: []string{"AES256"}}, IsTLS: true, ShouldFail: false}, // 2 + {Header: http.Header{crypto.SSECKey: []string{""}}, IsTLS: true, ShouldFail: false}, // 3 + {Header: http.Header{crypto.SSECopyAlgorithm: []string{""}}, IsTLS: false, ShouldFail: true}, // 4 +} + +func TestSSETLSHandler(t *testing.T) { + defer func(isSSL bool) { globalIsSSL = isSSL }(globalIsSSL) // reset globalIsSSL after test + + var okHandler http.HandlerFunc = func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusOK) + } + for i, test := range sseTLSHandlerTests { + globalIsSSL = test.IsTLS + + w := httptest.NewRecorder() + r := new(http.Request) + r.Header = test.Header + + h := setSSETLSHandler(okHandler) + h.ServeHTTP(w, r) + + switch { + case test.ShouldFail && w.Code == http.StatusOK: + t.Errorf("Test %d: should fail but status code is HTTP %d", i, w.Code) + case !test.ShouldFail && w.Code != http.StatusOK: + t.Errorf("Test %d: should not fail but status code is HTTP %d and not 200 OK", i, w.Code) + } + } +} diff --git a/cmd/routers.go b/cmd/routers.go index 8a21dba06..5b59e5ee9 100644 --- a/cmd/routers.go +++ b/cmd/routers.go @@ -82,6 +82,8 @@ var globalHandlers = []HandlerFunc{ // routes them accordingly. Client receives a HTTP error for // invalid/unsupported signatures. setAuthHandler, + // Enforce rules specific for TLS requests + setSSETLSHandler, // filters HTTP headers which are treated as metadata and are reserved // for internal use only. filterReservedMetadata,