-
Notifications
You must be signed in to change notification settings - Fork 930
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
Conversation
* 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.
There was a problem hiding this 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
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: |
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 ( Regards |
@teagy The goal here is two-fold:
Regards |
Hi @aps-dnxt, thanks for the PR! I was traveling last week, but will be looking at this PR this week. Thanks! |
👋 @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. |
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. Regards |
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 🙇 |
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! |
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! 🙇♂️ |
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. |
Thanks @jimmyjames. I just couldn't manage to find time to tackle this properly. I will in the future open a new one. |
Changes
Treble all sign and verify methods to accept the provider name or a java.security.Provider instance as it provides more flexibility.
References
Testing
Tests pass without a hitch.
Checklist