Skip to content

Commit

Permalink
Make TLS connections to a remote wallet non-mandatory (#7953)
Browse files Browse the repository at this point in the history 
* 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>
  • Loading branch information
@rkapka @prestonvanloon @prylabs-bulldozer
3 people committed Dec 3, 2020
1 parent c51754f commit 323769b
Show file tree
Hide file tree
Showing 8 changed files with 104 additions and 43 deletions.
6 changes: 4 additions & 2 deletions validator/accounts/accounts_list_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand All @@ -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
Expand All @@ -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
Expand Down
2 changes: 2 additions & 0 deletions validator/accounts/cmd_wallet.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ var WalletCommands = &cli.Command{
flags.WalletDirFlag,
flags.KeymanagerKindFlag,
flags.GrpcRemoteAddressFlag,
flags.DisableRemoteSignerTlsFlag,
flags.RemoteSignerCertPathFlag,
flags.RemoteSignerKeyPathFlag,
flags.RemoteSignerCACertPathFlag,
Expand Down Expand Up @@ -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,
Expand Down
35 changes: 23 additions & 12 deletions validator/accounts/prompt/prompt.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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)",
Expand All @@ -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)",
Expand All @@ -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)",
Expand All @@ -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,
Expand Down
1 change: 1 addition & 0 deletions validator/accounts/wallet_create_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down
2 changes: 2 additions & 0 deletions validator/accounts/wallet_edit_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand All @@ -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",
Expand Down
6 changes: 6 additions & 0 deletions validator/flags/flags.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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{
Expand Down
68 changes: 43 additions & 25 deletions validator/keymanager/remote/keymanager.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"`
Expand All @@ -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")

// 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,
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")
Expand Down Expand Up @@ -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,
)
Expand Down
27 changes: 23 additions & 4 deletions validator/keymanager/remote/keymanager_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -85,19 +85,22 @@ 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",
},
{
name: "NoClientKey",
opts: &KeymanagerOpts{
RemoteCertificate: &CertificateConfig{
RequireTls: true,
ClientCertPath: "/foo/client.crt",
ClientKeyPath: "",
},
Expand All @@ -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: "",
Expand All @@ -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",
},
Expand All @@ -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",
},
Expand All @@ -139,6 +147,7 @@ func TestNewRemoteKeymanager(t *testing.T) {
clientKey: validClientKey,
opts: &KeymanagerOpts{
RemoteCertificate: &CertificateConfig{
RequireTls: true,
CACertPath: `bad`,
},
},
Expand Down Expand Up @@ -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)
Expand Down

0 comments on commit 323769b

Please sign in to comment.