From 9f298d231192bedad99af29f58e6d4ea7bb46210 Mon Sep 17 00:00:00 2001 From: Krishnan Parthasarathi Date: Thu, 13 Feb 2020 22:12:42 -0800 Subject: [PATCH] Omit empty algorithm tags in bucket encryption XML (#8987) - Bucket encryption config returned by MinIO would always have the xml namespace set - Make unit tests in pkg/bucket/encryption more robust --- pkg/bucket/encryption/bucket-sse-config.go | 12 +- .../encryption/bucket-sse-config_test.go | 189 ++++++++---------- 2 files changed, 95 insertions(+), 106 deletions(-) diff --git a/pkg/bucket/encryption/bucket-sse-config.go b/pkg/bucket/encryption/bucket-sse-config.go index 646eb4f25..8b1ae1e4b 100644 --- a/pkg/bucket/encryption/bucket-sse-config.go +++ b/pkg/bucket/encryption/bucket-sse-config.go @@ -58,8 +58,8 @@ 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 @@ -67,8 +67,11 @@ 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 index 35a8c4fba..78c5ac9c9 100644 --- a/pkg/bucket/encryption/bucket-sse-config_test.go +++ b/pkg/bucket/encryption/bucket-sse-config_test.go @@ -25,105 +25,7 @@ import ( // TestParseBucketSSEConfig performs basic sanity tests on ParseBucketSSEConfig func TestParseBucketSSEConfig(t *testing.T) { - testCases := []struct { - inputXML string - expectedErr error - shouldPass bool - }{ - // 1. Valid XML SSE-S3 - { - inputXML: ` - - - AES256 - - - `, - expectedErr: nil, - shouldPass: true, - }, - // 2. Valid XML SSE-KMS - { - inputXML: ` - - - aws:kms - arn:aws:kms:us-east-1:1234/5678example - - - `, - expectedErr: nil, - shouldPass: true, - }, - // 3. Invalid - more than one rule - { - inputXML: ` - - - AES256 - - - - - AES256 - - - `, - expectedErr: errors.New("Only one server-side encryption rule is allowed"), - shouldPass: false, - }, - // 4. Invalid XML - master key ID present in AES256 - { - inputXML: ` - - - AES256 - arn:aws:kms:us-east-1:1234/5678example - - - `, - expectedErr: errors.New("MasterKeyID is allowed with aws:kms only"), - shouldPass: false, - }, - // 5. Invalid XML - master key ID not found in aws:kms algorithm - { - inputXML: ` - - - aws:kms - - - `, - expectedErr: errors.New("MasterKeyID is missing"), - shouldPass: false, - }, - // 6. Invalid Algorithm - { - inputXML: ` - - - InvalidAlgorithm - - - `, - expectedErr: errors.New("Unknown SSE algorithm"), - shouldPass: false, - }, - // 7. Allow missing namespace - { - inputXML: ` - - - AES256 - - - `, - expectedErr: nil, - shouldPass: true, - }, - } - - actualConfig := &BucketSSEConfig{ + actualAES256NoNSConfig := &BucketSSEConfig{ XMLName: xml.Name{ Local: "ServerSideEncryptionConfiguration", }, @@ -136,6 +38,88 @@ func TestParseBucketSSEConfig(t *testing.T) { }, } + 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 + expectedConfig *BucketSSEConfig + }{ + // 1. Valid XML SSE-S3 + { + inputXML: `AES256`, + expectedErr: nil, + shouldPass: true, + expectedConfig: actualAES256Config, + }, + // 2. Valid XML SSE-KMS + { + inputXML: `aws:kmsarn:aws:kms:us-east-1:1234/5678example`, + expectedErr: nil, + shouldPass: true, + expectedConfig: actualKMSConfig, + }, + // 3. Invalid - more than one rule + { + inputXML: `AES256AES256`, + expectedErr: errors.New("Only one server-side encryption rule is allowed"), + shouldPass: false, + }, + // 4. Invalid XML - master key ID present along with AES256 + { + inputXML: `AES256arn:aws:kms:us-east-1:1234/5678example`, + expectedErr: errors.New("MasterKeyID is allowed with aws:kms only"), + shouldPass: false, + }, + // 5. Invalid XML - master key ID not provided when algorithm is set to aws:kms algorithm + { + inputXML: `aws:kms`, + expectedErr: errors.New("MasterKeyID is missing"), + shouldPass: false, + }, + // 6. Invalid Algorithm + { + inputXML: `InvalidAlgorithm`, + expectedErr: errors.New("Unknown SSE algorithm"), + shouldPass: false, + }, + // 7. Valid XML without the namespace set + { + inputXML: `AES256`, + expectedErr: nil, + shouldPass: true, + expectedConfig: actualAES256NoNSConfig, + }, + } + for i, tc := range testCases { _, err := ParseBucketSSEConfig(bytes.NewReader([]byte(tc.inputXML))) if tc.shouldPass && err != nil { @@ -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) } } }