diff --git a/core/scheduler/service/notification_service.go b/core/scheduler/service/notification_service.go index d4860065ad..c3ad423f4e 100644 --- a/core/scheduler/service/notification_service.go +++ b/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 { diff --git a/core/tenant/handler/v1beta1/secret_test.go b/core/tenant/handler/v1beta1/secret_test.go index 22840a7b3a..ee6227fe72 100644 --- a/core/tenant/handler/v1beta1/secret_test.go +++ b/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) diff --git a/core/tenant/secret.go b/core/tenant/secret.go index 234f2ab57b..a6bd93e387 100644 --- a/core/tenant/secret.go +++ b/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,7 +58,9 @@ 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() @@ -62,6 +68,21 @@ func (p PlainTextSecrets) ToMap() map[string]string { 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 diff --git a/core/tenant/secret_test.go b/core/tenant/secret_test.go index 572368f41b..4f1d66b4b0 100644 --- a/core/tenant/secret_test.go +++ b/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()) diff --git a/core/tenant/service/secret_service.go b/core/tenant/service/secret_service.go index b2a1714cc9..a2da352b75 100644 --- a/core/tenant/service/secret_service.go +++ b/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) { diff --git a/core/tenant/service/secret_service_test.go b/core/tenant/service/secret_service_test.go index 9fc63d993b..e97799b60b 100644 --- a/core/tenant/service/secret_service_test.go +++ b/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,9 +152,6 @@ 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) @@ -173,7 +159,7 @@ func TestSecretService(t *testing.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) }) }) diff --git a/core/tenant/service/tenant_service_test.go b/core/tenant/service/tenant_service_test.go index 4f6224c0ed..44d417219f 100644 --- a/core/tenant/service/tenant_service_test.go +++ b/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()) }) diff --git a/core/tenant/tenant.go b/core/tenant/tenant.go index 133f9b0fbd..823257778b 100644 --- a/core/tenant/tenant.go +++ b/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 } diff --git a/docs/docs/rfcs/20211002_secret_management.md b/docs/docs/rfcs/20211002_secret_management.md index 1b83339f9b..a58082ba50 100644 --- a/docs/docs/rfcs/20211002_secret_management.md +++ b/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_` 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. diff --git a/internal/store/postgres/migrations/000056_capitalize_secret_name.down.sql b/internal/store/postgres/migrations/000056_capitalize_secret_name.down.sql new file mode 100644 index 0000000000..4c71696503 --- /dev/null +++ b/internal/store/postgres/migrations/000056_capitalize_secret_name.down.sql @@ -0,0 +1 @@ +-- the secrets key name capitalization can not be undone \ No newline at end of file diff --git a/internal/store/postgres/migrations/000056_capitalize_secret_name.up.sql b/internal/store/postgres/migrations/000056_capitalize_secret_name.up.sql new file mode 100644 index 0000000000..0de4f4e1b7 --- /dev/null +++ b/internal/store/postgres/migrations/000056_capitalize_secret_name.up.sql @@ -0,0 +1 @@ +UPDATE SECRET SET NAME = UPPER(NAME); \ No newline at end of file diff --git a/internal/store/postgres/tenant/secret_repository_test.go b/internal/store/postgres/tenant/secret_repository_test.go index 5bbe9a4e2a..1c3f4292e7 100644 --- a/internal/store/postgres/tenant/secret_repository_test.go +++ b/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) {