ENT-11101: Fix all crypto issues introduced by Java 17 upgrade #7675
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
d6127e4 to
5c5361d
Compare
5c5361d to
518d4a4
Compare
|
This supersedes #7600. |
f49fd67 to
e390b16
Compare
shamsasari
commented
Mar 1, 2024
Comment on lines
+142
to
+147
| // The default signature scheme, EDDSA_ED25519_SHA512, generates public keys which have a writeReplace method (EdDSAPublicKeyImpl). | ||
| // This is picked up by Quasar's custom ReplaceableObjectKryo, which will *always* use the writeReplace for serialisation, ignoring | ||
| // any Kryo serialisers that might have been configured. This thus means the deserialisation path does not go via | ||
| // Cryto.decodePublicKey, which interns the materialised PublicKey. To avoid all of this, and test the last assertion, we use | ||
| // ECDSA keys, whose implementation doesn't have a writeReplace method. | ||
| val keyPair = Crypto.generateKeyPair(Crypto.ECDSA_SECP256R1_SHA256) |
There was a problem hiding this comment.
There was a problem hiding this comment.
That's unfortunate, but not a blocker. Will have to have a think about how we might work around it. I suppose a wrapper public key class of our own making would work (we might have to unwrap on entry to some of the crypto methods). Or we could poke something into FORBIDDEN_CLASSES in ReplaceObjectKryo - which is ugly - and/or try and make that extensible in our Quasar fork.
adelel1
reviewed
Mar 4, 2024
adelel1
reviewed
Mar 4, 2024
core/src/main/kotlin/net/corda/core/crypto/internal/Secp256k1SupportProvider.kt
Outdated
Show resolved
Hide resolved
adelel1
reviewed
Mar 4, 2024
core/src/main/kotlin/net/corda/core/crypto/internal/Secp256k1SupportProvider.kt
Outdated
Show resolved
Hide resolved
adelel1
reviewed
Mar 4, 2024
core/src/main/kotlin/net/corda/core/crypto/internal/Secp256k1SupportProvider.kt
Outdated
Show resolved
Hide resolved
The various crypto tests that were previously ignored have been re-enabled. The abandoned i2p EdDSA library has been replaced with native support that was added in Java 15. Java 17 (via the `SunEC` provider) does not support the secp256k1 curve (one of the two ECDSA curves supported in Corda). This would not normally have been an issue as secp256k1 is already taken care of by Bouncy Castle. However, this only works if the `Crypto` API is used or if `”BC”` is explicitly specified as the provider (e.g. `Signature.getInstance(“SHA256withECDSA”, “BC”)`). If no provider is specified, which is what is more common, and actually what the Java docs recommend, then this doesn’t work as the `SunEC` provider is selected. To resolve this, a custom provider was created, installed just in front of `SunEC`, which “augments” `SunEC` by delegating to Bouncy Castle if keys or parameters for secp256k1 are encountered. `X509Utilities.createCertificate` now calls `X509Certificate.verify()` to verify the created certificate, rather than using the Bouncy Castle API. This is more representative of how certificates will be verified (e.g. during SSL handshake) and weeds out other issues (such as unsupported curve error for secp256k1). `BCCryptoService` has been renamed to `DefaultCryptoService` as it no longer explicitly uses Bouncy Castle but rather uses the installed security providers. This was done to fix a failing test. Further, `BCCryptoService` was already relying on the installed providers in some places. The hack to get Corda `SecureRandom` working was also resolved. Also, as an added bonus, tests which ignored `SPHINCS256_SHA256` have been reinstated. Note, there is a slightly inconsistency between how EdDSA and ECDSA keys are handled (and also RSA). For the later, Bouncy Castle is preferred, and methods such as `toSupportedKey*` will convert any JDK class to Bouncy Castle. For EdDSA the preference is the JDK (`SunEC`). However, this is simply a continuation of the previous preference of the i2p library over Bouncy Castle.
e390b16 to
0091807
Compare
adelel1
approved these changes
Mar 4, 2024
rick-r3
approved these changes
Mar 5, 2024
|
I guess we have yet to see what the performance of the JDK implementation is like. |
This was referenced Mar 6, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The various crypto tests that were previously ignored have been re-enabled.
The abandoned i2p EdDSA library has been replaced with native support that was added in Java 15.
Java 17 (via the
SunECprovider) does not support the secp256k1 curve (one of the two ECDSA curves supported in Corda). This would not normally have been an issue as secp256k1 is already taken care of by Bouncy Castle. However, this only works if theCryptoAPI is used or if”BC”is explicitly specified as the provider (e.g.Signature.getInstance(“SHA256withECDSA”, “BC”)). If no provider is specified, which is what is more common, and actually what the Java docs recommend, then this doesn’t work as theSunECprovider is selected. To resolve this, a custom provider was created, installed just in front ofSunEC, which “augments”SunECby delegating to Bouncy Castle if keys or parameters for secp256k1 are encountered.X509Utilities.createCertificatenow callsX509Certificate.verify()to verify the created certificate, rather than using the Bouncy Castle API. This is more representative of how certificates will be verified (e.g. during SSL handshake) and weeds out other issues (such as unsupported curve error for secp256k1).BCCryptoServicehas been renamed toDefaultCryptoServiceas it no longer explicitly uses Bouncy Castle but rather uses the installed security providers. This was done to fix a failing test.BCCryptoServicewas already relying on the installed providers in some places anyway.The hack to get Corda
SecureRandomworking was also resolved. Also, as an added bonus, tests which ignoredSPHINCS256_SHA256have been reinstated.Note, there is a slight inconsistency between how EdDSA and ECDSA keys are handled (and also RSA). For the later, Bouncy Castle is preferred, and methods such as
toSupportedKey*will convert any JDK class to Bouncy Castle. For EdDSA the preference is the JDK (SunEC). However, this is simply a continuation of the previous preference of the i2p library over Bouncy Castle.