From eeadb7e771f18fa62dc6b5d0cb757666cde43aeb Mon Sep 17 00:00:00 2001 From: xavi <114113189+volcanonoodle@users.noreply.github.com> Date: Thu, 13 Feb 2025 18:02:54 +0100 Subject: [PATCH] IAM: Log error when malformed json arrays are found in SSO configs (#99896) --- pkg/login/social/connectors/azuread_oauth.go | 20 +++++++++--- pkg/login/social/connectors/common.go | 25 ++++++++++++++- pkg/login/social/connectors/generic_oauth.go | 31 ++++++++++++++++--- pkg/login/social/connectors/github_oauth.go | 30 ++++++++++++++---- pkg/login/social/connectors/gitlab_oauth.go | 2 +- pkg/login/social/connectors/google_oauth.go | 2 +- .../social/connectors/grafana_com_oauth.go | 20 +++++++++--- pkg/login/social/connectors/okta_oauth.go | 2 +- pkg/login/social/socialimpl/service.go | 4 +-- pkg/util/strings.go | 17 +++++++--- public/app/features/auth-config/utils/data.ts | 5 ++- 11 files changed, 128 insertions(+), 30 deletions(-) diff --git a/pkg/login/social/connectors/azuread_oauth.go b/pkg/login/social/connectors/azuread_oauth.go index 0f4a72ed5a7..8ae1f3380ab 100644 --- a/pkg/login/social/connectors/azuread_oauth.go +++ b/pkg/login/social/connectors/azuread_oauth.go @@ -88,10 +88,17 @@ type keySetJWKS struct { } func NewAzureADProvider(info *social.OAuthInfo, cfg *setting.Cfg, orgRoleMapper *OrgRoleMapper, ssoSettings ssosettings.Service, features featuremgmt.FeatureToggles, cache remotecache.CacheStorage) *SocialAzureAD { + s := newSocialBase(social.AzureADProviderName, orgRoleMapper, info, features, cfg) + + allowedOrganizations, err := util.SplitStringWithError(info.Extra[allowedOrganizationsKey]) + if err != nil { + s.log.Error("Invalid auth configuration setting", "config", allowedOrganizationsKey, "provider", social.AzureADProviderName, "error", err) + } + provider := &SocialAzureAD{ - SocialBase: newSocialBase(social.AzureADProviderName, orgRoleMapper, info, features, cfg), + SocialBase: s, cache: cache, - allowedOrganizations: util.SplitString(info.Extra[allowedOrganizationsKey]), + allowedOrganizations: allowedOrganizations, forceUseGraphAPI: MustBool(info.Extra[forceUseGraphAPIKey], ExtraAzureADSettingKeys[forceUseGraphAPIKey].DefaultValue.(bool)), } @@ -236,7 +243,7 @@ func (s *SocialAzureAD) managedIdentityCallback(ctx context.Context) (string, er } func (s *SocialAzureAD) Reload(ctx context.Context, settings ssoModels.SSOSettings) error { - newInfo, err := CreateOAuthInfoFromKeyValues(settings.Settings) + newInfo, err := CreateOAuthInfoFromKeyValuesWithLogging(s.log, social.AzureADProviderName, settings.Settings) if err != nil { return ssosettings.ErrInvalidSettings.Errorf("SSO settings map cannot be converted to OAuthInfo: %v", err) } @@ -250,7 +257,12 @@ func (s *SocialAzureAD) Reload(ctx context.Context, settings ssoModels.SSOSettin appendUniqueScope(s.Config, social.OfflineAccessScope) } - s.allowedOrganizations = util.SplitString(newInfo.Extra[allowedOrganizationsKey]) + allowedOrganizations, err := util.SplitStringWithError(newInfo.Extra[allowedOrganizationsKey]) + if err != nil { + s.log.Error("Invalid auth configuration setting", "config", allowedOrganizationsKey, "provider", social.AzureADProviderName, "error", err) + } + + s.allowedOrganizations = allowedOrganizations s.forceUseGraphAPI = MustBool(newInfo.Extra[forceUseGraphAPIKey], false) return nil diff --git a/pkg/login/social/connectors/common.go b/pkg/login/social/connectors/common.go index 255b96742e8..15fc590f359 100644 --- a/pkg/login/social/connectors/common.go +++ b/pkg/login/social/connectors/common.go @@ -2,6 +2,7 @@ package connectors import ( "context" + "errors" "fmt" "io" "net/http" @@ -13,6 +14,7 @@ import ( "github.com/mitchellh/mapstructure" "golang.org/x/oauth2" + "github.com/grafana/grafana/pkg/infra/log" "github.com/grafana/grafana/pkg/login/social" "github.com/grafana/grafana/pkg/setting" "github.com/grafana/grafana/pkg/util" @@ -165,9 +167,25 @@ func MustBool(value any, defaultValue bool) bool { return result } +// CreateOAuthInfoFromKeyValuesWithLogging creates an OAuthInfo struct from a map[string]any using mapstructure +// it puts all extra key values into OAuthInfo's Extra map. +// It logs as errors any parsing errors that are not critical +func CreateOAuthInfoFromKeyValuesWithLogging(l log.Logger, provider string, settingsKV map[string]any) (*social.OAuthInfo, error) { + parsingWarns := []error{} + info, err := createOAuthInfoFromKeyValues(settingsKV, &parsingWarns) + if len(parsingWarns) > 0 { + l.Error("Invalid auth configuration setting", "error", errors.Join(parsingWarns...), "provider", provider) + } + return info, err +} + // CreateOAuthInfoFromKeyValues creates an OAuthInfo struct from a map[string]any using mapstructure // it puts all extra key values into OAuthInfo's Extra map func CreateOAuthInfoFromKeyValues(settingsKV map[string]any) (*social.OAuthInfo, error) { + return createOAuthInfoFromKeyValues(settingsKV, nil) +} + +func createOAuthInfoFromKeyValues(settingsKV map[string]any, parsingWarns *[]error) (*social.OAuthInfo, error) { emptyStrToSliceDecodeHook := func(from reflect.Type, to reflect.Type, data any) (any, error) { if from.Kind() == reflect.String && to.Kind() == reflect.Slice { strData, ok := data.(string) @@ -178,7 +196,12 @@ func CreateOAuthInfoFromKeyValues(settingsKV map[string]any) (*social.OAuthInfo, if strData == "" { return []string{}, nil } - return util.SplitString(strData), nil + + splitStr, err := util.SplitStringWithError(strData) + if err != nil && parsingWarns != nil { + *parsingWarns = append(*parsingWarns, err) + } + return splitStr, nil } return data, nil } diff --git a/pkg/login/social/connectors/generic_oauth.go b/pkg/login/social/connectors/generic_oauth.go index eb4a32f8381..15989c0df93 100644 --- a/pkg/login/social/connectors/generic_oauth.go +++ b/pkg/login/social/connectors/generic_oauth.go @@ -53,6 +53,18 @@ type SocialGenericOAuth struct { } func NewGenericOAuthProvider(info *social.OAuthInfo, cfg *setting.Cfg, orgRoleMapper *OrgRoleMapper, ssoSettings ssosettings.Service, features featuremgmt.FeatureToggles) *SocialGenericOAuth { + s := newSocialBase(social.GenericOAuthProviderName, orgRoleMapper, info, features, cfg) + + teamIds, err := util.SplitStringWithError(info.Extra[teamIdsKey]) + if err != nil { + s.log.Error("Invalid auth configuration setting", "config", teamIdsKey, "provider", social.GenericOAuthProviderName, "error", err) + } + + allowedOrganizations, err := util.SplitStringWithError(info.Extra[allowedOrganizationsKey]) + if err != nil { + s.log.Error("Invalid auth configuration setting", "config", allowedOrganizationsKey, "provider", social.GenericOAuthProviderName, "error", err) + } + provider := &SocialGenericOAuth{ SocialBase: newSocialBase(social.GenericOAuthProviderName, orgRoleMapper, info, features, cfg), teamsUrl: info.TeamsUrl, @@ -63,8 +75,8 @@ func NewGenericOAuthProvider(info *social.OAuthInfo, cfg *setting.Cfg, orgRoleMa loginAttributePath: info.Extra[loginAttributePathKey], idTokenAttributeName: info.Extra[idTokenAttributeNameKey], teamIdsAttributePath: info.TeamIdsAttributePath, - teamIds: util.SplitString(info.Extra[teamIdsKey]), - allowedOrganizations: util.SplitString(info.Extra[allowedOrganizationsKey]), + teamIds: teamIds, + allowedOrganizations: allowedOrganizations, } if features.IsEnabledGlobally(featuremgmt.FlagSsoSettingsApi) { @@ -118,7 +130,7 @@ func validateTeamsUrlWhenNotEmpty(info *social.OAuthInfo, requester identity.Req } func (s *SocialGenericOAuth) Reload(ctx context.Context, settings ssoModels.SSOSettings) error { - newInfo, err := CreateOAuthInfoFromKeyValues(settings.Settings) + newInfo, err := CreateOAuthInfoFromKeyValuesWithLogging(s.log, social.GenericOAuthProviderName, settings.Settings) if err != nil { return ssosettings.ErrInvalidSettings.Errorf("SSO settings map cannot be converted to OAuthInfo: %v", err) } @@ -128,6 +140,15 @@ func (s *SocialGenericOAuth) Reload(ctx context.Context, settings ssoModels.SSOS s.updateInfo(ctx, social.GenericOAuthProviderName, newInfo) + teamIds, err := util.SplitStringWithError(newInfo.Extra[teamIdsKey]) + if err != nil { + s.log.Error("Invalid auth configuration setting", "config", teamIdsKey, "provider", social.GenericOAuthProviderName, "error", err) + } + allowedOrganizations, err := util.SplitStringWithError(newInfo.Extra[allowedOrganizationsKey]) + if err != nil { + s.log.Error("Invalid auth configuration setting", "config", allowedOrganizationsKey, "provider", social.GenericOAuthProviderName, "error", err) + } + s.teamsUrl = newInfo.TeamsUrl s.emailAttributeName = newInfo.EmailAttributeName s.emailAttributePath = newInfo.EmailAttributePath @@ -136,8 +157,8 @@ func (s *SocialGenericOAuth) Reload(ctx context.Context, settings ssoModels.SSOS s.loginAttributePath = newInfo.Extra[loginAttributePathKey] s.idTokenAttributeName = newInfo.Extra[idTokenAttributeNameKey] s.teamIdsAttributePath = newInfo.TeamIdsAttributePath - s.teamIds = util.SplitString(newInfo.Extra[teamIdsKey]) - s.allowedOrganizations = util.SplitString(newInfo.Extra[allowedOrganizationsKey]) + s.teamIds = teamIds + s.allowedOrganizations = allowedOrganizations return nil } diff --git a/pkg/login/social/connectors/github_oauth.go b/pkg/login/social/connectors/github_oauth.go index 124b642f822..f5f0b43b3f3 100644 --- a/pkg/login/social/connectors/github_oauth.go +++ b/pkg/login/social/connectors/github_oauth.go @@ -62,13 +62,23 @@ var ( ) func NewGitHubProvider(info *social.OAuthInfo, cfg *setting.Cfg, orgRoleMapper *OrgRoleMapper, ssoSettings ssosettings.Service, features featuremgmt.FeatureToggles) *SocialGithub { - teamIdsSplitted := util.SplitString(info.Extra[teamIdsKey]) + s := newSocialBase(social.GitHubProviderName, orgRoleMapper, info, features, cfg) + + teamIdsSplitted, err := util.SplitStringWithError(info.Extra[teamIdsKey]) + if err != nil { + s.log.Error("Invalid auth configuration setting", "config", teamIdsKey, "provider", social.GitHubProviderName, "error", err) + } teamIds := mustInts(teamIdsSplitted) + allowedOrganizations, err := util.SplitStringWithError(info.Extra[allowedOrganizationsKey]) + if err != nil { + s.log.Error("Invalid auth configuration setting", "config", allowedOrganizationsKey, "provider", social.GitHubProviderName, "error", err) + } + provider := &SocialGithub{ - SocialBase: newSocialBase(social.GitHubProviderName, orgRoleMapper, info, features, cfg), + SocialBase: s, teamIds: teamIds, - allowedOrganizations: util.SplitString(info.Extra[allowedOrganizationsKey]), + allowedOrganizations: allowedOrganizations, } if len(teamIdsSplitted) != len(teamIds) { @@ -117,14 +127,22 @@ func teamIdsNumbersValidator(info *social.OAuthInfo, requester identity.Requeste } func (s *SocialGithub) Reload(ctx context.Context, settings ssoModels.SSOSettings) error { - newInfo, err := CreateOAuthInfoFromKeyValues(settings.Settings) + newInfo, err := CreateOAuthInfoFromKeyValuesWithLogging(s.log, social.GitHubProviderName, settings.Settings) if err != nil { return ssosettings.ErrInvalidSettings.Errorf("SSO settings map cannot be converted to OAuthInfo: %v", err) } - teamIdsSplitted := util.SplitString(newInfo.Extra[teamIdsKey]) + teamIdsSplitted, err := util.SplitStringWithError(newInfo.Extra[teamIdsKey]) + if err != nil { + s.log.Error("Invalid auth configuration setting", "config", teamIdsKey, "provider", social.GitHubProviderName, "error", err) + } teamIds := mustInts(teamIdsSplitted) + allowedOrganizations, err := util.SplitStringWithError(newInfo.Extra[allowedOrganizationsKey]) + if err != nil { + s.log.Error("Invalid auth configuration setting", "config", allowedOrganizationsKey, "provider", social.GitHubProviderName, "error", err) + } + if len(teamIdsSplitted) != len(teamIds) { s.log.Warn("Failed to parse team ids. Team ids must be a list of numbers.", "teamIds", teamIdsSplitted) } @@ -135,7 +153,7 @@ func (s *SocialGithub) Reload(ctx context.Context, settings ssoModels.SSOSetting s.updateInfo(ctx, social.GitHubProviderName, newInfo) s.teamIds = teamIds - s.allowedOrganizations = util.SplitString(newInfo.Extra[allowedOrganizationsKey]) + s.allowedOrganizations = allowedOrganizations return nil } diff --git a/pkg/login/social/connectors/gitlab_oauth.go b/pkg/login/social/connectors/gitlab_oauth.go index d51544dd7c7..7497c24d619 100644 --- a/pkg/login/social/connectors/gitlab_oauth.go +++ b/pkg/login/social/connectors/gitlab_oauth.go @@ -87,7 +87,7 @@ func (s *SocialGitlab) Validate(ctx context.Context, newSettings ssoModels.SSOSe } func (s *SocialGitlab) Reload(ctx context.Context, settings ssoModels.SSOSettings) error { - newInfo, err := CreateOAuthInfoFromKeyValues(settings.Settings) + newInfo, err := CreateOAuthInfoFromKeyValuesWithLogging(s.log, social.GitlabProviderName, settings.Settings) if err != nil { return ssosettings.ErrInvalidSettings.Errorf("SSO settings map cannot be converted to OAuthInfo: %v", err) } diff --git a/pkg/login/social/connectors/google_oauth.go b/pkg/login/social/connectors/google_oauth.go index 3044548948b..113b4c0ef5a 100644 --- a/pkg/login/social/connectors/google_oauth.go +++ b/pkg/login/social/connectors/google_oauth.go @@ -87,7 +87,7 @@ func (s *SocialGoogle) Validate(ctx context.Context, newSettings ssoModels.SSOSe } func (s *SocialGoogle) Reload(ctx context.Context, settings ssoModels.SSOSettings) error { - newInfo, err := CreateOAuthInfoFromKeyValues(settings.Settings) + newInfo, err := CreateOAuthInfoFromKeyValuesWithLogging(s.log, social.GoogleProviderName, settings.Settings) if err != nil { return ssosettings.ErrInvalidSettings.Errorf("SSO settings map cannot be converted to OAuthInfo: %v", err) } diff --git a/pkg/login/social/connectors/grafana_com_oauth.go b/pkg/login/social/connectors/grafana_com_oauth.go index 3f016b1f01c..84ea01632d1 100644 --- a/pkg/login/social/connectors/grafana_com_oauth.go +++ b/pkg/login/social/connectors/grafana_com_oauth.go @@ -39,15 +39,22 @@ type OrgRecord struct { } func NewGrafanaComProvider(info *social.OAuthInfo, cfg *setting.Cfg, orgRoleMapper *OrgRoleMapper, ssoSettings ssosettings.Service, features featuremgmt.FeatureToggles) *SocialGrafanaCom { + s := newSocialBase(social.GrafanaComProviderName, orgRoleMapper, info, features, cfg) + // Override necessary settings info.AuthUrl = cfg.GrafanaComURL + "/oauth2/authorize" info.TokenUrl = cfg.GrafanaComURL + "/api/oauth2/token" info.AuthStyle = "inheader" + allowedOrganizations, err := util.SplitStringWithError(info.Extra[allowedOrganizationsKey]) + if err != nil { + s.log.Error("Invalid auth configuration setting", "config", allowedOrganizationsKey, "provider", social.GrafanaComProviderName, "error", err) + } + provider := &SocialGrafanaCom{ - SocialBase: newSocialBase(social.GrafanaComProviderName, orgRoleMapper, info, features, cfg), + SocialBase: s, url: cfg.GrafanaComURL, - allowedOrganizations: util.SplitString(info.Extra[allowedOrganizationsKey]), + allowedOrganizations: allowedOrganizations, } if features.IsEnabledGlobally(featuremgmt.FlagSsoSettingsApi) { @@ -80,11 +87,16 @@ func (s *SocialGrafanaCom) Validate(ctx context.Context, newSettings ssoModels.S } func (s *SocialGrafanaCom) Reload(ctx context.Context, settings ssoModels.SSOSettings) error { - newInfo, err := CreateOAuthInfoFromKeyValues(settings.Settings) + newInfo, err := CreateOAuthInfoFromKeyValuesWithLogging(s.log, social.GrafanaComProviderName, settings.Settings) if err != nil { return ssosettings.ErrInvalidSettings.Errorf("SSO settings map cannot be converted to OAuthInfo: %v", err) } + allowedOrganizations, err := util.SplitStringWithError(newInfo.Extra[allowedOrganizationsKey]) + if err != nil { + s.log.Error("Invalid auth configuration setting", "config", allowedOrganizationsKey, "provider", social.GrafanaComProviderName, "error", err) + } + // Override necessary settings newInfo.AuthUrl = s.cfg.GrafanaComURL + "/oauth2/authorize" newInfo.TokenUrl = s.cfg.GrafanaComURL + "/api/oauth2/token" @@ -96,7 +108,7 @@ func (s *SocialGrafanaCom) Reload(ctx context.Context, settings ssoModels.SSOSet s.updateInfo(ctx, social.GrafanaComProviderName, newInfo) s.url = s.cfg.GrafanaComURL - s.allowedOrganizations = util.SplitString(newInfo.Extra[allowedOrganizationsKey]) + s.allowedOrganizations = allowedOrganizations return nil } diff --git a/pkg/login/social/connectors/okta_oauth.go b/pkg/login/social/connectors/okta_oauth.go index b126c2acd1d..ffa3a32a350 100644 --- a/pkg/login/social/connectors/okta_oauth.go +++ b/pkg/login/social/connectors/okta_oauth.go @@ -84,7 +84,7 @@ func (s *SocialOkta) Validate(ctx context.Context, newSettings ssoModels.SSOSett } func (s *SocialOkta) Reload(ctx context.Context, settings ssoModels.SSOSettings) error { - newInfo, err := CreateOAuthInfoFromKeyValues(settings.Settings) + newInfo, err := CreateOAuthInfoFromKeyValuesWithLogging(s.log, social.OktaProviderName, settings.Settings) if err != nil { return ssosettings.ErrInvalidSettings.Errorf("SSO settings map cannot be converted to OAuthInfo: %v", err) } diff --git a/pkg/login/social/socialimpl/service.go b/pkg/login/social/socialimpl/service.go index ddfb9bf7016..65a9a5573cc 100644 --- a/pkg/login/social/socialimpl/service.go +++ b/pkg/login/social/socialimpl/service.go @@ -65,7 +65,7 @@ func ProvideService(cfg *setting.Cfg, continue } - info, err := connectors.CreateOAuthInfoFromKeyValues(ssoSetting.Settings) + info, err := connectors.CreateOAuthInfoFromKeyValuesWithLogging(ss.log, ssoSetting.Provider, ssoSetting.Settings) if err != nil { ss.log.Error("Failed to create OAuthInfo for provider", "error", err, "provider", ssoSetting.Provider) continue @@ -85,7 +85,7 @@ func ProvideService(cfg *setting.Cfg, settingsKVs := convertIniSectionToMap(sec) - info, err := connectors.CreateOAuthInfoFromKeyValues(settingsKVs) + info, err := connectors.CreateOAuthInfoFromKeyValuesWithLogging(ss.log, name, settingsKVs) if err != nil { ss.log.Error("Failed to create OAuthInfo for provider", "error", err, "provider", name) continue diff --git a/pkg/util/strings.go b/pkg/util/strings.go index f3a2d35540f..b3bbed21cf2 100644 --- a/pkg/util/strings.go +++ b/pkg/util/strings.go @@ -33,9 +33,18 @@ func stringsFallback(vals ...string) string { // SplitString splits a string and returns a list of strings. It supports JSON list syntax and strings separated by commas or spaces. // It supports quoted strings with spaces, e.g. "foo bar", "baz". +// It will return an empty list if it fails to parse the string. func SplitString(str string) []string { + result, _ := SplitStringWithError(str) + return result +} + +// SplitStringWithError splits a string and returns a list of strings. It supports JSON list syntax and strings separated by commas or spaces. +// It supports quoted strings with spaces, e.g. "foo bar", "baz". +// It returns an error if it cannot parse the string. +func SplitStringWithError(str string) ([]string, error) { if len(str) == 0 { - return []string{} + return []string{}, nil } // JSON list syntax support @@ -43,9 +52,9 @@ func SplitString(str string) []string { var res []string err := json.Unmarshal([]byte(str), &res) if err != nil { - return []string{} + return []string{}, fmt.Errorf("incorrect format: %s", str) } - return res + return res, nil } matches := stringListItemMatcher.FindAllString(str, -1) @@ -55,7 +64,7 @@ func SplitString(str string) []string { result[i] = strings.Trim(match, "\"") } - return result + return result, nil } // GetAgeString returns a string representing certain time from years to minutes. diff --git a/public/app/features/auth-config/utils/data.ts b/public/app/features/auth-config/utils/data.ts index c0beae9ec27..00a3dd84453 100644 --- a/public/app/features/auth-config/utils/data.ts +++ b/public/app/features/auth-config/utils/data.ts @@ -56,7 +56,10 @@ const strToValue = (val: string | string[]): SelectableValue[] => { } // Stored as JSON Array if (val.startsWith('[') && val.endsWith(']')) { - return JSON.parse(val).map((v: string) => ({ label: v, value: v })); + // Fallback to parsing it like a non-json string if it is not valid json, instead of crashing. + try { + return JSON.parse(val).map((v: string) => ({ label: v, value: v })); + } catch {} } return val.split(/[\s,]/).map((s) => ({ label: s, value: s }));