From 822cfad9945510b8488385a6baf08024a96b64d3 Mon Sep 17 00:00:00 2001 From: Leandro Lafin Date: Fri, 12 Jul 2024 15:57:12 -0300 Subject: [PATCH] Feature/add option to skip loading claims from profile url --- CHANGELOG.md | 1 + pkg/apis/options/legacy_options.go | 32 ++++++++++--------- pkg/apis/options/providers.go | 3 ++ providers/provider_data.go | 18 +++++++---- providers/provider_data_test.go | 49 +++++++++++++++++++++++++----- providers/providers.go | 1 + 6 files changed, 75 insertions(+), 29 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 146372c7..b9dcf34a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -23,6 +23,7 @@ - [#1866](https://github.com/oauth2-proxy/oauth2-proxy/pull/1866) Add support for unix socker as upstream (@babs) - [#1949](https://github.com/oauth2-proxy/oauth2-proxy/pull/1949) Allow cookie names with dots in redis sessions (@miguelborges99) - [#2297](https://github.com/oauth2-proxy/oauth2-proxy/pull/2297) Add nightly build and push (@tuunit) +- [#2329](https://github.com/oauth2-proxy/oauth2-proxy/pull/2329) Add an option to skip request to profile URL for resolving missing claims in id_token (@nilsgstrabo) - [#2299](https://github.com/oauth2-proxy/oauth2-proxy/pull/2299) bugfix: OIDCConfig based providers are not respecting flags and configs (@tuunit) - [#2343](https://github.com/oauth2-proxy/oauth2-proxy/pull/2343) chore: Added checksums for .tar.gz (@kvanzuijlen) - [#2371](https://github.com/oauth2-proxy/oauth2-proxy/pull/2371) Remove nsswitch DNS resolution workaround (@tuunit) diff --git a/pkg/apis/options/legacy_options.go b/pkg/apis/options/legacy_options.go index 18cdbe4d..b3728f25 100644 --- a/pkg/apis/options/legacy_options.go +++ b/pkg/apis/options/legacy_options.go @@ -557,6 +557,7 @@ type LegacyProvider struct { LoginURL string `flag:"login-url" cfg:"login_url"` RedeemURL string `flag:"redeem-url" cfg:"redeem_url"` ProfileURL string `flag:"profile-url" cfg:"profile_url"` + SkipClaimsFromProfileURL bool `flag:"skip-claims-from-profile-url" cfg:"skip_claims_from_profile_url"` IntrospectURL string `flag:"introspect-url" cfg:"introspect_url"` ProtectedResource string `flag:"resource" cfg:"resource"` ValidateURL string `flag:"validate-url" cfg:"validate_url"` @@ -616,6 +617,7 @@ func legacyProviderFlagSet() *pflag.FlagSet { flagSet.String("login-url", "", "Authentication endpoint") flagSet.String("redeem-url", "", "Token redemption endpoint") flagSet.String("profile-url", "", "Profile access endpoint") + flagSet.Bool("skip-claims-from-profile-url", false, "Skip loading missing claims from profile URL") flagSet.String("introspect-url", "", "Introspect claims access endpoint") flagSet.String("resource", "", "The resource that is protected (Azure AD only)") flagSet.String("validate-url", "", "Access token validation endpoint") @@ -697,21 +699,21 @@ func (l *LegacyProvider) convert() (Providers, error) { providers := Providers{} provider := Provider{ - ClientID: l.ClientID, - ClientSecret: l.ClientSecret, - ClientSecretFile: l.ClientSecretFile, - Type: ProviderType(l.ProviderType), - CAFiles: l.ProviderCAFiles, - UseSystemTrustStore: l.UseSystemTrustStore, - LoginURL: l.LoginURL, - RedeemURL: l.RedeemURL, - ProfileURL: l.ProfileURL, - ProtectedResource: l.ProtectedResource, - ValidateURL: l.ValidateURL, - Scope: l.Scope, - AllowedGroups: l.AllowedGroups, - CodeChallengeMethod: l.CodeChallengeMethod, - IntrospectURL: l.IntrospectURL, + ClientID: l.ClientID, + ClientSecret: l.ClientSecret, + ClientSecretFile: l.ClientSecretFile, + Type: ProviderType(l.ProviderType), + CAFiles: l.ProviderCAFiles, + UseSystemTrustStore: l.UseSystemTrustStore, + LoginURL: l.LoginURL, + RedeemURL: l.RedeemURL, + ProfileURL: l.ProfileURL, + SkipClaimsFromProfileURL: l.SkipClaimsFromProfileURL, + ProtectedResource: l.ProtectedResource, + ValidateURL: l.ValidateURL, + Scope: l.Scope, + AllowedGroups: l.AllowedGroups, + CodeChallengeMethod: l.CodeChallengeMethod, } // This part is out of the switch section for all providers that support OIDC diff --git a/pkg/apis/options/providers.go b/pkg/apis/options/providers.go index b89ff734..acf246f4 100644 --- a/pkg/apis/options/providers.go +++ b/pkg/apis/options/providers.go @@ -70,6 +70,9 @@ type Provider struct { RedeemURL string `json:"redeemURL,omitempty"` // ProfileURL is the profile access endpoint ProfileURL string `json:"profileURL,omitempty"` + // SkipClaimsFromProfileURL allows to skip request to Profile URL for resolving claims not present in id_token + // default set to 'false' + SkipClaimsFromProfileURL bool `json:"skipClaimsFromProfileURL,omitempty"` // IntrospectURL is the claims and permissions access Endpoint (OIDC) IntrospectURL string `json:"introspectURL,omitempty"` // ProtectedResource is the resource that is protected (Azure AD and ADFS only) diff --git a/providers/provider_data.go b/providers/provider_data.go index 97508e37..4e72735b 100644 --- a/providers/provider_data.go +++ b/providers/provider_data.go @@ -44,11 +44,12 @@ type ProviderData struct { SupportedCodeChallengeMethods []string `json:"code_challenge_methods_supported,omitempty"` // Common OIDC options for any OIDC-based providers to consume - AllowUnverifiedEmail bool - UserClaim string - EmailClaim string - GroupsClaim string - Verifier internaloidc.IDTokenVerifier + AllowUnverifiedEmail bool + UserClaim string + EmailClaim string + GroupsClaim string + Verifier internaloidc.IDTokenVerifier + SkipClaimsFromProfileURL bool // Universal Group authorization data structure // any provider can set to consume @@ -285,7 +286,12 @@ func (p *ProviderData) buildSessionFromClaims(rawIDToken, accessToken string) (* } func (p *ProviderData) getClaimExtractor(rawIDToken, accessToken string) (util.ClaimExtractor, error) { - extractor, err := util.NewClaimExtractor(context.TODO(), rawIDToken, p.ProfileURL, p.getAuthorizationHeader(accessToken)) + profileURL := p.ProfileURL + if p.SkipClaimsFromProfileURL { + profileURL = &url.URL{} + } + + extractor, err := util.NewClaimExtractor(context.TODO(), rawIDToken, profileURL, p.getAuthorizationHeader(accessToken)) if err != nil { return nil, fmt.Errorf("could not initialise claim extractor: %v", err) } diff --git a/providers/provider_data_test.go b/providers/provider_data_test.go index 838c061b..4aa4844e 100644 --- a/providers/provider_data_test.go +++ b/providers/provider_data_test.go @@ -8,6 +8,8 @@ import ( "encoding/json" "errors" "fmt" + "net/http" + "net/http/httptest" "net/url" "strings" "testing" @@ -233,13 +235,16 @@ func TestProviderData_verifyIDToken(t *testing.T) { func TestProviderData_buildSessionFromClaims(t *testing.T) { testCases := map[string]struct { - IDToken idTokenClaims - AllowUnverified bool - UserClaim string - EmailClaim string - GroupsClaim string - ExpectedError error - ExpectedSession *sessions.SessionState + IDToken idTokenClaims + AllowUnverified bool + UserClaim string + EmailClaim string + GroupsClaim string + SkipClaimsFromProfileURL bool + SetProfileURL bool + ExpectedError error + ExpectedSession *sessions.SessionState + ExpectProfileURLCalled bool }{ "Standard": { IDToken: defaultIDToken, @@ -408,10 +413,34 @@ func TestProviderData_buildSessionFromClaims(t *testing.T) { PreferredUsername: "Jane Dobbs", }, }, + "Request claims from ProfileURL": { + IDToken: minimalIDToken, + SetProfileURL: true, + ExpectProfileURLCalled: true, + ExpectedSession: &sessions.SessionState{}, + }, + "Skip claims request to ProfileURL": { + IDToken: minimalIDToken, + SetProfileURL: true, + SkipClaimsFromProfileURL: true, + ExpectedSession: &sessions.SessionState{}, + }, } for testName, tc := range testCases { t.Run(testName, func(t *testing.T) { g := NewWithT(t) + var ( + profileURL *url.URL + profileURLCalled bool + ) + if tc.SetProfileURL { + profileURLSrv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + profileURLCalled = true + w.Write([]byte("{}")) + })) + defer profileURLSrv.Close() + profileURL, _ = url.Parse(profileURLSrv.URL) + } verificationOptions := internaloidc.IDTokenVerificationOptions{ AudienceClaims: []string{"aud"}, @@ -423,22 +452,26 @@ func TestProviderData_buildSessionFromClaims(t *testing.T) { mockJWKS{}, &oidc.Config{ClientID: oidcClientID}, ), verificationOptions), + ProfileURL: profileURL, + getAuthorizationHeaderFunc: func(s string) http.Header { return http.Header{} }, } provider.AllowUnverifiedEmail = tc.AllowUnverified provider.UserClaim = tc.UserClaim provider.EmailClaim = tc.EmailClaim provider.GroupsClaim = tc.GroupsClaim + provider.SkipClaimsFromProfileURL = tc.SkipClaimsFromProfileURL rawIDToken, err := newSignedTestIDToken(tc.IDToken) g.Expect(err).ToNot(HaveOccurred()) - ss, err := provider.buildSessionFromClaims(rawIDToken, "") + ss, err := provider.buildSessionFromClaims(rawIDToken, "testtoken") if err != nil { g.Expect(err).To(Equal(tc.ExpectedError)) } if ss != nil { g.Expect(ss).To(Equal(tc.ExpectedSession)) } + g.Expect(profileURLCalled).To(Equal(tc.ExpectProfileURLCalled)) }) } } diff --git a/providers/providers.go b/providers/providers.go index bf9159e1..e0c31812 100644 --- a/providers/providers.go +++ b/providers/providers.go @@ -140,6 +140,7 @@ func newProviderDataFromConfig(providerConfig options.Provider) (*ProviderData, p.AllowUnverifiedEmail = providerConfig.OIDCConfig.InsecureAllowUnverifiedEmail p.EmailClaim = providerConfig.OIDCConfig.EmailClaim p.GroupsClaim = providerConfig.OIDCConfig.GroupsClaim + p.SkipClaimsFromProfileURL = providerConfig.SkipClaimsFromProfileURL // Set PKCE enabled or disabled based on discovery and force options p.CodeChallengeMethod = parseCodeChallengeMethod(providerConfig)