Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Verify main vs extra JWT bearers differently #596

Merged
merged 3 commits into from
Jun 25, 2020
Merged

Verify main vs extra JWT bearers differently #596

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
788d8ec
Verify main v extra JWT bearers differently
May 30, 2020
c2c1caa
Set User = Subject in ExtraJWTBearer sessions
Jun 1, 2020
a3eef17
Improve default CreateSessionStateFromBearerToken tests
Jun 4, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
129 changes: 70 additions & 59 deletions oauthproxy.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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,
}
}

Expand Down Expand Up @@ -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)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I removed this and defaulted to the logging happening only if all verifiers failed. My users have been confused with error messages when 1st or 2nd verifiers on the array fail and the 3rd extraJwt Verifier succeeded (seeing the failed words even though it succeeded was confusing)

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"))
}
Expand Down
6 changes: 3 additions & 3 deletions oauthproxy_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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")
}

Expand Down
4 changes: 0 additions & 4 deletions pkg/validation/options.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
1 change: 0 additions & 1 deletion providers/oidc_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
11 changes: 5 additions & 6 deletions providers/provider_default.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -164,14 +164,13 @@ func (p *ProviderData) CreateSessionStateFromBearerToken(ctx context.Context, ra

newSession := &sessions.SessionState{
Email: claims.Email,
User: claims.Email,
User: claims.Subject,
JoelSpeed marked this conversation as resolved.
Show resolved Hide resolved
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
}
41 changes: 41 additions & 0 deletions providers/provider_default_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
)
Expand Down Expand Up @@ -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)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's got to be a cleaner way to get an oidc.IDToken from a rawIDToken string...? Getting it through a dummy oidc.NewVerifier worked, but just feels sloppy to me -- but nothing stood out to me when I tried to dig through oidc 🤷

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is just a struct at the end of the day, you could theoretically construct it, but then you can't set the private methods. I think what you've done is probably the best we can do sadly

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)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the overall architectural rationale for forcing a session.Email to a claim.Subject when an email claim is missing? And not using the claim.Subject at all in the session and setting both the session.Email and session.User to the Email?

oauth2-proxy/providers/provider_default.go

Line 155 in f7b28cb

if claims.Email == "" {

I left the existing logic untouched -- but shouldn't session.User == claim.Subject (That's what my OIDC provider results in going the normal route).

oauth2-proxy/providers/provider_default.go

Line 165 in f7b28cb

User: claims.Email,

And should we allow session.Email to be "" if the IDToken is missing the claim? And adjust the email domains validator to accept it if its set to * (if set to anything else it would fail since it is missing a @ character)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

User ID claim was added so that we could make sure that a particular claim was added, to be considered the user, but the work hasn't been completed so it's only half way through

As for falling back, I'm not sure why we are doing that, i think we currently require all sessions to have a user and email field that's not empty, so I guess that's why? I think I need to do some archaeology on this

assert.Equal(t, "123456789", session.User)
assert.Empty(t, session.RefreshToken)
assert.Empty(t, session.PreferredUsername)
}