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

Rework ECDSA #212

Merged
merged 2 commits into from
Nov 6, 2017
Merged

Rework ECDSA #212

merged 2 commits into from
Nov 6, 2017

Conversation

lbalmaceda
Copy link
Contributor

@lbalmaceda lbalmaceda commented Oct 23, 2017

This PR fixes a lot of ECDSA Algorithm issues. The problem was mainly that the signature format conversion was being made on one way but not in the other. I've added some tests that mimic the signature format and test the converter methods. What I'm not so sure about is if we should have a different exception message when the total signature length or the R/S numbers length is not the expected. We could throw the same error message "invalid signature length" for all 3 cases. Check lines 104, 113 and 125.

Tests should run against both JDK 7 & JDK 8. I don't know why but tests were failing on JDK8, something we didn't caught because we never run the tests on that JDK. I added a separate PR to use CircleCI Workflows to attempt to run the tests in both JDKs, but because the tests don't pass in master merging that to master will "break master" until this PR is merged. I'd add it sourced from this last commit.

Copy link
Member

@cocojoe cocojoe left a comment

Choose a reason for hiding this comment

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

Tests work on Sun and BC providers now?

@lbalmaceda
Copy link
Contributor Author

Yes. The ECDSABouncyCastleProviderTests is a copy of the ECDSAAlgorithmTests with a BeforeClass and AfterClass that setup the preferred provider. Because we don't hardcode the provider name when requiring one in our logic, it will always pick the preferred one.. which in one test file is the "JDK's default" and in the other one is "bc".

I've added a small test to ensure that the first provider is actually BC.
https://github.com/auth0/java-jwt/pull/212/files#diff-d52011bdc8b4ab49a8ed80b686f1978bR70

I've removed invalid tests as DER signature encoding should NOT be allowed in a JWT and added the corresponding ones to check that it fails on those cases.

Use a copy of the ECDSA tests with BouncyCastle provider
@aaguiarz aaguiarz merged commit 4eb4b62 into master Nov 6, 2017
@lbalmaceda lbalmaceda deleted the rework-ecdsa branch November 6, 2017 15:12
@lbalmaceda lbalmaceda modified the milestones: v3-Next, 3.3.0 Nov 6, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants