Issue #4128 - test the decoding of OpenId Credentials #4166

lachlan-roberts · 2019-10-07T22:10:16Z

new tests for changes from PR #4129 for issue #4128

Signed-off-by: Lachlan Roberts <lachlan@webtide.com>

…128-testDecoderPadding

jetty-openid/src/main/java/org/eclipse/jetty/security/openid/CredentialsDecoder.java

jetty-openid/src/test/java/org/eclipse/jetty/security/openid/CredentialsDecoderTest.java

jetty-openid/src/test/java/org/eclipse/jetty/security/openid/OpenIdAuthenticationTest.java

jetty-openid/src/test/java/org/eclipse/jetty/security/openid/JwtEncoder.java

Signed-off-by: Lachlan Roberts <lachlan@webtide.com>

…128-testDecoderPadding Signed-off-by: Lachlan Roberts <lachlan@webtide.com>

Signed-off-by: Lachlan Roberts <lachlan@webtide.com>

jetty-openid/src/test/java/org/eclipse/jetty/security/openid/JwtDecoderTest.java

jetty-openid/src/main/java/org/eclipse/jetty/security/openid/JwtDecoder.java

joakime

Needs real world encoded examples that MUST decode AND require the strange padding hack.

jetty-openid/src/test/java/org/eclipse/jetty/security/openid/JwtDecoderTest.java

travisspencer · 2019-10-10T16:27:52Z

@joakime , it would be great if Java provided a decoder/encoder that worked with JOSE, but, to the best of my knowledge, it doesn't. That knowledge has been gained by built an entire OpenID Connect server in Java writing similar Java modules to this for use in Web apps and APIs.

The "real-world" examples may not exist in the test, but what @lachlan-roberts has added are the quintessentials plus some more exotic corner cases that we should not expect to receive but can now handle in case we do. The reason for this is that the decoding is done on parts of the JWT. a JWS (a signed JWT) is made up of 3 parts separated by a dot. This special decoding has to be done on each of these 3 parts. The "hacky" padJWTSection method (which is similar to the example in the JOSE RFC, so not sure how "hacky" it is BTW), is just to decode each of these parts of the JWS. For this reason, the real world input would need to execute more than just the code that's currently under test.

Hope this clarifies and relieves any anxiety you might have about this test or this new code.

joakime · 2019-11-05T22:31:58Z

I have nothing to add to this PR.
At this point I'm neutral on it.
My only concerns were with the Base64 behaviors, not the OpenID behaviors.
I'm withdrawing review request entirely.

I'm neutral.

…128-testDecoderPadding Signed-off-by: Lachlan Roberts <lachlan@webtide.com>

Signed-off-by: Lachlan Roberts <lachlan@webtide.com>

lachlan-roberts · 2019-11-13T05:19:43Z

@travisspencer could we use your example from #4128 in a test as a real world example which demonstrates this issue?

travisspencer · 2019-11-13T09:10:35Z

jetty-openid/src/main/java/org/eclipse/jetty/security/openid/JwtDecoder.java

+        Object parsedJwtHeader = JSON.parse(jwtHeaderString);
+        if (!(parsedJwtHeader instanceof Map))
+            throw new IllegalStateException("Invalid JWT header");
+        Map<String, Object> jwtHeader = (Map)parsedJwtHeader;


This is not the safest of casts. I'd check that the keys are strings using code like we've done in previous PRs.

Would a Map with keys that are not strings even be valid JSON. If our JSON parser is capable of returning invalid JSON then we should fix that in the parser rather than testing the return type is valid everywhere it is used.

That's a fair point, and do not oppose this change given that argument. However, I still would do the conversion at run-time to a Map<String, Object> and not cast it to that. We're not in a high throughput case here. We can tolerate 250ms for the entire code flow + login (say 6 request/responses) in most cases I've seen. So, I'd err on the side of clean code that relies on the compiler for type checking. As I said though, your argument is hard to dispute.

travisspencer · 2019-11-13T09:12:16Z

jetty-openid/src/main/java/org/eclipse/jetty/security/openid/JwtDecoder.java

+        Object parsedClaims = JSON.parse(jwtClaimString);
+        if (!(parsedClaims instanceof Map))
+            throw new IllegalStateException("Could not decode JSON for JWT claims.");
+        return (Map)parsedClaims;


This method returns a typed map. These types should be checked as well before returning.

travisspencer · 2019-11-13T09:16:35Z

Yes, @lachlan-roberts . That JWT should work. You can decode it using oauth.tools, BTW. There, you can see that the purpose claim is id, so it's an ID token like the one that an OP will issue to Jetty.

…28-testDecoderPadding

Signed-off-by: Lachlan Roberts <lachlan@webtide.com>

janbartel

In general, this looks good to me - seems like it's mostly a refactor of existing code. But, not being an expert in the JWT area, I'm leaving critique of the substance of this code in the very capable hands of @travisspencer . My comments are purely in the syntactic/coding practice area. I also note @joakime's suggestion to have some positive tests, and that would be good, if at all possible.

jetty-openid/src/test/java/org/eclipse/jetty/security/openid/JwtEncoder.java

Signed-off-by: Lachlan Roberts <lachlan@webtide.com>

travisspencer · 2019-11-20T02:25:02Z

@janbartel , I've looked again. The handling of the JWTs seems good to me.

lachlan-roberts · 2019-11-20T03:22:33Z

Thanks @travisspencer!
I have also added the test with the JWT example from #4128 in commit 67ec3d2.
I will go ahead and merge.

Issue #4128 - test the decoding of OpenId Credentials

153c404

Signed-off-by: Lachlan Roberts <lachlan@webtide.com>

lachlan-roberts requested a review from joakime October 7, 2019 22:10

add missing licence headers

cee3552

Signed-off-by: Lachlan Roberts <lachlan@webtide.com>

lachlan-roberts mentioned this pull request Oct 7, 2019

Issue #4128 - Add missing padding and use URL decoder #4129

Merged

Merge remote-tracking branch 'eclipse/jetty-9.4.x' into jetty-9.4.x-4…

8b7c131

…128-testDecoderPadding

travisspencer approved these changes Oct 7, 2019

View reviewed changes

lachlan-roberts added 3 commits October 8, 2019 10:39

changes from review

218505b

Signed-off-by: Lachlan Roberts <lachlan@webtide.com>

Merge remote-tracking branch 'eclipse/jetty-9.4.x' into jetty-9.4.x-4…

dd8914d

…128-testDecoderPadding Signed-off-by: Lachlan Roberts <lachlan@webtide.com>

changes from review

0c73cac

Signed-off-by: Lachlan Roberts <lachlan@webtide.com>

joakime reviewed Oct 9, 2019

View reviewed changes

jetty-openid/src/test/java/org/eclipse/jetty/security/openid/JwtDecoderTest.java Show resolved Hide resolved

joakime reviewed Oct 9, 2019

View reviewed changes

jetty-openid/src/main/java/org/eclipse/jetty/security/openid/JwtDecoder.java Show resolved Hide resolved

joakime previously requested changes Oct 9, 2019

View reviewed changes

jetty-openid/src/test/java/org/eclipse/jetty/security/openid/JwtDecoderTest.java Show resolved Hide resolved

lachlan-roberts requested a review from joakime October 21, 2019 23:44

joakime requested review from joakime and removed request for joakime November 5, 2019 22:32

joakime removed their request for review November 5, 2019 22:32

lachlan-roberts added 2 commits November 13, 2019 16:11

Merge remote-tracking branch 'eclipse/jetty-9.4.x' into jetty-9.4.x-4…

06b7925

…128-testDecoderPadding Signed-off-by: Lachlan Roberts <lachlan@webtide.com>

remove file from merge

2b0e1f5

Signed-off-by: Lachlan Roberts <lachlan@webtide.com>

lachlan-roberts requested a review from janbartel November 13, 2019 05:20

travisspencer reviewed Nov 13, 2019

View reviewed changes

lachlan-roberts added 2 commits November 19, 2019 10:45

Merge remote-tracking branch 'origin/jetty-9.4.x' into jetty-9.4.x-41…

7d94370

…28-testDecoderPadding

Add test using JWT example from #4128 which demonstrates the issue

67ec3d2

Signed-off-by: Lachlan Roberts <lachlan@webtide.com>

janbartel approved these changes Nov 19, 2019

View reviewed changes

jetty-openid/src/test/java/org/eclipse/jetty/security/openid/JwtEncoder.java Show resolved Hide resolved

jetty-openid/src/test/java/org/eclipse/jetty/security/openid/JwtEncoder.java Show resolved Hide resolved

Add javadoc for JwtEncoder

f784135

Signed-off-by: Lachlan Roberts <lachlan@webtide.com>

lachlan-roberts merged commit cff6bb4 into jetty-9.4.x Nov 20, 2019

lachlan-roberts deleted the jetty-9.4.x-4128-testDecoderPadding branch November 20, 2019 03:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue #4128 - test the decoding of OpenId Credentials #4166

Issue #4128 - test the decoding of OpenId Credentials #4166

lachlan-roberts commented Oct 7, 2019

joakime left a comment

travisspencer commented Oct 10, 2019

joakime commented Nov 5, 2019

lachlan-roberts commented Nov 13, 2019

travisspencer Nov 13, 2019

lachlan-roberts Nov 13, 2019

travisspencer Nov 14, 2019

travisspencer Nov 13, 2019

travisspencer commented Nov 13, 2019

janbartel left a comment

travisspencer commented Nov 20, 2019

lachlan-roberts commented Nov 20, 2019

Issue #4128 - test the decoding of OpenId Credentials #4166

Issue #4128 - test the decoding of OpenId Credentials #4166

Conversation

lachlan-roberts commented Oct 7, 2019

joakime left a comment

Choose a reason for hiding this comment

travisspencer commented Oct 10, 2019

joakime commented Nov 5, 2019

lachlan-roberts commented Nov 13, 2019

travisspencer Nov 13, 2019

Choose a reason for hiding this comment

lachlan-roberts Nov 13, 2019

Choose a reason for hiding this comment

travisspencer Nov 14, 2019

Choose a reason for hiding this comment

travisspencer Nov 13, 2019

Choose a reason for hiding this comment

travisspencer commented Nov 13, 2019

janbartel left a comment

Choose a reason for hiding this comment

travisspencer commented Nov 20, 2019

lachlan-roberts commented Nov 20, 2019