jwtk / jjwt

Java JWT: JSON Web Token for Java and Android
Apache License 2.0
10.2k stars 1.32k forks source link

No way to read body, and then later verify a token #86

Closed chadjaros closed 6 years ago

chadjaros commented 8 years ago

I want to have a signed token with a subject claim. I can create the token perfectly well.

When I'm verifying the token, I would like to first determine the subject. After determining the subject and retrieving some data, I would like to verify the signature.

I can do the parsing of the body manually, but it would be nice to have this as a function of the parser class

jong64 commented 7 years ago

Since base64 encoded text is practically no different from plain text for all intents and purposes (except that the result is URI safe), we can hypothetically assume a token that looks like this for the sole purpose of illustration -

Hello-jjwt-simply-rocks-<HS512 signed signature value of the string "Hello-jjwt-simply-rocks">

So, arguing that you (as either human being or computer software) shouldn't be able to parse the string "Hello-jjwt-simply-rocks" and make sense out of it UNTIL after you validate the signature first is like saying that you cannot smell the food in front of you until you're allowed to do so.

The JWT specification NEVER prohibits anyone from parsing and reading the claims. The only thing it does require is that you MUST NOT trust the claims you read UNTIL/UNLESS the signature validates successfully.

So, whether you should be able to read the claims or not, really has nothing to do with the security.

Moriadry-zz commented 7 years ago

@lhazlewood your explanation really helps me a lot, thank you! :)

dogeared commented 7 years ago

folks:

Please read previous replies with code examples.

Using the signing key resolver pattern, you have (safe) access to the header and claims. You can log anything you'd need to within while still having a succinct block of parser code that will fail if the signature can't be verified.

JWTs do not have to be signed. But, if they are, the signature must be verified.

============================================== Micah Silverman, CSM http://bit.ly/WikiScrum Phone: 631.606.8928 Co-Author: Mastering Enterprise Javabeans 3.0 http://bit.ly/MasteringEJB3

"Debugging is twice as hard as writing the code in the first place. Therefore, if you write the code as cleverly as possible, you are, by definition, not smart enough to debug it." by Brian Kernighan

[image: Namez] http://namez.com/profiles/1720-micah-silverman/?autoPlay=true

On Wed, Apr 26, 2017 at 9:41 AM, Brill Pappin notifications@github.com wrote:

I'm actually becoming more, and more convinced that not being able to parse before the key is verified is a disaster.

Not only is that specific case not the spec (as per @jong64 https://github.com/jong64 comment above), but it encourages people to include the key where it should never be included, and it encourages people to hack around the library, possibly compromising it in unexpected ways.

Essentially it removed the value proposition when you artificially enforce constraints that should not be enforced.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/jwtk/jjwt/issues/86#issuecomment-297411996, or mute the thread https://github.com/notifications/unsubscribe-auth/AAWQEpnGSmZ6k2RmYb8nYf6UE7brrMNpks5rz0l2gaJpZM4HNxsd .

smileatom commented 7 years ago

This issue should be closed by the owner. There is no point in having this conversation and there is nothing to be learned here. If folks still have questions they should fork it. This repo does not have any intention of satisfying the use case being discussed here.

jansohn commented 6 years ago

For auditing purposes I also would like to access the claims without validating the signature (as validation has already failed in this case). Stripping the signature as explained by @lhazlewood (https://github.com/jwtk/jjwt/issues/86#issuecomment-180089267) only works if the JWT hasn't expired. Otherwise we'll have to add the next workaround, e.g. by setting the setAllowedClockSkewSeconds(long seconds) method to a ridiculously high value to actually have access to the claims of the invalid JWS/JWT:

int i = jws.lastIndexOf('.')
String withoutSignature = jws.substring(0, i+1);
Jwt<Header,Claims> untrusted = Jwts.parser()
    .setAllowedClockSkewSeconds(TimeUnit.DAYS.toSeconds(365L))
    .parseClaimsJwt(withoutSignature);

Is this still no valid use case to include such functionality?

lhazlewood commented 6 years ago

@jansohn and everyone else that wants to access parsed information and you don't care about vaidation:

You can do this today, and you have always been able to do this in JJWT since the introduction of the SigningKeyResolver concept.

All you have to do is implement a SigningKeyResolver (1), as @dogeared demonstrated above and set it on the parser (2):

// (1)
SigningKeyResolver signingKeyResolver = new SigningKeyResolverAdapter() {
    @Override
    public Key resolveSigningKey(JwsHeader header, Claims claims) {
        // inspect anything you want in the header or claims object.  This is
        // called after parsing, but *before* validation assertions.
    }
};

Claims claims = Jwts.parser()
    .setSigningKeyResolver(signingKeyResolver) // (2)
    .parseClaimsJws(token)
    .getBody();

This allows you to do exactly what you want, albeit maybe not as easily as another approach. But the functionality is there. If you don't care about validation errors, just wrap the parser call in a try/catch block and ignore the InvalidTokenException and put some JavaDoc in your code why you're ignoring it. At least this will self-document why you don't care about the validation errors that surface from parsing an invalid/expired token, and it will be much better understood by other engineers (or even yourself a year or two later) why you're ok trusting an invalid token for your very specific use case.

All of this said, the JJWT authors are not perfect - we're human after all, and we could just be wrong that there is still be a reason for making it easier to ignore token validation on parsing. But the JJWT team (at the moment at least) likes that the current design A) allows you definitely to access parsed information, even from invalid tokens, as described above and B) helps make it more self-documented in your own codebase why the invalid token is allowed to be used.

Finally, #148 might just solve this well-enough for everyone. Validation is still represented in code (and you can ignore the exception if you want), but at least you'd have access to the JWT data if you wanted it, without having to write a SigningKeyResolver.

Because of this, I'm closing this ticket in favor of #148 . If #148 still isn't good enough - with now 2 different ways of ignoring validation and still being able to get the data - it might be warranted to re-open this issue if enough people demand it.

RyanBard commented 6 years ago

Not trying to stir the pot here, but I'm a little confused why a parseUnsafe/Untrusted method for auditing/logging purposes is being argued against so hard when JwtParser.parse exists in the code.

It is much much worse.

It seems to describe what I might want to do (there isn't a verify method, but parse seems to do verification), it does the validation I ask it (exp checks, verify aud/iss claims, verify signature, verifies invalid JWT), and it isn't deprecated, however, if you aren't pulling claims out of the JWT (you just want to validate to decide whether to allow the call through) it is very easily tricked into accepting any token.

If you aren't diligent enough with your test cases, you'll have a very false sense of security.

private void validateAuth(String token, PublicKey key) {
    Jwts.parser()
            .setAllowedClockSkewSeconds(30)
            .requireAudience("https://my-audience.com")
            .requireIssuer("https://my-issuer.com")
            .setSigningKey(key)
            .parse(token);
}

Good so far, this is definitely the method I need!

The correct code for this validateAuth method is:

private void validateAuth(String token, String userId, PublicKey key) {
    Jwts.parser()
            .setAllowedClockSkewSeconds(30)
            .requireAudience("https://my-audience.com")
            .requireIssuer("https://my-issuer.com")
            .requireSubject(userId)
            .setSigningKey(key)
            .parseClaimsJws(token);
}

Very subtle difference, very devastating mistake, very specific test to flush it out.

Luckily, you usually want something out of the claims and the types will clue you in on your mistake, but that parse method (and parseClaimsJwt too imo) really seems like a landmine in the code.

Does the javadoc explain that parseClaimsJwt is unsafe? yes Does the javadoc explain that parse works with a JWT or a JWS? yes

But like you say, JWT is the default lingo and you're trying to make the defaults less surprising for the less experienced, so I think parse runs counter to that goal.

I really think the root problem with all of this is the lumping together of parsing and validation. That is why things aren't quite fitting (what if this situation, what if that situation, etc.). If we had a time machine, I would recommend going back and having 2 distinct functionalities for this: parse & verify

If you wanted to enforce some default security, work some magic in the builders:

For the normal case for most people:

// jwt is from the authorization header
// alg & key are configured out-of-band
Claims c = Jwt.verifier()
        .token(jwt)
        .signedWith(alg, key)
        .aud("my-expected-audience")
        .iss("my-expected-issuer")
        .verifyForParsing()
        .parse(); // no verification at all, that's not parse's job

For the logging/auditing case:

// jwt is from the authorization header
Claims c = Jwt.unsafeParser()
        .token(jwt)
        .parse(); // no verification at all, that's not parse's job

For the case that SigningKeyResolver is actually appropriate for:

// jwt is from the authorization header
Claims untrusted = Jwt.unsafeParser()
        .token(jwt)
        .parse();

String issuer = untrusted.get("iss");

AlgAndKey aak = db.findAlgAndKeyForIssuer(issuer);

return Jwt.verifier()
        .claims(untrusted)
        .signedWith(aak.getAlg(), aak.getKey())
        .aud("my-expected-audience")
        .verifyOnly();

(so for that, the key resolver concept fits, so let's implement that in this vaporware):

// jwt is from the authorization header
return Jwt.verifier()
        .token(jwt)
        .keyResolver((untrustedHeaders, untrustedClaims) -> db.findAlgAndKeyForIssuer(untrustedClaims.get("iss")))
        .aud("my-expected-audience")
        .verifyOnly();

Custom verification depending on something in the JWT (you have to be extra careful with things like this, but it's not necessarily insecure as long as all paths are doing reasonable enough validation):

// jwt is from the authorization header
// alg & key are configured out-of-band
Claims untrusted = Jwt.unsafeParser()
        .token(jwt)
        .parse(); // no verification at all, that's not parse's job

String customClaim = untrusted.get("foo");

JwtVerifier verifier = Jwt.verifier()
        .token(jwt)
        .signedWith(alg, key);

if ("bar".equals(customClaim)) {
    verifier = verifier.aud("my-bar-audience");
} else if ("baz".equals(customClaim)) {
    verifier = verifier.aud("my-baz-audience");
} else {
    // or maybe just don't add the extra aud check (contrived example is contrived)
    throw new IllegalArgumentException("Unknown foo: " + customClaim);
}

return verifier.verifyOnly();
RyanBard commented 6 years ago

Using the signing key resolver pattern, you have (safe) access to the header and claims.

@dogeared, not sure if you just misspoke or weren't aware. The JWT header is not safe. It's not signed and comes from the caller, so it can never really be trusted. At best, it can give you an excuse to reject a JWT before doing extra work validating with a trusted algorithm you've already decided on out-of-band.

At worst, it can trick you into doing something you shouldn't. The alg NONE and asymmetric key (RS/EC) vs. symmetric (HMAC) flaws are evidence of that.

bpappin commented 6 years ago

I get the feeling there is a little NIH going on with this. We're making it work, but we are having to hack around simple cases.

I think @RyanBard has made a simple but significant suggestion on how to improve things.

lhazlewood commented 6 years ago

@RyanBard and @bpappin Per Ryan's argument against accidental mistakes (e.g. parse vs parseClaimsJws), it seems the very argument asking for this behavior is the same argument that should prevent it). (Thanks for the parse vs validation examples Ryan - I agree that better illustrates separation of concerns. That said, if we ever did something like this - and I'm not sure we should - it would definitely have to wait for a major version increase, e.g. 1.0).

Here are some commonly quoted scenarios that I don't agree with because they most definitely open up people to security holes.

Finally, wouldn't #148 solve this problem for everyone and be self-documenting? It's impossible to misunderstand looking at code that ignores an exception and continues because the developer is indicating in code "I know this is normally wrong, but I'm doing it anyway for my particular use case". And that's a good thing IMO because it's showing a calculated divergence from best practices.

(FWIW, JJWT always parses both the claims and headers before performing validation. This separation of steps just isn't exposed to API users currently.)

I think this thread (mostly) has been in favor of "I just wanna do this" instead of "it should be not-too-easy-but-still-doable for me to do this because the security side-effects can be massive" and #148 makes that even easier. IMO JJWT should be as easy to use as possible and eliminate where possible shooting yourself in the foot.

That said, I find that most people participating in this thread are pragmatic and responsible folks, and I appreciate the thoughtful and civil discourse. Thanks for chiming in - I'm looking forward to your response.

mluis commented 4 years ago

In my case, because validation is a separated concern of Claim parsing.