Make TLS connections to a remote wallet non-mandatory (#7953)

* disable-remote-signer-tls flag

* use flag in edit-config

* send requests without TLS

* change warning message

* fix account list output test

Co-authored-by: Preston Van Loon <preston@prysmaticlabs.com>
Co-authored-by: prylabs-bulldozer[bot] <58059840+prylabs-bulldozer[bot]@users.noreply.github.com>

validator/accounts/accounts_list_test.go

@@ -381,6 +381,7 @@ func TestListAccounts_RemoteKeymanager(t *testing.T) {
 		publicKeys: pubKeys,
 		opts: &remote.KeymanagerOpts{
 			RemoteCertificate: &remote.CertificateConfig{
+				RequireTls:     true,
 				ClientCertPath: "/tmp/client.crt",
 				ClientKeyPath:  "/tmp/client.key",
 				CACertPath:     "/tmp/ca.crt",
@@ -407,6 +408,7 @@ func TestListAccounts_RemoteKeymanager(t *testing.T) {
 
 		Configuration options
 		Remote gRPC address: localhost:4000
+		Require TLS: true
 		Client cert path: /tmp/client.crt
 		Client key path: /tmp/client.key
 		CA cert path: /tmp/ca.crt
@@ -424,9 +426,9 @@ func TestListAccounts_RemoteKeymanager(t *testing.T) {
 	*/
 
 	// Expected output format definition
-	const prologLength = 10
+	const prologLength = 11
 	const configOffset = 4
-	const configLength = 4
+	const configLength = 5
 	const accountLength = 4
 	const nameOffset = 1
 	const keyOffset = 2

validator/accounts/cmd_wallet.go

@@ -22,6 +22,7 @@ var WalletCommands = &cli.Command{
 				flags.WalletDirFlag,
 				flags.KeymanagerKindFlag,
 				flags.GrpcRemoteAddressFlag,
+				flags.DisableRemoteSignerTlsFlag,
 				flags.RemoteSignerCertPathFlag,
 				flags.RemoteSignerKeyPathFlag,
 				flags.RemoteSignerCACertPathFlag,
@@ -53,6 +54,7 @@ var WalletCommands = &cli.Command{
 			Flags: cmd.WrapFlags([]cli.Flag{
 				flags.WalletDirFlag,
 				flags.GrpcRemoteAddressFlag,
+				flags.DisableRemoteSignerTlsFlag,
 				flags.RemoteSignerCertPathFlag,
 				flags.RemoteSignerKeyPathFlag,
 				flags.RemoteSignerCACertPathFlag,

validator/accounts/prompt/prompt.go

@@ -69,6 +69,7 @@ func InputDirectory(cliCtx *cli.Context, promptText string, flag *cli.StringFlag
 // InputRemoteKeymanagerConfig via the cli.
 func InputRemoteKeymanagerConfig(cliCtx *cli.Context) (*remote.KeymanagerOpts, error) {
 	addr := cliCtx.String(flags.GrpcRemoteAddressFlag.Name)
+	requireTls := !cliCtx.Bool(flags.DisableRemoteSignerTlsFlag.Name)
 	crt := cliCtx.String(flags.RemoteSignerCertPathFlag.Name)
 	key := cliCtx.String(flags.RemoteSignerKeyPathFlag.Name)
 	ca := cliCtx.String(flags.RemoteSignerCACertPathFlag.Name)
@@ -83,7 +84,7 @@ func InputRemoteKeymanagerConfig(cliCtx *cli.Context) (*remote.KeymanagerOpts, e
 			return nil, err
 		}
 	}
-	if crt == "" {
+	if requireTls && crt == "" {
 		crt, err = promptutil.ValidatePrompt(
 			os.Stdin,
 			"Path to TLS crt (such as /path/to/client.crt)",
@@ -92,7 +93,7 @@ func InputRemoteKeymanagerConfig(cliCtx *cli.Context) (*remote.KeymanagerOpts, e
 			return nil, err
 		}
 	}
-	if key == "" {
+	if requireTls && key == "" {
 		key, err = promptutil.ValidatePrompt(
 			os.Stdin,
 			"Path to TLS key (such as /path/to/client.key)",
@@ -101,7 +102,7 @@ func InputRemoteKeymanagerConfig(cliCtx *cli.Context) (*remote.KeymanagerOpts, e
 			return nil, err
 		}
 	}
-	if ca == "" {
+	if requireTls && ca == "" {
 		ca, err = promptutil.ValidatePrompt(
 			os.Stdin,
 			"Path to certificate authority (CA) crt (such as /path/to/ca.crt)",
@@ -110,20 +111,30 @@ func InputRemoteKeymanagerConfig(cliCtx *cli.Context) (*remote.KeymanagerOpts, e
 			return nil, err
 		}
 	}
-	crtPath, err := fileutil.ExpandPath(strings.TrimRight(crt, "\r\n"))
-	if err != nil {
-		return nil, errors.Wrapf(err, "could not determine absolute path for %s", crt)
+
+	crtPath, keyPath, caPath := "", "", ""
+	if crt != "" {
+		crtPath, err = fileutil.ExpandPath(strings.TrimRight(crt, "\r\n"))
+		if err != nil {
+			return nil, errors.Wrapf(err, "could not determine absolute path for %s", crt)
+		}
 	}
-	keyPath, err := fileutil.ExpandPath(strings.TrimRight(key, "\r\n"))
-	if err != nil {
-		return nil, errors.Wrapf(err, "could not determine absolute path for %s", crt)
+	if key != "" {
+		keyPath, err = fileutil.ExpandPath(strings.TrimRight(key, "\r\n"))
+		if err != nil {
+			return nil, errors.Wrapf(err, "could not determine absolute path for %s", crt)
+		}
 	}
-	caPath, err := fileutil.ExpandPath(strings.TrimRight(ca, "\r\n"))
-	if err != nil {
-		return nil, errors.Wrapf(err, "could not determine absolute path for %s", crt)
+	if ca != "" {
+		caPath, err = fileutil.ExpandPath(strings.TrimRight(ca, "\r\n"))
+		if err != nil {
+			return nil, errors.Wrapf(err, "could not determine absolute path for %s", crt)
+		}
 	}
+
 	newCfg := &remote.KeymanagerOpts{
 		RemoteCertificate: &remote.CertificateConfig{
+			RequireTls:     requireTls,
 			ClientCertPath: crtPath,
 			ClientKeyPath:  keyPath,
 			CACertPath:     caPath,

validator/accounts/wallet_create_test.go

@@ -221,6 +221,7 @@ func TestCreateWallet_Remote(t *testing.T) {
 	walletDir, _, walletPasswordFile := setupWalletAndPasswordsDir(t)
 	wantCfg := &remote.KeymanagerOpts{
 		RemoteCertificate: &remote.CertificateConfig{
+			RequireTls:     true,
 			ClientCertPath: "/tmp/client.crt",
 			ClientKeyPath:  "/tmp/client.key",
 			CACertPath:     "/tmp/ca.crt",

validator/accounts/wallet_edit_test.go

@@ -30,6 +30,7 @@ func TestEditWalletConfiguration(t *testing.T) {
 
 	originalCfg := &remote.KeymanagerOpts{
 		RemoteCertificate: &remote.CertificateConfig{
+			RequireTls:     true,
 			ClientCertPath: "/tmp/a.crt",
 			ClientKeyPath:  "/tmp/b.key",
 			CACertPath:     "/tmp/c.crt",
@@ -42,6 +43,7 @@ func TestEditWalletConfiguration(t *testing.T) {
 
 	wantCfg := &remote.KeymanagerOpts{
 		RemoteCertificate: &remote.CertificateConfig{
+			RequireTls:     true,
 			ClientCertPath: "/tmp/client.crt",
 			ClientKeyPath:  "/tmp/client.key",
 			CACertPath:     "/tmp/ca.crt",

validator/flags/flags.go

@@ -237,6 +237,12 @@ var (
 		Usage: "Host:port of a gRPC server for a remote keymanager",
 		Value: "",
 	}
+	// DisableRemoteSignerTlsFlag disables TLS when connecting to a remote signer.
+	DisableRemoteSignerTlsFlag = &cli.BoolFlag{
+		Name:  "disable-remote-signer-tls",
+		Usage: "Disables TLS when connecting to a remote signer. (WARNING! This will result in insecure requests!)",
+		Value: false,
+	}
 	// RemoteSignerCertPathFlag defines the path to a client.crt file for a wallet to connect to
 	// a secure signer via TLS and gRPC.
 	RemoteSignerCertPathFlag = &cli.StringFlag{

validator/keymanager/remote/keymanager.go

@@ -41,6 +41,7 @@ type KeymanagerOpts struct {
 // certificate authority certs, client certs, and client keys
 // for TLS gRPC connections.
 type CertificateConfig struct {
+	RequireTls     bool   `json:"require_tls"`
 	ClientCertPath string `json:"crt_path"`
 	ClientKeyPath  string `json:"key_path"`
 	CACertPath     string `json:"ca_crt_path"`
@@ -64,43 +65,53 @@ type Keymanager struct {
 func NewKeymanager(_ context.Context, cfg *SetupConfig) (*Keymanager, error) {
 	// Load the client certificates.
 	if cfg.Opts.RemoteCertificate == nil {
-		return nil, errors.New("certificates are required")
-	}
-	if cfg.Opts.RemoteCertificate.ClientCertPath == "" {
-		return nil, errors.New("client certificate is required")
-	}
-	if cfg.Opts.RemoteCertificate.ClientKeyPath == "" {
-		return nil, errors.New("client key is required")
-	}
-	clientPair, err := tls.LoadX509KeyPair(cfg.Opts.RemoteCertificate.ClientCertPath, cfg.Opts.RemoteCertificate.ClientKeyPath)
-	if err != nil {
-		return nil, errors.Wrap(err, "failed to obtain client's certificate and/or key")
+		return nil, errors.New("certificate configuration is missing")
 	}
 
-	// Load the CA for the server certificate if present.
-	cp := x509.NewCertPool()
-	if cfg.Opts.RemoteCertificate.CACertPath != "" {
-		serverCA, err := ioutil.ReadFile(cfg.Opts.RemoteCertificate.CACertPath)
+	var clientCreds credentials.TransportCredentials
+
+	if cfg.Opts.RemoteCertificate.RequireTls {
+		if cfg.Opts.RemoteCertificate.ClientCertPath == "" {
+			return nil, errors.New("client certificate is required")
+		}
+		if cfg.Opts.RemoteCertificate.ClientKeyPath == "" {
+			return nil, errors.New("client key is required")
+		}
+		clientPair, err := tls.LoadX509KeyPair(cfg.Opts.RemoteCertificate.ClientCertPath, cfg.Opts.RemoteCertificate.ClientKeyPath)
 		if err != nil {
-			return nil, errors.Wrap(err, "failed to obtain server's CA certificate")
+			return nil, errors.Wrap(err, "failed to obtain client's certificate and/or key")
 		}
-		if !cp.AppendCertsFromPEM(serverCA) {
-			return nil, errors.Wrap(err, "failed to add server's CA certificate to pool")
-		}
-	}
 
-	tlsCfg := &tls.Config{
-		Certificates: []tls.Certificate{clientPair},
-		RootCAs:      cp,
+		// Load the CA for the server certificate if present.
+		cp := x509.NewCertPool()
+		if cfg.Opts.RemoteCertificate.CACertPath != "" {
+			serverCA, err := ioutil.ReadFile(cfg.Opts.RemoteCertificate.CACertPath)
+			if err != nil {
+				return nil, errors.Wrap(err, "failed to obtain server's CA certificate")
+			}
+			if !cp.AppendCertsFromPEM(serverCA) {
+				return nil, errors.Wrap(err, "failed to add server's CA certificate to pool")
+			}
+		}
+
+		tlsCfg := &tls.Config{
+			Certificates: []tls.Certificate{clientPair},
+			RootCAs:      cp,
+		}
+		clientCreds = credentials.NewTLS(tlsCfg)
 	}
-	clientCreds := credentials.NewTLS(tlsCfg)
 
 	grpcOpts := []grpc.DialOption{
-		// Require TLS with client certificate.
-		grpc.WithTransportCredentials(clientCreds),
 		// Receive large messages without erroring.
 		grpc.WithDefaultCallOptions(grpc.MaxCallRecvMsgSize(cfg.MaxMessageSize)),
 	}
+	if cfg.Opts.RemoteCertificate.RequireTls {
+		// Require TLS with client certificate.
+		grpcOpts = append(grpcOpts, grpc.WithTransportCredentials(clientCreds))
+	} else {
+		grpcOpts = append(grpcOpts, grpc.WithInsecure())
+	}
+
 	conn, err := grpc.Dial(cfg.Opts.RemoteAddr, grpcOpts...)
 	if err != nil {
 		return nil, errors.New("failed to connect to remote wallet")
@@ -147,6 +158,13 @@ func (opts *KeymanagerOpts) String() string {
 		log.Error(err)
 		return ""
 	}
+	strRequireTls := fmt.Sprintf(
+		"%s: %t\n", au.BrightMagenta("Require TLS"), opts.RemoteCertificate.RequireTls,
+	)
+	if _, err := b.WriteString(strRequireTls); err != nil {
+		log.Error(err)
+		return ""
+	}
 	strCrt := fmt.Sprintf(
 		"%s: %s\n", au.BrightMagenta("Client cert path"), opts.RemoteCertificate.ClientCertPath,
 	)

validator/keymanager/remote/keymanager_test.go

@@ -85,12 +85,14 @@ func TestNewRemoteKeymanager(t *testing.T) {
 			opts: &KeymanagerOpts{
 				RemoteCertificate: nil,
 			},
-			err: "certificates are required",
+			err: "certificate configuration is missing",
 		},
 		{
 			name: "NoClientCertificate",
 			opts: &KeymanagerOpts{
-				RemoteCertificate: &CertificateConfig{},
+				RemoteCertificate: &CertificateConfig{
+					RequireTls: true,
+				},
 			},
 			err: "client certificate is required",
 		},
@@ -98,6 +100,7 @@ func TestNewRemoteKeymanager(t *testing.T) {
 			name: "NoClientKey",
 			opts: &KeymanagerOpts{
 				RemoteCertificate: &CertificateConfig{
+					RequireTls:     true,
 					ClientCertPath: "/foo/client.crt",
 					ClientKeyPath:  "",
 				},
@@ -108,6 +111,7 @@ func TestNewRemoteKeymanager(t *testing.T) {
 			name: "MissingClientKey",
 			opts: &KeymanagerOpts{
 				RemoteCertificate: &CertificateConfig{
+					RequireTls:     true,
 					ClientCertPath: "/foo/client.crt",
 					ClientKeyPath:  "/foo/client.key",
 					CACertPath:     "",
@@ -120,7 +124,9 @@ func TestNewRemoteKeymanager(t *testing.T) {
 			clientCert: `bad`,
 			clientKey:  validClientKey,
 			opts: &KeymanagerOpts{
-				RemoteCertificate: &CertificateConfig{},
+				RemoteCertificate: &CertificateConfig{
+					RequireTls: true,
+				},
 			},
 			err: "failed to obtain client's certificate and/or key: tls: failed to find any PEM data in certificate input",
 		},
@@ -129,7 +135,9 @@ func TestNewRemoteKeymanager(t *testing.T) {
 			clientCert: validClientCert,
 			clientKey:  `bad`,
 			opts: &KeymanagerOpts{
-				RemoteCertificate: &CertificateConfig{},
+				RemoteCertificate: &CertificateConfig{
+					RequireTls: true,
+				},
 			},
 			err: "failed to obtain client's certificate and/or key: tls: failed to find any PEM data in key input",
 		},
@@ -139,6 +147,7 @@ func TestNewRemoteKeymanager(t *testing.T) {
 			clientKey:  validClientKey,
 			opts: &KeymanagerOpts{
 				RemoteCertificate: &CertificateConfig{
+					RequireTls: true,
 					CACertPath: `bad`,
 				},
 			},
@@ -180,6 +189,16 @@ func TestNewRemoteKeymanager(t *testing.T) {
 	}
 }
 
+func TestNewRemoteKeymanager_TlsDisabled(t *testing.T) {
+	opts := &KeymanagerOpts{
+		RemoteCertificate: &CertificateConfig{
+			RequireTls: false,
+		},
+	}
+	_, err := NewKeymanager(context.Background(), &SetupConfig{Opts: opts, MaxMessageSize: 1})
+	assert.NoError(t, err)
+}
+
 func TestRemoteKeymanager_Sign(t *testing.T) {
 	ctrl := gomock.NewController(t)
 	m := mock.NewMockRemoteSignerClient(ctrl)