Upgrade to latest JJWT (means we can remove JAXB Api)#10726
Upgrade to latest JJWT (means we can remove JAXB Api)#10726mergify[bot] merged 2 commits intoplayframework:masterfrom
Conversation
| "When parsing SecretConfiguration" should { | ||
| "in DEV mode" should { | ||
| "return 'changeme' when it is configured to it" in { | ||
| "generate a secret when value is configured to 'changeme'" in { |
There was a problem hiding this comment.
This heading was always wrong, it should have always be written like this from the beginning...
| "configured to true" in Helpers.running(_.configure("play.http.session.secure" -> true)) { _ => | ||
| val secretConfiguration = SecretConfiguration() | ||
| val secretConfiguration = | ||
| SecretConfiguration(secret = "vQU@MgnjTohP?w>jpu?X0oqvmz21o[AHP;/rPj?CB><YMFcl?xXfq]6o>1QuNcXU") |
There was a problem hiding this comment.
Making sure the tests use an at least 32 byte long secrets.
| .setAllowedClockSkewSeconds(jwtConfiguration.clockSkew.toSeconds) | ||
| .deserializeJsonWith(new JacksonDeserializer(objectMapper)) | ||
| .build() | ||
| .parseClaimsJws(encodedString) |
There was a problem hiding this comment.
.parser() is deprecated, got replaced by .parserBuilder()....build()....:
https://github.com/jwtk/jjwt/blob/0.11.2/api/src/main/java/io/jsonwebtoken/Jwts.java#L98-L122
| // Even though secretKey already knows about the algorithm we have to pass signatureAlgorithm separately as well again. | ||
| // If not passing it, JJWT would try to determine the algorithm from the secretKey bit length via SignatureAlgorithm.forSigningKey(...) | ||
| // That would be a problem when e.g. in app conf HS256 is set (the default), but the secret has >= 64 bytes, then JJWT would choose HS512. | ||
| builder.signWith(secretKey, signatureAlgorithm).compact() |
There was a problem hiding this comment.
signWith(SignatureAlgorithm alg, String base64EncodedSecretKey) got deprecated as well:
https://github.com/jwtk/jjwt/blob/0.11.2/api/src/main/java/io/jsonwebtoken/JwtBuilder.java#L429-L433
| parseSecret(Mode.Dev, Some("changeme")) must_!= "a test secret which is more than 32 bytes" | ||
| } | ||
| "generate a secret if no secret in dev" in { | ||
| parseSecret(Mode.Dev) must_!= "" |
There was a problem hiding this comment.
This test actually was wrong. Without passing null the config would fall back to the default defined in application:conf: "a test secret..." The test wrongly passed because of course "a test secret..." != "".
Therefore I added checks everywhere for != "a test secret..." to make sure the value is not the fallback from the config.
| // currently jjwt needs the JAXB Api package in JDK 9+ | ||
| // since it actually uses javax/xml/bind/DatatypeConverter | ||
| // See: https://github.com/jwtk/jjwt/issues/317 | ||
| val jaxbApi = "jakarta.xml.bind" % "jakarta.xml.bind-api" % "2.3.3" |
|
|
||
| object JWTCookieDataCodec { | ||
|
|
||
| private val objectMapper: ObjectMapper = new ObjectMapper() |
There was a problem hiding this comment.
I added notes to the migration guide already, I think that is all it's necessary. In the worst case some people might have to update their secrets so they are longer, but IMHO that will hardly be the case, because when using Play's generate/updatesecret task you get a 64 byte secret already, so people already should be safe. |
I may be mixing apples and oranges on my head or I missed some details reviewing the code (very likely) but if I have two nodes running Play I need both nodes to use the same secret, no? So, if my current deployment uses short secrets I will need to stop all running nodes before I start the new deployment with a long secret. Put another way, how can users migrate from short secrets to long secrets? Do we have any docs on the limitations and steps to do it? |
We have https://www.playframework.com/documentation/2.8.x/ApplicationSecret which I updated with this pull request as well and also which explains the requirements of the application secret and how to generate/update it. I even link to this page in the migration notes I added. |
https://www.playframework.com/documentation/2.9.x/ApplicationSecret Note that as atom-workshop currently doesn't _use_ the PLAY_SESSION cookie for anything (mainly because it uses https://github.com/guardian/pan-domain-authentication for authentication) it's probably okay for play.http.secret.key to _not_ be secret (which it isn't, being in this public repo) - but it _does_ need to be long enough to satisfy the Playframework and JJWT: playframework/playframework#10726 If atom-workshop ever _does_ use the PLAY_SESSION cookie, it should adopt https://github.com/guardian/play-secret-rotation to stay secure.
Fixes #10211
Fixes #8446
Two things:
First: Latest jjwt (I think starting with 0.10 already) got much stricter and enforces a minimum length for the secret that is used to sign the cookie, dependent on the algorithm used. If using a smaller secret jjwt will throw an exception now. That's quite a change from the current behaviour, where you could basically pass any secret with any length to jjwt... Therefore, back in Play 2.7, via #8720, Will introduced a minimum length check of the secret at startup, however even this check is insufficient now. Because jjwt requires a 32 bytes secret for Play's default algorithm, #8720 won't help because it just printed warnings if the secret is smaller than fifteen characters / bytes. So in Play 2.7 / 2.8 you could use a 16 byte secret, no problem, however now a 16 byte secret will blow up the app at the moment a Cookie will be en- or decoded... To resolve this I modified the checks at startup to make the app fail fast if an algorithm used requires a longer secret.
Second: If you look at the pull request, there are two commits. Now that jjwt requires at least 32 bytes for the default algorithm, I had to make sure that existing tests have a long enough secret. Of course that means that encoding a cookie results in a different encoded string. Now to make sure my changes are completely backwards compatible, I ran the modifed tests of the first commit against Play's current jjwt implementation (which uses v0.9.1). That passed, which means the old implementation produces the same encoded string as the new implementation (when you fast forward to the second commit).