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

crypto.ecdsa: expand ecdsa module to support another curve. #23407

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

blackshirt
Copy link
Contributor

@blackshirt blackshirt commented Jan 8, 2025

There are lack of functionality commonly
needed in the crypto.ecdsa module.
Some of them are

  • Currently, only support for NIST P-256 curve, we need more curve on the stock.
  • Current ecdaa.PrivateKey.sign(message) does not work on hash (digest) of the message, but its work on message directly.. Its not recommended working on signing messages directly.

This PR address some of them issue, by adding and expand support for another popular ecc curve, ie,

  • Add support for NIST P-384, ie, secp384r1 curve
  • Add support for NIST P-521, ie, secp521r1 curve
  • Add support for famous |secp256k1 curve used in bitcoin ecosystem.

This PR also add a routine to sign message with options to use irecommended hash function or custom hash (maybe its can be unified with the current sign routine)

This PR also add a routine to load a public key part from bytes array, commonly parsed from pem-formatted public key file.

This PR was a first time i was working with c wrapping, so, i'm totally noob in the sense, please give its a review.

Huly®: V_0.6-21838

@JalonSolov
Copy link
Contributor

Hmm... if this is tied directly to openssl, that's not a good thing. V ships with mbedtls, so it should work with that (if it can). Otherwise, there is a command line option to tell V to work with openssl, so that option should be checked, rather than hard-coding openssl support.

@blackshirt
Copy link
Contributor Author

blackshirt commented Jan 8, 2025

Hmm... if this is tied directly to openssl, that's not a good thing. V ships with mbedtls, so it should work with that (if it can). Otherwise, there is a command line option to tell V to work with openssl, so that option should be checked, rather than hard-coding openssl support.

The current ecdsa module from @medvednikov commit already wraps ecdsa functionality from openssl with only support for prime256v1 curve, so this PR only expand it a bits..
I dont know too much about mbedtls, and have no opinion on this

@JalonSolov
Copy link
Contributor

Ugh. I missed that in ecdsa, then. That should be fixed, too. Hard-coding to wrap openssl can lead to too many problems, especially on Windows (or any other os/distro) which doesn't have it installed by default.

@medvednikov
Copy link
Member

@JalonSolov that's a decision I made after lots of research.

Writing it in pure V was not an option, can't translate from Go due to assembly usage. mbedtls is 2.5 times slower, and this needs critical performance.

@medvednikov
Copy link
Member

@blackshirt

FAIL  [  77/2246] C:  1158.6 ms, R:     0.000 ms vlib/crypto/ecdsa/util_test.v
>> compilation failed:
================== C compilation error (from clang): ==============
cc:                                                    ^
Error: cc: /tmp/v_501/tsession_1f0298f80_01JH2SACHN9Y0H0WW2YCYSR2E0/util_test.01JH2SBPCB3YX6DRV2MWYKYYZF.tmp.c:10829:12: error: call to undeclared function 'd2i_PUBKEY'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
cc:         pub_key = d2i_PUBKEY(&pub_key, &bytes.data, bytes.len);
cc:                   ^
Error: cc: /tmp/v_501/tsession_1f0298f80_01JH2SACHN9Y0H0WW2YCYSR2E0/util_test.01JH2SBPCB3YX6DRV2MWYKYYZF.tmp.c:10829:10: error: incompatible integer to pointer conversion assigning to 'EVP_PKEY *' (aka 'struct evp_pkey_st *') from 'int' [-Wint-conversion]
cc:         pub_key = d2i_PUBKEY(&pub_key, &bytes.data, bytes.len);
cc:                 ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Warning: cc: /tmp/v_501/tsession_1f0298f80_01JH2SACHN9Y0H0WW2YCYSR2E0/util_test.01JH2SBPCB3YX6DRV2MWYKYYZF.tmp.c:10839:18: warning: 'EVP_PKEY_get1_EC_KEY' is deprecated [-Wdeprecated-declarations]
cc:         EC_KEY* eckey = EVP_PKEY_get1_EC_KEY(pub_key);
cc:                         ^

@blackshirt
Copy link
Contributor Author

@blackshirt

FAIL  [  77/2246] C:  1158.6 ms, R:     0.000 ms vlib/crypto/ecdsa/util_test.v
>> compilation failed:
================== C compilation error (from clang): ==============
cc:                                                    ^
Error: cc: /tmp/v_501/tsession_1f0298f80_01JH2SACHN9Y0H0WW2YCYSR2E0/util_test.01JH2SBPCB3YX6DRV2MWYKYYZF.tmp.c:10829:12: error: call to undeclared function 'd2i_PUBKEY'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
cc:         pub_key = d2i_PUBKEY(&pub_key, &bytes.data, bytes.len);
cc:                   ^
Error: cc: /tmp/v_501/tsession_1f0298f80_01JH2SACHN9Y0H0WW2YCYSR2E0/util_test.01JH2SBPCB3YX6DRV2MWYKYYZF.tmp.c:10829:10: error: incompatible integer to pointer conversion assigning to 'EVP_PKEY *' (aka 'struct evp_pkey_st *') from 'int' [-Wint-conversion]
cc:         pub_key = d2i_PUBKEY(&pub_key, &bytes.data, bytes.len);
cc:                 ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Warning: cc: /tmp/v_501/tsession_1f0298f80_01JH2SACHN9Y0H0WW2YCYSR2E0/util_test.01JH2SBPCB3YX6DRV2MWYKYYZF.tmp.c:10839:18: warning: 'EVP_PKEY_get1_EC_KEY' is deprecated [-Wdeprecated-declarations]
cc:         EC_KEY* eckey = EVP_PKEY_get1_EC_KEY(pub_key);
cc:                         ^

I would check it and try to resolve on next iteration

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants