fix: trim arn:aws:kms from incoming SSE aws-kms-key-id (#15540)

cmd/api-errors.go

@@ -196,8 +196,9 @@ const (
 	ErrInvalidTagDirective
 	// Add new error codes here.
 
-	// SSE-S3 related API errors
+	// SSE-S3/SSE-KMS related API errors
 	ErrInvalidEncryptionMethod
+	ErrInvalidEncryptionKeyID
 
 	// Server-Side-Encryption (with Customer provided key) related API errors.
 	ErrInsecureSSECustomerRequest
@@ -1072,6 +1073,11 @@ var errorCodes = errorCodeMap{
 		Description:    "The encryption method specified is not supported",
 		HTTPStatusCode: http.StatusBadRequest,
 	},
+	ErrInvalidEncryptionKeyID: {
+		Code:           "InvalidRequest",
+		Description:    "The specified KMS KeyID contains unsupported characters",
+		HTTPStatusCode: http.StatusBadRequest,
+	},
 	ErrInsecureSSECustomerRequest: {
 		Code:           "InvalidRequest",
 		Description:    "Requests specifying Server Side Encryption with Customer provided keys must be made over a secure connection.",
@@ -1921,6 +1927,8 @@ func toAPIErrorCode(ctx context.Context, err error) (apiErr APIErrorCode) {
 		apiErr = ErrInvalidEncryptionParameters
 	case crypto.ErrInvalidEncryptionMethod:
 		apiErr = ErrInvalidEncryptionMethod
+	case crypto.ErrInvalidEncryptionKeyID:
+		apiErr = ErrInvalidEncryptionKeyID
 	case crypto.ErrInvalidCustomerAlgorithm:
 		apiErr = ErrInvalidSSECustomerAlgorithm
 	case crypto.ErrMissingCustomerKey:

cmd/encryption-v1.go

@@ -82,7 +82,7 @@ func (o *MultipartInfo) KMSKeyID() string { return kmsKeyIDFromMetadata(o.UserDe
 // metadata, if any. It returns an empty ID if no key ID is
 // present.
 func kmsKeyIDFromMetadata(metadata map[string]string) string {
-	const ARNPrefix = "arn:aws:kms:"
+	const ARNPrefix = crypto.ARNPrefix
 	if len(metadata) == 0 {
 		return ""
 	}

internal/bucket/encryption/bucket-sse-config.go

@@ -22,6 +22,7 @@ import (
 	"errors"
 	"io"
 	"net/http"
+	"strings"
 
 	"github.com/minio/minio/internal/crypto"
 	xhttp "github.com/minio/minio/internal/http"
@@ -102,9 +103,14 @@ func ParseBucketSSEConfig(r io.Reader) (*BucketSSEConfig, error) {
 				return nil, errors.New("MasterKeyID is allowed with aws:kms only")
 			}
 		case AWSKms:
-			if rule.DefaultEncryptionAction.MasterKeyID == "" {
+			keyID := rule.DefaultEncryptionAction.MasterKeyID
+			if keyID == "" {
 				return nil, errors.New("MasterKeyID is missing with aws:kms")
 			}
+			spaces := strings.HasPrefix(keyID, " ") || strings.HasSuffix(keyID, " ")
+			if spaces {
+				return nil, errors.New("MasterKeyID contains unsupported characters")
+			}
 		}
 	}
 
@@ -164,7 +170,7 @@ func (b *BucketSSEConfig) Algo() Algorithm {
 // empty key ID.
 func (b *BucketSSEConfig) KeyID() string {
 	for _, rule := range b.Rules {
-		return rule.DefaultEncryptionAction.MasterKeyID
+		return strings.TrimPrefix(rule.DefaultEncryptionAction.MasterKeyID, crypto.ARNPrefix)
 	}
 	return ""
 }

internal/bucket/encryption/bucket-sse-config_test.go

@@ -62,7 +62,7 @@ func TestParseBucketSSEConfig(t *testing.T) {
 			{
 				DefaultEncryptionAction: EncryptionAction{
 					Algorithm:   AWSKms,
-					MasterKeyID: "arn:aws:kms:us-east-1:1234/5678example",
+					MasterKeyID: "arn:aws:kms:my-minio-key",
 				},
 			},
 		},
@@ -70,6 +70,7 @@ func TestParseBucketSSEConfig(t *testing.T) {
 
 	testCases := []struct {
 		inputXML       string
+		keyID          string
 		expectedErr    error
 		shouldPass     bool
 		expectedConfig *BucketSSEConfig
@@ -83,10 +84,11 @@ func TestParseBucketSSEConfig(t *testing.T) {
 		},
 		// 2. Valid XML SSE-KMS
 		{
-			inputXML:       `<ServerSideEncryptionConfiguration xmlns="http://s3.amazonaws.com/doc/2006-03-01/"><Rule><ApplyServerSideEncryptionByDefault><SSEAlgorithm>aws:kms</SSEAlgorithm><KMSMasterKeyID>arn:aws:kms:us-east-1:1234/5678example</KMSMasterKeyID></ApplyServerSideEncryptionByDefault></Rule></ServerSideEncryptionConfiguration>`,
+			inputXML:       `<ServerSideEncryptionConfiguration xmlns="http://s3.amazonaws.com/doc/2006-03-01/"><Rule><ApplyServerSideEncryptionByDefault><SSEAlgorithm>aws:kms</SSEAlgorithm><KMSMasterKeyID>arn:aws:kms:my-minio-key</KMSMasterKeyID></ApplyServerSideEncryptionByDefault></Rule></ServerSideEncryptionConfiguration>`,
 			expectedErr:    nil,
 			shouldPass:     true,
 			expectedConfig: actualKMSConfig,
+			keyID:          "my-minio-key",
 		},
 		// 3. Invalid - more than one rule
 		{
@@ -119,23 +121,33 @@ func TestParseBucketSSEConfig(t *testing.T) {
 			shouldPass:     true,
 			expectedConfig: actualAES256NoNSConfig,
 		},
+		// 8. Space characters in MasterKeyID
+		{
+			inputXML:    `<ServerSideEncryptionConfiguration xmlns="http://s3.amazonaws.com/doc/2006-03-01/"><Rule><ApplyServerSideEncryptionByDefault><SSEAlgorithm>aws:kms</SSEAlgorithm><KMSMasterKeyID> arn:aws:kms:my-minio-key </KMSMasterKeyID></ApplyServerSideEncryptionByDefault></Rule></ServerSideEncryptionConfiguration>`,
+			expectedErr: errors.New("MasterKeyID contains unsupported characters"),
+			shouldPass:  false,
+		},
 	}
 
 	for i, tc := range testCases {
-		_, err := ParseBucketSSEConfig(bytes.NewReader([]byte(tc.inputXML)))
+		ssec, err := ParseBucketSSEConfig(bytes.NewReader([]byte(tc.inputXML)))
 		if tc.shouldPass && err != nil {
-			t.Fatalf("Test case %d: Expected to succeed but got %s", i+1, err)
+			t.Errorf("Test case %d: Expected to succeed but got %s", i+1, err)
 		}
 
 		if !tc.shouldPass {
 			if err == nil || err != nil && err.Error() != tc.expectedErr.Error() {
-				t.Fatalf("Test case %d: Expected %s but got %s", i+1, tc.expectedErr, err)
+				t.Errorf("Test case %d: Expected %s but got %s", i+1, tc.expectedErr, err)
 			}
 			continue
 		}
 
+		if tc.keyID != "" && tc.keyID != ssec.KeyID() {
+			t.Errorf("Test case %d: Expected bucket encryption KeyID %s but got %s", i+1, tc.keyID, ssec.KeyID())
+		}
+
 		if expectedXML, err := xml.Marshal(tc.expectedConfig); err != nil || !bytes.Equal(expectedXML, []byte(tc.inputXML)) {
-			t.Fatalf("Test case %d: Expected bucket encryption XML %s but got %s", i+1, string(expectedXML), tc.inputXML)
+			t.Errorf("Test case %d: Expected bucket encryption XML %s but got %s", i+1, string(expectedXML), tc.inputXML)
 		}
 	}
 }

internal/crypto/error.go

@@ -76,6 +76,9 @@ var (
 	// ErrIncompatibleEncryptionMethod indicates that both SSE-C headers and SSE-S3 headers were specified, and are incompatible
 	// The client needs to remove the SSE-S3 header or the SSE-C headers
 	ErrIncompatibleEncryptionMethod = Errorf("Server side encryption specified with both SSE-C and SSE-S3 headers")
+
+	// ErrInvalidEncryptionKeyID returns error when KMS key id contains invalid characters
+	ErrInvalidEncryptionKeyID = Errorf("KMS KeyID contains unsupported characters")
 )
 
 var (

internal/crypto/metadata.go

@@ -56,6 +56,9 @@ const (
 	// be part of the object. Therefore, the bucket/object name must be added
 	// to the context, if not present, whenever a decryption is performed.
 	MetaContext = "X-Minio-Internal-Server-Side-Encryption-Context"
+
+	// ARNPrefix prefix for "arn:aws:kms"
+	ARNPrefix = "arn:aws:kms:"
 )
 
 // IsMultiPart returns true if the object metadata indicates

internal/crypto/sse-kms.go

@@ -55,7 +55,8 @@ func (ssekms) IsRequested(h http.Header) bool {
 		return true
 	}
 	if _, ok := h[xhttp.AmzServerSideEncryption]; ok {
-		return strings.ToUpper(h.Get(xhttp.AmzServerSideEncryption)) != xhttp.AmzEncryptionAES // Return only true if the SSE header is specified and does not contain the SSE-S3 value
+		// Return only true if the SSE header is specified and does not contain the SSE-S3 value
+		return strings.ToUpper(h.Get(xhttp.AmzServerSideEncryption)) != xhttp.AmzEncryptionAES
 	}
 	return false
 }
@@ -63,6 +64,10 @@ func (ssekms) IsRequested(h http.Header) bool {
 // ParseHTTP parses the SSE-KMS headers and returns the SSE-KMS key ID
 // and the KMS context on success.
 func (ssekms) ParseHTTP(h http.Header) (string, kms.Context, error) {
+	if h == nil {
+		return "", nil, ErrInvalidEncryptionMethod
+	}
+
 	algorithm := h.Get(xhttp.AmzServerSideEncryption)
 	if algorithm != xhttp.AmzEncryptionKMS {
 		return "", nil, ErrInvalidEncryptionMethod
@@ -80,7 +85,13 @@ func (ssekms) ParseHTTP(h http.Header) (string, kms.Context, error) {
 			return "", nil, err
 		}
 	}
-	return h.Get(xhttp.AmzServerSideEncryptionKmsID), ctx, nil
+
+	keyID := h.Get(xhttp.AmzServerSideEncryptionKmsID)
+	spaces := strings.HasPrefix(keyID, " ") || strings.HasSuffix(keyID, " ")
+	if spaces {
+		return "", nil, ErrInvalidEncryptionKeyID
+	}
+	return strings.TrimPrefix(keyID, ARNPrefix), ctx, nil
 }
 
 // IsEncrypted returns true if the object metadata indicates