-
Notifications
You must be signed in to change notification settings - Fork 21.9k
crypto: add DecompressPubkey, VerifySignature #15615
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
Changes from all commits
c637aca
0a3a970
8de2b8a
c565899
d4e098f
7dac4d3
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -27,10 +27,12 @@ import ( | |
| "github.com/ethereum/go-ethereum/crypto/secp256k1" | ||
| ) | ||
|
|
||
| // Ecrecover returns the uncompressed public key that created the given signature. | ||
| func Ecrecover(hash, sig []byte) ([]byte, error) { | ||
| return secp256k1.RecoverPubkey(hash, sig) | ||
| } | ||
|
|
||
| // SigToPub returns the public key that created the given signature. | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. "SigToPub returns the public key" -> "SigToPub returns the uncompressed public key"
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Compressed/uncompressed doesn't matter for SigToPub because the return value isn't encoded. |
||
| func SigToPub(hash, sig []byte) (*ecdsa.PublicKey, error) { | ||
| s, err := Ecrecover(hash, sig) | ||
| if err != nil { | ||
|
|
@@ -58,6 +60,22 @@ func Sign(hash []byte, prv *ecdsa.PrivateKey) (sig []byte, err error) { | |
| return secp256k1.Sign(hash, seckey) | ||
| } | ||
|
|
||
| // VerifySignature checks that the given public key created signature over hash. | ||
| // The public key should be in compressed (33 bytes) or uncompressed (65 bytes) format. | ||
| // The signature should have the 64 byte [R || S] format. | ||
| func VerifySignature(pubkey, hash, signature []byte) bool { | ||
| return secp256k1.VerifySignature(pubkey, hash, signature) | ||
| } | ||
|
|
||
| // DecompressPubkey parses a public key in the 33-byte compressed format. | ||
| func DecompressPubkey(pubkey []byte) (*ecdsa.PublicKey, error) { | ||
| x, y := secp256k1.DecompressPubkey(pubkey) | ||
| if x == nil { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Design question: Why does the underlying method return
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I decided this way to simplify the interface. There is only one possible error anyway: "invalid".
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You are right though, maybe this should be changed. |
||
| return nil, fmt.Errorf("invalid public key") | ||
| } | ||
| return &ecdsa.PublicKey{X: x, Y: y, Curve: S256()}, nil | ||
| } | ||
|
|
||
| // S256 returns an instance of the secp256k1 curve. | ||
| func S256() elliptic.Curve { | ||
| return secp256k1.S256() | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -21,11 +21,14 @@ package crypto | |
| import ( | ||
| "crypto/ecdsa" | ||
| "crypto/elliptic" | ||
| "errors" | ||
| "fmt" | ||
| "math/big" | ||
|
|
||
| "github.com/btcsuite/btcd/btcec" | ||
| ) | ||
|
|
||
| // Ecrecover returns the uncompressed public key that created the given signature. | ||
| func Ecrecover(hash, sig []byte) ([]byte, error) { | ||
| pub, err := SigToPub(hash, sig) | ||
| if err != nil { | ||
|
|
@@ -35,6 +38,7 @@ func Ecrecover(hash, sig []byte) ([]byte, error) { | |
| return bytes, err | ||
| } | ||
|
|
||
| // SigToPub returns the public key that created the given signature. | ||
| func SigToPub(hash, sig []byte) (*ecdsa.PublicKey, error) { | ||
| // Convert to btcec input format with 'recovery id' v at the beginning. | ||
| btcsig := make([]byte, 65) | ||
|
|
@@ -71,6 +75,33 @@ func Sign(hash []byte, prv *ecdsa.PrivateKey) ([]byte, error) { | |
| return sig, nil | ||
| } | ||
|
|
||
| // VerifySignature checks that the given public key created signature over hash. | ||
| // The public key should be in compressed (33 bytes) or uncompressed (65 bytes) format. | ||
| // The signature should have the 64 byte [R || S] format. | ||
| func VerifySignature(pubkey, hash, signature []byte) bool { | ||
| if len(signature) != 64 { | ||
| return false | ||
| } | ||
| sig := &btcec.Signature{R: new(big.Int).SetBytes(signature[:32]), S: new(big.Int).SetBytes(signature[32:])} | ||
| key, err := btcec.ParsePubKey(pubkey, btcec.S256()) | ||
| if err != nil { | ||
| return false | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Another design question: we're throwing away error-info here, is that really needed?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. btcec is the fallback for platforms that can't link C code. It provides better errors because it's Go code. There is no good way to get those errors from the C version. IMHO it doesn't matter much: the signature is invalid if any of the inputs are.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe more context: I added this because I want a function that does decompression and verification in one call without any pubkey/signature decoding. We get the compressed pubkey in ENR and need to verify that it created the signature. The fastest and simplest way is to just pass these values into libsecp256k1. We could go ahead and make it so |
||
| } | ||
| return sig.Verify(hash, key) | ||
| } | ||
|
|
||
| // DecompressPubkey parses a public key in the 33-byte compressed format. | ||
| func DecompressPubkey(pubkey []byte) (*ecdsa.PublicKey, error) { | ||
| if len(pubkey) != 33 { | ||
| return nil, errors.New("invalid compressed public key length") | ||
| } | ||
| key, err := btcec.ParsePubKey(pubkey, btcec.S256()) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| return key.ToECDSA(), nil | ||
| } | ||
|
|
||
| // S256 returns an instance of the secp256k1 curve. | ||
| func S256() elliptic.Curve { | ||
| return btcec.S256() | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -18,19 +18,95 @@ package crypto | |
|
|
||
| import ( | ||
| "bytes" | ||
| "encoding/hex" | ||
| "testing" | ||
|
|
||
| "github.com/ethereum/go-ethereum/common" | ||
| "github.com/ethereum/go-ethereum/common/hexutil" | ||
| ) | ||
|
|
||
| var ( | ||
| testmsg = hexutil.MustDecode("0xce0677bb30baa8cf067c88db9811f4333d131bf8bcf12fe7065d211dce971008") | ||
| testsig = hexutil.MustDecode("0x90f27b8b488db00b00606796d2987f6a5f59ae62ea05effe84fef5b8b0e549984a691139ad57a3f0b906637673aa2f63d1f55cb1a69199d4009eea23ceaddc9301") | ||
| testpubkey = hexutil.MustDecode("0x04e32df42865e97135acfb65f3bae71bdc86f4d49150ad6a440b6f15878109880a0a2b2667f7e725ceea70c673093bf67663e0312623c8e091b13cf2c0f11ef652") | ||
| testpubkeyc = hexutil.MustDecode("0x02e32df42865e97135acfb65f3bae71bdc86f4d49150ad6a440b6f15878109880a") | ||
| ) | ||
|
|
||
| func TestRecoverSanity(t *testing.T) { | ||
| msg, _ := hex.DecodeString("ce0677bb30baa8cf067c88db9811f4333d131bf8bcf12fe7065d211dce971008") | ||
| sig, _ := hex.DecodeString("90f27b8b488db00b00606796d2987f6a5f59ae62ea05effe84fef5b8b0e549984a691139ad57a3f0b906637673aa2f63d1f55cb1a69199d4009eea23ceaddc9301") | ||
| pubkey1, _ := hex.DecodeString("04e32df42865e97135acfb65f3bae71bdc86f4d49150ad6a440b6f15878109880a0a2b2667f7e725ceea70c673093bf67663e0312623c8e091b13cf2c0f11ef652") | ||
| pubkey2, err := Ecrecover(msg, sig) | ||
| func TestEcrecover(t *testing.T) { | ||
| pubkey, err := Ecrecover(testmsg, testsig) | ||
| if err != nil { | ||
| t.Fatalf("recover error: %s", err) | ||
| } | ||
| if !bytes.Equal(pubkey1, pubkey2) { | ||
| t.Errorf("pubkey mismatch: want: %x have: %x", pubkey1, pubkey2) | ||
| if !bytes.Equal(pubkey, testpubkey) { | ||
| t.Errorf("pubkey mismatch: want: %x have: %x", testpubkey, pubkey) | ||
| } | ||
| } | ||
|
|
||
| func TestVerifySignature(t *testing.T) { | ||
| sig := testsig[:len(testsig)-1] // remove recovery id | ||
| if !VerifySignature(testpubkey, testmsg, sig) { | ||
| t.Errorf("can't verify signature with uncompressed key") | ||
| } | ||
| if !VerifySignature(testpubkeyc, testmsg, sig) { | ||
| t.Errorf("can't verify signature with compressed key") | ||
| } | ||
|
|
||
| if VerifySignature(nil, testmsg, sig) { | ||
| t.Errorf("signature valid with no key") | ||
| } | ||
| if VerifySignature(testpubkey, nil, sig) { | ||
| t.Errorf("signature valid with no message") | ||
| } | ||
| if VerifySignature(testpubkey, testmsg, nil) { | ||
| t.Errorf("nil signature valid") | ||
| } | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. More potential testcases:
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. added |
||
| if VerifySignature(testpubkey, testmsg, append(common.CopyBytes(sig), 1, 2, 3)) { | ||
| t.Errorf("signature valid with extra bytes at the end") | ||
| } | ||
| if VerifySignature(testpubkey, testmsg, sig[:len(sig)-2]) { | ||
| t.Errorf("signature valid even though it's incomplete") | ||
| } | ||
| } | ||
|
|
||
| func TestDecompressPubkey(t *testing.T) { | ||
| key, err := DecompressPubkey(testpubkeyc) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. More potential testcases:
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. added |
||
| if err != nil { | ||
| t.Fatal(err) | ||
| } | ||
| if uncompressed := FromECDSAPub(key); !bytes.Equal(uncompressed, testpubkey) { | ||
| t.Errorf("wrong public key result: got %x, want %x", uncompressed, testpubkey) | ||
| } | ||
| if _, err := DecompressPubkey(nil); err == nil { | ||
| t.Errorf("no error for nil pubkey") | ||
| } | ||
| if _, err := DecompressPubkey(testpubkeyc[:5]); err == nil { | ||
| t.Errorf("no error for incomplete pubkey") | ||
| } | ||
| if _, err := DecompressPubkey(append(common.CopyBytes(testpubkeyc), 1, 2, 3)); err == nil { | ||
| t.Errorf("no error for pubkey with extra bytes at the end") | ||
| } | ||
| } | ||
|
|
||
| func BenchmarkEcrecoverSignature(b *testing.B) { | ||
| for i := 0; i < b.N; i++ { | ||
| if _, err := Ecrecover(testmsg, testsig); err != nil { | ||
| b.Fatal("ecrecover error", err) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| func BenchmarkVerifySignature(b *testing.B) { | ||
| sig := testsig[:len(testsig)-1] // remove recovery id | ||
| for i := 0; i < b.N; i++ { | ||
| if !VerifySignature(testpubkey, testmsg, sig) { | ||
| b.Fatal("verify error") | ||
| } | ||
| } | ||
| } | ||
|
|
||
| func BenchmarkDecompressPubkey(b *testing.B) { | ||
| for i := 0; i < b.N; i++ { | ||
| if _, err := DecompressPubkey(testpubkeyc); err != nil { | ||
| b.Fatal(err) | ||
| } | ||
| } | ||
| } | ||
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
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.
Shouldn't
len(pubkey)be one of two values ? So either compressed or uncompressed at65bytes, or compressed at33bytes. If so, I'd prefer to check against one of those. Better to be strict whenever possible.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'd prefer to leave it to libsecp256k1 to check for the length. If you disagree strongly we can add the check here too. The check for zero is needed to prevent a crash for the
&pubkey[0]construction.Uh oh!
There was an error while loading. Please reload this page.
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 length check is done here.