[pv] add ability to use ipc validator (#2866)

Ref #2827

(I have since seen #2847 which is a fix for the same issue; this PR has tests and docs too ;) )

CHANGELOG_PENDING.md

@@ -2,8 +2,6 @@
 
 ## v0.26.4
 
-*TBD*
-
 Special thanks to external contributors on this release:
 
 Friendly reminder, we have a [bug bounty
@@ -28,6 +26,7 @@ program](https://hackerone.com/tendermint).
 - [config] \#2877 add blocktime_iota to the config.toml (@ackratos)
 - [mempool] \#2855 add txs from Update to cache
 - [mempool] \#2835 Remove local int64 counter from being stored in every tx
+- [node] \#2827 add ability to instantiate IPCVal (@joe-bowman)
 
 ### BUG FIXES:
 

docs/architecture/adr-008-priv-validator.md

@@ -5,14 +5,17 @@ implementations:
 
 - FilePV uses an unencrypted private key in a "priv_validator.json" file - no
   configuration required (just `tendermint init`).
-- SocketPV uses a socket to send signing requests to another process - user is
+- TCPVal and IPCVal use TCP and Unix sockets respectively to send signing requests
-  responsible for starting that process themselves.
+  to another process - the user is responsible for starting that process themselves.
 
-The SocketPV address can be provided via flags at the command line - doing so
+Both TCPVal and IPCVal addresses can be provided via flags at the command line
-will cause Tendermint to ignore any "priv_validator.json" file and to listen on
+or in the configuration file; TCPVal addresses must be of the form
-the given address for incoming connections from an external priv_validator
+`tcp://<ip_address>:<port>` and IPCVal addresses `unix:///path/to/file.sock` -
-process. It will halt any operation until at least one external process
+doing so will cause Tendermint to ignore any private validator files.
-succesfully connected.
+
+TCPVal will listen on the given address for incoming connections from an external
+private validator process. It will halt any operation until at least one external
+process successfully connected.
 
 The external priv_validator process will dial the address to connect to
 Tendermint, and then Tendermint will send requests on the ensuing connection to
@@ -21,6 +24,9 @@ but the Tendermint process makes all requests. In a later stage we're going to
 support multiple validators for fault tolerance. To prevent double signing they
 need to be synced, which is deferred to an external solution (see #1185).
 
+Conversely, IPCVal will make an outbound connection to an existing socket opened
+by the external validator process.
+
 In addition, Tendermint will provide implementations that can be run in that
 external process. These include:
 

node/node.go

@@ -148,6 +148,44 @@ type Node struct {
 	prometheusSrv    *http.Server
 }
 
+func createExternalPrivValidator(listenAddr string, logger log.Logger) (types.PrivValidator, error) {
+	protocol, address := cmn.ProtocolAndAddress(listenAddr)
+
+	var pvsc types.PrivValidator
+
+	switch (protocol) {
+	case "unix":
+		pvsc = privval.NewIPCVal(
+			logger.With("module", "privval"),
+			address,
+		)
+
+	case "tcp":
+		// TODO: persist this key so external signer
+		// can actually authenticate us
+		pvsc = privval.NewTCPVal(
+			logger.With("module", "privval"),
+			listenAddr,
+			ed25519.GenPrivKey(),
+		)
+
+	default:
+		return nil, fmt.Errorf(
+			"Error creating private validator: expected either tcp or unix "+
+			"protocols, got %s",
+			protocol,
+		)
+	}
+
+	pvServ, _ := pvsc.(cmn.Service)
+	if err := pvServ.Start(); err != nil {
+		return nil, fmt.Errorf("Error starting private validator client: %v", err)
+	}
+
+	return pvsc, nil
+
+}
+
 // NewNode returns a new, ready to go, Tendermint Node.
 func NewNode(config *cfg.Config,
 	privValidator types.PrivValidator,
@@ -220,25 +258,13 @@ func NewNode(config *cfg.Config,
 		)
 	}
 
+	if config.PrivValidatorListenAddr != "" {
 		// If an address is provided, listen on the socket for a
 		// connection from an external signing process.
-	if config.PrivValidatorListenAddr != "" {
+		privValidator, err = createExternalPrivValidator(config.PrivValidatorListenAddr, logger)
-		var (
+		if err != nil {
-			// TODO: persist this key so external signer
+			return nil, err
-			// can actually authenticate us
-			privKey = ed25519.GenPrivKey()
-			pvsc    = privval.NewTCPVal(
-				logger.With("module", "privval"),
-				config.PrivValidatorListenAddr,
-				privKey,
-			)
-		)
-
-		if err := pvsc.Start(); err != nil {
-			return nil, fmt.Errorf("Error starting private validator client: %v", err)
 		}
-
-		privValidator = pvsc
 	}
 
 	// Decide whether to fast-sync or not
@@ -600,9 +626,10 @@ func (n *Node) OnStop() {
 		}
 	}
 
-	if pvsc, ok := n.privValidator.(*privval.TCPVal); ok {
+
+	if pvsc, ok := n.privValidator.(cmn.Service); ok {
 		if err := pvsc.Stop(); err != nil {
-			n.Logger.Error("Error stopping priv validator socket client", "err", err)
+			n.Logger.Error("Error stopping priv validator client", "err", err)
 		}
 	}
 

node/node_test.go

@@ -7,19 +7,24 @@ import (
 	"syscall"
 	"testing"
 	"time"
+	"net"
 
 	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
 
 	"github.com/tendermint/tendermint/abci/example/kvstore"
 	"github.com/tendermint/tendermint/libs/log"
 	"github.com/tendermint/tendermint/p2p"
 	sm "github.com/tendermint/tendermint/state"
 	"github.com/tendermint/tendermint/version"
+	"github.com/tendermint/tendermint/crypto/ed25519"
 
 	cfg "github.com/tendermint/tendermint/config"
+	cmn "github.com/tendermint/tendermint/libs/common"
 	"github.com/tendermint/tendermint/types"
 
 	tmtime "github.com/tendermint/tendermint/types/time"
+	"github.com/tendermint/tendermint/privval"
 )
 
 func TestNodeStartStop(t *testing.T) {
@@ -113,3 +118,100 @@ func TestNodeSetAppVersion(t *testing.T) {
 	// check version is set in node info
 	assert.Equal(t, n.nodeInfo.(p2p.DefaultNodeInfo).ProtocolVersion.App, appVersion)
 }
+
+func TestNodeSetPrivValTCP(t *testing.T) {
+	addr := "tcp://" + testFreeAddr(t)
+
+	rs := privval.NewRemoteSigner(
+		log.TestingLogger(),
+		cmn.RandStr(12),
+		addr,
+		types.NewMockPV(),
+		ed25519.GenPrivKey(),
+	)
+	privval.RemoteSignerConnDeadline(5 * time.Millisecond)(rs)
+	privval.RemoteSignerConnRetries(1e6)(rs)
+
+	config := cfg.ResetTestRoot("node_priv_val_tcp_test")
+	config.BaseConfig.PrivValidatorListenAddr = addr
+
+	// kick off remote signer routine, and then start TM.
+	go func(rs *privval.RemoteSigner) {
+		rs.Start()
+		defer rs.Stop()
+		time.Sleep(100 * time.Millisecond)
+	}(rs)
+
+	n, err := DefaultNewNode(config, log.TestingLogger())
+
+	assert.NoError(t, err, "expected no err on DefaultNewNode")
+
+	assert.IsType(t, &privval.TCPVal{}, n.PrivValidator())
+}
+
+func TestNodeSetPrivValTCPNoPrefix(t *testing.T) {
+	addr := "tcp://" + testFreeAddr(t)
+
+	rs := privval.NewRemoteSigner(
+		log.TestingLogger(),
+		cmn.RandStr(12),
+		addr,
+		types.NewMockPV(),
+		ed25519.GenPrivKey(),
+	)
+	privval.RemoteSignerConnDeadline(5 * time.Millisecond)(rs)
+	privval.RemoteSignerConnRetries(1e6)(rs)
+	config := cfg.ResetTestRoot("node_priv_val_tcp_test")
+	config.BaseConfig.PrivValidatorListenAddr = addr
+
+	// kick off remote signer routine, and then start TM.
+	go func(rs *privval.RemoteSigner) {
+		rs.Start()
+		defer rs.Stop()
+		time.Sleep(100 * time.Millisecond)
+	}(rs)
+
+	n, err := DefaultNewNode(config, log.TestingLogger())
+
+	assert.NoError(t, err, "expected no err on DefaultNewNode")
+	assert.IsType(t, &privval.TCPVal{}, n.PrivValidator())
+}
+
+func TestNodeSetPrivValIPC(t *testing.T) {
+	tmpfile := "/tmp/kms." + cmn.RandStr(6) + ".sock"
+	defer os.Remove(tmpfile) // clean up
+	addr := "unix://" + tmpfile
+
+	rs := privval.NewIPCRemoteSigner(
+		log.TestingLogger(),
+		cmn.RandStr(12),
+		tmpfile,
+		types.NewMockPV(),
+	)
+
+	privval.IPCRemoteSignerConnDeadline(3 * time.Second)(rs)
+
+	// kick off remote signer routine, and then start TM.
+	go func(rs *privval.IPCRemoteSigner) {
+		rs.Start()
+		defer rs.Stop()
+		time.Sleep(500 * time.Millisecond)
+	}(rs)
+
+	config := cfg.ResetTestRoot("node_priv_val_tcp_test")
+	config.BaseConfig.PrivValidatorListenAddr = addr
+	n, err := DefaultNewNode(config, log.TestingLogger())
+
+	assert.NoError(t, err, "expected no err on DefaultNewNode")
+	assert.IsType(t, &privval.IPCVal{}, n.PrivValidator())
+}
+
+
+// testFreeAddr claims a free port so we don't block on listener being ready.
+func testFreeAddr(t *testing.T) string {
+	ln, err := net.Listen("tcp", "127.0.0.1:0")
+	require.NoError(t, err)
+	defer ln.Close()
+
+	return fmt.Sprintf("127.0.0.1:%d", ln.Addr().(*net.TCPAddr).Port)
+}