-
Notifications
You must be signed in to change notification settings - Fork 27
Add secp256k1 & brainpool EC curves #60
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
Conversation
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.
Thanks for the initiative and PR!
Are those added curves (brainpool) supported by both openssl and JDK (with BC and with default) providers?
It would be nice to add tests for them.
Additionally, you will need to run apiDump
task
secp256k1:
brainpoolP256r1:
|
Nice!
|
Hey @waltkb, are you planning to finalize PR based on the comment above, or could I finish it by myself? |
Hi, is it fine like this now? |
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.
Nice!
Some small remarks and we are good to go!
|
||
generateDigests { digest, _ -> | ||
if (!supportsDigest(digest)) return@generateDigests | ||
if (!supportsDigest(digest)) { | ||
println("Skipping digest $digest for curve ${curve.name}") |
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.
Could you please remove all such println
?
It's possible to enable more logging for supports*
and executes algorithms via flipping enabled
flag here
In case you think that the logging is necessary, please use logger.log
, as there could be problems with excessive logging on CI if enabled :(
assertEquals( | ||
rawSignatureSize, | ||
sigEmpty.size, | ||
"RAW signature size mismatch for empty data on ${curve.name} / ${digest.name}" |
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.
Nice! Thanks for adding those!
@@ -87,27 +149,41 @@ abstract class EcdsaTest(provider: CryptographyProvider) : AlgorithmTest<ECDSA>( | |||
|
|||
@Test | |||
fun testFunctions() = testWithAlgorithm { | |||
if (!supportsFunctions()) return@testWithAlgorithm | |||
if (!supportsFunctions()) { | |||
println("Skipping function test because functions are not supported by provider") |
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.
same here about println
(in whole file)
@@ -24,40 +24,94 @@ abstract class EcdsaTest(provider: CryptographyProvider) : AlgorithmTest<ECDSA>( | |||
data class EcdsaSize( | |||
val curve: EC.Curve, | |||
val rawSignatureSize: Int, | |||
val derSignatureSizes: List<Int>, | |||
val derSignatureSizes: IntRange, |
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.
nice idea!
Hi there! Thanks for the work on adding the BP curves. I made a patch replacing the println statements. As the merge request has conflicts I also performed a rebase against main. I can issue a merge request against the walt repo if that helps. |
Merged via #78 |
No description provided.