From 323769bf1aefb6aced651794514fdc65649efa0b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rados=C5=82aw=20Kapka?= Date: Thu, 3 Dec 2020 01:18:15 +0100 Subject: [PATCH] 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 Co-authored-by: prylabs-bulldozer[bot] <58059840+prylabs-bulldozer[bot]@users.noreply.github.com> --- validator/accounts/accounts_list_test.go | 6 +- validator/accounts/cmd_wallet.go | 2 + validator/accounts/prompt/prompt.go | 35 ++++++---- validator/accounts/wallet_create_test.go | 1 + validator/accounts/wallet_edit_test.go | 2 + validator/flags/flags.go | 6 ++ validator/keymanager/remote/keymanager.go | 70 ++++++++++++------- .../keymanager/remote/keymanager_test.go | 27 +++++-- 8 files changed, 105 insertions(+), 44 deletions(-) diff --git a/validator/accounts/accounts_list_test.go b/validator/accounts/accounts_list_test.go index 54e39efe4c..dc3a35c575 100644 --- a/validator/accounts/accounts_list_test.go +++ b/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 diff --git a/validator/accounts/cmd_wallet.go b/validator/accounts/cmd_wallet.go index 5dcd6835e7..98864d9566 100644 --- a/validator/accounts/cmd_wallet.go +++ b/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, diff --git a/validator/accounts/prompt/prompt.go b/validator/accounts/prompt/prompt.go index 0bcccc72ca..65b5676b55 100644 --- a/validator/accounts/prompt/prompt.go +++ b/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, diff --git a/validator/accounts/wallet_create_test.go b/validator/accounts/wallet_create_test.go index 49d4d0f131..a40c1fc96d 100644 --- a/validator/accounts/wallet_create_test.go +++ b/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", diff --git a/validator/accounts/wallet_edit_test.go b/validator/accounts/wallet_edit_test.go index 442e4788f8..1b833cf968 100644 --- a/validator/accounts/wallet_edit_test.go +++ b/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", diff --git a/validator/flags/flags.go b/validator/flags/flags.go index 3178cea36b..7e3637af6e 100644 --- a/validator/flags/flags.go +++ b/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{ diff --git a/validator/keymanager/remote/keymanager.go b/validator/keymanager/remote/keymanager.go index 666f0d3013..18581c5c63 100644 --- a/validator/keymanager/remote/keymanager.go +++ b/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, ) diff --git a/validator/keymanager/remote/keymanager_test.go b/validator/keymanager/remote/keymanager_test.go index 32e4000c71..3ea26c081e 100644 --- a/validator/keymanager/remote/keymanager_test.go +++ b/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)