From 8c7cb70263cc1a789b36bc51032d6a9f713bfb95 Mon Sep 17 00:00:00 2001 From: Matthew Sykes Date: Mon, 1 Mar 2021 22:08:12 -0500 Subject: [PATCH] comm: move Certificate to SecureOptions#ClientCert Signed-off-by: Matthew Sykes --- core/deliverservice/deliveryclient.go | 6 +++++- internal/peer/common/common.go | 29 +++++++++++++++++++++++++-- internal/peer/common/ordererclient.go | 21 +++++++------------ internal/peer/common/peerclient.go | 29 ++++++++++----------------- internal/pkg/comm/client.go | 10 --------- internal/pkg/comm/client_test.go | 10 ++++----- internal/pkg/comm/config.go | 15 +++++++++----- 7 files changed, 65 insertions(+), 55 deletions(-) diff --git a/core/deliverservice/deliveryclient.go b/core/deliverservice/deliveryclient.go index 69b81e31037..1e25a003a16 100644 --- a/core/deliverservice/deliveryclient.go +++ b/core/deliverservice/deliveryclient.go @@ -140,7 +140,11 @@ func (d *deliverServiceImpl) StartDeliverForChannel(chainID string, ledgerInfo b } if d.conf.DeliverServiceConfig.SecOpts.RequireClientCert { - dc.TLSCertHash = util.ComputeSHA256(d.conf.DeliverGRPCClient.Certificate().Certificate[0]) + cert, err := d.conf.DeliverServiceConfig.SecOpts.ClientCertificate() + if err != nil { + return fmt.Errorf("failed to access client TLS configuration: %w", err) + } + dc.TLSCertHash = util.ComputeSHA256(cert.Certificate[0]) } d.blockProviders[chainID] = dc diff --git a/internal/peer/common/common.go b/internal/peer/common/common.go index 8e5469a885d..b220b3f3eb0 100644 --- a/internal/peer/common/common.go +++ b/internal/peer/common/common.go @@ -83,8 +83,33 @@ var ( type CommonClient struct { *comm.GRPCClient - Address string - sn string + clientConfig comm.ClientConfig + address string + sn string +} + +func newCommonClient(address, override string, clientConfig comm.ClientConfig) (*CommonClient, error) { + gClient, err := comm.NewGRPCClient(clientConfig) + if err != nil { + return nil, err + } + return &CommonClient{ + GRPCClient: gClient, + clientConfig: clientConfig, + address: address, + sn: override, + }, nil +} + +func (cc *CommonClient) Certificate() tls.Certificate { + if !cc.clientConfig.SecOpts.RequireClientCert { + return tls.Certificate{} + } + cert, err := cc.clientConfig.SecOpts.ClientCertificate() + if err != nil { + panic(err) + } + return cert } func init() { diff --git a/internal/peer/common/ordererclient.go b/internal/peer/common/ordererclient.go index 852cfab8c7f..2aa8af85182 100644 --- a/internal/peer/common/ordererclient.go +++ b/internal/peer/common/ordererclient.go @@ -17,7 +17,7 @@ import ( // OrdererClient represents a client for communicating with an ordering // service type OrdererClient struct { - CommonClient + *CommonClient } // NewOrdererClientFromEnv creates an instance of an OrdererClient from the @@ -27,25 +27,18 @@ func NewOrdererClientFromEnv() (*OrdererClient, error) { if err != nil { return nil, errors.WithMessage(err, "failed to load config for OrdererClient") } - gClient, err := comm.NewGRPCClient(clientConfig) + cc, err := newCommonClient(address, override, clientConfig) if err != nil { return nil, errors.WithMessage(err, "failed to create OrdererClient from config") } - oClient := &OrdererClient{ - CommonClient: CommonClient{ - GRPCClient: gClient, - Address: address, - sn: override, - }, - } - return oClient, nil + return &OrdererClient{CommonClient: cc}, nil } // Broadcast returns a broadcast client for the AtomicBroadcast service func (oc *OrdererClient) Broadcast() (ab.AtomicBroadcast_BroadcastClient, error) { - conn, err := oc.CommonClient.NewConnection(oc.Address, comm.ServerNameOverride(oc.sn)) + conn, err := oc.CommonClient.NewConnection(oc.address, comm.ServerNameOverride(oc.sn)) if err != nil { - return nil, errors.WithMessagef(err, "orderer client failed to connect to %s", oc.Address) + return nil, errors.WithMessagef(err, "orderer client failed to connect to %s", oc.address) } // TODO: check to see if we should actually handle error before returning return ab.NewAtomicBroadcastClient(conn).Broadcast(context.TODO()) @@ -53,9 +46,9 @@ func (oc *OrdererClient) Broadcast() (ab.AtomicBroadcast_BroadcastClient, error) // Deliver returns a deliver client for the AtomicBroadcast service func (oc *OrdererClient) Deliver() (ab.AtomicBroadcast_DeliverClient, error) { - conn, err := oc.CommonClient.NewConnection(oc.Address, comm.ServerNameOverride(oc.sn)) + conn, err := oc.CommonClient.NewConnection(oc.address, comm.ServerNameOverride(oc.sn)) if err != nil { - return nil, errors.WithMessagef(err, "orderer client failed to connect to %s", oc.Address) + return nil, errors.WithMessagef(err, "orderer client failed to connect to %s", oc.address) } // TODO: check to see if we should actually handle error before returning return ab.NewAtomicBroadcastClient(conn).Deliver(context.TODO()) diff --git a/internal/peer/common/peerclient.go b/internal/peer/common/peerclient.go index 9212f13f10a..7c1f3cf6938 100644 --- a/internal/peer/common/peerclient.go +++ b/internal/peer/common/peerclient.go @@ -20,7 +20,7 @@ import ( // PeerClient represents a client for communicating with a peer type PeerClient struct { - CommonClient + *CommonClient } // NewPeerClientFromEnv creates an instance of a PeerClient from the global @@ -78,34 +78,27 @@ func NewPeerClientForAddress(address, tlsRootCertFile string) (*PeerClient, erro func newPeerClientForClientConfig(address, override string, clientConfig comm.ClientConfig) (*PeerClient, error) { // set the default keepalive options to match the server clientConfig.KaOpts = comm.DefaultKeepaliveOptions - gClient, err := comm.NewGRPCClient(clientConfig) + cc, err := newCommonClient(address, override, clientConfig) if err != nil { return nil, errors.WithMessage(err, "failed to create PeerClient from config") } - pClient := &PeerClient{ - CommonClient: CommonClient{ - GRPCClient: gClient, - Address: address, - sn: override, - }, - } - return pClient, nil + return &PeerClient{CommonClient: cc}, nil } // Endorser returns a client for the Endorser service func (pc *PeerClient) Endorser() (pb.EndorserClient, error) { - conn, err := pc.CommonClient.NewConnection(pc.Address, comm.ServerNameOverride(pc.sn)) + conn, err := pc.CommonClient.NewConnection(pc.address, comm.ServerNameOverride(pc.sn)) if err != nil { - return nil, errors.WithMessagef(err, "endorser client failed to connect to %s", pc.Address) + return nil, errors.WithMessagef(err, "endorser client failed to connect to %s", pc.address) } return pb.NewEndorserClient(conn), nil } // Deliver returns a client for the Deliver service func (pc *PeerClient) Deliver() (pb.Deliver_DeliverClient, error) { - conn, err := pc.CommonClient.NewConnection(pc.Address, comm.ServerNameOverride(pc.sn)) + conn, err := pc.CommonClient.NewConnection(pc.address, comm.ServerNameOverride(pc.sn)) if err != nil { - return nil, errors.WithMessagef(err, "deliver client failed to connect to %s", pc.Address) + return nil, errors.WithMessagef(err, "deliver client failed to connect to %s", pc.address) } return pb.NewDeliverClient(conn).Deliver(context.TODO()) } @@ -113,9 +106,9 @@ func (pc *PeerClient) Deliver() (pb.Deliver_DeliverClient, error) { // PeerDeliver returns a client for the Deliver service for peer-specific use // cases (i.e. DeliverFiltered) func (pc *PeerClient) PeerDeliver() (pb.DeliverClient, error) { - conn, err := pc.CommonClient.NewConnection(pc.Address, comm.ServerNameOverride(pc.sn)) + conn, err := pc.CommonClient.NewConnection(pc.address, comm.ServerNameOverride(pc.sn)) if err != nil { - return nil, errors.WithMessagef(err, "deliver client failed to connect to %s", pc.Address) + return nil, errors.WithMessagef(err, "deliver client failed to connect to %s", pc.address) } return pb.NewDeliverClient(conn), nil } @@ -181,9 +174,9 @@ func GetPeerDeliverClient(address, tlsRootCertFile string) (pb.DeliverClient, er // SnapshotClient returns a client for the snapshot service func (pc *PeerClient) SnapshotClient() (pb.SnapshotClient, error) { - conn, err := pc.CommonClient.NewConnection(pc.Address, comm.ServerNameOverride(pc.sn)) + conn, err := pc.CommonClient.NewConnection(pc.address, comm.ServerNameOverride(pc.sn)) if err != nil { - return nil, errors.WithMessagef(err, "snapshot client failed to connect to %s", pc.Address) + return nil, errors.WithMessagef(err, "snapshot client failed to connect to %s", pc.address) } return pb.NewSnapshotClient(conn), nil } diff --git a/internal/pkg/comm/client.go b/internal/pkg/comm/client.go index ba76359b56e..05c5b96429b 100644 --- a/internal/pkg/comm/client.go +++ b/internal/pkg/comm/client.go @@ -41,16 +41,6 @@ func NewGRPCClient(config ClientConfig) (*GRPCClient, error) { }, nil } -// Certificate returns the tls.Certificate used to make TLS connections -// when client certificates are required by the server -func (client *GRPCClient) Certificate() tls.Certificate { - cert := tls.Certificate{} - if client.tlsConfig != nil && len(client.tlsConfig.Certificates) > 0 { - cert = client.tlsConfig.Certificates[0] - } - return cert -} - // TLSEnabled is a flag indicating whether to use TLS for client // connections func (client *GRPCClient) TLSEnabled() bool { diff --git a/internal/pkg/comm/client_test.go b/internal/pkg/comm/client_test.go index 02e88b24da4..be38d30f4ee 100644 --- a/internal/pkg/comm/client_test.go +++ b/internal/pkg/comm/client_test.go @@ -47,7 +47,6 @@ func TestNewGRPCClient_GoodConfig(t *testing.T) { config := comm.ClientConfig{} client, err := comm.NewGRPCClient(config) require.NoError(t, err) - require.Equal(t, tls.Certificate{}, client.Certificate()) require.False(t, client.TLSEnabled()) secOpts := comm.SecureOptions{ @@ -56,7 +55,6 @@ func TestNewGRPCClient_GoodConfig(t *testing.T) { config.SecOpts = secOpts client, err = comm.NewGRPCClient(config) require.NoError(t, err) - require.Equal(t, tls.Certificate{}, client.Certificate()) require.False(t, client.TLSEnabled()) secOpts = comm.SecureOptions{ @@ -76,11 +74,13 @@ func TestNewGRPCClient_GoodConfig(t *testing.T) { ServerRootCAs: [][]byte{testCerts.caPEM}, RequireClientCert: true, } + clientCert, err := secOpts.ClientCertificate() + require.NoError(t, err) + require.Equal(t, testCerts.clientCert, clientCert) config.SecOpts = secOpts client, err = comm.NewGRPCClient(config) require.NoError(t, err) require.True(t, client.TLSEnabled()) - require.Equal(t, testCerts.clientCert, client.Certificate()) } func TestNewGRPCClient_BadConfig(t *testing.T) { @@ -105,7 +105,7 @@ func TestNewGRPCClient_BadConfig(t *testing.T) { RequireClientCert: true, } _, err = comm.NewGRPCClient(config) - require.Equal(t, missing, err.Error()) + require.ErrorContains(t, err, missing) // missing cert config.SecOpts = comm.SecureOptions{ @@ -114,7 +114,7 @@ func TestNewGRPCClient_BadConfig(t *testing.T) { RequireClientCert: true, } _, err = comm.NewGRPCClient(config) - require.Equal(t, missing, err.Error()) + require.ErrorContains(t, err, missing) // bad key failed := "failed to load client certificate" diff --git a/internal/pkg/comm/config.go b/internal/pkg/comm/config.go index d32391eedcc..22860239340 100644 --- a/internal/pkg/comm/config.go +++ b/internal/pkg/comm/config.go @@ -173,11 +173,7 @@ func (so SecureOptions) TLSConfig() (*tls.Config, error) { } if so.RequireClientCert { - // make sure we have both Key and Certificate - if so.Key == nil || so.Certificate == nil { - return nil, errors.New("both Key and Certificate are required when using mutual TLS") - } - cert, err := tls.X509KeyPair(so.Certificate, so.Key) + cert, err := so.ClientCertificate() if err != nil { return nil, errors.WithMessage(err, "failed to load client certificate") } @@ -193,6 +189,15 @@ func (so SecureOptions) TLSConfig() (*tls.Config, error) { return tlsConfig, nil } +// ClientCertificate returns the client certificate that will be used +// for mutual TLS. +func (so SecureOptions) ClientCertificate() (tls.Certificate, error) { + if so.Key == nil || so.Certificate == nil { + return tls.Certificate{}, errors.New("both Key and Certificate are required when using mutual TLS") + } + return tls.X509KeyPair(so.Certificate, so.Key) +} + // KeepaliveOptions is used to set the gRPC keepalive settings for both // clients and servers type KeepaliveOptions struct {