Issue/350 #357
Issue/350 #357
Conversation
| if err := validatePrivateKeyType(privateKey); err != nil { | ||
| return nil, errors.WithStack(err) | ||
| } | ||
| return &Signer{privateKey: privateKey}, nil |
There was a problem hiding this comment.
return statements should not be cuddled if block has more than two lines (from wsl)
| if pk, ok := e.privateKey.(*secp256k1.PrivateKey); ok { | ||
| prv, err = pk.ECDSA() | ||
| } | ||
| if pk, ok := e.privateKey.(secp256k1.PrivateKey); ok { |
There was a problem hiding this comment.
if statements should only be cuddled with assignments (from wsl)
| } | ||
| return types.SignTx(opts.Tx, types.HomesteadSigner{}, ecdsa) | ||
| return types.SignTx(opts.Tx, types.HomesteadSigner{}, prv) |
There was a problem hiding this comment.
return statements should not be cuddled if block has more than two lines (from wsl)
| wantErr bool | ||
| }{ | ||
| { | ||
| "success-ed25519-sofia", | ||
| "success-ed25519-charlotte", |
| false, | ||
| }, | ||
| { | ||
| "err-secp256k1-sofia", | ||
| "err-secp256k1-charlotte", |
| } | ||
|
|
||
| func validatePrivateKeyType(pk crypto.PrivateKey) ([]byte, error) { | ||
| switch pk := pk.(type) { | ||
| func validatePrivateKeyType(pk crypto.PrivateKey) error { |
There was a problem hiding this comment.
I your instinct to trying and simplify this. Let me give you a bit more information on why this method returns the []byte because its not clear from the code. pk.Bytes() returns the private key bytes, however this method has multiple uses (which is a bad thing!) its used for storing the private key in the keystore, decrypting and signing private keys. This needs to be resolved in the long term and I'll create an issue around that but there is a bit of investigation needed, I''ll raise it and would be great to get your comments. This method then is responsible for ensuring the key is of a valid type but also extracting the bytes that are relevant for decryption. If [32:] is moved when calling easyOpen you have then it is not aware of what type of key it is and it may pick the incorrect bytes.
There was a problem hiding this comment.
I see. Thanks for the clarification
| } | ||
|
|
||
| return easyOpen(data, privKeyBytes) | ||
| return easyOpen(data, d.privateKey.Bytes()[32:]) |
There was a problem hiding this comment.
Different keys may have different ranges for returning the bytes to use when decrypting. If [32:] is done here you have then it is not aware of what type of key it is and it may pick the incorrect bytes.
| @@ -31,8 +32,11 @@ type SignerOptions struct { | |||
| } | |||
|
|
|||
| // NewSigner returns a new ethereum signer that can be used to sign transactions. | |||
| func NewSigner(privateKey crypto.PrivateKey) Signer { | |||
| return Signer{privateKey: privateKey} | |||
| func NewSigner(privateKey crypto.PrivateKey) (*Signer, error) { | |||
| @@ -26,7 +26,7 @@ import ( | |||
| func Signer(protocol string, pk crypto.PrivateKey) (signer.Signer, error) { | |||
| switch protocol { | |||
| case protocols.Ethereum: | |||
| return ethereum.NewSigner(pk), nil | |||
| return ethereum.NewSigner(pk) | |||
There was a problem hiding this comment.
Thanks for the feedback!
I'm a little bit confused with issues #350 and #331, they appear same to me, with the only difference being that in the description of #350 that the public key type needs to be ed25519 and in #331 secp256k1
Also the title of #350 says to ensure valid keys are used for nacl.Decrypter isn't that already done here ?
There was a problem hiding this comment.
Sorry for the lack of clarity @Despire let me add a bit more information.
- Ensure only valid keys are used to create nacl.Decrypter #350 is related encryption which is used to encrypt / decrypt messages this change is in the
naclpackage. - Check private key is valid before returning signer #331 is related to the signing transactions, this change is in the
ethereumpackage.
In the case of signing a transaction and decrypting a message the private key is required. The code that you are implementing is very similar with the main difference being using secp256k1 or ed25519.
No here is the part that's made it very confusing for which I am very sorry for! You are absolutely correct the link does make that issue redundant. I will instead open an issue for nacl.NewEncrypter() #359
There was a problem hiding this comment.
Again thanks for the clarification 😄
I guess this PR can be closed along with issue #350.
This should fix the creation of signer with an invalid key type as described in #350 . I believe that
ed25519in the description was supposed to besecp256k1.Also simplified
nalc.Decryptoras it was doing an unnecessary second key validation indecryptor.Decrypt