Skip to content

Make show-validator kms aware#2975

Closed
zmanian wants to merge 1 commit intocosmos:developfrom
zmanian:zaki/show_validator_kms_aware
Closed

Make show-validator kms aware#2975
zmanian wants to merge 1 commit intocosmos:developfrom
zmanian:zaki/show_validator_kms_aware

Conversation

@zmanian
Copy link
Copy Markdown
Contributor

@zmanian zmanian commented Dec 1, 2018

Make the show-validator command aware of the KMS and pull a public key from the kms

  • Linked to github-issue with discussion and accepted design OR link to spec that describes this work.
  • Wrote tests
  • Updated relevant documentation (docs/)
  • Added entries in PENDING.md with issue #
  • rereviewed Files changed in the github PR explorer

For Admin Use:

  • Added appropriate labels to PR (ex. wip, ready-for-review, docs)
  • Reviewers Assigned
  • Squashed all commits, uses message "Merge pull request #XYZ: [title]" (coding standards)

@codecov
Copy link
Copy Markdown

codecov Bot commented Dec 1, 2018

Codecov Report

Merging #2975 into develop will decrease coverage by 0.17%.
The diff coverage is 0%.

@@             Coverage Diff             @@
##           develop    #2975      +/-   ##
===========================================
- Coverage    56.29%   56.11%   -0.18%     
===========================================
  Files          120      120              
  Lines         8406     8432      +26     
===========================================
  Hits          4732     4732              
- Misses        3352     3378      +26     
  Partials       322      322

Comment thread server/tm_cmds.go
return &cmd
}

//This is a copy of a the same function from Tendermint.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Could we import it from Tendermint then?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

For that it would be needed to be exported/public first.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is there a reason for it not to be, if other code (e.g. the SDK) is likely to want to use it?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

++

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yes, you are probably right. If this is the only use-case (return the remote signer's pub-key), we might consider adding a simple method that does exactly this instead. I thought privValidator.GetPubKey() does exactly this (as it sends a request in the remote case and returns what was read from the file in the FilePV case). Have not tried if this works though.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Sounds great!

Copy link
Copy Markdown
Contributor

@liamsi liamsi Dec 3, 2018

Choose a reason for hiding this comment

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

OK, it's not as simple as this. I think, a way to make this work without changing the tendermint API: add a flag --priv_validator_laddr to this command. If this is not empty (e.g. --priv_validator_laddr=tcp://127.0.0.1:26659), spin up a tendermint node (like in the start sub-command), get the validator object via:

        pubKey := tmNode.PrivValidator().GetPubKey() // this might err (currently even panic) if remote signer is not avail.
	pk, _ := types.Bech32ifyConsPub(pubKey)

	fmt.Println(pk)

Alternatively, the method below createAndStartPrivValidatorSocketClient could be exported.

@alexanderbez
Copy link
Copy Markdown
Contributor

Is there a way to implement a unit test for this?

@liamsi
Copy link
Copy Markdown
Contributor

liamsi commented Dec 3, 2018

Is there a way to implement a unit test for this?

Would be more like an integration test: spin up a remote signer and see if the ShowValidatorCmd returns the expected remote signer's pub-key (and that it actually requests it from the remote signer). I'll try it.

Comment thread server/tm_cmds.go

//Check to see if there is valid KMS url
if cfg.PrivValidator != "" {
privval, err := createAndStartPrivValidatorSocketClient(cfg.PrivValidator, ctx.Logger)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This won't work. cfg.PrivValidator will contain config/priv_validator.json while createAndStartPrivValidatorSocketClient expects a listen address.

@jackzampolin
Copy link
Copy Markdown
Contributor

@liamsi Are you working on this PR?

@cwgoes
Copy link
Copy Markdown
Contributor

cwgoes commented Dec 13, 2018

It looks like we're fixing this upstream - @liamsi is that correct / can we close this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants