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

Adding Basic Private Key Management #671

Merged
merged 36 commits into from Nov 8, 2018
Merged

Conversation

nisdas
Copy link
Member

@nisdas nisdas commented Oct 17, 2018

This is an initial PR which will deal with #290.

Objectives:

  • Allow clients to derive a public key from a provided keystore.
  • Add relevant tests

Keystore:
The keystore will allow users to encrypt a bls secret key and its corresponding public key in a keystore using the AES-CTR-256 encryption algorithm. This borrows a lot from how keystore's are managed in go-ethereum currently and uses the relevant parts from it.

Users can then provide the keystore in the future when running a validator client, and the client will make use of the public key to listen for logs, verify identity of users, etc.

Currently it uses the BLS API to implement all the private key/public key functions even though they are currently stubs now. To see the full functionality being implemented we would have wait till #698 is able to be merged.

@nisdas nisdas added In Progress Priority: High High priority item labels Oct 17, 2018
@nisdas nisdas added this to the Ruby - v0.1 milestone Oct 17, 2018
@nisdas nisdas self-assigned this Oct 17, 2018
@nisdas nisdas added this to To do in Validator Client via automation Oct 17, 2018
@nisdas nisdas added this to In progress in Beacon Chain via automation Oct 17, 2018
func DecryptKeystore(keyjson []byte, password string) (ecdsa.PublicKey, error) {
key, err := keystore.DecryptKey(keyjson, password)
if err != nil {
return ecdsa.PublicKey{}, err
Copy link
Contributor

Choose a reason for hiding this comment

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

note: we won't be using ecdsa but rather bls, so feel free to use the bls.PublicKey{} definition from the shared/bls package here

@codecov
Copy link

codecov bot commented Oct 19, 2018

Codecov Report

Merging #671 into master will decrease coverage by 1.22%.
The diff coverage is 55.55%.

@@            Coverage Diff             @@
##           master     #671      +/-   ##
==========================================
- Coverage   75.14%   73.92%   -1.23%     
==========================================
  Files          59       62       +3     
  Lines        3834     4061     +227     
==========================================
+ Hits         2881     3002     +121     
- Misses        692      766      +74     
- Partials      261      293      +32

@rauljordan rauljordan added this to In progress in Runtime UX Oct 19, 2018
@nisdas nisdas removed this from To do in Validator Client Oct 20, 2018
@nisdas nisdas removed this from In progress in Beacon Chain Oct 20, 2018
Copy link
Contributor

@rauljordan rauljordan left a comment

Choose a reason for hiding this comment

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

Lookin good! Just a few things

return &PublicKey{}, nil
}

func (s *SecretKey) BufferedSecretKey() []byte {
Copy link
Contributor

Choose a reason for hiding this comment

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

Comments on these funcs to prevent lint failures

@@ -0,0 +1,223 @@
package keystore
Copy link
Contributor

Choose a reason for hiding this comment

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

This is from geth right? Let's give credit by adding the header comments geth adds to their files here

Copy link
Member

@terencechain terencechain left a comment

Choose a reason for hiding this comment

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

looks good, just a few comments


// PublicKey corresponding to secret key used in the BLS scheme.
type PublicKey struct{}

// PublicKey returns the corresponding public key for the
// Secret Key
Copy link
Member

Choose a reason for hiding this comment

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

punctuation


inputKey := ctx.GlobalString(types.PubKeyFlag.Name)
if inputKey == "" {
var err error

pubKey, err = GeneratePubKey()
Copy link
Member

Choose a reason for hiding this comment

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

What kind of public key is this generating? How is the private key getting handled here?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah , I think this was part of the demo , so if no pubkey is inputted the node would generate a random one. I think with private key management being functional , there will be no need for this. Will remove it


// Key is the object that stores all the user data related to their public/secret keys.
type Key struct {
ID uuid.UUID // Version 4 "random" for unique id not derived from key data
Copy link
Member

Choose a reason for hiding this comment

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

What is this?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is for storing the key in the keystore, we are following how geth is doing it.

func TestWriteFile(t *testing.T) {
tmpdir := os.TempDir()
filedir := tmpdir + "/keystore"
//t.Errorf("%s ------ %s", tmpdir, filedir)
Copy link
Member

Choose a reason for hiding this comment

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

we should take out the commented code

@terencechain
Copy link
Member

shouldn't we push this to the branch bls?

@nisdas nisdas added Ready For Review A pull request ready for code review and removed In Progress labels Nov 6, 2018
@rauljordan
Copy link
Contributor

@nisdas this PR has a good bunch of stuff in it - could you update the description to elaborate on some of the things you included and what functionality can we expect from merging this?

@nisdas nisdas merged commit 37bc1c6 into prysmaticlabs:master Nov 8, 2018
Signature Aggregation & Verification automation moved this from In progress to Done Nov 8, 2018
Runtime UX automation moved this from In progress to Done Nov 8, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Priority: High High priority item Ready For Review A pull request ready for code review
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

3 participants