From 471a3a650aa1895c06ae47715fd61bc55ac0eb41 Mon Sep 17 00:00:00 2001 From: Harshavardhana Date: Fri, 13 Dec 2019 12:36:45 -0800 Subject: [PATCH] fix: Don't allow to set unconfigured notification ARNs (#8643) Fixes #8642 --- cmd/bucket-notification-handlers.go | 53 +++++++++++++---- cmd/config-current.go | 3 +- cmd/config/notify/parse.go | 90 +++++++++++++++-------------- cmd/notification.go | 17 ++++++ pkg/event/config.go | 11 ++-- 5 files changed, 114 insertions(+), 60 deletions(-) diff --git a/cmd/bucket-notification-handlers.go b/cmd/bucket-notification-handlers.go index c94fcae3f..d42797b7a 100644 --- a/cmd/bucket-notification-handlers.go +++ b/cmd/bucket-notification-handlers.go @@ -17,7 +17,6 @@ package cmd import ( - "bytes" "encoding/json" "encoding/xml" "errors" @@ -25,6 +24,7 @@ import ( "net/http" "net/url" "path" + "reflect" "time" "github.com/gorilla/mux" @@ -52,7 +52,6 @@ func (api objectAPIHandlers) GetBucketNotificationHandler(w http.ResponseWriter, vars := mux.Vars(r) bucketName := vars["bucket"] - var config *event.Config objAPI := api.ObjectAPI() if objAPI == nil { @@ -79,22 +78,56 @@ func (api objectAPIHandlers) GetBucketNotificationHandler(w http.ResponseWriter, // Construct path to notification.xml for the given bucket. configFile := path.Join(bucketConfigPrefix, bucketName, bucketNotificationConfig) + var config = event.Config{} configData, err := readConfig(ctx, objAPI, configFile) if err != nil { if err != errConfigNotFound { writeErrorResponse(ctx, w, toAPIError(ctx, err), r.URL, guessIsBrowserReq(r)) return } - config = &event.Config{} - } else { - if err = xml.NewDecoder(bytes.NewReader(configData)).Decode(&config); err != nil { + config.XMLNS = "http://s3.amazonaws.com/doc/2006-03-01/" + config.SetRegion(globalServerRegion) + notificationBytes, err := xml.Marshal(config) + if err != nil { writeErrorResponse(ctx, w, toAPIError(ctx, err), r.URL, guessIsBrowserReq(r)) return } + + writeSuccessResponseXML(w, notificationBytes) + return } config.SetRegion(globalServerRegion) + if err = xml.Unmarshal(configData, &config); err != nil { + writeErrorResponse(ctx, w, toAPIError(ctx, err), r.URL, guessIsBrowserReq(r)) + return + } + + if err = config.Validate(globalServerRegion, globalNotificationSys.targetList); err != nil { + arnErr, ok := err.(*event.ErrARNNotFound) + if !ok { + writeErrorResponse(ctx, w, toAPIError(ctx, err), r.URL, guessIsBrowserReq(r)) + return + } + for i, queue := range config.QueueList { + // Remove ARN not found queues, because we previously allowed + // adding unexpected entries into the config. + // + // With newer config disallowing changing / turning off + // notification targets without removing ARN in notification + // configuration we won't see this problem anymore. + if reflect.DeepEqual(queue.ARN, arnErr.ARN) { + config.QueueList = append(config.QueueList[:i], + config.QueueList[i+1:]...) + } + // This is a one time activity we shall do this + // here and allow stale ARN to be removed. We shall + // never reach a stage where we will have stale + // notification configs. + } + } + // If xml namespace is empty, set a default value before returning. if config.XMLNS == "" { config.XMLNS = "http://s3.amazonaws.com/doc/2006-03-01/" @@ -147,17 +180,15 @@ func (api objectAPIHandlers) PutBucketNotificationHandler(w http.ResponseWriter, return } - var config *event.Config - config, err = event.ParseConfig(io.LimitReader(r.Body, r.ContentLength), globalServerRegion, globalNotificationSys.targetList) + lreader := io.LimitReader(r.Body, r.ContentLength) + config, err := event.ParseConfig(lreader, globalServerRegion, globalNotificationSys.targetList) if err != nil { apiErr := errorCodes.ToAPIErr(ErrMalformedXML) if event.IsEventError(err) { apiErr = toAPIError(ctx, err) } - if _, ok := err.(*event.ErrARNNotFound); !ok { - writeErrorResponse(ctx, w, apiErr, r.URL, guessIsBrowserReq(r)) - return - } + writeErrorResponse(ctx, w, apiErr, r.URL, guessIsBrowserReq(r)) + return } if err = saveNotificationConfig(ctx, objectAPI, bucketName, config); err != nil { diff --git a/cmd/config-current.go b/cmd/config-current.go index bbea8753b..cc30f6fff 100644 --- a/cmd/config-current.go +++ b/cmd/config-current.go @@ -282,7 +282,8 @@ func validateConfig(s config.Config) error { return err } - return notify.TestNotificationTargets(s, GlobalServiceDoneCh, NewCustomHTTPTransport()) + return notify.TestNotificationTargets(s, GlobalServiceDoneCh, NewCustomHTTPTransport(), + globalNotificationSys.ConfiguredTargetIDs()) } func lookupConfigs(s config.Config) (err error) { diff --git a/cmd/config/notify/parse.go b/cmd/config/notify/parse.go index db5d2f2fd..0e3f19864 100644 --- a/cmd/config/notify/parse.go +++ b/cmd/config/notify/parse.go @@ -39,15 +39,18 @@ const ( // TestNotificationTargets is similar to GetNotificationTargets() // avoids explicit registration. -func TestNotificationTargets(cfg config.Config, doneCh <-chan struct{}, transport *http.Transport) error { - _, err := RegisterNotificationTargets(cfg, doneCh, transport, true) +func TestNotificationTargets(cfg config.Config, doneCh <-chan struct{}, transport *http.Transport, + targetIDs []event.TargetID) error { + test := true + _, err := RegisterNotificationTargets(cfg, doneCh, transport, targetIDs, test) return err } // GetNotificationTargets registers and initializes all notification // targets, returns error if any. func GetNotificationTargets(cfg config.Config, doneCh <-chan struct{}, transport *http.Transport) (*event.TargetList, error) { - return RegisterNotificationTargets(cfg, doneCh, transport, false) + test := false + return RegisterNotificationTargets(cfg, doneCh, transport, nil, test) } // RegisterNotificationTargets - returns TargetList which contains enabled targets in serverConfig. @@ -55,7 +58,7 @@ func GetNotificationTargets(cfg config.Config, doneCh <-chan struct{}, transport // * Add a new target in pkg/event/target package. // * Add newly added target configuration to serverConfig.Notify.. // * Handle the configuration in this function to create/add into TargetList. -func RegisterNotificationTargets(cfg config.Config, doneCh <-chan struct{}, transport *http.Transport, test bool) (*event.TargetList, error) { +func RegisterNotificationTargets(cfg config.Config, doneCh <-chan struct{}, transport *http.Transport, targetIDs []event.TargetID, test bool) (*event.TargetList, error) { targetList := event.NewTargetList() if err := checkValidNotificationKeys(cfg); err != nil { return nil, err @@ -119,13 +122,12 @@ func RegisterNotificationTargets(cfg config.Config, doneCh <-chan struct{}, tran if err != nil { return nil, err } - if test { - newTarget.Close() - continue - } if err = targetList.Add(newTarget); err != nil { return nil, err } + if test { + newTarget.Close() + } } for id, args := range esTargets { @@ -137,13 +139,12 @@ func RegisterNotificationTargets(cfg config.Config, doneCh <-chan struct{}, tran return nil, err } - if test { - newTarget.Close() - continue - } if err = targetList.Add(newTarget); err != nil { return nil, err } + if test { + newTarget.Close() + } } for id, args := range kafkaTargets { @@ -155,13 +156,12 @@ func RegisterNotificationTargets(cfg config.Config, doneCh <-chan struct{}, tran if err != nil { return nil, err } - if test { - newTarget.Close() - continue - } if err = targetList.Add(newTarget); err != nil { return nil, err } + if test { + newTarget.Close() + } } for id, args := range mqttTargets { @@ -173,13 +173,12 @@ func RegisterNotificationTargets(cfg config.Config, doneCh <-chan struct{}, tran if err != nil { return nil, err } - if test { - newTarget.Close() - continue - } if err = targetList.Add(newTarget); err != nil { return nil, err } + if test { + newTarget.Close() + } } for id, args := range mysqlTargets { @@ -190,13 +189,12 @@ func RegisterNotificationTargets(cfg config.Config, doneCh <-chan struct{}, tran if err != nil { return nil, err } - if test { - newTarget.Close() - continue - } if err = targetList.Add(newTarget); err != nil { return nil, err } + if test { + newTarget.Close() + } } for id, args := range natsTargets { @@ -207,13 +205,12 @@ func RegisterNotificationTargets(cfg config.Config, doneCh <-chan struct{}, tran if err != nil { return nil, err } - if test { - newTarget.Close() - continue - } if err = targetList.Add(newTarget); err != nil { return nil, err } + if test { + newTarget.Close() + } } for id, args := range nsqTargets { @@ -224,13 +221,12 @@ func RegisterNotificationTargets(cfg config.Config, doneCh <-chan struct{}, tran if err != nil { return nil, err } - if test { - newTarget.Close() - continue - } if err = targetList.Add(newTarget); err != nil { return nil, err } + if test { + newTarget.Close() + } } for id, args := range postgresTargets { @@ -241,13 +237,12 @@ func RegisterNotificationTargets(cfg config.Config, doneCh <-chan struct{}, tran if err != nil { return nil, err } - if test { - newTarget.Close() - continue - } if err = targetList.Add(newTarget); err != nil { return nil, err } + if test { + newTarget.Close() + } } for id, args := range redisTargets { @@ -258,13 +253,12 @@ func RegisterNotificationTargets(cfg config.Config, doneCh <-chan struct{}, tran if err != nil { return nil, err } - if test { - newTarget.Close() - continue - } if err = targetList.Add(newTarget); err != nil { return nil, err } + if test { + newTarget.Close() + } } for id, args := range webhookTargets { @@ -275,12 +269,24 @@ func RegisterNotificationTargets(cfg config.Config, doneCh <-chan struct{}, tran if err != nil { return nil, err } + if err := targetList.Add(newTarget); err != nil { + return nil, err + } if test { newTarget.Close() continue } - if err := targetList.Add(newTarget); err != nil { - return nil, err + } + + if test { + // Verify if user is trying to disable already configured + // notification targets, based on their target IDs + for _, targetID := range targetIDs { + if !targetList.Exists(targetID) { + return nil, config.Errorf(config.SafeModeKind, + "Unable to disable configured targets '%v'", + targetID) + } } } diff --git a/cmd/notification.go b/cmd/notification.go index a83f5ed43..e1cdd2381 100644 --- a/cmd/notification.go +++ b/cmd/notification.go @@ -796,6 +796,23 @@ func (sys *NotificationSys) RemoveRulesMap(bucketName string, rulesMap event.Rul } } +// ConfiguredTargetIDs - returns list of configured target id's +func (sys *NotificationSys) ConfiguredTargetIDs() []event.TargetID { + sys.RLock() + defer sys.RUnlock() + + var targetIDs []event.TargetID + for _, rmap := range sys.bucketRulesMap { + for _, rules := range rmap { + for _, targetSet := range rules { + targetIDs = append(targetIDs, targetSet.ToSlice()...) + } + } + } + + return targetIDs +} + // RemoveNotification - removes all notification configuration for bucket name. func (sys *NotificationSys) RemoveNotification(bucketName string) { sys.Lock() diff --git a/pkg/event/config.go b/pkg/event/config.go index f9c938f10..7a31fdc47 100644 --- a/pkg/event/config.go +++ b/pkg/event/config.go @@ -255,8 +255,6 @@ func (conf Config) Validate(region string, targetList *TargetList) error { if err := queue.Validate(region, targetList); err != nil { return err } - - // TODO: Need to discuss/check why same ARN cannot be used in another queue configuration. } return nil @@ -283,13 +281,14 @@ func (conf *Config) ToRulesMap() RulesMap { // ParseConfig - parses data in reader to notification configuration. func ParseConfig(reader io.Reader, region string, targetList *TargetList) (*Config, error) { var config Config - var err error - if err = xml.NewDecoder(reader).Decode(&config); err != nil { + if err := xml.NewDecoder(reader).Decode(&config); err != nil { return nil, err } - err = config.Validate(region, targetList) + if err := config.Validate(region, targetList); err != nil { + return nil, err + } config.SetRegion(region) @@ -298,5 +297,5 @@ func ParseConfig(reader io.Reader, region string, targetList *TargetList) (*Conf config.XMLNS = "http://s3.amazonaws.com/doc/2006-03-01/" } - return &config, err + return &config, nil }