Support specifying the Cryptography Provider by name and instance

aps-augentictech commented 2 years ago

Changes

Treble all sign and verify methods to accept the provider name or a java.security.Provider instance as it provides more flexibility.

The overloads that take the provider name check if it is loaded into Security and call the overload with the provider instance from Security. If it has not been added, they throw a NoSuchProviderException;
Old methods without provider, call the new ones with provider as null;
The methods that take a provider check if null has been passed in. If null, they call the getInstance only with the algorithm as parameter, which will use the lowest ranked provider in Security. This was the default behaviour.

References

support ticket

Testing

Tests pass without a hitch.

[X] Tests were updated to comply with the changes introduced, namely, the exception expected by Mockito in createSignatureFor and verifySignature. This had to be made because the old methods now call the overload with provider argument and mockito does not follow the implementation. It rather returns immediately.

Checklist

[X] I have read the Auth0 general contribution guidelines
[X] I have read the Auth0 Code of Conduct
[X] All existing and new tests complete without errors

teagy commented 2 years ago

Correct me if I'm wrong but this pull request should allow for the provider to be specified along with the algorithm like so:

Example of specifying BouncyCastle FIPS provider along with algorithm assuming the BC jar is on the classpath: Mac mac = Mac.getInstance(“HmacSHA256, "BCFIPS");

aps-augentictech commented 2 years ago

Correct me if I'm wrong but this pull request should allow for the provider to be specified along with the algorithm like so:

Example of specifying BouncyCastle FIPS provider along with algorithm assuming the BC jar is on the classpath: Mac mac = Mac.getInstance(“HmacSHA256, "BCFIPS");

Hi @teagy,

You are correct. But it does not suffice for the BouncyCastle's provider being on the classpath. By giving the name of the provider, Java will still look for it in the installed providers ( java.Security.getProvider(String providerName) ). I also introduced the possibility to give an instance of the Provider. In such case, the instance is created by the caller, sufficing the provider's jar to be in the classpath.

Regards APS

aps-augentictech commented 2 years ago

@teagy The goal here is two-fold:

Support specifying the provider (either by name or instance)
Support using different providers for different actions (sign, verify, MAC)

Regards APS

jimmyjames commented 2 years ago

Hi @aps-dnxt, thanks for the PR! I was traveling last week, but will be looking at this PR this week. Thanks!

jimmyjames commented 2 years ago

👋 @aps-dnxt as I started looking at the changes, I notice that many methods or fields have been moved around. It's hard to decipher exactly what has or hasn't changed in those cases. For the purposes of this PR, would you mind not including any of those types of changes, so we can focus on the actual additions/changes? We can always follow with a separate PR that just reorganizes code if needed.

aps-augentictech commented 2 years ago

Hi @jimmyjames

What I tried to do was to keep the overloads close to the old methods but I get it, it becomes more difficult to read, although easier to write. I can try to move them to the end of the source files if it would make it easier. In a nutshell, what I did was to empty the old sign and verify methods to use the new that takes a Provider parameter. I also included another one that takes the Provider name, gets the Provider instance from Security and uses the new method. I'll try to clean it up. Do you also mean the test cases?

Regards APS

jimmyjames commented 2 years ago

Hi @aps-dnxt, I'm just referring to any changes to the src classes/public APIs. I know it's a bit of ask to reorganize your work, but when I look at the diffs and see fields or methods removed when they're simply moved, it's harder to review. Hope you understand and thanks for your contribution 🙇

Degerada commented 2 years ago

We are dependant on Bouncy Castle because in our spec we are using an Elliptic Curve table for signing JWTs that is not available in SunJCE Provider, so being able to specify Bouncy Castle would be of great help for us!

stale[bot] commented 1 year ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If you have not received a response for our team (apologies for the delay) and this is still a blocker, please reply with additional information or just a ping. Thank you for your contribution! 🙇‍♂️

jimmyjames commented 1 year ago

Closing this PR, but do think the general is a good one, happy to look at a new PR and will put something on our backlog to do this in the future.

aps-augentictech commented 1 year ago

Thanks @jimmyjames. I just couldn't manage to find time to tackle this properly. I will in the future open a new one.

auth0 / java-jwt