Skip to content

Commit

Permalink
[FAB-3950] support conc generateCert invocations
Browse files Browse the repository at this point in the history 
usage of gossip/comm/crypto.go:GenerateCertificates is prone to race
conditions problems if it's used with the same parameters for file names.

The method is used only in test code, but the tests run concurrently.

Because the function that invokes GenerateCertificates usually uses
the same file name as parameter and adds defer os.Remove() on the file
names, and as a result - if it's used concurrently you may have a race
condition in which one goroutine deletes the files that the other
goroutine uses to read from.

I changed the code to do both generation, removal and loading
in the same method.
The generation uses a random file name so concurrent invocations
can now be used.

Change-Id: I6742b4ca0347bcc868f3df5a429c34bd8098d505
Signed-off-by: Yacov Manevich <yacovm@il.ibm.com>
  • Loading branch information
@yacovm
yacovm committed May 21, 2017
1 parent eaf7f4d commit 837fc68
Show file tree
Hide file tree
Showing 5 changed files with 47 additions and 92 deletions.
44 changes: 12 additions & 32 deletions gossip/comm/comm_impl.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,6 @@ import (
"errors"
"fmt"
"net"
"os"
"reflect"
"sync"
"sync/atomic"
Expand Down Expand Up @@ -620,39 +619,20 @@ func createGRPCLayer(port int) (*grpc.Server, net.Listener, api.PeerSecureDialOp
var serverOpts []grpc.ServerOption
var dialOpts []grpc.DialOption

keyFileName := fmt.Sprintf("key.%d.pem", util.RandomUInt64())
certFileName := fmt.Sprintf("cert.%d.pem", util.RandomUInt64())
cert := GenerateCertificatesOrPanic()
returnedCertHash = certHashFromRawCert(cert.Certificate[0])

defer os.Remove(keyFileName)
defer os.Remove(certFileName)

err = GenerateCertificates(keyFileName, certFileName)
if err == nil {
cert, err := tls.LoadX509KeyPair(certFileName, keyFileName)
if err != nil {
panic(err)
}

if len(cert.Certificate) == 0 {
panic(errors.New("Certificate chain is nil"))
}

returnedCertHash = certHashFromRawCert(cert.Certificate[0])

tlsConf := &tls.Config{
Certificates: []tls.Certificate{cert},
ClientAuth: tls.RequestClientCert,
InsecureSkipVerify: true,
}
serverOpts = append(serverOpts, grpc.Creds(credentials.NewTLS(tlsConf)))
ta := credentials.NewTLS(&tls.Config{
Certificates: []tls.Certificate{cert},
InsecureSkipVerify: true,
})
dialOpts = append(dialOpts, grpc.WithTransportCredentials(&authCreds{tlsCreds: ta}))
} else {
dialOpts = append(dialOpts, grpc.WithInsecure())
tlsConf := &tls.Config{
Certificates: []tls.Certificate{cert},
ClientAuth: tls.RequestClientCert,
InsecureSkipVerify: true,
}
serverOpts = append(serverOpts, grpc.Creds(credentials.NewTLS(tlsConf)))
ta := credentials.NewTLS(&tls.Config{
Certificates: []tls.Certificate{cert},
InsecureSkipVerify: true,
})
dialOpts = append(dialOpts, grpc.WithTransportCredentials(&authCreds{tlsCreds: ta}))

listenAddress := fmt.Sprintf("%s:%d", "", port)
ll, err = net.Listen("tcp", listenAddress)
Expand Down
31 changes: 6 additions & 25 deletions gossip/comm/comm_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -111,12 +111,7 @@ func newCommInstance(port int, sec api.MessageCryptoService) (Comm, error) {

func handshaker(endpoint string, comm Comm, t *testing.T, sigMutator func([]byte) []byte, pkiIDmutator func([]byte) []byte, mutualTLS bool) <-chan proto.ReceivedMessage {
c := &commImpl{}
err := GenerateCertificates("key.pem", "cert.pem")
assert.NoError(t, err, "%v", err)
defer os.Remove("cert.pem")
defer os.Remove("key.pem")
cert, err := tls.LoadX509KeyPair("cert.pem", "key.pem")
assert.NoError(t, err, "%v", err)
cert := GenerateCertificatesOrPanic()
tlsCfg := &tls.Config{
InsecureSkipVerify: true,
}
Expand Down Expand Up @@ -286,28 +281,19 @@ func TestBasic(t *testing.T) {

func TestProdConstructor(t *testing.T) {
t.Parallel()
keyFileName := fmt.Sprintf("key.%d.pem", util.RandomUInt64())
certFileName := fmt.Sprintf("cert.%d.pem", util.RandomUInt64())

GenerateCertificates(keyFileName, certFileName)
cert, _ := tls.LoadX509KeyPair(certFileName, keyFileName)
os.Remove(keyFileName)
os.Remove(certFileName)
peerIdentity := GenerateCertificatesOrPanic()
srv, lsnr, dialOpts, certHash := createGRPCLayer(20000)
defer srv.Stop()
defer lsnr.Close()
comm1, _ := NewCommInstance(srv, &cert, identity.NewIdentityMapper(naiveSec), []byte("localhost:20000"), dialOpts)
comm1, _ := NewCommInstance(srv, &peerIdentity, identity.NewIdentityMapper(naiveSec), []byte("localhost:20000"), dialOpts)
comm1.(*commImpl).selfCertHash = certHash
go srv.Serve(lsnr)

GenerateCertificates(keyFileName, certFileName)
cert, _ = tls.LoadX509KeyPair(certFileName, keyFileName)
os.Remove(keyFileName)
os.Remove(certFileName)
peerIdentity = GenerateCertificatesOrPanic()
srv, lsnr, dialOpts, certHash = createGRPCLayer(30000)
defer srv.Stop()
defer lsnr.Close()
comm2, _ := NewCommInstance(srv, &cert, identity.NewIdentityMapper(naiveSec), []byte("localhost:30000"), dialOpts)
comm2, _ := NewCommInstance(srv, &peerIdentity, identity.NewIdentityMapper(naiveSec), []byte("localhost:30000"), dialOpts)
comm2.(*commImpl).selfCertHash = certHash
go srv.Serve(lsnr)
defer comm1.Stop()
Expand Down Expand Up @@ -350,12 +336,7 @@ func TestCloseConn(t *testing.T) {
defer comm1.Stop()
acceptChan := comm1.Accept(acceptAll)

err := GenerateCertificates("key.pem", "cert.pem")
assert.NoError(t, err, "%v", err)
defer os.Remove("cert.pem")
defer os.Remove("key.pem")
cert, err := tls.LoadX509KeyPair("cert.pem", "key.pem")
assert.NoError(t, err, "%v", err)
cert := GenerateCertificatesOrPanic()
tlsCfg := &tls.Config{
InsecureSkipVerify: true,
Certificates: []tls.Certificate{cert},
Expand Down
32 changes: 25 additions & 7 deletions gossip/comm/crypto.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,12 +23,15 @@ import (
"crypto/tls"
"crypto/x509"
"encoding/pem"
"errors"
"fmt"
"math/big"
"net"
"os"
"time"

"github.com/hyperledger/fabric/common/util"
gutil "github.com/hyperledger/fabric/gossip/util"
"golang.org/x/net/context"
"google.golang.org/grpc/credentials"
"google.golang.org/grpc/peer"
Expand All @@ -43,15 +46,20 @@ func writeFile(filename string, keyType string, data []byte) error {
return pem.Encode(f, &pem.Block{Type: keyType, Bytes: data})
}

func GenerateCertificates(privKeyFile string, certKeyFile string) error {
func GenerateCertificatesOrPanic() tls.Certificate {
privKeyFile := fmt.Sprintf("key.%d.priv", gutil.RandomUInt64())
certKeyFile := fmt.Sprintf("cert.%d.pub", gutil.RandomUInt64())

defer os.Remove(privKeyFile)
defer os.Remove(certKeyFile)
privateKey, err := ecdsa.GenerateKey(elliptic.P256(), rand.Reader)
if err != nil {
return err
panic(err)
}

sn, err := rand.Int(rand.Reader, new(big.Int).Lsh(big.NewInt(1), 128))
if err != nil {
return err
panic(err)
}
template := x509.Certificate{
KeyUsage: x509.KeyUsageKeyEncipherment | x509.KeyUsageDigitalSignature,
Expand All @@ -60,18 +68,28 @@ func GenerateCertificates(privKeyFile string, certKeyFile string) error {
}
rawBytes, err := x509.CreateCertificate(rand.Reader, &template, &template, &privateKey.PublicKey, privateKey)
if err != nil {
return err
panic(err)
}
err = writeFile(certKeyFile, "CERTIFICATE", rawBytes)
if err != nil {
return err
panic(err)
}
privBytes, err := x509.MarshalECPrivateKey(privateKey)
if err != nil {
return err
panic(err)
}
err = writeFile(privKeyFile, "EC PRIVATE KEY", privBytes)
return err
if err != nil {
panic(err)
}
cert, err := tls.LoadX509KeyPair(certKeyFile, privKeyFile)
if err != nil {
panic(err)
}
if len(cert.Certificate) == 0 {
panic(errors.New("Certificate chain is empty"))
}
return cert
}

func certHashFromRawCert(rawCert []byte) []byte {
Expand Down
18 changes: 3 additions & 15 deletions gossip/comm/crypto_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@ import (
"crypto/tls"
"fmt"
"net"
"os"
"sync"
"testing"
"time"
Expand Down Expand Up @@ -84,27 +83,16 @@ func (s *gossipTestServer) Ping(context.Context, *proto.Empty) (*proto.Empty, er
}

func TestCertificateExtraction(t *testing.T) {
err := GenerateCertificates("key.pem", "cert.pem")
defer os.Remove("cert.pem")
defer os.Remove("key.pem")
assert.NoError(t, err, "%v", err)
serverCert, err := tls.LoadX509KeyPair("cert.pem", "key.pem")
assert.NoError(t, err, "%v", err)

srv := createTestServer(t, &serverCert)
cert := GenerateCertificatesOrPanic()
srv := createTestServer(t, &cert)
defer srv.stop()

GenerateCertificates("key2.pem", "cert2.pem")
defer os.Remove("cert2.pem")
defer os.Remove("key2.pem")
clientCert, err := tls.LoadX509KeyPair("cert2.pem", "key2.pem")
clientCert := GenerateCertificatesOrPanic()
clientCertHash := certHashFromRawCert(clientCert.Certificate[0])
assert.NoError(t, err)
ta := credentials.NewTLS(&tls.Config{
Certificates: []tls.Certificate{clientCert},
InsecureSkipVerify: true,
})
assert.NoError(t, err, "%v", err)
ac := &authCreds{tlsCreds: ta}
assert.Equal(t, "1.2", ac.Info().SecurityVersion)
assert.Equal(t, "tls", ac.Info().SecurityProtocol)
Expand Down
14 changes: 1 addition & 13 deletions gossip/gossip/anchor_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@ import (
"fmt"
"io"
"net"
"os"
"sync"
"sync/atomic"
"testing"
Expand Down Expand Up @@ -107,18 +106,7 @@ func newPeerMock(port int, expectedMsgs2Receive int, t *testing.T, msgAssertions
}

func newGRPCServerWithTLS() (*grpc.Server, []byte) {
// TODO: For this change set I simply exported the function, but I think that we should
// Instead have a function that returns a TLS gRPC server from the comm package instead,
// and then we could remove the boilerplate code below both from this file and from the
// comm test code.
// However, it needs to be attended in a subsequent change set rather than this.
comm.GenerateCertificates("key.pem", "cert.pem")
defer os.Remove("cert.pem")
defer os.Remove("key.pem")
cert, err := tls.LoadX509KeyPair("cert.pem", "key.pem")
if err != nil {
panic(err)
}
cert := comm.GenerateCertificatesOrPanic()
tlsConf := &tls.Config{
Certificates: []tls.Certificate{cert},
ClientAuth: tls.RequestClientCert,
Expand Down

0 comments on commit 837fc68

Please sign in to comment.