Skip to content

crypto: improve error messages in LoadECDSA #20718

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.

Sign up for GitHub

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

Merged
merged 12 commits into from
Apr 8, 2020
Merged

crypto: improve error messages in LoadECDSA #20718

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
f39afd5
Specific error message when the private key to be imported is too short
adamschmideg Feb 26, 2020
0a22d23
Use fmt.Errorf
adamschmideg Feb 26, 2020
745f0fd
goimports fix
adamschmideg Feb 26, 2020
ae3c77b
Check file size before reading it into memory
adamschmideg Mar 3, 2020
a66d88e
Improve wording of error message
adamschmideg Mar 3, 2020
fe0bc49
Allow newline after account
adamschmideg Mar 4, 2020
3db06e4
More idiomatic way to check for trailing whitespace
adamschmideg Mar 4, 2020
969c671
Use canonical table-driven test format
adamschmideg Mar 17, 2020
3d3d5bb
Formatting, oh my
adamschmideg Mar 17, 2020
b4916e6
Use string literals, no magic
adamschmideg Mar 17, 2020
b68c0f5
crypto: improve LoadECDSA
fjl Apr 7, 2020
0d574ee
cmd/geth: simplify TestAccountImport
fjl Apr 7, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 32 additions & 0 deletions cmd/geth/accountcmd_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,38 @@ Path of the secret key file: .*UTC--.+--[0-9a-f]{40}
`)
}

func TestAccountImport(t *testing.T) {
bytes64 := "0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef"
success := `Address: {[0-9a-f]{40}}`
failure := `Fatal: Failed to load the private key: expected 64 bytes, got \d+`
keyToMsg := make(map[string]string)
keyToMsg[bytes64] = success
keyToMsg[bytes64[:40]] = failure
keyToMsg[bytes64+"\n"] = success
keyToMsg[bytes64+"\r\n"] = success
keyToMsg[bytes64+"1"] = failure
keyToMsg[bytes64+"x"] = failure
keyToMsg[bytes64+bytes64] = failure
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The canonical way to define table driven tests would be:

tests := []struct{
  key string
  err error
} {
  {key: "0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef", err: nil},
  {key: "0123456789abcdef0123456789abcdef01234567", err: errSomethingSomething},
  [...]
}

Please also define a separate error constant if you want to check for it. Using a string is suuuuper brittle. Alternatively wait until Go 1.14.1 is out so we can deprecate 1.11 and 1.12 and then use the fancy new error constructs.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also please don't use regexps for error or success checks. Since you know the exact input, you also know the exact output. Using regexps would just hide potential errors.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • I refactored to canonical table-driven test format
  • I replaced regexps with exact matches

I'm still using the bytes64 + "\n" format because I find it more understandable than a long string with a \n at the end. I can change that, too.

Since I compare the string returned by the command line, I don't know how to use an error here. The underlying TestCmd doesn't expose the error returned by the comparison, it simply calls t.Fatal() if there's a mismatch.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm still using the bytes64 + "\n" format because I find it more understandable than a long string with a \n at the end. I can change that, too.

Please change it, this is a short test suite, but when things get bigger (e.g. hundreds of lines of test cases), you don't want to keep jumping back and forth to find what variables in there mean.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, replaced with string literals

for key, msg := range keyToMsg {
importAccountWithExpect(t, key, msg)
}
}

func importAccountWithExpect(t *testing.T, key string, expectedRegexp string) {
dir := tmpdir(t)
keyfile := filepath.Join(dir, "key.prv")
if err := ioutil.WriteFile(keyfile, []byte(key), 0644); err != nil {
t.Error(err)
}
passwordFile := filepath.Join(dir, "password.txt")
if err := ioutil.WriteFile(passwordFile, []byte("foobar"), 0644); err != nil {
t.Error(err)
}
geth := runGeth(t, "account", "import", keyfile, "-password", passwordFile)
defer geth.ExpectExit()
geth.ExpectRegexp(expectedRegexp)
}

func TestAccountNewBadRepeat(t *testing.T) {
geth := runGeth(t, "account", "new", "--lightkdf")
defer geth.ExpectExit()
Expand Down
22 changes: 16 additions & 6 deletions crypto/crypto.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,6 @@ import (
"encoding/hex"
"errors"
"fmt"
"io"
"io/ioutil"
"math/big"
"os"
Expand Down Expand Up @@ -166,17 +165,28 @@ func HexToECDSA(hexkey string) (*ecdsa.PrivateKey, error) {

// LoadECDSA loads a secp256k1 private key from the given file.
func LoadECDSA(file string) (*ecdsa.PrivateKey, error) {
buf := make([]byte, 64)
fd, err := os.Open(file)
stat, err := os.Stat(file)
if err != nil {
return nil, err
}
defer fd.Close()
if _, err := io.ReadFull(fd, buf); err != nil {
size := stat.Size()
// Allow two extra chars for possible line ending to be checked later
if size < 64 || size > 66 {
return nil, fmt.Errorf("expected 64 bytes, got %v", size)
}
buf, err := ioutil.ReadFile(file)
if err != nil {
return nil, err
}
// Check line ending
maybeLineEnding := buf[64:]
for _, ch := range maybeLineEnding {
if ch != '\n' && ch != '\r' {
return nil, fmt.Errorf("expected 64 bytes, got %v", size)
}
}

key, err := hex.DecodeString(string(buf))
key, err := hex.DecodeString(string(buf[:64]))
if err != nil {
return nil, err
}
Expand Down