X509 by jdahmubed · Pull Request #7 · GJWT/javaOIDCMsg

jdahmubed · 2017-12-20T22:49:46Z

No description provided.

lccodes · 2017-12-21T00:57:47Z

lib/src/main/java/com/auth0/jwt/algorithms/RSAAlgorithm.java

    }

+    @Override
+    public void verifyWithX509(DecodedJWT jwt, EncodeType encodeType, String jwksFile, String pemFile) throws Exception {


Why does this require a jwksFile and a pemFile?

It's needed to get the x509 cert. why do you feel like it's unnecessary?

Can you explain what's in each that you need? I thought the jwksFile contained the key.

jwksFile contains the x5c value. pemFile is the public key.

lccodes · 2017-12-21T00:58:15Z

lib/src/main/java/com/auth0/jwt/algorithms/RSAAlgorithm.java

    }

+    @Override
+    public void verifyWithX509(DecodedJWT jwt, EncodeType encodeType, String jwksFile, String pemFile) throws Exception {


Can you write a wrapper for this that infers its EncodingType? That will probably work better in most cases

good point. i can have a constant that stores what type of encodingtype was used to sign the JWT with and use it accordingly.

fixed locally

Cool. Can you do anything for unknown JWTs? Can't we see what base something is in based off what characters are used?

i dont think there's any other way. i tried this, which keeps returning UTF-8: https://code.google.com/archive/p/juniversalchardet/

what i can try to do is run the decode() method for each of the base encodings for the particular string and whichever one doesnt throw an exception is the one. is that what you want?

lyzhang27 · 2017-12-21T07:14:28Z

lib/src/main/java/com/auth0/jwt/algorithms/RSAAlgorithm.java


+    @Override
+    public void verifyWithX509(DecodedJWT jwt, EncodeType encodeType, String jwksFile, String pemFile) throws Exception {
+        List<byte[]> byteArrayList = decode(jwt, encodeType);


Why is it that we have to run "decode" again on a "decoded jwt"? I think there could be better naming here.

fetchContentAndSignatureByteArrays()?

Sounds good

lyzhang27 · 2017-12-21T07:14:38Z

lib/src/main/java/com/auth0/jwt/algorithms/RSAAlgorithm.java

+
    @Override
    public void verify(DecodedJWT jwt, EncodeType encodeType) throws Exception {
+        List<byte[]> byteArrayList = decode(jwt, encodeType);


Why is it that we have to run "decode" again on a "decoded jwt"? I think there could be better naming here.

lyzhang27 · 2017-12-21T16:05:03Z

lib/src/main/java/com/auth0/jwt/creators/JWTCreator.java

 import java.net.URLDecoder;
 import java.net.URLEncoder;
 import java.nio.charset.StandardCharsets;
+import java.util.Arrays;


This is never used, along with org.apache.commons.codec.Encoder, org.apache.commons.codec.binary.StringUtils,
java.net.URLDecoder, java.util.logging.Logger

removed locally. i was thinking of cleaning these up in a separate PR later.

lyzhang27 · 2017-12-21T16:15:08Z

lib/src/main/java/com/auth0/jwt/jwts/JWT.java

        @Override
        public Verification withNbf(long nbf) {
-            throw new UnsupportedOperationException("you shouldn't be calling this method");
+            throw new UnsupportedOperationException("this method has not been implemented");


Why is it that we have all these methods that shouldn't be called?

because they're implementing the interface method. they are required to implement it or declare the class abstract.

@lbalmaceda

The comment from @lyzhang27 is correct. You should not expose methods you don't want your users to call. A good inheritance design (parent/child classes) can prevent this.

lbalmaceda · 2018-01-15T14:15:27Z

lib/src/main/java/com/auth0/jwt/JWTDecoder.java

    private final Header header;
    private final Payload payload;

+    private static final String FACEBOOK = "facebook";


Prepend something that indicates where this could be used. i.e. "ISSUER_FACEBOOK". Same for the one below.

lbalmaceda · 2018-01-15T14:18:23Z

lib/src/main/java/com/auth0/jwt/JWTDecoder.java

+
+    public static GoogleOrFbJwtCreator decodeJWT(DecodedJWT jwt) {
+        Map<String, Claim> claims = jwt.getClaims();
+        String issuer = claims.get(Claims.ISSUER).asString();


Claims.asString might return null. Can the issuer be null? Probably it can. Better flip the conditionals to avoid NPE. We know in advance that the constants are not null:

if(issuer.contains(FACEBOOK)) { ---> if(FACEBOOK.contains(issuer)) {

but then they're 2 diff meanings. i will do a null/empty check on issuer upstream though

lbalmaceda · 2018-01-15T14:24:39Z

lib/src/main/java/com/auth0/jwt/algorithms/Algorithm.java

     */
    public abstract void verify(DecodedJWT jwt, EncodeType encodeType) throws Exception;

+    public abstract void verifyWithX509(DecodedJWT jwt, String jwksFile, String pemFile) throws Exception;


lbalmaceda · 2018-01-15T14:25:48Z

lib/src/main/java/com/auth0/jwt/algorithms/HMACAlgorithm.java

 import org.apache.commons.codec.binary.StringUtils;

-import java.io.UnsupportedEncodingException;
+import java.io.*;


Try to import only the required classes.

lbalmaceda · 2018-01-15T14:27:05Z

lib/src/main/java/com/auth0/jwt/algorithms/HMACAlgorithm.java


+    @Override
+    public void verifyWithX509(DecodedJWT jwt, String jwksFile, String pemFile) throws Exception {
+        throw new UnsupportedOperationException("X509 is not supported for HMAC");


"X509 is not supported for HMAC algorithm"

lbalmaceda · 2018-01-15T18:48:59Z

lib/src/main/java/com/auth0/jwt/creators/AccessJwtCreator.java

-            if(!addedClaims.get(claim))
+        for(String claim : requiredClaims.keySet())
+            if(!requiredClaims.get(claim))
                throw new Exception("Standard claim: " + claim + " has not been set");


Can we please throw something more descriptive than "Exception"? i.e. IllegalStateException/IllegalArgumentException or even a RequiredClaimException

lbalmaceda · 2018-01-15T18:52:45Z

lib/src/test/java/com/auth0/jwt/impl/JsonNodeClaimTest.java

        assertThat(claim, is(notNullValue()));
        Map map = claim.as(Map.class);
-        assertThat(((Map<String, Object>) map.get("key")), hasEntry("name", (Object) "john"));
+        assertThat(((Map<String, Object>) map.get("key")), hasEntry(Claims.NAME, (Object) "john"));


Don't use Claims constants

i dont see why its a prob for tests

Simple put:

//constants (values source) class Claims { NAME="name" //... } //library code class Foo { void Bar { //... message.addClaim(Claims.NAME, "John Doe"); } } //test code class FooTest { //... assert(message.getClaim(Claims.NAME), is("John Doe")); }

Now go and change Claims.NAME to "namee".

Will the test pass?

there are many more failing tests after its changed to "namee", which is expected, right? thats why i dont get why you're against using constants in tests.

lbalmaceda · 2018-01-15T18:53:08Z

lib/src/test/java/com/auth0/jwt/impl/PayloadDeserializerTest.java

        assertThat(payload.getClaim("exp").asDouble(), is(11111111D));
        assertThat(payload.getClaim("nbf").asDouble(), is(10101011D));
-        assertThat(payload.getClaim("jti").asString(), is("idid"));
+        assertThat(payload.getClaim(Claims.JWT_ID).asString(), is("idid"));


Don't use Claims constants

lbalmaceda · 2018-01-15T18:53:12Z

lib/src/test/java/com/auth0/jwt/impl/PayloadImplTest.java

        assertThat(claims, is(notNullValue()));
        exception.expect(UnsupportedOperationException.class);
-        claims.put("name", null);
+        claims.put(Claims.NAME, null);


Don't use Claims constants

lbalmaceda · 2018-01-15T18:53:21Z

lib/src/test/java/com/auth0/jwt/impl/PayloadSerializerTest.java

    @Test
    public void shouldSerializeStrings() throws Exception {
-        ClaimsHolder holder = holderFor("name", "Auth0 Inc");
+        ClaimsHolder holder = holderFor(Claims.NAME, "Auth0 Inc");


Don't use Claims constants

lbalmaceda

General things:

Keep the PR short. This is already long and needs to be shrinked.
Avoid pressing the hotkey to reorder imports as they generate excessive and unnecessary diff. Rollback those changes if possible.
Don't change the logic of the existing tests. They are there for a reason.
Either use the constants on the tests or on the exported classes, not in both.
Don't export files you don't want people to have
Don't add methods that throw an exception because "they shouldn't be called".

Important one: Any public API changes are to be discussed with the team. Pause the coding and create a doc to discuss this.

lbalmaceda · 2018-03-07T15:37:45Z

lib/build.gradle

    compile 'commons-codec:commons-codec:1.11'
    compile 'com.google.code.gson:gson:2.8.2'
+    compile 'com.auth0:jwks-rsa:0.3.0'
+    compile 'com.nimbusds:nimbus-jose-jwt:2.19.1'


It feels wrong that a jwt library is using another jwt library. Where is this used for and can it be coded in this same library rather than depending on them?

its for the PemReader object used in these 2 lines:

PemReader reader = new PemReader(file); X509EncodedKeySpec caKeySpec = new X509EncodedKeySpec(reader.readPemObject().getContent());

lbalmaceda · 2018-03-07T15:38:33Z

lib/build.gradle

+    compile 'org.apache.httpcomponents:httpcore:4.4.1'
+    compile 'org.apache.httpcomponents:httpclient:4.5'
+    compile group: 'org.slf4j', name:'slf4j-api', version: '1.7.2'
+    compile group: 'com.googlecode.json-simple', name: 'json-simple', version: '1.1.1'


Yet another JSON library?

probably was an unused import. removed.

lbalmaceda · 2018-03-07T15:39:20Z

lib/jwksRSA.json

@@ -0,0 +1 @@
+{"keys":[{"alg":"RS256","kty":"RSA","use":"sig","x5c":["MIIDDTCCAfWgAwIBAgIJAJVkuSv2H8mDMA0GCSqGSIb3DQEBBQUAMB0xGzAZBgNVBAMMEnNhbmRyaW5vLmF1dGgwLmNvbTAeFw0xNDA1MTQyMTIyMjZaFw0yODAxMjEyMTIyMjZaMB0xGzAZBgNVBAMMEnNhbmRyaW5vLmF1dGgwLmNvbTCCASIwDQYJKoZIhvcNAQEBBQADggEPADCCAQoCggEBAL6jWASkHhXz5Ug6t5BsYBrXDIgrWu05f3oq2fE+5J5REKJiY0Ddc+Kda34ZwOptnUoef3JwKPDAckTJQDugweNNZPwOmFMRKj4xqEpxEkIX8C+zHs41Q6x54ZZy0xU+WvTGcdjzyZTZ/h0iOYisswFQT/s6750tZG0BOBtZ5qS/80tmWH7xFitgewdWteJaASE/eO1qMtdNsp9fxOtN5U/pZDUyFm3YRfOcODzVqp3wOz+dcKb7cdZN11EYGZOkjEekpcedzHCo9H4aOmdKCpytqL/9FXoihcBMg39s1OW3cfwfgf5/kvOJdcqR4PoATQTfsDVoeMWVB4XLGR6SC5kCAwEAAaNQME4wHQYDVR0OBBYEFHDYn9BQdup1CoeoFi0Rmf5xn/W9MB8GA1UdIwQYMBaAFHDYn9BQdup1CoeoFi0Rmf5xn/W9MAwGA1UdEwQFMAMBAf8wDQYJKoZIhvcNAQEFBQADggEBAGLpQZdd2ICVnGjc6CYfT3VNoujKYWk7E0shGaCXFXptrZ8yaryfo6WAizTfgOpQNJH+Jz+QsCjvkRt6PBSYX/hb5OUDU2zNJN48/VOw57nzWdjI70H2Ar4oJLck36xkIRs/+QX+mSNCjZboRwh0LxanXeALHSbCgJkbzWbjVnfJEQUP9P/7NGf0MkO5I95C/Pz9g91y8gU+R3imGppLy9Zx+OwADFwKAEJak4JrNgcjHBQenakAXnXP6HG4hHH4MzO8LnLiKv8ZkKVL67da/80PcpO0miMNPaqBBMd2Cy6GzQYE0ag6k0nk+DMIFn7K+o21gjUuOEJqIbAvhbf2KcM="],"n":"vqNYBKQeFfPlSDq3kGxgGtcMiCta7Tl_eirZ8T7knlEQomJjQN1z4p1rfhnA6m2dSh5_cnAo8MByRMlAO6DB401k_A6YUxEqPjGoSnESQhfwL7MezjVDrHnhlnLTFT5a9MZx2PPJlNn-HSI5iKyzAVBP-zrvnS1kbQE4G1nmpL_zS2ZYfvEWK2B7B1a14loBIT947Woy102yn1_E603lT-lkNTIWbdhF85w4PNWqnfA7P51wpvtx1k3XURgZk6SMR6Slx53McKj0fho6Z0oKnK2ov_0VeiKFwEyDf2zU5bdx_B-B_n-S84l1ypHg-gBNBN-wNWh4xZUHhcsZHpILmQ","e":"AQAB","kid":"8RGoVdVjD8fItyR3FFo0hVNaZYtPGwoP6xKi9e_V7bI","x5t":"RkI5MjI5OUY5ODc1N0Q4QzM0OUYzNkVGMTJDOUEzQkFCOTU3NjE2Rg"}]}


what is this for and why is it here?

its for the tests to test x509 functionality

lbalmaceda · 2018-03-07T15:46:43Z

lib/src/main/java/com/auth0/jwt/JWTDecoder.java

+                payloadJson = URLDecoder.decode(new String(base32.decode(parts[1]), StandardCharsets.UTF_8.name()));
                break;
            case Base64:
                headerJson = StringUtils.newStringUtf8(Base64.decodeBase64(parts[0]));


why StringUtils.newStringUtf8() here and new String("", UTF8) above?. Keep 1. Normally, stick to classes provided in the JDK.

good point. changed.

lbalmaceda · 2018-03-07T15:47:22Z

lib/src/main/java/com/auth0/jwt/JWTDecoder.java

@@ -55,13 +60,13 @@ public JWTDecoder(String jwt, EncodeType encodeType) throws Exception {
        String payloadJson = null;
        switch (encodeType) {


What if the encode type doesn't match any value defined in the switch?

i provide the values myself, so theres no way it cant be one of the 3

lbalmaceda · 2018-03-08T13:53:12Z

lib/src/main/java/com/auth0/jwt/jwts/GoogleJWT.java

    @Override
    public Verification createVerifierForExtended(String picture, String email, List<String> issuer, List<String> audience, String name, long nbf, long expLeeway, long iatLeeway) {
-        throw new UnsupportedOperationException("you shouldn't be calling this method");
+        throw new UnsupportedOperationException("this method has not been implemented");


Sure. Share in the slack channel a doc with all the classes you want to have and the methods/claims each one should have to begin with.

lbalmaceda · 2018-03-08T14:07:01Z

lib/src/main/java/com/auth0/jwt/jwts/JWT.java

        @Override
        public Verification createVerifierForAccess(List<String> issuer, List<String> audience, long expLeeway, long iatLeeway) {
-            throw new UnsupportedOperationException("you shouldn't be calling this method");
+            throw new UnsupportedOperationException("this method has not been implemented");


The only reason I asked you to change the message to "It hasn't been implemented" is if the method is eventually going to be available. If this class is not supposed to have the methods to begin with, remove them.

lbalmaceda · 2018-03-08T14:12:21Z

lib/src/main/java/com/auth0/jwt/verification/VerificationAndAssertion.java


-    private static void assertValidIssuerClaim(List<String> issuer, List<String> value) {
-        if (issuer == null || !issuer.containsAll(value) || issuer.size() != value.size()) {
+    private static void assertValidIssuerClaim(List<String> actual, List<String> expected) {


https://tools.ietf.org/html/rfc7519#section-4.1.1

lbalmaceda · 2018-03-08T14:20:35Z

lib/src/test/java/com/auth0/jwt/algorithms/HMACAlgorithmTest.java

        String token = "eyJhbGciOiJIUzI1NiIsImN0eSI6IkpXVCJ9.eyJpc3MiOiJhdXRoMCJ9.mZ0m_N1J4PgeqWmi903JuUoDRZDBPB7HwkS4nVyWH1M";
        Algorithm algorithmString = Algorithm.HMAC256("secret");
-        Algorithm algorithmBytes = Algorithm.HMAC256("secret".getBytes(StandardCharsets.UTF_8));
        JWT jwt = JWT.require(algorithmString).withIssuer("auth0").build();


You changed all the tests to use JWT.require instead. Please roll back all these changes. Those tests are already available here.
This comment applies to all XAlgorithmTest classes

lbalmaceda · 2018-03-08T14:23:06Z

lib/src/test/java/com/auth0/jwt/creators/AccessJwtCreatorTest.java

+        assertTrue(claims.get(Claims.ISSUER).asString().equals("issuer"));
+        assertTrue(claims.get(Claims.SUBJECT).asString().equals("subject"));
+        assertTrue(claims.get(Claims.AUDIENCE).asString().equals("audience"));
+        assertTrue(claims.get(Claims.EXPIRES_AT).asDate().toString().equals(exp.toString()));


what do you mean by "doesn't work"? If two classes are of the same type and have valid values I don't see why equals won't work.

jdahmubed requested a review from TMSCH December 20, 2017 22:49

lccodes reviewed Dec 21, 2017

View reviewed changes

lyzhang27 reviewed Dec 21, 2017

View reviewed changes

lccodes requested a review from lbalmaceda January 10, 2018 17:25

lbalmaceda requested changes Jan 15, 2018

View reviewed changes

jdahmubed added 5 commits January 24, 2018 15:38

X509

92f43dc

X509

84459bc

X509

fc6782b

Refactoring

050e87d

X509

29fc485

jdahmubed force-pushed the x509 branch from 6763f15 to 29fc485 Compare March 1, 2018 22:25

lbalmaceda requested changes Mar 8, 2018

View reviewed changes

TMSCH removed their request for review May 23, 2018 21:02

		@@ -0,0 +1 @@
		{"keys":[{"alg":"RS256","kty":"RSA","use":"sig","x5c":["MIIDDTCCAfWgAwIBAgIJAJVkuSv2H8mDMA0GCSqGSIb3DQEBBQUAMB0xGzAZBgNVBAMMEnNhbmRyaW5vLmF1dGgwLmNvbTAeFw0xNDA1MTQyMTIyMjZaFw0yODAxMjEyMTIyMjZaMB0xGzAZBgNVBAMMEnNhbmRyaW5vLmF1dGgwLmNvbTCCASIwDQYJKoZIhvcNAQEBBQADggEPADCCAQoCggEBAL6jWASkHhXz5Ug6t5BsYBrXDIgrWu05f3oq2fE+5J5REKJiY0Ddc+Kda34ZwOptnUoef3JwKPDAckTJQDugweNNZPwOmFMRKj4xqEpxEkIX8C+zHs41Q6x54ZZy0xU+WvTGcdjzyZTZ/h0iOYisswFQT/s6750tZG0BOBtZ5qS/80tmWH7xFitgewdWteJaASE/eO1qMtdNsp9fxOtN5U/pZDUyFm3YRfOcODzVqp3wOz+dcKb7cdZN11EYGZOkjEekpcedzHCo9H4aOmdKCpytqL/9FXoihcBMg39s1OW3cfwfgf5/kvOJdcqR4PoATQTfsDVoeMWVB4XLGR6SC5kCAwEAAaNQME4wHQYDVR0OBBYEFHDYn9BQdup1CoeoFi0Rmf5xn/W9MB8GA1UdIwQYMBaAFHDYn9BQdup1CoeoFi0Rmf5xn/W9MAwGA1UdEwQFMAMBAf8wDQYJKoZIhvcNAQEFBQADggEBAGLpQZdd2ICVnGjc6CYfT3VNoujKYWk7E0shGaCXFXptrZ8yaryfo6WAizTfgOpQNJH+Jz+QsCjvkRt6PBSYX/hb5OUDU2zNJN48/VOw57nzWdjI70H2Ar4oJLck36xkIRs/+QX+mSNCjZboRwh0LxanXeALHSbCgJkbzWbjVnfJEQUP9P/7NGf0MkO5I95C/Pz9g91y8gU+R3imGppLy9Zx+OwADFwKAEJak4JrNgcjHBQenakAXnXP6HG4hHH4MzO8LnLiKv8ZkKVL67da/80PcpO0miMNPaqBBMd2Cy6GzQYE0ag6k0nk+DMIFn7K+o21gjUuOEJqIbAvhbf2KcM="],"n":"vqNYBKQeFfPlSDq3kGxgGtcMiCta7Tl_eirZ8T7knlEQomJjQN1z4p1rfhnA6m2dSh5_cnAo8MByRMlAO6DB401k_A6YUxEqPjGoSnESQhfwL7MezjVDrHnhlnLTFT5a9MZx2PPJlNn-HSI5iKyzAVBP-zrvnS1kbQE4G1nmpL_zS2ZYfvEWK2B7B1a14loBIT947Woy102yn1_E603lT-lkNTIWbdhF85w4PNWqnfA7P51wpvtx1k3XURgZk6SMR6Slx53McKj0fho6Z0oKnK2ov_0VeiKFwEyDf2zU5bdx_B-B_n-S84l1ypHg-gBNBN-wNWh4xZUHhcsZHpILmQ","e":"AQAB","kid":"8RGoVdVjD8fItyR3FFo0hVNaZYtPGwoP6xKi9e_V7bI","x5t":"RkI5MjI5OUY5ODc1N0Q4QzM0OUYzNkVGMTJDOUEzQkFCOTU3NjE2Rg"}]} No newline at end of file

		@@ -55,13 +60,13 @@ public JWTDecoder(String jwt, EncodeType encodeType) throws Exception {
		String payloadJson = null;
		switch (encodeType) {

Comments

Conversation

jdahmubed commented Dec 20, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lyzhang27 Dec 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jdahmubed Mar 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lyzhang27 Dec 21, 2017 •

edited

Loading

jdahmubed Mar 7, 2018 •

edited

Loading