Support nonce checks in OIDC Provider #967

NickMeves · 2020-12-25T00:44:38Z

Implements OIDC Nonce setting:

A nonce is always created and sent as a parameter to all provider's GetLoginURL (even if they don't use it).
Once a session is created, the nonce is added to session.Nonce for any providers to use during the lifespan of the session to validate the nonce.

In the initial OAuth flow, after LoginURL, Redeem, EnrichSession - ValidateSession is now called as well (for all providers). For OIDC, this allows a final check on the ID Token nonce claim before minting the session as valid.

The --insecure-oidc-skip-nonce flag is true by default for now in case any existing OIDC IdPs don't support it properly.

CSRF management is refactored to its own struct in cookies package. It handles both the OAuth2 state nonce & the OIDC nonce. The cookie value is now additionally signed and verified before any CSRF checks with the OAuth2 callback or OIDC ID Token nonce claim.

Any nonce checks use hmac.Equal for constant time comparisons to not leak time details.

(Slight Scope Creep) Unit tests added to the cookies package for existing code in addition to new CSRF code.

Checklist:

My change requires a change to the documentation or CHANGELOG.
I have updated the documentation/CHANGELOG accordingly.
I have created a feature (non-master) branch for my PR.

NickMeves · 2020-12-25T04:51:47Z

I'm pretty sure I did this wrong and the nonce parameter needs to be on the initial authentication request and not the second token request.

I don't think I have a session available to sync the nonce at that earlier stage, I'll likely need to do something similar to the state tracking for OAuth in the CSRF cookie 🤔

NickMeves · 2020-12-26T21:25:20Z

Ok all fixed up. A unique OIDC nonce (distinct from the OAuth state nonce for added security benefits) is set in the LoginURL and persisted between calls in the CSRF token adjacent to the OAuth state nonce.

CSRF tracking logic could potentially use some refactoring now that it is more complex -- get it out of OAuthProxy and into its own package.

NickMeves · 2020-12-29T23:00:38Z

This is in a pre-reviewable state now. I just need to write tests for the new CSRF code now in the cookie package.

NickMeves · 2020-12-31T00:14:23Z

@JoelSpeed The code is ready for a review.

I want to find time to hook this into a few test OIDC instances to manually peek at the ID Token and see the nonce claim. I especially want to verify how it behaves with a fresh ID Token after a token refresh.

JoelSpeed

This is great! Nothing major to report but found a few bits and have a few questions as I'm going through

Very pleased to be reusing the cookie options directly into the main package, there really was no reason for us to copy all those values over, thanks for that!

JoelSpeed · 2021-01-01T10:05:51Z

oauthproxy.go

-	CookieRefresh  time.Duration
-	CookieSameSite string
-	Validator      func(string) bool
+	CookieOptions *options.Cookie


Oooh! I like this :D

The first step in a long journey of figuring out what the heck is going on in the OAuthProxy struct -- and why some fields are public and others private 😅

And why other fields even exist at all.

oauthproxy.go

pkg/cookies/csrf.go

JoelSpeed · 2021-01-01T10:20:06Z

pkg/cookies/csrf.go

+func LoadCSRFCookie(req *http.Request, opts *options.Cookie) (*CSRF, error) {
+	cookie, err := req.Cookie(csrfCookieName(opts))
+	if err != nil {
+		// Don't wrap this error to allow `err == http.ErrNoCookie` checks


We should change err == http.ErrNoCookie to use errors.Is, then we can wrap to our hearts content!

I think this comment came over when refactoring from other cookie areas 🤔

I think the only place that cares about http.ErrNoCookie in our codebase is unrelated to this and is related to session store management.

Should we just remove this comment here you think?

pkg/cookies/csrf.go

pkg/cookies/csrf_test.go

pkg/encryption/nonce.go

NickMeves · 2021-01-01T19:40:01Z

@JoelSpeed What are your thoughts on the ValidateSession usage in general in the project? This PR changes it to explicitly be called at the end of the initial authentication flow before we do a final Authorize on it (as the place to check the OIDC ID Token nonce claim).

I uncovered some existing issues with its usage and documented them here: #856 (comment)

My current thinking: we repurposes the usage of those functions in the project. For sure after the initial authentication request. Even for providers that have a network round trip in ValidateSession to a ValidateURL, that slowness doesn't matter at this stage.

The other point in time is immediately after a RefreshSession call where we now potentially have fresh Access & IDTokens in our session. While a call to ValidateSession might be nice to ensure security -- the network round trip in some providers might be an issuer here. The RefreshSession is already in the middle of a normal proxied request, so the RefreshSession RTT already adds a huge increase to the request proxying time.

Maybe if we decide validating is good there, we add a refresh bool argument to ValidateSession so providers can alter its behavior in the post RefreshSession flow? The OIDC Provider for instance would benefit from ValidateSession post RefreshSession because its implementation has no network calls and only checks token signatures and claim contents.

Either way -- I think the legacy usage of ValidateSession is broken if it is only called when RefreshSessionIfNeeded doesn't trigger. I think some providers that don't implement RefreshSessionIfNeeded but implement ValidateSession are unintentionally getting validate RTTs every request after the --cookie-refresh time if set has passed.

NickMeves · 2021-01-06T00:41:11Z

pkg/cookies/csrf.go

+
+// CSRF manages various nonces stored in the CSRF cookie during the initial
+// authentication flows.
+type CSRF interface {


Should this be under pkg/apis with some of the other interfaces?

Depends whether it's causing circular imports, I think it's fine here for now

NickMeves · 2021-02-20T01:20:52Z

@JoelSpeed This is ready for a final pass. Just a couple new commits (1 alignment to 1.16, 1 bugfix).

I did manual testing with both Dex & OneLogin OIDC and confirmed the nonce claim is present in the ID Token in both the initial redeem & again after a refresh with the new refreshed ID Token.

oauthproxy.go

CHANGELOG.md

oauthproxy.go

pkg/cookies/cookies.go

JoelSpeed · 2021-02-20T10:26:28Z

pkg/cookies/cookies.go

-		}
+// MakeCookieFromOptions constructs a cookie based on the given *options.CookieOptions,
+// value and creation time
+func MakeCookieFromOptions(req *http.Request, name string, value string, opts *options.Cookie, expiration time.Duration, now time.Time) *http.Cookie {


Future thought for a separate PR, but I wonder if we should create some cookie builder abstraction kind of like the request builder so that you don't have to pass it the basic options every time and you can just add the bits you want to override

Oh yeah - that's a great idea! I like it -- I remember trying to fight with this method to see if there was a way I could get its method parameter count down to cleanup that codeclimate finding and gave up 😅

JoelSpeed · 2021-02-20T10:29:07Z

pkg/cookies/csrf.go

+	"github.com/vmihailenco/msgpack/v4"
+)
+
+var now = time.Now


Would it make sense to make this part of the csrf itself? With an internal method that allows it to be overridden? How have we done this elsewhere 🤔

I think in a lot of other cases, we frequently passed in a now time.Time method argument following the dependency injection pattern that maybe lives on from its Java roots? I think in all cases we've done it, its been for time mocking in tests (this is the same case here, I just use it for cookie new expiration unit tests we hadn't had before).

As an alternative, I've tried this to expose time in tests while keeping the method singatures clean in normal code. For now I've left it private since this only has time manipulated by my cookies package unit tests.

After working with jwt-go here: https://github.com/oauth2-proxy/mockoidc/blob/65483e3cdabeaeb796f9180b6e4d5dd253d05985/mockoidc.go#L189

I see the huge advantage in potentially exporting TimeFunc in our various packages more. Allows us to simplify method signatures (removing the dummy now time.Now variables we have bloating them up) while giving access to time.Now to manipulate across packages (in case a sub-dependency in a test also needs a custom view of time).

@JoelSpeed I think this was the last piece of outstanding feedback in this PR? Did you have any final thoughts on this design?

I'd still lean to creating a timeFunc member in the csrf struct, then rather than overriding a package level var, you override it just for the instance of csrf.

I did the following recently when I needed to mock time in my day job, WDYT?

type csrf struct { nowFunc func() time.Time } func (c csrf) now() time.Time { if c.nowFunc != nil { return c.nowFunc() } return time.Now() }

Yeah - I think this can work too.

I just want us to get away from method signatures that have now time.Time as a parameter for the sole purpose of time mocking in tests -- but having the baggage of extra arguments in normal code flows is annoying.

The downside to this & my original code (private package var now = time.Now) is we can't fully mock time in units tests that might have dependencies across packages. We definitely have that in our test suite, and in that scenario - I see the testing elegance in public TimeFunc variables (like in the jwt-go sample I mentioned).

I'll jump to this pattern for now, but let's chat more about a good architecture to standardize how we handle time.Now in the codebase.

JoelSpeed · 2021-02-20T10:29:25Z

pkg/cookies/csrf.go

+
+// CSRF manages various nonces stored in the CSRF cookie during the initial
+// authentication flows.
+type CSRF interface {


Depends whether it's causing circular imports, I think it's fine here for now

pkg/cookies/csrf.go

NickMeves · 2021-03-04T19:56:15Z

Semi-related to this PR - I got a intermittent CSRF failure myself for once and caught the log: http: named cookie not present

I have the CSRF cookie in my browser, no other tabs, and this interesting line in the logs:
47.144.131.12 - nick.meves@greenhouse.io [2021/03/04 19:50:52] [AuthFailure] Invalid authentication via OAuth2: unable to obtain CSRF cookie

Invalid authentication... but I'm very obviously authenticated since my username is in the logs 🤔

JoelSpeed · 2021-03-21T19:44:10Z

Not sure where we are with this right now, looks like it needs a rebase and a new review? Did we decide how we wanted to handle the time mocking in tests?

NickMeves · 2021-03-21T20:07:09Z

Not sure where we are with this right now, looks like it needs a rebase and a new review? Did we decide how we wanted to handle the time mocking in tests?

It was pretty much done, I think we decided to punt until after v7.1 since its content was more related to a Provider focus v7.2 will have.

I moved time.Now stubbing to be a private struct field as per your suggestion. But you are right, it might be good for us to put our heads together on a good strategy project wide for how we want to manage this in tests.

NickMeves · 2021-04-20T02:15:17Z

OOF - that rebase was brutal after the multiple provider support PR merged 😅

I have tests passing... but I'm gonna want to eyeball this whole thing again myself to make sure I didn't accidentally clobber something during the rebase.

JoelSpeed

I think it would be better to update the documentation to be more detailed, otherwise I'm happy for this to be merged. WDYT to my suggestion?

JoelSpeed · 2021-04-20T17:45:19Z

pkg/apis/options/providers.go

+	// InsecureSkipNonce skips verifying the ID Token's nonce claim
+	InsecureSkipNonce bool `json:"insecureSkipNonce,omitempty"`


Perhaps we want to expand on the documentation on this field. This is what gets generated into the reference docs after all.

I'd say let's include the default as others are and maybe even the little warning about it changing in a future release

JoelSpeed

LGTM, thanks @NickMeves

* Set and verify a nonce with OIDC * Create a CSRF object to manage nonces & cookies * Add missing generic cookie unit tests * Add config flag to control OIDC SkipNonce * Send hashed nonces in authentication requests * Encrypt the CSRF cookie * Add clarity to naming & add more helper methods * Make CSRF an interface and keep underlying nonces private * Add ReverseProxy scope to cookie tests * Align to new 1.16 SameSite cookie default * Perform SecretBytes conversion on CSRF cookie crypto * Make state encoding signatures consistent * Mock time in CSRF struct via Clock * Improve InsecureSkipNonce docstring

NickMeves added enhancement WIP Work in progress labels Dec 25, 2020

NickMeves force-pushed the oidc-nonce-validation branch from e99479d to e7e6e5d Compare December 26, 2020 21:21

NickMeves force-pushed the oidc-nonce-validation branch 3 times, most recently from 021c8d2 to 8d0ac72 Compare December 29, 2020 22:58

NickMeves force-pushed the oidc-nonce-validation branch from 8d0ac72 to cf6e269 Compare December 31, 2020 00:05

NickMeves removed the WIP Work in progress label Dec 31, 2020

NickMeves marked this pull request as ready for review December 31, 2020 04:40

NickMeves requested a review from a team as a code owner December 31, 2020 04:40

NickMeves force-pushed the oidc-nonce-validation branch 2 times, most recently from b912e65 to 2f8eb7e Compare January 1, 2021 01:24

JoelSpeed reviewed Jan 1, 2021

View reviewed changes

NickMeves commented Jan 6, 2021

View reviewed changes

NickMeves force-pushed the oidc-nonce-validation branch from ad7b823 to afca1a6 Compare February 9, 2021 03:04

NickMeves added this to In Progress (Pull Requests) in Release v7.1.0 Feb 10, 2021

NickMeves force-pushed the oidc-nonce-validation branch from afca1a6 to 6414933 Compare February 20, 2021 01:17

NickMeves commented Feb 20, 2021

View reviewed changes

oauthproxy.go Outdated Show resolved Hide resolved

NickMeves linked an issue Feb 20, 2021 that may be closed by this pull request

Support Nonce Parameter & Claim in OIDC Providers #966

Closed

JoelSpeed reviewed Feb 20, 2021

View reviewed changes

NickMeves force-pushed the oidc-nonce-validation branch from 0bcafcc to 53dc04c Compare February 20, 2021 18:09

This was referenced Feb 22, 2021

extract email from id_token for azure provider #914

Merged

Support private_key_jwt in OIDC Provider #1062

Closed

NickMeves mentioned this pull request Mar 28, 2021

Add a clock package for better time mocking #1136

Merged

3 tasks

NickMeves mentioned this pull request Apr 5, 2021

Improve the CSRF Failure experience #708

Closed

4 tasks

Nick Meves added 3 commits April 19, 2021 17:38

Set and verify a nonce with OIDC

24de18d

Create a CSRF object to manage nonces & cookies

7040b44

Add missing generic cookie unit tests

0a8f818

NickMeves force-pushed the oidc-nonce-validation branch from e7d86ce to f1f91fb Compare April 20, 2021 02:09

Nick Meves added 10 commits April 19, 2021 19:19

Add config flag to control OIDC SkipNonce

74752a9

Send hashed nonces in authentication requests

7ecd4eb

Encrypt the CSRF cookie

d32840a

Add clarity to naming & add more helper methods

047916c

Make CSRF an interface and keep underlying nonces private

fb27263

Add ReverseProxy scope to cookie tests

a669708

Align to new 1.16 SameSite cookie default

7aac5c9

Perform SecretBytes conversion on CSRF cookie crypto

af79aa3

Make state encoding signatures consistent

e6007ea

Mock time in CSRF struct via Clock

3148dcf

NickMeves force-pushed the oidc-nonce-validation branch from f1f91fb to 3148dcf Compare April 20, 2021 02:20

JoelSpeed reviewed Apr 20, 2021

View reviewed changes

Improve InsecureSkipNonce docstring

99cadeb

JoelSpeed approved these changes Apr 21, 2021

View reviewed changes

JoelSpeed merged commit 7eeaea0 into oauth2-proxy:master Apr 21, 2021

Release 7.2.0 automation moved this from In progress to Done Apr 21, 2021

NickMeves mentioned this pull request Feb 20, 2022

PKCE Support #1541

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support nonce checks in OIDC Provider #967

Support nonce checks in OIDC Provider #967

NickMeves commented Dec 25, 2020 •

edited

NickMeves commented Dec 25, 2020

NickMeves commented Dec 26, 2020

NickMeves commented Dec 29, 2020

NickMeves commented Dec 31, 2020

JoelSpeed left a comment

JoelSpeed Jan 1, 2021

NickMeves Jan 1, 2021

JoelSpeed Jan 1, 2021

NickMeves Jan 1, 2021

NickMeves commented Jan 1, 2021

NickMeves Jan 6, 2021

JoelSpeed Feb 20, 2021

NickMeves commented Feb 20, 2021

JoelSpeed Feb 20, 2021

NickMeves Feb 20, 2021

JoelSpeed Feb 20, 2021

NickMeves Feb 20, 2021

NickMeves Feb 27, 2021

JoelSpeed Mar 6, 2021

NickMeves Mar 6, 2021

NickMeves Mar 6, 2021

JoelSpeed Feb 20, 2021

NickMeves commented Mar 4, 2021

JoelSpeed commented Mar 21, 2021

NickMeves commented Mar 21, 2021

NickMeves commented Apr 20, 2021

JoelSpeed left a comment

JoelSpeed Apr 20, 2021

JoelSpeed left a comment

		// InsecureSkipNonce skips verifying the ID Token's nonce claim
		InsecureSkipNonce bool `json:"insecureSkipNonce,omitempty"`

Support nonce checks in OIDC Provider #967

Support nonce checks in OIDC Provider #967

Conversation

NickMeves commented Dec 25, 2020 • edited

Checklist:

NickMeves commented Dec 25, 2020

NickMeves commented Dec 26, 2020

NickMeves commented Dec 29, 2020

NickMeves commented Dec 31, 2020

JoelSpeed left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NickMeves commented Jan 1, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NickMeves commented Feb 20, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NickMeves commented Mar 4, 2021

JoelSpeed commented Mar 21, 2021

NickMeves commented Mar 21, 2021

NickMeves commented Apr 20, 2021

JoelSpeed left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JoelSpeed left a comment

Choose a reason for hiding this comment

NickMeves commented Dec 25, 2020 •

edited