-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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.
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
Unable to extract claims when cty specified in JWS header (>0.12.0) #897
Comments
Hi @icecreamhead! Thank you for reaching out! Let me explain what's happening, and see if we can find a workaround for you. As you noted, this change is purely driven by the RFC, and the RFC is quite clear that libraries (e.g. JJWT) are not to interpret payloads that have an assigned content type, and it must be the application (i.e. your code). Per RFC 7515, Section 4.1.10, "cty" (Content Type) Header Parameter:
and, most important for JJWT:
So the spec is implicitly saying: "If the Consequently, if JJWT >= BUT! It doesn't mean that JJWT ignores security. If found to be a JWS, JJWT does indeed perform signature verification, and if successful, constructs a Consequently, the reason you're seeing that exception is that because, per the
So, in JJWT's current form, you have two choices:
|
@icecreamhead my immediately preceding comment was to explain why things are the way they are, and how to potentially address your needs today, without JJWT code changes. This comment addresses a discussion of how we might support When working towards the 0.12.0 release, and its associated And the application could register various type handlers with the The reason this wasn't done for 0.12.0 was due to time constraints and complexity. The JWE and JWK stuff was so large and comprehensive, and delayed for quite a while, we just had to get it out. And considering that JJWT is spec-compliant with regard to the RFC, it made sense. The other reason was content-type/media-type identifier parsing, and registration of various handler types. If you've ever seen the Spring Framework's (very thorough and robust) Perhaps a middle ground could be made, at least initially, where JJWT has such a Anyway, I hope these comments have been helpful in understanding why we made those various decisions. |
Just noting in the general case, this doesn't appear to be a viable solution: If the In other words, a payload can definitely be JSON, and any kind of JSON at all. Claims are a further restriction on JSON structure/parameters, so there needs to be additional information in a media type identifier to indicate "not only is this JSON, but it is JWT Claims JSON" in order to (correctly) parse it into Claims. There is no IANA-registered media type for Claims JSON. If there was it'd be something like |
Hi @lhazlewood, thank you for a speedy, comprehensive and well-reasoned response. It's genuinely appreciated! Unfortunately, asking the upstream token issuers to fix their JWSs is out of the question for us. My use-case is for the UK Open Banking & payee confirmation ecosystem, of which there are hundreds of independent participants. We've observed that the majority actually are setting the cty field on their tokens. We actually have implemented your first suggestion of capturing the byte[] and parsing the claims ourselves, but this feels like an unnecessary overhead when the library can (and has previously) done the legwork for us. I think the aspect of the behaviour that doesn't make sense to me is that by invoking My request would be to always try to decode the claims when |
The other minor smell is that |
Happy to help! Let's see if we can keep working on a better solution...
That's frustrating to hear that they do that; it is in direct conflict with the RFC. It explicitly says, if the payload is JSON Claims, that
Out of curiosity, what is the media type you see being used for most of these? Is it always
Agreed, this is a pain, I'd like to have a more elegant solution now that I understand what you're experiencing.
Ahah, I think we're making progress 😄. Yes, at the moment, those methods are purely conveniences. parse(compact).accept(Jws.CLAIMS); So it is parsed, per normal RFC parsing rules (and Even so, let's assume we did convey such a hint to the What if the I suppose at least after verifying the signature, the risks for blind parsing are lessened, but still we have to be really careful about security implications.
I'd be hesitant to add that due to the security implications. But I'm wondering if a simple Jwts.parser().cty(handler)... And the handler would be called when detecting a At least that way it would be quite intentional by the app developer, at least assuming they understood the security repercussions of purposefully ignoring any indicated Anyway, I'm just thinking 'out loud', and still very interested in finding a clean solution that makes things simple for people while still enabling strong security by default. |
We've observed a mix of
I think this would be ideal for us. I'd be more than happy to throw a PR together if it helps? |
That's helpful, thanks.
That would be appreciated, but as I dug into this potential change more, it's not exactly simple:
Jws<?> parseSignedContent(CharSequence jws); which is in conflict with the existing ( In other words, a I don't know how difficult a change that would/will be, but it's not exactly trivial. |
@icecreamhead in thinking of the You could just implement the public class AlwaysClaimsVisitor extends SupportedJwtVisitor<Claims> {
@Override
public Claims onVerifiedContent(Jws<byte[]> jws) {
// if cty header is set, application needs to convert as necessary:
byte[] payload = jws.getPayload();
ObjectMapper objectMapper = new ObjectMapper(); // or get other application singleton
Map<String,?> map = (Map<String,?>)objectMapper.readValue(payload, Map.class);
return Jwts.claims().add(map).build();
}
@Override
public Claims onVerifiedClaims(Jws<Claims> jws) {
return jws.getPayload();
}
} and then use that visitor: AlwaysClaimsVisitor visitor = new AlwaysClaimsVisitor();
Jwt<?, ?> jwt = Jwts.parser().build()/*... */.parse(jws);
Claims claims = jwt.accept(visitor); This seems like a pretty clean workaround, no? The visitor is your 'handler' implementation, and the existing API already supports this use case. This seems pretty clean, but I could be missing something. Please let me know your thoughts! |
I managed to get the handler pattern working (and correctly inferring the output type) but the code isn't pleasant. I'll stick it on a branch. I think you're right that the visitor pattern is the right solution (it's what we've switched to right now in lieu of
|
@icecreamhead What does a header and payload look like for a openbanking JWS (use fake data)? {
"alg": "RS512",
"kid": "90210ABAD",
"b64": false,
"http://openbanking.org.uk/iat": 1501497671,
"http://openbanking.org.uk/iss": "C=UK, ST=England, L=London, O=Acme Ltd.",
"http://openbanking.org.uk/tan": "openbanking.org.uk",
"crit": [ "b64", "http://openbanking.org.uk/iat", "http://openbanking.org.uk/iss", "http://openbanking.org.uk/tan"]
} This seems to imply there are other validation steps that should happen:
Personally, I'm a fan of this logic being moved into the header vs the payload, but that is a little off spec (per the JWT/JWS rfcs) We could make sure this functionality is exposed in JJWT though for these types of use cases (as @lhazlewood mentioned above |
I think dynamic client registration is the only place we actually receive fully-fledged JWSs. Here's an example of a registration request:
The jose header is generally sent as a detached http header with the body stripped out. The payload is sent as the raw http body (and is vanilla json).
|
Do you have an example with the |
I'm assuming you're referring to the OpenBanking spec? If so, that spec directly conflicts with the recommendations in JWT RFC 7519 (Section 5.2) that the
In fact, it is the very absence of a Otherwise, the
The reason for this is that there is no media type for Claims JSON (e.g. In other words, if a JWT payload was So how does a JWT library then 'know' when to treat the payload as Claims vs any other JSON data structure? The only mechanism identified in the JWT RFC 7519 is the absence of the Someone should definitely bring this up to the OpenBanking spec committee because the very presence of their JJWT's primary responsibility is to implement the RFC behavior specified by the various specifications managed by the JOSE Working Group. Additional specifications beyond that (e.g. OpenID Connect, OpenBanking, etc) is not currently a design goal (but could be in the future), as anything built on top of JOSE RFCs are expected to be symbiotic, not conflict with them. BUT! That doesn't mean we can't/won't implement enhancements that can allow this additional behavior, or try to help to find workarounds, etc. We'll do our best, I'm just stating that the JOSE WG RFCs are the primary goal and anything in conflict or contradictory to them are secondary goals. |
That's true at the moment, but it's a trivial exercise to implement the ...
Jwt<?, ?> jwt = Jwts.parser().build()/*... */.parse(jws);
Claims claims = jwt.accept(visitor);
Jws<?> jws = (Jws<?>)jwt;
// ClaimsJws in your project implements Jws<Claims>
return new ClaimsJws(jws.header(), claims, jws.getSignature(), jws.getDigest()); I think that's a reasonable (albeit not ideal) workaround until we can finalize a |
FWIW, I contacted the secretary of the OpenBanking Working Group to indicate there is a conflict between the two specifications. We'll see if/how there might be a process to modify their specification to be congruent. |
|
I should clarify that the open banking DCR spec does not reference |
Describe the bug
In 0.11.5 and below, the
Claims
object can be extracted from JWS regardless of whether thecty
field is set on header or not.From 0.12.0 onwards, if the
cty
header is set, an exception is thrown when attempting to extract theClaims
object, even when the content type is json.The behaviour appears to have changed in this PR. The change is clearly driven by the RFC but I don't believe the jjwt library should automatically throw an exception in this scenario. We have no control over whether the client has included the cty field or not, but if they specify json, then I think we should continue to parse the claims as if the field weren't specified at all.
To Reproduce
Any attempt to invoke
DefaultJwtParser#parseSignedClaims()
when the supplied JWS has thecty
field set results inio.jsonwebtoken.UnsupportedJwtException: Unexpected content JWS.
. This occurs even if the specified content type is json.Expected behavior
The claims are parsed successfully and returned from the method.
Screenshots
Old, expected behaviour (0.11.5)
New, unexpected behaviour (0.12.3)
The text was updated successfully, but these errors were encountered: