Skip to content
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

Support empty claims values #858

Closed
RaphiMC opened this issue Oct 8, 2023 · 6 comments · Fixed by #861
Closed

Support empty claims values #858

RaphiMC opened this issue Oct 8, 2023 · 6 comments · Fixed by #861
Assignees
Milestone

Comments

@RaphiMC
Copy link

RaphiMC commented Oct 8, 2023

Describe the bug
In versions before 0.12.x it was possible to add empty values in the claims of the JWT (for example .claim("test", "") or .claim("test", new ArrayList<>())). In versions after 0.12.0 empty claims are not serialized (compacted) anymore which breaks my use case where the receiving end expects the JWT to have all keys (even ones with empty values) in the claim map.

To Reproduce
Try to create a JWT with some claims that have empty values (See above example)
My exact code: https://github.com/RaphiMC/ViaBedrock/blob/5674c4b0a04f64b5560064ba326d8ac3e6e5403a/src/main/java/net/raphimc/viabedrock/protocol/packets/LoginPackets.java#L341

Expected behavior
I am expecting to be able to add empty values into the claims map (See above example)

@lhazlewood
Copy link
Contributor

@RaphiMC compact JWTs are supposed to be compact/minimal, and having empty claims is counter to providing small payloads.

Is there a reason you can't treat a missing claim as empty?

@RaphiMC
Copy link
Author

RaphiMC commented Oct 9, 2023

I need all claims since I don't control the server (Minecraft: Bedrock Edition) thats handling the JWT (I am writing a client which is supposed to be 1:1 the same on the networking level as Minecraft: Bedrock Edition). If the server doesn't receive all claims it will kick me as their parser doesn't implement this case. I used .compact previously as well, is there another method instead of .compact which gets me the JWT with all claims even empty ones?

@lhazlewood
Copy link
Contributor

@RaphiMC this is helpful, thank you. No, there isn't a separate method, we'll have to think about how we can/should support this in a future release.

@lhazlewood lhazlewood changed the title Empty values in claims are no longer supported as of 0.12.x Support empty claims values Oct 9, 2023
@lhazlewood
Copy link
Contributor

I'm working on a fix for this, but I noticed something unrelated in the referenced code that looked strange to me:

https://github.com/RaphiMC/ViaBedrock/blob/5674c4b0a04f64b5560064ba326d8ac3e6e5403a/src/main/java/net/raphimc/viabedrock/protocol/packets/LoginPackets.java#L322

x5u is defined in the RFC to be a URL/URI to a resource that contains an X509 Certificate or X509 Certificate chain, not the actual encoded public key value itself.

If you want to include the public key in the header, that's what the jwk header parameter is for, e.g.

EcPublicJwk jwk = Jwks.builder.key(publicKey).build();
Jwts.builder().header().jwk(jwk).and()...

Anyway, just an observation. Do with it what you will 😉 .

lhazlewood added a commit that referenced this issue Oct 14, 2023
Custom claims can now be empty again (behavior for <= 0.11.5).
@lhazlewood lhazlewood added this to the 0.12.3 milestone Oct 14, 2023
@lhazlewood lhazlewood self-assigned this Oct 14, 2023
@lhazlewood lhazlewood linked a pull request Oct 14, 2023 that will close this issue
lhazlewood added a commit that referenced this issue Oct 14, 2023
Closes #858.

Custom claims can now be empty again (which was the behavior for <= 0.11.5).
@lhazlewood
Copy link
Contributor

This change is in the 0.12.3 release.

@RaphiMC
Copy link
Author

RaphiMC commented Oct 15, 2023

Thanks for the quick fix and thanks for looking over the surrounding JWT code and checking it for errors. I just dumped the JWT chain from the original Minecraft: Bedrock Edition client and can confirm that the x5u header field contains the encoded public key and not an URL/URI. I assume Mojang (Devs of the game) just didn't want to mess around with sending multiple requests to external servers from the client and server to verify validity of the JWT chain, so they just broke spec and did their own thing. I can't use the .jwk(jwk) method if it doesn't produce the exact same output as me manually handling the x5u header (Where I assume it produces different output), because the server (Which I am sending the JWT chain to) would have no idea on how to interpet this data and would kick me due to not being able to verify validity of my JWT chain.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants