From 2e15f57b701a6ff8976f7dd68720bd44fd3df446 Mon Sep 17 00:00:00 2001 From: Joel Speed Date: Tue, 15 Feb 2022 11:46:12 +0000 Subject: [PATCH] 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 5d586cdd..771d076f 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 03afb22a..3d22ec9e 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 489f94d0..587b9289 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 }