For now, you can manually instantiate a PrivValidator object and then you don't have to provide the PrivKey as described in the comment.

That would allow you to provide it a custom signer.

@adrianbrink I've created #641 to implement those changes.

@ebuchman Should I work on implementing the 2nd abstraction in this PR or can we merge this and open a new one for the next step?

@dmjones500 Does this work for you? The next step is to extract PrivValidator as an interface so that the HSM can handle everything (including round tracking).

@adrianbrink Sounds good to me. I've made some progress on a basic implementation, just overriding the signer. As soon as you've extracted an interface, I'll bring more logic into the HSM.

Still working on this. Looking better though!

To add to my comment on #647

I think that to outside parties it makes more sense to just ask for a Go crypto.Signer. Looking at your PrivValidator implementations in this PR I can see why you have the interface you do with SignVote, SignProposal, and SignHeartbeat.

However I think the fact you need them is a symptom that your SignXXX methods conflate verification, digest, and signing. For example the signByteHRS method: https://github.com/tendermint/tendermint/blob/feature/hsm/types/priv_validator.go#L228-L278 is both verifying a structure, requiring specific knowledge of the structure, and signing it requiring none. This bubble up so that to provide a PrivValidator we need to know the signing secret and arcane knowledge of how to check and produce the digest. I suggest you somehow pull these apart. Do you need different verification logic for different implementations of PrivValidator? Can you make do just be taking a validatorSigner crypto.Signer and using the same logic? If all parts of the process need to vary then you could using interfaces like Digestable, Verifiable, and Signer.

I also think that:

type Signable interface {
	WriteSignBytes(chainID string, w io.Writer, n *int, err *error)
}

Should become:

type Digestable interface {
	Digest(w io.Writer) (int, error)
}

If you want chainID included just close it in to a new Digestable and form a joint digest rather than mandating its extra special status:

func JointDigest(w io.Write, digestables... Digestable) (n int, err error) {
	for _, d := range digestables {
		dn, err := d.Digest(w)
		n += dn
		if err != nil {
			return	
		}
	}
	return
}

@silasdavis Teasing out a separate Signer interface (based on Go's crypto module) sounds sensible. However, there are cases where it would be useful to override normal priv validator behaviour too. The specific example I have in mind is incorporating the validation logic within a trusted component. Rather than simply saying "sign this" to your HSM (or enclave, or whatever), you pass sufficient information to allow an informed decision about performing the signature.

This use case is what is triggering the redesign work to abstract the priv validator. (Or at least it's one of the reasons why).

@dmjones500 having validation/verification happen in a trusted enclave makes sense and sounds like a nice feature. And I see t(hat you need to stay within that enclave to ensure the bytes that get signed were the same ones it verified, so I suppose having two calls to hardware of Verify(message) and Sign(messageDigest) doesn't work in this case.

This being the case then perhaps the interface you have is closer to what NewNode should take. If we can provide a decorator along the lines of proxy.NewLocalClientCreator and friends that you pass a crypto.Signer and you get a software-based default PrivValidator subsuming the verify, digest, and signing logic:

type softwarePrivValidator struct {
	signer crypto.Signer
}

...

func SoftwarePrivValidator(signer crypto.Signer) types.PrivValidator {
	return &softwarePrivValidator{
		signer: signer,
	}
}

Then HSM-integrators that want to host he verification logic can provide the message-specific signing functions with a custom PrivValidator but others can just provide the Signer and boost it up to a PrivValidator.

Codecov Report

Merging #637 into develop will increase coverage by 0.69%.
The diff coverage is 53.33%.

@@             Coverage Diff             @@
##           develop     #637      +/-   ##
===========================================
+ Coverage    55.67%   56.37%   +0.69%     
===========================================
  Files           81       81              
  Lines         8404     8403       -1     
===========================================
+ Hits          4679     4737      +58     
+ Misses        3331     3262      -69     
- Partials       394      404      +10

@silasdavis take a look at latest commit re NewNode. Does that look good to you?

I think there are two use cases for in-process here:

1. folks that want the whole CLI and just want to replace the privValidator and/or the in-proc app. These folks can just copy the main file and replace the NewRunNodeCmd. Perhaps we should just let all the NewNode params flow through the NewRunNodeCmd ...

2. folks that want to control the whole CLI. These folks can use the new NewNode, which now has db and gendoc providers.

I think PrivValidator is fine rather than Signer because we expose a function to make a new PrivValidator from a Signer: LoadPrivValidatorFSWithSigner(filePath, signerFunc)

Does that sound sufficient?

I'll still look into using Go's crypto.Signer

@ebuchman looks great so far. If 1 is a use case you know about then that sounds reasonable. However, personally my concern would be feature creep, whereby people want a little bit of extra flexibility in configuring the CLI and you end up maintaining half-arsed command line wrapper when you just want to be a good consensus engine. You say they can copy the main.go and change things, I'd say why not have them just copy NewRunNodeCmd and have it in the source tree as an example. Then you have one interface to maintain and you still meet your goal of providing a quick start for your use case 1.

Regarding LoadPrivValidatorFSWithSigner that does almost what I'd want, but again I'd rather not have my library messing around with the filesystem if it doesn't need to. Particularly given the Save() panics: https://github.com/tendermint/tendermint/blob/feature/hsm/types/priv_validator.go#L143!

I understand that the PrivValidator needs access to some blockchain state to do its signing, but why does it need to write it to disk (and why in the priv_validator.json?)? Surely the node has access to such state. I think originally it was a convenient place to dump latest state that would read in on startup. But IMO it's always been ugly that it mixed private key matter and the last signed information. Furthermore for testing it causes various timing issues and what should be a static asset (the keys) has to be reset between runs.

Can we remove the dependence on the filesystem from PrivValidator altogether and just have it as an in-memory object (connected to some signing resource you don't control)? Even for a PrivValidator file storing private keys I don't think it's great to have the block height and last signed information held there. Surely we have the persisted state database for this, and failing that the consensus write-ahead log?

So to give a bit of background the motivation on this 'leave the filesystem alone' (and the DBProvider) stuff from me is:

1. Allows me to track and reason about the totality of the state which makes it easier to:

• Set up and and tear down test environments
• Ship around the same logical validator to a different availability zone
• Perform state rollbacks, recover from fatal errors, restore state

2. Virtualise the state stored on disk (e.g. store in a database, store on IPFS)

This looks like it will be really useful for me so thanks !

I suppose what I'm really asking is we separate the verification logic from how the LastSignedInfo is obtained, so that it is not coupled to reading filesystem. So how about something like:

func LoadPrivValidatorDBWithSigner(dbProvider DBProvider, 
	signerFunc func(ValidatorID) Signer) *PrivValidatorDB {

Or just:

func LoadPrivValidatorWithSigner(infoProvider func() *LastSignedInfo, 
	signerFunc func(ValidatorID) Signer) PrivValidator {

why not have them just copy NewRunNodeCmd and have it in the source tree as an example.

So they get the rest of the CLI as well - init, unsafe_reset, etc.

Can we remove the dependence on the filesystem from PrivValidator altogether

No, I think the PrivValidator needs a persistent store to write to. We can have it take something more generic though, like a Writer, rather than a file name. But we do need an object that wraps the signer that checks persisted conditions before signing.

As for:

func LoadPrivValidatorWithSigner(infoProvider func() *LastSignedInfo, 
	signerFunc func(ValidatorID) Signer) PrivValidator {

this seems nice, but we need the PrivValidator to persist the LastSignedInfo after every time it changes. So something, either the PrivValidator itself or the LastSignedInfo, needs to expose a Save function, which needs to be able to be backed by something persistent. For testing, it should be fine to use an in memory item.

As for crypto.Signer, I don't think we should use this yet. We need a go-crypto.Signature in the rest of the codebase, so we have to convert the returned sig []byte into a crypto.Signature, which means the thing returned by a generic signer would need to either be go-wire encoded according to go-crypto type bytes, or we'd need some other scheme. In either case, I think we're better just being explicit about returning go-crypto.Signature for now.

Let me know if you have other/better ideas about this.

As for SignBytes -> Digest ... my understanding is that a Digest is typically the hash of the content, whereas the SignBytes are the actual content, we let the signer handle any hashing for now.

Here's an idea ... what if we just move signBytesHRS onto the LastSignedInfo and expose it - then it becomes super easy to implement a PrivValidator using whatever backing you want!

I implemented a wrapper around crypto.PrivKey and crypto.PubKey to use the ledger nano s. It works pretty well, except for a question about error handling. By doing so, I was able to use the cli key manager embedded in basecoin with almost no changes (just registering a CreateLedger function to parallel CreateEd25519.

I am not sure what the specific use-case here is. Which HSM? I assume no human input, but it wants to verify the data before signing as well? As a separate app?

Maybe my work on tendermint/go-crypto#37 can be an inspiration of what (or what not) to do for hardware integration

Nice idea with SignByteHRS, but if the point is we just want to use a custom signer... maybe this inversion would make sense:

current proposal:

func (info *LastSignedInfo) SignBytesHRS(signer Signer,
	height, round int, step int8, signBytes []byte) (crypto.Signature, error) {

alternative:

type Signer interface {
  // ....
  SignBytesHRS(info *LastSignedInfo,
	height, round int, step int8, signBytes []byte) (crypto.Signature, error)
}

But then that doesn't make for full security, as the hardware signer shouldn't trust lastsignedinfo coming from code. maybe this:

type Signer interface {
  // ....
  SignBytesHRS(height, round int, step int8, signBytes []byte) (crypto.Signature, error)
}

and then a signer should do the check on LastSignedInfo itself. Does that make sense?

Should LastSignedInfo at least have the possibility of being hosted and stored in the secure enclave/signer then? I can see the sense in that.

Yes, @silasdavis that was my idea.

If the signer doesn't check, just keeps the keys safe, then the current work is fine, and just a new Signer implementation. (Which will need a (Signature, error) return value to detect unplugged signers, etc)

The comments from @dmjones500 suggested that they would like to store the LastSignedInfo on the hardware device, then get enough info before signing to make sure they never double-sign.

Maybe stateless-signer (that trusts no double-sigs come to it), and a stateful-signer that checks for double-sigs

Ok, I reverted the changes to PrivValidatorFS pending a more formal write up and further discussion. So this PR is now more just about the new interface