lifcycle: Add more validation to the config (#11382)

pkg/bucket/lifecycle/and.go

@@ -20,22 +20,33 @@ import (
 	"encoding/xml"
 )
 
+var errDuplicateTagKey = Errorf("Duplicate Tag Keys are not allowed")
+
 // And - a tag to combine a prefix and multiple tags for lifecycle configuration rule.
 type And struct {
 	XMLName xml.Name `xml:"And"`
-	Prefix  string   `xml:"Prefix,omitempty"`
+	Prefix  Prefix   `xml:"Prefix,omitempty"`
 	Tags    []Tag    `xml:"Tag,omitempty"`
 }
 
-var errDuplicateTagKey = Errorf("Duplicate Tag Keys are not allowed")
-
 // isEmpty returns true if Tags field is null
 func (a And) isEmpty() bool {
-	return len(a.Tags) == 0 && a.Prefix == ""
+	return len(a.Tags) == 0 && !a.Prefix.set
 }
 
 // Validate - validates the And field
 func (a And) Validate() error {
+	emptyPrefix := !a.Prefix.set
+	emptyTags := len(a.Tags) == 0
+
+	if emptyPrefix && emptyTags {
+		return nil
+	}
+
+	if emptyPrefix && !emptyTags || !emptyPrefix && emptyTags {
+		return errXMLNotWellFormed
+	}
+
 	if a.ContainsDuplicateTag() {
 		return errDuplicateTagKey
 	}

pkg/bucket/lifecycle/filter.go

@@ -18,6 +18,7 @@ package lifecycle
 
 import (
 	"encoding/xml"
+	"io"
 )
 
 var (
@@ -27,10 +28,14 @@ var (
 // Filter - a filter for a lifecycle configuration Rule.
 type Filter struct {
 	XMLName xml.Name `xml:"Filter"`
-	Prefix  string
-	And     And
-	Tag     Tag
 
+	Prefix Prefix
+
+	And    And
+	andSet bool
+
+	Tag    Tag
+	tagSet bool
 	// Caching tags, only once
 	cachedTags []string
 }
@@ -61,11 +66,62 @@ func (f Filter) MarshalXML(e *xml.Encoder, start xml.StartElement) error {
 	return e.EncodeToken(xml.EndElement{Name: start.Name})
 }
 
+// UnmarshalXML - decodes XML data.
+func (f *Filter) UnmarshalXML(d *xml.Decoder, start xml.StartElement) (err error) {
+	for {
+		// Read tokens from the XML document in a stream.
+		t, err := d.Token()
+		if err != nil {
+			if err == io.EOF {
+				break
+			}
+			return err
+		}
+
+		switch se := t.(type) {
+		case xml.StartElement:
+			switch se.Name.Local {
+			case "Prefix":
+				var p Prefix
+				if err = d.DecodeElement(&p, &se); err != nil {
+					return err
+				}
+				f.Prefix = p
+			case "And":
+				var and And
+				if err = d.DecodeElement(&and, &se); err != nil {
+					return err
+				}
+				f.And = and
+				f.andSet = true
+			case "Tag":
+				var tag Tag
+				if err = d.DecodeElement(&tag, &se); err != nil {
+					return err
+				}
+				f.Tag = tag
+				f.tagSet = true
+			default:
+				return errUnknownXMLTag
+			}
+		}
+	}
+	return nil
+}
+
+// IsEmpty returns true if Filter is not specified in the XML
+func (f Filter) IsEmpty() bool {
+	return !f.Prefix.set && !f.andSet && !f.tagSet
+}
+
 // Validate - validates the filter element
 func (f Filter) Validate() error {
+	if !f.Prefix.set && !f.andSet && !f.tagSet {
+		return errXMLNotWellFormed
+	}
 	// A Filter must have exactly one of Prefix, Tag, or And specified.
 	if !f.And.isEmpty() {
-		if f.Prefix != "" {
+		if f.Prefix.set {
 			return errInvalidFilter
 		}
 		if !f.Tag.IsEmpty() {
@@ -75,12 +131,15 @@ func (f Filter) Validate() error {
 			return err
 		}
 	}
-	if f.Prefix != "" {
+	if f.Prefix.set {
 		if !f.Tag.IsEmpty() {
 			return errInvalidFilter
 		}
 	}
 	if !f.Tag.IsEmpty() {
+		if f.Prefix.set {
+			return errInvalidFilter
+		}
 		if err := f.Tag.Validate(); err != nil {
 			return err
 		}

pkg/bucket/lifecycle/filter_test.go

@@ -35,7 +35,7 @@ func TestUnsupportedFilters(t *testing.T) {
 							<Prefix>key-prefix</Prefix>
 						</And>
 						</Filter>`,
-			expectedErr: nil,
+			expectedErr: errXMLNotWellFormed,
 		},
 		{ // Filter with Tag tags
 			inputXML: ` <Filter>
@@ -85,6 +85,7 @@ func TestUnsupportedFilters(t *testing.T) {
 		{ // Filter with And and multiple Tag tags
 			inputXML: ` <Filter>
 							<And>
+							<Prefix></Prefix>
 							<Tag>
 								<Key>key1</Key>
 								<Value>value1</Value>

pkg/bucket/lifecycle/lifecycle.go

@@ -79,16 +79,16 @@ func (lc Lifecycle) HasActiveRules(prefix string, recursive bool) bool {
 			continue
 		}
 
-		if len(prefix) > 0 && len(rule.Filter.Prefix) > 0 {
+		if len(prefix) > 0 && len(rule.GetPrefix()) > 0 {
 			if !recursive {
 				// If not recursive, incoming prefix must be in rule prefix
-				if !strings.HasPrefix(prefix, rule.Filter.Prefix) {
+				if !strings.HasPrefix(prefix, rule.GetPrefix()) {
 					continue
 				}
 			}
 			if recursive {
 				// If recursive, we can skip this rule if it doesn't match the tested prefix.
-				if !strings.HasPrefix(prefix, rule.Filter.Prefix) && !strings.HasPrefix(rule.Filter.Prefix, prefix) {
+				if !strings.HasPrefix(prefix, rule.GetPrefix()) && !strings.HasPrefix(rule.GetPrefix(), prefix) {
 					continue
 				}
 			}
@@ -167,7 +167,7 @@ func (lc Lifecycle) FilterActionableRules(obj ObjectOpts) []Rule {
 		if rule.Status == Disabled {
 			continue
 		}
-		if !strings.HasPrefix(obj.Name, rule.Prefix()) {
+		if !strings.HasPrefix(obj.Name, rule.GetPrefix()) {
 			continue
 		}
 		// Indicates whether MinIO will remove a delete marker with no

pkg/bucket/lifecycle/lifecycle_test.go

@@ -25,72 +25,6 @@ import (
 )
 
 func TestParseAndValidateLifecycleConfig(t *testing.T) {
-	// Test for  lifecycle config with more than 1000 rules
-	var manyRules []Rule
-	for i := 0; i < 1001; i++ {
-		rule := Rule{
-			ID:         fmt.Sprintf("toManyRule%d", i),
-			Status:     "Enabled",
-			Expiration: Expiration{Days: ExpirationDays(i)},
-		}
-		manyRules = append(manyRules, rule)
-	}
-
-	manyRuleLcConfig, err := xml.Marshal(Lifecycle{Rules: manyRules})
-	if err != nil {
-		t.Fatal("Failed to marshal lifecycle config with more than 1000 rules")
-	}
-
-	// Test for lifecycle config with rules containing overlapping prefixes
-	rule1 := Rule{
-		ID:         "rule1",
-		Status:     "Enabled",
-		Expiration: Expiration{Days: ExpirationDays(3)},
-		Filter: Filter{
-			Prefix: "/a/b",
-		},
-	}
-	rule2 := Rule{
-		ID:         "rule2",
-		Status:     "Enabled",
-		Expiration: Expiration{Days: ExpirationDays(3)},
-		Filter: Filter{
-			And: And{
-				Prefix: "/a/b/c",
-			},
-		},
-	}
-	overlappingRules := []Rule{rule1, rule2}
-	overlappingLcConfig, err := xml.Marshal(Lifecycle{Rules: overlappingRules})
-	if err != nil {
-		t.Fatal("Failed to marshal lifecycle config with rules having overlapping prefix")
-	}
-
-	// Test for lifecycle rules with duplicate IDs
-	rule3 := Rule{
-		ID:         "duplicateID",
-		Status:     "Enabled",
-		Expiration: Expiration{Days: ExpirationDays(3)},
-		Filter: Filter{
-			Prefix: "/a/b",
-		},
-	}
-	rule4 := Rule{
-		ID:         "duplicateID",
-		Status:     "Enabled",
-		Expiration: Expiration{Days: ExpirationDays(4)},
-		Filter: Filter{
-			And: And{
-				Prefix: "/x/z",
-			},
-		},
-	}
-	duplicateIDRules := []Rule{rule3, rule4}
-	duplicateIDLcConfig, err := xml.Marshal(Lifecycle{Rules: duplicateIDRules})
-	if err != nil {
-		t.Fatal("Failed to marshal lifecycle config of rules with duplicate ID.")
-	}
-
 	testCases := []struct {
 		inputConfig           string
 		expectedParsingErr    error
@@ -136,21 +70,28 @@ func TestParseAndValidateLifecycleConfig(t *testing.T) {
 			expectedParsingErr:    nil,
 			expectedValidationErr: errLifecycleNoRule,
 		},
-		{ // lifecycle config with more than 1000 rules
-			inputConfig:           string(manyRuleLcConfig),
-			expectedParsingErr:    nil,
-			expectedValidationErr: errLifecycleTooManyRules,
-		},
 		{ // lifecycle config with rules having overlapping prefix
-			inputConfig:           string(overlappingLcConfig),
+			inputConfig:           `<LifecycleConfiguration><Rule><ID>rule1</ID><Status>Enabled</Status><Filter><Prefix>/a/b</Prefix></Filter><Expiration><Days>3</Days></Expiration></Rule><Rule><ID>rule2</ID><Status>Enabled</Status><Filter><And><Prefix>/a/b/c</Prefix><Tag><Key>key1</Key><Value>val1</Value></Tag></And></Filter><Expiration><Days>3</Days></Expiration></Rule></LifecycleConfiguration> `,
 			expectedParsingErr:    nil,
 			expectedValidationErr: nil,
 		},
 		{ // lifecycle config with rules having duplicate ID
-			inputConfig:           string(duplicateIDLcConfig),
+			inputConfig:           `<LifecycleConfiguration><Rule><ID>duplicateID</ID><Status>Enabled</Status><Filter><Prefix>/a/b</Prefix></Filter><Expiration><Days>3</Days></Expiration></Rule><Rule><ID>duplicateID</ID><Status>Enabled</Status><Filter><And><Prefix>/x/z</Prefix><Tag><Key>key1</Key><Value>val1</Value></Tag></And></Filter><Expiration><Days>4</Days></Expiration></Rule></LifecycleConfiguration>`,
 			expectedParsingErr:    nil,
 			expectedValidationErr: errLifecycleDuplicateID,
 		},
+		// Missing <Tag> in <And>
+		{
+			inputConfig:           `<LifecycleConfiguration xmlns="http://s3.amazonaws.com/doc/2006-03-01/"><Rule><ID>sample-rule-2</ID><Filter><And><Prefix>/a/b/c</Prefix></And></Filter><Status>Enabled</Status><Expiration><Days>1</Days></Expiration></Rule></LifecycleConfiguration>`,
+			expectedParsingErr:    nil,
+			expectedValidationErr: errXMLNotWellFormed,
+		},
+		// Legitimate lifecycle
+		{
+			inputConfig:           `<LifecycleConfiguration xmlns="http://s3.amazonaws.com/doc/2006-03-01/"><Rule><ID>rule</ID><Prefix /><Status>Enabled</Status><Expiration><Days>1</Days></Expiration></Rule></LifecycleConfiguration>`,
+			expectedParsingErr:    nil,
+			expectedValidationErr: nil,
+		},
 	}
 
 	for i, tc := range testCases {
@@ -181,17 +122,17 @@ func TestMarshalLifecycleConfig(t *testing.T) {
 		Rules: []Rule{
 			{
 				Status:     "Enabled",
-				Filter:     Filter{Prefix: "prefix-1"},
+				Filter:     Filter{Prefix: Prefix{string: "prefix-1", set: true}},
 				Expiration: Expiration{Days: ExpirationDays(3)},
 			},
 			{
 				Status:     "Enabled",
-				Filter:     Filter{Prefix: "prefix-1"},
+				Filter:     Filter{Prefix: Prefix{string: "prefix-1", set: true}},
 				Expiration: Expiration{Date: ExpirationDate(midnightTS)},
 			},
 			{
 				Status:                      "Enabled",
-				Filter:                      Filter{Prefix: "prefix-1"},
+				Filter:                      Filter{Prefix: Prefix{string: "prefix-1", set: true}},
 				Expiration:                  Expiration{Date: ExpirationDate(midnightTS)},
 				NoncurrentVersionTransition: NoncurrentVersionTransition{NoncurrentDays: 2, StorageClass: "TEST"},
 			},

pkg/bucket/lifecycle/noncurrentversion.go

@@ -1,5 +1,5 @@
 /*
- * MinIO Cloud Storage, (C) 2019 MinIO, Inc.
+ * MinIO Cloud Storage, (C) 2019-2020 MinIO, Inc.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -24,12 +24,7 @@ import (
 type NoncurrentVersionExpiration struct {
 	XMLName        xml.Name       `xml:"NoncurrentVersionExpiration"`
 	NoncurrentDays ExpirationDays `xml:"NoncurrentDays,omitempty"`
-}
-
-// NoncurrentVersionTransition - an action for lifecycle configuration rule.
-type NoncurrentVersionTransition struct {
-	NoncurrentDays ExpirationDays `xml:"NoncurrentDays"`
-	StorageClass   string         `xml:"StorageClass"`
+	set            bool
 }
 
 // MarshalXML if non-current days not set to non zero value
@@ -41,11 +36,43 @@ func (n NoncurrentVersionExpiration) MarshalXML(e *xml.Encoder, start xml.StartE
 	return e.EncodeElement(noncurrentVersionExpirationWrapper(n), start)
 }
 
+// UnmarshalXML decodes NoncurrentVersionExpiration
+func (n *NoncurrentVersionExpiration) UnmarshalXML(d *xml.Decoder, startElement xml.StartElement) error {
+	type noncurrentVersionExpirationWrapper NoncurrentVersionExpiration
+	var val noncurrentVersionExpirationWrapper
+	err := d.DecodeElement(&val, &startElement)
+	if err != nil {
+		return err
+	}
+	*n = NoncurrentVersionExpiration(val)
+	n.set = true
+	return nil
+}
+
 // IsDaysNull returns true if days field is null
 func (n NoncurrentVersionExpiration) IsDaysNull() bool {
 	return n.NoncurrentDays == ExpirationDays(0)
 }
 
+// Validate returns an error with wrong value
+func (n NoncurrentVersionExpiration) Validate() error {
+	if !n.set {
+		return nil
+	}
+	val := int(n.NoncurrentDays)
+	if val <= 0 {
+		return errXMLNotWellFormed
+	}
+	return nil
+}
+
+// NoncurrentVersionTransition - an action for lifecycle configuration rule.
+type NoncurrentVersionTransition struct {
+	NoncurrentDays ExpirationDays `xml:"NoncurrentDays"`
+	StorageClass   string         `xml:"StorageClass"`
+	set            bool
+}
+
 // MarshalXML is extended to leave out
 // <NoncurrentVersionTransition></NoncurrentVersionTransition> tags
 func (n NoncurrentVersionTransition) MarshalXML(e *xml.Encoder, start xml.StartElement) error {
@@ -60,3 +87,27 @@ func (n NoncurrentVersionTransition) MarshalXML(e *xml.Encoder, start xml.StartE
 func (n NoncurrentVersionTransition) IsDaysNull() bool {
 	return n.NoncurrentDays == ExpirationDays(0)
 }
+
+// UnmarshalXML decodes NoncurrentVersionExpiration
+func (n *NoncurrentVersionTransition) UnmarshalXML(d *xml.Decoder, startElement xml.StartElement) error {
+	type noncurrentVersionTransitionWrapper NoncurrentVersionTransition
+	var val noncurrentVersionTransitionWrapper
+	err := d.DecodeElement(&val, &startElement)
+	if err != nil {
+		return err
+	}
+	*n = NoncurrentVersionTransition(val)
+	n.set = true
+	return nil
+}
+
+// Validate returns an error with wrong value
+func (n NoncurrentVersionTransition) Validate() error {
+	if !n.set {
+		return nil
+	}
+	if int(n.NoncurrentDays) <= 0 || n.StorageClass == "" {
+		return errXMLNotWellFormed
+	}
+	return nil
+}

pkg/bucket/lifecycle/prefix.go

@@ -0,0 +1,50 @@
+/*
+ * MinIO Cloud Storage, (C) 2020 MinIO, Inc.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package lifecycle
+
+import (
+	"encoding/xml"
+)
+
+// Prefix holds the prefix xml tag in <Rule> and <Filter>
+type Prefix struct {
+	string
+	set bool
+}
+
+// UnmarshalXML - decodes XML data.
+func (p *Prefix) UnmarshalXML(d *xml.Decoder, start xml.StartElement) (err error) {
+	var s string
+	if err = d.DecodeElement(&s, &start); err != nil {
+		return err
+	}
+	*p = Prefix{string: s, set: true}
+	return nil
+}
+
+// MarshalXML - decodes XML data.
+func (p Prefix) MarshalXML(e *xml.Encoder, startElement xml.StartElement) error {
+	if !p.set {
+		return nil
+	}
+	return e.EncodeElement(p.string, startElement)
+}
+
+// String returns the prefix string
+func (p Prefix) String() string {
+	return p.string
+}

pkg/bucket/lifecycle/rule.go

@@ -38,6 +38,7 @@ type Rule struct {
 	ID         string     `xml:"ID,omitempty"`
 	Status     Status     `xml:"Status"`
 	Filter     Filter     `xml:"Filter,omitempty"`
+	Prefix     Prefix     `xml:"Prefix,omitempty"`
 	Expiration Expiration `xml:"Expiration,omitempty"`
 	Transition Transition `xml:"Transition,omitempty"`
 	// FIXME: add a type to catch unsupported AbortIncompleteMultipartUpload AbortIncompleteMultipartUpload `xml:"AbortIncompleteMultipartUpload,omitempty"`
@@ -92,27 +93,44 @@ func (r Rule) validateStatus() error {
 	return nil
 }
 
-func (r Rule) validateAction() error {
+func (r Rule) validateExpiration() error {
 	return r.Expiration.Validate()
 }
 
-func (r Rule) validateFilter() error {
-	return r.Filter.Validate()
+func (r Rule) validateNoncurrentExpiration() error {
+	return r.NoncurrentVersionExpiration.Validate()
+}
+
+func (r Rule) validatePrefixAndFilter() error {
+	if !r.Prefix.set && r.Filter.IsEmpty() || r.Prefix.set && !r.Filter.IsEmpty() {
+		return errXMLNotWellFormed
+	}
+	if !r.Prefix.set {
+		return r.Filter.Validate()
+	}
+	return nil
 }
 
 func (r Rule) validateTransition() error {
 	return r.Transition.Validate()
 }
 
-// Prefix - a rule can either have prefix under <filter></filter> or under
-// <filter><and></and></filter>. This method returns the prefix from the
-// location where it is available
-func (r Rule) Prefix() string {
-	if r.Filter.Prefix != "" {
-		return r.Filter.Prefix
+func (r Rule) validateNoncurrentTransition() error {
+	return r.NoncurrentVersionTransition.Validate()
+}
+
+// GetPrefix - a rule can either have prefix under <rule></rule>, <filter></filter>
+// or under <filter><and></and></filter>. This method returns the prefix from the
+// location where it is available.
+func (r Rule) GetPrefix() string {
+	if p := r.Prefix.String(); p != "" {
+		return p
 	}
-	if r.Filter.And.Prefix != "" {
-		return r.Filter.And.Prefix
+	if p := r.Filter.Prefix.String(); p != "" {
+		return p
+	}
+	if p := r.Filter.And.Prefix.String(); p != "" {
+		return p
 	}
 	return ""
 }
@@ -145,14 +163,23 @@ func (r Rule) Validate() error {
 	if err := r.validateStatus(); err != nil {
 		return err
 	}
-	if err := r.validateAction(); err != nil {
+	if err := r.validateExpiration(); err != nil {
+		return err
+	}
+	if err := r.validateNoncurrentExpiration(); err != nil {
 		return err
 	}
-	if err := r.validateFilter(); err != nil {
+	if err := r.validatePrefixAndFilter(); err != nil {
 		return err
 	}
 	if err := r.validateTransition(); err != nil {
 		return err
 	}
+	if err := r.validateNoncurrentTransition(); err != nil {
+		return err
+	}
+	if !r.Expiration.set && !r.Transition.set && !r.NoncurrentVersionExpiration.set && !r.NoncurrentVersionTransition.set {
+		return errXMLNotWellFormed
+	}
 	return nil
 }

pkg/bucket/lifecycle/rule_test.go

@@ -38,6 +38,7 @@ func TestInvalidRules(t *testing.T) {
 		{ // Rule with empty ID
 			inputXML: `<Rule>
 							<ID></ID>
+							<Filter><Prefix></Prefix></Filter>
 							<Expiration>
 								<Days>365</Days>
 							</Expiration>