From 3107c9212f044d8bd712e158c926ff023b1f7590 Mon Sep 17 00:00:00 2001 From: Bill Havanki Date: Fri, 16 Aug 2024 15:18:44 -0400 Subject: [PATCH] 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. --- store/ssmstore.go | 94 +++++++++++++++++++++- store/ssmstore_test.go | 175 +++++++++++++++++++++++++++++++++++++---- store/store.go | 8 ++ 3 files changed, 262 insertions(+), 15 deletions(-) diff --git a/store/ssmstore.go b/store/ssmstore.go index aca5171..8800a96 100644 --- 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,12 +348,17 @@ 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 } @@ -297,6 +366,29 @@ func (s *SSMStore) DeleteTags(ctx context.Context, id SecretId, tagKeys []string 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 index e4e69b1..14dc8fe 100644 --- 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 index 5710292..5332eea 100644 --- 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 +}