From a53198725ea7c4127d1d0bf55b56fecc013d61b4 Mon Sep 17 00:00:00 2001 From: Nick Meves Date: Sat, 19 Jun 2021 16:06:58 -0700 Subject: [PATCH 1/5] Use `upn` as EmailClaim throughout ADFSProvider By only overriding in the EnrichSession, any Refresh calls would've overriden it with the `email` claim. --- CHANGELOG.md | 1 + providers/adfs.go | 52 +++++++++--------------------------------- providers/adfs_test.go | 4 ++-- 3 files changed, 14 insertions(+), 43 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 8defa4f8..b94310f4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,7 @@ ## Changes since v7.2.0 +- [#1247](https://github.com/oauth2-proxy/oauth2-proxy/pull/1247) Use `upn` claim consistently in ADFSProvider (@NickMeves) - [#1447](https://github.com/oauth2-proxy/oauth2-proxy/pull/1447) Fix docker build/push issues found during last release (@JoelSpeed) - [#1433](https://github.com/oauth2-proxy/oauth2-proxy/pull/1433) Let authentication fail when session validation fails (@stippi2) - [#1445](https://github.com/oauth2-proxy/oauth2-proxy/pull/1445) Fix docker container multi arch build issue by passing GOARCH details to make build (@jkandasa) diff --git a/providers/adfs.go b/providers/adfs.go index c626bd09..89119c7a 100644 --- a/providers/adfs.go +++ b/providers/adfs.go @@ -1,36 +1,32 @@ package providers import ( - "context" - "errors" - "fmt" "net/url" "strings" - - "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/apis/sessions" ) // ADFSProvider represents an ADFS based Identity Provider type ADFSProvider struct { *OIDCProvider - SkipScope bool + skipScope bool } var _ Provider = (*ADFSProvider)(nil) const ( - ADFSProviderName = "ADFS" - ADFSDefaultScope = "openid email profile" - ADFSSkipScope = false + adfsProviderName = "ADFS" + adfsDefaultScope = "openid email profile" + adfsSkipScope = false + adfsEmailClaim = "upn" ) // NewADFSProvider initiates a new ADFSProvider func NewADFSProvider(p *ProviderData) *ADFSProvider { - p.setProviderDefaults(providerDefaults{ - name: ADFSProviderName, - scope: ADFSDefaultScope, + name: adfsProviderName, + scope: adfsDefaultScope, }) + p.EmailClaim = adfsEmailClaim if p.ProtectedResource != nil && p.ProtectedResource.String() != "" { resource := p.ProtectedResource.String() @@ -48,13 +44,13 @@ func NewADFSProvider(p *ProviderData) *ADFSProvider { ProviderData: p, SkipNonce: true, }, - SkipScope: ADFSSkipScope, + skipScope: adfsSkipScope, } } // Configure defaults the ADFSProvider configuration options func (p *ADFSProvider) Configure(skipScope bool) { - p.SkipScope = skipScope + p.skipScope = skipScope } // GetLoginURL Override to double encode the state parameter. If not query params are lost @@ -65,36 +61,10 @@ func (p *ADFSProvider) GetLoginURL(redirectURI, state, nonce string) string { extraParams.Add("nonce", nonce) } loginURL := makeLoginURL(p.Data(), redirectURI, url.QueryEscape(state), extraParams) - if p.SkipScope { + if p.skipScope { q := loginURL.Query() q.Del("scope") loginURL.RawQuery = q.Encode() } return loginURL.String() } - -// EnrichSession to add email -func (p *ADFSProvider) EnrichSession(ctx context.Context, s *sessions.SessionState) error { - if s.Email != "" { - return nil - } - - idToken, err := p.Verifier.Verify(ctx, s.IDToken) - if err != nil { - return err - } - - p.EmailClaim = "upn" - c, err := p.getClaims(idToken) - - if err != nil { - return fmt.Errorf("couldn't extract claims from id_token (%v)", err) - } - s.Email = c.Email - - if s.Email == "" { - err = errors.New("email not set") - } - - return err -} diff --git a/providers/adfs_test.go b/providers/adfs_test.go index bd2a82ce..ae306cd1 100644 --- a/providers/adfs_test.go +++ b/providers/adfs_test.go @@ -134,8 +134,8 @@ var _ = Describe("ADFS Provider Tests", func() { idToken, err := p.Verifier.Verify(context.Background(), rawIDToken) Expect(err).To(BeNil()) session, err := p.buildSessionFromClaims(idToken) - session.IDToken = rawIDToken Expect(err).To(BeNil()) + session.IDToken = rawIDToken err = p.EnrichSession(context.Background(), session) Expect(session.Email).To(Equal("janed@me.com")) Expect(err).To(BeNil()) @@ -149,7 +149,7 @@ var _ = Describe("ADFS Provider Tests", func() { ProtectedResource: resource, Scope: "", }) - p.SkipScope = true + p.skipScope = true result := p.GetLoginURL("https://example.com/adfs/oauth2/", "", "") Expect(result).NotTo(ContainSubstring("scope=")) From 4980f6af7d86337aa66cfb3dfbebccb0d9df8b80 Mon Sep 17 00:00:00 2001 From: Nick Meves Date: Tue, 22 Jun 2021 18:50:47 -0700 Subject: [PATCH 2/5] Use upn claim as a fallback in Enrich & Refresh Only when `email` claim is missing, fallback to `upn` claim which may have it. --- providers/adfs.go | 66 ++++++++++++++++++++++++++++++---- providers/adfs_test.go | 82 ++++++++++++++++++++++++++++++++++++++++-- 2 files changed, 138 insertions(+), 10 deletions(-) diff --git a/providers/adfs.go b/providers/adfs.go index 89119c7a..95177d76 100644 --- a/providers/adfs.go +++ b/providers/adfs.go @@ -1,14 +1,22 @@ package providers import ( + "context" + "fmt" "net/url" "strings" + + "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/apis/sessions" ) // ADFSProvider represents an ADFS based Identity Provider type ADFSProvider struct { *OIDCProvider + skipScope bool + // Expose for unit testing + oidcEnrichFunc func(context.Context, *sessions.SessionState) error + oidcRefreshFunc func(context.Context, *sessions.SessionState) (bool, error) } var _ Provider = (*ADFSProvider)(nil) @@ -17,7 +25,7 @@ const ( adfsProviderName = "ADFS" adfsDefaultScope = "openid email profile" adfsSkipScope = false - adfsEmailClaim = "upn" + adfsUPNClaim = "upn" ) // NewADFSProvider initiates a new ADFSProvider @@ -26,7 +34,6 @@ func NewADFSProvider(p *ProviderData) *ADFSProvider { name: adfsProviderName, scope: adfsDefaultScope, }) - p.EmailClaim = adfsEmailClaim if p.ProtectedResource != nil && p.ProtectedResource.String() != "" { resource := p.ProtectedResource.String() @@ -39,12 +46,16 @@ func NewADFSProvider(p *ProviderData) *ADFSProvider { } } + oidcProvider := &OIDCProvider{ + ProviderData: p, + SkipNonce: true, + } + return &ADFSProvider{ - OIDCProvider: &OIDCProvider{ - ProviderData: p, - SkipNonce: true, - }, - skipScope: adfsSkipScope, + OIDCProvider: oidcProvider, + skipScope: adfsSkipScope, + oidcEnrichFunc: oidcProvider.EnrichSession, + oidcRefreshFunc: oidcProvider.RefreshSession, } } @@ -68,3 +79,44 @@ func (p *ADFSProvider) GetLoginURL(redirectURI, state, nonce string) string { } return loginURL.String() } + +// EnrichSession calls the OIDC ProfileURL to backfill any fields missing +// from the claims. If Email is missing, falls back to ADFS `upn` claim. +func (p *ADFSProvider) EnrichSession(ctx context.Context, s *sessions.SessionState) error { + err := p.oidcEnrichFunc(ctx, s) + if err != nil { + return err + } + + if s.Email == "" { + return p.fallbackUPN(ctx, s) + } + return nil +} + +// RefreshSession refreshes via the OIDC implementation. If email is missing, +// falls back to ADFS `upn` claim. +func (p *ADFSProvider) RefreshSession(ctx context.Context, s *sessions.SessionState) (bool, error) { + refreshed, err := p.oidcRefreshFunc(ctx, s) + if err != nil || s.Email != "" { + return refreshed, err + } + err = p.fallbackUPN(ctx, s) + return refreshed, err +} + +func (p *ADFSProvider) fallbackUPN(ctx context.Context, s *sessions.SessionState) error { + idToken, err := p.Verifier.Verify(ctx, s.IDToken) + if err != nil { + return err + } + claims, err := p.getClaims(idToken) + if err != nil { + return fmt.Errorf("couldn't extract claims from id_token (%v)", err) + } + upn := claims.raw[adfsUPNClaim] + if upn != nil { + s.Email = fmt.Sprint(upn) + } + return nil +} diff --git a/providers/adfs_test.go b/providers/adfs_test.go index ae306cd1..adbec455 100644 --- a/providers/adfs_test.go +++ b/providers/adfs_test.go @@ -2,6 +2,8 @@ package providers import ( "context" + "crypto/rand" + "crypto/rsa" "encoding/base64" "net/http" "net/http/httptest" @@ -9,6 +11,7 @@ import ( "strings" "github.com/coreos/go-oidc/v3/oidc" + "github.com/dgrijalva/jwt-go" "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/apis/sessions" . "github.com/onsi/ginkgo" . "github.com/onsi/ginkgo/extensions/table" @@ -25,8 +28,18 @@ func (fakeADFSJwks) VerifySignature(_ context.Context, jwt string) (payload []by return decodeString, nil } -func testADFSProvider(hostname string) *ADFSProvider { +type adfsClaims struct { + UPN string `json:"upn,omitempty"` + idTokenClaims +} +func newSignedTestADFSToken(tokenClaims adfsClaims) (string, error) { + key, _ := rsa.GenerateKey(rand.Reader, 2048) + standardClaims := jwt.NewWithClaims(jwt.SigningMethodRS256, tokenClaims) + return standardClaims.SignedString(key) +} + +func testADFSProvider(hostname string) *ADFSProvider { o := oidc.NewVerifier( "https://issuer.example.com", fakeADFSJwks{}, @@ -41,6 +54,7 @@ func testADFSProvider(hostname string) *ADFSProvider { ValidateURL: &url.URL{}, Scope: "", Verifier: o, + EmailClaim: OIDCEmailClaim, }) if hostname != "" { @@ -54,7 +68,6 @@ func testADFSProvider(hostname string) *ADFSProvider { } func testADFSBackend() *httptest.Server { - authResponse := ` { "access_token": "my_access_token", @@ -129,7 +142,6 @@ var _ = Describe("ADFS Provider Tests", func() { Context("with valid token", func() { It("should not throw an error", func() { - p.EmailClaim = "email" rawIDToken, _ := newSignedTestIDToken(defaultIDToken) idToken, err := p.Verifier.Verify(context.Background(), rawIDToken) Expect(err).To(BeNil()) @@ -202,4 +214,68 @@ var _ = Describe("ADFS Provider Tests", func() { }), ) }) + + Context("UPN Fallback", func() { + var idToken string + var session *sessions.SessionState + + BeforeEach(func() { + var err error + idToken, err = newSignedTestADFSToken(adfsClaims{ + UPN: "upn@company.com", + idTokenClaims: minimalIDToken, + }) + Expect(err).ToNot(HaveOccurred()) + + session = &sessions.SessionState{ + IDToken: idToken, + } + }) + + Describe("EnrichSession", func() { + It("uses email claim if present", func() { + p.oidcEnrichFunc = func(_ context.Context, s *sessions.SessionState) error { + s.Email = "person@company.com" + return nil + } + + err := p.EnrichSession(context.Background(), session) + Expect(err).ToNot(HaveOccurred()) + Expect(session.Email).To(Equal("person@company.com")) + }) + + It("falls back to UPN claim if Email is missing", func() { + p.oidcEnrichFunc = func(_ context.Context, s *sessions.SessionState) error { + return nil + } + + err := p.EnrichSession(context.Background(), session) + Expect(err).ToNot(HaveOccurred()) + Expect(session.Email).To(Equal("upn@company.com")) + }) + }) + + Describe("RefreshSession", func() { + It("uses email claim if present", func() { + p.oidcRefreshFunc = func(_ context.Context, s *sessions.SessionState) (bool, error) { + s.Email = "person@company.com" + return true, nil + } + + _, err := p.RefreshSession(context.Background(), session) + Expect(err).ToNot(HaveOccurred()) + Expect(session.Email).To(Equal("person@company.com")) + }) + + It("falls back to UPN claim if Email is missing", func() { + p.oidcRefreshFunc = func(_ context.Context, s *sessions.SessionState) (bool, error) { + return true, nil + } + + _, err := p.RefreshSession(context.Background(), session) + Expect(err).ToNot(HaveOccurred()) + Expect(session.Email).To(Equal("upn@company.com")) + }) + }) + }) }) From 1621ea3bbab0ea09ff71b7940b74ec324302e71e Mon Sep 17 00:00:00 2001 From: Nick Meves Date: Tue, 22 Jun 2021 18:57:21 -0700 Subject: [PATCH 3/5] ADFS supports IDToken nonce, use it --- providers/adfs.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/providers/adfs.go b/providers/adfs.go index 95177d76..ceecdb24 100644 --- a/providers/adfs.go +++ b/providers/adfs.go @@ -48,7 +48,7 @@ func NewADFSProvider(p *ProviderData) *ADFSProvider { oidcProvider := &OIDCProvider{ ProviderData: p, - SkipNonce: true, + SkipNonce: false, } return &ADFSProvider{ From bdfca925a37e45357be3311312ce7f7df487e71b Mon Sep 17 00:00:00 2001 From: Nick Meves Date: Sat, 3 Jul 2021 13:40:34 -0700 Subject: [PATCH 4/5] Handle UPN fallback when profileURL isn't set --- providers/adfs.go | 7 ++----- providers/adfs_test.go | 11 +++++++++++ 2 files changed, 13 insertions(+), 5 deletions(-) diff --git a/providers/adfs.go b/providers/adfs.go index ceecdb24..797c8566 100644 --- a/providers/adfs.go +++ b/providers/adfs.go @@ -84,11 +84,8 @@ func (p *ADFSProvider) GetLoginURL(redirectURI, state, nonce string) string { // from the claims. If Email is missing, falls back to ADFS `upn` claim. func (p *ADFSProvider) EnrichSession(ctx context.Context, s *sessions.SessionState) error { err := p.oidcEnrichFunc(ctx, s) - if err != nil { - return err - } - - if s.Email == "" { + if err != nil || s.Email == "" { + // OIDC only errors if email is missing return p.fallbackUPN(ctx, s) } return nil diff --git a/providers/adfs_test.go b/providers/adfs_test.go index adbec455..40a11b57 100644 --- a/providers/adfs_test.go +++ b/providers/adfs_test.go @@ -5,6 +5,7 @@ import ( "crypto/rand" "crypto/rsa" "encoding/base64" + "errors" "net/http" "net/http/httptest" "net/url" @@ -253,6 +254,16 @@ var _ = Describe("ADFS Provider Tests", func() { Expect(err).ToNot(HaveOccurred()) Expect(session.Email).To(Equal("upn@company.com")) }) + + It("falls back to UPN claim on errors", func() { + p.oidcEnrichFunc = func(_ context.Context, s *sessions.SessionState) error { + return errors.New("neither the id_token nor the profileURL set an email") + } + + err := p.EnrichSession(context.Background(), session) + Expect(err).ToNot(HaveOccurred()) + Expect(session.Email).To(Equal("upn@company.com")) + }) }) Describe("RefreshSession", func() { From 0fa8fca27607082be3c3057bd15b97f5aaec60cc Mon Sep 17 00:00:00 2001 From: Nick Meves Date: Wed, 1 Dec 2021 19:16:42 -0800 Subject: [PATCH 5/5] Update ADFS to new jwt lib --- providers/adfs_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/providers/adfs_test.go b/providers/adfs_test.go index 40a11b57..a12a4e53 100644 --- a/providers/adfs_test.go +++ b/providers/adfs_test.go @@ -12,7 +12,7 @@ import ( "strings" "github.com/coreos/go-oidc/v3/oidc" - "github.com/dgrijalva/jwt-go" + "github.com/golang-jwt/jwt" "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/apis/sessions" . "github.com/onsi/ginkgo" . "github.com/onsi/ginkgo/extensions/table"