-
-
Notifications
You must be signed in to change notification settings - Fork 47
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
Add ECDSA over secp256k1 signatures and verification #490
Conversation
constantine/ecdsa/ecdsa.nim
Outdated
## XXX: raise / what else to do if `sysrand` call fails? | ||
doAssert b.limbs.sysrand() |
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.
In case this were to fail, do we want a different failure mode?
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.
For now it's fine not to address this, AFAIK most protocols use RFC6979 to avoid adding the RNG to the attack surface. If the RNG fails to produce data, I'm not even sure what it means at the OS level, no more entropy maybe?
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.
Not sure either to be honest, but given that we do return a bool
I didn't just want to discard it.
constantine/ecdsa/ecdsa.nim
Outdated
## XXX: smarter way for this? | ||
let r = Fr[C].fromBig(rx.toBig()) |
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.
We need to convert the coordinate in Fp
to a scalar in Fr
. The "best" way I could come up with is this to big / from big conversion. Is there a cleaner way?
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.
This is the best generic conversion.
In theory for secp256k1, you could take mod r
because Fp/Fr can be both use canonical representation and still have a fast reduction (#445) however at the moment only Fp uses canonical repr and fast reduction while Fr uses Montgomery representation and Montgomery reduction.
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.
Note that for Ethereum the hash function is Keccak. But it's good to have both and be hash agnostic to help test.
The out-of-place function will need to be replaced by in-place.
You can check constantine/signatures/bls_signatures.nim
in general for API guidance.
Also https://github.com/bitcoin-core/secp256k1 which is the reference implementation for Bitcoin and Ethereum to implement serializer without dynamic allocation.
And https://github.com/status-im/nim-eth/blob/master/eth/common/keys.nim for how secp256k1 is actually used in Ethereum
The PEM/DER part is unused in blockchain, but it's helpful for testing against OpenSSL, and if we ever want to add TLS/QUIC support to Constantine, it wil be necessary.
Regarding testing vs openssl if the API for ECDSA is less messy than hashes, it can be used as a library like so
constantine/tests/t_hash_sha256_vs_openssl.nim
Lines 8 to 54 in 13d5bb7
# Deal with platform mess | |
# -------------------------------------------------------------------- | |
when defined(windows): | |
when sizeof(int) == 8: | |
const DLLSSLName* = "(libssl-1_1-x64|ssleay64|libssl64).dll" | |
else: | |
const DLLSSLName* = "(libssl-1_1|ssleay32|libssl32).dll" | |
else: | |
when defined(macosx) or defined(macos) or defined(ios): | |
const versions = "(.1.1|.38|.39|.41|.43|.44|.45|.46|.47|.48|.10|.1.0.2|.1.0.1|.1.0.0|.0.9.9|.0.9.8|)" | |
else: | |
const versions = "(.1.1|.1.0.2|.1.0.1|.1.0.0|.0.9.9|.0.9.8|.48|.47|.46|.45|.44|.43|.41|.39|.38|.10|)" | |
when defined(macosx) or defined(macos) or defined(ios): | |
const DLLSSLName* = "libssl" & versions & ".dylib" | |
elif defined(genode): | |
const DLLSSLName* = "libssl.lib.so" | |
else: | |
const DLLSSLName* = "libssl.so" & versions | |
# OpenSSL wrapper | |
# -------------------------------------------------------------------- | |
# OpenSSL removed direct use of their SHA256 function. https://github.com/openssl/openssl/commit/4d49b68504cc494e552bce8e0b82ec8b501d5abe | |
# It isn't accessible anymore in Windows CI on Github Action. | |
# But the new API isn't expose on Linux :/ | |
# TODO: fix Windows | |
when not defined(windows): | |
proc SHA256[T: byte|char]( | |
msg: openarray[T], | |
digest: ptr array[32, byte] = nil | |
): ptr array[32, byte] {.noconv, dynlib: DLLSSLName, importc.} | |
# proc EVP_Q_digest[T: byte|char]( | |
# ossl_libctx: pointer, | |
# algoName: cstring, | |
# propq: cstring, | |
# data: openArray[T], | |
# digest: var array[32, byte], | |
# size: ptr uint): int32 {.noconv, dynlib: DLLSSLName, importc.} | |
proc SHA256_OpenSSL[T: byte|char]( | |
digest: var array[32, byte], | |
s: openArray[T]) = | |
discard SHA256(s, digest.addr) | |
# discard EVP_Q_digest(nil, "SHA256", nil, s, digest, nil) |
constantine/ecdsa/ecdsa.nim
Outdated
@@ -0,0 +1,356 @@ | |||
import |
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.
ECDSA should be in the constantine/signatures
folder
constantine/ecdsa/ecdsa.nim
Outdated
@@ -0,0 +1,356 @@ | |||
import | |||
../hashes/h_sha256, |
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 should be hash agnostic, with it's actual instantiation using a predefined hash.
See the function signature of BLS signatures
constantine/constantine/signatures/bls_signatures.nim
Lines 51 to 88 in 13d5bb7
func coreSign*[Sig, SecKey]( | |
signature: var Sig, | |
secretKey: SecKey, | |
message: openArray[byte], | |
H: type CryptoHash, | |
k: static int, | |
augmentation: openArray[byte], | |
domainSepTag: openArray[byte]) {.genCharAPI.} = | |
## Computes a signature for the message from the specified secret key. | |
## | |
## Output: | |
## - `signature` is overwritten with `message` signed with `secretKey` | |
## | |
## Inputs: | |
## - `Hash` a cryptographic hash function. | |
## - `Hash` MAY be a Merkle-Damgaard hash function like SHA-2 | |
## - `Hash` MAY be a sponge-based hash function like SHA-3 or BLAKE2 | |
## - Otherwise, H MUST be a hash function that has been proved | |
## indifferentiable from a random oracle [MRH04] under a reasonable | |
## cryptographic assumption. | |
## - k the security parameter of the suite in bits (for example 128) | |
## - `output`, an elliptic curve point that will be overwritten. | |
## - `augmentation`, an optional augmentation to the message. This will be prepended, | |
## prior to hashing. | |
## This is used for building the "message augmentation" variant of BLS signatures | |
## https://www.ietf.org/archive/id/draft-irtf-cfrg-bls-signature-05.html#section-3.2 | |
## which requires `CoreSign(SK, PK || message)` | |
## and `CoreVerify(PK, PK || message, signature)` | |
## - `message` is the message to hash | |
## - `domainSepTag` is the protocol domain separation tag (DST). | |
type ECP_Jac = EC_ShortW_Jac[Sig.F, Sig.G] | |
var sig {.noInit.}: ECP_Jac | |
H.hashToCurve(k, sig, augmentation, message, domainSepTag) | |
sig.scalarMul(secretKey) | |
signature.affine(sig) |
constantine/ecdsa/ecdsa.nim
Outdated
# For easier readibility, define the curve and generator | ||
# as globals in this file | ||
const C* = Secp256k1 | ||
const G = Secp256k1.getGenerator("G1") |
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.
The signature scheme itself should be generic in constantine/signatures
and concrete implementations should appear in constantine/my_protocol.nim
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.
Yup, fair enough. I wrote it by defining the curve like this so that the code itself is already an (almost) valid generic / static proc. Didn't know if we want to make it fully generic immediately or start with secp256k1 only. Will address it.
constantine/ecdsa/ecdsa.nim
Outdated
var h {.noinit.}: sha256 | ||
h.init() | ||
h.update(message) | ||
h.finish(result) |
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.
This duplicates constantine/hashes.nim
constantine/constantine/hashes.nim
Lines 17 to 54 in 13d5bb7
type | |
CryptoHash* = concept h, var ctx, type H | |
## Interface of a cryptographic hash function | |
## | |
## - digestSizeInBytes is the hash output size in bytes | |
## - internalBlockSize, in bits: | |
## hash functions are supposed to ingest fixed block size | |
## that are padded if necessary | |
## - SHA256 block size is 64 bits | |
## - SHA512 block size is 128 bits | |
## - SHA3-512 block size is 72 bits | |
# should we avoid int to avoid exception? But they are compile-time | |
H.digestSize is static int | |
H.internalBlockSize is static int | |
# Context | |
# ------------------------------------------- | |
ctx.init() | |
ctx.update(openarray[byte]) | |
ctx.finish(var array[H.digestSize, byte]) | |
ctx.clear() | |
func hash*[DigestSize: static int]( | |
HashKind: type CryptoHash, | |
digest: var array[DigestSize, byte], | |
message: openArray[byte], | |
clearMem = false) {.genCharAPI.} = | |
## Produce a digest from a message | |
static: doAssert DigestSize == HashKind.type.digestSize | |
var ctx {.noInit.}: HashKind | |
ctx.init() | |
ctx.update(message) | |
ctx.finish(digest) | |
if clearMem: | |
ctx.clear() |
constantine/ecdsa/ecdsa.nim
Outdated
h.update(message) | ||
h.finish(result) | ||
|
||
proc toBytes(x: Fr[C] | Fp[C]): array[32, byte] = |
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.
proc toBytes(x: Fr[C] | Fp[C]): array[32, byte] = | |
proc toBytes(x: Fr[C] | Fp[C]): array[32, byte] {.noInit.} = |
I assume this is temporary for debugging? Returning big arrays tends to generate bad code:
- Out-of-place functions lead to bad codegen #145
- Internal API: in-place vs result #21
It's also a recommendation in C / C++ to avoid return values that don't fit in registers.
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.
Ah, that either slipped my mind or I just wasn't aware of it! Good to know. Will change it to an in-place variant.
constantine/ecdsa/ecdsa.nim
Outdated
## XXX: smarter way for this? | ||
let r = Fr[C].fromBig(rx.toBig()) |
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.
This is the best generic conversion.
In theory for secp256k1, you could take mod r
because Fp/Fr can be both use canonical representation and still have a fast reduction (#445) however at the moment only Fp uses canonical repr and fast reduction while Fr uses Montgomery representation and Montgomery reduction.
constantine/ecdsa/ecdsa.nim
Outdated
proc getPublicKey*(pk: Fr[C]): EC_ShortW_Aff[Fp[C], G1] = | ||
## Derives the public key from a given private key, | ||
## `privateKey · G` in affine coordinates. | ||
result = (pk * G).getAffine() |
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.
Can reuse
constantine/constantine/signatures/bls_signatures.nim
Lines 37 to 49 in 13d5bb7
func derivePubkey*[Pubkey, SecKey](pubkey: var Pubkey, seckey: SecKey) = | |
## Generates the public key associated with the input secret key. | |
## | |
## The secret key MUST be in range (0, curve order) | |
## 0 is INVALID | |
const Group = Pubkey.G | |
type Field = Pubkey.F | |
const EC = Field.Name | |
var pk {.noInit.}: EC_ShortW_Jac[Field, Group] | |
pk.setGenerator() | |
pk.scalarMul(seckey) | |
pubkey.affine(pk) |
constantine/ecdsa/ecdsa.nim
Outdated
# 6. Verify r_computed equals provided r | ||
result = bool(r_computed == signature.r) | ||
|
||
proc generatePrivateKey*(): Fr[C] = |
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.
This is not needed at this level, except maybe for testing.
Blockchain protocols have special ways to generate a private key usually from a mnemonic or via a HD derivation path (hierarchical deterministic) like BIP32 https://github.com/bitcoin/bips/blob/master/bip-0032.mediawiki or EIP-2333 (https://github.com/mratsim/constantine/blob/13d5bb7/constantine/ethereum_eip2333_bls12381_key_derivation.nim)
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.
Yeah, I assumed having a way to generate a random private key might be useful. For testing we can just move it to a test file of course.
constantine/ecdsa/ecdsa.nim
Outdated
# Combine all parts | ||
let contents = algoId & pubKeyEncoded | ||
result.add(byte(contents.len)) | ||
result.add(contents) |
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.
For constantine/serialization
and without dynamic alloc
constantine/ecdsa/ecdsa.nim
Outdated
# with `h`: message hash as `Fr[C]` (if we didn't use SHA256 w/ 32 byte output | ||
# we'd need to truncate to N bits for N being bits in modulo `n`) | ||
k.inv() | ||
var s = (k * (message_hash + r * privateKey)) |
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 might be faster to do k*privateKey and then a multiscalarmul([k, k*privkey], [message_hash, r]).
Note that the out-of-place functions are for convenience when debugging/testing/developping but not for production code (#413)
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.
I understand. Still hasn't fully sunk in (intuitively) to avoid out-of-place procs! My bad. 😁
Just pushed some more changes for most remaining comments. Still a few things to do. Essentially put serialization into proper submodule, update test cases to use library of OpenSSL and adjust API for specific ECDSA over secp256k1 impl (the latter will be done as part of the test rewrite).
|
Mostly done now, I'd say. Addressed everything I could think of. The public API for ECDSA over secp256k1 could be extended a bit more, I suppose. I wanted to use OpenSSL for PEM file writing as part of the tests to verify our serializers, but ended up giving up after wasting 2 hours or so with either getting segfaults in OpenSSL procedures or only having empty PEM files. So instead for that we still call to an external process to generate PEM files from our private and public keys, sign and verify a message. I'll rebase and fix the conflict of the two files later. I pulled out the OpenSSL wrapper into a single file. Guess the files were updated on master. The two main features lacking are public key recovery and batch verification. |
(We might want to take this out before we merge?)
Adds a test vector generator and a test case (and test vectors) for verification of OpenSSL generated signatures.
Inspired by Bitcoin's implementation: https://github.com/bitcoin-core/secp256k1/blob/f79f46c70386c693ff4e7aef0b9e7923ba284e56/src/ecdsa_impl.h#L171-L193 Essentially, we just have a 72 byte buffer and only fill it up to `len` bytes.
The size is now a static argument of the type, depending on the curve. `DERSigSize` can be used to calculate the size required for the given curve.
Also cleans up the imports of the ECDSA file and adds the copyright header
…rrays Written for OpenSSL interop for test suite (to be moved to serialization with the other related procs).
b8c972e
to
2693aec
Compare
Just rebased and force pushed the changes to fix the merge conflict. |
Not sure what's happening here. Never seen that with any C wrappers.
API not currently available in the OpenSSL version on GH actions
From testing with OpenSSL for hash functions:
but it's fine to skip testing vs OpenSSL on non-linux platforms for 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.
Review part 1.
We will need a followup refactoring on serialization to remove stdlib dependencies and ideally allocation as well.
Part 2 next year.
## Has a `data` buffer of 72 bytes (maximum possible size for | ||
## a signature for `secp256k1`) and `len` of actually used data. | ||
## `data[0 ..< len]` is the actual signature. | ||
DERSignature*[N: static int] = object |
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.
Mmmh, similar to Ecc or Kzg, I would capitalize as Der but DerSignature feels wrong in German xD
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.
Haha, I think we can live with that. ;) We speak perfect German anyway, so no one would confuse 'DerSignature' with 'Der Signatur'. Only the latter would be wrong German after all!
constantine/math/io/io_bigints, | ||
constantine/ecdsa_secp256k1 | ||
|
||
import std / [strutils, base64, math, importutils] |
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.
This needs a follow up issue to remove the dependencies
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.
Yes, as also mentioned in the docstring at the top. I'll open an issue for it!
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.
Issue: #506
proc wrap(s: string, maxLineWidth = 64): string = | ||
## Wrap the given string at `maxLineWidth` over multiple lines | ||
let lines = s.len.ceilDiv maxLineWidth | ||
result = newStringOfCap(s.len + lines) |
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.
In the refactoring issue, we should also remove the allocation.
ECDSA is used in embedded devices and in context where memory cannot be allocated dynamically like trusted enclaves or smart-cards, and I'd like constantine to be alloc-free for those use-cases
if i < lines-1: | ||
result.add "\n" | ||
|
||
proc toPemFile*(publicKey: PublicKey): string = |
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.
This is not a file
# 3. Wrap in begin/end public key template | ||
result = "-----BEGIN PUBLIC KEY-----\n" | ||
result.add der64 & "\n" | ||
result.add "-----END PUBLIC KEY-----\n" |
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 seems like the length of the result is fixed?
So in the refactoring issue for ECDSA serialization, we should replace the string output with a var array
or var openarray
(+ size check)
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.
Yes, the length is fixed (in contrast to DER encoded signatures). The newStringOfCap
was partially out of laziness as the inclusion of the stdlib base64
encoder renders the code anyhow in need of an overhaul. Will add as part of the issue I'll open.
edit: Issue #506
|
||
var pk {.noInit.}: EC_ShortW_Jac[Field, Group] | ||
pk.setGenerator() | ||
pk.scalarMul(seckey) |
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.
I'm pretty sure lattices don't derive public keys the same way ;).
Probably better to rename the file ecc_sig_ops.nim
or something.
to clarify that these are only for EC based signatures.
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.
Review part 2. LGTM
constantine/signatures/ecdsa.nim
Outdated
|
||
result.fromBig(b) | ||
|
||
proc arrayWith[N: static int](res: var array[N, byte], val: byte) = |
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.
proc arrayWith[N: static int](res: var array[N, byte], val: byte) = | |
proc byteArrayWith(N: static int, val: byte): array[N, byte] {.noInit, inline.} = |
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.
This is only used twice and just after initialization. We can avoid bad codegen with {.inline.}
and we can avoid avoid an extra memset(0)
with {.noInit.}
constantine/signatures/ecdsa.nim
Outdated
# Step b: `V = 0x01 0x01 0x01 ... 0x01` | ||
var v: array[N, byte]; v.arrayWith(byte 0x01) | ||
# Step c: `K = 0x00 0x00 0x00 ... 0x00` | ||
var k: array[N, byte]; k.arrayWith(byte 0x00) |
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.
awkward, see change to arrayWith
to do declaration and init.
point1.scalarMul(u1, G) | ||
point2.scalarMul(u2, publicKey) | ||
var R {.noinit.}: EC_ShortW_Jac[Fp[Name], G1] | ||
R.sum(point1, point2) |
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.
As mentioned in #504 (comment) this should be a dedicated proc, which would be subject to optimization in a later PR as it's a bottleneck for blockchain (signatures are verified all the time) and TLS.
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.
Yup. I'll open an issue about these two cases (here and the other PR).
edit: #507
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.
LGTM, merging
## Sign `message` using `secretKey` and store the signature in `sig`. The nonce | ||
## will either be randomly sampled `nsRandom` or deterministically calculated according | ||
## to RFC6979 (`nsRfc6979`) | ||
sig.coreSign(secretKey.raw, message, sha256, nonceSampler) |
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.
I think it's Bitcoin protocol that uses SHA256?
We can rename the file btc_ecdsa_secp256k1.
This adds support for the Elliptic Curve Digital Signature Algorithm (ECDSA) defined over secp256k1.
The current API supports:
base64
encoding)Currently still missing:
public key recovery⇒ will be part of PR AddECRecover
EVM precompile #504In addition, there are a few minor things I'm unsure about. Will highlight them below.