From bf153294928ca4fcc06f152b58f0b2591a9d2f05 Mon Sep 17 00:00:00 2001 From: Mihai Doarna Date: Tue, 23 Apr 2024 16:50:16 +0300 Subject: [PATCH] SSO: run the validation on upsert with all secrets in settings (#86579) * run the validation on upsert with all secrets in settings * rename social to reloadable --- .../ssosettings/ssosettingsimpl/service.go | 54 ++++++----- .../ssosettingsimpl/service_test.go | 89 ++++++++++++++++++- 2 files changed, 116 insertions(+), 27 deletions(-) diff --git a/pkg/services/ssosettings/ssosettingsimpl/service.go b/pkg/services/ssosettings/ssosettingsimpl/service.go index 38862651e74..b3edf2910a0 100644 --- a/pkg/services/ssosettings/ssosettingsimpl/service.go +++ b/pkg/services/ssosettings/ssosettingsimpl/service.go @@ -192,24 +192,28 @@ func (s *Service) Upsert(ctx context.Context, settings *models.SSOSettings, requ return ssosettings.ErrNotConfigurable } - social, ok := s.reloadables[settings.Provider] + reloadable, ok := s.reloadables[settings.Provider] if !ok { return ssosettings.ErrInvalidProvider.Errorf("provider %s not found in reloadables", settings.Provider) } - err := social.Validate(ctx, *settings, requester) - if err != nil { - return err - } - storedSettings, err := s.GetForProvider(ctx, settings.Provider) if err != nil { return err } - secrets := collectSecrets(settings, storedSettings) + settingsWithSecrets, err := mergeSecrets(settings.Settings, storedSettings.Settings) + if err != nil { + return err + } + settings.Settings = settingsWithSecrets - settings.Settings, err = s.encryptSecrets(ctx, settings.Settings, storedSettings.Settings) + err = reloadable.Validate(ctx, *settings, requester) + if err != nil { + return err + } + + settings.Settings, err = s.encryptSecrets(ctx, settings.Settings) if err != nil { return err } @@ -221,9 +225,9 @@ func (s *Service) Upsert(ctx context.Context, settings *models.SSOSettings, requ // make a copy of current settings for reload operation and apply overrides reloadSettings := *settings - reloadSettings.Settings = overrideMaps(storedSettings.Settings, settings.Settings, secrets) + reloadSettings.Settings = overrideMaps(storedSettings.Settings, settingsWithSecrets) - go s.reload(social, settings.Provider, reloadSettings) + go s.reload(reloadable, settings.Provider, reloadSettings) return nil } @@ -317,7 +321,7 @@ func (s *Service) getFallbackStrategyFor(provider string) (ssosettings.FallbackS return nil, false } -func (s *Service) encryptSecrets(ctx context.Context, settings, storedSettings map[string]any) (map[string]any, error) { +func (s *Service) encryptSecrets(ctx context.Context, settings map[string]any) (map[string]any, error) { result := make(map[string]any) for k, v := range settings { if isSecret(k) && v != "" { @@ -326,10 +330,6 @@ func (s *Service) encryptSecrets(ctx context.Context, settings, storedSettings m return result, fmt.Errorf("failed to encrypt %s setting because it is not a string: %v", k, v) } - if !isNewSecretValue(strValue) { - strValue = storedSettings[k].(string) - } - encryptedSecret, err := s.secrets.Encrypt(ctx, []byte(strValue), secrets.WithoutScope()) if err != nil { return result, err @@ -483,20 +483,26 @@ func mergeSettings(storedSettings, systemSettings map[string]any) map[string]any return settings } -// collectSecrets collects all the secrets from the request and the currently stored settings -// and returns a new map -func collectSecrets(settings *models.SSOSettings, storedSettings *models.SSOSettings) map[string]any { - secrets := map[string]any{} - for k, v := range settings.Settings { +// mergeSecrets returns a new map with the current value for secrets that have not been updated +func mergeSecrets(settings map[string]any, storedSettings map[string]any) (map[string]any, error) { + settingsWithSecrets := map[string]any{} + for k, v := range settings { if isSecret(k) { - if isNewSecretValue(v.(string)) { - secrets[k] = v.(string) // use the new value + strValue, ok := v.(string) + if !ok { + return nil, fmt.Errorf("secret value is not a string") + } + + if isNewSecretValue(strValue) { + settingsWithSecrets[k] = strValue // use the new value continue } - secrets[k] = storedSettings.Settings[k] // keep the currently stored value + settingsWithSecrets[k] = storedSettings[k] // keep the currently stored value + } else { + settingsWithSecrets[k] = v } } - return secrets + return settingsWithSecrets, nil } func overrideMaps(maps ...map[string]any) map[string]any { diff --git a/pkg/services/ssosettings/ssosettingsimpl/service_test.go b/pkg/services/ssosettings/ssosettingsimpl/service_test.go index 52c2ae69027..0c6fde35b18 100644 --- a/pkg/services/ssosettings/ssosettingsimpl/service_test.go +++ b/pkg/services/ssosettings/ssosettingsimpl/service_test.go @@ -979,6 +979,29 @@ func TestService_Upsert(t *testing.T) { require.Error(t, err) }) + t.Run("returns error if a secret does not have the type string", func(t *testing.T) { + t.Parallel() + + env := setupTestEnv(t, false, false, nil) + + provider := social.OktaProviderName + settings := models.SSOSettings{ + Provider: provider, + Settings: map[string]any{ + "client_id": "client-id", + "client_secret": 123, + "enabled": true, + }, + IsDeleted: false, + } + + reloadable := ssosettingstests.NewMockReloadable(t) + env.reloadables[provider] = reloadable + + err := env.service.Upsert(context.Background(), &settings, &user.SignedInUser{}) + require.Error(t, err) + }) + t.Run("returns error if secrets encryption failed", func(t *testing.T) { t.Parallel() @@ -1027,8 +1050,15 @@ func TestService_Upsert(t *testing.T) { }, } + expected := settings + expected.Settings = make(map[string]any) + for key, value := range settings.Settings { + expected.Settings[key] = value + } + expected.Settings["client_secret"] = "encrypted-client-secret" + reloadable := ssosettingstests.NewMockReloadable(t) - reloadable.On("Validate", mock.Anything, settings, mock.Anything).Return(nil) + reloadable.On("Validate", mock.Anything, expected, mock.Anything).Return(nil) reloadable.On("Reload", mock.Anything, mock.Anything).Return(nil).Maybe() env.reloadables[provider] = reloadable env.secrets.On("Decrypt", mock.Anything, []byte("current-client-secret"), mock.Anything).Return([]byte("encrypted-client-secret"), nil).Once() @@ -1037,8 +1067,61 @@ func TestService_Upsert(t *testing.T) { err := env.service.Upsert(context.Background(), &settings, &user.SignedInUser{}) require.NoError(t, err) - settings.Settings["client_secret"] = base64.RawStdEncoding.EncodeToString([]byte("current-client-secret")) - require.EqualValues(t, settings, env.store.ActualSSOSettings) + expected.Settings["client_secret"] = base64.RawStdEncoding.EncodeToString([]byte("current-client-secret")) + require.EqualValues(t, expected, env.store.ActualSSOSettings) + }) + + t.Run("run validation with all new and current secrets available in settings", func(t *testing.T) { + t.Parallel() + + env := setupTestEnv(t, false, false, nil) + + provider := social.AzureADProviderName + settings := models.SSOSettings{ + Provider: provider, + Settings: map[string]any{ + "client_secret": setting.RedactedPassword, + "private_key": setting.RedactedPassword, + "certificate": "new-certificate", + }, + IsDeleted: false, + } + + env.store.ExpectedSSOSetting = &models.SSOSettings{ + Provider: provider, + Settings: map[string]any{ + "client_secret": base64.RawStdEncoding.EncodeToString([]byte("encrypted-current-client-secret")), + "private_key": base64.RawStdEncoding.EncodeToString([]byte("encrypted-current-private-key")), + "certificate": base64.RawStdEncoding.EncodeToString([]byte("encrypted-current-certificate")), + }, + } + + expected := settings + expected.Settings = make(map[string]any) + for key, value := range settings.Settings { + expected.Settings[key] = value + } + expected.Settings["client_secret"] = "current-client-secret" + expected.Settings["private_key"] = "current-private-key" + + reloadable := ssosettingstests.NewMockReloadable(t) + reloadable.On("Validate", mock.Anything, expected, mock.Anything).Return(nil) + reloadable.On("Reload", mock.Anything, mock.Anything).Return(nil).Maybe() + env.reloadables[provider] = reloadable + env.secrets.On("Decrypt", mock.Anything, []byte("encrypted-current-client-secret"), mock.Anything).Return([]byte("current-client-secret"), nil).Once() + env.secrets.On("Decrypt", mock.Anything, []byte("encrypted-current-private-key"), mock.Anything).Return([]byte("current-private-key"), nil).Once() + env.secrets.On("Decrypt", mock.Anything, []byte("encrypted-current-certificate"), mock.Anything).Return([]byte("current-certificate"), nil).Once() + env.secrets.On("Encrypt", mock.Anything, []byte("current-client-secret"), mock.Anything).Return([]byte("encrypted-current-client-secret"), nil).Once() + env.secrets.On("Encrypt", mock.Anything, []byte("current-private-key"), mock.Anything).Return([]byte("encrypted-current-private-key"), nil).Once() + env.secrets.On("Encrypt", mock.Anything, []byte("new-certificate"), mock.Anything).Return([]byte("encrypted-new-certificate"), nil).Once() + + err := env.service.Upsert(context.Background(), &settings, &user.SignedInUser{}) + require.NoError(t, err) + + expected.Settings["client_secret"] = base64.RawStdEncoding.EncodeToString([]byte("encrypted-current-client-secret")) + expected.Settings["private_key"] = base64.RawStdEncoding.EncodeToString([]byte("encrypted-current-private-key")) + expected.Settings["certificate"] = base64.RawStdEncoding.EncodeToString([]byte("encrypted-new-certificate")) + require.EqualValues(t, expected, env.store.ActualSSOSettings) }) t.Run("returns error if store failed to upsert settings", func(t *testing.T) {