Repository: oauth2-proxy/oauth2-proxy
Pull request #1247 (2021-06-19 23:10:50)

Use `upn` as EmailClaim throughout ADFSProvider
<!--- Provide a general summary of your changes in the Title above -->

## Description

<!--- Describe your changes in detail -->
By only overriding `EmailClaim` to `upn` in the `EnrichSession` call that is only post login, any Refresh calls would've overriden it with the `email` claim, changing the underlying session.Email mid session.

## Motivation and Context

Noticed the potential bug while working a similar logic bug here: https://github.com/oauth2-proxy/oauth2-proxy/pull/1239

## How Has This Been Tested?

Unit test

HELP NEEDED: Community ADFS user interactive testing.

## Checklist:

- [x] My change requires a change to the documentation or CHANGELOG.
- [x] I have updated the documentation/CHANGELOG accordingly.
- [x] I have created a feature (non-master) branch for my PR.


NickMeves: @samirachoadi Do you mind testing this PR interactively in your ADFS environment? I think I noticed a subtle bug.  Particularly make sure after an underlying `RefreshSessionIfNeeded` call, your `session.Email` is still from the `upn` claim.
samirachoadi: With this change is still possible to define a different emailClaim with --oidc-email-claim ?
NickMeves: It is not possible -- the provider initialization is after the ProviderData setup with the flags; so this overrides whatever might be set.

But that was also the case previously, setting `p.EmailClaim = "upn"` in `EnrichSession` also had the same affect. This just does it consistently on login & refresh.

What are some scenarios where's you'd want to change this in ADFS? I would assume we'd make the ADFS provider opinionated about configuration; if users wanted flexibility, they'd opt for the generic OIDC provider?
samirachoadi: What I wanted to do is to set it to upn as fallback only if the email was empty. If it's as you said, then I think it's better to avoid setting it up at all and let the user just configure it with --oidc-email-claim
NickMeves: > What I wanted to do is to set it to upn as fallback only if the email was empty. If it's as you said, then I think it's better to avoid setting it up at all and let the user just configure it with --oidc-email-claim

Ah gotcha! Thanks for the context -- I see that now with the leading empty email check in `EnrichSession`.

I'll tweak this -- it still probably doesn't play nice with refreshes.
NickMeves: @samirachoadi Mind testing this out? This commit has the fix that preserves the UPN fallback behavior: https://github.com/oauth2-proxy/oauth2-proxy/pull/1247/commits/1a91ec1af8078277ddf7707b0c802a373397c6c6

The problem with the old implementation is `p.EmailClaim` was a provider wide variable, so when you override it for when user when you detect they need to fallback, now for the rest of the server uptime, all future logins will use `upn` no matter what (I think...).

Sorry I didn't catch this in the PR!
NickMeves: @JoelSpeed - This is ready to go.

Haven't heard back a final word from any ADFS users 🤷‍♂️ 

But I think the current implementation bug (overriding a provider wide setting in response to per session details) is much bigger & scarier than anything I likely missed in unit tests.
samiracho: Hi, I tested your commit and the fallback doesnt seem to work. If I remove the flag emailClaim: upn I get the following error:
`[2021/07/03 06:36:31] [oauthproxy.go:727] Error creating session during OAuth2 callback: id_token did not contain an email and profileURL is not defined`

I think it's easier just to remove all that logic and just let the user set the emailClaim in the ouath2proxy configuration.
NickMeves: > Hi, I tested your commit and the fallback doesnt seem to work. If I remove the flag emailClaim: upn I get the following error:
> `[2021/07/03 06:36:31] [oauthproxy.go:727] Error creating session during OAuth2 callback: id_token did not contain an email and profileURL is not defined`
> 
> I think it's easier to remove all that logic, and just let the user set the emailClaim in the oauth2proxy configuration.

Thanks for the test. If email missing is common across ADFS, we should handle it. (Is it common?)

Since it is a specific provider, we should be opinionated about defaults. OIDC only has so many knobs to turn because it is used by a bazillion different OIDC IdPs
NickMeves: Ok -- attempt 3 😅 
samirachoadi: What's the status here? Do you want me to test your last commit?
github-actions[bot]: This pull request has been inactive for 60 days. If the pull request is still relevant please comment to re-activate the pull request. If no action is taken within 7 days, the pull request will be marked closed.
samiracho: From my point of view the cleanest solution is to "remove this conditional upn fallback and require users to setup the oidc-email-claim"
JoelSpeed: > Remove this conditional upn fallback and require users to setup the oidc-email-claim

@NickMeves Do you have time to implement this change instead?
github-actions[bot]: This pull request has been inactive for 60 days. If the pull request is still relevant please comment to re-activate the pull request. If no action is taken within 7 days, the pull request will be marked closed.
JoelSpeed: This is still waiting on Nick, who I know is quite busy at the moment. If anyone has cycles to pick this up and rebase it, take it to the finish line, I'm sure Nick wouldn't mind

Patch:
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..797c8566 100644
--- a/providers/adfs.go
+++ b/providers/adfs.go
@@ -2,7 +2,6 @@ package providers
 
 import (
 	"context"
-	"errors"
 	"fmt"
 	"net/url"
 	"strings"
@@ -13,23 +12,27 @@ import (
 // ADFSProvider represents an ADFS based Identity Provider
 type ADFSProvider struct {
 	*OIDCProvider
-	SkipScope bool
+
+	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)
 
 const (
-	ADFSProviderName = "ADFS"
-	ADFSDefaultScope = "openid email profile"
-	ADFSSkipScope    = false
+	adfsProviderName = "ADFS"
+	adfsDefaultScope = "openid email profile"
+	adfsSkipScope    = false
+	adfsUPNClaim     = "upn"
 )
 
 // NewADFSProvider initiates a new ADFSProvider
 func NewADFSProvider(p *ProviderData) *ADFSProvider {
-
 	p.setProviderDefaults(providerDefaults{
-		name:  ADFSProviderName,
-		scope: ADFSDefaultScope,
+		name:  adfsProviderName,
+		scope: adfsDefaultScope,
 	})
 
 	if p.ProtectedResource != nil && p.ProtectedResource.String() != "" {
@@ -43,18 +46,22 @@ func NewADFSProvider(p *ProviderData) *ADFSProvider {
 		}
 	}
 
+	oidcProvider := &OIDCProvider{
+		ProviderData: p,
+		SkipNonce:    false,
+	}
+
 	return &ADFSProvider{
-		OIDCProvider: &OIDCProvider{
-			ProviderData: p,
-			SkipNonce:    true,
-		},
-		SkipScope: ADFSSkipScope,
+		OIDCProvider:    oidcProvider,
+		skipScope:       adfsSkipScope,
+		oidcEnrichFunc:  oidcProvider.EnrichSession,
+		oidcRefreshFunc: oidcProvider.RefreshSession,
 	}
 }
 
 // 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,7 +72,7 @@ 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()
@@ -73,28 +80,40 @@ func (p *ADFSProvider) GetLoginURL(redirectURI, state, nonce string) string {
 	return loginURL.String()
 }
 
-// EnrichSession to add email
+// 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 {
-	if s.Email != "" {
-		return nil
+	err := p.oidcEnrichFunc(ctx, s)
+	if err != nil || s.Email == "" {
+		// OIDC only errors if email is missing
+		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
 	}
-
-	p.EmailClaim = "upn"
-	c, err := p.getClaims(idToken)
-
+	claims, 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")
+	upn := claims.raw[adfsUPNClaim]
+	if upn != nil {
+		s.Email = fmt.Sprint(upn)
 	}
-
-	return err
+	return nil
 }


Test patch:
diff --git a/providers/adfs_test.go b/providers/adfs_test.go
index bd2a82ce..a12a4e53 100644
--- a/providers/adfs_test.go
+++ b/providers/adfs_test.go
@@ -2,13 +2,17 @@ package providers
 
 import (
 	"context"
+	"crypto/rand"
+	"crypto/rsa"
 	"encoding/base64"
+	"errors"
 	"net/http"
 	"net/http/httptest"
 	"net/url"
 	"strings"
 
 	"github.com/coreos/go-oidc/v3/oidc"
+	"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"
@@ -25,8 +29,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 +55,7 @@ func testADFSProvider(hostname string) *ADFSProvider {
 		ValidateURL:  &url.URL{},
 		Scope:        "",
 		Verifier:     o,
+		EmailClaim:   OIDCEmailClaim,
 	})
 
 	if hostname != "" {
@@ -54,7 +69,6 @@ func testADFSProvider(hostname string) *ADFSProvider {
 }
 
 func testADFSBackend() *httptest.Server {
-
 	authResponse := `
 		{
 			"access_token": "my_access_token",
@@ -129,13 +143,12 @@ 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())
 			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 +162,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="))
@@ -202,4 +215,78 @@ 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"))
+			})
+
+			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() {
+			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"))
+			})
+		})
+	})
 })