fix: Don't allow to set unconfigured notification ARNs (#8643)

Fixes #8642

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{}
+		config.XMLNS = "http://s3.amazonaws.com/doc/2006-03-01/"
-	} else {
+		config.SetRegion(globalServerRegion)
-		if err = xml.NewDecoder(bytes.NewReader(configData)).Decode(&config); err != nil {
+		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,18 +180,16 @@ func (api objectAPIHandlers) PutBucketNotificationHandler(w http.ResponseWriter,
 		return
 	}
 
-	var config *event.Config
+	lreader := io.LimitReader(r.Body, r.ContentLength)
-	config, err = event.ParseConfig(io.LimitReader(r.Body, r.ContentLength), globalServerRegion, globalNotificationSys.targetList)
+	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
 	}
-	}
 
 	if err = saveNotificationConfig(ctx, objectAPI, bucketName, config); err != nil {
 		writeErrorResponse(ctx, w, toAPIError(ctx, err), r.URL, guessIsBrowserReq(r))

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) {

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 {
+func TestNotificationTargets(cfg config.Config, doneCh <-chan struct{}, transport *http.Transport,
-	_, err := RegisterNotificationTargets(cfg, doneCh, transport, true)
+	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.<TARGET_NAME>.
 // * 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)
+			}
 		}
 	}
 

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()

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
 }