Unify local and external keys in keybase interface#117
Conversation
|
To-do:
|
|
@liamsi Thoughts on the API changes? |
|
The interface changes look good to me! Totally in favour of sign returning an error. I also like changing Info to an interface. I might change the |
d245bae to
84bff8e
Compare
84bff8e to
60827af
Compare
keys/keybase.go
Outdated
| case ledgerInfo: | ||
| case offlineInfo: | ||
| if passphrase != "yes" { | ||
| return fmt.Errorf("Enter exactly 'yes' to delete the key") |
There was a problem hiding this comment.
nitpick: Error messages shouldn be capitalized. Ideally, run golint before finalizing the PR.
There was a problem hiding this comment.
Addressed. Agreed on golint, unfortunately this repo is very noncompliant at the moment...
There was a problem hiding this comment.
Yeah, you can run go-lint only on the files you've modified. That would be great.. Otherwise, don't bother. I'll delete so much code in #116 ... after that nothing is left and it's secure and very compliant ;-)
59609f8 to
94d6b8d
Compare
liamsi
left a comment
There was a problem hiding this comment.
LGTM, left some nitpicks in comments
encode_test.go
Outdated
| // Check (de/en)codings of PubKeys. | ||
| pubKey := tc.privKey.PubKey() | ||
| pubKey, err := tc.privKey.PubKey() | ||
| assert.Nil(t, err) |
There was a problem hiding this comment.
Can we move this newly introduced asserts to assert.NoError? It slightly increases the readability of the tests.
There was a problem hiding this comment.
All instances changed to assert.NoError.
keys/keybase.go
Outdated
| linfo := info.(offlineInfo) | ||
| fmt.Printf("Bytes to sign:\n%s", msg) | ||
| buf := bufio.NewReader(os.Stdin) | ||
| fmt.Printf("\nEnter encoded signature:\n") |
There was a problem hiding this comment.
Be more specific: encoded in what?
There was a problem hiding this comment.
Amino-encoded signature (updated), for now at least.
| if err != nil { | ||
| return nil, nil, err | ||
| } | ||
| cdc.MustUnmarshalBinary([]byte(signed), sig) |
There was a problem hiding this comment.
Can you add a comment, that 1) this will block until the user inputs the signature and 2) this assumes that the offline computer is aware of the structure to be signed (TXs?) and properly encodes it in amino. Are we writing the offline-signing software, too? Otherwise, it might be reasonable to assume some other more common encoding. Thoughts?
There was a problem hiding this comment.
I agree that requiring Amino is suboptimal, let's track that separately though - #121.
Added a comment about blocking.
There was a problem hiding this comment.
Great, thanks! I'll join the discussion in #121 👍
| case ledgerInfo: | ||
| case offlineInfo: | ||
| if passphrase != "yes" { | ||
| return fmt.Errorf("enter exactly 'yes' to delete the key") |
There was a problem hiding this comment.
Is 'yes' always a user entered value? Otherwise: "call with seedphrase 'yes' to delete the key".
There was a problem hiding this comment.
In the only downstream consumer I know of, it is user-entered. Leaving for now, if in the future we have other consumers we should probably just change this API anyways.
There was a problem hiding this comment.
Can this support y also? (Alot of people are probably in the habit of this)
There was a problem hiding this comment.
Most bash commands don't delete keys (even just public keys, as in this case) though. IMO better to require that the user enter exactly 'yes'.
keys/keybase.go
Outdated
|
|
||
| func (kb dbKeybase) writePubKey(pub crypto.PubKey, name string) Info { | ||
| func (kb dbKeybase) writeLocalKey(priv crypto.PrivKey, name, passphrase string) Info { | ||
| // generate the encrypted privkey |
There was a problem hiding this comment.
Does this actually generate anything? Proposal:
// encrypt private key using passphrase
There was a problem hiding this comment.
Agreed, reworded file so "generate" is only used for actually generating keys.
| panic(err) | ||
| } | ||
| return bz | ||
| return cdc.MustMarshalBinaryBare(privKey) |
There was a problem hiding this comment.
Hmm, unrelated to your code but why do we amino encode these bytes?
Rework
PrivKey,Keybase, andInfoto work with both locally stored and external keys (offline, HW wallets, etc).This involves the following API changes:
privkey.Sign()andprivkey.PubKey(), as we may be querying an external datastoreKeybaseinterface to track Ledger HD ed25519 keys and offline keys, so that signing by name automatically looks up the right key and prompts the user for a password / communicates with the Ledger / prompts the user to sign offline, as appropriate.