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

Issues in upgrading to 0.12.6 #957

Open
tiwarishubham635 opened this issue Jul 23, 2024 · 18 comments
Open

Issues in upgrading to 0.12.6 #957

tiwarishubham635 opened this issue Jul 23, 2024 · 18 comments

Comments

@tiwarishubham635
Copy link

Hello! I am tring to upgrade twilio-java to 0.12.6. This because of some known vulnerabilities like CVE-2024-31033 in jjwt 0.11.2. I saw there are some breaking changes in 0.12 version. Since this vulnerability is a blocker for our customers, we have to upgrade. However, I would appreciate if there is some other way we can get rid of this vulnerability.

Anyways, the main issue is that while upgrading to 0.12.6 I am facing a lot of errors. Here is the PR that I have raised. For example, setSigningKey method (though it is deprecated, it is showing errors.

Can someone help in upgrading the version? Maybe if there is some upgrade guide, that would be helpful.

@bdemers
Copy link
Member

bdemers commented Jul 23, 2024

Hi @tiwarishubham635!

A couple of things, CVE-2024-31033 is contested, there is a bit more discussion around it here: #930 TL;DR we are waiting for that to get resolved. (but I get it... nobody wants vuln reports, even false reports in their builds)

Related to 0.12.6, the error you are seeing is related to your cty header value of: twilio-fpa;v=1
In 0.12, JJWT became more strict in how it parses JWTs, twilio-fpa;v=1 is an unknown content type, and JJWT, doesn't know how to parse that, same would be true if this were image/png.

You can read more about how the cty header is processed in the JWS RFC

In order to support non JWT content types, you can use the method: parseSignedContent(<token>)
That [indirectly] gives you a byte array (call .accept(JwtVisitor) to convert it to your desired object.

There is a similar discussion on this here: #897 (where there is also some discussion on wrapping this logic in a something like a ContentTypeHandler, e.g. Jwts.parser().cty(handler)...

Your feedback on the ideas in that thread would be helpful!

@tonedef71
Copy link

I, too, am having an issue with migrating to version 0.12.6.

An example from my original code looks like this:

  jwtBuilder.signWith(signingKey, SignatureAlgorithm.PS384);

It is flagged with warnings regarding deprecation.

I thought that the way to update it was to do this:

  jwtBuilder.signWith(signingKey, Jwts.SIG.PS384);

But that fails because the members of the Jwts.SIG class have types SignatureAlgorithm and MacAlgorithm which are not compatible with the SecureDigestAlgorithm<? extends Key, ?> type of the non-deprecated version of the signWith() method:
<K extends Key> JwtBuilder signWith(K key, SecureDigestAlgorithm<? super K, ?> alg).

How should one go about migrating usages of the deprecated signWith() methods which allow the caller to specify the desired signing algorithm?

@lhazlewood
Copy link
Contributor

@tonedef71 it's hard to tell without seeing the type of Key instance, but the newer (non-deprecated) method signature guarantees compile-time safety that the type of key being used matches the type of key required by the SecureDigestAlgorithm.

For example, if the alg instance requires a PrivateKey, ensure that your key instance is that type, and not just a generic Key. Or if a MAC algorithm, the key is a SecretKey and not just a Key.

Does that help? Hopefully that makes sense, but if not, let me know and we'll figure it out :)

@tonedef71
Copy link

tonedef71 commented Nov 26, 2024

@lhazlewood Thank you for the reply.

I have a utility POJO that dynamically derives signing keys and such based on configuration property String values:

  static PublicKey readPublicKey(...) throws Exception {
  }

  static PrivateKey deriveRsaSigningKey(...) throws Exception {
  }

  static SecretKey deriveHmacSigningKey(...) {
  }

  static Key deriveConsumerSigningKey(...) throws Exception {
    Key signingKey = null;
    if (...) {
      signingKey = readPublicKey(...); 
    }
    else if (...) {
      signingKey = deriveHmacSigningKey(...);
    }

    return signingKey;
  }

  static Key deriveProducerSigningKey(...) throws Exception {
    Key signingKey = null;
    if (...) {
      signingKey = deriveRsaSigningKey(...);
    }
    else if (...) {
      signingKey = deriveHmacSigningKey(...);
    }

    return signingKey;
  }

  static String generate(...) throws Exception {
    final Key signingKey = deriveProducerSigningKey(...);
    if (null != signingKey) {
      jwtBuilder.signWith(signingKey, signatureAlgorithm); // e.g. SignatureAlgorithm.RS384, SignatureAlgorithm.HS384, SignatureAlgorithm.ES384, SignatureAlgorithm.PS384
    }
    ...
    
    return jwtBuilder.compact();
  }

I conveniently refer to the various key interface types by their base interface type Key.

@tonedef71
Copy link

tonedef71 commented Dec 2, 2024

@lhazlewood When the audience of the JWT is a single recipient, how does one coax JwtBuilder.compact() to generate a JWT with an aud claim assigned a single string value instead of a single value array?

UPDATE:
Actually, it appears that the JwtBuilder.compact() method handles the single recipient aud just fine when the deprecated JwtBuilder.audience().single() method is used to add the single String value. Why is the method deprecated?

@lhazlewood
Copy link
Contributor

@tonedef71 it's deprecated to discourage its use, the RFC updated to prefer/recommend a String array, so people ideally shouldn't be using single string values anymore. It's there for backwards compatibility for legacy systems only. I hope that helps!

Also, if additional context helps, there's this as well: #944 (comment)

@lhazlewood
Copy link
Contributor

also if it matters, the JavaDoc for the .single method states:

image

@tonedef71
Copy link

@tonedef71 it's deprecated to discourage its use, the RFC updated to prefer/recommend a String array, so people ideally shouldn't be using single string values anymore. It's there for backwards compatibility for legacy systems only. I hope that helps!

Also, if additional context helps, there's this as well: #944 (comment)

@lhazlewood SIGH I dislike deprecation warnings. Oracle NetSuite seems to reject JWTs with single value arrays.

Do you plan on keeping all of the deprecated stuff around in future releases of the library indefinitely?

@lhazlewood
Copy link
Contributor

@tonedef71 when we release v 1.0, we will remove all deprecated methods except this one and any others mandated as a requirement by the RFCs. For the very few (one? two? can't remember) that must remain per RFC requirements, marking them @Deprecated is the only way to indicate to developers that they ideally shouldn't be used, and the associated JavaDoc is very clear as to why - I'm afraid that's the best we can do as there's no other way that I know of other than "you should have read the project documentation" (which very few people do unfortunately).

In this particular case, you can't see it without viewing the source code, but we have an internal developer note for that specific API method to ensure its later retention:

// DO NOT REMOVE EVER. This is a required RFC feature, but marked as deprecated to discourage its use

@lhazlewood
Copy link
Contributor

Oracle NetSuite seems to reject JWTs with single value arrays.

They are in clear violation of the RFCs then. If you have the ability to submit a ticket to them, it could help others. 🙏

@tonedef71
Copy link

@tonedef71 when we release v 1.0, we will remove all deprecated methods except this one and any others mandated as a requirement by the RFCs.
In this particular case, you can't see it without viewing the source code, but we have an internal developer note for that specific API method to ensure its later retention:

// DO NOT REMOVE EVER. This is a required RFC feature, but marked as deprecated to discourage its use

Will JwtBuilder.signWith() also be spared from the great purge of deprecated stuff?

@lhazlewood
Copy link
Contributor

lhazlewood commented Dec 2, 2024

Will JwtBuilder.signWith() also be spared from the great purge of deprecated stuff?

There are two overloaded variants of that method that are not marked as deprecated and will remain, but the other 4 currently marked as deprecated will be removed because they:

  1. Confuse enough people that don't understand that Strings are not themselves keys, which could open them up to security risks, and
  2. Use the old io.jsonwebtoken.SignatureAlgorithm enum which was replaced by the newer io.jsonwebtoken.security.*Algorithm interfaces for custom algorithm support in 0.12.0 and later (0.11.x and earlier could not support custom algorithms, nor overriding the default ones if one necessary).

@tonedef71
Copy link

@lhazlewood Thank you for elaborating further. What does one need to do to get JwtBuilder.signWith() to sign with the PS384 algorithm when provided with an RSA private key?

@lhazlewood
Copy link
Contributor

@tonedef71 .signWith(rsaPrivateKey, Jwts.SIG.PS384)... should do what you're looking for:

Per that method's JavaDoc (which references Jwts.SIG):

image

I hope that helps! 😄

@tonedef71
Copy link

@lhazlewood Thank you, but I should have been more specific. How does one coax the simple method JwtBuilder.signWith(Key) to sign with the PS384 algorithm when provided with an RSA private key? What qualifications does JwtBuilder.signWith(Key) look for in order to autonomously select PS384 as the signing algorithm?

@tonedef71
Copy link

tonedef71 commented Dec 2, 2024

Use the old io.jsonwebtoken.SignatureAlgorithm enum which was replaced

By the way, the old io.jsonwebtoken.SignatureAlgorithm enum is tagged as deprecated. Will it be exempt from future purges of deprecated material?

@lhazlewood
Copy link
Contributor

@tonedef71 those qualifications are documented in the .signWith(Key) method. The PS* algorithms are not auto-selected in any case, because the standard RS* algorithms take precedence for all RSA key sizes:

Pasted_Image_12_2_24__1_14 PM

This precedence was defined long ago in JJWT's history when JDK 8 was the dominant/most-adopted JDK because the PS* algorithms weren't introduced until JDK 11, implying .signWith(rsaPrivateKey) would have thrown an exception for most applications at the time.

This will likely change in future JJWT releases however because RSA PS* algorithms are always better than the RS* ones due to the additional secure-random elements in every PS* signature. So I definitely recommend using the overloaded .signWith(rsaPrivateKey, Jwts.SIG.PS***) variant for now if you're on >= JDK 11 and/or using BouncyCastle.

@lhazlewood
Copy link
Contributor

Use the old io.jsonwebtoken.SignatureAlgorithm enum which was replaced

By the way, the old io.jsonwebtoken.SignatureAlgorithm enum is tagged as deprecated. Will it be exempt from future purges of deprecated material?

No, it will be removed because the newer io.jsonwebtoken.security.*Algorithm variants have replaced the enum concept. They offer better type-safety, they're more flexible, can be overridden in a pinch, and also allow applications to use custom algorithms if desired - the enum didn't allow any of that, so it will be removed fully when releasing 1.0 since there's no benefit to them at all compared to those in the Jwts.SIG.* registry.

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

No branches or pull requests

4 participants