From 96009975d6c0900dc1df5d3c3f6fe975ca38d520 Mon Sep 17 00:00:00 2001 From: Anis Elleuch Date: Mon, 18 May 2020 16:33:43 +0100 Subject: [PATCH] relax validation when loading lifecycle document from the backend (#9612) --- cmd/bucket-lifecycle-handler.go | 6 +++++ cmd/bucket-lifecycle.go | 9 ++++++- pkg/bucket/lifecycle/and.go | 8 +++---- pkg/bucket/lifecycle/filter.go | 4 +--- pkg/bucket/lifecycle/lifecycle.go | 3 --- pkg/bucket/lifecycle/lifecycle_test.go | 33 ++++++++++++++++---------- pkg/bucket/lifecycle/tag.go | 29 ++++++++++++++++------ 7 files changed, 61 insertions(+), 31 deletions(-) diff --git a/cmd/bucket-lifecycle-handler.go b/cmd/bucket-lifecycle-handler.go index 285968058..58ecd8a09 100644 --- a/cmd/bucket-lifecycle-handler.go +++ b/cmd/bucket-lifecycle-handler.go @@ -72,6 +72,12 @@ func (api objectAPIHandlers) PutBucketLifecycleHandler(w http.ResponseWriter, r return } + // Validate the received bucket policy document + if err = bucketLifecycle.Validate(); err != nil { + writeErrorResponse(ctx, w, toAPIError(ctx, err), r.URL, guessIsBrowserReq(r)) + return + } + if err = objAPI.SetBucketLifecycle(ctx, bucket, bucketLifecycle); err != nil { writeErrorResponse(ctx, w, toAPIError(ctx, err), r.URL, guessIsBrowserReq(r)) return diff --git a/cmd/bucket-lifecycle.go b/cmd/bucket-lifecycle.go index b1784184a..12009b1d8 100644 --- a/cmd/bucket-lifecycle.go +++ b/cmd/bucket-lifecycle.go @@ -23,6 +23,7 @@ import ( "path" "sync" + "github.com/minio/minio/cmd/logger" "github.com/minio/minio/pkg/bucket/lifecycle" ) @@ -148,9 +149,15 @@ func (sys *LifecycleSys) load(buckets []BucketInfo, objAPI ObjectLayer) error { return err } + // Do not load the lifecycle configuration if it is not valid + err = config.Validate() + if err != nil { + logger.LogIf(context.Background(), err) + continue + } + sys.Set(bucket.Name, config) } - return nil } diff --git a/pkg/bucket/lifecycle/and.go b/pkg/bucket/lifecycle/and.go index 977a92745..c1ef17ca2 100644 --- a/pkg/bucket/lifecycle/and.go +++ b/pkg/bucket/lifecycle/and.go @@ -18,15 +18,13 @@ package lifecycle import ( "encoding/xml" - - "github.com/minio/minio-go/v6/pkg/tags" ) // 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"` - Tags []tags.Tag `xml:"Tag,omitempty"` + XMLName xml.Name `xml:"And"` + Prefix string `xml:"Prefix,omitempty"` + Tags []Tag `xml:"Tag,omitempty"` } var errDuplicateTagKey = Errorf("Duplicate Tag Keys are not allowed") diff --git a/pkg/bucket/lifecycle/filter.go b/pkg/bucket/lifecycle/filter.go index 563abac92..b505e9cf9 100644 --- a/pkg/bucket/lifecycle/filter.go +++ b/pkg/bucket/lifecycle/filter.go @@ -18,8 +18,6 @@ package lifecycle import ( "encoding/xml" - - "github.com/minio/minio-go/v6/pkg/tags" ) var ( @@ -31,7 +29,7 @@ type Filter struct { XMLName xml.Name `xml:"Filter"` Prefix string And And - Tag tags.Tag + Tag Tag } // MarshalXML - produces the xml representation of the Filter struct diff --git a/pkg/bucket/lifecycle/lifecycle.go b/pkg/bucket/lifecycle/lifecycle.go index 9b7ae7390..8ed208a9f 100644 --- a/pkg/bucket/lifecycle/lifecycle.go +++ b/pkg/bucket/lifecycle/lifecycle.go @@ -57,9 +57,6 @@ func ParseLifecycleConfig(reader io.Reader) (*Lifecycle, error) { if err := xml.NewDecoder(reader).Decode(&lc); err != nil { return nil, err } - if err := lc.Validate(); err != nil { - return nil, err - } return &lc, nil } diff --git a/pkg/bucket/lifecycle/lifecycle_test.go b/pkg/bucket/lifecycle/lifecycle_test.go index 0b7a2f1f8..b695620a6 100644 --- a/pkg/bucket/lifecycle/lifecycle_test.go +++ b/pkg/bucket/lifecycle/lifecycle_test.go @@ -24,7 +24,7 @@ import ( "time" ) -func TestParseLifecycleConfig(t *testing.T) { +func TestParseAndValidateLifecycleConfig(t *testing.T) { // Test for lifecycle config with more than 1000 rules var manyRules []Rule rule := Rule{ @@ -64,8 +64,9 @@ func TestParseLifecycleConfig(t *testing.T) { } testCases := []struct { - inputConfig string - expectedErr error + inputConfig string + expectedParsingErr error + expectedValidationErr error }{ { // Valid lifecycle config inputConfig: ` @@ -84,28 +85,36 @@ func TestParseLifecycleConfig(t *testing.T) { 3 `, - expectedErr: nil, + expectedParsingErr: nil, + expectedValidationErr: nil, }, { // lifecycle config with no rules inputConfig: ` `, - expectedErr: errLifecycleNoRule, + expectedParsingErr: nil, + expectedValidationErr: errLifecycleNoRule, }, { // lifecycle config with more than 1000 rules - inputConfig: string(manyRuleLcConfig), - expectedErr: errLifecycleTooManyRules, + inputConfig: string(manyRuleLcConfig), + expectedParsingErr: nil, + expectedValidationErr: errLifecycleTooManyRules, }, { // lifecycle config with rules having overlapping prefix - inputConfig: string(overlappingLcConfig), - expectedErr: errLifecycleOverlappingPrefix, + inputConfig: string(overlappingLcConfig), + expectedParsingErr: nil, + expectedValidationErr: errLifecycleOverlappingPrefix, }, } for i, tc := range testCases { t.Run(fmt.Sprintf("Test %d", i+1), func(t *testing.T) { - var err error - if _, err = ParseLifecycleConfig(bytes.NewReader([]byte(tc.inputConfig))); err != tc.expectedErr { - t.Fatalf("%d: Expected %v but got %v", i+1, tc.expectedErr, err) + lc, err := ParseLifecycleConfig(bytes.NewReader([]byte(tc.inputConfig))) + if err != tc.expectedParsingErr { + t.Fatalf("%d: Expected %v during parsing but got %v", i+1, tc.expectedParsingErr, err) + } + err = lc.Validate() + if err != tc.expectedValidationErr { + t.Fatalf("%d: Expected %v during parsing but got %v", i+1, tc.expectedValidationErr, err) } }) } diff --git a/pkg/bucket/lifecycle/tag.go b/pkg/bucket/lifecycle/tag.go index f3c9bcaed..ada5a4f4f 100644 --- a/pkg/bucket/lifecycle/tag.go +++ b/pkg/bucket/lifecycle/tag.go @@ -18,6 +18,7 @@ package lifecycle import ( "encoding/xml" + "unicode/utf8" ) // Tag - a tag for a lifecycle configuration Rule filter. @@ -27,15 +28,29 @@ type Tag struct { Value string `xml:"Value,omitempty"` } -var errTagUnsupported = Errorf("Specifying is not supported") +var ( + errInvalidTagKey = Errorf("The TagKey you have provided is invalid") + errInvalidTagValue = Errorf("The TagValue you have provided is invalid") +) -// UnmarshalXML is extended to indicate lack of support for Tag -// xml tag in object lifecycle configuration -func (t Tag) UnmarshalXML(d *xml.Decoder, start xml.StartElement) error { - return errTagUnsupported +func (tag Tag) String() string { + return tag.Key + "=" + tag.Value } -// MarshalXML is extended to leave out tags -func (t Tag) MarshalXML(e *xml.Encoder, start xml.StartElement) error { +// IsEmpty returns whether this tag is empty or not. +func (tag Tag) IsEmpty() bool { + return tag.Key == "" +} + +// Validate checks this tag. +func (tag Tag) Validate() error { + if len(tag.Key) == 0 || utf8.RuneCountInString(tag.Key) > 128 { + return errInvalidTagKey + } + + if utf8.RuneCountInString(tag.Value) > 256 { + return errInvalidTagValue + } + return nil }