Skip to content
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

Secure key management #855

Closed
kmw101 opened this issue Jul 4, 2019 · 7 comments · Fixed by #923
Closed

Secure key management #855

kmw101 opened this issue Jul 4, 2019 · 7 comments · Fixed by #923
Labels
design Needs design work

Comments

@kmw101
Copy link
Contributor

kmw101 commented Jul 4, 2019

Implement Secure key management.

It is best to use key storage with the ability to set the current key. Then when we use bnscli it just automagically takes the chosen key or errors out if none is chosen. We won't have to tell bnscli anything then as it will already know.

Proposal (by @ruseinov):

  1. Use Encrypt/Decrypt from cosmos-sdk as described here: Secure key management #855 (comment) to encrypt/decrypt the file.
  2. Use protobuf objects like these (write them into two separate files):
message EncryptedStore {
   string mnemonic = 1;
   repeated Key keys = 2; ( { name, hexEncodedprivateKey }, sequence is an index within the array
}

message PublicStore {
  repeated PubKey keys = 1; ({ name, pubKey, address}), sequence and name correspond to those of encryptedStore
}

to store data.
3. Use the flow described in #855 (comment) for create/list/sign.

Key derivation

See #856 (comment) for details

@kmw101 kmw101 added the design Needs design work label Jul 4, 2019
@ethanfrey
Copy link
Contributor

ethanfrey commented Jul 9, 2019

From #856 we can use a mnemonic to create iov-core compatible account sequences.

For secure key management, we should:

  • store the info in an encrypted file
  • prompt user for a password (on stdin) to decrypt the file when we sign
  • use account sequences (0, 1, 2) and optional names with each key to identify it.
  • default to --account=0 if nothing is provided
  • add bnscli key create [name] to create the next sequence with an optional name (this requires a password to access the mnemonic)
  • add bnscli key list to show all keys and addresses (ideally without requiring a password, we can store the public info unencrypted)
  • bnscli sign [-keyfile=<file>] [-account=N] will load keys from the keyfile (provide default path) and select account by sequence or name

Reference code for handling password input securely: https://github.com/cosmos/cosmos-sdk/blob/master/client/keys/utils.go#L40-L77
Reference code for handling keys (and persisting encrypted to disk): https://github.com/cosmos/cosmos-sdk/blob/master/crypto/keys/keybase.go

@ethanfrey
Copy link
Contributor

ethanfrey commented Jul 9, 2019

For actual encryption/decryption, we can use the following (unless there is a better suggestion):

This uses bcrypt as a KDF, along with xsalsa20 for encryption. I do think iov-core's use of scrypt with argon2 KDF and Xchacha20poly1305Ietf encryption is more modern and arguably more secure. (In fact both algorithms are the named successors to those used in cosmos-sdk). However, importing the cosmos-sdk encyption is a no-brainer and has been audited. Discuss which approach to take before starting.

@alpe
Copy link
Contributor

alpe commented Jul 29, 2019

This task includes the key derivation as mentioned in #856 (comment) .

@alpe
Copy link
Contributor

alpe commented Jul 29, 2019

Addresses contain a key prefix in iov-core

  • "iov" mainnet prefix
  • "tiov" testnets

@husio husio assigned husio and unassigned ruseinov Jul 29, 2019
@webmaster128
Copy link
Contributor

This requires the following software-based key derivation:

  1. Generate and English BIP39 compatible mnemonic (See Create the base secret)
  2. Store this secret on disk (using whatever encryption; we can start unencrypted)
  3. Use a BIP39 library to create the seed with no password (see From_mnemonic_to_seed; test vectors)
  4. Have a SLIP-0010 library derive the first Ed25519 keypair using derivation path m/44'/234'/0' (See docs; test vectors)
  5. Derive weave address from Ed25519 pubkey (see docs; test here is based on weave test vecors)
  6. Derive Bech32 address from weave address (see docs; test vectors)

husio added a commit that referenced this issue Jul 29, 2019
Instead of generating a random private key use algorithm described in
https://github.com/iov-one/iov-core/blob/master/docs/address-derivation-v1.md
to create a mnemonic and later use that mnemonic to create a private
key.

resolve #855

Tests are missing.
@husio husio closed this as completed in #923 Aug 1, 2019
husio added a commit that referenced this issue Aug 1, 2019
cmd/bnscli: use mnemonic to generate private key

Instead of generating a random private key use algorithm described in
https://github.com/iov-one/iov-core/blob/master/docs/address-derivation-v1.md
to create a mnemonic and later use that mnemonic to create a private
key.

resolve #855
@ruseinov
Copy link
Contributor

ruseinov commented Aug 1, 2019

Reopening as only the derivation part of this ticket is done, the key store (described in issue body) hasn't been approached yet.

@ruseinov ruseinov reopened this Aug 1, 2019
@husio husio removed their assignment Aug 1, 2019
@husio
Copy link
Contributor

husio commented Aug 1, 2019

As discussed today during our call, @webmaster128 and @davepuchyr will decide what functionality we want to provide in bnscli. The current implementation and possible improvements were discussed. Now we need a decision what changes do we need to apply to complete key management functionality in bnscli

@kmw101 kmw101 closed this as completed Aug 7, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
design Needs design work
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants