-
Notifications
You must be signed in to change notification settings - Fork 1
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
Allows to sign using the BLS curve #121
base: main
Are you sure you want to change the base?
Conversation
c990563
to
09147e0
Compare
src/keys.c
Outdated
@@ -87,7 +84,7 @@ cx_err_t generate_public_key(cx_ecfp_public_key_t *public_key, | |||
signature_type_t signature_type = derivation_type_to_signature_type(derivation_type); | |||
cx_curve_t cx_curve = signature_type_to_cx_curve(signature_type); | |||
|
|||
public_key->W_len = ELLIPTIC_CURVE_PUB_KEY_LENGTH; | |||
public_key->W_len = 65; |
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.
Avoid using hard number and replace with named macros
src/keys.c
Outdated
@@ -105,7 +102,7 @@ cx_err_t generate_public_key(cx_ecfp_public_key_t *public_key, | |||
if (cx_curve == CX_CURVE_Ed25519) { | |||
CX_CHECK( | |||
cx_edwards_compress_point_no_throw(CX_CURVE_Ed25519, public_key->W, public_key->W_len)); | |||
public_key->W_len = PUB_KEY_COMPPRESSED_LENGTH; | |||
public_key->W_len = 33; |
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.
No hard numbers , use named macros
src/keys.c
Outdated
@@ -151,7 +148,7 @@ static cx_err_t public_key_hash(uint8_t *const hash_out, | |||
case SIGNATURE_TYPE_SECP256R1: { | |||
memcpy(compressed.W, public_key->W, public_key->W_len); | |||
compressed.W[0] = 0x02 + (public_key->W[64] & 0x01); | |||
compressed.W_len = PUB_KEY_COMPPRESSED_LENGTH; | |||
compressed.W_len = 33; |
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, no hard numbers use named macros
src/keys.h
Outdated
/** | ||
* @brief This structure represents a bip32 path and its curve | ||
* | ||
* Defines a key when associates with a mnemonic seed |
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.
Derivation type is always required. Also where is the key? It only lists derivation type.
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 comment come from type.h
, it is just a copy.
The comment just tells that you can define a key
with a bip32path-&-curve
if you associate it to a mnemonic seed
(for example, using generate_public_key
)
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.
You can fix the grammer like this
Defines the key when associated with a mnemonic seed.
src/crypto.h
Outdated
size_t path_len, | ||
uint8_t raw_pubkey[static 97]); | ||
|
||
#ifndef TARGET_NANOS |
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.
If we cannot sign BLS on nanos then it does not make sense to allow setting up BLS on nanos either. This will add to confusion.
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.
You're right !
I have added it to allow tz4 holders to use the existing features. But it is indeed confusing if they can not sign.
I have removed BLS support for NanoS !
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.
if you can not sign BLS with nanos, how did you get these screenshots, what happens when you press accept.
need to remove the entire nanos folder for tests with BLS too
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 was still testing BLS with NanoS.
I have fixed it by skipping tests using BLS account for NanoS.
c5c8a1c
to
4591406
Compare
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.
Check if eth apps do this or any other app does this? As a last resort we can just ask ledger if saving the private key to RAM is safe or should be avoided .
src/apdu_sign.c
Outdated
cdata->ptr, | ||
cdata->size, | ||
G.final_hash, | ||
sizeof(G.final_hash))); |
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 G.final_hash and sizeof(G.final_hash) is ignored by cx_hash_no_throw when last is false, it would be more readable with an if statement.
if (!last){ cx_hash_update(...., Null, 0); } else{ cx_hash_no_throw(.., CX_LAST, ..., G.final_hash, sizeof..)
#ifndef TARGET_NANOS | ||
// The BLS signature uses its own hash function. | ||
// The entire message must be stored in `G.message`. | ||
if (global.path_with_curve.derivation_type == DERIVATION_TYPE_BLS12_381) { |
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.
if the BLS performs its own hash, can we skip hashing step for BLS in handle_sign above. I think we are hasing twice.
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 can not skip hashing step for BLS in handle_sign
because we have to send it if the user asks it: INS_SIGN_WITH_HASH
.
src/keys.h
Outdated
/** | ||
* @brief This structure represents a bip32 path and its curve | ||
* | ||
* Defines a key when associates with a mnemonic seed |
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.
You can fix the grammer like this
Defines the key when associated with a mnemonic seed.
cx_err_t error = CX_OK; | ||
cx_curve_t curve = CX_CURVE_BLS12_381_G1; | ||
|
||
uint8_t tmp[BLS_SK_LEN * 2u] = {0}; |
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.
Overall it seems we are doing multiple steps for signing a single msg using BLS function everytime:
- hash the message in handle_sign
- derive private key for BLS
- derive a public key for BLS
- Again generate hash for BLS
- sign bls
I think we can optimize by storing BLS pubic and private key when user registers the BLS key as delegate and store it in RAM or when the device starts up.
Then dont hash the msg in handle_sign if the derivation is BLS.
I would also suggest you to check your approach and how it affects benchmarking of existing derivations. I suspect we have increased the time take to sign fo tz2 and tz3 as well.
So when BLS signing requests come we can simply hash and sign.
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.
Actually we can store public and private key for all the derivations in the global ram and compute it when the device starts for the first time. This way we can save some time for all the signatures. I believe we did utilize this approach before.
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 we can optimize by storing BLS pubic and private key when user registers the BLS key as delegate and store it in RAM or when the device starts up.
I'm not sure Legder
would agree to store the private key in the RAM (nor keep it alive while the app is open).
Storing the public key should be possible.
We have to ask Ledger for that.
Then dont hash the msg in handle_sign if the derivation is BLS.
We have to if the user asks for it. I will change that !
I suspect we have increased the time take to sign fo tz2 and tz3 as well.
I'm not sure to understand why.
Actually we can store public and private key for all the derivations in the global ram and compute it when the device starts for the first time.
For others derivations, the public key is not required.
Please have a talk with Ledger !
too avoid manual convertion
- use `cx_ecfp_256_public_key_t` instead of `cx_ecfp_public_key_t` because it will become an abstract type - create an union in order to be able to handle all `cx_ecfp_public_key_t` at the same time. `cx_ecfp_384_public_key_t` will be add later for BLS keys
- alias `cx_ecfp_compressed_public_key_t` to differentiate it from `cx_ecfp_public_key_t` - use customize `cx_ecfp_public_key_t` to fit tezos compressed public key and merge them in a struct `tz_ecfp_compressed_public_key_t`
- use directly derivation_type - inline derivation_mode
- gen pk - sign - parsable in operations
As the BLS signature uses its own hash function
- init hash state at first packet - let cx_hash_no_throw handle the hashing
4591406
to
c4ef534
Compare
Associated
LedgerHQ/ledger-app-database
PR: LedgerHQ/ledger-app-database#279