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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 11 commits into from Nov 20, 2019
Merged

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
153c404
Issue #4128 - test the decoding of OpenId Credentials
lachlan-roberts Oct 7, 2019
cee3552
add missing licence headers
lachlan-roberts Oct 7, 2019
8b7c131
Merge remote-tracking branch 'eclipse/jetty-9.4.x' into jetty-9.4.x-4…
lachlan-roberts Oct 7, 2019
218505b
changes from review
lachlan-roberts Oct 7, 2019
dd8914d
Merge remote-tracking branch 'eclipse/jetty-9.4.x' into jetty-9.4.x-4…
lachlan-roberts Oct 7, 2019
0c73cac
changes from review
lachlan-roberts Oct 8, 2019
06b7925
Merge remote-tracking branch 'eclipse/jetty-9.4.x' into jetty-9.4.x-4…
lachlan-roberts Nov 13, 2019
2b0e1f5
remove file from merge
lachlan-roberts Nov 13, 2019
7d94370
Merge remote-tracking branch 'origin/jetty-9.4.x' into jetty-9.4.x-41…
lachlan-roberts Nov 18, 2019
67ec3d2
Add test using JWT example from #4128 which demonstrates the issue
lachlan-roberts Nov 18, 2019
f784135
Add javadoc for JwtEncoder
lachlan-roberts Nov 19, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
10 changes: 8 additions & 2 deletions jetty-openid/src/main/java/org/eclipse/jetty/security/openid/JwtDecoder.java
View file
Open in desktop
Expand Up @@ -53,7 +53,10 @@ public static Map<String, Object> decode(String jwt)
String jwtClaimString = new String(decoder.decode(padJWTSection(sections[1])), StandardCharsets.UTF_8);
String jwtSignature = sections[2];

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

if (LOG.isDebugEnabled())
LOG.debug("JWT Header: {}", jwtHeader);

Expand All @@ -63,7 +66,10 @@ and the Token Endpoint (which it is in this flow), the TLS server validation
if (LOG.isDebugEnabled())
LOG.debug("JWT signature not validated {}", jwtSignature);

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}

static byte[] padJWTSection(String unpaddedEncodedJwtSection)
Expand Down
5 changes: 1 addition & 4 deletions jetty-openid/src/main/java/org/eclipse/jetty/security/openid/OpenIdCredentials.java
View file
Open in desktop
Expand Up @@ -18,9 +18,7 @@

package org.eclipse.jetty.security.openid;

import java.io.IOException;
import java.io.Serializable;
import java.nio.charset.StandardCharsets;
import java.util.Arrays;
import java.util.Map;
import java.util.concurrent.TimeUnit;
Expand Down Expand Up @@ -170,7 +168,7 @@ public boolean isExpired()
return false;
}

private Map<String, Object> claimAuthCode(String authCode) throws IOException
private Map<String, Object> claimAuthCode(HttpClient httpClient, String authCode) throws Exception
{
Fields fields = new Fields();
fields.add("code", authCode);
Expand All @@ -190,7 +188,6 @@ private Map<String, Object> claimAuthCode(String authCode) throws IOException
Object parsedResponse = JSON.parse(responseBody);
if (!(parsedResponse instanceof Map))
throw new IllegalStateException("Malformed response from OpenID Provider");

return (Map)parsedResponse;
}
}
3 changes: 3 additions & 0 deletions ...d-cdi-webapp/overlays/org.eclipse.jetty.tests.test-cdi-common-webapp-9.4.22-SNAPSHOT.info
View file
Open in desktop
@@ -0,0 +1,3 @@
1573621109636
(?:[^/]+/)*?[^/]*?
META-INF(?:$|/.+)
You are viewing a condensed version of this merge commit. You can view the full changes here.