feat: Enforce required tags in SSM store (#548)

The behavior is as follows: * When writing a new secret, all required tags must be provided. * When writing tags to a secret, and other tags are being deleted, then all required tags that are already present on the secret must be provided. * When deleting tags from a secret, required tags may not be provided. The second item is a bit subtle. If other tags aren't being deleted, i.e., tags not provided are being left alone, then required tags don't have to be provided (they may be, but don't have to be). Also, if other tags *are* being deleted, then the only required tags that have to be provided are those that are already set. These behaviors are designed so that after a store's configuration is updated to require tags, you don't have to then immediately set them all on every existing secret. Once you do set them though, you can't get rid of them.
segmentio · Aug 16, 2024 · 3107c92 · 3107c92
1 parent 39c4376
commit 3107c92
Show file tree

Hide file tree

Showing 3 changed files with 262 additions and 15 deletions.
diff --git a/store/ssmstore.go b/store/ssmstore.go
@@ -162,6 +162,11 @@ func (s *SSMStore) write(ctx context.Context, id SecretId, value string, tags ma
 		return errors.New("tags on write only supported for new secrets")
 	}
 
+	err = s.checkForRequiredTags(ctx, tags, version)
+	if err != nil {
+		return err
+	}
+
 	putParameterInput := &ssm.PutParameterInput{
 		KeyId:       aws.String(s.KMSKey()),
 		Name:        aws.String(s.idToName(id)),
@@ -186,6 +191,28 @@ func (s *SSMStore) write(ctx context.Context, id SecretId, value string, tags ma
 	return nil
 }
 
+func (s *SSMStore) checkForRequiredTags(ctx context.Context, tags map[string]string, version int) error {
+	if version != 1 {
+		return nil
+	}
+	requiredTags, err := requiredTags(ctx, s)
+	if err != nil {
+		return err
+	}
+
+	var missingTags []string
+	for _, requiredTag := range requiredTags {
+		if _, ok := tags[requiredTag]; !ok {
+			missingTags = append(missingTags, requiredTag)
+		}
+	}
+	if len(missingTags) > 0 {
+		return fmt.Errorf("required tags %v are missing", missingTags)
+	}
+
+	return nil
+}
+
 // Read reads a secret from the parameter store at a specific version.
 // To grab the latest version, use -1 as the version number.
 func (s *SSMStore) Read(ctx context.Context, id SecretId, version int) (Secret, error) {
@@ -204,6 +231,14 @@ func (s *SSMStore) WriteTags(ctx context.Context, id SecretId, tags map[string]s
 			return err
 		}
 
+		// fail if any required tags are already present but not being written, because they'd be deleted
+		// (a required tag that hasn't been set yet may be left out)
+		err = s.checkForPresentRequiredTags(ctx, currentTags, tags)
+		if err != nil {
+			return err
+		}
+
+		// remove any tags that are not being written
 		var tagKeysToRemove []string
 		for k := range currentTags {
 			if _, ok := tags[k]; !ok {
@@ -218,6 +253,7 @@ func (s *SSMStore) WriteTags(ctx context.Context, id SecretId, tags map[string]s
 		}
 	}
 
+	// add or update tags
 	addTags := make([]types.Tag, len(tags))
 	i := 0
 	for k, v := range tags {
@@ -241,6 +277,34 @@ func (s *SSMStore) WriteTags(ctx context.Context, id SecretId, tags map[string]s
 	return nil
 }
 
+// checkForPresentRequiredTags returns an error if the given map of tags is
+// missing any required tags that are already present (in currentTags). This is
+// a problem only for a tag write command where any tags not being written are
+// to be deleted ("delete other tags"), because that would cause some required
+// tags to be deleted. Instead, the caller has to explicitly provide values for
+// all required tags, even if they aren't changing.
+func (s *SSMStore) checkForPresentRequiredTags(ctx context.Context, currentTags map[string]string, tags map[string]string) error {
+	requiredTags, err := requiredTags(ctx, s)
+	if err != nil {
+		return err
+	}
+	var missingTags []string
+	for _, requiredTag := range requiredTags {
+		_, alreadyPresent := currentTags[requiredTag]
+		_, beingUpdated := tags[requiredTag]
+		if alreadyPresent && !beingUpdated {
+			// this required tag is present already but isn't being rewritten, which
+			// is a problem when "delete other tags" is set
+			missingTags = append(missingTags, requiredTag)
+		}
+	}
+	if len(missingTags) > 0 {
+		return fmt.Errorf("required tags %v are already present, so they must be rewritten", missingTags)
+	}
+
+	return nil
+}
+
 func (s *SSMStore) ReadTags(ctx context.Context, id SecretId) (map[string]string, error) {
 	input := &ssm.ListTagsForResourceInput{
 		ResourceType: types.ResourceTypeForTaggingParameter,
@@ -284,19 +348,47 @@ func (s *SSMStore) Delete(ctx context.Context, id SecretId) error {
 }
 
 func (s *SSMStore) DeleteTags(ctx context.Context, id SecretId, tagKeys []string) error {
+	err := s.checkIfDeletingRequiredTags(ctx, tagKeys)
+	if err != nil {
+		return err
+	}
+
 	removeTagsInput := &ssm.RemoveTagsFromResourceInput{
 		ResourceType: types.ResourceTypeForTaggingParameter,
 		ResourceId:   aws.String(s.idToName(id)),
 		TagKeys:      tagKeys,
 	}
-	_, err := s.svc.RemoveTagsFromResource(ctx, removeTagsInput)
+	_, err = s.svc.RemoveTagsFromResource(ctx, removeTagsInput)
 	if err != nil {
 		return err
 	}
 
 	return nil
 }
 
+func (s *SSMStore) checkIfDeletingRequiredTags(ctx context.Context, tagKeys []string) error {
+	requiredTags, err := requiredTags(ctx, s)
+	if err != nil {
+		return err
+	}
+	tags := make(map[string]any)
+	for _, key := range tagKeys {
+		tags[key] = struct{}{}
+	}
+
+	var foundTags []string
+	for _, requiredTag := range requiredTags {
+		if _, ok := tags[requiredTag]; ok {
+			foundTags = append(foundTags, requiredTag)
+		}
+	}
+	if len(foundTags) > 0 {
+		return fmt.Errorf("required tags %v may not be deleted", foundTags)
+	}
+
+	return nil
+}
+
 func (s *SSMStore) readVersion(ctx context.Context, id SecretId, version int) (Secret, error) {
 	getParameterHistoryInput := &ssm.GetParameterHistoryInput{
 		Name:           aws.String(s.idToName(id)),

diff --git a/store/ssmstore_test.go b/store/ssmstore_test.go
@@ -507,6 +507,41 @@ func TestWriteWithTags(t *testing.T) {
 	})
 }
 
+func TestWriteWithRequiredTags(t *testing.T) {
+	ctx := context.Background()
+	storeConfigMockParameter := storeConfigMockParameter(`{"version":"1","requiredTags":["key1", "key2"]}`)
+	parameters := map[string]mockParameter{
+		*storeConfigMockParameter.meta.Name: storeConfigMockParameter,
+	}
+	store := NewTestSSMStore(parameters)
+
+	t.Run("Setting a new key with required tags should work", func(t *testing.T) {
+		secretId := SecretId{Service: "test", Key: "mykey1"}
+		tags := map[string]string{
+			"key1": "value1",
+			"key2": "value2",
+			"key3": "value3",
+		}
+		err := store.WriteWithTags(ctx, secretId, "value", tags)
+		assert.NoError(t, err)
+		assert.Contains(t, parameters, store.idToName(secretId))
+
+		paramTags := parameters[store.idToName(secretId)].tags
+		assert.Equal(t, "value1", paramTags["key1"])
+		assert.Equal(t, "value2", paramTags["key2"])
+		assert.Equal(t, "value3", paramTags["key3"])
+	})
+	t.Run("Setting a new key without required tags should fail", func(t *testing.T) {
+		secretId := SecretId{Service: "test", Key: "mykey2"}
+		tags := map[string]string{
+			"key2": "value2",
+			"key3": "value3",
+		}
+		err := store.WriteWithTags(ctx, secretId, "value", tags)
+		assert.Error(t, err)
+	})
+}
+
 func TestReadPaths(t *testing.T) {
 	ctx := context.Background()
 	parameters := map[string]mockParameter{}
@@ -714,6 +749,82 @@ func TestWriteTags(t *testing.T) {
 	})
 }
 
+func TestWriteTagsWithRequiredTags(t *testing.T) {
+	ctx := context.Background()
+	storeConfigMockParameter := storeConfigMockParameter(`{"version":"1","requiredTags":["key1", "key2"]}`)
+	parameters := map[string]mockParameter{
+		*storeConfigMockParameter.meta.Name: storeConfigMockParameter,
+	}
+	store := NewTestSSMStore(parameters)
+	secretId := SecretId{Service: "test", Key: "key"}
+	require.NoError(t, store.WriteWithTags(ctx, secretId, "value", map[string]string{
+		"key1": "value1",
+		"key2": "value2",
+		"key3": "value3",
+	}))
+
+	t.Run("Writing tags can omit required tags when not deleting others", func(t *testing.T) {
+		tags := map[string]string{
+			"key3": "value3a",
+		}
+		err := store.WriteTags(ctx, secretId, tags, false)
+		assert.NoError(t, err)
+		assert.Contains(t, parameters, store.idToName(secretId))
+		paramTags := parameters[store.idToName(secretId)].tags
+		assert.Equal(t, "value1", paramTags["key1"])
+		assert.Equal(t, "value2", paramTags["key2"])
+		assert.Equal(t, "value3a", paramTags["key3"])
+	})
+
+	t.Run("Writing tags can include required tags when not deleting others", func(t *testing.T) {
+		tags := map[string]string{
+			"key1": "value1a",
+		}
+		err := store.WriteTags(ctx, secretId, tags, false)
+		assert.NoError(t, err)
+		assert.Contains(t, parameters, store.idToName(secretId))
+		paramTags := parameters[store.idToName(secretId)].tags
+		assert.Equal(t, "value1a", paramTags["key1"])
+		assert.Equal(t, "value2", paramTags["key2"])
+		assert.Equal(t, "value3a", paramTags["key3"])
+	})
+
+	t.Run("Writing tags must not omit present required tags when deleting others", func(t *testing.T) {
+		tags := map[string]string{
+			"key1": "value1b",
+			// skipping required key2
+			"key4": "value4",
+		}
+		err := store.WriteTags(ctx, secretId, tags, true)
+		assert.Error(t, err)
+		assert.Contains(t, parameters, store.idToName(secretId))
+		paramTags := parameters[store.idToName(secretId)].tags
+		assert.Equal(t, "value1a", paramTags["key1"])
+		assert.Equal(t, "value2", paramTags["key2"])
+		assert.Equal(t, "value3a", paramTags["key3"])
+		_, ok := paramTags["key4"]
+		assert.False(t, ok)
+	})
+
+	t.Run("Writing tags must include all present required tags when deleting others", func(t *testing.T) {
+		tags := map[string]string{
+			"key1": "value1b",
+			"key2": "value2b",
+			// key3 to be deleted
+			"key4": "value4",
+		}
+		err := store.WriteTags(ctx, secretId, tags, true)
+		assert.NoError(t, err)
+		assert.Contains(t, parameters, store.idToName(secretId))
+		paramTags := parameters[store.idToName(secretId)].tags
+		assert.Equal(t, "value1b", paramTags["key1"])
+		assert.Equal(t, "value2b", paramTags["key2"])
+		assert.Equal(t, "value4", paramTags["key4"])
+		_, ok := paramTags["key3"]
+		assert.False(t, ok)
+	})
+}
+
 func TestReadTags(t *testing.T) {
 	ctx := context.Background()
 	parameters := map[string]mockParameter{}
@@ -778,6 +889,37 @@ func TestDeleteTags(t *testing.T) {
 	})
 }
 
+func TestDeleteTagsWithRequiredTags(t *testing.T) {
+	ctx := context.Background()
+	storeConfigMockParameter := storeConfigMockParameter(`{"version":"1","requiredTags":["key1", "key2"]}`)
+	parameters := map[string]mockParameter{
+		*storeConfigMockParameter.meta.Name: storeConfigMockParameter,
+	}
+	store := NewTestSSMStore(parameters)
+	secretId := SecretId{Service: "test", Key: "key"}
+	require.NoError(t, store.WriteWithTags(ctx, secretId, "value", map[string]string{
+		"key1": "value1",
+		"key2": "value2",
+		"key3": "value3",
+	}))
+
+	t.Run("Deleting non-required tags should work", func(t *testing.T) {
+		err := store.DeleteTags(ctx, secretId, []string{"key3"})
+		assert.NoError(t, err)
+		readTags, err := store.ReadTags(ctx, secretId)
+		assert.NoError(t, err)
+		assert.Equal(t, map[string]string{"key1": "value1", "key2": "value2"}, readTags)
+	})
+
+	t.Run("Deleting required tags should fail", func(t *testing.T) {
+		err := store.DeleteTags(ctx, secretId, []string{"key2"})
+		assert.Error(t, err)
+		readTags, err := store.ReadTags(ctx, secretId)
+		assert.NoError(t, err)
+		assert.Equal(t, map[string]string{"key1": "value1", "key2": "value2"}, readTags)
+	})
+}
+
 func TestValidations(t *testing.T) {
 	parameters := map[string]mockParameter{}
 	pathStore := NewTestSSMStore(parameters)
@@ -856,21 +998,9 @@ func (a ByKeyRaw) Swap(i, j int)      { a[i], a[j] = a[j], a[i] }
 func (a ByKeyRaw) Less(i, j int) bool { return a[i].Key < a[j].Key }
 
 func TestSSMStoreConfig(t *testing.T) {
-	storeConfigName := fmt.Sprintf("/%s/%s", ChamberService, storeConfigKey)
+	storeConfigMockParameter := storeConfigMockParameter(`{"version":"2","requiredTags":["key1", "key2"]}`)
 	parameters := map[string]mockParameter{
-		storeConfigName: {
-			currentParam: &types.Parameter{
-				Name:  aws.String(storeConfigName),
-				Type:  types.ParameterTypeSecureString,
-				Value: aws.String(`{"version":"2","requiredTags":["key1", "key2"]}`),
-			},
-			meta: &types.ParameterMetadata{
-				Name:             aws.String(storeConfigName),
-				Description:      aws.String("1"),
-				LastModifiedDate: aws.Time(time.Now()),
-				LastModifiedUser: aws.String("test"),
-			},
-		},
+		*storeConfigMockParameter.meta.Name: storeConfigMockParameter,
 	}
 	store := NewTestSSMStore(parameters)
 
@@ -910,3 +1040,20 @@ func TestSSMStoreSetConfig(t *testing.T) {
 	assert.Equal(t, "2.1", config.Version)
 	assert.Equal(t, []string{"key1.1", "key2.1"}, config.RequiredTags)
 }
+
+func storeConfigMockParameter(content string) mockParameter {
+	storeConfigName := fmt.Sprintf("/%s/%s", ChamberService, storeConfigKey)
+	return mockParameter{
+		currentParam: &types.Parameter{
+			Name:  aws.String(storeConfigName),
+			Type:  types.ParameterTypeSecureString,
+			Value: aws.String(content),
+		},
+		meta: &types.ParameterMetadata{
+			Name:             aws.String(storeConfigName),
+			Description:      aws.String("1"),
+			LastModifiedDate: aws.Time(time.Now()),
+			LastModifiedUser: aws.String("test"),
+		},
+	}
+}
diff --git a/store/store.go b/store/store.go
@@ -99,3 +99,11 @@ type Store interface {
 	Delete(ctx context.Context, id SecretId) error
 	DeleteTags(ctx context.Context, id SecretId, tagKeys []string) error
 }
+
+func requiredTags(ctx context.Context, s Store) ([]string, error) {
+	config, err := s.Config(ctx)
+	if err != nil {
+		return nil, err
+	}
+	return config.RequiredTags, nil
+}