From 52bb90904de8988319024e6508033a3453a91ce4 Mon Sep 17 00:00:00 2001 From: Dave Enyeart Date: Mon, 27 Jan 2025 15:38:25 -0500 Subject: [PATCH] Properly handle expired identities in gossip (release-2.5) (#5122) * Properly handle expired identities in gossip When a peer's certificate expires, gossip still retains past messages it has sent, and gossips them to other peers. Aside from peers doing redundant work, this also impairs their connectivity to the peer with the renewed certificate. The reason is that peers try connect to the peer of the renewed certificate but abort because they cannot find its (old) PKI-ID in the identity store, which purged its old PKI-ID once its certificate has expired. This commit fixes this problem by making the peer forget about peers that their identities have been purged from the identity store. Signed-off-by: David Enyeart Signed-off-by: Yacov Manevich * Fix gossip cert renewal integration test The membership check via discovery does not work consistently due to the renewed cert signature not matching expectations. For now, it is sufficient to do the membership check via checking the log. Signed-off-by: David Enyeart --------- Signed-off-by: David Enyeart Signed-off-by: Yacov Manevich --- gossip/comm/comm.go | 2 +- gossip/comm/comm_impl.go | 2 +- gossip/comm/mock/mock_comm.go | 2 +- gossip/gossip/gossip_impl.go | 18 +++- integration/gossip/gossip_test.go | 163 ++++++++++++++++++++++++++++++ 5 files changed, 183 insertions(+), 4 deletions(-) diff --git a/gossip/comm/comm.go b/gossip/comm/comm.go index af9462e482e..3288c297bf8 100644 --- a/gossip/comm/comm.go +++ b/gossip/comm/comm.go @@ -45,7 +45,7 @@ type Comm interface { PresumedDead() <-chan common.PKIidType // IdentitySwitch returns a read-only channel about identity change events - IdentitySwitch() <-chan common.PKIidType + IdentitySwitch() chan common.PKIidType // CloseConn closes a connection to a certain endpoint CloseConn(peer *RemotePeer) diff --git a/gossip/comm/comm_impl.go b/gossip/comm/comm_impl.go index 5a75901d378..84ca1fd960b 100644 --- a/gossip/comm/comm_impl.go +++ b/gossip/comm/comm_impl.go @@ -359,7 +359,7 @@ func (c *commImpl) PresumedDead() <-chan common.PKIidType { return c.deadEndpoints } -func (c *commImpl) IdentitySwitch() <-chan common.PKIidType { +func (c *commImpl) IdentitySwitch() chan common.PKIidType { return c.identityChanges } diff --git a/gossip/comm/mock/mock_comm.go b/gossip/comm/mock/mock_comm.go index 6acf3bde55e..ead615be0f2 100644 --- a/gossip/comm/mock/mock_comm.go +++ b/gossip/comm/mock/mock_comm.go @@ -134,7 +134,7 @@ func (mock *commMock) start() { } } -func (mock *commMock) IdentitySwitch() <-chan common.PKIidType { +func (mock *commMock) IdentitySwitch() chan common.PKIidType { panic("implement me") } diff --git a/gossip/gossip/gossip_impl.go b/gossip/gossip/gossip_impl.go index 1c908229395..394cb4f6ec3 100644 --- a/gossip/gossip/gossip_impl.go +++ b/gossip/gossip/gossip_impl.go @@ -96,8 +96,24 @@ func New(conf *Config, s *grpc.Server, sa api.SecurityAdvisor, g.stateInfoMsgStore = g.newStateInfoMsgStore() g.idMapper = identity.NewIdentityMapper(mcs, selfIdentity, func(pkiID common.PKIidType, identity api.PeerIdentityType) { - g.comm.CloseConn(&comm.RemotePeer{PKIID: pkiID}) + // Identities which are purged from the membership store + // should not be communicated with anymore, as the purge is done + // because the identity of the corresponding PKI-ID not being + // valid anymore, such as it being expired. + + // Remove the identity from the identity replication + // for it to not be replicated any further. g.certPuller.Remove(string(pkiID)) + + // Notify the identity switch channel of the communication layer, + // which in turn is used to notify the discovery layer + // about the PKI-ID not being relevant anymore, which causes + // the discovery layer to purge it from memory. + // Afterwards, gossip never communicates with this PKI-ID. + g.comm.IdentitySwitch() <- pkiID + + // Cease communication with the node, if connected to it. + g.comm.CloseConn(&comm.RemotePeer{PKIID: pkiID}) }, sa) commConfig := comm.CommConfig{ diff --git a/integration/gossip/gossip_test.go b/integration/gossip/gossip_test.go index 69ab4a58e66..6bfc4b53bcb 100644 --- a/integration/gossip/gossip_test.go +++ b/integration/gossip/gossip_test.go @@ -7,9 +7,16 @@ package gossip import ( + "crypto/ecdsa" + "crypto/rand" + "crypto/sha256" + "crypto/x509" + "encoding/pem" "fmt" "io/ioutil" + "math/big" "os" + "path/filepath" "syscall" "time" @@ -265,8 +272,164 @@ var _ = Describe("Gossip State Transfer and Membership", func() { assertPeerMembershipUpdate(network, peer1Org1, []*nwo.Peer{peer0Org2, peer1Org2}, nwprocs, expectedMsgFromExpirationCallback) }) }) + + It("updates membership for a peer with a renewed certificate", func() { + network.Bootstrap() + orderer := network.Orderer("orderer") + nwprocs.ordererRunner = network.OrdererRunner(orderer) + nwprocs.ordererProcess = ifrit.Invoke(nwprocs.ordererRunner) + Eventually(nwprocs.ordererProcess.Ready(), network.EventuallyTimeout).Should(BeClosed()) + + peer0Org1 := network.Peer("Org1", "peer0") + peer0Org2 := network.Peer("Org2", "peer0") + + By("bringing up a peer in each organization") + startPeers(nwprocs, false, peer0Org1, peer0Org2) + + By("creating and joining both peers to channel") + network.CreateChannel(channelName, orderer, peer0Org1) + network.JoinChannel(channelName, orderer, peer0Org1, peer0Org2) + network.UpdateChannelAnchors(orderer, channelName) + + By("verifying membership of both peers") + Eventually(nwo.DiscoverPeers(network, peer0Org1, "User1", "testchannel"), 50*time.Second, 100*time.Millisecond).Should(ContainElements(network.DiscoveredPeer(peer0Org2, "_lifecycle"))) + + By("stopping, renewing peer0Org2 certificate before expiration, and restarting") + stopPeers(nwprocs, peer0Org2) + renewPeerCertificate(network, peer0Org2, time.Now().Add(time.Minute)) + startPeers(nwprocs, false, peer0Org2) + + By("verifying membership after cert renewed") + peer0Org1Runner := nwprocs.peerRunners[peer0Org1.ID()] + Eventually(peer0Org1Runner.Err(), network.EventuallyTimeout).Should(gbytes.Say("Membership view has changed. peers went online")) + /* + // TODO - Replace membership log check with membership discovery check (not currently working since renewed cert signature doesn't always match expectations even though it is forced to be Low-S) + Eventually( + nwo.DiscoverPeers(network, peer0Org1, "User1", "testchannel"), + 60*time.Second, + 100*time.Millisecond). + Should(ContainElements(network.DiscoveredPeer(network.Peer("Org2", "peer0"), "_lifecycle"))) + */ + + By("waiting for cert to expire within a minute") + Eventually(peer0Org1Runner.Err(), time.Minute*2).Should(gbytes.Say("gossipping peer identity expired")) + + By("stopping, renewing peer0Org2 certificate again after its expiration, restarting") + stopPeers(nwprocs, peer0Org2) + renewPeerCertificate(network, peer0Org2, time.Now().Add(time.Hour)) + startPeers(nwprocs, false, peer0Org2) + + By("verifying membership after cert expired and renewed again") + Eventually(peer0Org1Runner.Err(), network.EventuallyTimeout).Should(gbytes.Say("Membership view has changed. peers went online")) + + /* + // TODO - Replace membership log check with membership discovery check (not currently working since renewed cert signature doesn't always match expectations even though it is forced to be Low-S) + Eventually( + nwo.DiscoverPeers(network, peer0Org1, "User1", "testchannel"), + 60*time.Second, + 100*time.Millisecond). + Should(ContainElements(network.DiscoveredPeer(network.Peer("Org2", "peer0"), "_lifecycle"))) + */ + }) }) +// renewPeerCertificate renews the certificate with a given expirationTime and re-writes it to the peer's signcert directory +func renewPeerCertificate(network *nwo.Network, peer *nwo.Peer, expirationTime time.Time) { + peerDomain := network.Organization(peer.Organization).Domain + + peerCAKeyPath := filepath.Join(network.RootDir, "crypto", "peerOrganizations", peerDomain, "ca", "priv_sk") + peerCAKey, err := os.ReadFile(peerCAKeyPath) + Expect(err).NotTo(HaveOccurred()) + + peerCACertPath := filepath.Join(network.RootDir, "crypto", "peerOrganizations", peerDomain, "ca", fmt.Sprintf("ca.%s-cert.pem", peerDomain)) + peerCACert, err := os.ReadFile(peerCACertPath) + Expect(err).NotTo(HaveOccurred()) + + peerCertPath := filepath.Join(network.PeerLocalMSPDir(peer), "signcerts", fmt.Sprintf("peer0.%s-cert.pem", peerDomain)) + peerCert, err := os.ReadFile(peerCertPath) + Expect(err).NotTo(HaveOccurred()) + + renewedCert, _ := expireCertificate(peerCert, peerCACert, peerCAKey, expirationTime) + err = os.WriteFile(peerCertPath, renewedCert, 0o600) + Expect(err).NotTo(HaveOccurred()) +} + +// expireCertificate re-creates and re-signs a certificate with a new expirationTime +func expireCertificate(certPEM, caCertPEM, caKeyPEM []byte, expirationTime time.Time) (expiredcertPEM []byte, earlyMadeCACertPEM []byte) { + keyAsDER, _ := pem.Decode(caKeyPEM) + caKeyWithoutType, err := x509.ParsePKCS8PrivateKey(keyAsDER.Bytes) + Expect(err).NotTo(HaveOccurred()) + caKey := caKeyWithoutType.(*ecdsa.PrivateKey) + + caCertAsDER, _ := pem.Decode(caCertPEM) + caCert, err := x509.ParseCertificate(caCertAsDER.Bytes) + Expect(err).NotTo(HaveOccurred()) + + certAsDER, _ := pem.Decode(certPEM) + cert, err := x509.ParseCertificate(certAsDER.Bytes) + Expect(err).NotTo(HaveOccurred()) + + cert.Raw = nil + caCert.Raw = nil + // The certificate was made 1 minute ago (1 hour doesn't work since cert will be before original CA cert NotBefore time) + cert.NotBefore = time.Now().Add((-1) * time.Minute) + // As well as the CA certificate + caCert.NotBefore = time.Now().Add((-1) * time.Minute) + // The certificate expires now + cert.NotAfter = expirationTime + + // The CA creates and signs a temporary certificate + tempCertBytes, err := x509.CreateCertificate(rand.Reader, cert, caCert, cert.PublicKey, caKey) + Expect(err).NotTo(HaveOccurred()) + + // Force the certificate to use Low-S signature to be compatible with the identities that Fabric uses + + // Parse the certificate to extract the TBS (to-be-signed) data + tempParsedCert, err := x509.ParseCertificate(tempCertBytes) + Expect(err).NotTo(HaveOccurred()) + + // Hash the TBS data + hash := sha256.Sum256(tempParsedCert.RawTBSCertificate) + + // Sign the hash using forceLowS + r, s, err := forceLowS(caKey, hash[:]) + Expect(err).NotTo(HaveOccurred()) + + // Encode the signature (DER format) + signature := append(r.Bytes(), s.Bytes()...) + + // Replace the signature in the certificate with the low-s signature + tempParsedCert.Signature = signature + + // Re-encode the certificate with the low-s signature + certBytes, err := x509.CreateCertificate(rand.Reader, tempParsedCert, caCert, cert.PublicKey, caKey) + Expect(err).NotTo(HaveOccurred()) + + // The CA signs its own certificate + caCertBytes, err := x509.CreateCertificate(rand.Reader, caCert, caCert, caCert.PublicKey, caKey) + Expect(err).NotTo(HaveOccurred()) + + expiredcertPEM = pem.EncodeToMemory(&pem.Block{Bytes: certBytes, Type: "CERTIFICATE"}) + earlyMadeCACertPEM = pem.EncodeToMemory(&pem.Block{Bytes: caCertBytes, Type: "CERTIFICATE"}) + return +} + +// forceLowS ensures the ECDSA signature's S value is low +func forceLowS(priv *ecdsa.PrivateKey, hash []byte) (r, s *big.Int, err error) { + r, s, err = ecdsa.Sign(rand.Reader, priv, hash) + Expect(err).NotTo(HaveOccurred()) + + curveOrder := priv.Curve.Params().N + halfOrder := new(big.Int).Rsh(curveOrder, 1) // curveOrder / 2 + + // If s is greater than half the order, replace it with curveOrder - s + if s.Cmp(halfOrder) > 0 { + s.Sub(curveOrder, s) + } + + return r, s, nil +} + func runTransactions(n *nwo.Network, orderer *nwo.Orderer, peer *nwo.Peer, chaincodeName string, channelID string) { for i := 0; i < 5; i++ { sess, err := n.PeerUserSession(peer, "User1", commands.ChaincodeInvoke{