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

Support specifying the Cryptography Provider by name and instance #610

Closed
wants to merge 20 commits into from

Conversation

aps-augentictech
Copy link

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

Testing

Tests pass without a hitch.

  • 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

aps-augentictech and others added 9 commits July 12, 2022 22:29
* No tests were changed and still compile, so the API is unchanged
* ProviderName is only checked ate crypto helper. If null, then the previous code still stands. Otherwise it is used when getting an instance of java.security.Signature
* Hope you find this work useful
…ava.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.
@aps-augentictech aps-augentictech requested a review from a team as a code owner September 26, 2022 04:19
@aps-augentictech aps-augentictech marked this pull request as draft September 26, 2022 04:47
Copy link
Author

@aps-augentictech aps-augentictech left a comment

Choose a reason for hiding this comment

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

it wasn't finished

@aps-augentictech aps-augentictech marked this pull request as ready for review September 26, 2022 05:47
@aps-augentictech aps-augentictech marked this pull request as draft September 26, 2022 05:58
@aps-augentictech aps-augentictech marked this pull request as ready for review September 26, 2022 14:14
@teagy
Copy link

teagy commented Sep 28, 2022

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
Copy link
Author

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
Copy link
Author

@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
Copy link
Contributor

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

@jimmyjames
Copy link
Contributor

👋 @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
Copy link
Author

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
Copy link
Contributor

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
Copy link

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
Copy link

stale bot commented May 21, 2023

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! 🙇‍♂️

@stale stale bot added the closed:stale Issue or PR has not seen activity recently label May 21, 2023
@Widcket Widcket removed the closed:stale Issue or PR has not seen activity recently label Jun 6, 2023
@jimmyjames
Copy link
Contributor

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.

@jimmyjames jimmyjames closed this Aug 7, 2023
@aps-augentictech
Copy link
Author

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

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

Successfully merging this pull request may close these issues.

None yet

5 participants