diff --git a/CHANGELOG.md b/CHANGELOG.md index 4de29d7b..effa411d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,7 @@ ## Changes since v7.2.1 +- [#1559](https://github.com/oauth2-proxy/oauth2-proxy/pull/1559) Introduce ProviderVerifier to clean up OIDC discovery code (@JoelSpeed) - [#1561](https://github.com/oauth2-proxy/oauth2-proxy/pull/1561) Add ppc64le support (@mgiessing) - [#1563](https://github.com/oauth2-proxy/oauth2-proxy/pull/1563) Ensure claim extractor does not attempt profile call when URL is empty (@JoelSpeed) - [#1560](https://github.com/oauth2-proxy/oauth2-proxy/pull/1560) Fix provider data initialisation (@JoelSpeed) diff --git a/go.mod b/go.mod index 1df17f69..2621368c 100644 --- a/go.mod +++ b/go.mod @@ -9,6 +9,7 @@ require ( github.com/bitly/go-simplejson v0.5.0 github.com/bsm/redislock v0.7.0 github.com/coreos/go-oidc/v3 v3.0.0 + github.com/dgrijalva/jwt-go v3.2.0+incompatible github.com/fsnotify/fsnotify v1.4.9 github.com/ghodss/yaml v1.0.1-0.20190212211648-25d852aebe32 github.com/go-redis/redis/v8 v8.2.3 @@ -18,6 +19,7 @@ require ( github.com/justinas/alice v1.2.0 github.com/mbland/hmacauth v0.0.0-20170912233209-44256dfd4bfa github.com/mitchellh/mapstructure v1.1.2 + github.com/oauth2-proxy/mockoidc v0.0.0-20210703044157-382d3faf2671 github.com/oauth2-proxy/tools/reference-gen v0.0.0-20210118095127-56ffd7384404 github.com/onsi/ginkgo v1.14.1 github.com/onsi/gomega v1.10.2 @@ -26,7 +28,7 @@ require ( github.com/spf13/cast v1.3.0 github.com/spf13/pflag v1.0.5 github.com/spf13/viper v1.6.3 - github.com/stretchr/testify v1.6.1 + github.com/stretchr/testify v1.7.0 github.com/vmihailenco/msgpack/v4 v4.3.11 golang.org/x/crypto v0.0.0-20210711020723-a769d52b0f97 golang.org/x/net v0.0.0-20210226172049-e18ecbb05110 diff --git a/go.sum b/go.sum index a0940db2..54be4caf 100644 --- a/go.sum +++ b/go.sum @@ -81,6 +81,7 @@ github.com/creack/pty v1.1.7/go.mod h1:lj5s0c3V2DBrqTV7llrYr5NG6My20zk30Fl46Y7Do github.com/davecgh/go-spew v1.1.0/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38= github.com/davecgh/go-spew v1.1.1 h1:vj9j/u1bqnvCEfJOwUhtlOARqs3+rkHYY13jYWTU97c= github.com/davecgh/go-spew v1.1.1/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38= +github.com/dgrijalva/jwt-go v3.2.0+incompatible h1:7qlOGliEKZXTDg6OTjfoBKDXWrumCAMpl/TFQ4/5kLM= github.com/dgrijalva/jwt-go v3.2.0+incompatible/go.mod h1:E3ru+11k8xSBh+hMPgOLZmtrrCbhqsmaPHjLKYnJCaQ= github.com/dgryski/go-rendezvous v0.0.0-20200823014737-9f7001d12a5f h1:lO4WD4F/rVNCu3HqELle0jiPLLBs70cWOduZpkS1E78= github.com/dgryski/go-rendezvous v0.0.0-20200823014737-9f7001d12a5f/go.mod h1:cuUVRXasLTGF7a8hSLbxyZXjz+1KgoB3wDUb6vlszIc= @@ -169,8 +170,9 @@ github.com/google/go-cmp v0.2.0/go.mod h1:oXzfMopK8JAjlY9xF4vHSVASa0yLyX7SntLO5a github.com/google/go-cmp v0.3.0/go.mod h1:8QqcDgzrUqlUb/G2PQTWiueGozuR1884gddMywk6iLU= github.com/google/go-cmp v0.3.1/go.mod h1:8QqcDgzrUqlUb/G2PQTWiueGozuR1884gddMywk6iLU= github.com/google/go-cmp v0.4.0/go.mod h1:v8dTdLbMG2kIc/vJvl+f65V22dbkXbowE6jgT/gNBxE= -github.com/google/go-cmp v0.5.1 h1:JFrFEBb2xKufg6XkJsJr+WbKb4FQlURi5RUcBveYu9k= github.com/google/go-cmp v0.5.1/go.mod h1:v8dTdLbMG2kIc/vJvl+f65V22dbkXbowE6jgT/gNBxE= +github.com/google/go-cmp v0.5.4 h1:L8R9j+yAqZuZjsqh/z+F1NCffTKKLShY6zXTItVIZ8M= +github.com/google/go-cmp v0.5.4/go.mod h1:v8dTdLbMG2kIc/vJvl+f65V22dbkXbowE6jgT/gNBxE= github.com/google/gofuzz v1.0.0/go.mod h1:dBl0BpW6vV/+mYPU4Po3pmUjxk6FQPldtuIdl/M65Eg= github.com/google/gofuzz v1.1.0/go.mod h1:dBl0BpW6vV/+mYPU4Po3pmUjxk6FQPldtuIdl/M65Eg= github.com/google/martian v2.1.0+incompatible/go.mod h1:9I4somxYTbIHy5NJKHRl3wXiIaQGbYVAs8BPL6v8lEs= @@ -293,6 +295,8 @@ github.com/nats-io/nkeys v0.1.3/go.mod h1:xpnFELMwJABBLVhffcfd1MZx6VsNRFpEugbxzi github.com/nats-io/nuid v1.0.1/go.mod h1:19wcPz3Ph3q0Jbyiqsd0kePYG7A95tJPxeL+1OSON2c= github.com/nxadm/tail v1.4.4 h1:DQuhQpB1tVlglWS2hLQ5OV6B5r8aGxSrPc5Qo6uTN78= github.com/nxadm/tail v1.4.4/go.mod h1:kenIhsEOeOJmVchQTgglprH7qJGnHDVpk1VPCcaMI8A= +github.com/oauth2-proxy/mockoidc v0.0.0-20210703044157-382d3faf2671 h1:rCw45DEDqAy46HsVu2WZQgEqKth79c8k6HZP2uPDTls= +github.com/oauth2-proxy/mockoidc v0.0.0-20210703044157-382d3faf2671/go.mod h1:ejAP84CdQJiV7GcnIF0wNhbhcWsxew0qXV04CB+r+Mw= github.com/oauth2-proxy/tools/reference-gen v0.0.0-20210118095127-56ffd7384404 h1:ZpzR4Ou1nhldBG/vEzauoqyaUlofaUcLkv1C/gBK8ls= github.com/oauth2-proxy/tools/reference-gen v0.0.0-20210118095127-56ffd7384404/go.mod h1:YpORG8zs14vNlpXvuHYnnDvWazIRaDk02MaY8lafqdI= github.com/oklog/oklog v0.3.2/go.mod h1:FCV+B7mhrz4o+ueLpx+KqkyXRGMWOYEvfiXtdGtbWGs= @@ -412,8 +416,9 @@ github.com/stretchr/testify v1.2.2/go.mod h1:a8OnRcib4nhh0OaRAV+Yts87kKdq0PP7pXf github.com/stretchr/testify v1.3.0/go.mod h1:M5WIy9Dh21IEIfnGCwXGc5bZfKNJtfHm1UVUgZn+9EI= github.com/stretchr/testify v1.4.0/go.mod h1:j7eGeouHqKxXV5pUuKE4zz7dFj8WfuZ+81PSLYec5m4= github.com/stretchr/testify v1.5.1/go.mod h1:5W2xD1RspED5o8YsWQXVCued0rvSQ+mT+I5cxcmMvtA= -github.com/stretchr/testify v1.6.1 h1:hDPOHmpOpP40lSULcqw7IrRb/u7w6RpDC9399XyoNd0= github.com/stretchr/testify v1.6.1/go.mod h1:6Fq8oRcR53rry900zMqJjRRixrwX3KX962/h/Wwjteg= +github.com/stretchr/testify v1.7.0 h1:nwc3DEeHmmLAfoZucVR881uASk0Mfjw8xYJ99tb5CcY= +github.com/stretchr/testify v1.7.0/go.mod h1:6Fq8oRcR53rry900zMqJjRRixrwX3KX962/h/Wwjteg= github.com/subosito/gotenv v1.2.0 h1:Slr1R9HxAlEKefgq5jn9U+DnETlIUa6HfgEzj0g5d7s= github.com/subosito/gotenv v1.2.0/go.mod h1:N0PQaV/YGNqwC0u51sEeR/aUtSLEXKX9iv69rRypqCw= github.com/tmc/grpc-websocket-proxy v0.0.0-20170815181823-89b8d40f7ca8/go.mod h1:ncp9v5uamzpCO7NfCPTXjqaC+bZgJeR0sMTm6dMHP7U= @@ -456,6 +461,7 @@ golang.org/x/crypto v0.0.0-20190510104115-cbcb75029529/go.mod h1:yigFU9vqHzYiE8U golang.org/x/crypto v0.0.0-20190701094942-4def268fd1a4/go.mod h1:yigFU9vqHzYiE8UmvKecakEJjdnWj3jj499lnFckfCI= golang.org/x/crypto v0.0.0-20191011191535-87dc89f01550/go.mod h1:yigFU9vqHzYiE8UmvKecakEJjdnWj3jj499lnFckfCI= golang.org/x/crypto v0.0.0-20200622213623-75b288015ac9/go.mod h1:LzIPMQfyMNhhGPhUkYOs5KpL4U8rLKemX1yGLhDgUto= +golang.org/x/crypto v0.0.0-20201221181555-eec23a3978ad/go.mod h1:jdWPYTVW3xRLrWPugEBEK3UY2ZEsg3UU495nc5E+M+I= golang.org/x/crypto v0.0.0-20210711020723-a769d52b0f97 h1:/UOmuWzQfxxo9UtlXMwuQU8CMgg1eZXqTRwkSQJWKOI= golang.org/x/crypto v0.0.0-20210711020723-a769d52b0f97/go.mod h1:GvvjBRRGRdwPK5ydBHafDWAxML/pGHZbMvKqRZ5+Abc= golang.org/x/exp v0.0.0-20190121172915-509febef88a4/go.mod h1:CJ0aWSM057203Lf6IL+f9T1iT9GByDxfZKAQTCR3kQA= @@ -535,6 +541,7 @@ golang.org/x/sys v0.0.0-20190826190057-c7b8b68b1456/go.mod h1:h1NjWce9XRLGQEsW7w golang.org/x/sys v0.0.0-20190904154756-749cb33beabd/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs= golang.org/x/sys v0.0.0-20191001151750-bb3f8db39f24/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs= golang.org/x/sys v0.0.0-20191005200804-aed5e4c7ecf9/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs= +golang.org/x/sys v0.0.0-20191026070338-33540a1f6037/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs= golang.org/x/sys v0.0.0-20191120155948-bd437916bb0e/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs= golang.org/x/sys v0.0.0-20191220142924-d4481acd189f/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs= golang.org/x/sys v0.0.0-20200106162015-b016eb3dc98e/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs= @@ -547,6 +554,7 @@ golang.org/x/sys v0.0.0-20201119102817-f84b799fce68/go.mod h1:h1NjWce9XRLGQEsW7w golang.org/x/sys v0.0.0-20201214210602-f9fddec55a1e/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs= golang.org/x/sys v0.0.0-20210615035016-665e8c7367d1 h1:SrN+KX8Art/Sf4HNj6Zcz06G7VEz+7w9tdXTPOZ7+l4= golang.org/x/sys v0.0.0-20210615035016-665e8c7367d1/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg= +golang.org/x/term v0.0.0-20201117132131-f5c789dd3221/go.mod h1:Nr5EML6q2oocZ2LXRh80K7BxOlk5/8JxuGnuhpl+muw= golang.org/x/term v0.0.0-20201126162022-7de9c90e9dd1/go.mod h1:bj7SfCRtBDWHUb9snDiAeCFNEtKQo2Wmx5Cou7ajbmo= golang.org/x/text v0.3.0/go.mod h1:NqM8EUOU14njkJ3fqMW+pc6Ldnwhi/IjpwHt7yyuwOQ= golang.org/x/text v0.3.1-0.20180807135948-17ff2d5776d2/go.mod h1:NqM8EUOU14njkJ3fqMW+pc6Ldnwhi/IjpwHt7yyuwOQ= diff --git a/oauthproxy_test.go b/oauthproxy_test.go index 51dad4a1..7c72069a 100644 --- a/oauthproxy_test.go +++ b/oauthproxy_test.go @@ -21,7 +21,7 @@ import ( "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/apis/sessions" "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/cookies" "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/logger" - internaloidc "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/oidc" + internaloidc "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/providers/oidc" sessionscookie "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/sessions/cookie" "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/upstream" "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/validation" @@ -1747,7 +1747,7 @@ func TestGetJwtSession(t *testing.T) { verifier := oidc.NewVerifier("https://issuer.example.com", keyset, &oidc.Config{ClientID: "https://test.myapp.com", SkipExpiryCheck: true, SkipClientIDCheck: true}) - verificationOptions := &internaloidc.IDTokenVerificationOptions{ + verificationOptions := internaloidc.IDTokenVerificationOptions{ AudienceClaims: []string{"aud"}, ClientID: "https://test.myapp.com", ExtraAudiences: []string{}, diff --git a/pkg/apis/options/options.go b/pkg/apis/options/options.go index 2535b581..8641a430 100644 --- a/pkg/apis/options/options.go +++ b/pkg/apis/options/options.go @@ -5,7 +5,7 @@ import ( "net/url" ipapi "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/apis/ip" - internaloidc "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/oidc" + internaloidc "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/providers/oidc" "github.com/spf13/pflag" ) @@ -68,26 +68,26 @@ type Options struct { // internal values that are set after config validation redirectURL *url.URL signatureData *SignatureData - oidcVerifier *internaloidc.IDTokenVerifier - jwtBearerVerifiers []*internaloidc.IDTokenVerifier + oidcVerifier internaloidc.IDTokenVerifier + jwtBearerVerifiers []internaloidc.IDTokenVerifier realClientIPParser ipapi.RealClientIPParser } // Options for Getting internal values -func (o *Options) GetRedirectURL() *url.URL { return o.redirectURL } -func (o *Options) GetSignatureData() *SignatureData { return o.signatureData } -func (o *Options) GetOIDCVerifier() *internaloidc.IDTokenVerifier { return o.oidcVerifier } -func (o *Options) GetJWTBearerVerifiers() []*internaloidc.IDTokenVerifier { +func (o *Options) GetRedirectURL() *url.URL { return o.redirectURL } +func (o *Options) GetSignatureData() *SignatureData { return o.signatureData } +func (o *Options) GetOIDCVerifier() internaloidc.IDTokenVerifier { return o.oidcVerifier } +func (o *Options) GetJWTBearerVerifiers() []internaloidc.IDTokenVerifier { return o.jwtBearerVerifiers } 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) 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/oidc/oidc_suite_test.go b/pkg/providers/oidc/oidc_suite_test.go similarity index 100% rename from pkg/oidc/oidc_suite_test.go rename to pkg/providers/oidc/oidc_suite_test.go diff --git a/pkg/providers/oidc/provider.go b/pkg/providers/oidc/provider.go new file mode 100644 index 00000000..d1b88644 --- /dev/null +++ b/pkg/providers/oidc/provider.go @@ -0,0 +1,98 @@ +package oidc + +import ( + "context" + "fmt" + "strings" + + "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/logger" + "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/requests" +) + +// providerJSON resresents the information we need from an OIDC discovery +type providerJSON struct { + Issuer string `json:"issuer"` + AuthURL string `json:"authorization_endpoint"` + TokenURL string `json:"token_endpoint"` + JWKsURL string `json:"jwks_uri"` + UserInfoURL string `json:"userinfo_endpoint"` + CodeChallengeAlgs []string `json:"code_challenge_methods_supported"` +} + +// Endpoints represents the endpoints discovered as part of the OIDC discovery process +// that will be used by the authentication providers. +type Endpoints struct { + AuthURL string + TokenURL string + JWKsURL string + UserInfoURL string +} + +// PKCE holds information relevant to the PKCE (code challenge) support of the +// provider. +type PKCE struct { + CodeChallengeAlgs []string +} + +// DiscoveryProvider holds information about an identity provider having +// used OIDC discovery to retrieve the information. +type DiscoveryProvider interface { + Endpoints() Endpoints + PKCE() PKCE +} + +// NewProvider allows a user to perform an OIDC discovery and returns the DiscoveryProvider. +// We implement this here as opposed to using oidc.Provider so that we can override the Issuer verification check. +// As we have our own verifier and fetch the userinfo separately, the rest of the oidc.Provider implementation is not +// useful to us. +func NewProvider(ctx context.Context, issuerURL string, skipIssuerVerification bool) (DiscoveryProvider, 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...") + + var p providerJSON + requestURL := strings.TrimSuffix(issuerURL, "/") + "/.well-known/openid-configuration" + if err := requests.New(requestURL).WithContext(ctx).Do().UnmarshalInto(&p); err != nil { + return nil, fmt.Errorf("failed to discover OIDC configuration: %v", err) + } + + if !skipIssuerVerification && p.Issuer != issuerURL { + return nil, fmt.Errorf("oidc: issuer did not match the issuer returned by provider, expected %q got %q", issuerURL, p.Issuer) + } + + return &discoveryProvider{ + authURL: p.AuthURL, + tokenURL: p.TokenURL, + jwksURL: p.JWKsURL, + userInfoURL: p.UserInfoURL, + codeChallengeAlgs: p.CodeChallengeAlgs, + }, nil +} + +// discoveryProvider holds the discovered endpoints +type discoveryProvider struct { + authURL string + tokenURL string + jwksURL string + userInfoURL string + codeChallengeAlgs []string +} + +// Endpoints returns the discovered endpoints needed for an authentication provider. +func (p *discoveryProvider) Endpoints() Endpoints { + return Endpoints{ + AuthURL: p.authURL, + TokenURL: p.tokenURL, + JWKsURL: p.jwksURL, + UserInfoURL: p.userInfoURL, + } +} + +// PKCE returns information related to the PKCE (code challenge) support of the provider. +func (p *discoveryProvider) PKCE() PKCE { + return PKCE{ + CodeChallengeAlgs: p.codeChallengeAlgs, + } +} diff --git a/pkg/providers/oidc/provider_test.go b/pkg/providers/oidc/provider_test.go new file mode 100644 index 00000000..2f822e97 --- /dev/null +++ b/pkg/providers/oidc/provider_test.go @@ -0,0 +1,153 @@ +package oidc + +import ( + "context" + "encoding/json" + "net" + "net/http" + + "github.com/oauth2-proxy/mockoidc" + . "github.com/onsi/ginkgo" + . "github.com/onsi/ginkgo/extensions/table" + . "github.com/onsi/gomega" +) + +var _ = Describe("Provider", func() { + type newProviderTableInput struct { + skipIssuerVerification bool + expectedError string + middlewares func(*mockoidc.MockOIDC) []func(http.Handler) http.Handler + } + + DescribeTable("NewProvider", func(in *newProviderTableInput) { + m, err := mockoidc.NewServer(nil) + Expect(err).ToNot(HaveOccurred()) + + if in.middlewares != nil { + middlewares := in.middlewares(m) + for _, middlware := range middlewares { + m.AddMiddleware(middlware) + } + } + + ln, err := net.Listen("tcp", "127.0.0.1:0") + Expect(err).ToNot(HaveOccurred()) + + Expect(m.Start(ln, nil)).To(Succeed()) + defer func() { + Expect(m.Shutdown()).To(Succeed()) + }() + + provider, err := NewProvider(context.Background(), m.Issuer(), in.skipIssuerVerification) + if in.expectedError != "" { + Expect(err).To(MatchError(HavePrefix(in.expectedError))) + return + } + Expect(err).ToNot(HaveOccurred()) + + endpoints := provider.Endpoints() + Expect(endpoints.AuthURL).To(Equal(m.AuthorizationEndpoint())) + Expect(endpoints.TokenURL).To(Equal(m.TokenEndpoint())) + Expect(endpoints.JWKsURL).To(Equal(m.JWKSEndpoint())) + Expect(endpoints.UserInfoURL).To(Equal(m.UserinfoEndpoint())) + }, + Entry("with issuer verification and the issuer matches", &newProviderTableInput{ + skipIssuerVerification: false, + }), + Entry("with skip issuer verification and the issuer matches", &newProviderTableInput{ + skipIssuerVerification: true, + }), + Entry("with issuer verification and an invalid issuer", &newProviderTableInput{ + skipIssuerVerification: false, + middlewares: func(m *mockoidc.MockOIDC) []func(http.Handler) http.Handler { + return []func(http.Handler) http.Handler{ + newInvalidIssuerMiddleware(m), + } + }, + expectedError: "oidc: issuer did not match the issuer returned by provider", + }), + Entry("with skip issuer verification and an invalid issuer", &newProviderTableInput{ + skipIssuerVerification: true, + middlewares: func(m *mockoidc.MockOIDC) []func(http.Handler) http.Handler { + return []func(http.Handler) http.Handler{ + newInvalidIssuerMiddleware(m), + } + }, + }), + Entry("when the issuer returns a bad response", &newProviderTableInput{ + skipIssuerVerification: false, + middlewares: func(m *mockoidc.MockOIDC) []func(http.Handler) http.Handler { + return []func(http.Handler) http.Handler{ + newBadRequestMiddleware(), + } + }, + expectedError: "failed to discover OIDC configuration: unexpected status \"400\"", + }), + ) + + It("with code challenges supported on the provider, shold populate PKCE information", func() { + m, err := mockoidc.NewServer(nil) + Expect(err).ToNot(HaveOccurred()) + m.AddMiddleware(newCodeChallengeIssuerMiddleware(m)) + + ln, err := net.Listen("tcp", "127.0.0.1:0") + Expect(err).ToNot(HaveOccurred()) + + Expect(m.Start(ln, nil)).To(Succeed()) + defer func() { + Expect(m.Shutdown()).To(Succeed()) + }() + + provider, err := NewProvider(context.Background(), m.Issuer(), false) + Expect(err).ToNot(HaveOccurred()) + + Expect(provider.PKCE().CodeChallengeAlgs).To(ConsistOf("S256", "plain")) + }) +}) + +func newInvalidIssuerMiddleware(m *mockoidc.MockOIDC) func(http.Handler) http.Handler { + return func(next http.Handler) http.Handler { + return http.HandlerFunc(func(rw http.ResponseWriter, req *http.Request) { + p := providerJSON{ + Issuer: "invalid", + AuthURL: m.AuthorizationEndpoint(), + TokenURL: m.TokenEndpoint(), + JWKsURL: m.JWKSEndpoint(), + UserInfoURL: m.UserinfoEndpoint(), + } + data, err := json.Marshal(p) + if err != nil { + rw.WriteHeader(500) + } + rw.Write(data) + }) + } +} + +func newCodeChallengeIssuerMiddleware(m *mockoidc.MockOIDC) func(http.Handler) http.Handler { + return func(next http.Handler) http.Handler { + return http.HandlerFunc(func(rw http.ResponseWriter, req *http.Request) { + p := providerJSON{ + Issuer: m.Issuer(), + AuthURL: m.AuthorizationEndpoint(), + TokenURL: m.TokenEndpoint(), + JWKsURL: m.JWKSEndpoint(), + UserInfoURL: m.UserinfoEndpoint(), + CodeChallengeAlgs: []string{"S256", "plain"}, + } + data, err := json.Marshal(p) + if err != nil { + rw.WriteHeader(500) + } + rw.Write(data) + }) + } +} + +func newBadRequestMiddleware() func(http.Handler) http.Handler { + return func(next http.Handler) http.Handler { + return http.HandlerFunc(func(rw http.ResponseWriter, req *http.Request) { + rw.WriteHeader(400) + }) + } +} diff --git a/pkg/providers/oidc/provider_verifier.go b/pkg/providers/oidc/provider_verifier.go new file mode 100644 index 00000000..6da100a1 --- /dev/null +++ b/pkg/providers/oidc/provider_verifier.go @@ -0,0 +1,155 @@ +package oidc + +import ( + "context" + "errors" + "fmt" + + "github.com/coreos/go-oidc/v3/oidc" + k8serrors "k8s.io/apimachinery/pkg/util/errors" +) + +// ProviderVerifier represents the OIDC discovery and verification process +type ProviderVerifier interface { + DiscoveryEnabled() bool + Provider() DiscoveryProvider + Verifier() IDTokenVerifier +} + +// ProviderVerifierOptions allows you to configure a ProviderVerifier +type ProviderVerifierOptions struct { + // AudienceClaim allows to define any claim that is verified against the client id + // By default `aud` claim is used for verification. + AudienceClaims []string + + // ClientID is the OAuth Client ID that is defined in the provider + ClientID string + + // ExtraAudiences is a list of additional audiences that are allowed + // to pass verification in addition to the client id. + ExtraAudiences []string + + // IssuerURL is the OpenID Connect issuer URL + // eg: https://accounts.google.com + IssuerURL string + + // JWKsURL is the OpenID Connect JWKS URL + // eg: https://www.googleapis.com/oauth2/v3/certs + JWKsURL string + + // SkipDiscovery allows to skip OIDC discovery and use manually supplied Endpoints + SkipDiscovery bool + + // SkipIssuerVerification skips verification of ID token issuers. + // When false, ID Token Issuers must match the OIDC discovery URL. + SkipIssuerVerification bool +} + +// validate checks that the required options are present before attempting to create +// the ProviderVerifier. +func (p ProviderVerifierOptions) validate() error { + var errs []error + + if p.IssuerURL == "" { + errs = append(errs, errors.New("missing required setting: issuer-url")) + } + + if p.SkipDiscovery && p.JWKsURL == "" { + errs = append(errs, errors.New("missing required setting: jwks-url")) + } + + if len(errs) > 0 { + return k8serrors.NewAggregate(errs) + } + return nil +} + +// toVerificationOptions returns an IDTokenVerificationOptions based on the configured options. +func (p ProviderVerifierOptions) toVerificationOptions() IDTokenVerificationOptions { + return IDTokenVerificationOptions{ + AudienceClaims: p.AudienceClaims, + ClientID: p.ClientID, + ExtraAudiences: p.ExtraAudiences, + } +} + +// toOIDCConfig returns an oidc.Config based on the configured options. +func (p ProviderVerifierOptions) toOIDCConfig() *oidc.Config { + return &oidc.Config{ + ClientID: p.ClientID, + SkipIssuerCheck: p.SkipIssuerVerification, + SkipClientIDCheck: true, + } +} + +// NewProviderVerifier constructs a ProviderVerifier from the options given. +func NewProviderVerifier(ctx context.Context, opts ProviderVerifierOptions) (ProviderVerifier, error) { + if err := opts.validate(); err != nil { + return nil, fmt.Errorf("invalid provider verifier options: %v", err) + } + + verifierBuilder, provider, err := getVerifierBuilder(ctx, opts) + if err != nil { + return nil, fmt.Errorf("could not get verifier builder: %v", err) + } + verifier := NewVerifier(verifierBuilder(opts.toOIDCConfig()), opts.toVerificationOptions()) + + if provider == nil { + // To avoid the possibility of nil pointers, always return an empty provider if discovery didn't occur. + // Users are expected to check whether discovery was enabled before using the provider. + provider = &discoveryProvider{} + } + + return &providerVerifier{ + discoveryEnabled: !opts.SkipDiscovery, + provider: provider, + verifier: verifier, + }, nil +} + +type verifierBuilder func(*oidc.Config) *oidc.IDTokenVerifier + +func getVerifierBuilder(ctx context.Context, opts ProviderVerifierOptions) (verifierBuilder, DiscoveryProvider, error) { + if opts.SkipDiscovery { + // Instead of discovering the JWKs URK, it needs to be specified in the opts already + return newVerifierBuilder(ctx, opts.IssuerURL, opts.JWKsURL), nil, nil + } + + provider, err := NewProvider(ctx, opts.IssuerURL, opts.SkipIssuerVerification) + if err != nil { + return nil, nil, fmt.Errorf("error while discovery OIDC configuration: %v", err) + } + verifierBuilder := newVerifierBuilder(ctx, opts.IssuerURL, provider.Endpoints().JWKsURL) + return verifierBuilder, provider, nil +} + +// newVerifierBuilder returns a function to create a IDToken verifier from an OIDC config. +func newVerifierBuilder(ctx context.Context, issuerURL, jwksURL string) verifierBuilder { + keySet := oidc.NewRemoteKeySet(ctx, jwksURL) + return func(oidcConfig *oidc.Config) *oidc.IDTokenVerifier { + return oidc.NewVerifier(issuerURL, keySet, oidcConfig) + } +} + +// providerVerifier is an implementation of the ProviderVerifier interface +type providerVerifier struct { + discoveryEnabled bool + provider DiscoveryProvider + verifier IDTokenVerifier +} + +// DiscoveryEnabled returns whether the provider verifier was constructed +// using the OIDC discovery process or whether it was manually discovered. +func (p *providerVerifier) DiscoveryEnabled() bool { + return p.discoveryEnabled +} + +// Provider returns the OIDC discovery provider +func (p *providerVerifier) Provider() DiscoveryProvider { + return p.provider +} + +// Verifier returns the ID token verifier +func (p *providerVerifier) Verifier() IDTokenVerifier { + return p.verifier +} diff --git a/pkg/providers/oidc/provider_verifier_test.go b/pkg/providers/oidc/provider_verifier_test.go new file mode 100644 index 00000000..86bfff94 --- /dev/null +++ b/pkg/providers/oidc/provider_verifier_test.go @@ -0,0 +1,173 @@ +package oidc + +import ( + "context" + "time" + + "github.com/dgrijalva/jwt-go" + "github.com/oauth2-proxy/mockoidc" + . "github.com/onsi/ginkgo" + . "github.com/onsi/ginkgo/extensions/table" + . "github.com/onsi/gomega" +) + +var _ = Describe("ProviderVerifier", func() { + var m *mockoidc.MockOIDC + + BeforeEach(func() { + var err error + m, err = mockoidc.Run() + Expect(err).ToNot(HaveOccurred()) + }) + + AfterEach(func() { + Expect(m.Shutdown()).To(Succeed()) + }) + + type newProviderVerifierTableInput struct { + modifyOpts func(*ProviderVerifierOptions) + expectedError string + } + + DescribeTable("when constructing the provider verifier", func(in *newProviderVerifierTableInput) { + opts := ProviderVerifierOptions{ + AudienceClaims: []string{"aud"}, + ClientID: m.Config().ClientID, + ExtraAudiences: []string{}, + IssuerURL: m.Issuer(), + } + if in.modifyOpts != nil { + in.modifyOpts(&opts) + } + + pv, err := NewProviderVerifier(context.Background(), opts) + if in.expectedError != "" { + Expect(err).To(MatchError(HavePrefix(in.expectedError))) + return + } + Expect(err).ToNot(HaveOccurred()) + + Expect(pv.DiscoveryEnabled()).ToNot(Equal(opts.SkipDiscovery), "DiscoveryEnabled should be the reverse of skip discovery") + Expect(pv.Provider()).ToNot(BeNil()) + + if pv.DiscoveryEnabled() { + endpoints := pv.Provider().Endpoints() + Expect(endpoints.AuthURL).To(Equal(m.AuthorizationEndpoint())) + Expect(endpoints.TokenURL).To(Equal(m.TokenEndpoint())) + Expect(endpoints.JWKsURL).To(Equal(m.JWKSEndpoint())) + Expect(endpoints.UserInfoURL).To(Equal(m.UserinfoEndpoint())) + } + }, + Entry("should be succesfful when discovering the OIDC provider", &newProviderVerifierTableInput{ + modifyOpts: func(_ *ProviderVerifierOptions) {}, + }), + Entry("when the issuer URL is missing", &newProviderVerifierTableInput{ + modifyOpts: func(p *ProviderVerifierOptions) { + p.IssuerURL = "" + }, + expectedError: "invalid provider verifier options: missing required setting: issuer-url", + }), + Entry("when the issuer URL is invalid", &newProviderVerifierTableInput{ + modifyOpts: func(p *ProviderVerifierOptions) { + p.IssuerURL = "invalid" + }, + expectedError: "could not get verifier builder: error while discovery OIDC configuration: failed to discover OIDC configuration: error performing request: Get \"invalid/.well-known/openid-configuration\": unsupported protocol scheme \"\"", + }), + Entry("with skip discovery and the JWKs URL is missing", &newProviderVerifierTableInput{ + modifyOpts: func(p *ProviderVerifierOptions) { + p.SkipDiscovery = true + p.JWKsURL = "" + }, + expectedError: "invalid provider verifier options: missing required setting: jwks-url", + }), + Entry("should be succesfful when skipping discovery with the JWKs URL specified", &newProviderVerifierTableInput{ + modifyOpts: func(p *ProviderVerifierOptions) { + p.SkipDiscovery = true + p.JWKsURL = m.JWKSEndpoint() + }, + }), + ) + + type verifierTableInput struct { + modifyOpts func(*ProviderVerifierOptions) + modifyClaims func(*jwt.StandardClaims) + expectedError string + } + + DescribeTable("when constructing the provider verifier", func(in *verifierTableInput) { + opts := ProviderVerifierOptions{ + AudienceClaims: []string{"aud"}, + ClientID: m.Config().ClientID, + ExtraAudiences: []string{}, + IssuerURL: m.Issuer(), + } + if in.modifyOpts != nil { + in.modifyOpts(&opts) + } + + pv, err := NewProviderVerifier(context.Background(), opts) + Expect(err).ToNot(HaveOccurred()) + + now := time.Now() + claims := jwt.StandardClaims{ + Audience: m.Config().ClientID, + Issuer: m.Issuer(), + ExpiresAt: now.Add(1 * time.Hour).Unix(), + IssuedAt: now.Unix(), + Subject: "user", + } + if in.modifyClaims != nil { + in.modifyClaims(&claims) + } + + rawIDToken, err := m.Keypair.SignJWT(claims) + Expect(err).ToNot(HaveOccurred()) + + idToken, err := pv.Verifier().Verify(context.Background(), rawIDToken) + if in.expectedError != "" { + Expect(err).To(MatchError(HavePrefix(in.expectedError))) + return + } + Expect(err).ToNot(HaveOccurred()) + + Expect(idToken.Issuer).To(Equal(claims.Issuer)) + Expect(idToken.Audience).To(ConsistOf(claims.Audience)) + Expect(idToken.Subject).To(Equal(claims.Subject)) + }, + Entry("with the default opts and claims", &verifierTableInput{}), + Entry("when the audience is mismatched", &verifierTableInput{ + modifyClaims: func(j *jwt.StandardClaims) { + j.Audience = "OtherClient" + }, + expectedError: "audience from claim aud with value [OtherClient] does not match with any of allowed audiences", + }), + Entry("when the audience is an extra audience", &verifierTableInput{ + modifyOpts: func(p *ProviderVerifierOptions) { + p.ExtraAudiences = []string{"ExtraIssuer"} + }, + modifyClaims: func(j *jwt.StandardClaims) { + j.Audience = "ExtraIssuer" + }, + }), + Entry("when the issuer is mismatched", &verifierTableInput{ + modifyClaims: func(j *jwt.StandardClaims) { + j.Issuer = "OtherIssuer" + }, + expectedError: "failed to verify token: oidc: id token issued by a different provider", + }), + Entry("when the issuer is mismatched with skip issuer verification", &verifierTableInput{ + modifyOpts: func(p *ProviderVerifierOptions) { + p.SkipIssuerVerification = true + }, + modifyClaims: func(j *jwt.StandardClaims) { + j.Issuer = "OtherIssuer" + }, + }), + Entry("when the token has expired", &verifierTableInput{ + modifyClaims: func(j *jwt.StandardClaims) { + j.ExpiresAt = time.Now().Add(-1 * time.Hour).Unix() + }, + expectedError: "failed to verify token: oidc: token is expired", + }), + ) +}) diff --git a/pkg/oidc/verify.go b/pkg/providers/oidc/verifier.go similarity index 65% rename from pkg/oidc/verify.go rename to pkg/providers/oidc/verifier.go index c0ad9b80..34c4a006 100755 --- a/pkg/oidc/verify.go +++ b/pkg/providers/oidc/verifier.go @@ -8,33 +8,42 @@ import ( "github.com/coreos/go-oidc/v3/oidc" ) -// IDTokenVerifier Used to verify an ID Token and extends oidc.IDTokenVerifier from the underlying oidc library -type IDTokenVerifier struct { - *oidc.IDTokenVerifier - *IDTokenVerificationOptions - allowedAudiences map[string]struct{} +// idTokenVerifier allows an ID Token to be verified against the issue and provided keys. +type IDTokenVerifier interface { + Verify(context.Context, string) (*oidc.IDToken, error) } -// IDTokenVerificationOptions options for the oidc.IDTokenVerifier that are required to verify an ID Token +// idTokenVerifier Used to verify an ID Token and extends oidc.idTokenVerifier from the underlying oidc library +type idTokenVerifier struct { + verifier *oidc.IDTokenVerifier + verificationOptions IDTokenVerificationOptions + allowedAudiences map[string]struct{} +} + +// IDTokenVerificationOptions options for the oidc.idTokenVerifier that are required to verify an ID Token type IDTokenVerificationOptions struct { AudienceClaims []string ClientID string ExtraAudiences []string } -// NewVerifier constructs a new IDTokenVerifier -func NewVerifier(iv *oidc.IDTokenVerifier, vo *IDTokenVerificationOptions) *IDTokenVerifier { +// NewVerifier constructs a new idTokenVerifier +func NewVerifier(iv *oidc.IDTokenVerifier, vo IDTokenVerificationOptions) IDTokenVerifier { allowedAudiences := make(map[string]struct{}) allowedAudiences[vo.ClientID] = struct{}{} for _, extraAudience := range vo.ExtraAudiences { allowedAudiences[extraAudience] = struct{}{} } - return &IDTokenVerifier{iv, vo, allowedAudiences} + return &idTokenVerifier{ + verifier: iv, + verificationOptions: vo, + allowedAudiences: allowedAudiences, + } } // Verify verifies incoming ID Token -func (v *IDTokenVerifier) Verify(ctx context.Context, rawIDToken string) (*oidc.IDToken, error) { - token, err := v.IDTokenVerifier.Verify(ctx, rawIDToken) +func (v *idTokenVerifier) Verify(ctx context.Context, rawIDToken string) (*oidc.IDToken, error) { + token, err := v.verifier.Verify(ctx, rawIDToken) if err != nil { return nil, fmt.Errorf("failed to verify token: %v", err) } @@ -51,8 +60,8 @@ func (v *IDTokenVerifier) Verify(ctx context.Context, rawIDToken string) (*oidc. return token, err } -func (v *IDTokenVerifier) verifyAudience(token *oidc.IDToken, claims map[string]interface{}) (bool, error) { - for _, audienceClaim := range v.AudienceClaims { +func (v *idTokenVerifier) verifyAudience(token *oidc.IDToken, claims map[string]interface{}) (bool, error) { + for _, audienceClaim := range v.verificationOptions.AudienceClaims { if audienceClaimValue, audienceClaimExists := claims[audienceClaim]; audienceClaimExists { // audience claim value can be either interface{} or []interface{}, @@ -72,10 +81,10 @@ func (v *IDTokenVerifier) verifyAudience(token *oidc.IDToken, claims map[string] } return false, fmt.Errorf("audience claims %v do not exist in claims: %v", - v.AudienceClaims, claims) + v.verificationOptions.AudienceClaims, claims) } -func (v *IDTokenVerifier) isValidAudience(claim string, audience []string, allowedAudiences map[string]struct{}) (bool, error) { +func (v *idTokenVerifier) isValidAudience(claim string, audience []string, allowedAudiences map[string]struct{}) (bool, error) { for _, aud := range audience { if _, allowedAudienceExists := allowedAudiences[aud]; allowedAudienceExists { return true, nil @@ -87,7 +96,7 @@ func (v *IDTokenVerifier) isValidAudience(claim string, audience []string, allow claim, audience, allowedAudiences) } -func (v *IDTokenVerifier) interfaceSliceToString(slice interface{}) []string { +func (v *idTokenVerifier) interfaceSliceToString(slice interface{}) []string { s := reflect.ValueOf(slice) if s.Kind() != reflect.Slice { panic(fmt.Sprintf("given a non-slice type %s", s.Kind())) diff --git a/pkg/oidc/verify_test.go b/pkg/providers/oidc/verifier_test.go similarity index 89% rename from pkg/oidc/verify_test.go rename to pkg/providers/oidc/verifier_test.go index a9aaece0..c82de7af 100755 --- a/pkg/oidc/verify_test.go +++ b/pkg/providers/oidc/verifier_test.go @@ -17,7 +17,7 @@ var _ = Describe("Verify", func() { ctx := context.Background() It("Succeeds with default aud behavior", func() { - result, err := verify(ctx, &IDTokenVerificationOptions{ + result, err := verify(ctx, IDTokenVerificationOptions{ AudienceClaims: []string{"aud"}, ClientID: "1226737", ExtraAudiences: []string{}, @@ -32,7 +32,7 @@ var _ = Describe("Verify", func() { }) It("Fails with default aud behavior", func() { - result, err := verify(ctx, &IDTokenVerificationOptions{ + result, err := verify(ctx, IDTokenVerificationOptions{ AudienceClaims: []string{"aud"}, ClientID: "7817818", ExtraAudiences: []string{}, @@ -46,7 +46,7 @@ var _ = Describe("Verify", func() { }) It("Succeeds with extra audiences", func() { - result, err := verify(ctx, &IDTokenVerificationOptions{ + result, err := verify(ctx, IDTokenVerificationOptions{ AudienceClaims: []string{"aud"}, ClientID: "7817818", ExtraAudiences: []string{"xyz", "1226737"}, @@ -61,7 +61,7 @@ var _ = Describe("Verify", func() { }) It("Fails with extra audiences", func() { - result, err := verify(ctx, &IDTokenVerificationOptions{ + result, err := verify(ctx, IDTokenVerificationOptions{ AudienceClaims: []string{"aud"}, ClientID: "7817818", ExtraAudiences: []string{"xyz", "abc"}, @@ -76,7 +76,7 @@ var _ = Describe("Verify", func() { }) It("Succeeds with non default aud behavior", func() { - result, err := verify(ctx, &IDTokenVerificationOptions{ + result, err := verify(ctx, IDTokenVerificationOptions{ AudienceClaims: []string{"client_id"}, ClientID: "1226737", ExtraAudiences: []string{}, @@ -91,7 +91,7 @@ var _ = Describe("Verify", func() { }) It("Fails with non default aud behavior", func() { - result, err := verify(ctx, &IDTokenVerificationOptions{ + result, err := verify(ctx, IDTokenVerificationOptions{ AudienceClaims: []string{"client_id"}, ClientID: "7817818", ExtraAudiences: []string{}, @@ -105,7 +105,7 @@ var _ = Describe("Verify", func() { }) It("Succeeds with non default aud behavior and extra audiences", func() { - result, err := verify(ctx, &IDTokenVerificationOptions{ + result, err := verify(ctx, IDTokenVerificationOptions{ AudienceClaims: []string{"client_id"}, ClientID: "7817818", ExtraAudiences: []string{"xyz", "1226737"}, @@ -120,7 +120,7 @@ var _ = Describe("Verify", func() { }) It("Fails with non default aud behavior and extra audiences", func() { - result, err := verify(ctx, &IDTokenVerificationOptions{ + result, err := verify(ctx, IDTokenVerificationOptions{ AudienceClaims: []string{"client_id"}, ClientID: "7817818", ExtraAudiences: []string{"xyz", "abc"}, @@ -135,7 +135,7 @@ var _ = Describe("Verify", func() { }) It("Fails if audience claim does not exist", func() { - result, err := verify(ctx, &IDTokenVerificationOptions{ + result, err := verify(ctx, IDTokenVerificationOptions{ AudienceClaims: []string{"not_exists"}, ClientID: "7817818", ExtraAudiences: []string{"xyz", "abc"}, @@ -151,7 +151,7 @@ var _ = Describe("Verify", func() { }) It("Succeeds with multiple audiences", func() { - var result, err = verify(ctx, &IDTokenVerificationOptions{ + var result, err = verify(ctx, IDTokenVerificationOptions{ AudienceClaims: []string{"client_id", "aud"}, ClientID: "123456789", ExtraAudiences: []string{"1226737"}, @@ -167,7 +167,7 @@ var _ = Describe("Verify", func() { }) It("Succeeds if aud claim match", func() { - result, err := verify(ctx, &IDTokenVerificationOptions{ + result, err := verify(ctx, IDTokenVerificationOptions{ AudienceClaims: []string{"client_id", "aud"}, ClientID: "1226737", ExtraAudiences: []string{"xyz", "abc"}, @@ -207,7 +207,7 @@ func (t *testVerifier) VerifySignature(ctx context.Context, jwt string) ([]byte, return jws.Verify(&t.jwk) } -func verify(ctx context.Context, verificationOptions *IDTokenVerificationOptions, payload payload) (*oidc.IDToken, error) { +func verify(ctx context.Context, verificationOptions IDTokenVerificationOptions, payload payload) (*oidc.IDToken, error) { config := &oidc.Config{ ClientID: "1226737", SkipClientIDCheck: true, diff --git a/pkg/validation/options.go b/pkg/validation/options.go index 771d076f..cd8f24f9 100644 --- a/pkg/validation/options.go +++ b/pkg/validation/options.go @@ -8,13 +8,11 @@ import ( "net/url" "strings" - "github.com/coreos/go-oidc/v3/oidc" "github.com/mbland/hmacauth" "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/apis/options" "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/ip" "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" + internaloidc "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/providers/oidc" "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/util" ) @@ -148,32 +146,27 @@ func parseJwtIssuers(issuers []string, msgs []string) ([]jwtIssuer, []string) { // newVerifierFromJwtIssuer takes in issuer information in jwtIssuer info and returns // a verifier for that issuer. -func newVerifierFromJwtIssuer(audienceClaims []string, extraAudiences []string, jwtIssuer jwtIssuer) (*internaloidc.IDTokenVerifier, error) { - config := &oidc.Config{ - ClientID: jwtIssuer.audience, - SkipClientIDCheck: true, // client id check is done within oauth2-proxy: IDTokenVerifier.Verify - } - // Try as an OpenID Connect Provider first - var verifier *oidc.IDTokenVerifier - provider, err := oidc.NewProvider(context.Background(), jwtIssuer.issuerURI) - if err != nil { - // Try as JWKS URI - jwksURI := strings.TrimSuffix(jwtIssuer.issuerURI, "/") + "/.well-known/jwks.json" - if err := requests.New(jwksURI).Do().Error(); err != nil { - return nil, err - } - - verifier = oidc.NewVerifier(jwtIssuer.issuerURI, oidc.NewRemoteKeySet(context.Background(), jwksURI), config) - } else { - verifier = provider.Verifier(config) - } - verificationOptions := &internaloidc.IDTokenVerificationOptions{ +func newVerifierFromJwtIssuer(audienceClaims []string, extraAudiences []string, jwtIssuer jwtIssuer) (internaloidc.IDTokenVerifier, error) { + pvOpts := internaloidc.ProviderVerifierOptions{ AudienceClaims: audienceClaims, ClientID: jwtIssuer.audience, ExtraAudiences: extraAudiences, - // ExtraAudiences: o.Providers[0].OIDCConfig.ExtraAudiences, + IssuerURL: jwtIssuer.issuerURI, } - return internaloidc.NewVerifier(verifier, verificationOptions), nil + + pv, err := internaloidc.NewProviderVerifier(context.TODO(), pvOpts) + if err != nil { + // If the discovery didn't work, try again without discovery + pvOpts.JWKsURL = strings.TrimSuffix(jwtIssuer.issuerURI, "/") + "/.well-known/jwks.json" + pvOpts.SkipDiscovery = true + + pv, err = internaloidc.NewProviderVerifier(context.TODO(), pvOpts) + if err != nil { + return nil, fmt.Errorf("could not construct provider verifier for JWT Issuer: %v", err) + } + } + + return pv.Verifier(), nil } // jwtIssuer hold parsed JWT issuer info that's used to construct a verifier. diff --git a/providers/adfs_test.go b/providers/adfs_test.go index 434175d9..19356b75 100755 --- a/providers/adfs_test.go +++ b/providers/adfs_test.go @@ -15,7 +15,7 @@ import ( "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" + internaloidc "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/providers/oidc" . "github.com/onsi/ginkgo" . "github.com/onsi/ginkgo/extensions/table" . "github.com/onsi/gomega" @@ -43,7 +43,7 @@ func newSignedTestADFSToken(tokenClaims adfsClaims) (string, error) { } func testADFSProvider(hostname string) *ADFSProvider { - verificationOptions := &internaloidc.IDTokenVerificationOptions{ + verificationOptions := internaloidc.IDTokenVerificationOptions{ AudienceClaims: []string{"aud"}, ClientID: "https://test.myapp.com", } diff --git a/providers/azure_test.go b/providers/azure_test.go index 5e912851..794ed570 100644 --- a/providers/azure_test.go +++ b/providers/azure_test.go @@ -17,7 +17,7 @@ import ( "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" + internaloidc "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/providers/oidc" . "github.com/onsi/gomega" "github.com/stretchr/testify/assert" @@ -41,7 +41,7 @@ type azureOAuthPayload struct { } func testAzureProvider(hostname string, opts options.AzureOptions) *AzureProvider { - verificationOptions := &internaloidc.IDTokenVerificationOptions{ + verificationOptions := internaloidc.IDTokenVerificationOptions{ AudienceClaims: []string{"aud"}, ClientID: "cd6d4fae-f6a6-4a34-8454-2c6b598e9532", } diff --git a/providers/keycloak_oidc_test.go b/providers/keycloak_oidc_test.go index 1e1cb734..d1a50b2a 100644 --- a/providers/keycloak_oidc_test.go +++ b/providers/keycloak_oidc_test.go @@ -14,7 +14,7 @@ import ( . "github.com/onsi/ginkgo" . "github.com/onsi/gomega" - internaloidc "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/oidc" + internaloidc "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/providers/oidc" ) const ( @@ -45,7 +45,7 @@ func newTestKeycloakOIDCSetup() (*httptest.Server, *KeycloakOIDCProvider) { } func newKeycloakOIDCProvider(serverURL *url.URL, opts options.KeycloakOptions) *KeycloakOIDCProvider { - verificationOptions := &internaloidc.IDTokenVerificationOptions{ + verificationOptions := internaloidc.IDTokenVerificationOptions{ AudienceClaims: []string{defaultAudienceClaim}, ClientID: mockClientID, } diff --git a/providers/oidc_test.go b/providers/oidc_test.go index b462de59..1341830a 100644 --- a/providers/oidc_test.go +++ b/providers/oidc_test.go @@ -14,7 +14,7 @@ import ( "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" + internaloidc "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/providers/oidc" "github.com/stretchr/testify/assert" ) @@ -27,7 +27,7 @@ type redeemTokenResponse struct { } func newOIDCProvider(serverURL *url.URL, skipNonce bool) *OIDCProvider { - verificationOptions := &internaloidc.IDTokenVerificationOptions{ + verificationOptions := internaloidc.IDTokenVerificationOptions{ AudienceClaims: []string{"aud"}, ClientID: "https://test.myapp.com", } diff --git a/providers/provider_data.go b/providers/provider_data.go index 03eeb93e..9bcb8729 100644 --- a/providers/provider_data.go +++ b/providers/provider_data.go @@ -13,7 +13,7 @@ import ( "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" + internaloidc "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/providers/oidc" "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/providers/util" "golang.org/x/oauth2" ) @@ -47,7 +47,7 @@ type ProviderData struct { UserClaim string EmailClaim string GroupsClaim string - Verifier *internaloidc.IDTokenVerifier + Verifier internaloidc.IDTokenVerifier // Universal Group authorization data structure // any provider can set to consume diff --git a/providers/provider_data_test.go b/providers/provider_data_test.go index 64c8326d..40892cbc 100644 --- a/providers/provider_data_test.go +++ b/providers/provider_data_test.go @@ -16,7 +16,7 @@ import ( "github.com/golang-jwt/jwt" "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" + internaloidc "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/providers/oidc" . "github.com/onsi/gomega" "golang.org/x/oauth2" ) @@ -202,7 +202,7 @@ func TestProviderData_verifyIDToken(t *testing.T) { provider := &ProviderData{} if tc.Verifier { - verificationOptions := &internaloidc.IDTokenVerificationOptions{ + verificationOptions := internaloidc.IDTokenVerificationOptions{ AudienceClaims: []string{"aud"}, ClientID: oidcClientID, } @@ -409,7 +409,7 @@ func TestProviderData_buildSessionFromClaims(t *testing.T) { t.Run(testName, func(t *testing.T) { g := NewWithT(t) - verificationOptions := &internaloidc.IDTokenVerificationOptions{ + verificationOptions := internaloidc.IDTokenVerificationOptions{ AudienceClaims: []string{"aud"}, ClientID: oidcClientID, } @@ -478,7 +478,7 @@ func TestProviderData_checkNonce(t *testing.T) { g.Expect(err).ToNot(HaveOccurred()) tc.Session.IDToken = rawIDToken - verificationOptions := &internaloidc.IDTokenVerificationOptions{ + verificationOptions := internaloidc.IDTokenVerificationOptions{ AudienceClaims: []string{"aud"}, ClientID: oidcClientID, } diff --git a/providers/providers.go b/providers/providers.go index 84dcc97f..2993dc2c 100644 --- a/providers/providers.go +++ b/providers/providers.go @@ -2,17 +2,12 @@ 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" + internaloidc "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/providers/oidc" k8serrors "k8s.io/apimachinery/pkg/util/errors" ) @@ -86,16 +81,27 @@ func newProviderDataFromConfig(providerConfig options.Provider) (*ProviderData, } if needsVerifier { - oidcProvider, verifier, err := newOIDCProviderVerifier(providerConfig) + pv, err := internaloidc.NewProviderVerifier(context.TODO(), internaloidc.ProviderVerifierOptions{ + AudienceClaims: providerConfig.OIDCConfig.AudienceClaims, + ClientID: providerConfig.ClientID, + ExtraAudiences: providerConfig.OIDCConfig.ExtraAudiences, + IssuerURL: providerConfig.OIDCConfig.IssuerURL, + JWKsURL: providerConfig.OIDCConfig.JwksURL, + SkipDiscovery: providerConfig.OIDCConfig.SkipDiscovery, + SkipIssuerVerification: providerConfig.OIDCConfig.InsecureSkipIssuerVerification, + }) if err != nil { - return nil, fmt.Errorf("error setting OIDC configuration: %v", err) + return nil, fmt.Errorf("error building OIDC ProviderVerifier: %v", err) } - p.Verifier = verifier - if oidcProvider != nil { + p.Verifier = pv.Verifier() + if pv.DiscoveryEnabled() { // Use the discovered values rather than any specified values - providerConfig.LoginURL = oidcProvider.Endpoint().AuthURL - providerConfig.RedeemURL = oidcProvider.Endpoint().TokenURL + endpoints := pv.Provider().Endpoints() + providerConfig.LoginURL = endpoints.AuthURL + providerConfig.RedeemURL = endpoints.TokenURL + providerConfig.ProfileURL = endpoints.UserInfoURL + providerConfig.OIDCConfig.JwksURL = endpoints.JWKsURL } } @@ -159,118 +165,3 @@ func providerRequiresOIDCProviderVerifier(providerType options.ProviderType) (bo 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 index 38917b70..02521ad7 100644 --- a/providers/providers_test.go +++ b/providers/providers_test.go @@ -87,7 +87,7 @@ func TestSkipOIDCDiscovery(t *testing.T) { } _, 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]")) + g.Expect(err).To(MatchError("error building OIDC ProviderVerifier: invalid provider verifier options: missing required setting: jwks-url")) providerConfig.LoginURL = msAuthURL providerConfig.RedeemURL = msTokenURL