Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

SSO: fix settings merge for SAML fields #86970

Merged
merged 3 commits into from Apr 30, 2024
Merged

SSO: fix settings merge for SAML fields #86970

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
51eb42a
fix sso settings merge for saml fields
dmihai Apr 26, 2024
e4f680d
change func name
dmihai Apr 26, 2024
593f123
Merge branch 'main' into dmihai/sso-merge-saml
dmihai Apr 26, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
18 changes: 17 additions & 1 deletion pkg/services/ssosettings/ssosettingsimpl/service.go
View file
Open in desktop
Expand Up @@ -470,7 +470,9 @@ func mergeSettings(storedSettings, systemSettings map[string]any) map[string]any

for k, v := range systemSettings {
if _, ok := settings[k]; !ok {
settings[k] = v
if isMergingAllowed(k) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to be extra defensive, should we also add a check for making sure that the isMergingAllowed is only applied to SAML settings?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should keep the SSO settings service as generic as possible without introducing too much logic specific to individual providers. Also at some point maybe we want to use this new functionality for Oauth as well. Client ID and client secret are good candidates there. I don't think we would ever want to merge client ID from one source (for example .ini file) with the client secret from a different source (such as SSO settings).

Copy link
Contributor

@kalleep kalleep Apr 29, 2024 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with @dmihai that we for sure should keep this generic and try to not add provider specific things here. Another way to do this would be to extend the provider interface with this functionality, or add an optional interface that providers can implement. That way we can make sure we don't block for other provider in case of key collision.

settings[k] = v
}
} else if isURL(k) && isEmptyString(settings[k]) {
// Overwrite all URL settings from the DB containing an empty string with their value
// from the system settings. This fixes an issue with empty auth_url, api_url and token_url
Expand All @@ -483,6 +485,20 @@ func mergeSettings(storedSettings, systemSettings map[string]any) map[string]any
return settings
}

// isMergingAllowed returns true if the field provided can be merged from the system settings.
// It won't allow SAML fields that are part of a group of settings to be merged from system settings
// because the DB settings already contain one valid setting from each group.
func isMergingAllowed(fieldName string) bool {
forbiddenMergePatterns := []string{"certificate", "private_key", "idp_metadata"}

for _, v := range forbiddenMergePatterns {
if strings.Contains(strings.ToLower(fieldName), strings.ToLower(v)) {
return false
}
}
return true
}

// 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{}
Expand Down
85 changes: 67 additions & 18 deletions pkg/services/ssosettings/ssosettingsimpl/service_test.go
View file
Open in desktop
Expand Up @@ -40,13 +40,15 @@ func TestService_GetForProvider(t *testing.T) {
t.Parallel()

testCases := []struct {
name string
setup func(env testEnv)
want *models.SSOSettings
wantErr bool
name string
provider string
setup func(env testEnv)
want *models.SSOSettings
wantErr bool
}{
{
name: "should return successfully",
name: "should return successfully",
provider: "github",
setup: func(env testEnv) {
env.store.ExpectedSSOSetting = &models.SSOSettings{
Provider: "github",
Expand All @@ -72,13 +74,15 @@ func TestService_GetForProvider(t *testing.T) {
wantErr: false,
},
{
name: "should return error if store returns an error different than not found",
setup: func(env testEnv) { env.store.ExpectedError = fmt.Errorf("error") },
want: nil,
wantErr: true,
name: "should return error if store returns an error different than not found",
provider: "github",
setup: func(env testEnv) { env.store.ExpectedError = fmt.Errorf("error") },
want: nil,
wantErr: true,
},
{
name: "should fallback to the system settings if store returns not found",
name: "should fallback to the system settings if store returns not found",
provider: "github",
setup: func(env testEnv) {
env.store.ExpectedError = ssosettings.ErrNotFound
env.fallbackStrategy.ExpectedIsMatch = true
Expand All @@ -99,7 +103,8 @@ func TestService_GetForProvider(t *testing.T) {
wantErr: false,
},
{
name: "should return error if the fallback strategy was not found",
name: "should return error if the fallback strategy was not found",
provider: "github",
setup: func(env testEnv) {
env.store.ExpectedError = ssosettings.ErrNotFound
env.fallbackStrategy.ExpectedIsMatch = false
Expand All @@ -108,7 +113,8 @@ func TestService_GetForProvider(t *testing.T) {
wantErr: true,
},
{
name: "should return error if fallback strategy returns error",
name: "should return error if fallback strategy returns error",
provider: "github",
setup: func(env testEnv) {
env.store.ExpectedError = ssosettings.ErrNotFound
env.fallbackStrategy.ExpectedIsMatch = true
Expand All @@ -118,7 +124,8 @@ func TestService_GetForProvider(t *testing.T) {
wantErr: true,
},
{
name: "should decrypt secrets if data is coming from store",
name: "should decrypt secrets if data is coming from store",
provider: "github",
setup: func(env testEnv) {
env.store.ExpectedSSOSetting = &models.SSOSettings{
Provider: "github",
Expand Down Expand Up @@ -152,7 +159,8 @@ func TestService_GetForProvider(t *testing.T) {
wantErr: false,
},
{
name: "should not decrypt secrets if data is coming from the fallback strategy",
name: "should not decrypt secrets if data is coming from the fallback strategy",
provider: "github",
setup: func(env testEnv) {
env.store.ExpectedError = ssosettings.ErrNotFound
env.fallbackStrategy.ExpectedIsMatch = true
Expand All @@ -176,7 +184,8 @@ func TestService_GetForProvider(t *testing.T) {
wantErr: false,
},
{
name: "should return an error if the data in the store is invalid",
name: "should return an error if the data in the store is invalid",
provider: "github",
setup: func(env testEnv) {
env.store.ExpectedSSOSetting = &models.SSOSettings{
Provider: "github",
Expand All @@ -196,7 +205,8 @@ func TestService_GetForProvider(t *testing.T) {
wantErr: true,
},
{
name: "correctly merge the DB and system settings",
name: "correctly merge URLs from the DB and system settings",
provider: "github",
setup: func(env testEnv) {
env.store.ExpectedSSOSetting = &models.SSOSettings{
Provider: "github",
Expand Down Expand Up @@ -231,6 +241,45 @@ func TestService_GetForProvider(t *testing.T) {
},
wantErr: false,
},
{
name: "correctly merge group of settings for SAML",
provider: "saml",
setup: func(env testEnv) {
env.store.ExpectedSSOSetting = &models.SSOSettings{
Provider: "saml",
Settings: map[string]any{
"certificate": base64.RawStdEncoding.EncodeToString([]byte("valid-certificate")),
"private_key_path": base64.RawStdEncoding.EncodeToString([]byte("path/to/private/key")),
"idp_metadata_url": "https://idp-metadata.com",
},
Source: models.DB,
}
env.fallbackStrategy.ExpectedIsMatch = true
env.fallbackStrategy.ExpectedConfigs = map[string]map[string]any{
"saml": {
"name": "test-settings",
"certificate_path": "path/to/certificate",
"private_key": "this-is-a-valid-private-key",
"idp_metadata_path": "path/to/metadata",
"max_issue_delay": "1h",
},
}
env.secrets.On("Decrypt", mock.Anything, []byte("valid-certificate"), mock.Anything).Return([]byte("decrypted-valid-certificate"), nil).Once()
env.secrets.On("Decrypt", mock.Anything, []byte("path/to/private/key"), mock.Anything).Return([]byte("decrypted/path/to/private/key"), nil).Once()
},
want: &models.SSOSettings{
Provider: "saml",
Settings: map[string]any{
"name": "test-settings",
"certificate": "decrypted-valid-certificate",
"private_key_path": "decrypted/path/to/private/key",
"idp_metadata_url": "https://idp-metadata.com",
"max_issue_delay": "1h",
},
Source: models.DB,
},
wantErr: false,
},
}

for _, tc := range testCases {
Expand All @@ -241,12 +290,12 @@ func TestService_GetForProvider(t *testing.T) {
t.Run(tc.name, func(t *testing.T) {
t.Parallel()

env := setupTestEnv(t, false, false, false)
env := setupTestEnv(t, true, false, true)
if tc.setup != nil {
tc.setup(env)
}

actual, err := env.service.GetForProvider(context.Background(), "github")
actual, err := env.service.GetForProvider(context.Background(), tc.provider)

if tc.wantErr {
require.Error(t, err)
Expand Down