Repository: oauth2-proxy/oauth2-proxy
Pull request #596 (2020-05-30 21:23:56)

Verify main vs extra JWT bearers differently
When using the configured provider JWT Verifier, it makes
sense to use the provider `CreateSessionStateFromBearerToken`
method. For any extra JWT Issuers, they should use a generic
default verifier.

https://github.com/oauth2-proxy/oauth2-proxy/issues/592

## How Has This Been Tested?

Unit tests added for legacy/generic bearer IDToken -> SessionState.

TODO: deploy to a local kubernetes cluster and validate standard human OIDC with k8s service account tokens as an extra jwt issuer. UPDATE: DONE

## 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: On further reflection, I think this is definitely a bug? https://github.com/oauth2-proxy/oauth2-proxy/blob/a17c48810fc2b5267be256cb4812584c14ccb9d1/providers/provider_default.go#L165

I had a user make a web app that used this proxy for use with CLI tools that did OIDC via PKSE and passed the IDToken as the bearer. He got the `X-Forwarded-User` as the `email` claim, vs all the other normal webapps made with our OneLogin OIDC provider had the `subject` claim as the `X-Forwarded-User`.

I was very confused at the time, but this is definitely why.
NickMeves: I still owe you an interactive test on this, I haven't dockerized this branch yet to test how it all works deployed as a sidecar in a local kubernetes environment that is setup for OIDC projected service account tokens.
NickMeves: Should I make the `default_provider` aware of the `--user-id-claim` flag when mapping claims to a session.Email just the same as the OIDC provider?
NickMeves: @JoelSpeed Let me know what you find out as far as the history of the forced `session.Email` and not allowing it to be "".

This commit is my take on allowing it: https://github.com/oauth2-proxy/oauth2-proxy/pull/596/commits/f2bf5a39b19e19cf8881b34205153f18c8ad84df

I can ditch it if needed (or cherry pick it to a separate PR to keep this PR purely about the bearer bugs).
NickMeves: > I still owe you an interactive test on this, I haven't dockerized this branch yet to test how it all works deployed as a sidecar in a local kubernetes environment that is setup for OIDC projected service account tokens.

This is done, looks good in a kubernetes environment allowing both OneLogin IDTokens (configured provider) OR kubernetes projected service account tokens (extra bearer JWT):

```
[2020/06/03 17:15:09] [oauthproxy.go:283] Skipping JWT tokens from configured OIDC issuer: "https://greenhouse.onelogin.com/oidc/2"
[2020/06/03 17:15:09] [oauthproxy.go:285] Skipping JWT tokens from extra JWT issuer: "http://oidc.default.svc.cluster.local=sample-app"
...
192.168.144.128 - nick.meves@greenhouse.io [2020/06/03 17:21:27] sample-app.local GET 127.0.0.1:5000 "/sso" HTTP/1.1 "Ruby" 200 1336 0.003
192.168.144.128 - system:serviceaccount:default:dummy [2020/06/03 17:23:46] sample-app.local GET 127.0.0.1:5000 "/sso" HTTP/1.1 "Ruby" 200 1174 0.005
```
NickMeves: I had the `/sso` endpoint just spit back the `X-Forwarded-*` auth headers, you can see the behavior allowing empty session.Email in the service account token output (the experimental commit we can remove from this this PR if desired):

OneLogin OIDC Human IDToken:
```
{
  "data": {
     "username":  "[REDACTED]",
     "prefered_username": "nick.meves@greenhouse.io",
     "email":  "nick.meves@greenhouse.io"
  }
}
```

Kubernetes Projected Service Account IDToken:
```
{
  "data": {
    "username": "system:serviceaccount:default:dummy",
    "prefered_username": null,
    "email": null
  }
}
```
JoelSpeed: So I did some archaeology today, my conclusion is that, if the scope contained `email`, then the response and the userinfo endpoint should have the `email` value, but if `email` is not in the scope, then we don't need for it to fallback to anything I don't think

As far as I can tell, the OAuth2 Proxy doesn't require the email to be returned from the provider anywhere, so I think we can get away with not setting it. It's probably better that I rework this into #560 

This will likely be a breaking change, though, technically better than the existing behaviour.
I think we should probably look into the user-id-claim work and work out how we move forward with that.
NickMeves: > So I did some archaeology today, my conclusion is that, if the scope contained `email`, then the response and the userinfo endpoint should have the `email` value, but if `email` is not in the scope, then we don't need for it to fallback to anything I don't think
> 
> As far as I can tell, the OAuth2 Proxy doesn't require the email to be returned from the provider anywhere, so I think we can get away with not setting it. It's probably better that I rework this into #560
> 
> This will likely be a breaking change, though, technically better than the existing behaviour.
> I think we should probably look into the user-id-claim work and work out how we move forward with that.

I can remove that commit, isn't the end of the world to have a `session.Email` that isn't an email in cases where the subject claim isn't an email (with it as blank or a non-email from the subject -- both routes require `--emails-domains=*` to validate appropriately).

I'm not sure this is best handled in the #560 -- its that weird middle ground for how do we agnostically treat extra JWT bearers that aren't from the main provider in a generic way.

IE my case, I like my OIDC provider settings have the user-id claim to `email` like the default, but I don't want to change that to something just to appease compatibility with whatever extra JWT Bearer Verifiers I add since their IDToken structures could feasibly be all over the place (subject/audience is likely the only thing we can rely on it having).
NickMeves: > @NickMeves Can you just confirm this is good to go from your perspective, I believe it is and have no further comments
> 
> @steakunderscore I'd appreciate if you could also review this one before merging

Yup, ready to go.  I ran some manual tests in a local kubernetes environment and it looked good.

Only area I had a lingering question is the new unit test I added that feels a little sloppy.
NickMeves: Give me the heads up when we're ready and I'll rebase before the merge.  I think I keep dismissing your reviews when I push up rebases 😄 
JoelSpeed: I think let's just get this merged, can you rebase?

Patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index 5ea4fc00..d1568de2 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -55,6 +55,7 @@
 
 ## Changes since v5.1.1
 
+- [#596](https://github.com/oauth2-proxy/oauth2-proxy/pull/596) Validate Bearer IDTokens in headers with correct provider/extra JWT Verifier (@NickMeves)
 - [#620](https://github.com/oauth2-proxy/oauth2-proxy/pull/620) Add HealthCheck middleware (@JoelSpeed)
 - [#597](https://github.com/oauth2-proxy/oauth2-proxy/pull/597) Don't log invalid redirect if redirect is empty (@JoelSpeed)
 - [#604](https://github.com/oauth2-proxy/oauth2-proxy/pull/604) Add Keycloak local testing environment (@EvgeniGordeev)
diff --git a/oauthproxy.go b/oauthproxy.go
index 4c2b23c0..8c303df8 100644
--- a/oauthproxy.go
+++ b/oauthproxy.go
@@ -88,35 +88,36 @@ type OAuthProxy struct {
 	AuthOnlyPath      string
 	UserInfoPath      string
 
-	redirectURL          *url.URL // the url to receive requests at
-	whitelistDomains     []string
-	provider             providers.Provider
-	providerNameOverride string
-	sessionStore         sessionsapi.SessionStore
-	ProxyPrefix          string
-	SignInMessage        string
-	HtpasswdFile         *HtpasswdFile
-	DisplayHtpasswdForm  bool
-	serveMux             http.Handler
-	SetXAuthRequest      bool
-	PassBasicAuth        bool
-	SetBasicAuth         bool
-	SkipProviderButton   bool
-	PassUserHeaders      bool
-	BasicAuthPassword    string
-	PassAccessToken      bool
-	SetAuthorization     bool
-	PassAuthorization    bool
-	PreferEmailToUser    bool
-	skipAuthRegex        []string
-	skipAuthPreflight    bool
-	skipJwtBearerTokens  bool
-	jwtBearerVerifiers   []*oidc.IDTokenVerifier
-	compiledRegex        []*regexp.Regexp
-	templates            *template.Template
-	realClientIPParser   ipapi.RealClientIPParser
-	Banner               string
-	Footer               string
+	redirectURL             *url.URL // the url to receive requests at
+	whitelistDomains        []string
+	provider                providers.Provider
+	providerNameOverride    string
+	sessionStore            sessionsapi.SessionStore
+	ProxyPrefix             string
+	SignInMessage           string
+	HtpasswdFile            *HtpasswdFile
+	DisplayHtpasswdForm     bool
+	serveMux                http.Handler
+	SetXAuthRequest         bool
+	PassBasicAuth           bool
+	SetBasicAuth            bool
+	SkipProviderButton      bool
+	PassUserHeaders         bool
+	BasicAuthPassword       string
+	PassAccessToken         bool
+	SetAuthorization        bool
+	PassAuthorization       bool
+	PreferEmailToUser       bool
+	skipAuthRegex           []string
+	skipAuthPreflight       bool
+	skipJwtBearerTokens     bool
+	mainJwtBearerVerifier   *oidc.IDTokenVerifier
+	extraJwtBearerVerifiers []*oidc.IDTokenVerifier
+	compiledRegex           []*regexp.Regexp
+	templates               *template.Template
+	realClientIPParser      ipapi.RealClientIPParser
+	Banner                  string
+	Footer                  string
 }
 
 // UpstreamProxy represents an upstream server to proxy to
@@ -317,32 +318,33 @@ func NewOAuthProxy(opts *options.Options, validator func(string) bool) *OAuthPro
 		AuthOnlyPath:      fmt.Sprintf("%s/auth", opts.ProxyPrefix),
 		UserInfoPath:      fmt.Sprintf("%s/userinfo", opts.ProxyPrefix),
 
-		ProxyPrefix:          opts.ProxyPrefix,
-		provider:             opts.GetProvider(),
-		providerNameOverride: opts.ProviderName,
-		sessionStore:         opts.GetSessionStore(),
-		serveMux:             serveMux,
-		redirectURL:          redirectURL,
-		whitelistDomains:     opts.WhitelistDomains,
-		skipAuthRegex:        opts.SkipAuthRegex,
-		skipAuthPreflight:    opts.SkipAuthPreflight,
-		skipJwtBearerTokens:  opts.SkipJwtBearerTokens,
-		jwtBearerVerifiers:   opts.GetJWTBearerVerifiers(),
-		compiledRegex:        opts.GetCompiledRegex(),
-		realClientIPParser:   opts.GetRealClientIPParser(),
-		SetXAuthRequest:      opts.SetXAuthRequest,
-		PassBasicAuth:        opts.PassBasicAuth,
-		SetBasicAuth:         opts.SetBasicAuth,
-		PassUserHeaders:      opts.PassUserHeaders,
-		BasicAuthPassword:    opts.BasicAuthPassword,
-		PassAccessToken:      opts.PassAccessToken,
-		SetAuthorization:     opts.SetAuthorization,
-		PassAuthorization:    opts.PassAuthorization,
-		PreferEmailToUser:    opts.PreferEmailToUser,
-		SkipProviderButton:   opts.SkipProviderButton,
-		templates:            loadTemplates(opts.CustomTemplatesDir),
-		Banner:               opts.Banner,
-		Footer:               opts.Footer,
+		ProxyPrefix:             opts.ProxyPrefix,
+		provider:                opts.GetProvider(),
+		providerNameOverride:    opts.ProviderName,
+		sessionStore:            opts.GetSessionStore(),
+		serveMux:                serveMux,
+		redirectURL:             redirectURL,
+		whitelistDomains:        opts.WhitelistDomains,
+		skipAuthRegex:           opts.SkipAuthRegex,
+		skipAuthPreflight:       opts.SkipAuthPreflight,
+		skipJwtBearerTokens:     opts.SkipJwtBearerTokens,
+		mainJwtBearerVerifier:   opts.GetOIDCVerifier(),
+		extraJwtBearerVerifiers: opts.GetJWTBearerVerifiers(),
+		compiledRegex:           opts.GetCompiledRegex(),
+		realClientIPParser:      opts.GetRealClientIPParser(),
+		SetXAuthRequest:         opts.SetXAuthRequest,
+		PassBasicAuth:           opts.PassBasicAuth,
+		SetBasicAuth:            opts.SetBasicAuth,
+		PassUserHeaders:         opts.PassUserHeaders,
+		BasicAuthPassword:       opts.BasicAuthPassword,
+		PassAccessToken:         opts.PassAccessToken,
+		SetAuthorization:        opts.SetAuthorization,
+		PassAuthorization:       opts.PassAuthorization,
+		PreferEmailToUser:       opts.PreferEmailToUser,
+		SkipProviderButton:      opts.SkipProviderButton,
+		templates:               loadTemplates(opts.CustomTemplatesDir),
+		Banner:                  opts.Banner,
+		Footer:                  opts.Footer,
 	}
 }
 
@@ -1139,15 +1141,24 @@ func (p *OAuthProxy) GetJwtSession(req *http.Request) (*sessionsapi.SessionState
 		return nil, err
 	}
 
-	for _, verifier := range p.jwtBearerVerifiers {
-		bearerToken, err := verifier.Verify(req.Context(), rawBearerToken)
+	// If we are using an oidc provider, go ahead and try that provider first with its Verifier
+	// and Bearer Token -> Session converter
+	if p.mainJwtBearerVerifier != nil {
+		bearerToken, err := p.mainJwtBearerVerifier.Verify(req.Context(), rawBearerToken)
+		if err == nil {
+			return p.provider.CreateSessionStateFromBearerToken(req.Context(), rawBearerToken, bearerToken)
+		}
+	}
 
+	// Otherwise, attempt to verify against the extra JWT issuers and use a more generic
+	// Bearer Token -> Session converter
+	for _, verifier := range p.extraJwtBearerVerifiers {
+		bearerToken, err := verifier.Verify(req.Context(), rawBearerToken)
 		if err != nil {
-			logger.Printf("failed to verify bearer token: %v", err)
 			continue
 		}
 
-		return p.provider.CreateSessionStateFromBearerToken(req.Context(), rawBearerToken, bearerToken)
+		return (*providers.ProviderData)(nil).CreateSessionStateFromBearerToken(req.Context(), rawBearerToken, bearerToken)
 	}
 	return nil, fmt.Errorf("unable to verify jwt token %s", req.Header.Get("Authorization"))
 }
diff --git a/pkg/validation/options.go b/pkg/validation/options.go
index d3ae4ece..c8d633c0 100644
--- a/pkg/validation/options.go
+++ b/pkg/validation/options.go
@@ -178,10 +178,6 @@ func Validate(o *options.Options) error {
 	}
 
 	if o.SkipJwtBearerTokens {
-		// If we are using an oidc provider, go ahead and add that provider to the list
-		if o.GetOIDCVerifier() != nil {
-			o.SetJWTBearerVerifiers(append(o.GetJWTBearerVerifiers(), o.GetOIDCVerifier()))
-		}
 		// Configure extra issuers
 		if len(o.ExtraJwtIssuers) > 0 {
 			var jwtIssuers []jwtIssuer
diff --git a/providers/provider_default.go b/providers/provider_default.go
index 9f7ba3c5..14cec9fe 100644
--- a/providers/provider_default.go
+++ b/providers/provider_default.go
@@ -164,14 +164,13 @@ func (p *ProviderData) CreateSessionStateFromBearerToken(ctx context.Context, ra
 
 	newSession := &sessions.SessionState{
 		Email:             claims.Email,
-		User:              claims.Email,
+		User:              claims.Subject,
 		PreferredUsername: claims.PreferredUsername,
+		AccessToken:       rawIDToken,
+		IDToken:           rawIDToken,
+		RefreshToken:      "",
+		ExpiresOn:         &idToken.Expiry,
 	}
 
-	newSession.AccessToken = rawIDToken
-	newSession.IDToken = rawIDToken
-	newSession.RefreshToken = ""
-	newSession.ExpiresOn = &idToken.Expiry
-
 	return newSession, nil
 }


Test patch:
diff --git a/oauthproxy_test.go b/oauthproxy_test.go
index 32385323..0c244bae 100644
--- a/oauthproxy_test.go
+++ b/oauthproxy_test.go
@@ -1578,7 +1578,7 @@ func TestGetJwtSession(t *testing.T) {
 	// Bearer
 	expires := time.Unix(1912151821, 0)
 	session, _ := test.proxy.GetJwtSession(test.req)
-	assert.Equal(t, session.User, "john@example.com")
+	assert.Equal(t, session.User, "1234567890")
 	assert.Equal(t, session.Email, "john@example.com")
 	assert.Equal(t, session.ExpiresOn, &expires)
 	assert.Equal(t, session.IDToken, goodJwt)
@@ -1590,12 +1590,12 @@ func TestGetJwtSession(t *testing.T) {
 
 	// Check PassAuthorization, should overwrite Basic header
 	assert.Equal(t, test.req.Header.Get("Authorization"), authHeader)
-	assert.Equal(t, test.req.Header.Get("X-Forwarded-User"), "john@example.com")
+	assert.Equal(t, test.req.Header.Get("X-Forwarded-User"), "1234567890")
 	assert.Equal(t, test.req.Header.Get("X-Forwarded-Email"), "john@example.com")
 
 	// SetAuthorization and SetXAuthRequest
 	assert.Equal(t, test.rw.Header().Get("Authorization"), authHeader)
-	assert.Equal(t, test.rw.Header().Get("X-Auth-Request-User"), "john@example.com")
+	assert.Equal(t, test.rw.Header().Get("X-Auth-Request-User"), "1234567890")
 	assert.Equal(t, test.rw.Header().Get("X-Auth-Request-Email"), "john@example.com")
 }
 
diff --git a/providers/oidc_test.go b/providers/oidc_test.go
index c5d6b521..12b62a47 100644
--- a/providers/oidc_test.go
+++ b/providers/oidc_test.go
@@ -124,7 +124,6 @@ func newOIDCServer(body []byte) (*url.URL, *httptest.Server) {
 }
 
 func newSignedTestIDToken(tokenClaims idTokenClaims) (string, error) {
-
 	key, _ := rsa.GenerateKey(rand.Reader, 2048)
 	standardClaims := jwt.NewWithClaims(jwt.SigningMethodRS256, tokenClaims)
 	return standardClaims.SignedString(key)
diff --git a/providers/provider_default_test.go b/providers/provider_default_test.go
index 74d7096f..745bf2d4 100644
--- a/providers/provider_default_test.go
+++ b/providers/provider_default_test.go
@@ -2,10 +2,15 @@ package providers
 
 import (
 	"context"
+	"crypto/rand"
+	"crypto/rsa"
 	"net/url"
 	"testing"
 	"time"
 
+	"github.com/coreos/go-oidc"
+	"github.com/dgrijalva/jwt-go"
+
 	"github.com/oauth2-proxy/oauth2-proxy/pkg/apis/sessions"
 	"github.com/stretchr/testify/assert"
 )
@@ -47,3 +52,39 @@ func TestAcrValuesConfigured(t *testing.T) {
 	result := p.GetLoginURL("https://my.test.app/oauth", "")
 	assert.Contains(t, result, "acr_values=testValue")
 }
+
+func TestCreateSessionStateFromBearerToken(t *testing.T) {
+	minimalIDToken := jwt.StandardClaims{
+		Audience:  "asdf1234",
+		ExpiresAt: time.Now().Add(time.Duration(5) * time.Minute).Unix(),
+		Id:        "id-some-id",
+		IssuedAt:  time.Now().Unix(),
+		Issuer:    "https://issuer.example.com",
+		NotBefore: 0,
+		Subject:   "123456789",
+	}
+	// From oidc_test.go
+	verifier := oidc.NewVerifier(
+		"https://issuer.example.com",
+		fakeKeySetStub{},
+		&oidc.Config{ClientID: "asdf1234"},
+	)
+
+	key, err := rsa.GenerateKey(rand.Reader, 2048)
+	assert.NoError(t, err)
+	rawIDToken, err := jwt.NewWithClaims(jwt.SigningMethodRS256, minimalIDToken).SignedString(key)
+	assert.NoError(t, err)
+	// Pass to a dummy Verifier to get an oidc.IDToken from the rawIDToken for our actual test below
+	idToken, err := verifier.Verify(context.Background(), rawIDToken)
+	assert.NoError(t, err)
+
+	session, err := (*ProviderData)(nil).CreateSessionStateFromBearerToken(context.Background(), rawIDToken, idToken)
+	assert.NoError(t, err)
+
+	assert.Equal(t, rawIDToken, session.AccessToken)
+	assert.Equal(t, rawIDToken, session.IDToken)
+	assert.Equal(t, "123456789", session.Email)
+	assert.Equal(t, "123456789", session.User)
+	assert.Empty(t, session.RefreshToken)
+	assert.Empty(t, session.PreferredUsername)
+}