handler/oauth2: allow stateless introspection of jwt access tokens #141

ccbrown · 2017-01-21T12:45:16Z

Conversation started in #140. A quick draft was requested by @arekkas, which I provided there, but I figured starting a review might be a better way to talk about the code than inline code blocks and patch files. I won't be offended if this isn't consistent with the roadmap. The functionality is either going into my application code or fosite, so it's time well spent either way.

The goal here is to be able to perform validation of JWT access tokens without hitting the storage backend.

Without too much effort, we can define an introspector to do it. No public symbols or behavior are changed. Just added a few things that make this nicer.

ccbrown · 2017-01-21T12:50:14Z

handler/oauth2/strategy_jwt.go

 	}

-	// validate the token
-	if err = t.Claims.Valid(); err != nil {
+	if err != nil {
 		if e, ok := err.(*jwtx.ValidationError); ok {


This entire block is dead code btw. I didn't change it because I was going for minimal impact and didn't want to alter the behavior, but the RS256JWTStrategy.Decode method already does all of the validation, and it always returns a new string error. 😞

Ah right, this should be errors.Cause(err) instead

ccbrown · 2017-01-21T12:52:37Z

handler/oauth2/introspector_jwt_test.go

+	}
+}
+
+func BenchmarkIntrospectJWT(b *testing.B) {


There aren't any other benchmarks, so maybe performance is a non-goal of the library, but this was of interest to me. Understood if you want to delete in the interest of staying slim. It's certainly not useful if no one ever runs it.

aeneasr · 2017-01-21T15:12:26Z

Thanks for the PR!

I strongly dislike that scopes are currently not supported - it's a core OAuth2 concept and not using it is an anti-pattern, so it shouldn't be encouraged by the code base!

Apart from that, I understand the case now better. One issue I have is that token recovation does not have an effect when the JWT strategy is used. This could be very misleading and mount up to a security issue when the library is configured incorrectly by a developer.

Don't be mistaken though, I would like to support statelessness, I think it's pretty cool.

Also, I think it would be essential to have some sort of spec test for the validation strategies - so something that tests the different implementations against the same test data. Maybe you could refactor existing integration tests to make that work.

Signed-off-by: Christopher Brown <ccbrown112@gmail.com>

ccbrown

In order to support validating scopes, we have to write them to the JWTs. There's not a registered IANA key for that, but there is one drafted here, which I've seen a few other libraries using: https://tools.ietf.org/html/draft-ietf-oauth-token-exchange-07

ccbrown · 2017-01-22T03:06:42Z

authorize_helper_test.go

@@ -75,22 +75,22 @@ func TestDoesClientWhiteListRedirect(t *testing.T) {
 			isError: true,


Some random go fmt changes in this file.

ccbrown · 2017-01-22T03:09:23Z

compose/compose_oauth2.go

+//
+// Due to the stateless nature of this factory, THE BUILT-IN REVOCATION MECHANISMS WILL NOT WORK.
+// If you need revocation, you can validate JWTs statefully, using the other factories.
+func OAuth2StatelessJWTIntrospectionFactory(config *Config, storage interface{}, strategy interface{}) interface{} {


Definitely agreed that there's an increased risk of misconfiguration. Hoping renaming this to emphasize the statelessness and adding a cautionary doc block will minimize that risk.

ccbrown · 2017-01-22T03:22:36Z

handler/oauth2/strategy_jwt.go

+
+		if claims.Issuer == "" {
+			claims.Issuer = h.Issuer
+		}


There seemed to be a lot of cases where these things would be left empty, making it difficult to use JWTs interchangeably in tests.

Noticed the OpenID strategy had this Issuer field, which I added here just because it's handy.

ccbrown · 2017-01-22T03:28:01Z

token/jwt/claims_jwt.go

-	ret["nbf"] = float64(c.NotBefore.Unix()) // jwt-go does not support int64 as datatype
+
+	if !c.IssuedAt.IsZero() {
+		ret["iat"] = float64(c.IssuedAt.Unix()) // jwt-go does not support int64 as datatype


Filtering out values of -62135596800 from JWTs for the optional fields.

Signed-off-by: Christopher Brown <ccbrown112@gmail.com>

ccbrown · 2017-01-22T03:36:36Z

I had to rewrite the history for those last two commits since I just realized my email address in the "signed-off-by" line was wrong. Hope that doesn't make looking at this too weird.

aeneasr · 2017-01-22T12:41:27Z

Yup, the scp claim is what I'd be doing as well!

aeneasr · 2017-01-22T12:46:20Z

How about a recovation strategy that comes with this introspection strategy and that always returns an error on revocation? Then combine them in a composer wrapper. Could that be a solution?

ccbrown · 2017-01-22T12:54:21Z

I did consider making it a runtime error by doing that or something similar, but I think there may be valid use-cases for custom revocation handlers with this introspector. Not really sure if that's a common enough need to consider it though. (Personally, I don't have that need.)

aeneasr · 2017-01-22T13:07:35Z

Ok, if we don't find a solution I'd probably accept the refactoring required for your implementation to work, but would not include the implementation itself. You could provide a package fosite-stateless-validation and we'd link it in the docs. Then, in the readme, you could point out that token revocation won't work with this code base. What do you think about this? I think it's a fair trade off between your specific use case, a benefit for the open source eco system, and a way to keep the concepts within fosite unisono and repeatable

ccbrown · 2017-01-22T13:12:08Z

Sounds fair to me. 👍

I'll see if something good comes to mind, and either update the PR with something or remove the StatelessJWTValidator struct.

ccbrown · 2017-01-23T10:41:45Z

Went with the return-an-error-on-revocation idea. I think the valid use-cases for revocation with this type of JWT validation would require writing entirely new custom handlers anyways.

aeneasr · 2017-01-25T08:46:25Z

handler/oauth2/strategy_jwt.go

 			switch e.Errors {
 			case jwtx.ValidationErrorMalformed:
-				return errors.Wrap(fosite.ErrInvalidTokenFormat, err.Error())
+				err = errors.Wrap(fosite.ErrInvalidTokenFormat, err.Error())


I'm not sure right now, does golang fallthrough or break per default?

It definitely feels weird coming from other languages that just fall through, but Go breaks if you don't add a fallthrough statement: https://golang.org/ref/spec#Switch_statements

aeneasr · 2017-01-25T08:48:21Z

handler/oauth2/strategy_jwt.go

-		claims.ExpiresAt = jwtSession.GetExpiresAt(tokenType)
+
+		if claims.ExpiresAt.IsZero() {
+			claims.ExpiresAt = jwtSession.GetExpiresAt(tokenType)


The idea here was to explicitly override the expires_at value. What's the reasoning to move it into IsZero?

Hmm. I don't remember if this was intentional or if it was meant to be temporary while I was testing... Doesn't have any impact on tests, so I'd better change it back.

aeneasr · 2017-01-25T08:50:15Z

integration/helper_endpoints_test.go

 	return func(rw http.ResponseWriter, req *http.Request) {
 		req.ParseForm()
 		ctx := fosite.NewContext()

-		accessRequest, err := oauth2.NewAccessRequest(ctx, req, &fosite.DefaultSession{})
+		accessRequest, err := provider.NewAccessRequest(ctx, req, &oauth2.JWTSession{})


Why is this no longer the default Session?

It has to be JWTSession for TestIntrospectToken to be able to test any of the JWT strategies.

aeneasr · 2017-01-25T08:53:00Z

Thanks, that looks pretty solid, I love that we have scopes in JWTs now!

I've added a few comments. We would also need some clarification in the docs that the JWT Stateless Strategy isn't supporting Token Revocation, and also a compose factory that sets up the stateless strategy appropriately

ccbrown · 2017-01-25T09:04:41Z

I think I have those things... The compose factory is compose.OAuth2StatelessJWTIntrospectionFactory, and that and StatelessJWTValidator both document the lack of revocation. Did you have something else in mind?

aeneasr · 2017-01-25T09:06:35Z

Sorry, I probably simply overlooked it! Will review it again later today or latest tomorrow

aeneasr · 2017-01-25T09:41:55Z

Also, can you verify that your implementation works as expected with the example: https://github.com/ory-am/fosite-example ? :)

ccbrown · 2017-01-25T09:53:06Z

Yep. Works fine as far as I can tell.

aeneasr · 2017-02-11T10:03:11Z

Sorry, I completely forgot about this issue. I will get to it next week!

aeneasr · 2017-02-15T14:08:16Z

Thank you for your contribution!

ccbrown commented Jan 21, 2017

View reviewed changes

handler/oauth2: allow stateless introspection of jwt access tokens

6ada567

Signed-off-by: Christopher Brown <ccbrown112@gmail.com>

ccbrown force-pushed the stateless-jwt-introspection branch from 1e512fe to 6ada567 Compare January 22, 2017 03:04

ccbrown commented Jan 22, 2017

View reviewed changes

dont client id for aud

8e4543b

Signed-off-by: Christopher Brown <ccbrown112@gmail.com>

ccbrown force-pushed the stateless-jwt-introspection branch from 794638a to 8e4543b Compare January 22, 2017 03:34

make stateless validator return an error on revocation

832eb3d

aeneasr reviewed Jan 25, 2017

View reviewed changes

revert unintentional change

eb75811

aeneasr self-assigned this Feb 11, 2017

aeneasr merged commit 14a18a7 into ory:master Feb 15, 2017

mitar mentioned this pull request May 30, 2020

scope in access token is not space delimited #362

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

handler/oauth2: allow stateless introspection of jwt access tokens #141

handler/oauth2: allow stateless introspection of jwt access tokens #141

ccbrown commented Jan 21, 2017 •

edited

Loading

ccbrown Jan 21, 2017 •

edited

Loading

aeneasr Jan 21, 2017

ccbrown Jan 21, 2017

aeneasr Jan 25, 2017

aeneasr commented Jan 21, 2017 •

edited

Loading

ccbrown left a comment

ccbrown Jan 22, 2017

ccbrown Jan 22, 2017

ccbrown Jan 22, 2017

ccbrown Jan 22, 2017

ccbrown commented Jan 22, 2017

aeneasr commented Jan 22, 2017

aeneasr commented Jan 22, 2017

ccbrown commented Jan 22, 2017

aeneasr commented Jan 22, 2017

ccbrown commented Jan 22, 2017

ccbrown commented Jan 23, 2017

aeneasr Jan 25, 2017

ccbrown Jan 25, 2017

aeneasr Jan 25, 2017

ccbrown Jan 25, 2017

aeneasr Jan 25, 2017

ccbrown Jan 25, 2017

aeneasr commented Jan 25, 2017

ccbrown commented Jan 25, 2017 •

edited

Loading

aeneasr commented Jan 25, 2017

aeneasr commented Jan 25, 2017

ccbrown commented Jan 25, 2017

aeneasr commented Feb 11, 2017

aeneasr commented Feb 15, 2017

		@@ -75,22 +75,22 @@ func TestDoesClientWhiteListRedirect(t *testing.T) {
		isError: true,

handler/oauth2: allow stateless introspection of jwt access tokens #141

handler/oauth2: allow stateless introspection of jwt access tokens #141

Conversation

ccbrown commented Jan 21, 2017 • edited Loading

ccbrown Jan 21, 2017 • edited Loading

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

aeneasr commented Jan 21, 2017 • edited Loading

ccbrown 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

ccbrown commented Jan 22, 2017

aeneasr commented Jan 22, 2017

aeneasr commented Jan 22, 2017

ccbrown commented Jan 22, 2017

aeneasr commented Jan 22, 2017

ccbrown commented Jan 22, 2017

ccbrown commented Jan 23, 2017

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

aeneasr commented Jan 25, 2017

ccbrown commented Jan 25, 2017 • edited Loading

aeneasr commented Jan 25, 2017

aeneasr commented Jan 25, 2017

ccbrown commented Jan 25, 2017

aeneasr commented Feb 11, 2017

aeneasr commented Feb 15, 2017

ccbrown commented Jan 21, 2017 •

edited

Loading

ccbrown Jan 21, 2017 •

edited

Loading

aeneasr commented Jan 21, 2017 •

edited

Loading

ccbrown commented Jan 25, 2017 •

edited

Loading