Deterministic Base64 Behavior by lhazlewood · Pull Request #341 · jwtk/jjwt

lhazlewood · 2018-07-08T15:14:24Z

Fixes #333

Implemented new Base64 encoder forked from MigBase64 to guarantee deterministic behavior on all JDK and Android platforms
Allowed pluggable Encoder/Decoder for JWT building and parsing via new Encoder and Decoder and JwtBuilder#base64UrlEncodeWith and JwtParser#base64UrlDecodeWith methods respectively
Added tests for all new code to retain 100% code coverage
Enabled oraclejdk10 and openjdk10 builds in TravisCI
Replaced gmaven plugin with gmavenplus to work on JDK >= 9
Upgraded surefire and failsafe plugins to 2.22.0 to ensure build works on JDK >= 10
Ensured JavaDoc linter wouldn't fail the build for JDK >= 8 (was previously only 1.8)

lhazlewood · 2018-07-08T15:17:35Z

@RyanBard and @azagniotov - I'd appreciate a code review if you get a chance!

lhazlewood · 2018-07-08T15:22:44Z

CHANGELOG.md

 ```

-This will set a new `calg` header with the name of the compression algorithm used so that parsers can see that value and decompress accordingly.
+This will set a new `zip` header with the name of the compression algorithm used so that parsers can see that value and decompress accordingly.


I know calg --> zip changes (to reflect the actual JWT specification header name value) aren't Base64 related, but I fixed these during my work on base64 and I forgot to represent it as another commit (and I've already rebased). These changes are just JavaDoc changes however - not code related.

coveralls · 2018-07-08T15:28:01Z

Coverage remained the same at 100.0% when pulling 6e1415c on issue-333-base64 into 130a841 on master.

RyanBard

Some tests to show the padding (the RFC test vectors) and some tests to show the url-safe encode/decode (different output from the normal encode/decode) would be nice.

(assuming these aren't already covered outside of the diff in this PR)

RyanBard · 2018-07-08T16:39:22Z

src/test/groovy/io/jsonwebtoken/codec/impl/Base64DecoderTest.groovy

+
+    @Test
+    void testDecode() {
+        String encoded = 'SGVsbG8g5LiW55WM' // Hello 世界


I would add in the the RFC test vectors also (I'm not sure where the best place to put them would be among the various encode/decode tests in this PR): https://tools.ietf.org/html/rfc4648#page-12

BASE64("") = "" BASE64("f") = "Zg==" BASE64("fo") = "Zm8=" BASE64("foo") = "Zm9v" BASE64("foob") = "Zm9vYg==" BASE64("fooba") = "Zm9vYmE=" BASE64("foobar") = "Zm9vYmFy"

It would also be nice to cook up some examples to distinguish the differences between the url-safe one also.

Code coverage reports 100% of Base64's implementation covered, for all statements and branches.

That said, definitely happy to add as many test vectors as we can reliably trust :)

I wrote a quick program to generate printable ascii chars (it's a decent start at least, and we already have at least a few unicodes in the existing tests):

"special: [\n \t], ascii[32..126]: [ !\"#$%&'()*+,-./0123456789:;<=>?@ABCDEFGHIJKLMNOPQRSTUVWXYZ[\\]^_`abcdefghijklmnopqrstuvwxyz{|}~]\n"

I manually tinkered with it a little, so if my base64 encoding is off a little, it's probably me. We should also dump a \r in there (I don't trust myself making a \r in vim and didn't write this out to a file in code like I probably should have).

Linux's base64 of it:

c3BlY2lhbDogWwogXHRdLCBhc2NpaVszMi4uMTI2XTogWyAhIiMkJSYnKCkqKywtLi8wMTIzNDU2Nzg5Ojs8PT4/QEFCQ0RFRkdISUpLTE1OT1BRUlNUVVZXWFlaW1xdXl9gYWJjZGVmZ2hpamtsbW5vcHFyc3R1dnd4eXp7fH1+XQo=

(again, subtle details might be wrong, so verify independent of my manual tinkering -- manual tinkering being where I added slashes b/c I printed the combination of ascii rather than writing to a file)

I'll write it to a file properly in a bit (going to eat lunch rq).

@RyanBard I added the suggested test vectors (and duplicated them for Base64Url too). PR has been updated in the Base64Test groovy class.

I'm reasonably confident in this one also:

input: "special: [\r\n \t], ascii[32..126]: [ !\"#$%&'()*+,-./0123456789:;<=>?@ABCDEFGHIJKLMNOPQRSTUVWXYZ[\\]^_`abcdefghijklmnopqrstuvwxyz{|}~]\n" encoded: "c3BlY2lhbDogWw0KIAldLCBhc2NpaVszMi4uMTI2XTogWyAhIiMkJSYnKCkqKywtLi8wMTIzNDU2Nzg5Ojs8PT4/QEFCQ0RFRkdISUpLTE1OT1BRUlNUVVZXWFlaW1xdXl9gYWJjZGVmZ2hpamtsbW5vcHFyc3R1dnd4eXp7fH1+XQo="

I wrote that string to a file and used Linux's base64 util to encode (and then verified the decoding of that encoded str was my original output).

@RyanBard in the block directly above, is everything to the right of input: Java code? i.e. I can just copy-and-paste from that into a .java source code file?

@lhazlewood

They were just string literals to dump into an assert, but since your tests are groovy and double quotes are GStrings, it required a little modification (and some replaces for the base64url).

I've PR'd into your branch here: #342

Scavenge what you need from it.

RyanBard · 2018-07-08T16:56:57Z

src/main/java/io/jsonwebtoken/JwtBuilder.java

     * <p>This is a convenience method: the string argument is first BASE64-decoded to a byte array and this resulting
     * byte array is used to invoke {@link #signWith(SignatureAlgorithm, byte[])}.</p>
     *
+     * <h4>Deprecation Notice: Deprecated as of 0.10.0, will be removed in 1.0.0</h4>


I notice the deprecation comments say 0.10.0, but this PR seems to be targeting 0.9.1-SNAPSHOT.

If this isn't a mistake, do you mind giving me a brief explanation of your branching/release process (high level enough that it's no more than 2-3 min of your time)?

Ah, the PR targets master and master should be at 0.10.0-SNAPSHOT not at 0.9.1-SNAPSHOT as it currently is. I need to change that - thanks for the catch.

Technically the deprecation notices themselves can target any patch release (0.9.1, 0.9.2, etc) because they're just JavaDoc changes and the signatures of all non-internal/private API classes/methods are not changing. That said, it was too much of a hassle to create a separate PR and release just for that so I'm bundling those notifications in the 0.10.0 release.

The other changes associated with this PR have to be 0.10.0 because semver requires a minor revision increase when new public APIs are introduced (e.g. Encoder, Decoder, new public methods on JwtBuilder and JwtParser etc).

So, based on semver, the way I manage all my git repos is:

master always reflects the next minor release baseline (i.e. 'SNAPSHOT' in maven terminology) - 0.10.0-SNAPSHOT, 0.11.0-SNAPSHOT, 1.0.0-SNAPSHOT, etc) and contains all PRs/fixes/whatever pending the Maj.min.0 release.

If a bugfix or patch release is warranted for any Maj.min.0 release, a branch is created from the Maj.min.0 release tag with a Maj.min.x naming scheme (suffix is the actual string literal .x)

For example, if 0.9.0 has been released, and we need to get a patch out (e.g. upgrading Jackson as I did earlier this week), I create a 0.9.x branch from the 0.9.0 release tag. and that branch reflects 0.9.1-SNAPSHOT and contains merged PRs awaiting release.

When 0.9.1 is about to be released, the pending changes also get merged to master (to ensure they're always in all future Maj.min releases). Then 0.9.1 is released and then the 0.9.x branch reflects 0.9.2-SNAPSHOT in its pom.xml.

At this point we can either keep the 0.9.x branch around if we think there might be further patch releases (0.9.2, 0.9.3, etc), or just delete it. If we delete it and a 0.9.2 release is warranted, we would re-create the 0.9.x branch based on the 0.9.1 tag.

I tend to prefer to delete .x branches after a patch release to keep things clean unless I know there is an immediate need for another follow-up patch very soon.

So in summary:

master always reflects the staged changes about to go out in the next Maj.min.0 release and should always reflect Maj.min.0-SNAPSHOT in its pom.xml.

Maj.min.x branches (suffixed with the string literal 'x') always reflect staged changes about to go out in the next Maj.min.patch release and should always reflect Maj.min.patch-SNAPSHOT in its pom.xml.

After cutting a Maj.min.patch release, those changes should also be merged into master (except for pom.xml changes that violate # 1).

I hope this helps! Does that make sense?

I just converted the previous reply to a wiki entry: https://github.com/jwtk/jjwt/wiki/Release-Code-Management for future reference.

azagniotov · 2018-07-08T17:49:22Z

src/main/java/io/jsonwebtoken/impl/DefaultJwtBuilder.java

-        String base64UrlEncodedBody;
+        byte[] bytes;
+        try {
+            bytes = this.payload != null ? payload.getBytes(Strings.UTF_8) : toJson(claims);


Should you also trim the this.payload and check if it is not empty before getting the bytes?

Not really? I mean, leading and trailing space could be a valid payload, and we don't want to sanitize people's payloads. That said, this code isn't new - it's the same as it was before, just moved up a couple of lines, so I wouldn't want to change it unless there was a strong reason (e.g. in the RFC) to do so. Or at the very least open a new issue to track it and only do it on a 1.0.0 release.

Alright, just wanted to confirm 👍

azagniotov · 2018-07-08T18:02:06Z

I think the code changes look fine. I agree with @RyanBard to have more test vectors in place

…eterministic behavior on all JDK and Android platforms - Allowed pluggable Encoder/Decoder for JWT building and parsing via new Encoder/Decoder and JwtBuilder#base64UrlEncodeWith and JwtParser#base64UrlDecodeWith methods respectively - added RFC 4648 Base64 test vectors per code review - Added tests for all new code to retain 100% code coverage, verified by Clover and Coveralls - Enabled oraclejdk10 and openjdk10 builds in TravisCI - Replaced gmaven plugin with gmavenplus to work on JDK >= 9 - Upgraded surefire and failsafe plugins to 2.22.0 to ensure build works on JDK >= 10 - Ensured JavaDoc linter wouldn't fail the build for JDK >= 8 (was previously only 1.8) - Updated changelog doc to reflect new Base64 functionality

lhazlewood · 2018-07-09T02:20:07Z

Merged - many thanks to you @RyanBard and @azagniotov for your help and review on this one!

* `signWith()` & `setSigningKey()` now prefer to be handed a `Key` type rather than a base64-encoded String - deprecated in jwtk/jjwt#341 * To obtain a configured `JwtParser`, a builder is now preferred, ie `Jwts.parserBuilder().setSigningKey(key).build()` rather than `Jwts.parser().setSigningKey(key)` - deprecated in jwtk/jjwt#486

* `signWith()` & `setSigningKey()` now prefer to be handed a `Key` type rather than a base64-encoded String - deprecated in jwtk/jjwt#341 * To obtain a configured `JwtParser`, a builder is now preferred, ie `Jwts.parserBuilder().setSigningKey(key).build()` rather than `Jwts.parser().setSigningKey(key)` - deprecated in jwtk/jjwt#486 Note that `play-googleauth` currently supports two versions of Play: * Play v2.9, uses jjwt v0.11.5 * Play v3.0, uses jjwt v0.12.6 ...we need to support both, but thankfully everything we're using is supported in both versions of jjwt.

* `signWith()` & `setSigningKey()` now prefer to be handed a `Key` type rather than a base64-encoded String - deprecated in jwtk/jjwt#341 * To obtain a configured `JwtParser`, a builder is now preferred, ie `Jwts.parserBuilder().setSigningKey(key).build()` rather than `Jwts.parser().setSigningKey(key)` - deprecated in jwtk/jjwt#486 * Use `io.jsonwebtoken.security.SignatureException` rather than `io.jsonwebtoken.SignatureException` - deprecated in jwtk/jjwt#367 Note that `play-googleauth` currently supports two versions of Play: * Play v2.9, uses jjwt v0.11.5 * Play v3.0, uses jjwt v0.12.6 ...we need to support both, but thankfully everything we're using is supported in both versions of jjwt.

lhazlewood requested a review from dogeared July 8, 2018 15:14

lhazlewood commented Jul 8, 2018

View reviewed changes

lhazlewood force-pushed the issue-333-base64 branch from 38e5f55 to a527662 Compare July 8, 2018 16:01

RyanBard suggested changes Jul 8, 2018

View reviewed changes

RyanBard reviewed Jul 8, 2018

View reviewed changes

azagniotov reviewed Jul 8, 2018

View reviewed changes

lhazlewood force-pushed the issue-333-base64 branch from a527662 to 4b9a3ca Compare July 8, 2018 18:04

RyanBard approved these changes Jul 8, 2018

View reviewed changes

lhazlewood force-pushed the issue-333-base64 branch from d61916f to 6e1415c Compare July 9, 2018 00:30

lhazlewood mentioned this pull request Jul 9, 2018

Using Base64 instead of DatatypeConverter #318

Closed

lhazlewood merged commit bae78f0 into master Jul 9, 2018

lhazlewood deleted the issue-333-base64 branch July 9, 2018 02:19

PromanSEW mentioned this pull request Jul 17, 2018

added jaxb-api so that jdk 9+ classpath works without configuration playframework/playframework#8382

Merged

rtyley mentioned this pull request Jul 4, 2024

Avoid deprecated jjwt methods for signing & parsing guardian/play-googleauth#241

Merged

Conversation

lhazlewood commented Jul 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lhazlewood commented Jul 8, 2018

Uh oh!

lhazlewood Jul 8, 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!

coveralls commented Jul 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

RyanBard left a comment

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!

lhazlewood Jul 8, 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!

Choose a reason for hiding this comment

Uh oh!

RyanBard Jul 8, 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!

lhazlewood Jul 8, 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!

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!

azagniotov commented Jul 8, 2018

Uh oh!

lhazlewood commented Jul 9, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

lhazlewood commented Jul 8, 2018 •

edited

Loading

lhazlewood Jul 8, 2018 •

edited

Loading

coveralls commented Jul 8, 2018 •

edited

Loading

lhazlewood Jul 8, 2018 •

edited

Loading

RyanBard Jul 8, 2018 •

edited

Loading

lhazlewood Jul 8, 2018 •

edited

Loading