enigma: Added JWT generator and validator.

[leetal]

Finally. As per discussion in #16 👍

[aeneasr]

cool!

[aeneasr]

That's all looking really good! There's unfortunately one issue. The major question is: How do we get state data into enigma.GenerateChallenge?

You solved this by making JWTEnigma stateful. This is a normal way of approaching such an issue. Unfortunately, JWTEnigma is held by Fosite and Fosite is actually not stateful. You initialize Fosite once (in the factory) and it runs. By making JWTEnigma stateful Fosite must be initialized on every request. It is not enough to initialize fosite once and set the Context on every request because concurrent requests would result in very buggy behavior.

I see multiple ways to solve this problem:

1. Enigma could require some sort of additional input data to the secret. I remember that osin solved this by passing the AuthorizeRequest or AccessRequest object which had an additional UserData interface{} field so it was very easy to get that data in there.
2. We introduce authorize/accessRequest.GetSigningMethod() which is basically a stateful enigma object where you can set context etc
3. We do something like enigma.Clone()
4. Do you have an idea?

It's not so easy to explain the problem, I hope I phrased it somehow understandable. I wrote a little piece of code, maybe that helps a little more...

var i string

func myHandler(w http.ResponseWriter, r *http.Request) {
   i = i + " foo"
   w.Write([]byte(i))
}

After five requests you end up with a response of foo foo foo foo foo. The same thing would happen to the context variable and it would get increasingly unstable because the pointer pointing to the context value would constantly jump around. The more requests, the more jumping :D

[aeneasr]

Regarding option 2. This could look like (pseudocode):

// in authorize handler
ar := &AuthorizeRequest{
   // ...
   Enigma: fosite.Enigma.Clone(),
}

// ...
// in request
signer := ar.GetEnigma()
if jwtSigner, ok := signer .(*enigma.JWT); ok {
   jwtSigner.SetSubject("asdf")
}

[aeneasr]

There's another option. We are passing ctx context.Context variables down to all handlers. We could pass the context handler to Enigma's functions as well making JWTEnigma stateless

[aeneasr]

so basically changing the enigma interface to:

ValidateChallenge(ctx context.Context, t *Challenge)

I'm not sure if this is smart because the context is not really "beautiful" but it get's the job done.

However I think the enigma interface needs change because, as you pointed out secret's can't really be used with jwt so it doesn't make sense to be in the interface

[aeneasr]

Before I went to sleep, I had some more ideas spinning around. I'll write them down here so they don't get lost.

First: Authorize Code, Access Token, Refresh Token and ID Token (coming soon) are three (four) different types of tokens. For example, a refresh token might have different claims then an access token and an authorize code might just be a random string.

I think the best idea would be to put in between one layer of abstraction between the handler and any cryptographical implementation:

// these are just example names :)
myHandler := MyTokenEndpointHandler{
    AccessTokenEnigma: &JWTEnigma{JWTCrypto}, // JWTCrypto := current JWTEnigma
    RefreshTokenEnigma:  &JWTEnigma{JWTCryptoAlternativeImplementation},
    AuthorizeCodeEnigma: &OpaqueEnigma{}, // uses irreplaceable HMACSHAEnigma
    IDTokenEnigma: &JWTEnigma{IDTokenCrypto},
}

JWTEnigma and OpaqueEnigma both implement multiple interfaces:

interface AccessTokenGenerator {
    GenerateAccessToken(context.Context, http.Request, AccessRequester, session interface{}) (token string, signature string, error)
    ValidateAccessToken(token string, context.Context, http.Request, AccessRequester, session interface{}) (signature string, error)
}

interface RefreshTokenGenerator {
    GenerateRefreshToken(context.Context, http.Request, AccessRequester, AccessResponder, session interface{}) (token string, signature string, error)
    ValidateRefreshToken(c *Challenge, context.Context, http.Request, AccessRequester, session interface{}) (signature string, error)
}

// ....

As you maybe noticed, these signatures are very similar to https://github.com/ory-am/fosite/blob/master/oauth2.go#L43

JWTCryptoAlternativeImplementation and JWTCrypto and IDTokenCrypto all, for example, implement some other interface:

type Claims //...

type Crypto interface {
    GenerateJWT(Claims) ([]byte, error)
    ValidateJWT(jwt []byte) error
}

type JWTEnigma {
    Crypto Crypto
}

func (j JWTEnigma) GenerateAccessToken(_ context.Context, _ http.Request, areq AccessRequester, sess session interface{}) (token string, signature string, error) {
    jwtClaims["aud"] := areq.GetClient().GetID() 
    // ...

    token, err := j.Crypto.GenerateJWT()    
    // if err != nil ...

    // signature := strings.split(...
    return token, signature, nil
}

This is a little overhead but would allow for a very dynamic set up without bloating the cryptographical implementations (and making them harder to read, review and secure).

This would cause a little refactoring. But I think it is the best idea I came up with yet regarding this issue. I'd love to hear your feedback on this idea. If you're 💯 I'd implement this behaviour quickly (unless you want to give it a try), and you could then adapt your PR to the changes.

[leetal]

Back after a good nights sleep! Yes, I'm aware of that issue and it was a thing I wanted to discuss since it would essentially mean that I would have to rewrite parts of the base implementation and that's really nothing i want to do unless you're on the same page.

Since you found the issue despite me falling asleep last night, i think the best way of actually making everything stateless is to move the relevant "user data" like the global secret and the jwt claims to the context. But this has a major downside: We might "bloat" the context with data that's never used. One way of solving this would be to make that variable accept everything like "interface{}" but that will add unnecessary complexity in the enigmas since we would have to validate that data prior to using it. Decisions, Decisions, Decisions....

Another problem that exists is that the developer most likely want to update the JWT claims when the token is generated (like adding user id, user email, .... to the claims) and also like updating the private reserved claims (like "exp", ...). This could be done with callback methods (anonymous functions, not clojures) that is called before and after the token is generated and in those anonymous functions we pass a reference to the context. This will only have to be done for the access tokens and not the authorize/refresh tokens obviously. But this means that we will need a way of passing what signing method that was used as well most likely for future need. By doing that, alongside passing the context down to the enigmas, i believe we could achieve a full stateless behavior as well as a way for the developer to actually control the token generation (at least in the case of JWT. HMAC doesn't have this need).

Do you have any ideas on this approach?

[aeneasr]

Since you found the issue despite me falling asleep last night, i think the best way of actually making everything stateless is to move the relevant "user data" like the global secret and the jwt claims to the context.

I think the global secret is something that is shared amongst application state. We need the global secret on various occasions and the idea is, if you change the global secret ALL tokens generated with the old one become invalid. I would like to keep the global secret therefore global :)

But this has a major downside: We might "bloat" the context with data that's never used. One way of solving this would be to make that variable accept everything like "interface{}" but that will add unnecessary complexity in the enigmas since we would have to validate that data prior to using it.

Yes I agree 100%. Giving up type safety in important security modules is not a good idea.

Did you have a chance to read through #18 (comment) ? I kinda like this idea

[aeneasr]

Another problem that exists is that the developer most likely want to update the JWT claims when the token is generated (like adding user id, user email, .... to the claims) and also like updating the private reserved claims (like "exp", ...). This could be done with callback methods (anonymous functions, not clojures) that is called before and after the token is generated and in those anonymous functions we pass a reference to the context. This will only have to be done for the access tokens and not the authorize/refresh tokens obviously. But this means that we will need a way of passing what signing method that was used as well most likely for future need. By doing that, alongside passing the context down to the enigmas, i believe we could achieve a full stateless behavior as well as a way for the developer to actually control the token generation (at least in the case of JWT. HMAC doesn't have this need).

I think this would be possible with #18 (comment)

// in the request handler
session := NewJWTSession()
session.Set("foo", "bar")

// ...
fosite.NewAccessResponse(/* ... */, session)

// in JWTenigma
type JWTEnigma {
    Crypto Crypto
}

func (j JWTEnigma) GenerateAccessToken(_ context.Context, _ http.Request, areq AccessRequester, sess session interface{}) (token string, signature string, error) {
    jwtClaims := new(JWTClaims)

    if jwtSession, ok := session.(*JWTSession); ok {
        merge(jwtClaims, jwtSession.Claims)
    }

    jwtClaims["aud"] := areq.GetClient().GetID() 
    // ...

    token, err := j.Crypto.GenerateJWT()    
    // if err != nil ...

    // signature := strings.split(...
    return token, signature, nil
}

[aeneasr]

There's another thing. enigma.Challenge is not a good way to transport the various tokens around because it makes implications how a token should look like. That's why I replaced (Challenge, error) with (token string, signature string, error). We need the token and we require a signature for it (for secure storage in the database)

[leetal]

I think it's best if you implement the changes to the base and that i rebase my PR to adopt those changes both with regards of time and the fact that you have 90% better know-how of how everything works than me ;)

[aeneasr]

Absolutely :) I'll try to implement this tonight!

[aeneasr]

Hey, while I was at refactoring enigma I stumbled upon some APIs that didn't make a lot of sense to me or are incomplete. I'm currently refactoring some method signatures and some context structs / interfaces as well.

I'll probably need a little bit more time to get this right because I have to do a lot of fixes in the tests afterwards (fixing all the mocks is going to be terrible...)

[aeneasr]

Hey I finished the refactor: #18

Take a look and tell me if that would solve the JWT case :)

[aeneasr]

Cool, I'm really glad that you agree on the architectural draft! :) Feel free to ping me any time if you run into troubles. :)

[leetal]

@arekkas I think i'm done with the refactor, but running the example in the enigma-refactor branch gives an error on the explicit grant. Is this per design or something else? The error seems to be due to the GetAuthorizeCodeSession failing to retrieve "Code" from the Store. The error is present at line 94 in handler/core/explicit/explicit_token.go. I'll wait with the push until this is resolved. :)

[aeneasr]

yup that was a bug. definitely need to write more integration tests as this had not been catched by them.

[aeneasr]

ok i hope everything is fixed now :)

[leetal]

@arekkas Take a look now :) Finally had some time over after a pretty harsh weekend!

[aeneasr]

good idea!

[aeneasr]

At first glance, this looks all very good to me. Thank you for your hard work and your commitment, you just improved fosite a lot! :)

I will take a little more time to go over the changes in detail and make sure that we didn't miss anything but I will merge this either tonight or tomorrow if nothing comes up.

Big up and thanks!

[aeneasr]

Just in case you didn't check: your name is in the Hall of Fame :)

[leetal]

Nice ;) But you didn't have too! I'm just glad i can help the community grow!

[aeneasr]

I looked over everything in detail, there is just one little tiny bit missing! Every JWT must have a JSON Token ID (jti) which you already marked as "TODO" here: https://github.com/ory-am/fosite/pull/18/files#diff-548a8577b0942b2c142c8a3b60bebeb1R27

This ID is required because otherwise the tokens will look very similar. This is especially true if you issue refresh and access tokens at the same time (they have the same properties and are therefore the same string!).

I suggest to use uuid.New() ( https://github.com/pborman/uuid - uuid.New() creates a random uuid v4) as a fallback if no JTI was set by the session.

Other than this everything is perfect :)

[leetal]

Yeah ofc! I missed that! I'll fix it asap in 2 hours. :)

[leetal]

@arekkas I don't know if you've seen it, but the fixes was pushed yesterday :) Please look through them so i haven't missed out on anything.

[aeneasr]

GitHub should notify people if PRs get updated. I miss that every time. :D

[aeneasr]

🎉