Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

FAB-18192 Fixed TLS certs validation for consenters. #1888

Merged
merged 8 commits into from
Oct 6, 2020
40 changes: 9 additions & 31 deletions integration/raft/config_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@ package raft

import (
"bytes"
"crypto/x509"
"encoding/pem"
"fmt"
"io/ioutil"
"os"
Expand Down Expand Up @@ -207,7 +209,7 @@ var _ = Describe("EndToEnd reconfiguration and onboarding", func() {
exitCode := network.CreateChannelExitCode(channel, orderer, org1Peer0, org1Peer0, org2Peer0, orderer)
Expect(exitCode).NotTo(Equal(0))
Consistently(process.Wait).ShouldNot(Receive()) // malformed tx should not crash orderer
Expect(runner.Err()).To(gbytes.Say(`invalid new config metdadata: ElectionTick \(10\) must be greater than HeartbeatTick \(10\)`))
Expect(runner.Err()).To(gbytes.Say(`invalid new config metadata: ElectionTick \(10\) must be greater than HeartbeatTick \(10\)`))

By("Submitting channel config update with illegal value")
channel = network.SystemChannel.Name
Expand Down Expand Up @@ -235,7 +237,7 @@ var _ = Describe("EndToEnd reconfiguration and onboarding", func() {

sess := nwo.UpdateOrdererConfigSession(network, orderer, channel, config, updatedConfig, org1Peer0, orderer)
Eventually(sess, network.EventuallyTimeout).Should(gexec.Exit(1))
Expect(sess.Err).To(gbytes.Say(`invalid new config metdadata: ElectionTick \(10\) must be greater than HeartbeatTick \(10\)`))
Expect(sess.Err).To(gbytes.Say(`invalid new config metadata: ElectionTick \(10\) must be greater than HeartbeatTick \(10\)`))
})
})

Expand Down Expand Up @@ -389,41 +391,17 @@ var _ = Describe("EndToEnd reconfiguration and onboarding", func() {
By("Waiting for orderer3 to see the leader")
findLeader([]*ginkgomon.Runner{ordererRunners[2]})

By("Removing orderer3's TLS root CA certificate")
nwo.UpdateOrdererMSP(network, peer, orderer, "systemchannel", "OrdererOrg", func(config msp.FabricMSPConfig) msp.FabricMSPConfig {
config.TlsRootCerts = config.TlsRootCerts[:len(config.TlsRootCerts)-1]
return config
})

By("Killing orderer3")
o3Proc := ordererProcesses[2]
o3Proc.Signal(syscall.SIGKILL)
Eventually(o3Proc.Wait(), network.EventuallyTimeout).Should(Receive(MatchError("exit status 137")))

By("Restarting orderer3")
o3Runner := network.OrdererRunner(orderer3)
ordererRunners[2] = o3Runner
o3Proc = ifrit.Invoke(o3Runner)
Eventually(o3Proc.Ready(), network.EventuallyTimeout).Should(BeClosed())
ordererProcesses[2] = o3Proc

By("Ensuring TLS handshakes fail with the other orderers")
for i, oRunner := range ordererRunners {
if i < 2 {
Eventually(oRunner.Err(), network.EventuallyTimeout).Should(gbytes.Say("TLS handshake failed with error tls: failed to verify client certificate"))
continue
}
Eventually(oRunner.Err(), network.EventuallyTimeout).Should(gbytes.Say("TLS handshake failed with error remote error: tls: bad certificate"))
Eventually(oRunner.Err(), network.EventuallyTimeout).Should(gbytes.Say("Suspecting our own eviction from the channel"))
}

By("Attemping to add a consenter with invalid certs")
// create new certs that are not in the channel config
ca, err := tlsgen.NewCA()
Expect(err).NotTo(HaveOccurred())
client, err := ca.NewClientCertKeyPair()
Expect(err).NotTo(HaveOccurred())

newConsenterCertPem, _ := pem.Decode(client.Cert)
newConsenterCert, err := x509.ParseCertificate(newConsenterCertPem.Bytes)
Expect(err).NotTo(HaveOccurred())

current, updated := consenterAdder(
network,
peer,
Expand All @@ -438,7 +416,7 @@ var _ = Describe("EndToEnd reconfiguration and onboarding", func() {
)
sess = nwo.UpdateOrdererConfigSession(network, orderer, network.SystemChannel.Name, current, updated, peer, orderer)
Eventually(sess, network.EventuallyTimeout).Should(gexec.Exit(1))
Expect(sess.Err).To(gbytes.Say(fmt.Sprintf("BAD_REQUEST -- error applying config update to existing channel 'systemchannel': consensus metadata update for channel config update is invalid: verifying tls client cert with serial number %d: x509: certificate signed by unknown authority", client.TLSCert.SerialNumber)))
Expect(sess.Err).To(gbytes.Say(fmt.Sprintf("BAD_REQUEST -- error applying config update to existing channel 'systemchannel': consensus metadata update for channel config update is invalid: invalid new config metadata: verifying tls client cert with serial number %d: x509: certificate signed by unknown authority", newConsenterCert.SerialNumber)))
})
})

Expand Down
32 changes: 12 additions & 20 deletions orderer/common/msgprocessor/mocks/metadata_validator.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion orderer/common/msgprocessor/systemchannel.go
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ type ChannelConfigTemplator interface {

// MetadataValidator can be used to validate updates to the consensus-specific metadata.
type MetadataValidator interface {
ValidateConsensusMetadata(oldMetadata, newMetadata []byte, newChannel bool) error
ValidateConsensusMetadata(oldOrdererConfig, newOrdererConfig channelconfig.Orderer, newChannel bool) error
}

// SystemChannel implements the Processor interface for the system channel.
Expand Down
8 changes: 3 additions & 5 deletions orderer/common/msgprocessor/systemchannelfilter.go
Original file line number Diff line number Diff line change
Expand Up @@ -151,23 +151,21 @@ func (scf *SystemChainFilter) authorizeAndInspect(configTx *cb.Envelope) error {
return errors.Wrap(err, "new bundle invalid")
}

oc, ok := bundle.OrdererConfig()
ordererConfig, ok := bundle.OrdererConfig()
if !ok {
return errors.New("config is missing orderer group")
}
newMetadata := oc.ConsensusMetadata()

oldOrdererConfig, ok := scf.support.OrdererConfig()
if !ok {
logger.Panic("old config is missing orderer group")
}
oldMetadata := oldOrdererConfig.ConsensusMetadata()

if err = scf.validator.ValidateConsensusMetadata(oldMetadata, newMetadata, true); err != nil {
if err = scf.validator.ValidateConsensusMetadata(oldOrdererConfig, ordererConfig, true); err != nil {
return errors.Wrap(err, "consensus metadata update for channel creation is invalid")
}

if err = oc.Capabilities().Supported(); err != nil {
if err = ordererConfig.Capabilities().Supported(); err != nil {
return errors.Wrap(err, "config update is not compatible")
}

Expand Down
4 changes: 2 additions & 2 deletions orderer/common/msgprocessor/systemchannelfilter_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -455,6 +455,6 @@ func TestFailedMetadataValidation(t *testing.T) {
require.Equal(t, 1, mv.ValidateConsensusMetadataCallCount())
om, nm, nc := mv.ValidateConsensusMetadataArgsForCall(0)
require.True(t, nc)
require.Equal(t, []byte("old consensus metadata"), om)
require.Equal(t, []byte("new consensus metadata"), nm)
require.Equal(t, []byte("old consensus metadata"), om.ConsensusMetadata())
require.Equal(t, []byte("new consensus metadata"), nm.ConsensusMetadata())
}
4 changes: 1 addition & 3 deletions orderer/common/multichannel/chainsupport.go
Original file line number Diff line number Diff line change
Expand Up @@ -152,16 +152,14 @@ func (cs *ChainSupport) ProposeConfigUpdate(configtx *cb.Envelope) (*cb.ConfigEn
if !ok {
logger.Panic("old config is missing orderer group")
}
oldMetadata := oldOrdererConfig.ConsensusMetadata()

// we can remove this check since this is being validated in checkResources earlier
newOrdererConfig, ok := bundle.OrdererConfig()
if !ok {
return nil, errors.New("new config is missing orderer group")
}
newMetadata := newOrdererConfig.ConsensusMetadata()

if err = cs.ValidateConsensusMetadata(oldMetadata, newMetadata, false); err != nil {
if err = cs.ValidateConsensusMetadata(oldOrdererConfig, newOrdererConfig, false); err != nil {
return nil, errors.Wrap(err, "consensus metadata update for channel config update is invalid")
}
return env, nil
Expand Down
4 changes: 2 additions & 2 deletions orderer/common/multichannel/chainsupport_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -58,8 +58,8 @@ func TestConsensusMetadataValidation(t *testing.T) {
require.Equal(t, 1, mv.ValidateConsensusMetadataCallCount())
om, nm, nc := mv.ValidateConsensusMetadataArgsForCall(0)
require.False(t, nc)
require.Equal(t, oldConsensusMetadata, om)
require.Equal(t, newConsensusMetadata, nm)
require.Equal(t, oldConsensusMetadata, om.ConsensusMetadata())
require.Equal(t, newConsensusMetadata, nm.ConsensusMetadata())

// case 2: invalid consensus metadata update
mv.ValidateConsensusMetadataReturns(errors.New("bananas"))
Expand Down
4 changes: 2 additions & 2 deletions orderer/consensus/consensus.go
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ type MetadataValidator interface {
// updates on the channel.
// Since the ConsensusMetadata is specific to the consensus implementation (independent of the particular
// chain) this validation also needs to be implemented by the specific consensus implementation.
ValidateConsensusMetadata(oldMetadata, newMetadata []byte, newChannel bool) error
ValidateConsensusMetadata(oldOrdererConfig, newOrdererConfig channelconfig.Orderer, newChannel bool) error
}

// Chain defines a way to inject messages for ordering.
Expand Down Expand Up @@ -145,6 +145,6 @@ type NoOpMetadataValidator struct {

// ValidateConsensusMetadata determines the validity of a ConsensusMetadata update during config updates
// on the channel, and it always returns nil error for the NoOpMetadataValidator implementation.
func (n NoOpMetadataValidator) ValidateConsensusMetadata(oldMetadataBytes, newMetadataBytes []byte, newChannel bool) error {
func (n NoOpMetadataValidator) ValidateConsensusMetadata(oldChannelConfig, newChannelConfig channelconfig.Orderer, newChannel bool) error {
return nil
}
45 changes: 35 additions & 10 deletions orderer/consensus/etcdraft/chain.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import (
"context"
"encoding/pem"
"fmt"
"github.com/hyperledger/fabric/common/channelconfig"
"sync"
"sync/atomic"
"time"
Expand Down Expand Up @@ -963,7 +964,7 @@ func (c *Chain) detectConfChange(block *common.Block) *MembershipChanges {
c.sizeLimit = configMetadata.Options.SnapshotIntervalSize
}

changes, err := ComputeMembershipChanges(c.opts.BlockMetadata, c.opts.Consenters, configMetadata.Consenters, c.support.SharedConfig())
changes, err := ComputeMembershipChanges(c.opts.BlockMetadata, c.opts.Consenters, configMetadata.Consenters)
if err != nil {
c.logger.Panicf("illegal configuration change detected: %s", err)
}
Expand Down Expand Up @@ -1288,34 +1289,51 @@ func (c *Chain) newConfigMetadata(block *common.Block) *etcdraft.ConfigMetadata

// ValidateConsensusMetadata determines the validity of a
// ConsensusMetadata update during config updates on the channel.
func (c *Chain) ValidateConsensusMetadata(oldMetadataBytes, newMetadataBytes []byte, newChannel bool) error {
func (c *Chain) ValidateConsensusMetadata(oldOrdererConfig, newOrdererConfig channelconfig.Orderer, newChannel bool) error {
if newOrdererConfig == nil {
kopaygorodsky marked this conversation as resolved.
Show resolved Hide resolved
c.logger.Panic("Programming Error: ValidateConsensusMetadata called with nil new channel config")
return nil
}

// metadata was not updated
if newMetadataBytes == nil {
if newOrdererConfig.ConsensusMetadata() == nil {
return nil
}
if oldMetadataBytes == nil {

if oldOrdererConfig == nil {
c.logger.Panic("Programming Error: ValidateConsensusMetadata called with nil old channel config")
return nil
}

if oldOrdererConfig.ConsensusMetadata() == nil {
c.logger.Panic("Programming Error: ValidateConsensusMetadata called with nil old metadata")
return nil
}

oldMetadata := &etcdraft.ConfigMetadata{}
if err := proto.Unmarshal(oldMetadataBytes, oldMetadata); err != nil {
if err := proto.Unmarshal(oldOrdererConfig.ConsensusMetadata(), oldMetadata); err != nil {
c.logger.Panicf("Programming Error: Failed to unmarshal old etcdraft consensus metadata: %v", err)
}

newMetadata := &etcdraft.ConfigMetadata{}
if err := proto.Unmarshal(newMetadataBytes, newMetadata); err != nil {
if err := proto.Unmarshal(newOrdererConfig.ConsensusMetadata(), newMetadata); err != nil {
return errors.Wrap(err, "failed to unmarshal new etcdraft metadata configuration")
}

err := CheckConfigMetadata(newMetadata)
verifyOpts, err := createX509VerifyOptions(newOrdererConfig)
if err != nil {
return errors.Wrap(err, "invalid new config metdadata")
return errors.Wrapf(err, "failed to create x509 verify options from old and new orderer config")
}

if err := VerifyConfigMetadata(newMetadata, verifyOpts); err != nil {
return errors.Wrap(err, "invalid new config metadata")
}

if newChannel {
// check if the consenters are a subset of the existing consenters (system channel consenters)
set := ConsentersToMap(oldMetadata.Consenters)
for _, c := range newMetadata.Consenters {
if _, exits := set[string(c.ClientTlsCert)]; !exits {
if !set.Exists(c) {
return errors.New("new channel has consenter that is not part of system consenter set")
}
}
Expand All @@ -1328,11 +1346,18 @@ func (c *Chain) ValidateConsensusMetadata(oldMetadataBytes, newMetadataBytes []b
c.raftMetadataLock.RUnlock()

dummyOldConsentersMap := CreateConsentersMap(dummyOldBlockMetadata, oldMetadata)
changes, err := ComputeMembershipChanges(dummyOldBlockMetadata, dummyOldConsentersMap, newMetadata.Consenters, c.support.SharedConfig())
changes, err := ComputeMembershipChanges(dummyOldBlockMetadata, dummyOldConsentersMap, newMetadata.Consenters)
if err != nil {
return err
}

//new config metadata was verified above. Additionally need to check new consenters for certificates expiration
for _, c := range changes.AddedNodes {
if err := validateConsenterTLSCerts(c, verifyOpts, false); err != nil {
Copy link
Contributor

@jyellick jyellick Oct 5, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure that we should be checking certificate expiration here. For instance, we could have an expired certificate that we want to leave in place while we grow or shrink the consenter set. If we want to check expiration I'd say we need to only check expiration of new certificates.

I'd also note, that the orderer has been modified to perform ID validation not based on cert literals anymore, but rather based on public key, so, in some cases, even if the TLS certificate is expired in the channel config, an orderer with a valid and unexpired TLS certificate with matching public key may successfully be validated using this information.

Copy link
Contributor Author

@kopaygorodsky kopaygorodsky Oct 5, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jyellick but slicechanges.AddedNodes contains only new nodes(only one node actually), doesn't it?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're correct, sorry for missing this.

return errors.Wrapf(err, "consenter %s:%d has invalid certificates", c.Host, c.Port)
}
}

active := c.ActiveNodes.Load().([]uint64)
if changes.UnacceptableQuorumLoss(active) {
return errors.Errorf("%d out of %d nodes are alive, configuration will result in quorum loss", len(active), len(dummyOldConsentersMap))
Expand Down
Loading