From fc6e7fdbd11f979de2c4e5c1d3d7eb8b214566be Mon Sep 17 00:00:00 2001 From: Jan Larwig Date: Sat, 25 Nov 2023 12:32:31 +0100 Subject: [PATCH] bugfix: OIDCConfig based providers are not respecting flags and configs (#2299) * add full support for all oidc config based providers to use and respect all configs set via OIDCConfig * add changelog entry --------- Co-authored-by: Joel Speed --- CHANGELOG.md | 1 + providers/adfs.go | 6 +++--- providers/adfs_test.go | 12 +++++++----- providers/gitlab.go | 8 ++++---- providers/gitlab_test.go | 12 +++++++----- providers/keycloak_oidc.go | 6 +++--- providers/keycloak_oidc_test.go | 14 ++++++++------ providers/providers.go | 6 +++--- 8 files changed, 36 insertions(+), 29 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index ebbb37c1..06f5732e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -18,6 +18,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) +- [#2299](https://github.com/oauth2-proxy/oauth2-proxy/pull/2299) bugfix: OIDCConfig based providers are not respecting flags and configs (@tuunit) - [#2248](https://github.com/oauth2-proxy/oauth2-proxy/pull/2248) Added support for semicolons in query strings. # V7.5.1 diff --git a/providers/adfs.go b/providers/adfs.go index 586b0420..0facfdcf 100644 --- a/providers/adfs.go +++ b/providers/adfs.go @@ -29,7 +29,7 @@ const ( ) // NewADFSProvider initiates a new ADFSProvider -func NewADFSProvider(p *ProviderData, opts options.ADFSOptions) *ADFSProvider { +func NewADFSProvider(p *ProviderData, opts options.Provider) *ADFSProvider { p.setProviderDefaults(providerDefaults{ name: adfsProviderName, scope: adfsDefaultScope, @@ -46,11 +46,11 @@ func NewADFSProvider(p *ProviderData, opts options.ADFSOptions) *ADFSProvider { } } - oidcProvider := NewOIDCProvider(p, options.OIDCOptions{InsecureSkipNonce: false}) + oidcProvider := NewOIDCProvider(p, opts.OIDCConfig) return &ADFSProvider{ OIDCProvider: oidcProvider, - skipScope: opts.SkipScope, + skipScope: opts.ADFSConfig.SkipScope, oidcEnrichFunc: oidcProvider.EnrichSession, oidcRefreshFunc: oidcProvider.RefreshSession, } diff --git a/providers/adfs_test.go b/providers/adfs_test.go index 355fd939..92b1ac0d 100755 --- a/providers/adfs_test.go +++ b/providers/adfs_test.go @@ -63,7 +63,7 @@ func testADFSProvider(hostname string) *ADFSProvider { Scope: "", Verifier: o, EmailClaim: options.OIDCEmailClaim, - }, options.ADFSOptions{}) + }, options.Provider{}) if hostname != "" { updateURL(p.Data().LoginURL, hostname) @@ -134,12 +134,12 @@ var _ = Describe("ADFS Provider Tests", func() { Context("New Provider Init", func() { It("uses defaults", func() { - providerData := NewADFSProvider(&ProviderData{}, options.ADFSOptions{}).Data() + providerData := NewADFSProvider(&ProviderData{}, options.Provider{}).Data() Expect(providerData.ProviderName).To(Equal("ADFS")) Expect(providerData.Scope).To(Equal(oidcDefaultScope)) }) It("uses custom scope", func() { - providerData := NewADFSProvider(&ProviderData{Scope: "openid email"}, options.ADFSOptions{}).Data() + providerData := NewADFSProvider(&ProviderData{Scope: "openid email"}, options.Provider{}).Data() Expect(providerData.ProviderName).To(Equal("ADFS")) Expect(providerData.Scope).To(Equal("openid email")) Expect(providerData.Scope).NotTo(Equal(oidcDefaultScope)) @@ -172,7 +172,9 @@ var _ = Describe("ADFS Provider Tests", func() { p := NewADFSProvider(&ProviderData{ ProtectedResource: resource, Scope: "", - }, options.ADFSOptions{SkipScope: true}) + }, options.Provider{ + ADFSConfig: options.ADFSOptions{SkipScope: true}, + }) result := p.GetLoginURL("https://example.com/adfs/oauth2/", "", "", url.Values{}) Expect(result).NotTo(ContainSubstring("scope=")) @@ -192,7 +194,7 @@ var _ = Describe("ADFS Provider Tests", func() { p := NewADFSProvider(&ProviderData{ ProtectedResource: resource, Scope: in.scope, - }, options.ADFSOptions{}) + }, options.Provider{}) Expect(p.Data().Scope).To(Equal(in.expectedScope)) result := p.GetLoginURL("https://example.com/adfs/oauth2/", "", "", url.Values{}) diff --git a/providers/gitlab.go b/providers/gitlab.go index 4c9f2df0..ca39cf8e 100644 --- a/providers/gitlab.go +++ b/providers/gitlab.go @@ -31,7 +31,7 @@ type GitLabProvider struct { var _ Provider = (*GitLabProvider)(nil) // NewGitLabProvider initiates a new GitLabProvider -func NewGitLabProvider(p *ProviderData, opts options.GitLabOptions) (*GitLabProvider, error) { +func NewGitLabProvider(p *ProviderData, opts options.Provider) (*GitLabProvider, error) { p.setProviderDefaults(providerDefaults{ name: gitlabProviderName, }) @@ -40,15 +40,15 @@ func NewGitLabProvider(p *ProviderData, opts options.GitLabOptions) (*GitLabProv p.Scope = gitlabDefaultScope } - oidcProvider := NewOIDCProvider(p, options.OIDCOptions{InsecureSkipNonce: false}) + oidcProvider := NewOIDCProvider(p, opts.OIDCConfig) provider := &GitLabProvider{ OIDCProvider: oidcProvider, oidcRefreshFunc: oidcProvider.RefreshSession, } - provider.setAllowedGroups(opts.Group) + provider.setAllowedGroups(opts.GitLabConfig.Group) - if err := provider.setAllowedProjects(opts.Projects); err != nil { + if err := provider.setAllowedProjects(opts.GitLabConfig.Projects); err != nil { return nil, fmt.Errorf("could not configure allowed projects: %v", err) } diff --git a/providers/gitlab_test.go b/providers/gitlab_test.go index bc60858a..2ed4e8f7 100644 --- a/providers/gitlab_test.go +++ b/providers/gitlab_test.go @@ -14,7 +14,7 @@ import ( . "github.com/onsi/gomega" ) -func testGitLabProvider(hostname, scope string, opts options.GitLabOptions) (*GitLabProvider, error) { +func testGitLabProvider(hostname, scope string, opts options.Provider) (*GitLabProvider, error) { p, err := NewGitLabProvider( &ProviderData{ ProviderName: "", @@ -162,7 +162,7 @@ var _ = Describe("Gitlab Provider Tests", func() { bURL, err := url.Parse(b.URL) Expect(err).To(BeNil()) - p, err = testGitLabProvider(bURL.Host, "", options.GitLabOptions{}) + p, err = testGitLabProvider(bURL.Host, "", options.Provider{}) Expect(err).ToNot(HaveOccurred()) }) @@ -237,9 +237,11 @@ var _ = Describe("Gitlab Provider Tests", func() { bURL, err := url.Parse(b.URL) Expect(err).To(BeNil()) - p, err := testGitLabProvider(bURL.Host, in.scope, options.GitLabOptions{ - Group: in.allowedGroups, - Projects: in.allowedProjects, + p, err := testGitLabProvider(bURL.Host, in.scope, options.Provider{ + GitLabConfig: options.GitLabOptions{ + Group: in.allowedGroups, + Projects: in.allowedProjects, + }, }) if in.expectedError == nil { Expect(err).To(BeNil()) diff --git a/providers/keycloak_oidc.go b/providers/keycloak_oidc.go index 269fef4e..ab613752 100644 --- a/providers/keycloak_oidc.go +++ b/providers/keycloak_oidc.go @@ -16,16 +16,16 @@ type KeycloakOIDCProvider struct { } // NewKeycloakOIDCProvider makes a KeycloakOIDCProvider using the ProviderData -func NewKeycloakOIDCProvider(p *ProviderData, opts options.KeycloakOptions) *KeycloakOIDCProvider { +func NewKeycloakOIDCProvider(p *ProviderData, opts options.Provider) *KeycloakOIDCProvider { p.setProviderDefaults(providerDefaults{ name: keycloakOIDCProviderName, }) provider := &KeycloakOIDCProvider{ - OIDCProvider: NewOIDCProvider(p, options.OIDCOptions{InsecureSkipNonce: false}), + OIDCProvider: NewOIDCProvider(p, opts.OIDCConfig), } - provider.addAllowedRoles(opts.Roles) + provider.addAllowedRoles(opts.KeycloakConfig.Roles) return provider } diff --git a/providers/keycloak_oidc_test.go b/providers/keycloak_oidc_test.go index 4eda8aa8..fa316930 100644 --- a/providers/keycloak_oidc_test.go +++ b/providers/keycloak_oidc_test.go @@ -40,11 +40,11 @@ func getAccessToken() string { func newTestKeycloakOIDCSetup() (*httptest.Server, *KeycloakOIDCProvider) { redeemURL, server := newOIDCServer([]byte(fmt.Sprintf(`{"email": "new@thing.com", "expires_in": 300, "access_token": "%v"}`, getAccessToken()))) - provider := newKeycloakOIDCProvider(redeemURL, options.KeycloakOptions{}) + provider := newKeycloakOIDCProvider(redeemURL, options.Provider{}) return server, provider } -func newKeycloakOIDCProvider(serverURL *url.URL, opts options.KeycloakOptions) *KeycloakOIDCProvider { +func newKeycloakOIDCProvider(serverURL *url.URL, opts options.Provider) *KeycloakOIDCProvider { verificationOptions := internaloidc.IDTokenVerificationOptions{ AudienceClaims: []string{defaultAudienceClaim}, ClientID: mockClientID, @@ -90,7 +90,7 @@ func newKeycloakOIDCProvider(serverURL *url.URL, opts options.KeycloakOptions) * var _ = Describe("Keycloak OIDC Provider Tests", func() { Context("New Provider Init", func() { It("creates new keycloak oidc provider with expected defaults", func() { - p := newKeycloakOIDCProvider(nil, options.KeycloakOptions{}) + p := newKeycloakOIDCProvider(nil, options.Provider{}) providerData := p.Data() Expect(providerData.ProviderName).To(Equal(keycloakOIDCProviderName)) Expect(providerData.LoginURL.String()).To(Equal("https://keycloak-oidc.com/oauth/auth")) @@ -100,7 +100,7 @@ var _ = Describe("Keycloak OIDC Provider Tests", func() { Expect(providerData.Scope).To(Equal(oidcDefaultScope)) }) It("creates new keycloak oidc provider with custom scope", func() { - p := NewKeycloakOIDCProvider(&ProviderData{Scope: "openid email"}, options.KeycloakOptions{}) + p := NewKeycloakOIDCProvider(&ProviderData{Scope: "openid email"}, options.Provider{}) providerData := p.Data() Expect(providerData.ProviderName).To(Equal(keycloakOIDCProviderName)) @@ -111,8 +111,10 @@ var _ = Describe("Keycloak OIDC Provider Tests", func() { Context("Allowed Roles", func() { It("should prefix allowed roles and add them to groups", func() { - p := newKeycloakOIDCProvider(nil, options.KeycloakOptions{ - Roles: []string{"admin", "editor"}, + p := newKeycloakOIDCProvider(nil, options.Provider{ + KeycloakConfig: options.KeycloakOptions{ + Roles: []string{"admin", "editor"}, + }, }) Expect(p.AllowedGroups).To(HaveKey("role:admin")) Expect(p.AllowedGroups).To(HaveKey("role:editor")) diff --git a/providers/providers.go b/providers/providers.go index 67800dd7..192b86a0 100644 --- a/providers/providers.go +++ b/providers/providers.go @@ -38,7 +38,7 @@ func NewProvider(providerConfig options.Provider) (Provider, error) { } switch providerConfig.Type { case options.ADFSProvider: - return NewADFSProvider(providerData, providerConfig.ADFSConfig), nil + return NewADFSProvider(providerData, providerConfig), nil case options.AzureProvider: return NewAzureProvider(providerData, providerConfig.AzureConfig), nil case options.BitbucketProvider: @@ -50,13 +50,13 @@ func NewProvider(providerConfig options.Provider) (Provider, error) { case options.GitHubProvider: return NewGitHubProvider(providerData, providerConfig.GitHubConfig), nil case options.GitLabProvider: - return NewGitLabProvider(providerData, providerConfig.GitLabConfig) + return NewGitLabProvider(providerData, providerConfig) case options.GoogleProvider: return NewGoogleProvider(providerData, providerConfig.GoogleConfig) case options.KeycloakProvider: return NewKeycloakProvider(providerData, providerConfig.KeycloakConfig), nil case options.KeycloakOIDCProvider: - return NewKeycloakOIDCProvider(providerData, providerConfig.KeycloakConfig), nil + return NewKeycloakOIDCProvider(providerData, providerConfig), nil case options.LinkedInProvider: return NewLinkedInProvider(providerData), nil case options.LoginGovProvider: