◐ Shell
clean mode source ↗

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

jdahmubed

lccodes

}

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

Choose a reason for hiding this comment

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

Why does this require a jwksFile and a pemFile?

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

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

lccodes

}

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

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

fixed locally

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

fetchContentAndSignatureByteArrays()?

Choose a reason for hiding this comment

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

Sounds good

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

Choose a reason for hiding this comment

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

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

lyzhang27

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

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

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

lyzhang27

@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");

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

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

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

private final Header header;
private final Payload payload;

private static final String FACEBOOK = "facebook";

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

done

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

Choose a reason for hiding this comment

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

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)) {

Choose a reason for hiding this comment

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

but then they're 2 diff meanings. i will do a null/empty check on issuer upstream though
*/
public abstract void verify(DecodedJWT jwt, EncodeType encodeType) throws Exception;

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

Choose a reason for hiding this comment

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

javadoc?

Choose a reason for hiding this comment

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

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

import java.io.UnsupportedEncodingException;
import java.io.*;

Choose a reason for hiding this comment

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

Try to import only the required classes.

Choose a reason for hiding this comment

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

done

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

Choose a reason for hiding this comment

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

"X509 is not supported for HMAC algorithm"

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

done
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"));

Choose a reason for hiding this comment

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

Don't use Claims constants

Choose a reason for hiding this comment

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

i dont see why its a prob for tests

Choose a reason for hiding this comment

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

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?

Choose a reason for hiding this comment

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

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.
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"));

Choose a reason for hiding this comment

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

Don't use Claims constants
assertThat(claims, is(notNullValue()));
exception.expect(UnsupportedOperationException.class);
claims.put("name", null);
claims.put(Claims.NAME, null);

Choose a reason for hiding this comment

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

Don't use Claims constants
@Test
public void shouldSerializeStrings() throws Exception {
ClaimsHolder holder = holderFor("name", "Auth0 Inc");
ClaimsHolder holder = holderFor(Claims.NAME, "Auth0 Inc");

Choose a reason for hiding this comment

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

Don't use Claims constants

lbalmaceda

Choose a reason for hiding this comment

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

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.
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'

Choose a reason for hiding this comment

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

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?

Choose a reason for hiding this comment

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

its for the PemReader object used in these 2 lines:

            PemReader reader = new PemReader(file);
            X509EncodedKeySpec caKeySpec = new X509EncodedKeySpec(reader.readPemObject().getContent());
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'

Choose a reason for hiding this comment

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

Yet another JSON library?

Choose a reason for hiding this comment

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

probably was an unused import. removed.
@@ -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

Choose a reason for hiding this comment

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

what is this for and why is it here?

Choose a reason for hiding this comment

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

its for the tests to test x509 functionality
payloadJson = URLDecoder.decode(new String(base32.decode(parts[1]), StandardCharsets.UTF_8.name()));
break;
case Base64:
headerJson = StringUtils.newStringUtf8(Base64.decodeBase64(parts[0]));

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

i provide the values myself, so theres no way it cant be one of the 3
@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");

Choose a reason for hiding this comment

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

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.
@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");

Choose a reason for hiding this comment

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

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.

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) {

Choose a reason for hiding this comment

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

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();

Choose a reason for hiding this comment

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

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
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()));

Choose a reason for hiding this comment

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

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.

@TMSCH TMSCH removed their request for review

May 23, 2018 21:02