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

Investigating issues with ECDSA_SECP256k1 #53

Closed
wants to merge 10 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 9 additions & 5 deletions src/main/kotlin/org/onflow/flow/sdk/crypto/Crypto.kt
Original file line number Diff line number Diff line change
Expand Up @@ -220,13 +220,17 @@ internal class SignerImpl(
if (hashAlgo == HashAlgorithm.KECCAK256 || hashAlgo == HashAlgorithm.KMAC128) {
// Handle Keccak-256 and KMAC128 separately
val hash = hasher.hash(bytes)
val ecdsaSign = Signature.getInstance("NONEwithECDSA")
ecdsaSign.initSign(privateKey.key)
ecdsaSign.update(hash)
signature = ecdsaSign.sign()
val noneEcdsaSign = Signature.getInstance("NONEwithECDSA", "BC")
noneEcdsaSign.initSign(privateKey.key)
noneEcdsaSign.update(hash)
signature = noneEcdsaSign.sign()
} else {
// Handle other algorithms with valid IDs
val ecdsaSign = Signature.getInstance(hashAlgo.id)
val ecdsaSign = if (privateKey.key.algorithm == "EC" && privateKey.key is ECPrivateKey) {
Signature.getInstance("ECDSA", "BC")
} else {
Signature.getInstance(hashAlgo.id)
}
ecdsaSign.initSign(privateKey.key)
ecdsaSign.update(bytes)
signature = ecdsaSign.sign()
Expand Down
3 changes: 1 addition & 2 deletions src/main/kotlin/org/onflow/flow/sdk/models.kt
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,6 @@ enum class FlowChainId(
?: UNKNOWN
}
}

enum class SignatureAlgorithm(
val algorithm: String,
val curve: String,
Expand All @@ -58,7 +57,7 @@ enum class SignatureAlgorithm(
) {
UNKNOWN("unknown", "unknown", "unknown", -1, 0),
ECDSA_P256("ECDSA", "P-256", "ECDSA_P256", 2, 1),
ECDSA_SECP256k1("ECDSA", "secp256k1", "ECDSA_secp256k1", 3, 2);
ECDSA_SECP256k1("ECDSA", "secp256k1", "ECDSA_SECP256k1", 3, 2);

companion object {
@JvmStatic
Expand Down
63 changes: 63 additions & 0 deletions src/test/kotlin/org/onflow/flow/sdk/crypto/CryptoTest.kt
Original file line number Diff line number Diff line change
@@ -1,9 +1,12 @@
package org.onflow.flow.sdk.crypto

import org.bouncycastle.jce.provider.BouncyCastleProvider
import org.onflow.flow.sdk.HashAlgorithm
import org.junit.jupiter.api.Assertions.*
import org.junit.jupiter.api.Test
import org.onflow.flow.sdk.SignatureAlgorithm
import java.math.BigInteger
import java.security.Security
import java.security.Signature

internal class CryptoTest {
Expand Down Expand Up @@ -60,6 +63,13 @@ internal class CryptoTest {
assertNotNull(signer)
}

@Test
fun `Get signer for ECDSA_SECP256k1`() {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

While we are looking at the signer class I think it makes sense to improve the quality of these tests because they follow the same paradigm as the old hasher tests... will have a look at sanity checks Go SDK is currently using

val keyPair = Crypto.generateKeyPair(SignatureAlgorithm.ECDSA_SECP256k1)
val signer = Crypto.getSigner(keyPair.private, HashAlgorithm.SHA3_256)
assertNotNull(signer)
}

@Test
fun `Get hasher`() {
val hasher = Crypto.getHasher()
Expand All @@ -82,6 +92,23 @@ internal class CryptoTest {
assertEquals(expectedLength, normalizedSignature.size)
}

@Test
fun `Test normalizeSignature for ECDSA_SECP256k1`() {
Security.addProvider(BouncyCastleProvider())
val keyPair = Crypto.generateKeyPair(SignatureAlgorithm.ECDSA_SECP256k1)

val ecdsaSign = Signature.getInstance(HashAlgorithm.SHA3_256.id, "BC")
ecdsaSign.initSign(keyPair.private.key)
ecdsaSign.update("test".toByteArray())

val signature = ecdsaSign.sign()

val normalizedSignature = Crypto.normalizeSignature(signature, keyPair.private.ecCoupleComponentSize)

val expectedLength = 2 * keyPair.private.ecCoupleComponentSize
assertEquals(expectedLength, normalizedSignature.size)
}

@Test
fun `Test extractRS`() {
val keyPair = Crypto.generateKeyPair()
Expand All @@ -99,6 +126,24 @@ internal class CryptoTest {
assertTrue(s > BigInteger.ZERO)
}

@Test
fun `Test extractRS for ECDSA_SECP256k1`() {
Security.addProvider(BouncyCastleProvider())
val keyPair = Crypto.generateKeyPair(SignatureAlgorithm.ECDSA_SECP256k1)

val ecdsaSign = Signature.getInstance(HashAlgorithm.SHA3_256.id, "BC")
ecdsaSign.initSign(keyPair.private.key)

ecdsaSign.update("test".toByteArray())

val signature = ecdsaSign.sign()

val (r, s) = Crypto.extractRS(signature)

assertTrue(r > BigInteger.ZERO)
assertTrue(s > BigInteger.ZERO)
}

@Test
fun `Hasher implementation`() {
val hasher = HasherImpl(HashAlgorithm.SHA3_256)
Expand Down Expand Up @@ -239,4 +284,22 @@ internal class CryptoTest {
val signature = signer.sign("test".toByteArray())
assertNotNull(signature)
}

@Test
fun `Signer implementation for ECDSA_SECP256k1`() {
val keyPair = Crypto.generateKeyPair(SignatureAlgorithm.ECDSA_SECP256k1)
val signer = SignerImpl(keyPair.private, HashAlgorithm.SHA3_256)
val signature = signer.sign("test".toByteArray())
assertNotNull(signature)
}

@Test
fun `Signer implementation for ECDSA_SECP256k1 and non-standard hasher`() {
val keyPair = Crypto.generateKeyPair(SignatureAlgorithm.ECDSA_SECP256k1)
val key = "thisKeyIsAtLeast16Bytes".toByteArray()
val hasher = HasherImpl(HashAlgorithm.KMAC128, key)
val signer = SignerImpl(keyPair.private, HashAlgorithm.KMAC128, hasher)
val signature = signer.sign("test".toByteArray())
assertNotNull(signature)
}
}
Loading