Create verifier #314
Create verifier #314
Conversation
| glog.Warningf("error parsing EC key: %s", err) | ||
| return nil, trillian.SignatureAlgorithm_ANONYMOUS, errors.New("could not parse private key") | ||
| return key2, trillian.SignatureAlgorithm_ECDSA, nil | ||
|
|
There was a problem hiding this comment.
I couldn't immediately see - what's the benefit of the change here?
There was a problem hiding this comment.
Go style stipulates that the main code path should be not be indented. This puts the error case inside the if block and the success case at the outer level of indentation.
There was a problem hiding this comment.
In this case I think keeping the symmetry of the different cases is more helpful.
There was a problem hiding this comment.
Happy to change it back, though when reading the code, this deviation from the style guide caused me confusion
There was a problem hiding this comment.
I'm aware of the style guide recommendation, but I think consistency is better here -- all the arms above have the "non-error" path indented.
| R, S *big.Int | ||
| } | ||
| rest, err := asn1.Unmarshal(sig, &ecdsaSig) | ||
| if err != nil { |
There was a problem hiding this comment.
It would be nicer to propagate more error information, e.g.:
fmt.Errorf("failed to unmarshal signature: %v", err)
fmt.Errorf("trailing data in signature")
fmt.Errorf("failed to verify signature")
(which probably also means the general ErrVerify isn't needed)
There was a problem hiding this comment.
Errors are obscured for security reasons. I could log them, but I'm not clear on what logging scheme is preferred in Trillian.
There was a problem hiding this comment.
Not sure I follow the security concern -- wouldn't a concern about a Bleichenbacher-style oracle attack only apply at the point where results are exposed over a remote interface? Or to put it another way: a direct caller of this code could figure out the same error details themselves, by examining the inputs to the function.
(I just know that it's a pain in the neck to debug what's wrong when signatures don't immediately verify.)
There was a problem hiding this comment.
True, but we don't know how people will use this library. Prefer to have secure defaults. I know it's a pain in the neck too. - but I find that that pain tends to only happen during development, when inserting Printfs is a great debugging strategy. After the code is stable, it rarely comes up.
| ) | ||
|
|
||
| const ( | ||
| // openssl ecparam -name prime256v1 -genkey -out p256-key.pem |
There was a problem hiding this comment.
Yay for including a comment that indicates how the key data was generated!
| MHcCAQEEIGbhE2+z8d5lHzb0gmkS78d86gm5gHUtXCpXveFbK3pcoAoGCCqGSM49 | ||
| AwEHoUQDQgAEUxX42oxJ5voiNfbjoz8UgsGqh1bD1NXK9m8VivPmQSoYUdVFgNav | ||
| csFaQhohkiCEthY51Ga6Xa+ggn+eTZtf9Q== | ||
| -----END EC PRIVATE KEY-----` |
There was a problem hiding this comment.
Would the key in testonly/keys.go work here?
There was a problem hiding this comment.
or is there something special about this key for this test?
There was a problem hiding this comment.
It works, and I've used it.
This particular key does not have a password attached, testing a new code path, so it's remained.
| ) | ||
|
|
||
| func TestSignVerify(t *testing.T) { | ||
| for _, tc := range []struct { |
There was a problem hiding this comment.
I'm told test is more usual than tc
|
|
||
| km := NewPEMKeyManager() | ||
| if err := km.LoadPrivateKey(tc.PEM, ""); err != nil { | ||
| t.Error(err) |
There was a problem hiding this comment.
More information in failure messages would be good (throughout); Go form seems to normally be like t.Errorf("MyFunc(arg)=%v; want %v", got, want), so this one would be:
t.Errorf("LoadPrivateKey(pem)=%v; want nil", err)
| } | ||
| hasher, err := NewHasher(test.HashAlgo) | ||
| if err != nil { | ||
| t.Errorf("NewHasher(%v)=%v, want nil", test.HashAlgo, err) |
There was a problem hiding this comment.
Prefer to give a clue about which return value is unexpected, e.g.
t.Errorf("NewHasher(%v)=(_,%v), want (_,nil)", test.HashAlgo, err)
(passim)
In KeyManager: * Adds support for keys that are not password protected. * Supports getting the public key for private keys that are loaded.
In KeyManager: