Omit empty algorithm tags in bucket encryption XML (minio#8987)

- Bucket encryption config returned by MinIO would always have the xml namespace set - Make unit tests in pkg/bucket/encryption more robust
jeroiraz · Feb 14, 2020 · 9f298d2 · 9f298d2
1 parent 716a52f
commit 9f298d2
Show file tree

Hide file tree

Showing 2 changed files with 77 additions and 88 deletions.
diff --git a/pkg/bucket/encryption/bucket-sse-config.go b/pkg/bucket/encryption/bucket-sse-config.go
@@ -58,17 +58,20 @@ func (alg *SSEAlgorithm) MarshalXML(e *xml.Encoder, start xml.StartElement) erro
 
 // EncryptionAction - for ApplyServerSideEncryptionByDefault XML tag
 type EncryptionAction struct {
-	Algorithm   SSEAlgorithm `xml:"SSEAlgorithm"`
-	MasterKeyID string       `xml:"KMSMasterKeyID"`
+	Algorithm   SSEAlgorithm `xml:"SSEAlgorithm,omitempty"`
+	MasterKeyID string       `xml:"KMSMasterKeyID,omitempty"`
 }
 
 // SSERule - for ServerSideEncryptionConfiguration XML tag
 type SSERule struct {
 	DefaultEncryptionAction EncryptionAction `xml:"ApplyServerSideEncryptionByDefault"`
 }
 
+const xmlNS = "http://s3.amazonaws.com/doc/2006-03-01/"
+
 // BucketSSEConfig - represents default bucket encryption configuration
 type BucketSSEConfig struct {
+	XMLNS   string    `xml:"xmlns,attr,omitempty"`
 	XMLName xml.Name  `xml:"ServerSideEncryptionConfiguration"`
 	Rules   []SSERule `xml:"Rule"`
 }
@@ -99,5 +102,10 @@ func ParseBucketSSEConfig(r io.Reader) (*BucketSSEConfig, error) {
 			}
 		}
 	}
+
+	if config.XMLNS == "" {
+		config.XMLNS = xmlNS
+	}
+
 	return &config, nil
 }
diff --git a/pkg/bucket/encryption/bucket-sse-config_test.go b/pkg/bucket/encryption/bucket-sse-config_test.go
@@ -25,114 +25,98 @@ import (
 
 // TestParseBucketSSEConfig performs basic sanity tests on ParseBucketSSEConfig
 func TestParseBucketSSEConfig(t *testing.T) {
+	actualAES256NoNSConfig := &BucketSSEConfig{
+		XMLName: xml.Name{
+			Local: "ServerSideEncryptionConfiguration",
+		},
+		Rules: []SSERule{
+			{
+				DefaultEncryptionAction: EncryptionAction{
+					Algorithm: AES256,
+				},
+			},
+		},
+	}
+
+	actualAES256Config := &BucketSSEConfig{
+		XMLNS: xmlNS,
+		XMLName: xml.Name{
+			Local: "ServerSideEncryptionConfiguration",
+		},
+		Rules: []SSERule{
+			{
+				DefaultEncryptionAction: EncryptionAction{
+					Algorithm: AES256,
+				},
+			},
+		},
+	}
+
+	actualKMSConfig := &BucketSSEConfig{
+		XMLNS: xmlNS,
+		XMLName: xml.Name{
+			Local: "ServerSideEncryptionConfiguration",
+		},
+		Rules: []SSERule{
+			{
+				DefaultEncryptionAction: EncryptionAction{
+					Algorithm:   AWSKms,
+					MasterKeyID: "arn:aws:kms:us-east-1:1234/5678example",
+				},
+			},
+		},
+	}
+
 	testCases := []struct {
-		inputXML    string
-		expectedErr error
-		shouldPass  bool
+		inputXML       string
+		expectedErr    error
+		shouldPass     bool
+		expectedConfig *BucketSSEConfig
 	}{
 		// 1. Valid XML SSE-S3
 		{
-			inputXML: `<ServerSideEncryptionConfiguration xmlns="http://s3.amazonaws.com/doc/2006-03-01/">
-			<Rule>
-			<ApplyServerSideEncryptionByDefault>
-			<SSEAlgorithm>AES256</SSEAlgorithm>
-			</ApplyServerSideEncryptionByDefault>
-			</Rule>
-			</ServerSideEncryptionConfiguration>`,
-			expectedErr: nil,
-			shouldPass:  true,
+			inputXML:       `<ServerSideEncryptionConfiguration xmlns="http://s3.amazonaws.com/doc/2006-03-01/"><Rule><ApplyServerSideEncryptionByDefault><SSEAlgorithm>AES256</SSEAlgorithm></ApplyServerSideEncryptionByDefault></Rule></ServerSideEncryptionConfiguration>`,
+			expectedErr:    nil,
+			shouldPass:     true,
+			expectedConfig: actualAES256Config,
 		},
 		// 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>`,
-			expectedErr: nil,
-			shouldPass:  true,
+			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>`,
+			expectedErr:    nil,
+			shouldPass:     true,
+			expectedConfig: actualKMSConfig,
 		},
 		// 3. Invalid - more than one rule
 		{
-			inputXML: `<ServerSideEncryptionConfiguration xmlns="http://s3.amazonaws.com/doc/2006-03-01/">
-			<Rule>
-			<ApplyServerSideEncryptionByDefault>
-			<SSEAlgorithm>AES256</SSEAlgorithm>
-			</ApplyServerSideEncryptionByDefault>
-			</Rule>
-			<Rule>
-			<ApplyServerSideEncryptionByDefault>
-			<SSEAlgorithm>AES256</SSEAlgorithm>
-			</ApplyServerSideEncryptionByDefault>
-			</Rule>
-			</ServerSideEncryptionConfiguration>`,
+			inputXML:    `<ServerSideEncryptionConfiguration xmlns="http://s3.amazonaws.com/doc/2006-03-01/"><Rule><ApplyServerSideEncryptionByDefault><SSEAlgorithm>AES256</SSEAlgorithm></ApplyServerSideEncryptionByDefault></Rule><Rule><ApplyServerSideEncryptionByDefault><SSEAlgorithm>AES256</SSEAlgorithm></ApplyServerSideEncryptionByDefault></Rule></ServerSideEncryptionConfiguration>`,
 			expectedErr: errors.New("Only one server-side encryption rule is allowed"),
 			shouldPass:  false,
 		},
-		// 4. Invalid XML - master key ID present in AES256
+		// 4. Invalid XML - master key ID present along with AES256
 		{
-			inputXML: `<ServerSideEncryptionConfiguration xmlns="http://s3.amazonaws.com/doc/2006-03-01/">
-			<Rule>
-			<ApplyServerSideEncryptionByDefault>
-			<SSEAlgorithm>AES256</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>AES256</SSEAlgorithm><KMSMasterKeyID>arn:aws:kms:us-east-1:1234/5678example</KMSMasterKeyID></ApplyServerSideEncryptionByDefault></Rule></ServerSideEncryptionConfiguration>`,
 			expectedErr: errors.New("MasterKeyID is allowed with aws:kms only"),
 			shouldPass:  false,
 		},
-		// 5. Invalid XML - master key ID not found in aws:kms algorithm
+		// 5. Invalid XML - master key ID not provided when algorithm is set to aws:kms algorithm
 		{
-			inputXML: `<ServerSideEncryptionConfiguration xmlns="http://s3.amazonaws.com/doc/2006-03-01/">
-			<Rule>
-			<ApplyServerSideEncryptionByDefault>
-			<SSEAlgorithm>aws:kms</SSEAlgorithm>
-			</ApplyServerSideEncryptionByDefault>
-			</Rule>
-			</ServerSideEncryptionConfiguration>`,
+			inputXML:    `<ServerSideEncryptionConfiguration xmlns="http://s3.amazonaws.com/doc/2006-03-01/"><Rule><ApplyServerSideEncryptionByDefault><SSEAlgorithm>aws:kms</SSEAlgorithm></ApplyServerSideEncryptionByDefault></Rule></ServerSideEncryptionConfiguration>`,
 			expectedErr: errors.New("MasterKeyID is missing"),
 			shouldPass:  false,
 		},
 		// 6. Invalid Algorithm
 		{
-			inputXML: `<ServerSideEncryptionConfiguration xmlns="http://s3.amazonaws.com/doc/2006-03-01/">
-			<Rule>
-			<ApplyServerSideEncryptionByDefault>
-			<SSEAlgorithm>InvalidAlgorithm</SSEAlgorithm>
-			</ApplyServerSideEncryptionByDefault>
-			</Rule>
-			</ServerSideEncryptionConfiguration>`,
+			inputXML:    `<ServerSideEncryptionConfiguration xmlns="http://s3.amazonaws.com/doc/2006-03-01/"><Rule><ApplyServerSideEncryptionByDefault><SSEAlgorithm>InvalidAlgorithm</SSEAlgorithm></ApplyServerSideEncryptionByDefault></Rule></ServerSideEncryptionConfiguration>`,
 			expectedErr: errors.New("Unknown SSE algorithm"),
 			shouldPass:  false,
 		},
-		// 7. Allow missing namespace
+		// 7. Valid XML without the namespace set
 		{
-			inputXML: `<ServerSideEncryptionConfiguration>
-			<Rule>
-			<ApplyServerSideEncryptionByDefault>
-			<SSEAlgorithm>AES256</SSEAlgorithm>
-			</ApplyServerSideEncryptionByDefault>
-			</Rule>
-			</ServerSideEncryptionConfiguration>`,
-			expectedErr: nil,
-			shouldPass:  true,
-		},
-	}
-
-	actualConfig := &BucketSSEConfig{
-		XMLName: xml.Name{
-			Local: "ServerSideEncryptionConfiguration",
-		},
-		Rules: []SSERule{
-			{
-				DefaultEncryptionAction: EncryptionAction{
-					Algorithm: AES256,
-				},
-			},
+			inputXML:       `<ServerSideEncryptionConfiguration><Rule><ApplyServerSideEncryptionByDefault><SSEAlgorithm>AES256</SSEAlgorithm></ApplyServerSideEncryptionByDefault></Rule></ServerSideEncryptionConfiguration>`,
+			expectedErr:    nil,
+			shouldPass:     true,
+			expectedConfig: actualAES256NoNSConfig,
 		},
 	}
 
@@ -146,14 +130,11 @@ func TestParseBucketSSEConfig(t *testing.T) {
 			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)
 			}
-		}
-
-		if !tc.shouldPass {
 			continue
 		}
 
-		if actualXML, err := xml.Marshal(actualConfig); err != nil && bytes.Equal(actualXML, []byte(tc.inputXML)) {
-			t.Fatalf("Test case %d: Expected config %s but got %s", i+1, string(actualXML), tc.inputXML)
+		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)
 		}
 	}
 }