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

@@ -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:

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},

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
 	}

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

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)
+}

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)
+		}
+	}
+}

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,