cmd/cored: add signerd client #484

Merged
merged 15 commits into from Feb 8, 2017

Projects

None yet

5 participants

@ameets
Contributor
ameets commented Feb 7, 2017

No description provided.

@erykwalder erykwalder changed the title from Signerd c to cmd/cored: add signerd interface Feb 7, 2017
@erykwalder erykwalder changed the title from cmd/cored: add signerd interface to cmd/cored: add signerd client Feb 7, 2017
@ameets ameets added the PTAL label Feb 7, 2017
cmd/cored/main.go
+ if conf.BlockHSMURL != "" {
+ hsm = &remoteHSM{Client: &rpc.Client{
+ BaseURL: conf.BlockHSMURL,
+ AccessToken: conf.BlockHSMAccessToken,
@erykwalder
erykwalder Feb 7, 2017 Contributor

signerd only uses the password portion of the auth. seems like there are two options:

  1. take a "access token" string of "username:password" when configuring (in this case ":password") and don't worry about modifying it
  2. take a password when configuring, and convert to an access token string here
@erykwalder
erykwalder Feb 7, 2017 Contributor

looking for outside opinions here

@bobg
bobg Feb 8, 2017 Contributor

Slight preference for (2).

cmd/cored/main.go
@@ -381,6 +395,20 @@ type remoteSigner struct {
Key ed25519.PublicKey
}
+// remoteHSM is a client wrapper for an hsm that is used as a blocksigner.signer
@kr
kr Feb 8, 2017 Member

typo, signer should be capitalized

@@ -79,7 +79,10 @@ func configGenerator(db *sql.DB, args []string) {
var flags flag.FlagSet
maxIssuanceWindow := flags.Duration("w", 24*time.Hour, "the maximum issuance window `duration` for this generator")
- isSigner := flags.Bool("s", false, "whether this core is a signer")
+ flagK := flags.String("k", "", "local `pubkey` for signing blocks")
@bobg
bobg Feb 8, 2017 Contributor

Don't think backticks belong in these help strings.

@ameets
ameets Feb 8, 2017 Contributor

I copied the help string format from configNongenerator. Not sure about the previous history - do we want to change the format throughout?

@bobg
bobg Feb 8, 2017 Contributor

No strong opinion; your call.

@ameets
ameets Feb 8, 2017 Contributor

Will leave as is for now.

@kr
kr Feb 8, 2017 Member

FYI the back ticks are a feature of https://godoc.org/flag#PrintDefaults:

The listed type, here int, can be changed by placing a back-quoted name in the flag's usage string; the first such item in the message is taken to be a parameter name to show in the message and the back quotes are stripped from the message when displayed. For instance, given

flag.String("I", "", "search `directory` for include files")

the output will be

-I directory
	search directory for include files.
@@ -79,7 +79,10 @@ func configGenerator(db *sql.DB, args []string) {
var flags flag.FlagSet
maxIssuanceWindow := flags.Duration("w", 24*time.Hour, "the maximum issuance window `duration` for this generator")
- isSigner := flags.Bool("s", false, "whether this core is a signer")
+ flagK := flags.String("k", "", "local `pubkey` for signing blocks")
+ flagHSMURL := flags.String("hsm-url", "", "hsm `url` for signing blocks (mockhsm if empty)")
@bobg
bobg Feb 8, 2017 Contributor

How about just -url and -token?

(Here and in createToken.)

@ameets
ameets Feb 8, 2017 Contributor

I think it's confusing to just use -url is because there are two url arguments: the hsm url (as a flag) and a pubkey url (as an arg) "usage: corectl config-generator [flags] [quorum] [pubkey url]..."

Open to suggestions

@bobg
bobg Feb 8, 2017 Contributor

OK, fine as-is.

cmd/corectl/main.go
+ }
+
+ //hsm url and access token are
+ //ToDo (ameets): update when switching to x.509 authorization
@bobg
bobg Feb 8, 2017 Contributor

Please use the format TODO(ameets): ... throughout.

@ameets
Contributor
ameets commented Feb 8, 2017

PTAL

Quorum: quorum,
Signers: signers,
MaxIssuanceWindow: chainjson.Duration{
Duration: *maxIssuanceWindow,
},
+ IsSigner: *flagK != "",
@erykwalder
erykwalder Feb 8, 2017 Contributor

Just to double check, previously if IsSigner was passed true without BlockPub, then config.Configure would generate a block key. Are we fine losing that option?

@ameets
ameets Feb 8, 2017 Contributor

I think so. The idea here was to standardize the format between config.Nongenerator and config.Generator

cmd/cored/main.go
+
+ var hsm blocksigner.Signer = mockHSM
+ if conf.BlockHSMURL != "" {
+ //TODO(ameets): these members may not all be needed - reevaluate
@erykwalder
erykwalder Feb 8, 2017 Contributor

All of them at least add information to the user agent or header which can be helpful in debugging. I think it's fine to leave all of them and clear this todo.

@@ -79,7 +79,10 @@ func configGenerator(db *sql.DB, args []string) {
var flags flag.FlagSet
maxIssuanceWindow := flags.Duration("w", 24*time.Hour, "the maximum issuance window `duration` for this generator")
- isSigner := flags.Bool("s", false, "whether this core is a signer")
+ flagK := flags.String("k", "", "local `pubkey` for signing blocks")
@bobg
bobg Feb 8, 2017 Contributor

No strong opinion; your call.

@@ -79,7 +79,10 @@ func configGenerator(db *sql.DB, args []string) {
var flags flag.FlagSet
maxIssuanceWindow := flags.Duration("w", 24*time.Hour, "the maximum issuance window `duration` for this generator")
- isSigner := flags.Bool("s", false, "whether this core is a signer")
+ flagK := flags.String("k", "", "local `pubkey` for signing blocks")
+ flagHSMURL := flags.String("hsm-url", "", "hsm `url` for signing blocks (mockhsm if empty)")
@bobg
bobg Feb 8, 2017 Contributor

OK, fine as-is.

cmd/corectl/main.go
@@ -88,8 +91,18 @@ func configGenerator(db *sql.DB, args []string) {
flags.Parse(args)
args = flags.Args()
+ //not a blocksigner
@bobg
bobg Feb 8, 2017 Contributor

Sorry to be so nitpicky but I'd like to see a space after the // in all comments.

core/blocksigner/blocksigner.go
@@ -53,6 +53,7 @@ func New(pub ed25519.PublicKey, hsm Signer, db pg.DB, c *protocol.Chain) *BlockS
func (s *BlockSigner) SignBlock(ctx context.Context, b *bc.Block) ([]byte, error) {
sig, err := s.hsm.Sign(ctx, s.Pub, &b.BlockHeader)
if err != nil {
+ //TODO(ameets): rethink error pkg to preserve full stack trace
@bobg
bobg Feb 8, 2017 Contributor

I think this TODO should be removed. It's not about this PR or this line of code.

(Also note that judicious use of errors.Wrap/Wrapf is even better than full stack traces, since they let us preserve exactly the important information and leave out the chaff.)

cmd/cored/main.go
+ if conf.BlockHSMURL != "" {
+ hsm = &remoteHSM{Client: &rpc.Client{
+ BaseURL: conf.BlockHSMURL,
+ AccessToken: conf.BlockHSMAccessToken,
@bobg
bobg Feb 8, 2017 Contributor

Slight preference for (2).

@ameets
Contributor
ameets commented Feb 8, 2017

@bobg @erykwalder another PTAL

@bobg
bobg approved these changes Feb 8, 2017 View changes
@bobg
Contributor
bobg commented Feb 8, 2017

LGTM, suggest waiting for @erykwalder's LGTM too

@erykwalder
Contributor

LGTM

@chainbot chainbot merged commit df51dab into main Feb 8, 2017

3 checks passed

licence/cla Contributor License Agreement is signed.
Details
wercker/cored Wercker pipeline passed
Details
wercker/java Wercker pipeline passed
Details
@chainbot chainbot deleted the signerd-c branch Feb 8, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment