From 95dd2745c7f0d0de3812478f2dc49768042af2cc Mon Sep 17 00:00:00 2001 From: Joel Speed Date: Tue, 15 Feb 2022 11:07:13 +0000 Subject: [PATCH 1/5] Remove options dependency on providers package --- pkg/apis/options/legacy_options.go | 9 ++++----- pkg/apis/options/options.go | 16 +++++----------- pkg/apis/options/providers.go | 19 ++++++++++++++----- 3 files changed, 23 insertions(+), 21 deletions(-) diff --git a/pkg/apis/options/legacy_options.go b/pkg/apis/options/legacy_options.go index 97d63d0..b4f564d 100644 --- a/pkg/apis/options/legacy_options.go +++ b/pkg/apis/options/legacy_options.go @@ -8,7 +8,6 @@ import ( "time" "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/logger" - "github.com/oauth2-proxy/oauth2-proxy/v7/providers" "github.com/spf13/pflag" ) @@ -552,9 +551,9 @@ func legacyProviderFlagSet() *pflag.FlagSet { flagSet.Bool("insecure-oidc-skip-nonce", true, "skip verifying the OIDC ID Token's nonce claim") flagSet.Bool("skip-oidc-discovery", false, "Skip OIDC discovery and use manually supplied Endpoints") flagSet.String("oidc-jwks-url", "", "OpenID Connect JWKS URL (ie: https://www.googleapis.com/oauth2/v3/certs)") - flagSet.String("oidc-groups-claim", providers.OIDCGroupsClaim, "which OIDC claim contains the user groups") - flagSet.String("oidc-email-claim", providers.OIDCEmailClaim, "which OIDC claim contains the user's email") - flagSet.StringSlice("oidc-audience-claim", providers.OIDCAudienceClaims, "which OIDC claims are used as audience to verify against client id") + flagSet.String("oidc-groups-claim", OIDCGroupsClaim, "which OIDC claim contains the user groups") + flagSet.String("oidc-email-claim", OIDCEmailClaim, "which OIDC claim contains the user's email") + flagSet.StringSlice("oidc-audience-claim", OIDCAudienceClaims, "which OIDC claims are used as audience to verify against client id") flagSet.StringSlice("oidc-extra-audience", []string{}, "additional audiences allowed to pass audience verification") flagSet.String("login-url", "", "Authentication endpoint") flagSet.String("redeem-url", "", "Token redemption endpoint") @@ -570,7 +569,7 @@ func legacyProviderFlagSet() *pflag.FlagSet { flagSet.String("jwt-key-file", "", "path to the private key file in PEM format used to sign the JWT so that you can say something like -jwt-key-file=/etc/ssl/private/jwt_signing_key.pem: required by login.gov") flagSet.String("pubjwk-url", "", "JWK pubkey access endpoint: required by login.gov") - flagSet.String("user-id-claim", providers.OIDCEmailClaim, "(DEPRECATED for `oidc-email-claim`) which claim contains the user ID") + flagSet.String("user-id-claim", OIDCEmailClaim, "(DEPRECATED for `oidc-email-claim`) which claim contains the user ID") flagSet.StringSlice("allowed-group", []string{}, "restrict logins to members of this group (may be given multiple times)") flagSet.StringSlice("allowed-role", []string{}, "(keycloak-oidc) restrict logins to members of these roles (may be given multiple times)") diff --git a/pkg/apis/options/options.go b/pkg/apis/options/options.go index 4a0feaf..00dfbfb 100644 --- a/pkg/apis/options/options.go +++ b/pkg/apis/options/options.go @@ -6,7 +6,6 @@ import ( ipapi "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/apis/ip" internaloidc "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/oidc" - "github.com/oauth2-proxy/oauth2-proxy/v7/providers" "github.com/spf13/pflag" ) @@ -68,7 +67,6 @@ type Options struct { // internal values that are set after config validation redirectURL *url.URL - provider providers.Provider signatureData *SignatureData oidcVerifier *internaloidc.IDTokenVerifier jwtBearerVerifiers []*internaloidc.IDTokenVerifier @@ -77,7 +75,6 @@ type Options struct { // Options for Getting internal values func (o *Options) GetRedirectURL() *url.URL { return o.redirectURL } -func (o *Options) GetProvider() providers.Provider { return o.provider } func (o *Options) GetSignatureData() *SignatureData { return o.signatureData } func (o *Options) GetOIDCVerifier() *internaloidc.IDTokenVerifier { return o.oidcVerifier } func (o *Options) GetJWTBearerVerifiers() []*internaloidc.IDTokenVerifier { @@ -86,14 +83,11 @@ func (o *Options) GetJWTBearerVerifiers() []*internaloidc.IDTokenVerifier { func (o *Options) GetRealClientIPParser() ipapi.RealClientIPParser { return o.realClientIPParser } // Options for Setting internal values -func (o *Options) SetRedirectURL(s *url.URL) { o.redirectURL = s } -func (o *Options) SetProvider(s providers.Provider) { o.provider = s } -func (o *Options) SetSignatureData(s *SignatureData) { o.signatureData = s } -func (o *Options) SetOIDCVerifier(s *internaloidc.IDTokenVerifier) { o.oidcVerifier = s } -func (o *Options) SetJWTBearerVerifiers(s []*internaloidc.IDTokenVerifier) { - o.jwtBearerVerifiers = s -} -func (o *Options) SetRealClientIPParser(s ipapi.RealClientIPParser) { o.realClientIPParser = s } +func (o *Options) SetRedirectURL(s *url.URL) { o.redirectURL = s } +func (o *Options) SetSignatureData(s *SignatureData) { o.signatureData = s } +func (o *Options) SetOIDCVerifier(s *internaloidc.IDTokenVerifier) { o.oidcVerifier = s } +func (o *Options) SetJWTBearerVerifiers(s []*internaloidc.IDTokenVerifier) { o.jwtBearerVerifiers = s } +func (o *Options) SetRealClientIPParser(s ipapi.RealClientIPParser) { o.realClientIPParser = s } // NewOptions constructs a new Options with defaulted values func NewOptions() *Options { diff --git a/pkg/apis/options/providers.go b/pkg/apis/options/providers.go index 3eebbe4..a92764c 100644 --- a/pkg/apis/options/providers.go +++ b/pkg/apis/options/providers.go @@ -1,6 +1,15 @@ package options -import "github.com/oauth2-proxy/oauth2-proxy/v7/providers" +const ( + // OIDCEmailClaim is the generic email claim used by the OIDC provider. + OIDCEmailClaim = "email" + + // OIDCGroupsClaim is the generic groups claim used by the OIDC provider. + OIDCGroupsClaim = "groups" +) + +// OIDCAudienceClaims is the generic audience claim list used by the OIDC provider. +var OIDCAudienceClaims = []string{"aud"} // Providers is a collection of definitions for providers. type Providers []Provider @@ -194,10 +203,10 @@ func providerDefaults() Providers { InsecureAllowUnverifiedEmail: false, InsecureSkipNonce: true, SkipDiscovery: false, - UserIDClaim: providers.OIDCEmailClaim, // Deprecated: Use OIDCEmailClaim - EmailClaim: providers.OIDCEmailClaim, - GroupsClaim: providers.OIDCGroupsClaim, - AudienceClaims: providers.OIDCAudienceClaims, + UserIDClaim: OIDCEmailClaim, // Deprecated: Use OIDCEmailClaim + EmailClaim: OIDCEmailClaim, + GroupsClaim: OIDCGroupsClaim, + AudienceClaims: OIDCAudienceClaims, ExtraAudiences: []string{}, }, }, From d162b018a837b57075e2f8dd486041ccc4be9e5f Mon Sep 17 00:00:00 2001 From: Joel Speed Date: Tue, 15 Feb 2022 11:18:32 +0000 Subject: [PATCH 2/5] Move provider initialisation into providers package --- docs/docs/configuration/alpha_config.md | 13 +- pkg/apis/options/legacy_options.go | 2 +- pkg/apis/options/providers.go | 52 ++++- providers/adfs.go | 11 +- providers/adfs_test.go | 12 +- providers/azure.go | 25 +-- providers/azure_test.go | 21 +- providers/bitbucket.go | 22 +- providers/bitbucket_test.go | 21 +- providers/github.go | 23 +- providers/github_test.go | 116 +++++++--- providers/gitlab.go | 16 +- providers/gitlab_test.go | 36 +-- providers/google.go | 23 +- providers/google_test.go | 34 ++- providers/keycloak.go | 8 +- providers/keycloak_oidc.go | 13 +- providers/keycloak_oidc_test.go | 15 +- providers/keycloak_test.go | 9 +- providers/logingov.go | 47 +++- providers/logingov_test.go | 69 +++++- providers/nextcloud.go | 4 +- providers/oidc.go | 5 +- providers/oidc_test.go | 11 +- providers/provider_data.go | 11 +- providers/provider_default_test.go | 2 +- providers/providers.go | 283 +++++++++++++++++++++--- providers/providers_test.go | 93 ++++++++ 28 files changed, 786 insertions(+), 211 deletions(-) create mode 100644 providers/providers_test.go diff --git a/docs/docs/configuration/alpha_config.md b/docs/docs/configuration/alpha_config.md index 79a09bf..154d4f8 100644 --- a/docs/docs/configuration/alpha_config.md +++ b/docs/docs/configuration/alpha_config.md @@ -305,7 +305,7 @@ Provider holds all configuration for a single provider | `oidcConfig` | _[OIDCOptions](#oidcoptions)_ | OIDCConfig holds all configurations for OIDC provider
or providers utilize OIDC configurations. | | `loginGovConfig` | _[LoginGovOptions](#logingovoptions)_ | LoginGovConfig holds all configurations for LoginGov provider. | | `id` | _string_ | ID should be a unique identifier for the provider.
This value is required for all providers. | -| `provider` | _string_ | Type is the OAuth provider
must be set from the supported providers group,
otherwise 'Google' is set as default | +| `provider` | _[ProviderType](#providertype)_ | Type is the OAuth provider
must be set from the supported providers group,
otherwise 'Google' is set as default | | `name` | _string_ | Name is the providers display name
if set, it will be shown to the users in the login page. | | `caFiles` | _[]string_ | CAFiles is a list of paths to CA certificates that should be used when connecting to the provider.
If not specified, the default Go trust sources are used instead | | `loginURL` | _string_ | LoginURL is the authentication endpoint | @@ -319,6 +319,17 @@ Provider holds all configuration for a single provider | `allowedGroups` | _[]string_ | AllowedGroups is a list of restrict logins to members of this group | | `acrValues` | _string_ | AcrValues is a string of acr values | +### ProviderType +#### (`string` alias) + +(**Appears on:** [Provider](#provider)) + +ProviderType is used to enumerate the different provider type options +Valid options are: adfs, azure, bitbucket, digitalocean facebook, github, +gitlab, google, keycloak, keycloak-oidc, linkedin, login.gov, nextcloud +and oidc. + + ### Providers #### ([[]Provider](#provider) alias) diff --git a/pkg/apis/options/legacy_options.go b/pkg/apis/options/legacy_options.go index b4f564d..121724c 100644 --- a/pkg/apis/options/legacy_options.go +++ b/pkg/apis/options/legacy_options.go @@ -624,7 +624,7 @@ func (l *LegacyProvider) convert() (Providers, error) { ClientID: l.ClientID, ClientSecret: l.ClientSecret, ClientSecretFile: l.ClientSecretFile, - Type: l.ProviderType, + Type: ProviderType(l.ProviderType), CAFiles: l.ProviderCAFiles, LoginURL: l.LoginURL, RedeemURL: l.RedeemURL, diff --git a/pkg/apis/options/providers.go b/pkg/apis/options/providers.go index a92764c..2631e32 100644 --- a/pkg/apis/options/providers.go +++ b/pkg/apis/options/providers.go @@ -52,7 +52,7 @@ type Provider struct { // Type is the OAuth provider // must be set from the supported providers group, // otherwise 'Google' is set as default - Type string `json:"provider,omitempty"` + Type ProviderType `json:"provider,omitempty"` // Name is the providers display name // if set, it will be shown to the users in the login page. Name string `json:"name,omitempty"` @@ -84,6 +84,56 @@ type Provider struct { AcrValues string `json:"acrValues,omitempty"` } +// ProviderType is used to enumerate the different provider type options +// Valid options are: adfs, azure, bitbucket, digitalocean facebook, github, +// gitlab, google, keycloak, keycloak-oidc, linkedin, login.gov, nextcloud +// and oidc. +type ProviderType string + +const ( + // ADFSProvider is the provider type for ADFS + ADFSProvider ProviderType = "adfs" + + // AzureProvider is the provider type for Azure + AzureProvider ProviderType = "azure" + + // BitbucketProvider is the provider type for Bitbucket + BitbucketProvider ProviderType = "bitbucket" + + // DigitalOceanProvider is the provider type for DigitalOcean + DigitalOceanProvider ProviderType = "digitalocean" + + // FacebookProvider is the provider type for Facebook + FacebookProvider ProviderType = "facebook" + + // GitHubProvider is the provider type for GitHub + GitHubProvider ProviderType = "github" + + // GitLabProvider is the provider type for GitLab + GitLabProvider ProviderType = "gitlab" + + // GoogleProvider is the provider type for GoogleProvider + GoogleProvider ProviderType = "google" + + // KeycloakProvider is the provider type for Keycloak + KeycloakProvider ProviderType = "keycloak" + + // KeycloakOIDCProvider is the provider type for Keycloak OIDC + KeycloakOIDCProvider ProviderType = "keycloak-oidc" + + // LinkedInProvider is the provider type for LinkedIn + LinkedInProvider ProviderType = "linkedin" + + // LoginGovProvider is the provider type for LoginGov + LoginGovProvider ProviderType = "login.gov" + + // NextCloudProvider is the provider type for NextCloud + NextCloudProvider ProviderType = "nextcloud" + + // OIDCProvider is the provider type for OIDC + OIDCProvider ProviderType = "oidc" +) + type KeycloakOptions struct { // Group enables to restrict login to members of indicated group Groups []string `json:"groups,omitempty"` diff --git a/providers/adfs.go b/providers/adfs.go index f5cbbfc..844b2d2 100644 --- a/providers/adfs.go +++ b/providers/adfs.go @@ -6,6 +6,7 @@ import ( "net/url" "strings" + "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/apis/options" "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/apis/sessions" ) @@ -24,12 +25,11 @@ var _ Provider = (*ADFSProvider)(nil) const ( adfsProviderName = "ADFS" adfsDefaultScope = "openid email profile" - adfsSkipScope = false adfsUPNClaim = "upn" ) // NewADFSProvider initiates a new ADFSProvider -func NewADFSProvider(p *ProviderData) *ADFSProvider { +func NewADFSProvider(p *ProviderData, opts options.ADFSOptions) *ADFSProvider { p.setProviderDefaults(providerDefaults{ name: adfsProviderName, scope: adfsDefaultScope, @@ -53,17 +53,12 @@ func NewADFSProvider(p *ProviderData) *ADFSProvider { return &ADFSProvider{ OIDCProvider: oidcProvider, - skipScope: adfsSkipScope, + skipScope: opts.SkipScope, oidcEnrichFunc: oidcProvider.EnrichSession, oidcRefreshFunc: oidcProvider.RefreshSession, } } -// Configure defaults the ADFSProvider configuration options -func (p *ADFSProvider) Configure(skipScope bool) { - p.skipScope = skipScope -} - // GetLoginURL Override to double encode the state parameter. If not query params are lost // More info here: https://docs.microsoft.com/en-us/powerapps/maker/portals/configure/configure-saml2-settings func (p *ADFSProvider) GetLoginURL(redirectURI, state, nonce string) string { diff --git a/providers/adfs_test.go b/providers/adfs_test.go index 93e61ea..434175d 100755 --- a/providers/adfs_test.go +++ b/providers/adfs_test.go @@ -13,6 +13,7 @@ import ( "github.com/coreos/go-oidc/v3/oidc" "github.com/golang-jwt/jwt" + "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/apis/options" "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/apis/sessions" internaloidc "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/oidc" . "github.com/onsi/ginkgo" @@ -61,8 +62,8 @@ func testADFSProvider(hostname string) *ADFSProvider { ValidateURL: &url.URL{}, Scope: "", Verifier: o, - EmailClaim: OIDCEmailClaim, - }) + EmailClaim: options.OIDCEmailClaim, + }, options.ADFSOptions{}) if hostname != "" { updateURL(p.Data().LoginURL, hostname) @@ -133,7 +134,7 @@ var _ = Describe("ADFS Provider Tests", func() { Context("New Provider Init", func() { It("uses defaults", func() { - providerData := NewADFSProvider(&ProviderData{}).Data() + providerData := NewADFSProvider(&ProviderData{}, options.ADFSOptions{}).Data() Expect(providerData.ProviderName).To(Equal("ADFS")) Expect(providerData.Scope).To(Equal("openid email profile")) }) @@ -165,8 +166,7 @@ var _ = Describe("ADFS Provider Tests", func() { p := NewADFSProvider(&ProviderData{ ProtectedResource: resource, Scope: "", - }) - p.skipScope = true + }, options.ADFSOptions{SkipScope: true}) result := p.GetLoginURL("https://example.com/adfs/oauth2/", "", "") Expect(result).NotTo(ContainSubstring("scope=")) @@ -186,7 +186,7 @@ var _ = Describe("ADFS Provider Tests", func() { p := NewADFSProvider(&ProviderData{ ProtectedResource: resource, Scope: in.scope, - }) + }, options.ADFSOptions{}) Expect(p.Data().Scope).To(Equal(in.expectedScope)) result := p.GetLoginURL("https://example.com/adfs/oauth2/", "", "") diff --git a/providers/azure.go b/providers/azure.go index 10ea701..9c955a6 100644 --- a/providers/azure.go +++ b/providers/azure.go @@ -10,6 +10,7 @@ import ( "time" "github.com/bitly/go-simplejson" + "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/apis/options" "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/apis/sessions" "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/logger" "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/requests" @@ -62,7 +63,7 @@ var ( ) // NewAzureProvider initiates a new AzureProvider -func NewAzureProvider(p *ProviderData) *AzureProvider { +func NewAzureProvider(p *ProviderData, opts options.AzureOptions) *AzureProvider { p.setProviderDefaults(providerDefaults{ name: azureProviderName, loginURL: azureDefaultLoginURL, @@ -80,25 +81,19 @@ func NewAzureProvider(p *ProviderData) *AzureProvider { } p.getAuthorizationHeaderFunc = makeAzureHeader + tenant := "common" + if opts.Tenant != "" { + tenant = opts.Tenant + overrideTenantURL(p.LoginURL, azureDefaultLoginURL, tenant, "authorize") + overrideTenantURL(p.RedeemURL, azureDefaultRedeemURL, tenant, "token") + } + return &AzureProvider{ ProviderData: p, - Tenant: "common", + Tenant: tenant, } } -// Configure defaults the AzureProvider configuration options -func (p *AzureProvider) Configure(tenant string) { - if tenant == "" || tenant == "common" { - // tenant is empty or default, remain on the default "common" tenant - return - } - - // Specific tennant specified, override the Login and RedeemURLs - p.Tenant = tenant - overrideTenantURL(p.LoginURL, azureDefaultLoginURL, tenant, "authorize") - overrideTenantURL(p.RedeemURL, azureDefaultRedeemURL, tenant, "token") -} - func overrideTenantURL(current, defaultURL *url.URL, tenant, path string) { if current == nil || current.String() == "" || current.String() == defaultURL.String() { *current = url.URL{ diff --git a/providers/azure_test.go b/providers/azure_test.go index 25b8c20..5e91285 100644 --- a/providers/azure_test.go +++ b/providers/azure_test.go @@ -15,6 +15,7 @@ import ( "github.com/coreos/go-oidc/v3/oidc" "github.com/golang-jwt/jwt" + "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/apis/options" "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/apis/sessions" internaloidc "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/oidc" @@ -39,7 +40,7 @@ type azureOAuthPayload struct { IDToken string `json:"id_token,omitempty"` } -func testAzureProvider(hostname string) *AzureProvider { +func testAzureProvider(hostname string, opts options.AzureOptions) *AzureProvider { verificationOptions := &internaloidc.IDTokenVerificationOptions{ AudienceClaims: []string{"aud"}, ClientID: "cd6d4fae-f6a6-4a34-8454-2c6b598e9532", @@ -64,7 +65,7 @@ func testAzureProvider(hostname string) *AzureProvider { SkipExpiryCheck: true, }, ), verificationOptions), - }) + }, opts) if hostname != "" { updateURL(p.Data().LoginURL, hostname) @@ -80,7 +81,7 @@ func TestNewAzureProvider(t *testing.T) { g := NewWithT(t) // Test that defaults are set when calling for a new provider with nothing set - providerData := NewAzureProvider(&ProviderData{}).Data() + providerData := NewAzureProvider(&ProviderData{}, options.AzureOptions{}).Data() g.Expect(providerData.ProviderName).To(Equal("Azure")) g.Expect(providerData.LoginURL.String()).To(Equal("https://login.microsoftonline.com/common/oauth2/authorize")) g.Expect(providerData.RedeemURL.String()).To(Equal("https://login.microsoftonline.com/common/oauth2/token")) @@ -111,7 +112,8 @@ func TestAzureProviderOverrides(t *testing.T) { ProtectedResource: &url.URL{ Scheme: "https", Host: "example.com"}, - Scope: "profile"}) + Scope: "profile"}, + options.AzureOptions{}) assert.NotEqual(t, nil, p) assert.Equal(t, "Azure", p.Data().ProviderName) assert.Equal(t, "https://example.com/oauth/auth", @@ -128,8 +130,7 @@ func TestAzureProviderOverrides(t *testing.T) { } func TestAzureSetTenant(t *testing.T) { - p := testAzureProvider("") - p.Configure("example") + p := testAzureProvider("", options.AzureOptions{Tenant: "example"}) assert.Equal(t, "Azure", p.Data().ProviderName) assert.Equal(t, "example", p.Tenant) assert.Equal(t, "https://login.microsoftonline.com/example/oauth2/authorize", @@ -230,7 +231,7 @@ func TestAzureProviderEnrichSession(t *testing.T) { bURL, _ := url.Parse(b.URL) host = bURL.Host } - p := testAzureProvider(host) + p := testAzureProvider(host, options.AzureOptions{}) session := CreateAuthorizedSession() session.Email = testCase.Email err := p.EnrichSession(context.Background(), session) @@ -323,7 +324,7 @@ func TestAzureProviderRedeem(t *testing.T) { defer b.Close() bURL, _ := url.Parse(b.URL) - p := testAzureProvider(bURL.Host) + p := testAzureProvider(bURL.Host, options.AzureOptions{}) p.Data().RedeemURL.Path = "/common/oauth2/token" s, err := p.Redeem(context.Background(), "https://localhost", "1234") if testCase.InjectRedeemURLError { @@ -345,7 +346,7 @@ func TestAzureProviderRedeem(t *testing.T) { } func TestAzureProviderProtectedResourceConfigured(t *testing.T) { - p := testAzureProvider("") + p := testAzureProvider("", options.AzureOptions{}) p.ProtectedResource, _ = url.Parse("http://my.resource.test") result := p.GetLoginURL("https://my.test.app/oauth", "", "") assert.Contains(t, result, "resource="+url.QueryEscape("http://my.resource.test")) @@ -381,7 +382,7 @@ func TestAzureProviderRefresh(t *testing.T) { b := testAzureBackend(string(payloadBytes), newAccessToken, refreshToken) defer b.Close() bURL, _ := url.Parse(b.URL) - p := testAzureProvider(bURL.Host) + p := testAzureProvider(bURL.Host, options.AzureOptions{}) expires := time.Now().Add(time.Duration(-1) * time.Hour) session := &sessions.SessionState{AccessToken: "some_access_token", RefreshToken: refreshToken, IDToken: "some_id_token", ExpiresOn: &expires} diff --git a/providers/bitbucket.go b/providers/bitbucket.go index 2612a4b..75cd292 100644 --- a/providers/bitbucket.go +++ b/providers/bitbucket.go @@ -5,6 +5,7 @@ import ( "net/url" "strings" + "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/apis/options" "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/apis/sessions" "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/logger" "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/requests" @@ -53,7 +54,7 @@ var ( ) // NewBitbucketProvider initiates a new BitbucketProvider -func NewBitbucketProvider(p *ProviderData) *BitbucketProvider { +func NewBitbucketProvider(p *ProviderData, opts options.BitbucketOptions) *BitbucketProvider { p.setProviderDefaults(providerDefaults{ name: bitbucketProviderName, loginURL: bitbucketDefaultLoginURL, @@ -62,19 +63,28 @@ func NewBitbucketProvider(p *ProviderData) *BitbucketProvider { validateURL: bitbucketDefaultValidateURL, scope: bitbucketDefaultScope, }) - return &BitbucketProvider{ProviderData: p} + + provider := &BitbucketProvider{ProviderData: p} + + if opts.Team != "" { + provider.setTeam(opts.Team) + } + if opts.Repository != "" { + provider.setRepository(opts.Repository) + } + return provider } -// SetTeam defines the Bitbucket team the user must be part of -func (p *BitbucketProvider) SetTeam(team string) { +// setTeam defines the Bitbucket team the user must be part of +func (p *BitbucketProvider) setTeam(team string) { p.Team = team if !strings.Contains(p.Scope, "team") { p.Scope += " team" } } -// SetRepository defines the repository the user must have access to -func (p *BitbucketProvider) SetRepository(repository string) { +// setRepository defines the repository the user must have access to +func (p *BitbucketProvider) setRepository(repository string) { p.Repository = repository if !strings.Contains(p.Scope, "repository") { p.Scope += " repository" diff --git a/providers/bitbucket_test.go b/providers/bitbucket_test.go index 22ceda5..3502edc 100644 --- a/providers/bitbucket_test.go +++ b/providers/bitbucket_test.go @@ -8,6 +8,7 @@ import ( "net/url" "testing" + "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/apis/options" "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/apis/sessions" . "github.com/onsi/gomega" "github.com/stretchr/testify/assert" @@ -21,15 +22,12 @@ func testBitbucketProvider(hostname, team string, repository string) *BitbucketP RedeemURL: &url.URL{}, ProfileURL: &url.URL{}, ValidateURL: &url.URL{}, - Scope: ""}) - - if team != "" { - p.SetTeam(team) - } - - if repository != "" { - p.SetRepository(repository) - } + Scope: ""}, + options.BitbucketOptions{ + Team: team, + Repository: repository, + }, + ) if hostname != "" { updateURL(p.Data().LoginURL, hostname) @@ -65,7 +63,7 @@ func TestNewBitbucketProvider(t *testing.T) { g := NewWithT(t) // Test that defaults are set when calling for a new provider with nothing set - providerData := NewBitbucketProvider(&ProviderData{}).Data() + providerData := NewBitbucketProvider(&ProviderData{}, options.BitbucketOptions{}).Data() g.Expect(providerData.ProviderName).To(Equal("Bitbucket")) g.Expect(providerData.LoginURL.String()).To(Equal("https://bitbucket.org/site/oauth2/authorize")) g.Expect(providerData.RedeemURL.String()).To(Equal("https://bitbucket.org/site/oauth2/access_token")) @@ -101,7 +99,8 @@ func TestBitbucketProviderOverrides(t *testing.T) { Scheme: "https", Host: "example.com", Path: "/api/v3/user"}, - Scope: "profile"}) + Scope: "profile"}, + options.BitbucketOptions{}) assert.NotEqual(t, nil, p) assert.Equal(t, "Bitbucket", p.Data().ProviderName) assert.Equal(t, "https://example.com/oauth/auth", diff --git a/providers/github.go b/providers/github.go index 064329c..a357285 100644 --- a/providers/github.go +++ b/providers/github.go @@ -11,6 +11,7 @@ import ( "strconv" "strings" + "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/apis/options" "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/apis/sessions" "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/logger" "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/requests" @@ -62,7 +63,7 @@ var ( ) // NewGitHubProvider initiates a new GitHubProvider -func NewGitHubProvider(p *ProviderData) *GitHubProvider { +func NewGitHubProvider(p *ProviderData, opts options.GitHubOptions) *GitHubProvider { p.setProviderDefaults(providerDefaults{ name: githubProviderName, loginURL: githubDefaultLoginURL, @@ -71,7 +72,13 @@ func NewGitHubProvider(p *ProviderData) *GitHubProvider { validateURL: githubDefaultValidateURL, scope: githubDefaultScope, }) - return &GitHubProvider{ProviderData: p} + + provider := &GitHubProvider{ProviderData: p} + + provider.setOrgTeam(opts.Org, opts.Team) + provider.setRepo(opts.Repo, opts.Token) + provider.setUsers(opts.Users) + return provider } func makeGitHubHeader(accessToken string) http.Header { @@ -82,8 +89,8 @@ func makeGitHubHeader(accessToken string) http.Header { return makeAuthorizationHeader(tokenTypeToken, accessToken, extraHeaders) } -// SetOrgTeam adds GitHub org reading parameters to the OAuth2 scope -func (p *GitHubProvider) SetOrgTeam(org, team string) { +// setOrgTeam adds GitHub org reading parameters to the OAuth2 scope +func (p *GitHubProvider) setOrgTeam(org, team string) { p.Org = org p.Team = team if org != "" || team != "" { @@ -91,14 +98,14 @@ func (p *GitHubProvider) SetOrgTeam(org, team string) { } } -// SetRepo configures the target repository and optional token to use -func (p *GitHubProvider) SetRepo(repo, token string) { +// setRepo configures the target repository and optional token to use +func (p *GitHubProvider) setRepo(repo, token string) { p.Repo = repo p.Token = token } -// SetUsers configures allowed usernames -func (p *GitHubProvider) SetUsers(users []string) { +// setUsers configures allowed usernames +func (p *GitHubProvider) setUsers(users []string) { p.Users = users } diff --git a/providers/github_test.go b/providers/github_test.go index d19aaa3..6ef3197 100644 --- a/providers/github_test.go +++ b/providers/github_test.go @@ -7,12 +7,13 @@ import ( "net/url" "testing" + "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/apis/options" "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/apis/sessions" . "github.com/onsi/gomega" "github.com/stretchr/testify/assert" ) -func testGitHubProvider(hostname string) *GitHubProvider { +func testGitHubProvider(hostname string, opts options.GitHubOptions) *GitHubProvider { p := NewGitHubProvider( &ProviderData{ ProviderName: "", @@ -20,7 +21,8 @@ func testGitHubProvider(hostname string) *GitHubProvider { RedeemURL: &url.URL{}, ProfileURL: &url.URL{}, ValidateURL: &url.URL{}, - Scope: ""}) + Scope: ""}, + opts) if hostname != "" { updateURL(p.Data().LoginURL, hostname) updateURL(p.Data().RedeemURL, hostname) @@ -71,7 +73,7 @@ func TestNewGitHubProvider(t *testing.T) { g := NewWithT(t) // Test that defaults are set when calling for a new provider with nothing set - providerData := NewGitHubProvider(&ProviderData{}).Data() + providerData := NewGitHubProvider(&ProviderData{}, options.GitHubOptions{}).Data() g.Expect(providerData.ProviderName).To(Equal("GitHub")) g.Expect(providerData.LoginURL.String()).To(Equal("https://github.com/login/oauth/authorize")) g.Expect(providerData.RedeemURL.String()).To(Equal("https://github.com/login/oauth/access_token")) @@ -95,7 +97,8 @@ func TestGitHubProviderOverrides(t *testing.T) { Scheme: "https", Host: "api.example.com", Path: "/"}, - Scope: "profile"}) + Scope: "profile"}, + options.GitHubOptions{}) assert.NotEqual(t, nil, p) assert.Equal(t, "GitHub", p.Data().ProviderName) assert.Equal(t, "https://example.com/login/oauth/authorize", @@ -114,7 +117,7 @@ func TestGitHubProvider_getEmail(t *testing.T) { defer b.Close() bURL, _ := url.Parse(b.URL) - p := testGitHubProvider(bURL.Host) + p := testGitHubProvider(bURL.Host, options.GitHubOptions{}) session := CreateAuthorizedSession() err := p.getEmail(context.Background(), session) @@ -129,7 +132,7 @@ func TestGitHubProvider_getEmailNotVerified(t *testing.T) { defer b.Close() bURL, _ := url.Parse(b.URL) - p := testGitHubProvider(bURL.Host) + p := testGitHubProvider(bURL.Host, options.GitHubOptions{}) session := CreateAuthorizedSession() err := p.getEmail(context.Background(), session) @@ -149,8 +152,11 @@ func TestGitHubProvider_getEmailWithOrg(t *testing.T) { defer b.Close() bURL, _ := url.Parse(b.URL) - p := testGitHubProvider(bURL.Host) - p.Org = "testorg1" + p := testGitHubProvider(bURL.Host, + options.GitHubOptions{ + Org: "testorg1", + }, + ) session := CreateAuthorizedSession() err := p.getEmail(context.Background(), session) @@ -166,8 +172,12 @@ func TestGitHubProvider_getEmailWithWriteAccessToPublicRepo(t *testing.T) { defer b.Close() bURL, _ := url.Parse(b.URL) - p := testGitHubProvider(bURL.Host) - p.SetRepo("oauth2-proxy/oauth2-proxy", "") + p := testGitHubProvider(bURL.Host, + options.GitHubOptions{ + Repo: "oauth2-proxy/oauth2-proxy", + Token: "token", + }, + ) session := CreateAuthorizedSession() err := p.getEmail(context.Background(), session) @@ -183,8 +193,12 @@ func TestGitHubProvider_getEmailWithReadOnlyAccessToPrivateRepo(t *testing.T) { defer b.Close() bURL, _ := url.Parse(b.URL) - p := testGitHubProvider(bURL.Host) - p.SetRepo("oauth2-proxy/oauth2-proxy", "") + p := testGitHubProvider(bURL.Host, + options.GitHubOptions{ + Repo: "oauth2-proxy/oauth2-proxy", + Token: "token", + }, + ) session := CreateAuthorizedSession() err := p.getEmail(context.Background(), session) @@ -200,8 +214,12 @@ func TestGitHubProvider_getEmailWithWriteAccessToPrivateRepo(t *testing.T) { defer b.Close() bURL, _ := url.Parse(b.URL) - p := testGitHubProvider(bURL.Host) - p.SetRepo("oauth2-proxy/oauth2-proxy", "") + p := testGitHubProvider(bURL.Host, + options.GitHubOptions{ + Repo: "oauth2-proxy/oauth2-proxy", + Token: "token", + }, + ) session := CreateAuthorizedSession() err := p.getEmail(context.Background(), session) @@ -216,8 +234,11 @@ func TestGitHubProvider_getEmailWithNoAccessToPrivateRepo(t *testing.T) { defer b.Close() bURL, _ := url.Parse(b.URL) - p := testGitHubProvider(bURL.Host) - p.SetRepo("oauth2-proxy/oauth2-proxy", "") + p := testGitHubProvider(bURL.Host, + options.GitHubOptions{ + Repo: "oauth2-proxy/oauth2-proxy", + }, + ) session := CreateAuthorizedSession() err := p.getEmail(context.Background(), session) @@ -232,8 +253,12 @@ func TestGitHubProvider_getEmailWithToken(t *testing.T) { defer b.Close() bURL, _ := url.Parse(b.URL) - p := testGitHubProvider(bURL.Host) - p.SetRepo("oauth2-proxy/oauth2-proxy", "token") + p := testGitHubProvider(bURL.Host, + options.GitHubOptions{ + Repo: "oauth2-proxy/oauth2-proxy", + Token: "token", + }, + ) session := CreateAuthorizedSession() err := p.getEmail(context.Background(), session) @@ -248,7 +273,7 @@ func TestGitHubProvider_getEmailFailedRequest(t *testing.T) { defer b.Close() bURL, _ := url.Parse(b.URL) - p := testGitHubProvider(bURL.Host) + p := testGitHubProvider(bURL.Host, options.GitHubOptions{}) // We'll trigger a request failure by using an unexpected access // token. Alternatively, we could allow the parsing of the payload as @@ -266,7 +291,7 @@ func TestGitHubProvider_getEmailNotPresentInPayload(t *testing.T) { defer b.Close() bURL, _ := url.Parse(b.URL) - p := testGitHubProvider(bURL.Host) + p := testGitHubProvider(bURL.Host, options.GitHubOptions{}) session := CreateAuthorizedSession() err := p.getEmail(context.Background(), session) @@ -281,7 +306,7 @@ func TestGitHubProvider_getUser(t *testing.T) { defer b.Close() bURL, _ := url.Parse(b.URL) - p := testGitHubProvider(bURL.Host) + p := testGitHubProvider(bURL.Host, options.GitHubOptions{}) session := CreateAuthorizedSession() err := p.getUser(context.Background(), session) @@ -297,8 +322,12 @@ func TestGitHubProvider_getUserWithRepoAndToken(t *testing.T) { defer b.Close() bURL, _ := url.Parse(b.URL) - p := testGitHubProvider(bURL.Host) - p.SetRepo("oauth2-proxy/oauth2-proxy", "token") + p := testGitHubProvider(bURL.Host, + options.GitHubOptions{ + Repo: "oauth2-proxy/oauth2-proxy", + Token: "token", + }, + ) session := CreateAuthorizedSession() err := p.getUser(context.Background(), session) @@ -311,8 +340,12 @@ func TestGitHubProvider_getUserWithRepoAndTokenWithoutPushAccess(t *testing.T) { defer b.Close() bURL, _ := url.Parse(b.URL) - p := testGitHubProvider(bURL.Host) - p.SetRepo("oauth2-proxy/oauth2-proxy", "token") + p := testGitHubProvider(bURL.Host, + options.GitHubOptions{ + Repo: "oauth2-proxy/oauth2-proxy", + Token: "token", + }, + ) session := CreateAuthorizedSession() err := p.getUser(context.Background(), session) @@ -328,8 +361,11 @@ func TestGitHubProvider_getEmailWithUsername(t *testing.T) { defer b.Close() bURL, _ := url.Parse(b.URL) - p := testGitHubProvider(bURL.Host) - p.SetUsers([]string{"mbland", "octocat"}) + p := testGitHubProvider(bURL.Host, + options.GitHubOptions{ + Users: []string{"mbland", "octocat"}, + }, + ) session := CreateAuthorizedSession() err := p.getEmail(context.Background(), session) @@ -345,8 +381,11 @@ func TestGitHubProvider_getEmailWithNotAllowedUsername(t *testing.T) { defer b.Close() bURL, _ := url.Parse(b.URL) - p := testGitHubProvider(bURL.Host) - p.SetUsers([]string{"octocat"}) + p := testGitHubProvider(bURL.Host, + options.GitHubOptions{ + Users: []string{"octocat"}, + }, + ) session := CreateAuthorizedSession() err := p.getEmail(context.Background(), session) @@ -366,9 +405,12 @@ func TestGitHubProvider_getEmailWithUsernameAndNotBelongToOrg(t *testing.T) { defer b.Close() bURL, _ := url.Parse(b.URL) - p := testGitHubProvider(bURL.Host) - p.SetOrgTeam("not_belong_to", "") - p.SetUsers([]string{"mbland"}) + p := testGitHubProvider(bURL.Host, + options.GitHubOptions{ + Org: "not_belog_to", + Users: []string{"mbland"}, + }, + ) session := CreateAuthorizedSession() err := p.getEmail(context.Background(), session) @@ -385,9 +427,13 @@ func TestGitHubProvider_getEmailWithUsernameAndNoAccessToPrivateRepo(t *testing. defer b.Close() bURL, _ := url.Parse(b.URL) - p := testGitHubProvider(bURL.Host) - p.SetRepo("oauth2-proxy/oauth2-proxy", "") - p.SetUsers([]string{"mbland"}) + p := testGitHubProvider(bURL.Host, + options.GitHubOptions{ + Repo: "oauth2-proxy/oauth2-proxy", + Token: "token", + Users: []string{"mbland"}, + }, + ) session := CreateAuthorizedSession() err := p.getEmail(context.Background(), session) diff --git a/providers/gitlab.go b/providers/gitlab.go index b73e19d..1049a40 100644 --- a/providers/gitlab.go +++ b/providers/gitlab.go @@ -7,6 +7,7 @@ import ( "strconv" "strings" + "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/apis/options" "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/apis/sessions" "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/logger" "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/requests" @@ -30,7 +31,7 @@ type GitLabProvider struct { var _ Provider = (*GitLabProvider)(nil) // NewGitLabProvider initiates a new GitLabProvider -func NewGitLabProvider(p *ProviderData) *GitLabProvider { +func NewGitLabProvider(p *ProviderData, opts options.GitLabOptions) (*GitLabProvider, error) { p.ProviderName = gitlabProviderName if p.Scope == "" { p.Scope = gitlabDefaultScope @@ -41,15 +42,22 @@ func NewGitLabProvider(p *ProviderData) *GitLabProvider { SkipNonce: false, } - return &GitLabProvider{ + provider := &GitLabProvider{ OIDCProvider: oidcProvider, oidcRefreshFunc: oidcProvider.RefreshSession, } + provider.setAllowedGroups(opts.Group) + + if err := provider.setAllowedProjects(opts.Projects); err != nil { + return nil, fmt.Errorf("could not configure allowed projects: %v", err) + } + + return provider, nil } -// SetAllowedProjects adds Gitlab projects to the AllowedGroups list +// setAllowedProjects adds Gitlab projects to the AllowedGroups list // and tracks them to do a project API lookup during `EnrichSession`. -func (p *GitLabProvider) SetAllowedProjects(projects []string) error { +func (p *GitLabProvider) setAllowedProjects(projects []string) error { for _, project := range projects { gp, err := newGitlabProject(project) if err != nil { diff --git a/providers/gitlab_test.go b/providers/gitlab_test.go index d6635c2..9510701 100644 --- a/providers/gitlab_test.go +++ b/providers/gitlab_test.go @@ -7,21 +7,26 @@ import ( "net/http/httptest" "net/url" + "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/apis/options" "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/apis/sessions" . "github.com/onsi/ginkgo" . "github.com/onsi/ginkgo/extensions/table" . "github.com/onsi/gomega" ) -func testGitLabProvider(hostname string) *GitLabProvider { - p := NewGitLabProvider( +func testGitLabProvider(hostname, scope string, opts options.GitLabOptions) (*GitLabProvider, error) { + p, err := NewGitLabProvider( &ProviderData{ ProviderName: "", LoginURL: &url.URL{}, RedeemURL: &url.URL{}, ProfileURL: &url.URL{}, ValidateURL: &url.URL{}, - Scope: ""}) + Scope: scope}, + opts) + if err != nil { + return nil, err + } if hostname != "" { updateURL(p.Data().LoginURL, hostname) updateURL(p.Data().RedeemURL, hostname) @@ -29,7 +34,7 @@ func testGitLabProvider(hostname string) *GitLabProvider { updateURL(p.Data().ValidateURL, hostname) } - return p + return p, err } func testGitLabBackend() *httptest.Server { @@ -157,7 +162,8 @@ var _ = Describe("Gitlab Provider Tests", func() { bURL, err := url.Parse(b.URL) Expect(err).To(BeNil()) - p = testGitLabProvider(bURL.Host) + p, err = testGitLabProvider(bURL.Host, "", options.GitLabOptions{}) + Expect(err).ToNot(HaveOccurred()) }) AfterEach(func() { @@ -219,21 +225,23 @@ var _ = Describe("Gitlab Provider Tests", func() { DescribeTable("should return expected results", func(in entitiesTableInput) { - p.AllowUnverifiedEmail = true - if in.scope != "" { - p.Scope = in.scope - } - session := &sessions.SessionState{AccessToken: "gitlab_access_token"} + bURL, err := url.Parse(b.URL) + Expect(err).To(BeNil()) - p.SetAllowedGroups(in.allowedGroups) - - err := p.SetAllowedProjects(in.allowedProjects) + p, err := testGitLabProvider(bURL.Host, in.scope, options.GitLabOptions{ + Group: in.allowedGroups, + Projects: in.allowedProjects, + }) if in.expectedError == nil { Expect(err).To(BeNil()) } else { Expect(err).To(MatchError(in.expectedError)) return } + + p.AllowUnverifiedEmail = true + session := &sessions.SessionState{AccessToken: "gitlab_access_token"} + Expect(p.Scope).To(Equal(in.expectedScope)) err = p.EnrichSession(context.Background(), session) @@ -302,7 +310,7 @@ var _ = Describe("Gitlab Provider Tests", func() { }), Entry("invalid project format", entitiesTableInput{ allowedProjects: []string{"my_group/my_invalid_project=123"}, - expectedError: errors.New("invalid gitlab project access level specified (my_group/my_invalid_project)"), + expectedError: errors.New("could not configure allowed projects: invalid gitlab project access level specified (my_group/my_invalid_project)"), expectedScope: "openid email read_api", }), ) diff --git a/providers/google.go b/providers/google.go index a467c50..53e4c70 100644 --- a/providers/google.go +++ b/providers/google.go @@ -10,9 +10,11 @@ import ( "io" "io/ioutil" "net/url" + "os" "strings" "time" + "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/apis/options" "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/apis/sessions" "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/logger" "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/requests" @@ -79,7 +81,7 @@ var ( ) // NewGoogleProvider initiates a new GoogleProvider -func NewGoogleProvider(p *ProviderData) *GoogleProvider { +func NewGoogleProvider(p *ProviderData, opts options.GoogleOptions) (*GoogleProvider, error) { p.setProviderDefaults(providerDefaults{ name: googleProviderName, loginURL: googleDefaultLoginURL, @@ -88,7 +90,7 @@ func NewGoogleProvider(p *ProviderData) *GoogleProvider { validateURL: googleDefaultValidateURL, scope: googleDefaultScope, }) - return &GoogleProvider{ + provider := &GoogleProvider{ ProviderData: p, // Set a default groupValidator to just always return valid (true), it will // be overwritten if we configured a Google group restriction. @@ -96,6 +98,21 @@ func NewGoogleProvider(p *ProviderData) *GoogleProvider { return true }, } + + if opts.ServiceAccountJSON != "" { + file, err := os.Open(opts.ServiceAccountJSON) + if err != nil { + return nil, fmt.Errorf("invalid Google credentials file: %s", opts.ServiceAccountJSON) + } + + // Backwards compatibility with `--google-group` option + if len(opts.Groups) > 0 { + provider.setAllowedGroups(opts.Groups) + } + provider.setGroupRestriction(opts.Groups, opts.AdminEmail, file) + } + + return provider, nil } func claimsFromIDToken(idToken string) (*claims, error) { @@ -195,7 +212,7 @@ func (p *GoogleProvider) EnrichSession(_ context.Context, s *sessions.SessionSta // account credentials. // // TODO (@NickMeves) - Unit Test this OR refactor away from groupValidator func -func (p *GoogleProvider) SetGroupRestriction(groups []string, adminEmail string, credentialsReader io.Reader) { +func (p *GoogleProvider) setGroupRestriction(groups []string, adminEmail string, credentialsReader io.Reader) { adminService := getAdminService(adminEmail, credentialsReader) p.groupValidator = func(s *sessions.SessionState) bool { // Reset our saved Groups in case membership changed diff --git a/providers/google_test.go b/providers/google_test.go index 458439d..3ff3c60 100644 --- a/providers/google_test.go +++ b/providers/google_test.go @@ -10,6 +10,7 @@ import ( "net/url" "testing" + "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/apis/options" "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/apis/sessions" . "github.com/onsi/gomega" "github.com/stretchr/testify/assert" @@ -25,22 +26,29 @@ func newRedeemServer(body []byte) (*url.URL, *httptest.Server) { return u, s } -func newGoogleProvider() *GoogleProvider { - return NewGoogleProvider( +func newGoogleProvider(t *testing.T) *GoogleProvider { + g := NewWithT(t) + p, err := NewGoogleProvider( &ProviderData{ ProviderName: "", LoginURL: &url.URL{}, RedeemURL: &url.URL{}, ProfileURL: &url.URL{}, ValidateURL: &url.URL{}, - Scope: ""}) + Scope: ""}, + options.GoogleOptions{}) + g.Expect(err).ToNot(HaveOccurred()) + return p } func TestNewGoogleProvider(t *testing.T) { g := NewWithT(t) // Test that defaults are set when calling for a new provider with nothing set - providerData := NewGoogleProvider(&ProviderData{}).Data() + provider, err := NewGoogleProvider(&ProviderData{}, options.GoogleOptions{}) + g.Expect(err).ToNot(HaveOccurred()) + providerData := provider.Data() + g.Expect(providerData.ProviderName).To(Equal("Google")) g.Expect(providerData.LoginURL.String()).To(Equal("https://accounts.google.com/o/oauth2/auth?access_type=offline")) g.Expect(providerData.RedeemURL.String()).To(Equal("https://www.googleapis.com/oauth2/v3/token")) @@ -50,7 +58,7 @@ func TestNewGoogleProvider(t *testing.T) { } func TestGoogleProviderOverrides(t *testing.T) { - p := NewGoogleProvider( + p, err := NewGoogleProvider( &ProviderData{ LoginURL: &url.URL{ Scheme: "https", @@ -68,7 +76,9 @@ func TestGoogleProviderOverrides(t *testing.T) { Scheme: "https", Host: "example.com", Path: "/oauth/tokeninfo"}, - Scope: "profile"}) + Scope: "profile"}, + options.GoogleOptions{}) + assert.NoError(t, err) assert.NotEqual(t, nil, p) assert.Equal(t, "Google", p.Data().ProviderName) assert.Equal(t, "https://example.com/oauth/auth", @@ -90,7 +100,7 @@ type redeemResponse struct { } func TestGoogleProviderGetEmailAddress(t *testing.T) { - p := newGoogleProvider() + p := newGoogleProvider(t) body, err := json.Marshal(redeemResponse{ AccessToken: "a1234", ExpiresIn: 10, @@ -147,7 +157,7 @@ func TestGoogleProviderGroupValidator(t *testing.T) { for name, tc := range testCases { t.Run(name, func(t *testing.T) { g := NewWithT(t) - p := newGoogleProvider() + p := newGoogleProvider(t) if tc.validatorFunc != nil { p.groupValidator = tc.validatorFunc } @@ -158,7 +168,7 @@ func TestGoogleProviderGroupValidator(t *testing.T) { // func TestGoogleProviderGetEmailAddressInvalidEncoding(t *testing.T) { - p := newGoogleProvider() + p := newGoogleProvider(t) body, err := json.Marshal(redeemResponse{ AccessToken: "a1234", IDToken: "ignored prefix." + `{"email": "michael.bland@gsa.gov"}`, @@ -176,7 +186,7 @@ func TestGoogleProviderGetEmailAddressInvalidEncoding(t *testing.T) { } func TestGoogleProviderRedeemFailsNoCLientSecret(t *testing.T) { - p := newGoogleProvider() + p := newGoogleProvider(t) p.ProviderData.ClientSecretFile = "srvnoerre" session, err := p.Redeem(context.Background(), "http://redirect/", "code1234") @@ -188,7 +198,7 @@ func TestGoogleProviderRedeemFailsNoCLientSecret(t *testing.T) { } func TestGoogleProviderGetEmailAddressInvalidJson(t *testing.T) { - p := newGoogleProvider() + p := newGoogleProvider(t) body, err := json.Marshal(redeemResponse{ AccessToken: "a1234", @@ -208,7 +218,7 @@ func TestGoogleProviderGetEmailAddressInvalidJson(t *testing.T) { } func TestGoogleProviderGetEmailAddressEmailMissing(t *testing.T) { - p := newGoogleProvider() + p := newGoogleProvider(t) body, err := json.Marshal(redeemResponse{ AccessToken: "a1234", IDToken: "ignored prefix." + base64.URLEncoding.EncodeToString([]byte(`{"not_email": "missing"}`)), diff --git a/providers/keycloak.go b/providers/keycloak.go index a1e4f06..c1a8735 100644 --- a/providers/keycloak.go +++ b/providers/keycloak.go @@ -5,6 +5,7 @@ import ( "fmt" "net/url" + "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/apis/options" "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/apis/sessions" "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/logger" "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/requests" @@ -48,7 +49,7 @@ var ( ) // NewKeycloakProvider creates a KeyCloakProvider using the passed ProviderData -func NewKeycloakProvider(p *ProviderData) *KeycloakProvider { +func NewKeycloakProvider(p *ProviderData, opts options.KeycloakOptions) *KeycloakProvider { p.setProviderDefaults(providerDefaults{ name: keycloakProviderName, loginURL: keycloakDefaultLoginURL, @@ -57,7 +58,10 @@ func NewKeycloakProvider(p *ProviderData) *KeycloakProvider { validateURL: keycloakDefaultValidateURL, scope: keycloakDefaultScope, }) - return &KeycloakProvider{ProviderData: p} + + provider := &KeycloakProvider{ProviderData: p} + provider.setAllowedGroups(opts.Groups) + return provider } // EnrichSession uses the Keycloak userinfo endpoint to populate the session's diff --git a/providers/keycloak_oidc.go b/providers/keycloak_oidc.go index cb1971d..e16b438 100644 --- a/providers/keycloak_oidc.go +++ b/providers/keycloak_oidc.go @@ -4,6 +4,7 @@ import ( "context" "fmt" + "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/apis/options" "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/apis/sessions" ) @@ -15,21 +16,25 @@ type KeycloakOIDCProvider struct { } // NewKeycloakOIDCProvider makes a KeycloakOIDCProvider using the ProviderData -func NewKeycloakOIDCProvider(p *ProviderData) *KeycloakOIDCProvider { +func NewKeycloakOIDCProvider(p *ProviderData, opts options.KeycloakOptions) *KeycloakOIDCProvider { p.ProviderName = keycloakOIDCProviderName - return &KeycloakOIDCProvider{ + + provider := &KeycloakOIDCProvider{ OIDCProvider: &OIDCProvider{ ProviderData: p, }, } + + provider.addAllowedRoles(opts.Roles) + return provider } var _ Provider = (*KeycloakOIDCProvider)(nil) -// AddAllowedRoles sets Keycloak roles that are authorized. +// addAllowedRoles sets Keycloak roles that are authorized. // Assumes `SetAllowedGroups` is already called on groups and appends to that // with `role:` prefixed roles. -func (p *KeycloakOIDCProvider) AddAllowedRoles(roles []string) { +func (p *KeycloakOIDCProvider) addAllowedRoles(roles []string) { if p.AllowedGroups == nil { p.AllowedGroups = make(map[string]struct{}) } diff --git a/providers/keycloak_oidc_test.go b/providers/keycloak_oidc_test.go index bc13d5c..1e1cb73 100644 --- a/providers/keycloak_oidc_test.go +++ b/providers/keycloak_oidc_test.go @@ -9,6 +9,7 @@ import ( "github.com/coreos/go-oidc/v3/oidc" + "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/apis/options" "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/apis/sessions" . "github.com/onsi/ginkgo" . "github.com/onsi/gomega" @@ -39,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) + provider := newKeycloakOIDCProvider(redeemURL, options.KeycloakOptions{}) return server, provider } -func newKeycloakOIDCProvider(serverURL *url.URL) *KeycloakOIDCProvider { +func newKeycloakOIDCProvider(serverURL *url.URL, opts options.KeycloakOptions) *KeycloakOIDCProvider { verificationOptions := &internaloidc.IDTokenVerificationOptions{ AudienceClaims: []string{defaultAudienceClaim}, ClientID: mockClientID, @@ -66,7 +67,8 @@ func newKeycloakOIDCProvider(serverURL *url.URL) *KeycloakOIDCProvider { Scheme: "https", Host: "keycloak-oidc.com", Path: "/api/v3/user"}, - Scope: "openid email profile"}) + Scope: "openid email profile"}, + opts) if serverURL != nil { p.RedeemURL.Scheme = serverURL.Scheme @@ -88,7 +90,7 @@ func newKeycloakOIDCProvider(serverURL *url.URL) *KeycloakOIDCProvider { 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) + p := newKeycloakOIDCProvider(nil, options.KeycloakOptions{}) providerData := p.Data() Expect(providerData.ProviderName).To(Equal(keycloakOIDCProviderName)) Expect(providerData.LoginURL.String()).To(Equal("https://keycloak-oidc.com/oauth/auth")) @@ -101,8 +103,9 @@ 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) - p.AddAllowedRoles([]string{"admin", "editor"}) + p := newKeycloakOIDCProvider(nil, options.KeycloakOptions{ + Roles: []string{"admin", "editor"}, + }) Expect(p.AllowedGroups).To(HaveKey("role:admin")) Expect(p.AllowedGroups).To(HaveKey("role:editor")) }) diff --git a/providers/keycloak_test.go b/providers/keycloak_test.go index 513be64..b6d8b42 100644 --- a/providers/keycloak_test.go +++ b/providers/keycloak_test.go @@ -8,6 +8,7 @@ import ( "net/http/httptest" "net/url" + "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/apis/options" "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/apis/sessions" . "github.com/onsi/ginkgo" . "github.com/onsi/ginkgo/extensions/table" @@ -27,7 +28,8 @@ func testKeycloakProvider(backend *httptest.Server) (*KeycloakProvider, error) { RedeemURL: &url.URL{}, ProfileURL: &url.URL{}, ValidateURL: &url.URL{}, - Scope: ""}) + Scope: ""}, + options.KeycloakOptions{}) if backend != nil { bURL, err := url.Parse(backend.URL) @@ -48,7 +50,7 @@ func testKeycloakProvider(backend *httptest.Server) (*KeycloakProvider, error) { var _ = Describe("Keycloak Provider Tests", func() { Context("New Provider Init", func() { It("uses defaults", func() { - providerData := NewKeycloakProvider(&ProviderData{}).Data() + providerData := NewKeycloakProvider(&ProviderData{}, options.KeycloakOptions{}).Data() Expect(providerData.ProviderName).To(Equal("Keycloak")) Expect(providerData.LoginURL.String()).To(Equal("https://keycloak.org/oauth/authorize")) Expect(providerData.RedeemURL.String()).To(Equal("https://keycloak.org/oauth/token")) @@ -76,7 +78,8 @@ var _ = Describe("Keycloak Provider Tests", func() { Scheme: "https", Host: "example.com", Path: "/api/v3/user"}, - Scope: "profile"}) + Scope: "profile"}, + options.KeycloakOptions{}) providerData := p.Data() Expect(providerData.ProviderName).To(Equal("Keycloak")) diff --git a/providers/logingov.go b/providers/logingov.go index 103fdf1..117d273 100644 --- a/providers/logingov.go +++ b/providers/logingov.go @@ -5,12 +5,15 @@ import ( "context" "crypto/rand" "crypto/rsa" + "errors" "fmt" + "io/ioutil" "math/big" "net/url" "time" "github.com/golang-jwt/jwt" + "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/apis/options" "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/apis/sessions" "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/requests" "gopkg.in/square/go-jose.v2" @@ -78,7 +81,7 @@ var ( ) // NewLoginGovProvider initiates a new LoginGovProvider -func NewLoginGovProvider(p *ProviderData) *LoginGovProvider { +func NewLoginGovProvider(p *ProviderData, opts options.LoginGovOptions) (*LoginGovProvider, error) { p.setProviderDefaults(providerDefaults{ name: loginGovProviderName, loginURL: loginGovDefaultLoginURL, @@ -87,10 +90,50 @@ func NewLoginGovProvider(p *ProviderData) *LoginGovProvider { validateURL: loginGovDefaultProfileURL, scope: loginGovDefaultScope, }) - return &LoginGovProvider{ + provider := &LoginGovProvider{ ProviderData: p, Nonce: randSeq(32), } + + if err := provider.configure(opts); err != nil { + return nil, fmt.Errorf("could not configure login.gov provider: %v", err) + } + return provider, nil +} + +func (p *LoginGovProvider) configure(opts options.LoginGovOptions) error { + pubJWKURL, err := url.Parse(opts.PubJWKURL) + if err != nil { + return fmt.Errorf("could not parse Public JWK URL: %v", err) + } + p.PubJWKURL = pubJWKURL + + // JWT key can be supplied via env variable or file in the filesystem, but not both. + switch { + case opts.JWTKey != "" && opts.JWTKeyFile != "": + return errors.New("cannot set both jwt-key and jwt-key-file options") + case opts.JWTKey == "" && opts.JWTKeyFile == "": + return errors.New("login.gov provider requires a private key for signing JWTs") + case opts.JWTKey != "": + // The JWT Key is in the commandline argument + signKey, err := jwt.ParseRSAPrivateKeyFromPEM([]byte(opts.JWTKey)) + if err != nil { + return fmt.Errorf("could not parse RSA Private Key PEM: %v", err) + } + p.JWTKey = signKey + case opts.JWTKeyFile != "": + // The JWT key is in the filesystem + keyData, err := ioutil.ReadFile(opts.JWTKeyFile) + if err != nil { + return fmt.Errorf("could not read key file: %v", opts.JWTKeyFile) + } + signKey, err := jwt.ParseRSAPrivateKeyFromPEM(keyData) + if err != nil { + return fmt.Errorf("could not parse private key from PEM file: %v", opts.JWTKeyFile) + } + p.JWTKey = signKey + } + return nil } type loginGovCustomClaims struct { diff --git a/providers/logingov_test.go b/providers/logingov_test.go index db30262..fe371c9 100644 --- a/providers/logingov_test.go +++ b/providers/logingov_test.go @@ -1,11 +1,14 @@ package providers import ( + "bytes" "context" "crypto" "crypto/rand" "crypto/rsa" + "crypto/x509" "encoding/json" + "encoding/pem" "net/http" "net/http/httptest" "net/url" @@ -13,6 +16,7 @@ import ( "time" "github.com/golang-jwt/jwt" + "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/apis/options" . "github.com/onsi/gomega" "github.com/stretchr/testify/assert" "gopkg.in/square/go-jose.v2" @@ -32,12 +36,35 @@ func newLoginGovServer(body []byte) (*url.URL, *httptest.Server) { return u, s } -func newLoginGovProvider() (l *LoginGovProvider, serverKey *MyKeyData, err error) { +func newPrivateKeyBytes() ([]byte, error) { + privateKey, err := rsa.GenerateKey(rand.Reader, 2048) + if err != nil { + return nil, err + } + + keyBytes, err := x509.MarshalPKCS8PrivateKey(privateKey) + if err != nil { + return nil, err + } + + privateKeyBlock := &pem.Block{ + Type: "RSA PRIVATE KEY", + Bytes: keyBytes, + } + b := &bytes.Buffer{} + if err := pem.Encode(b, privateKeyBlock); err != nil { + return nil, err + } + + return b.Bytes(), nil +} + +func newLoginGovProvider() (*LoginGovProvider, *MyKeyData, error) { key, err := rsa.GenerateKey(rand.Reader, 2048) if err != nil { - return + return nil, nil, err } - serverKey = &MyKeyData{ + serverKey := &MyKeyData{ PubKey: key.Public(), PrivKey: key, PubJWK: jose.JSONWebKey{ @@ -48,29 +75,40 @@ func newLoginGovProvider() (l *LoginGovProvider, serverKey *MyKeyData, err error }, } - privateKey, err := rsa.GenerateKey(rand.Reader, 2048) + privKey, err := newPrivateKeyBytes() if err != nil { - return + return nil, nil, err } - l = NewLoginGovProvider( + l, err := NewLoginGovProvider( &ProviderData{ ProviderName: "", LoginURL: &url.URL{}, RedeemURL: &url.URL{}, ProfileURL: &url.URL{}, ValidateURL: &url.URL{}, - Scope: ""}) - l.JWTKey = privateKey + Scope: ""}, + options.LoginGovOptions{ + JWTKey: string(privKey), + }, + ) l.Nonce = "fakenonce" - return + return l, serverKey, err } func TestNewLoginGovProvider(t *testing.T) { g := NewWithT(t) + privKey, err := newPrivateKeyBytes() + g.Expect(err).ToNot(HaveOccurred()) + // Test that defaults are set when calling for a new provider with nothing set - providerData := NewLoginGovProvider(&ProviderData{}).Data() + provider, err := NewLoginGovProvider(&ProviderData{}, options.LoginGovOptions{ + JWTKey: string(privKey), + }) + g.Expect(err).ToNot(HaveOccurred()) + + providerData := provider.Data() g.Expect(providerData.ProviderName).To(Equal("login.gov")) g.Expect(providerData.LoginURL.String()).To(Equal("https://secure.login.gov/openid_connect/authorize")) g.Expect(providerData.RedeemURL.String()).To(Equal("https://secure.login.gov/api/openid_connect/token")) @@ -80,7 +118,10 @@ func TestNewLoginGovProvider(t *testing.T) { } func TestLoginGovProviderOverrides(t *testing.T) { - p := NewLoginGovProvider( + privKey, err := newPrivateKeyBytes() + assert.NoError(t, err) + + p, err := NewLoginGovProvider( &ProviderData{ LoginURL: &url.URL{ Scheme: "https", @@ -94,7 +135,11 @@ func TestLoginGovProviderOverrides(t *testing.T) { Scheme: "https", Host: "example.com", Path: "/oauth/profile"}, - Scope: "profile"}) + Scope: "profile"}, + options.LoginGovOptions{ + JWTKey: string(privKey), + }) + assert.NoError(t, err) assert.NotEqual(t, nil, p) assert.Equal(t, "login.gov", p.Data().ProviderName) assert.Equal(t, "https://example.com/oauth/auth", diff --git a/providers/nextcloud.go b/providers/nextcloud.go index e915601..882fc63 100644 --- a/providers/nextcloud.go +++ b/providers/nextcloud.go @@ -1,5 +1,7 @@ package providers +import "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/apis/options" + // NextcloudProvider represents an Nextcloud based Identity Provider type NextcloudProvider struct { *ProviderData @@ -13,7 +15,7 @@ const nextCloudProviderName = "Nextcloud" func NewNextcloudProvider(p *ProviderData) *NextcloudProvider { p.ProviderName = nextCloudProviderName p.getAuthorizationHeaderFunc = makeOIDCHeader - if p.EmailClaim == OIDCEmailClaim { + if p.EmailClaim == options.OIDCEmailClaim { // This implies the email claim has not been overridden, we should set a default // for this provider p.EmailClaim = "ocs.data.email" diff --git a/providers/oidc.go b/providers/oidc.go index cccb8d7..50ea841 100644 --- a/providers/oidc.go +++ b/providers/oidc.go @@ -7,6 +7,7 @@ import ( "net/url" "time" + "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/apis/options" "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/apis/sessions" "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/logger" "golang.org/x/oauth2" @@ -20,13 +21,13 @@ type OIDCProvider struct { } // NewOIDCProvider initiates a new OIDCProvider -func NewOIDCProvider(p *ProviderData) *OIDCProvider { +func NewOIDCProvider(p *ProviderData, opts options.OIDCOptions) *OIDCProvider { p.ProviderName = "OpenID Connect" p.getAuthorizationHeaderFunc = makeOIDCHeader return &OIDCProvider{ ProviderData: p, - SkipNonce: true, + SkipNonce: opts.InsecureSkipNonce, } } diff --git a/providers/oidc_test.go b/providers/oidc_test.go index 1a98f46..b462de5 100644 --- a/providers/oidc_test.go +++ b/providers/oidc_test.go @@ -11,6 +11,7 @@ import ( "testing" "github.com/coreos/go-oidc/v3/oidc" + "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/apis/options" "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/apis/sessions" "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/encryption" internaloidc "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/oidc" @@ -25,7 +26,7 @@ type redeemTokenResponse struct { IDToken string `json:"id_token,omitempty"` } -func newOIDCProvider(serverURL *url.URL) *OIDCProvider { +func newOIDCProvider(serverURL *url.URL, skipNonce bool) *OIDCProvider { verificationOptions := &internaloidc.IDTokenVerificationOptions{ AudienceClaims: []string{"aud"}, ClientID: "https://test.myapp.com", @@ -61,7 +62,9 @@ func newOIDCProvider(serverURL *url.URL) *OIDCProvider { ), verificationOptions), } - p := NewOIDCProvider(providerData) + p := NewOIDCProvider(providerData, options.OIDCOptions{ + InsecureSkipNonce: skipNonce, + }) return p } @@ -77,7 +80,7 @@ func newOIDCServer(body []byte) (*url.URL, *httptest.Server) { func newTestOIDCSetup(body []byte) (*httptest.Server, *OIDCProvider) { redeemURL, server := newOIDCServer(body) - provider := newOIDCProvider(redeemURL) + provider := newOIDCProvider(redeemURL, false) return server, provider } @@ -86,7 +89,7 @@ func TestOIDCProviderGetLoginURL(t *testing.T) { Scheme: "https", Host: "oauth2proxy.oidctest", } - provider := newOIDCProvider(serverURL) + provider := newOIDCProvider(serverURL, true) n, err := encryption.Nonce() assert.NoError(t, err) diff --git a/providers/provider_data.go b/providers/provider_data.go index 13241ee..03eeb93 100644 --- a/providers/provider_data.go +++ b/providers/provider_data.go @@ -10,6 +10,7 @@ import ( "strings" "github.com/coreos/go-oidc/v3/oidc" + "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/apis/options" "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/apis/sessions" "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/logger" internaloidc "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/oidc" @@ -18,14 +19,10 @@ import ( ) const ( - OIDCEmailClaim = "email" - OIDCGroupsClaim = "groups" // This is not exported as it's not currently user configurable oidcUserClaim = "sub" ) -var OIDCAudienceClaims = []string{"aud"} - // ProviderData contains information required to configure all implementations // of OAuth2 providers type ProviderData struct { @@ -76,9 +73,9 @@ func (p *ProviderData) GetClientSecret() (clientSecret string, err error) { return string(fileClientSecret), nil } -// SetAllowedGroups organizes a group list into the AllowedGroups map +// setAllowedGroups organizes a group list into the AllowedGroups map // to be consumed by Authorize implementations -func (p *ProviderData) SetAllowedGroups(groups []string) { +func (p *ProviderData) setAllowedGroups(groups []string) { p.AllowedGroups = make(map[string]struct{}, len(groups)) for _, group := range groups { p.AllowedGroups[group] = struct{}{} @@ -172,7 +169,7 @@ func (p *ProviderData) buildSessionFromClaims(rawIDToken, accessToken string) (* // `email_verified` must be present and explicitly set to `false` to be // considered unverified. - verifyEmail := (p.EmailClaim == OIDCEmailClaim) && !p.AllowUnverifiedEmail + verifyEmail := (p.EmailClaim == options.OIDCEmailClaim) && !p.AllowUnverifiedEmail var verified bool exists, err := extractor.GetClaimInto("email_verified", &verified) diff --git a/providers/provider_default_test.go b/providers/provider_default_test.go index 5d4ed1a..ce6eea4 100644 --- a/providers/provider_default_test.go +++ b/providers/provider_default_test.go @@ -107,7 +107,7 @@ func TestProviderDataAuthorize(t *testing.T) { Groups: tc.groups, } p := &ProviderData{} - p.SetAllowedGroups(tc.allowedGroups) + p.setAllowedGroups(tc.allowedGroups) authorized, err := p.Authorize(context.Background(), session) g.Expect(err).ToNot(HaveOccurred()) diff --git a/providers/providers.go b/providers/providers.go index a192f22..a4699e6 100644 --- a/providers/providers.go +++ b/providers/providers.go @@ -2,8 +2,18 @@ package providers import ( "context" + "errors" + "fmt" + "net/url" + "strings" + "github.com/coreos/go-oidc/v3/oidc" + "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/apis/options" "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/apis/sessions" + "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/logger" + internaloidc "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/oidc" + "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/requests" + k8serrors "k8s.io/apimachinery/pkg/util/errors" ) // Provider represents an upstream identity provider implementation @@ -20,38 +30,247 @@ type Provider interface { CreateSessionFromToken(ctx context.Context, token string) (*sessions.SessionState, error) } -// New provides a new Provider based on the configured provider string -func New(provider string, p *ProviderData) Provider { - switch provider { - case "linkedin": - return NewLinkedInProvider(p) - case "facebook": - return NewFacebookProvider(p) - case "github": - return NewGitHubProvider(p) - case "keycloak": - return NewKeycloakProvider(p) - case "keycloak-oidc": - return NewKeycloakOIDCProvider(p) - case "azure": - return NewAzureProvider(p) - case "adfs": - return NewADFSProvider(p) - case "gitlab": - return NewGitLabProvider(p) - case "oidc": - return NewOIDCProvider(p) - case "login.gov": - return NewLoginGovProvider(p) - case "bitbucket": - return NewBitbucketProvider(p) - case "nextcloud": - return NewNextcloudProvider(p) - case "digitalocean": - return NewDigitalOceanProvider(p) - case "google": - return NewGoogleProvider(p) +func NewProvider(providerConfig options.Provider) (Provider, error) { + providerData, err := newProviderDataFromConfig(providerConfig) + if err != nil { + return nil, fmt.Errorf("could not create provider data: %v", err) + } + switch providerConfig.Type { + case options.ADFSProvider: + return NewADFSProvider(providerData, providerConfig.ADFSConfig), nil + case options.AzureProvider: + return NewAzureProvider(providerData, providerConfig.AzureConfig), nil + case options.BitbucketProvider: + return NewBitbucketProvider(providerData, providerConfig.BitbucketConfig), nil + case options.DigitalOceanProvider: + return NewDigitalOceanProvider(providerData), nil + case options.FacebookProvider: + return NewFacebookProvider(providerData), nil + case options.GitHubProvider: + return NewGitHubProvider(providerData, providerConfig.GitHubConfig), nil + case options.GitLabProvider: + return NewGitLabProvider(providerData, providerConfig.GitLabConfig) + 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 + case options.LinkedInProvider: + return NewLinkedInProvider(providerData), nil + case options.LoginGovProvider: + return NewLoginGovProvider(providerData, providerConfig.LoginGovConfig) + case options.NextCloudProvider: + return NewNextcloudProvider(providerData), nil + case options.OIDCProvider: + return NewOIDCProvider(providerData, providerConfig.OIDCConfig), nil default: - return nil + return nil, fmt.Errorf("unknown provider type %q", providerConfig.Type) } } + +func newProviderDataFromConfig(providerConfig options.Provider) (*ProviderData, error) { + p := &ProviderData{ + Scope: providerConfig.Scope, + ClientID: providerConfig.ClientID, + ClientSecret: providerConfig.ClientSecret, + ClientSecretFile: providerConfig.ClientSecretFile, + Prompt: providerConfig.Prompt, + ApprovalPrompt: providerConfig.ApprovalPrompt, + AcrValues: providerConfig.AcrValues, + } + + needsVerifier, err := providerRequiresOIDCProviderVerifier(providerConfig.Type) + if err != nil { + return nil, err + } + + if needsVerifier { + oidcProvider, verifier, err := newOIDCProviderVerifier(providerConfig) + if err != nil { + return nil, fmt.Errorf("error setting OIDC configuration: %v", err) + } + + p.Verifier = verifier + if oidcProvider != nil { + // Use the discovered values rather than any specified values + providerConfig.LoginURL = oidcProvider.Endpoint().AuthURL + providerConfig.RedeemURL = oidcProvider.Endpoint().TokenURL + } + } + + errs := []error{} + for name, u := range map[string]struct { + dst *url.URL + raw string + }{ + "login": {dst: p.LoginURL, raw: providerConfig.LoginURL}, + "redeem": {dst: p.RedeemURL, raw: providerConfig.RedeemURL}, + "profile": {dst: p.ProfileURL, raw: providerConfig.ProfileURL}, + "validate": {dst: p.ValidateURL, raw: providerConfig.ValidateURL}, + "resource": {dst: p.ProtectedResource, raw: providerConfig.ProtectedResource}, + } { + var err error + u.dst, err = url.Parse(u.raw) + if err != nil { + errs = append(errs, fmt.Errorf("could not parse %s URL: %v", name, err)) + } + } + if len(errs) > 0 { + return nil, k8serrors.NewAggregate(errs) + } + + // Make the OIDC options available to all providers that support it + p.AllowUnverifiedEmail = providerConfig.OIDCConfig.InsecureAllowUnverifiedEmail + p.EmailClaim = providerConfig.OIDCConfig.EmailClaim + p.GroupsClaim = providerConfig.OIDCConfig.GroupsClaim + + // TODO (@NickMeves) - Remove This + // Backwards Compatibility for Deprecated UserIDClaim option + if providerConfig.OIDCConfig.EmailClaim == options.OIDCEmailClaim && + providerConfig.OIDCConfig.UserIDClaim != options.OIDCEmailClaim { + p.EmailClaim = providerConfig.OIDCConfig.UserIDClaim + } + + if providerConfig.Scope == "" { + providerConfig.Scope = "openid email profile" + + if len(providerConfig.AllowedGroups) > 0 { + providerConfig.Scope += " groups" + } + } + if providerConfig.OIDCConfig.UserIDClaim == "" { + providerConfig.OIDCConfig.UserIDClaim = "email" + } + + p.setAllowedGroups(providerConfig.AllowedGroups) + + return p, nil +} + +func providerRequiresOIDCProviderVerifier(providerType options.ProviderType) (bool, error) { + switch providerType { + case options.BitbucketProvider, options.DigitalOceanProvider, options.FacebookProvider, options.GitHubProvider, + options.GoogleProvider, options.KeycloakProvider, options.LinkedInProvider, options.LoginGovProvider, options.NextCloudProvider: + return false, nil + case options.ADFSProvider, options.AzureProvider, options.GitLabProvider, options.KeycloakOIDCProvider, options.OIDCProvider: + return true, nil + default: + return false, fmt.Errorf("unknown provider type: %s", providerType) + } +} + +func newOIDCProviderVerifier(providerConfig options.Provider) (*oidc.Provider, *internaloidc.IDTokenVerifier, error) { + // If the issuer isn't set, default it for platforms where it makes sense + if providerConfig.OIDCConfig.IssuerURL == "" { + switch providerConfig.Type { + case "gitlab": + providerConfig.OIDCConfig.IssuerURL = "https://gitlab.com" + case "oidc": + return nil, nil, errors.New("missing required setting: OIDC Issuer URL cannot be empty") + } + } + + switch { + case providerConfig.OIDCConfig.InsecureSkipIssuerVerification && !providerConfig.OIDCConfig.SkipDiscovery: + verifier, err := newInsecureSkipIssuerVerificationOIDCVerifier(providerConfig) + return nil, verifier, err + case providerConfig.OIDCConfig.SkipDiscovery: + verifier, err := newSkipDiscoveryOIDCVerifier(providerConfig) + return nil, verifier, err + default: + return newDiscoveryOIDCProviderVerifier(providerConfig) + } +} + +func newDiscoveryOIDCProviderVerifier(providerConfig options.Provider) (*oidc.Provider, *internaloidc.IDTokenVerifier, error) { + // Configure discoverable provider data. + provider, err := oidc.NewProvider(context.TODO(), providerConfig.OIDCConfig.IssuerURL) + if err != nil { + return nil, nil, err + } + verificationOptions := &internaloidc.IDTokenVerificationOptions{ + AudienceClaims: providerConfig.OIDCConfig.AudienceClaims, + ClientID: providerConfig.ClientID, + ExtraAudiences: providerConfig.OIDCConfig.ExtraAudiences, + } + verifier := internaloidc.NewVerifier(provider.Verifier(&oidc.Config{ + ClientID: providerConfig.ClientID, + SkipIssuerCheck: providerConfig.OIDCConfig.InsecureSkipIssuerVerification, + SkipClientIDCheck: true, // client id check is done within oauth2-proxy: IDTokenVerifier.Verify + }), verificationOptions) + + return provider, verifier, nil +} + +func newInsecureSkipIssuerVerificationOIDCVerifier(providerConfig options.Provider) (*internaloidc.IDTokenVerifier, error) { + // go-oidc doesn't let us pass bypass the issuer check this in the oidc.NewProvider call + // (which uses discovery to get the URLs), so we'll do a quick check ourselves and if + // we get the URLs, we'll just use the non-discovery path. + + logger.Printf("Performing OIDC Discovery...") + + requestURL := strings.TrimSuffix(providerConfig.OIDCConfig.IssuerURL, "/") + "/.well-known/openid-configuration" + body, err := requests.New(requestURL). + Do(). + UnmarshalJSON() + if err != nil { + return nil, fmt.Errorf("failed to discover OIDC configuration: %v", err) + } + + // Prefer manually configured URLs. It's a bit unclear + // why you'd be doing discovery and also providing the URLs + // explicitly though... + if providerConfig.LoginURL == "" { + providerConfig.LoginURL = body.Get("authorization_endpoint").MustString() + } + + if providerConfig.RedeemURL == "" { + providerConfig.RedeemURL = body.Get("token_endpoint").MustString() + } + + if providerConfig.OIDCConfig.JwksURL == "" { + providerConfig.OIDCConfig.JwksURL = body.Get("jwks_uri").MustString() + } + + if providerConfig.ProfileURL == "" { + providerConfig.ProfileURL = body.Get("userinfo_endpoint").MustString() + } + + // Now we have performed the discovery, construct the verifier manually + return newSkipDiscoveryOIDCVerifier(providerConfig) +} + +func newSkipDiscoveryOIDCVerifier(providerConfig options.Provider) (*internaloidc.IDTokenVerifier, error) { + var errs []error + + // Construct a manual IDTokenVerifier from issuer URL & JWKS URI + // instead of metadata discovery if we enable -skip-oidc-discovery. + // In this case we need to make sure the required endpoints for + // the provider are configured. + if providerConfig.LoginURL == "" { + errs = append(errs, errors.New("missing required setting: login-url")) + } + if providerConfig.RedeemURL == "" { + errs = append(errs, errors.New("missing required setting: redeem-url")) + } + if providerConfig.OIDCConfig.JwksURL == "" { + errs = append(errs, errors.New("missing required setting: oidc-jwks-url")) + } + if len(errs) > 0 { + return nil, k8serrors.NewAggregate(errs) + } + + keySet := oidc.NewRemoteKeySet(context.TODO(), providerConfig.OIDCConfig.JwksURL) + verificationOptions := &internaloidc.IDTokenVerificationOptions{ + AudienceClaims: providerConfig.OIDCConfig.AudienceClaims, + ClientID: providerConfig.ClientID, + ExtraAudiences: providerConfig.OIDCConfig.ExtraAudiences, + } + verifier := internaloidc.NewVerifier(oidc.NewVerifier(providerConfig.OIDCConfig.IssuerURL, keySet, &oidc.Config{ + ClientID: providerConfig.ClientID, + SkipIssuerCheck: providerConfig.OIDCConfig.InsecureSkipIssuerVerification, + SkipClientIDCheck: true, // client id check is done within oauth2-proxy: IDTokenVerifier.Verify + }), verificationOptions) + return verifier, nil +} diff --git a/providers/providers_test.go b/providers/providers_test.go new file mode 100644 index 0000000..dfa6a98 --- /dev/null +++ b/providers/providers_test.go @@ -0,0 +1,93 @@ +package providers + +import ( + "io/ioutil" + "os" + "testing" + + "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/apis/options" + . "github.com/onsi/gomega" +) + +const ( + clientID = "bazquux" + clientSecret = "xyzzyplugh" + providerID = "providerID" +) + +func TestClientSecretFileOptionFails(t *testing.T) { + g := NewWithT(t) + + providerConfig := options.Provider{ + ID: providerID, + Type: "google", + ClientID: clientID, + ClientSecretFile: clientSecret, + } + + p, err := newProviderDataFromConfig(providerConfig) + g.Expect(err).ToNot(HaveOccurred()) + g.Expect(p.ClientSecretFile).To(Equal(clientSecret)) + g.Expect(p.ClientSecret).To(BeEmpty()) + + s, err := p.GetClientSecret() + g.Expect(err).To(HaveOccurred()) + g.Expect(s).To(BeEmpty()) +} + +func TestClientSecretFileOption(t *testing.T) { + g := NewWithT(t) + + f, err := ioutil.TempFile("", "client_secret_temp_file_") + g.Expect(err).ToNot(HaveOccurred()) + + clientSecretFileName := f.Name() + + defer func() { + g.Expect(f.Close()).To(Succeed()) + g.Expect(os.Remove(clientSecretFileName)).To(Succeed()) + }() + + _, err = f.WriteString("testcase") + g.Expect(err).ToNot(HaveOccurred()) + + providerConfig := options.Provider{ + ID: providerID, + Type: "google", + ClientID: clientID, + ClientSecretFile: clientSecretFileName, + } + + p, err := newProviderDataFromConfig(providerConfig) + g.Expect(err).ToNot(HaveOccurred()) + g.Expect(p.ClientSecretFile).To(Equal(clientSecretFileName)) + g.Expect(p.ClientSecret).To(BeEmpty()) + + s, err := p.GetClientSecret() + g.Expect(err).ToNot(HaveOccurred()) + g.Expect(s).To(Equal("testcase")) +} + +func TestSkipOIDCDiscovery(t *testing.T) { + g := NewWithT(t) + providerConfig := options.Provider{ + ID: providerID, + Type: "oidc", + ClientID: clientID, + ClientSecretFile: clientSecret, + OIDCConfig: options.OIDCOptions{ + IssuerURL: "https://login.microsoftonline.com/fabrikamb2c.onmicrosoft.com/v2.0/", + SkipDiscovery: true, + }, + } + + _, err := newProviderDataFromConfig(providerConfig) + g.Expect(err).To(MatchError("error setting OIDC configuration: [missing required setting: login-url, missing required setting: redeem-url, missing required setting: oidc-jwks-url]")) + + providerConfig.LoginURL = "https://login.microsoftonline.com/fabrikamb2c.onmicrosoft.com/oauth2/v2.0/authorize?p=b2c_1_sign_in" + providerConfig.RedeemURL = "https://login.microsoftonline.com/fabrikamb2c.onmicrosoft.com/oauth2/v2.0/token?p=b2c_1_sign_in" + providerConfig.OIDCConfig.JwksURL = "https://login.microsoftonline.com/fabrikamb2c.onmicrosoft.com/discovery/v2.0/keys" + + _, err = newProviderDataFromConfig(providerConfig) + g.Expect(err).ToNot(HaveOccurred()) +} From 2e15f57b701a6ff8976f7dd68720bd44fd3df446 Mon Sep 17 00:00:00 2001 From: Joel Speed Date: Tue, 15 Feb 2022 11:46:12 +0000 Subject: [PATCH 3/5] Remove provider configuration from validation package --- pkg/validation/options.go | 241 --------------------------------- pkg/validation/options_test.go | 86 ------------ pkg/validation/providers.go | 4 + 3 files changed, 4 insertions(+), 327 deletions(-) diff --git a/pkg/validation/options.go b/pkg/validation/options.go index 5d586cd..771d076 100644 --- a/pkg/validation/options.go +++ b/pkg/validation/options.go @@ -4,14 +4,11 @@ import ( "context" "crypto/tls" "fmt" - "io/ioutil" "net/http" "net/url" - "os" "strings" "github.com/coreos/go-oidc/v3/oidc" - "github.com/golang-jwt/jwt" "github.com/mbland/hmacauth" "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/apis/options" "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/ip" @@ -19,7 +16,6 @@ import ( internaloidc "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/oidc" "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/requests" "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/util" - "github.com/oauth2-proxy/oauth2-proxy/v7/providers" ) // Validate checks that required options are set and validates those that they @@ -61,100 +57,6 @@ func Validate(o *options.Options) error { "\n use email-domain=* to authorize all email addresses") } - if o.Providers[0].OIDCConfig.IssuerURL != "" { - - ctx := context.Background() - - if o.Providers[0].OIDCConfig.InsecureSkipIssuerVerification && !o.Providers[0].OIDCConfig.SkipDiscovery { - // go-oidc doesn't let us pass bypass the issuer check this in the oidc.NewProvider call - // (which uses discovery to get the URLs), so we'll do a quick check ourselves and if - // we get the URLs, we'll just use the non-discovery path. - - logger.Printf("Performing OIDC Discovery...") - - requestURL := strings.TrimSuffix(o.Providers[0].OIDCConfig.IssuerURL, "/") + "/.well-known/openid-configuration" - body, err := requests.New(requestURL). - WithContext(ctx). - Do(). - UnmarshalJSON() - if err != nil { - logger.Errorf("error: failed to discover OIDC configuration: %v", err) - } else { - // Prefer manually configured URLs. It's a bit unclear - // why you'd be doing discovery and also providing the URLs - // explicitly though... - if o.Providers[0].LoginURL == "" { - o.Providers[0].LoginURL = body.Get("authorization_endpoint").MustString() - } - - if o.Providers[0].RedeemURL == "" { - o.Providers[0].RedeemURL = body.Get("token_endpoint").MustString() - } - - if o.Providers[0].OIDCConfig.JwksURL == "" { - o.Providers[0].OIDCConfig.JwksURL = body.Get("jwks_uri").MustString() - } - - if o.Providers[0].ProfileURL == "" { - o.Providers[0].ProfileURL = body.Get("userinfo_endpoint").MustString() - } - - o.Providers[0].OIDCConfig.SkipDiscovery = true - } - } - - config := &oidc.Config{ - ClientID: o.Providers[0].ClientID, - SkipIssuerCheck: o.Providers[0].OIDCConfig.InsecureSkipIssuerVerification, - SkipClientIDCheck: true, // client id check is done within oauth2-proxy: IDTokenVerifier.Verify - } - - verificationOptions := &internaloidc.IDTokenVerificationOptions{ - AudienceClaims: o.Providers[0].OIDCConfig.AudienceClaims, - ClientID: o.Providers[0].ClientID, - ExtraAudiences: o.Providers[0].OIDCConfig.ExtraAudiences, - } - - // Construct a manual IDTokenVerifier from issuer URL & JWKS URI - // instead of metadata discovery if we enable -skip-oidc-discovery. - // In this case we need to make sure the required endpoints for - // the provider are configured. - if o.Providers[0].OIDCConfig.SkipDiscovery { - if o.Providers[0].LoginURL == "" { - msgs = append(msgs, "missing setting: login-url") - } - if o.Providers[0].RedeemURL == "" { - msgs = append(msgs, "missing setting: redeem-url") - } - if o.Providers[0].OIDCConfig.JwksURL == "" { - msgs = append(msgs, "missing setting: oidc-jwks-url") - } - keySet := oidc.NewRemoteKeySet(ctx, o.Providers[0].OIDCConfig.JwksURL) - o.SetOIDCVerifier(internaloidc.NewVerifier( - oidc.NewVerifier(o.Providers[0].OIDCConfig.IssuerURL, keySet, config), verificationOptions)) - } else { - // Configure discoverable provider data. - provider, err := oidc.NewProvider(ctx, o.Providers[0].OIDCConfig.IssuerURL) - if err != nil { - return err - } - - o.SetOIDCVerifier(internaloidc.NewVerifier(provider.Verifier(config), verificationOptions)) - o.Providers[0].LoginURL = provider.Endpoint().AuthURL - o.Providers[0].RedeemURL = provider.Endpoint().TokenURL - } - if o.Providers[0].Scope == "" { - o.Providers[0].Scope = "openid email profile" - - if len(o.Providers[0].AllowedGroups) > 0 { - o.Providers[0].Scope += " groups" - } - } - if o.Providers[0].OIDCConfig.UserIDClaim == "" { - o.Providers[0].OIDCConfig.UserIDClaim = "email" - } - } - if o.SkipJwtBearerTokens { // Configure extra issuers if len(o.ExtraJwtIssuers) > 0 { @@ -182,7 +84,6 @@ func Validate(o *options.Options) error { } msgs = append(msgs, validateUpstreams(o.UpstreamServers)...) - msgs = parseProviderInfo(o, msgs) if o.ReverseProxy { parser, err := ip.GetRealClientIPParser(o.RealClientIPHeader) @@ -207,148 +108,6 @@ func Validate(o *options.Options) error { return nil } -func parseProviderInfo(o *options.Options, msgs []string) []string { - p := &providers.ProviderData{ - Scope: o.Providers[0].Scope, - ClientID: o.Providers[0].ClientID, - ClientSecret: o.Providers[0].ClientSecret, - ClientSecretFile: o.Providers[0].ClientSecretFile, - Prompt: o.Providers[0].Prompt, - ApprovalPrompt: o.Providers[0].ApprovalPrompt, - AcrValues: o.Providers[0].AcrValues, - } - p.LoginURL, msgs = parseURL(o.Providers[0].LoginURL, "login", msgs) - p.RedeemURL, msgs = parseURL(o.Providers[0].RedeemURL, "redeem", msgs) - p.ProfileURL, msgs = parseURL(o.Providers[0].ProfileURL, "profile", msgs) - p.ValidateURL, msgs = parseURL(o.Providers[0].ValidateURL, "validate", msgs) - p.ProtectedResource, msgs = parseURL(o.Providers[0].ProtectedResource, "resource", msgs) - - // Make the OIDC options available to all providers that support it - p.AllowUnverifiedEmail = o.Providers[0].OIDCConfig.InsecureAllowUnverifiedEmail - p.EmailClaim = o.Providers[0].OIDCConfig.EmailClaim - p.GroupsClaim = o.Providers[0].OIDCConfig.GroupsClaim - p.Verifier = o.GetOIDCVerifier() - - // TODO (@NickMeves) - Remove This - // Backwards Compatibility for Deprecated UserIDClaim option - if o.Providers[0].OIDCConfig.EmailClaim == providers.OIDCEmailClaim && - o.Providers[0].OIDCConfig.UserIDClaim != providers.OIDCEmailClaim { - p.EmailClaim = o.Providers[0].OIDCConfig.UserIDClaim - } - - p.SetAllowedGroups(o.Providers[0].AllowedGroups) - - provider := providers.New(o.Providers[0].Type, p) - if provider == nil { - msgs = append(msgs, fmt.Sprintf("invalid setting: provider '%s' is not available", o.Providers[0].Type)) - return msgs - } - o.SetProvider(provider) - - switch p := o.GetProvider().(type) { - case *providers.AzureProvider: - p.Configure(o.Providers[0].AzureConfig.Tenant) - case *providers.ADFSProvider: - p.Configure(o.Providers[0].ADFSConfig.SkipScope) - case *providers.GitHubProvider: - p.SetOrgTeam(o.Providers[0].GitHubConfig.Org, o.Providers[0].GitHubConfig.Team) - p.SetRepo(o.Providers[0].GitHubConfig.Repo, o.Providers[0].GitHubConfig.Token) - p.SetUsers(o.Providers[0].GitHubConfig.Users) - case *providers.KeycloakProvider: - // Backwards compatibility with `--keycloak-group` option - if len(o.Providers[0].KeycloakConfig.Groups) > 0 { - p.SetAllowedGroups(o.Providers[0].KeycloakConfig.Groups) - } - case *providers.KeycloakOIDCProvider: - if p.Verifier == nil { - msgs = append(msgs, "keycloak-oidc provider requires an oidc issuer URL") - } - p.AddAllowedRoles(o.Providers[0].KeycloakConfig.Roles) - case *providers.GoogleProvider: - if o.Providers[0].GoogleConfig.ServiceAccountJSON != "" { - file, err := os.Open(o.Providers[0].GoogleConfig.ServiceAccountJSON) - if err != nil { - msgs = append(msgs, "invalid Google credentials file: "+o.Providers[0].GoogleConfig.ServiceAccountJSON) - } else { - groups := o.Providers[0].AllowedGroups - // Backwards compatibility with `--google-group` option - if len(o.Providers[0].GoogleConfig.Groups) > 0 { - groups = o.Providers[0].GoogleConfig.Groups - p.SetAllowedGroups(groups) - } - p.SetGroupRestriction(groups, o.Providers[0].GoogleConfig.AdminEmail, file) - } - } - case *providers.BitbucketProvider: - p.SetTeam(o.Providers[0].BitbucketConfig.Team) - p.SetRepository(o.Providers[0].BitbucketConfig.Repository) - case *providers.OIDCProvider: - p.SkipNonce = o.Providers[0].OIDCConfig.InsecureSkipNonce - if p.Verifier == nil { - msgs = append(msgs, "oidc provider requires an oidc issuer URL") - } - case *providers.GitLabProvider: - p.SetAllowedGroups(o.Providers[0].GitLabConfig.Group) - err := p.SetAllowedProjects(o.Providers[0].GitLabConfig.Projects) - if err != nil { - msgs = append(msgs, "failed to setup gitlab project access level") - } - - if p.Verifier == nil { - // Initialize with default verifier for gitlab.com - ctx := context.Background() - - provider, err := oidc.NewProvider(ctx, "https://gitlab.com") - if err != nil { - msgs = append(msgs, "failed to initialize oidc provider for gitlab.com") - } else { - verificationOptions := &internaloidc.IDTokenVerificationOptions{ - AudienceClaims: o.Providers[0].OIDCConfig.AudienceClaims, - ClientID: o.Providers[0].ClientID, - ExtraAudiences: o.Providers[0].OIDCConfig.ExtraAudiences, - } - p.Verifier = internaloidc.NewVerifier(provider.Verifier(&oidc.Config{ - ClientID: o.Providers[0].ClientID, - }), verificationOptions) - - p.LoginURL, msgs = parseURL(provider.Endpoint().AuthURL, "login", msgs) - p.RedeemURL, msgs = parseURL(provider.Endpoint().TokenURL, "redeem", msgs) - } - } - case *providers.LoginGovProvider: - p.PubJWKURL, msgs = parseURL(o.Providers[0].LoginGovConfig.PubJWKURL, "pubjwk", msgs) - - // JWT key can be supplied via env variable or file in the filesystem, but not both. - switch { - case o.Providers[0].LoginGovConfig.JWTKey != "" && o.Providers[0].LoginGovConfig.JWTKeyFile != "": - msgs = append(msgs, "cannot set both jwt-key and jwt-key-file options") - case o.Providers[0].LoginGovConfig.JWTKey == "" && o.Providers[0].LoginGovConfig.JWTKeyFile == "": - msgs = append(msgs, "login.gov provider requires a private key for signing JWTs") - case o.Providers[0].LoginGovConfig.JWTKey != "": - // The JWT Key is in the commandline argument - signKey, err := jwt.ParseRSAPrivateKeyFromPEM([]byte(o.Providers[0].LoginGovConfig.JWTKey)) - if err != nil { - msgs = append(msgs, "could not parse RSA Private Key PEM") - } else { - p.JWTKey = signKey - } - case o.Providers[0].LoginGovConfig.JWTKeyFile != "": - // The JWT key is in the filesystem - keyData, err := ioutil.ReadFile(o.Providers[0].LoginGovConfig.JWTKeyFile) - if err != nil { - msgs = append(msgs, "could not read key file: "+o.Providers[0].LoginGovConfig.JWTKeyFile) - } - signKey, err := jwt.ParseRSAPrivateKeyFromPEM(keyData) - if err != nil { - msgs = append(msgs, "could not parse private key from PEM file:"+o.Providers[0].LoginGovConfig.JWTKeyFile) - } else { - p.JWTKey = signKey - } - } - } - return msgs -} - func parseSignatureKey(o *options.Options, msgs []string) []string { if o.SignatureKey == "" { return msgs diff --git a/pkg/validation/options_test.go b/pkg/validation/options_test.go index 03afb22..3d22ec9 100644 --- a/pkg/validation/options_test.go +++ b/pkg/validation/options_test.go @@ -56,63 +56,6 @@ func TestNewOptions(t *testing.T) { assert.Equal(t, expected, err.Error()) } -func TestClientSecretFileOptionFails(t *testing.T) { - o := options.NewOptions() - o.Cookie.Secret = cookieSecret - o.Providers[0].ID = providerID - o.Providers[0].ClientID = clientID - o.Providers[0].ClientSecretFile = clientSecret - o.EmailDomains = []string{"*"} - err := Validate(o) - assert.NotEqual(t, nil, err) - - p := o.GetProvider().Data() - assert.Equal(t, clientSecret, p.ClientSecretFile) - assert.Equal(t, "", p.ClientSecret) - - s, err := p.GetClientSecret() - assert.NotEqual(t, nil, err) - assert.Equal(t, "", s) -} - -func TestClientSecretFileOption(t *testing.T) { - var err error - f, err := ioutil.TempFile("", "client_secret_temp_file_") - if err != nil { - t.Fatalf("failed to create temp file: %v", err) - } - _, err = f.WriteString("testcase") - if err != nil { - t.Fatalf("failed to write to temp file: %v", err) - } - if err := f.Close(); err != nil { - t.Fatalf("failed to close temp file: %v", err) - } - clientSecretFileName := f.Name() - defer func(t *testing.T) { - if err := os.Remove(clientSecretFileName); err != nil { - t.Fatalf("failed to delete temp file: %v", err) - } - }(t) - - o := options.NewOptions() - o.Cookie.Secret = cookieSecret - o.Providers[0].ID = providerID - o.Providers[0].ClientID = clientID - o.Providers[0].ClientSecretFile = clientSecretFileName - o.EmailDomains = []string{"*"} - err = Validate(o) - assert.Equal(t, nil, err) - - p := o.GetProvider().Data() - assert.Equal(t, clientSecretFileName, p.ClientSecretFile) - assert.Equal(t, "", p.ClientSecret) - - s, err := p.GetClientSecret() - assert.Equal(t, nil, err) - assert.Equal(t, "testcase", s) -} - func TestGoogleGroupOptions(t *testing.T) { o := testOptions() o.Providers[0].GoogleConfig.Groups = []string{"googlegroup"} @@ -155,18 +98,6 @@ func TestRedirectURL(t *testing.T) { assert.Equal(t, expected, o.GetRedirectURL()) } -func TestDefaultProviderApiSettings(t *testing.T) { - o := testOptions() - assert.Equal(t, nil, Validate(o)) - p := o.GetProvider().Data() - assert.Equal(t, "https://accounts.google.com/o/oauth2/auth?access_type=offline", - p.LoginURL.String()) - assert.Equal(t, "https://www.googleapis.com/oauth2/v3/token", - p.RedeemURL.String()) - assert.Equal(t, "", p.ProfileURL.String()) - assert.Equal(t, "profile email", p.Scope) -} - func TestCookieRefreshMustBeLessThanCookieExpire(t *testing.T) { o := testOptions() assert.Equal(t, nil, Validate(o)) @@ -228,23 +159,6 @@ func TestValidateSignatureKeyUnsupportedAlgorithm(t *testing.T) { " unsupported signature hash algorithm: "+o.SignatureKey) } -func TestSkipOIDCDiscovery(t *testing.T) { - o := testOptions() - o.Providers[0].Type = "oidc" - o.Providers[0].OIDCConfig.IssuerURL = "https://login.microsoftonline.com/fabrikamb2c.onmicrosoft.com/v2.0/" - o.Providers[0].OIDCConfig.SkipDiscovery = true - - err := Validate(o) - assert.Equal(t, "invalid configuration:\n"+ - " missing setting: login-url\n missing setting: redeem-url\n missing setting: oidc-jwks-url", err.Error()) - - o.Providers[0].LoginURL = "https://login.microsoftonline.com/fabrikamb2c.onmicrosoft.com/oauth2/v2.0/authorize?p=b2c_1_sign_in" - o.Providers[0].RedeemURL = "https://login.microsoftonline.com/fabrikamb2c.onmicrosoft.com/oauth2/v2.0/token?p=b2c_1_sign_in" - o.Providers[0].OIDCConfig.JwksURL = "https://login.microsoftonline.com/fabrikamb2c.onmicrosoft.com/discovery/v2.0/keys" - - assert.Equal(t, nil, Validate(o)) -} - func TestGCPHealthcheck(t *testing.T) { o := testOptions() o.GCPHealthChecks = true diff --git a/pkg/validation/providers.go b/pkg/validation/providers.go index 489f94d..587b928 100644 --- a/pkg/validation/providers.go +++ b/pkg/validation/providers.go @@ -3,6 +3,7 @@ package validation import ( "fmt" "io/ioutil" + "os" "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/apis/options" ) @@ -77,7 +78,10 @@ func validateGoogleConfig(provider options.Provider) []string { } if provider.GoogleConfig.ServiceAccountJSON == "" { msgs = append(msgs, "missing setting: google-service-account-json") + } else if _, err := os.Stat(provider.GoogleConfig.ServiceAccountJSON); err != nil { + msgs = append(msgs, fmt.Sprintf("invalid Google credentials file: %s", provider.GoogleConfig.ServiceAccountJSON)) } } + return msgs } From 0791aef8cc5604990d3464dc7c533c32e8680069 Mon Sep 17 00:00:00 2001 From: Joel Speed Date: Tue, 15 Feb 2022 12:00:06 +0000 Subject: [PATCH 4/5] Integrate new provider constructor in main --- oauthproxy.go | 21 +++++++++++++-------- oauthproxy_test.go | 25 ++++++++++++++++--------- 2 files changed, 29 insertions(+), 17 deletions(-) diff --git a/oauthproxy.go b/oauthproxy.go index 1cd35e6..12996d8 100644 --- a/oauthproxy.go +++ b/oauthproxy.go @@ -114,6 +114,11 @@ func NewOAuthProxy(opts *options.Options, validator func(string) bool) (*OAuthPr } } + provider, err := providers.NewProvider(opts.Providers[0]) + if err != nil { + return nil, fmt.Errorf("error intiailising provider: %v", err) + } + pageWriter, err := pagewriter.NewWriter(pagewriter.Opts{ TemplatesPath: opts.Templates.Path, CustomLogo: opts.Templates.CustomLogo, @@ -121,7 +126,7 @@ func NewOAuthProxy(opts *options.Options, validator func(string) bool) (*OAuthPr Footer: opts.Templates.Footer, Version: VERSION, Debug: opts.Templates.Debug, - ProviderName: buildProviderName(opts.GetProvider(), opts.Providers[0].Name), + ProviderName: buildProviderName(provider, opts.Providers[0].Name), SignInMessage: buildSignInMessage(opts), DisplayLoginForm: basicAuthValidator != nil && opts.Templates.DisplayLoginForm, }) @@ -145,7 +150,7 @@ func NewOAuthProxy(opts *options.Options, validator func(string) bool) (*OAuthPr redirectURL.Path = fmt.Sprintf("%s/callback", opts.ProxyPrefix) } - logger.Printf("OAuthProxy configured for %s Client ID: %s", opts.GetProvider().Data().ProviderName, opts.Providers[0].ClientID) + logger.Printf("OAuthProxy configured for %s Client ID: %s", provider.Data().ProviderName, opts.Providers[0].ClientID) refresh := "disabled" if opts.Cookie.Refresh != time.Duration(0) { refresh = fmt.Sprintf("after %s", opts.Cookie.Refresh) @@ -171,7 +176,7 @@ func NewOAuthProxy(opts *options.Options, validator func(string) bool) (*OAuthPr if err != nil { return nil, fmt.Errorf("could not build pre-auth chain: %v", err) } - sessionChain := buildSessionChain(opts, sessionStore, basicAuthValidator) + sessionChain := buildSessionChain(opts, provider, sessionStore, basicAuthValidator) headersChain, err := buildHeadersChain(opts) if err != nil { return nil, fmt.Errorf("could not build headers chain: %v", err) @@ -190,7 +195,7 @@ func NewOAuthProxy(opts *options.Options, validator func(string) bool) (*OAuthPr SignInPath: fmt.Sprintf("%s/sign_in", opts.ProxyPrefix), ProxyPrefix: opts.ProxyPrefix, - provider: opts.GetProvider(), + provider: provider, sessionStore: sessionStore, redirectURL: redirectURL, allowedRoutes: allowedRoutes, @@ -346,12 +351,12 @@ func buildPreAuthChain(opts *options.Options) (alice.Chain, error) { return chain, nil } -func buildSessionChain(opts *options.Options, sessionStore sessionsapi.SessionStore, validator basic.Validator) alice.Chain { +func buildSessionChain(opts *options.Options, provider providers.Provider, sessionStore sessionsapi.SessionStore, validator basic.Validator) alice.Chain { chain := alice.New() if opts.SkipJwtBearerTokens { sessionLoaders := []middlewareapi.TokenToSessionFunc{ - opts.GetProvider().CreateSessionFromToken, + provider.CreateSessionFromToken, } for _, verifier := range opts.GetJWTBearerVerifiers() { @@ -369,8 +374,8 @@ func buildSessionChain(opts *options.Options, sessionStore sessionsapi.SessionSt chain = chain.Append(middleware.NewStoredSessionLoader(&middleware.StoredSessionLoaderOptions{ SessionStore: sessionStore, RefreshPeriod: opts.Cookie.Refresh, - RefreshSession: opts.GetProvider().RefreshSession, - ValidateSession: opts.GetProvider().ValidateSession, + RefreshSession: provider.RefreshSession, + ValidateSession: provider.ValidateSession, })) return chain diff --git a/oauthproxy_test.go b/oauthproxy_test.go index 6f5a553..3157c34 100644 --- a/oauthproxy_test.go +++ b/oauthproxy_test.go @@ -161,13 +161,11 @@ func Test_enrichSession(t *testing.T) { err := validation.Validate(opts) assert.NoError(t, err) - // intentionally set after validation.Validate(opts) since it will clobber - // our TestProvider and call `providers.New` defaulting to `providers.GoogleProvider` - opts.SetProvider(NewTestProvider(&url.URL{Host: "www.example.com"}, providerEmail)) proxy, err := NewOAuthProxy(opts, func(string) bool { return true }) if err != nil { t.Fatal(err) } + proxy.provider = NewTestProvider(&url.URL{Host: "www.example.com"}, providerEmail) err = proxy.enrichSessionState(context.Background(), tc.session) assert.NoError(t, err) @@ -232,13 +230,13 @@ func TestBasicAuthPassword(t *testing.T) { providerURL, _ := url.Parse(providerServer.URL) const emailAddress = "john.doe@example.com" - opts.SetProvider(NewTestProvider(providerURL, emailAddress)) proxy, err := NewOAuthProxy(opts, func(email string) bool { return email == emailAddress }) if err != nil { t.Fatal(err) } + proxy.provider = NewTestProvider(providerURL, emailAddress) // Save the required session rw := httptest.NewRecorder() @@ -390,10 +388,10 @@ func NewPassAccessTokenTest(opts PassAccessTokenTestOptions) (*PassAccessTokenTe testProvider := NewTestProvider(providerURL, emailAddress) testProvider.ValidToken = opts.ValidToken - patt.opts.SetProvider(testProvider) patt.proxy, err = NewOAuthProxy(patt.opts, func(email string) bool { return email == emailAddress }) + patt.proxy.provider = testProvider if err != nil { return nil, err } @@ -769,11 +767,17 @@ func NewProcessCookieTest(opts ProcessCookieTestOpts, modifiers ...OptionsModifi if err != nil { return nil, err } - pcTest.proxy.provider = &TestProvider{ + testProvider := &TestProvider{ ProviderData: &providers.ProviderData{}, ValidToken: opts.providerValidateCookieResponse, } - pcTest.proxy.provider.(*TestProvider).SetAllowedGroups(pcTest.opts.Providers[0].AllowedGroups) + + groups := pcTest.opts.Providers[0].AllowedGroups + testProvider.AllowedGroups = make(map[string]struct{}, len(groups)) + for _, group := range groups { + testProvider.AllowedGroups[group] = struct{}{} + } + pcTest.proxy.provider = testProvider // Now, zero-out proxy.CookieRefresh for the cases that don't involve // access_token validation. @@ -1359,12 +1363,12 @@ func TestAuthSkippedForPreflightRequests(t *testing.T) { assert.NoError(t, err) upstreamURL, _ := url.Parse(upstreamServer.URL) - opts.SetProvider(NewTestProvider(upstreamURL, "")) proxy, err := NewOAuthProxy(opts, func(string) bool { return false }) if err != nil { t.Fatal(err) } + proxy.provider = NewTestProvider(upstreamURL, "") rw := httptest.NewRecorder() req, _ := http.NewRequest("OPTIONS", "/preflight-request", nil) proxy.ServeHTTP(rw, req) @@ -1409,6 +1413,7 @@ type SignatureTest struct { header http.Header rw *httptest.ResponseRecorder authenticator *SignatureAuthenticator + authProvider providers.Provider } func NewSignatureTest() (*SignatureTest, error) { @@ -1443,7 +1448,7 @@ func NewSignatureTest() (*SignatureTest, error) { if err != nil { return nil, err } - opts.SetProvider(NewTestProvider(providerURL, "mbland@acm.org")) + testProvider := NewTestProvider(providerURL, "mbland@acm.org") return &SignatureTest{ opts, @@ -1453,6 +1458,7 @@ func NewSignatureTest() (*SignatureTest, error) { make(http.Header), httptest.NewRecorder(), authenticator, + testProvider, }, nil } @@ -1486,6 +1492,7 @@ func (st *SignatureTest) MakeRequestWithExpectedKey(method, body, key string) er if err != nil { return err } + proxy.provider = st.authProvider var bodyBuf io.ReadCloser if body != "" { From eda5eb9243185366221e32561260dbb4a510f1ec Mon Sep 17 00:00:00 2001 From: Joel Speed Date: Wed, 16 Feb 2022 11:46:32 +0000 Subject: [PATCH 5/5] Add changelog entry for provider refactor --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index b5655e4..33ebd72 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,7 @@ ## Changes since v7.2.1 +- [#1555](https://github.com/oauth2-proxy/oauth2-proxy/pull/1555) Refactor provider configuration into providers package (@JoelSpeed) - [#1394](https://github.com/oauth2-proxy/oauth2-proxy/pull/1394) Add generic claim extractor to get claims from ID Tokens (@JoelSpeed) - [#1468](https://github.com/oauth2-proxy/oauth2-proxy/pull/1468) Implement session locking with session state lock (@JoelSpeed, @Bibob7) - [#1489](https://github.com/oauth2-proxy/oauth2-proxy/pull/1489) Fix Docker Buildx push to include build version (@JoelSpeed)