From 2f3a01f79fda7f6c465dd64adf5cf5c7227f9d19 Mon Sep 17 00:00:00 2001 From: Karl Persson Date: Mon, 25 Mar 2024 14:22:24 +0100 Subject: [PATCH] OAuth: Make sub claim required for generic oauth behind feature toggle (#85065) * Add feature toggle for sub claims requirement * OAuth: require valid auth id * Fix feature toggle description --- .../src/types/featureToggles.gen.ts | 1 + pkg/services/authn/authnimpl/service.go | 2 +- pkg/services/authn/clients/oauth.go | 17 +++-- pkg/services/authn/clients/oauth_test.go | 63 ++++++++++++++++++- pkg/services/featuremgmt/registry.go | 8 +++ pkg/services/featuremgmt/toggles_gen.csv | 1 + pkg/services/featuremgmt/toggles_gen.go | 4 ++ pkg/services/featuremgmt/toggles_gen.json | 17 +++++ 8 files changed, 103 insertions(+), 10 deletions(-) diff --git a/packages/grafana-data/src/types/featureToggles.gen.ts b/packages/grafana-data/src/types/featureToggles.gen.ts index 7077d72318f..19abea7270e 100644 --- a/packages/grafana-data/src/types/featureToggles.gen.ts +++ b/packages/grafana-data/src/types/featureToggles.gen.ts @@ -176,4 +176,5 @@ export interface FeatureToggles { scopeFilters?: boolean; ssoSettingsSAML?: boolean; usePrometheusFrontendPackage?: boolean; + oauthRequireSubClaim?: boolean; } diff --git a/pkg/services/authn/authnimpl/service.go b/pkg/services/authn/authnimpl/service.go index 6573ef3dd85..39d1b310209 100644 --- a/pkg/services/authn/authnimpl/service.go +++ b/pkg/services/authn/authnimpl/service.go @@ -142,7 +142,7 @@ func ProvideService( for name := range socialService.GetOAuthProviders() { clientName := authn.ClientWithPrefix(name) - s.RegisterClient(clients.ProvideOAuth(clientName, cfg, oauthTokenService, socialService, settingsProviderService)) + s.RegisterClient(clients.ProvideOAuth(clientName, cfg, oauthTokenService, socialService, settingsProviderService, features)) } // FIXME (jguer): move to User package diff --git a/pkg/services/authn/clients/oauth.go b/pkg/services/authn/clients/oauth.go index 66d65051c4a..9cfcec0e635 100644 --- a/pkg/services/authn/clients/oauth.go +++ b/pkg/services/authn/clients/oauth.go @@ -18,6 +18,7 @@ import ( "github.com/grafana/grafana/pkg/login/social/connectors" "github.com/grafana/grafana/pkg/services/auth/identity" "github.com/grafana/grafana/pkg/services/authn" + "github.com/grafana/grafana/pkg/services/featuremgmt" "github.com/grafana/grafana/pkg/services/login" "github.com/grafana/grafana/pkg/services/oauthtoken" "github.com/grafana/grafana/pkg/services/org" @@ -65,12 +66,12 @@ var _ authn.RedirectClient = new(OAuth) func ProvideOAuth( name string, cfg *setting.Cfg, oauthService oauthtoken.OAuthTokenService, - socialService social.Service, settingsProviderService setting.Provider, + socialService social.Service, settingsProviderService setting.Provider, features featuremgmt.FeatureToggles, ) *OAuth { providerName := strings.TrimPrefix(name, "auth.client.") return &OAuth{ name, fmt.Sprintf("oauth_%s", providerName), providerName, - log.New(name), cfg, settingsProviderService, oauthService, socialService, + log.New(name), cfg, settingsProviderService, oauthService, socialService, features, } } @@ -84,6 +85,7 @@ type OAuth struct { settingsProviderSvc setting.Provider oauthService oauthtoken.OAuthTokenService socialService social.Service + features featuremgmt.FeatureToggles } func (c *OAuth) Name() string { @@ -148,10 +150,13 @@ func (c *OAuth) Authenticate(ctx context.Context, r *authn.Request) (*authn.Iden return nil, errOAuthUserInfo.Errorf("failed to get user info: %w", err) } - // Implement in Grafana 11 - // if userInfo.Id == "" { - // return nil, errors.New("idP did not return a user id") - // } + if userInfo.Id == "" { + if c.features.IsEnabledGlobally(featuremgmt.FlagOauthRequireSubClaim) { + return nil, errOAuthUserInfo.Errorf("missing required sub claims") + } else { + c.log.FromContext(ctx).Warn("Missing sub claim, oauth authentication without a sub claim is deprecated and will be rejected in future versions.") + } + } if userInfo.Email == "" { return nil, errOAuthMissingRequiredEmail.Errorf("required attribute email was not provided") diff --git a/pkg/services/authn/clients/oauth_test.go b/pkg/services/authn/clients/oauth_test.go index b1007594c80..7432ac2ad83 100644 --- a/pkg/services/authn/clients/oauth_test.go +++ b/pkg/services/authn/clients/oauth_test.go @@ -18,6 +18,7 @@ import ( "github.com/grafana/grafana/pkg/login/social/socialtest" "github.com/grafana/grafana/pkg/services/auth/identity" "github.com/grafana/grafana/pkg/services/authn" + "github.com/grafana/grafana/pkg/services/featuremgmt" "github.com/grafana/grafana/pkg/services/login" "github.com/grafana/grafana/pkg/services/oauthtoken/oauthtokentest" "github.com/grafana/grafana/pkg/services/org" @@ -37,6 +38,8 @@ func TestOAuth_Authenticate(t *testing.T) { addPKCECookie bool pkceCookieValue string + features []any + isEmailAllowed bool userInfo *social.BasicUserInfo @@ -120,6 +123,24 @@ func TestOAuth_Authenticate(t *testing.T) { isEmailAllowed: false, expectedErr: errOAuthEmailNotAllowed, }, + { + desc: "should return error when no auth id is set and feature toggle is enabled", + req: &authn.Request{ + HTTPRequest: &http.Request{ + Header: map[string][]string{}, + URL: mustParseURL("http://grafana.com/?state=some-state"), + }, + }, + features: []any{featuremgmt.FlagOauthRequireSubClaim}, + oauthCfg: &social.OAuthInfo{UsePKCE: true, Enabled: true}, + addStateCookie: true, + stateCookieValue: "some-state", + addPKCECookie: true, + pkceCookieValue: "some-pkce-value", + userInfo: &social.BasicUserInfo{Email: "some@email.com"}, + isEmailAllowed: false, + expectedErr: errOAuthUserInfo, + }, { desc: "should return identity for valid request", req: &authn.Request{HTTPRequest: &http.Request{ @@ -197,6 +218,42 @@ func TestOAuth_Authenticate(t *testing.T) { }, }, }, + { + desc: "should return identity when feature toggle is enabled and auth id is set", + req: &authn.Request{ + HTTPRequest: &http.Request{ + Header: map[string][]string{}, + URL: mustParseURL("http://grafana.com/?state=some-state"), + }, + }, + oauthCfg: &social.OAuthInfo{Enabled: true}, + addStateCookie: true, + stateCookieValue: "some-state", + isEmailAllowed: true, + features: []any{featuremgmt.FlagOauthRequireSubClaim}, + userInfo: &social.BasicUserInfo{ + Id: "123", + Name: "name", + Email: "some@email.com", + Role: "Admin", + }, + expectedIdentity: &authn.Identity{ + Email: "some@email.com", + AuthenticatedBy: login.AzureADAuthModule, + AuthID: "123", + Name: "name", + OAuthToken: &oauth2.Token{}, + OrgRoles: map[int64]org.RoleType{1: org.RoleAdmin}, + ClientParams: authn.ClientParams{ + SyncUser: true, + SyncTeams: true, + AllowSignUp: true, + FetchSyncedUser: true, + SyncOrgRoles: true, + LookUpParams: login.UserLookupParams{}, + }, + }, + }, } for _, tt := range tests { @@ -231,7 +288,7 @@ func TestOAuth_Authenticate(t *testing.T) { }, } - c := ProvideOAuth(authn.ClientWithPrefix("azuread"), cfg, nil, fakeSocialSvc, settingsProvider) + c := ProvideOAuth(authn.ClientWithPrefix("azuread"), cfg, nil, fakeSocialSvc, settingsProvider, featuremgmt.WithFeatures(tt.features...)) identity, err := c.Authenticate(context.Background(), tt.req) assert.ErrorIs(t, err, tt.expectedErr) @@ -314,7 +371,7 @@ func TestOAuth_RedirectURL(t *testing.T) { cfg := setting.NewCfg() - c := ProvideOAuth(authn.ClientWithPrefix("azuread"), cfg, nil, fakeSocialSvc, &setting.OSSImpl{Cfg: cfg}) + c := ProvideOAuth(authn.ClientWithPrefix("azuread"), cfg, nil, fakeSocialSvc, &setting.OSSImpl{Cfg: cfg}, featuremgmt.WithFeatures()) redirect, err := c.RedirectURL(context.Background(), nil) assert.ErrorIs(t, err, tt.expectedErr) @@ -427,7 +484,7 @@ func TestOAuth_Logout(t *testing.T) { fakeSocialSvc := &socialtest.FakeSocialService{ ExpectedAuthInfoProvider: tt.oauthCfg, } - c := ProvideOAuth(authn.ClientWithPrefix("azuread"), tt.cfg, mockService, fakeSocialSvc, &setting.OSSImpl{Cfg: tt.cfg}) + c := ProvideOAuth(authn.ClientWithPrefix("azuread"), tt.cfg, mockService, fakeSocialSvc, &setting.OSSImpl{Cfg: tt.cfg}, featuremgmt.WithFeatures()) redirect, ok := c.Logout(context.Background(), &authn.Identity{}, &login.UserAuth{}) diff --git a/pkg/services/featuremgmt/registry.go b/pkg/services/featuremgmt/registry.go index fd2e9d2b757..145f068ef45 100644 --- a/pkg/services/featuremgmt/registry.go +++ b/pkg/services/featuremgmt/registry.go @@ -1181,6 +1181,14 @@ var ( FrontendOnly: true, Owner: grafanaObservabilityMetricsSquad, }, + { + Name: "oauthRequireSubClaim", + Description: "Require that sub claims is present in oauth tokens.", + Stage: FeatureStageExperimental, + Owner: identityAccessTeam, + HideFromDocs: true, + HideFromAdminPage: true, + }, } ) diff --git a/pkg/services/featuremgmt/toggles_gen.csv b/pkg/services/featuremgmt/toggles_gen.csv index ef8fba63753..84b24304e1a 100644 --- a/pkg/services/featuremgmt/toggles_gen.csv +++ b/pkg/services/featuremgmt/toggles_gen.csv @@ -157,3 +157,4 @@ betterPageScrolling,GA,@grafana/grafana-frontend-platform,false,false,true scopeFilters,experimental,@grafana/dashboards-squad,false,false,false ssoSettingsSAML,experimental,@grafana/identity-access-team,false,false,false usePrometheusFrontendPackage,experimental,@grafana/observability-metrics,false,false,true +oauthRequireSubClaim,experimental,@grafana/identity-access-team,false,false,false diff --git a/pkg/services/featuremgmt/toggles_gen.go b/pkg/services/featuremgmt/toggles_gen.go index c36891a4b18..16730d2b283 100644 --- a/pkg/services/featuremgmt/toggles_gen.go +++ b/pkg/services/featuremgmt/toggles_gen.go @@ -638,4 +638,8 @@ const ( // FlagUsePrometheusFrontendPackage // Use the @grafana/prometheus frontend package in core Prometheus. FlagUsePrometheusFrontendPackage = "usePrometheusFrontendPackage" + + // FlagOauthRequireSubClaim + // Require that sub claims is present in oauth tokens. + FlagOauthRequireSubClaim = "oauthRequireSubClaim" ) diff --git a/pkg/services/featuremgmt/toggles_gen.json b/pkg/services/featuremgmt/toggles_gen.json index 14dce159956..3113c22decd 100644 --- a/pkg/services/featuremgmt/toggles_gen.json +++ b/pkg/services/featuremgmt/toggles_gen.json @@ -2046,6 +2046,23 @@ "stage": "experimental", "codeowner": "@grafana/alerting-squad" } + }, + { + "metadata": { + "name": "oauthRequireSubClaim", + "resourceVersion": "1711371458317", + "creationTimestamp": "2024-03-25T10:50:29Z", + "annotations": { + "grafana.app/updatedTimestamp": "2024-03-25 12:57:38.317427693 +0000 UTC" + } + }, + "spec": { + "description": "Require that sub claims is present in oauth tokens.", + "stage": "experimental", + "codeowner": "@grafana/identity-access-team", + "hideFromAdminPage": true, + "hideFromDocs": true + } } ] } \ No newline at end of file