feat: Secret case standardisation (#104)

* fix: make secrets case insensitive , always store as capitals

* fix: update doc to convey secret case standardisation info

* fix: integration tests

* fix: comment in migration file

* fix: test cases and methods type expectations

core/scheduler/service/notification_service.go

@@ -39,7 +39,7 @@ func (n *NotifyService) Push(ctx context.Context, event *scheduler.Event) error
 	}
 	notificationConfig := jobDetails.Alerts
 	multierror := errors.NewMultiError("ErrorsInNotifypush")
-	var secretMap map[string]string
+	var secretMap tenant.SecretMap
 	var plainTextSecretsList []*tenant.PlainTextSecret
 	for _, notify := range notificationConfig {
 		if event.Type.IsOfType(notify.On) {
@@ -57,15 +57,19 @@ func (n *NotifyService) Push(ctx context.Context, event *scheduler.Event) error
 						multierror.Append(err)
 						continue
 					}
-					secretMap = tenant.PlainTextSecrets(plainTextSecretsList).ToMap()
+					secretMap = tenant.PlainTextSecrets(plainTextSecretsList).ToSecretMap()
 				}
 
-				var secret string
+				var secretName string
 				switch scheme {
 				case NotificationSchemeSlack:
-					secret = secretMap[tenant.SecretNotifySlack]
+					secretName = tenant.SecretNotifySlack
 				case NotificationSchemePagerDuty:
-					secret = secretMap[strings.ReplaceAll(route, "#", "notify_")]
+					secretName = strings.ReplaceAll(route, "#", "notify_")
+				}
+				secret, err := secretMap.Get(secretName)
+				if err != nil {
+					return err
 				}
 
 				if notifyChannel, ok := n.notifyChannels[scheme]; ok {

core/tenant/handler/v1beta1/secret_test.go

@@ -281,7 +281,7 @@ func TestNewSecretsHandler(t *testing.T) {
 			_, err := handler.DeleteSecret(ctx, &deleteRequest)
 			assert.NotNil(t, err)
 			assert.EqualError(t, err, "rpc error: code = InvalidArgument desc = invalid argument for entity"+
-				" project: project name is empty: failed to delete secret name")
+				" project: project name is empty: failed to delete secret NAME")
 		})
 		t.Run("returns error when invalid secret name", func(t *testing.T) {
 			secretService := new(secretService)
@@ -316,7 +316,7 @@ func TestNewSecretsHandler(t *testing.T) {
 			_, err = handler.DeleteSecret(ctx, &deleteRequest)
 			assert.NotNil(t, err)
 			assert.EqualError(t, err, "rpc error: code = Internal desc = error in delete: failed to "+
-				"delete secret name")
+				"delete secret NAME")
 		})
 		t.Run("deletes the secrets", func(t *testing.T) {
 			secretService := new(secretService)

core/tenant/secret.go

@@ -1,6 +1,10 @@
 package tenant
 
-import "github.com/goto/optimus/internal/errors"
+import (
+	"strings"
+
+	"github.com/goto/optimus/internal/errors"
+)
 
 const (
 	EntitySecret = "secret"
@@ -16,7 +20,7 @@ func SecretNameFrom(name string) (SecretName, error) {
 	if name == "" {
 		return "", errors.InvalidArgument(EntitySecret, "secret name is empty")
 	}
-	return SecretName(name), nil
+	return SecretName(strings.ToUpper(name)), nil
 }
 
 func (sn SecretName) String() string {
@@ -54,14 +58,31 @@ func (p *PlainTextSecret) Name() SecretName {
 
 type PlainTextSecrets []*PlainTextSecret
 
-func (p PlainTextSecrets) ToMap() map[string]string {
+type SecretMap map[string]string
+
+func (p PlainTextSecrets) ToSecretMap() SecretMap {
 	secretMap := map[string]string{}
 	for _, item := range p {
 		secretMap[item.Name().String()] = item.Value()
 	}
 	return secretMap
 }
 
+func (s SecretMap) Get(secretName string) (string, error) {
+	if secretName == "" {
+		return "", errors.InvalidArgument(EntitySecret, "empty secret name")
+	}
+
+	if secret, ok := s[strings.ToUpper(secretName)]; ok {
+		return secret, nil
+	}
+	return "", errors.NotFound(EntitySecret, "value not found for: "+secretName)
+}
+
+func (s SecretMap) ToMap() map[string]string {
+	return s
+}
+
 type Secret struct {
 	name         SecretName
 	encodedValue string

core/tenant/secret_test.go

@@ -24,7 +24,7 @@ func TestEntitySecret(t *testing.T) {
 			pts, err := tenant.NewPlainTextSecret("secret_name", "secret_val")
 			assert.Nil(t, err)
 
-			assert.Equal(t, "secret_name", pts.Name().String())
+			assert.Equal(t, "SECRET_NAME", pts.Name().String())
 			assert.Equal(t, "secret_val", pts.Value())
 		})
 	})
@@ -51,7 +51,7 @@ func TestEntitySecret(t *testing.T) {
 			s, err := tenant.NewSecret("name", "encoded==", projName, nsName)
 
 			assert.Nil(t, err)
-			assert.Equal(t, "name", s.Name().String())
+			assert.Equal(t, "NAME", s.Name().String())
 			assert.Equal(t, "encoded==", s.EncodedValue())
 			assert.Equal(t, projName.String(), s.ProjectName().String())
 			assert.Equal(t, nsName, s.NamespaceName())

core/tenant/service/secret_service.go

@@ -95,7 +95,7 @@ func (s SecretService) Get(ctx context.Context, projName tenant.ProjectName, nam
 		return nil, err
 	}
 
-	return tenant.NewPlainTextSecret(name, string(cleartext))
+	return tenant.NewPlainTextSecret(secretName.String(), string(cleartext))
 }
 
 func (s SecretService) GetAll(ctx context.Context, projName tenant.ProjectName, namespaceName string) ([]*tenant.PlainTextSecret, error) {

core/tenant/service/secret_service_test.go

@@ -113,26 +113,18 @@ func TestSecretService(t *testing.T) {
 		})
 	})
 	t.Run("Get", func(t *testing.T) {
-		t.Run("returns error when secret name is empty", func(t *testing.T) {
-			secretRepo := new(secretRepo)
-
-			secretService := service.NewSecretService(key, secretRepo, logger)
-			_, err := secretService.Get(ctx, projectName, nsName, "")
-			assert.NotNil(t, err)
-			assert.EqualError(t, err, "invalid argument for entity secret: secret name is not valid")
-		})
+		sn, err := tenant.SecretNameFrom("name")
+		assert.Nil(t, err)
 		t.Run("returns error when project name is invalid", func(t *testing.T) {
 			secretRepo := new(secretRepo)
-
 			secretService := service.NewSecretService(key, secretRepo, logger)
+
 			_, err := secretService.Get(ctx, "", nsName, "name")
+
 			assert.NotNil(t, err)
 			assert.EqualError(t, err, "invalid argument for entity secret: tenant is not valid")
 		})
 		t.Run("returns error when repo returns error", func(t *testing.T) {
-			sn, err := tenant.SecretNameFrom("name")
-			assert.Nil(t, err)
-
 			secretRepo := new(secretRepo)
 			secretRepo.On("Get", ctx, projectName, nsName, sn).Return(nil, errors.New("error in get"))
 			defer secretRepo.AssertExpectations(t)
@@ -143,9 +135,6 @@ func TestSecretService(t *testing.T) {
 			assert.EqualError(t, err, "error in get")
 		})
 		t.Run("returns error when not able to decode", func(t *testing.T) {
-			sn, err := tenant.SecretNameFrom("name")
-			assert.Nil(t, err)
-
 			secretRepo := new(secretRepo)
 			secretRepo.On("Get", ctx, projectName, nsName, sn).Return(&invalidSecret, nil)
 			defer secretRepo.AssertExpectations(t)
@@ -163,17 +152,14 @@ func TestSecretService(t *testing.T) {
 			sec, err := tenant.NewSecret("name", string(encodedArr), projectName, nsName)
 			assert.Nil(t, err)
 
-			sn, err := tenant.SecretNameFrom("name")
-			assert.Nil(t, err)
-
 			secretRepo := new(secretRepo)
 			secretRepo.On("Get", ctx, projectName, nsName, sn).Return(sec, nil)
 			defer secretRepo.AssertExpectations(t)
 
 			secretService := service.NewSecretService(key, secretRepo, logger)
 			s, err := secretService.Get(ctx, projectName, nsName, "name")
 			assert.Nil(t, err)
-			assert.Equal(t, "name", s.Name().String())
+			assert.Equal(t, "NAME", s.Name().String())
 			assert.Equal(t, "value", s.Value())
 		})
 	})
@@ -220,7 +206,7 @@ func TestSecretService(t *testing.T) {
 			secretService := service.NewSecretService(key, secretRepo, logger)
 			s, err := secretService.GetAll(ctx, projectName, nsName)
 			assert.Nil(t, err)
-			assert.Equal(t, "name", s[0].Name().String())
+			assert.Equal(t, "NAME", s[0].Name().String())
 			assert.Equal(t, "value", s[0].Value())
 		})
 	})
@@ -255,7 +241,7 @@ func TestSecretService(t *testing.T) {
 			defer secretRepo.AssertExpectations(t)
 
 			secretService := service.NewSecretService(key, secretRepo, logger)
-			err = secretService.Delete(ctx, projectName, nsName, "name")
+			err = secretService.Delete(ctx, projectName, nsName, sn)
 			assert.Nil(t, err)
 		})
 	})

core/tenant/service/tenant_service_test.go

@@ -157,7 +157,6 @@ func TestTenantService(t *testing.T) {
 			secretsGetter := new(secretGetter)
 			tenantService := service.NewTenantService(nil, nil, secretsGetter, logger)
 			invalidTenant := tenant.Tenant{}
-
 			_, err := tenantService.GetSecret(ctx, invalidTenant, "secret")
 			assert.NotNil(t, err)
 			assert.EqualError(t, err, "invalid argument for entity tenant: tenant is invalid")
@@ -167,10 +166,9 @@ func TestTenantService(t *testing.T) {
 			secretsGetter := new(secretGetter)
 			secretsGetter.On("Get", ctx, proj.Name(), ns.Name().String(), "secret_name").Return(pts, nil)
 			defer secretsGetter.AssertExpectations(t)
-
 			tenantService := service.NewTenantService(nil, nil, secretsGetter, logger)
 
-			secret, err := tenantService.GetSecret(ctx, tnnt, pts.Name().String())
+			secret, err := tenantService.GetSecret(ctx, tnnt, "secret_name")
 			assert.Nil(t, err)
 			assert.Equal(t, "secret_value", secret.Value())
 		})

core/tenant/tenant.go

@@ -58,7 +58,7 @@ func NewTenantDetails(proj *Project, namespace *Namespace, secrets PlainTextSecr
 	return &WithDetails{
 		project:    *proj,
 		namespace:  *namespace,
-		secretsMap: secrets.ToMap(),
+		secretsMap: secrets.ToSecretMap().ToMap(),
 	}, nil
 }
 

docs/docs/rfcs/20211002_secret_management.md

@@ -15,6 +15,8 @@ To keep string literals as secret, it is a requirement Optimus keep them encrypt
 
 Optimus has two sets of secrets, user managed secrets & others which are needed for server operations. Each of server managed secrets should be prefixed by `_OPTIMUS_<key name>` and will not be allowed to be used by users in the job spec. Optimus should also disallow anyone using this prefix to register their secrets. The secrets can be namespaced by optimus namespace or at project level, and will be accessible accordingly. Secret names should be maintained unique across a project.
 
+All secret names within Optimus systems are considered case insensitive, treating different letter cases as identical. Furthermore, to promote consistency and ease of management, all secret names are uniformly stored in uppercase letters, regardless of their original format. The implementation of this policy is essential to enhance security, minimize potential inconsistencies, and facilitate seamless secret retrieval and utilization.
+
 #### Using secrets
 
 Secrets can be used as part of the job spec config using macros with their names. This will work as aliasing the secret to be used in containers. Only the secrets created at project & namespace the job belongs to can be referenced. So, for the plugin writers any secret that plugin needs can be accessed through environment variables defined in the job spec or can get the secrets by defining in any assets.

internal/store/postgres/migrations/000056_capitalize_secret_name.down.sql

@@ -0,0 +1 @@
+-- the secrets key name capitalization can not be undone

internal/store/postgres/migrations/000056_capitalize_secret_name.up.sql

@@ -0,0 +1 @@
+UPDATE SECRET SET NAME = UPPER(NAME);

internal/store/postgres/tenant/secret_repository_test.go

@@ -156,7 +156,7 @@ func TestPostgresSecretRepository(t *testing.T) {
 
 			err = repo.Update(ctx, updatedSecret)
 			assert.NotNil(t, err)
-			assert.EqualError(t, err, "not found for entity secret: unable to update, secret not found for secret_name")
+			assert.EqualError(t, err, "not found for entity secret: unable to update, secret not found for SECRET_NAME")
 		})
 	})
 	t.Run("Get", func(t *testing.T) {
@@ -171,7 +171,7 @@ func TestPostgresSecretRepository(t *testing.T) {
 
 			_, err = repo.Get(ctx, proj.Name(), namespace.Name().String(), validSecret.Name())
 			assert.NotNil(t, err)
-			assert.EqualError(t, err, "not found for entity secret: no record for secret_name")
+			assert.EqualError(t, err, "not found for entity secret: no record for SECRET_NAME")
 		})
 		t.Run("returns the secret when present", func(t *testing.T) {
 			db := dbSetup()
@@ -208,10 +208,12 @@ func TestPostgresSecretRepository(t *testing.T) {
 			err = repo.Save(ctx, secret2)
 			assert.Nil(t, err)
 
-			secret, err := repo.Get(ctx, proj.Name(), namespace.Name().String(), "secret_name3")
+			sn, err := tenant.SecretNameFrom("secret_name3")
+			assert.Nil(t, err)
+			secret, err := repo.Get(ctx, proj.Name(), namespace.Name().String(), sn)
 			assert.NoError(t, err)
 			assert.NotNil(t, secret)
-			assert.Equal(t, "secret_name3", secret.Name().String())
+			assert.Equal(t, "SECRET_NAME3", secret.Name().String())
 		})
 	})
 	t.Run("GetAll", func(t *testing.T) {
@@ -327,7 +329,7 @@ func TestPostgresSecretRepository(t *testing.T) {
 
 			err = repo.Delete(ctx, proj.Name(), namespace.Name().String(), validSecret.Name())
 			assert.NotNil(t, err)
-			assert.EqualError(t, err, "not found for entity secret: secret to delete not found secret_name")
+			assert.EqualError(t, err, "not found for entity secret: secret to delete not found SECRET_NAME")
 		})
 	})
 	t.Run("GetSecretsInfo", func(t *testing.T) {