Skip to content

Commit

Permalink
Fix bug in consenter cert validation logic
Browse files Browse the repository at this point in the history
It was accidentally verifying only the clientCert and
not the cert that was passed in.

FAB-18269

Signed-off-by: Will Lahti <wtlahti@us.ibm.com>
  • Loading branch information
wlahti committed Oct 12, 2020
1 parent e02367b commit 05e1791
Show file tree
Hide file tree
Showing 2 changed files with 57 additions and 12 deletions.
6 changes: 3 additions & 3 deletions orderer/consensus/etcdraft/util.go
Original file line number Diff line number Diff line change
Expand Up @@ -332,10 +332,10 @@ func validateConsenterTLSCerts(c *etcdraft.Consenter, opts x509.VerifyOptions, i
return errors.Wrapf(err, "parsing tls server cert of %s:%d", c.Host, c.Port)
}

var verify = func(certType string, cert *x509.Certificate, opts x509.VerifyOptions) error {
if _, err := clientCert.Verify(opts); err != nil {
verify := func(certType string, cert *x509.Certificate, opts x509.VerifyOptions) error {
if _, err := cert.Verify(opts); err != nil {
if validationRes, ok := err.(x509.CertificateInvalidError); !ok || (!ignoreExpiration || validationRes.Reason != x509.Expired) {
return errors.Errorf("verifying tls %s cert with serial number %d: %v", certType, clientCert.SerialNumber, err)
return errors.Wrapf(err, "verifying tls %s cert with serial number %d", certType, cert.SerialNumber)
}
}
return nil
Expand Down
63 changes: 54 additions & 9 deletions orderer/consensus/etcdraft/util_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ package etcdraft
import (
"crypto/x509"
"encoding/base64"
"fmt"
"io/ioutil"
"path/filepath"
"testing"
Expand Down Expand Up @@ -146,6 +147,31 @@ func TestVerifyConfigMetadata(t *testing.T) {
panic(err)
}

unknownTlsCA, err := tlsgen.NewCA()
if err != nil {
panic(err)
}

unknownServerPair, err := unknownTlsCA.NewServerCertKeyPair("unknownhost")
if err != nil {
panic(err)
}

unknownServerCert, err := parseCertificateFromBytes(unknownServerPair.Cert)
if err != nil {
panic(err)
}

unknownClientPair, err := unknownTlsCA.NewClientCertKeyPair()
if err != nil {
panic(err)
}

unknownClientCert, err := parseCertificateFromBytes(unknownClientPair.Cert)
if err != nil {
panic(err)
}

validOptions := &etcdraftproto.Options{
TickInterval: "500ms",
ElectionTick: 10,
Expand Down Expand Up @@ -330,20 +356,39 @@ func TestVerifyConfigMetadata(t *testing.T) {
errRegex: "duplicate consenter",
},
{
description: "consenter has cert signed by unknown authority",
description: "consenter has client cert signed by unknown authority",
metadata: &etcdraftproto.ConfigMetadata{
Options: validOptions,
Consenters: []*etcdraftproto.Consenter{
singleConsenter,
{
ClientTlsCert: unknownClientPair.Cert,
ServerTlsCert: serverPair.Cert,
},
},
},
verifyOpts: goodVerifyingOpts,
errRegex: fmt.Sprintf("verifying tls client cert with serial number %d: x509: certificate signed by unknown authority", unknownClientCert.SerialNumber),
},
{
description: "consenter has server cert signed by unknown authority",
metadata: &etcdraftproto.ConfigMetadata{
Options: validOptions,
Consenters: []*etcdraftproto.Consenter{
{
ServerTlsCert: unknownServerPair.Cert,
ClientTlsCert: clientPair.Cert,
},
},
},
verifyOpts: x509.VerifyOptions{},
errRegex: "certificate signed by unknown authority",
verifyOpts: goodVerifyingOpts,
errRegex: fmt.Sprintf("verifying tls server cert with serial number %d: x509: certificate signed by unknown authority", unknownServerCert.SerialNumber),
},
} {
err := VerifyConfigMetadata(testCase.metadata, testCase.verifyOpts)
assert.NotNil(t, err, testCase.description)
assert.Regexp(t, testCase.errRegex, err)
t.Run(testCase.description, func(t *testing.T) {
err := VerifyConfigMetadata(testCase.metadata, testCase.verifyOpts)
assert.NotNil(t, err)
assert.Regexp(t, testCase.errRegex, err)
})
}

//test use case when consenter has expired certificates
Expand All @@ -364,12 +409,12 @@ func TestVerifyConfigMetadata(t *testing.T) {
ServerTlsCert: tlsClientCert,
}

medatadaWithExpiredConsenter := &etcdraftproto.ConfigMetadata{
metadataWithExpiredConsenter := &etcdraftproto.ConfigMetadata{
Options: validOptions,
Consenters: []*etcdraftproto.Consenter{
consenterWithExpiredCerts,
},
}

assert.Nil(t, VerifyConfigMetadata(medatadaWithExpiredConsenter, expiredCertVerifyOpts))
assert.Nil(t, VerifyConfigMetadata(metadataWithExpiredConsenter, expiredCertVerifyOpts))
}

0 comments on commit 05e1791

Please sign in to comment.