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

fix: Use RSA-PSS when verifying handshake nonces #65

Merged
merged 1 commit into from
Jul 30, 2020
Merged

Conversation

gnarea
Copy link
Member

@gnarea gnarea commented Jul 28, 2020

No description provided.

@gnarea gnarea self-assigned this Jul 28, 2020
import org.bouncycastle.jce.provider.BouncyCastleProvider

object CryptoUtils {
val BC_PROVIDER = BouncyCastleProvider()
Copy link
Member Author

Choose a reason for hiding this comment

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

@sdsantos, are you aware of any potential performance/stability issues with using the security provider this way vs Security.addProvider()?

I came across https://bugs.openjdk.java.net/browse/JDK-8168469 but presumably that doesn't apply here because this BouncyCastleProvider should only be created once in the lifetime of the process, right?

I wanted to avoid the Security.addProvider() approach because I couldn't think of any simple way to call it in the unit/instrumentation test suites, but I'm happy to change tack if you can think of a way to do it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not familiar with it, but I've had to do something similar for Conscrypt, see App.setupTLSProvider(). It basically runs on App onCreate, which will also run for Instrumentation tests.

Copy link
Member Author

@gnarea gnarea Jul 29, 2020

Choose a reason for hiding this comment

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

Makes sense. I think the tricky bit is making it work in the unit test suite then: I think the only option would be to decorate the tests that directly or indirectly need BC with @ExtendWith(CustomJUnitExtensionThatRegistersBC).

Since we aren't aware of any performance/stability issue, what do you think if we keep the current implementation? We still have to call setProvider(...) in the cases where it matters, the difference is whether we pass the string "BC" or the BouncyCastleProvider instance.

@gnarea gnarea marked this pull request as ready for review July 28, 2020 17:30
@gnarea gnarea added the automerge Allow kodiak to automerge commit when all checks pass label Jul 28, 2020
@gnarea gnarea changed the title fix: Use RSA-PSS when producing/verifying handshake nonces fix: Use RSA-PSS when verifying handshake nonces Jul 28, 2020
gnarea added a commit to relaycorp/awala-poweb-jvm that referenced this pull request Jul 28, 2020
kodiakhq bot pushed a commit to relaycorp/awala-poweb-jvm that referenced this pull request Jul 28, 2020
@gnarea gnarea requested a review from sdsantos July 29, 2020 10:30
@kodiakhq kodiakhq bot merged commit b91f320 into master Jul 30, 2020
@kodiakhq kodiakhq bot deleted the fix-rsa-pss branch July 30, 2020 10:09
@github-actions
Copy link

🎉 This PR is included in version 1.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge Allow kodiak to automerge commit when all checks pass released
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants