[FAB-3763] Fixing Intermediate CA certs sanitization

This change-set addresses the following issue:
Currently, certificates of intermediate CAs are sanitized in the same
way root CA certificates are. Namely, by using as parent certificate
the certificate itself. But intermediate CA certificate must have
a parent and therefore the sanitation must happen with the respect
to that certificate.

Change-Id: I2f9c7d7eb9e4c1d3aab722ddac5ffdf8189fdf37
Signed-off-by: Angelo De Caro <[email protected]>

Author: adecaro

msp/identities.go

@@ -48,6 +48,7 @@ type identity struct {
 func newIdentity(id *IdentityIdentifier, cert *x509.Certificate, pk bccsp.Key, msp *bccspmsp) (Identity, error) {
 	mspLogger.Debugf("Creating identity instance for ID %s", id)
 
+	// Sanitize first the certificate
 	cert, err := msp.sanitizeCert(cert)
 	if err != nil {
 		return nil, err

msp/msp_test.go

@@ -27,6 +27,7 @@ import (
 
 	"github.com/golang/protobuf/proto"
 	"github.com/hyperledger/fabric/bccsp"
+	"github.com/hyperledger/fabric/bccsp/sw"
 	"github.com/hyperledger/fabric/core/config"
 	"github.com/hyperledger/fabric/protos/msp"
 	"github.com/stretchr/testify/assert"
@@ -859,3 +860,21 @@ func getIdentity(t *testing.T, path string) Identity {
 
 	return id
 }
+
+func getLocalMSP(t *testing.T, dir string) MSP {
+	conf, err := GetLocalMspConfig(dir, nil, "DEFAULT")
+	assert.NoError(t, err)
+
+	thisMSP, err := NewBccspMsp()
+	assert.NoError(t, err)
+	ks, err := sw.NewFileBasedKeyStore(nil, filepath.Join(dir, "keystore"), true)
+	assert.NoError(t, err)
+	csp, err := sw.New(256, "SHA2", ks)
+	assert.NoError(t, err)
+	thisMSP.(*bccspmsp).bccsp = csp
+
+	err = thisMSP.Setup(conf)
+	assert.NoError(t, err)
+
+	return thisMSP
+}

msp/mspimpl.go

@@ -310,6 +310,32 @@ func (msp *bccspmsp) Setup(conf1 *m.MSPConfig) error {
 	if len(conf.RootCerts) == 0 {
 		return errors.New("Expected at least one CA certificate")
 	}
+
+	// pre-create the verify options with roots and intermediates
+	// this is needed to make certificate sanitation working.
+	msp.opts = &x509.VerifyOptions{
+		Roots:         x509.NewCertPool(),
+		Intermediates: x509.NewCertPool(),
+	}
+	for _, v := range conf.RootCerts {
+		cert, err := msp.getCertFromPem(v)
+		if err != nil {
+			return err
+		}
+
+		msp.opts.Roots.AddCert(cert)
+	}
+	for _, v := range conf.IntermediateCerts {
+		cert, err := msp.getCertFromPem(v)
+		if err != nil {
+			return err
+		}
+
+		msp.opts.Intermediates.AddCert(cert)
+	}
+
+	// Load root and intermediate CA identities
+	// Recall that when an identity is created, its certificate gets sanitized
 	msp.rootCerts = make([]Identity, len(conf.RootCerts))
 	for i, trustedCert := range conf.RootCerts {
 		id, _, err := msp.getIdentityFromConf(trustedCert)
@@ -331,18 +357,6 @@ func (msp *bccspmsp) Setup(conf1 *m.MSPConfig) error {
 		msp.intermediateCerts[i] = id
 	}
 
-	// pre-create the verify options with roots and intermediates
-	msp.opts = &x509.VerifyOptions{
-		Roots:         x509.NewCertPool(),
-		Intermediates: x509.NewCertPool(),
-	}
-	for _, v := range msp.rootCerts {
-		msp.opts.Roots.AddCert(v.(*identity).cert)
-	}
-	for _, v := range msp.intermediateCerts {
-		msp.opts.Intermediates.AddCert(v.(*identity).cert)
-	}
-
 	// make and fill the set of admin certs (if present)
 	msp.admins = make([]Identity, len(conf.Admins))
 	for i, admCert := range conf.Admins {
@@ -712,26 +726,35 @@ func (msp *bccspmsp) getCertificationChainForBCCSPIdentity(id *identity) ([]*x50
 	return msp.getValidationChain(id.cert)
 }
 
-func (msp *bccspmsp) getValidationChain(cert *x509.Certificate) ([]*x509.Certificate, error) {
+func (msp *bccspmsp) getUniqueValidationChain(cert *x509.Certificate) ([]*x509.Certificate, error) {
 	// ask golang to validate the cert for us based on the options that we've built at setup time
-	validationChain, err := cert.Verify(*(msp.opts))
+	validationChains, err := cert.Verify(*(msp.opts))
 	if err != nil {
 		return nil, fmt.Errorf("The supplied identity is not valid, Verify() returned %s", err)
 	}
 
 	// we only support a single validation chain;
 	// if there's more than one then there might
 	// be unclarity about who owns the identity
-	if len(validationChain) != 1 {
-		return nil, fmt.Errorf("This MSP only supports a single validation chain, got %d", len(validationChain))
+	if len(validationChains) != 1 {
+		return nil, fmt.Errorf("This MSP only supports a single validation chain, got %d", len(validationChains))
+	}
+
+	return validationChains[0], nil
+}
+
+func (msp *bccspmsp) getValidationChain(cert *x509.Certificate) ([]*x509.Certificate, error) {
+	validationChain, err := msp.getUniqueValidationChain(cert)
+	if err != nil {
+		return nil, fmt.Errorf("Failed getting validation chain %s", err)
 	}
 
 	// we expect a chain of length at least 2
-	if len(validationChain[0]) < 2 {
+	if len(validationChain) < 2 {
 		return nil, fmt.Errorf("Expected a chain of length at least 2, got %d", len(validationChain))
 	}
 
-	return validationChain[0], nil
+	return validationChain, nil
 }
 
 // getCertificationChainIdentifier returns the certification chain identifier of the passed identity within this msp.
@@ -838,14 +861,29 @@ func (msp *bccspmsp) setupOUs(conf m.FabricMSPConfig) error {
 	return nil
 }
 
+// sanitizeCert ensures that x509 certificates signed using ECDSA
+// do have signatures in Low-S. If this is not the case, the certificate
+// is regenerated to have a Low-S signature.
 func (msp *bccspmsp) sanitizeCert(cert *x509.Certificate) (*x509.Certificate, error) {
 	if isECDSASignedCert(cert) {
 		// Lookup for a parent certificate to perform the sanitization
 		var parentCert *x509.Certificate
 		if cert.IsCA {
-			parentCert = cert
+			// at this point, cert might be a root CA certificate
+			// or an intermediate CA certificate
+			chain, err := msp.getUniqueValidationChain(cert)
+			if err != nil {
+				return nil, err
+			}
+			if len(chain) == 1 {
+				// cert is a root CA certificate
+				parentCert = cert
+			} else {
+				// cert is an intermediate CA certificate
+				parentCert = chain[1]
+			}
 		} else {
-			chain, err := msp.getValidationChain(cert)
+			chain, err := msp.getUniqueValidationChain(cert)
 			if err != nil {
 				return nil, err
 			}

msp/mspwithintermediatecas_test.go

@@ -19,99 +19,16 @@ package msp
 import (
 	"testing"
 
-	"github.com/golang/protobuf/proto"
-	"github.com/hyperledger/fabric/bccsp"
-	"github.com/hyperledger/fabric/protos/msp"
 	"github.com/stretchr/testify/assert"
 )
 
-// the following strings contain the credentials for a test MSP setup that has
-// 1) a key and a signcert (used to populate the default signing identity);
-//    signcert is not signed by a CA directly but by an intermediate CA
-// 2) intermediatecert is an intermediate CA, signed by the CA
-// 3) cacert is the CA that signed the intermediate
-const key = `-----BEGIN EC PRIVATE KEY-----
-MHcCAQEEII27gKS2mFIIGkyGFEvHyv1khaJHe+p+sDt0++JByCDToAoGCCqGSM49
-AwEHoUQDQgAEJUUpwMg/jQ+qpmkVewEvwTySl+XWbd4AXtb/0XsDqXNcyXl0DVgA
-gJNGnt5r+bvZdB8SOk1ySAEEsCQArkarMg==
------END EC PRIVATE KEY-----`
-
-var signcert = `-----BEGIN CERTIFICATE-----
-MIIDAzCCAqigAwIBAgIBAjAKBggqhkjOPQQDAjBsMQswCQYDVQQGEwJHQjEQMA4G
-A1UECAwHRW5nbGFuZDEOMAwGA1UECgwFQmFyMTkxDjAMBgNVBAsMBUJhcjE5MQ4w
-DAYDVQQDDAVCYXIxOTEbMBkGCSqGSIb3DQEJARYMQmFyMTktY2xpZW50MB4XDTE3
-MDIwOTE2MDcxMFoXDTE4MDIxOTE2MDcxMFowfDELMAkGA1UEBhMCR0IxEDAOBgNV
-BAgMB0VuZ2xhbmQxEDAOBgNVBAcMB0lwc3dpY2gxDjAMBgNVBAoMBUJhcjE5MQ4w
-DAYDVQQLDAVCYXIxOTEOMAwGA1UEAwwFQmFyMTkxGTAXBgkqhkiG9w0BCQEWCkJh
-cjE5LXBlZXIwWTATBgcqhkjOPQIBBggqhkjOPQMBBwNCAAQlRSnAyD+ND6qmaRV7
-AS/BPJKX5dZt3gBe1v/RewOpc1zJeXQNWACAk0ae3mv5u9l0HxI6TXJIAQSwJACu
-Rqsyo4IBKTCCASUwCQYDVR0TBAIwADARBglghkgBhvhCAQEEBAMCBkAwMwYJYIZI
-AYb4QgENBCYWJE9wZW5TU0wgR2VuZXJhdGVkIFNlcnZlciBDZXJ0aWZpY2F0ZTAd
-BgNVHQ4EFgQUwHzbLJQMaWd1cpHdkSaEFxdKB1owgYsGA1UdIwSBgzCBgIAUYxFe
-+cXOD5iQ223bZNdOuKCRiTKhZaRjMGExCzAJBgNVBAYTAkdCMRAwDgYDVQQIDAdF
-bmdsYW5kMRAwDgYDVQQHDAdJcHN3aWNoMQ4wDAYDVQQKDAVCYXIxOTEOMAwGA1UE
-CwwFQmFyMTkxDjAMBgNVBAMMBUJhcjE5ggEBMA4GA1UdDwEB/wQEAwIFoDATBgNV
-HSUEDDAKBggrBgEFBQcDATAKBggqhkjOPQQDAgNJADBGAiEAuMq65lOaie4705Ol
-Ow52DjbaO2YuIxK2auBCqNIu0gECIQCDoKdUQ/sa+9Ah1mzneE6iz/f/YFVWo4EP
-HeamPGiDTQ==
------END CERTIFICATE-----`
-
-var intermediatecert = `-----BEGIN CERTIFICATE-----
-MIICITCCAcigAwIBAgIBATAKBggqhkjOPQQDAjBhMQswCQYDVQQGEwJHQjEQMA4G
-A1UECAwHRW5nbGFuZDEQMA4GA1UEBwwHSXBzd2ljaDEOMAwGA1UECgwFQmFyMTkx
-DjAMBgNVBAsMBUJhcjE5MQ4wDAYDVQQDDAVCYXIxOTAeFw0xNzAyMDkxNTUyMDBa
-Fw0yNzAyMDcxNTUyMDBaMGwxCzAJBgNVBAYTAkdCMRAwDgYDVQQIDAdFbmdsYW5k
-MQ4wDAYDVQQKDAVCYXIxOTEOMAwGA1UECwwFQmFyMTkxDjAMBgNVBAMMBUJhcjE5
-MRswGQYJKoZIhvcNAQkBFgxCYXIxOS1jbGllbnQwWTATBgcqhkjOPQIBBggqhkjO
-PQMBBwNCAAQBymfTx4GWt1lnTV4Xp3skM5LJpZ40HVhCDLfvfrD8/3WQLHaLc7XW
-KpphhXW8HYLyyjkEZVLsAFHkKjwmlcpzo2YwZDAdBgNVHQ4EFgQUYxFe+cXOD5iQ
-223bZNdOuKCRiTIwHwYDVR0jBBgwFoAU4UJ1xRnh6zeW2IKABUOjIt9Wk8gwEgYD
-VR0TAQH/BAgwBgEB/wIBADAOBgNVHQ8BAf8EBAMCAYYwCgYIKoZIzj0EAwIDRwAw
-RAIgGUgzRtqWx98KkgKNDyeEmBmhpptW966iS7+c8ig4ksMCIEyzhATMpiI4pHzH
-xSwZMvo3y3wkMwgf/WrhwdCyZNku
------END CERTIFICATE-----`
-
-var cacert = `-----BEGIN CERTIFICATE-----
-MIICHDCCAcKgAwIBAgIJAJ/qse7uYF0LMAoGCCqGSM49BAMCMGExCzAJBgNVBAYT
-AkdCMRAwDgYDVQQIDAdFbmdsYW5kMRAwDgYDVQQHDAdJcHN3aWNoMQ4wDAYDVQQK
-DAVCYXIxOTEOMAwGA1UECwwFQmFyMTkxDjAMBgNVBAMMBUJhcjE5MB4XDTE3MDIw
-OTE1MzE1MloXDTM3MDIwNDE1MzE1MlowYTELMAkGA1UEBhMCR0IxEDAOBgNVBAgM
-B0VuZ2xhbmQxEDAOBgNVBAcMB0lwc3dpY2gxDjAMBgNVBAoMBUJhcjE5MQ4wDAYD
-VQQLDAVCYXIxOTEOMAwGA1UEAwwFQmFyMTkwWTATBgcqhkjOPQIBBggqhkjOPQMB
-BwNCAAQcG4qwA7jeGzgkakV+IYyQH/GwgtOw6+Y3ZabCmw8dk0vrDwdZ7fEI9C10
-b9ckm9n4LvnooSxQEzfLDk9N+S7yo2MwYTAdBgNVHQ4EFgQU4UJ1xRnh6zeW2IKA
-BUOjIt9Wk8gwHwYDVR0jBBgwFoAU4UJ1xRnh6zeW2IKABUOjIt9Wk8gwDwYDVR0T
-AQH/BAUwAwEB/zAOBgNVHQ8BAf8EBAMCAYYwCgYIKoZIzj0EAwIDSAAwRQIgGvB0
-854QmGi1yG5wnWMiwzQxtcEhvCXbnCuiQvr5VrkCIQDoMooDC/WmhBwuCfo7iGDo
-AsFd44a8aa9yzABfALG2Gw==
------END CERTIFICATE-----`
-
 func TestMSPWithIntermediateCAs(t *testing.T) {
-	keyinfo := &msp.KeyInfo{KeyIdentifier: "PEER", KeyMaterial: []byte(key)}
-
-	sigid := &msp.SigningIdentityInfo{PublicSigner: []byte(signcert), PrivateSigner: keyinfo}
-
-	cryptoConfig := &msp.FabricCryptoConfig{
-		SignatureHashFamily:            bccsp.SHA2,
-		IdentityIdentifierHashFunction: bccsp.SHA256,
-	}
-
-	fmspconf := &msp.FabricMSPConfig{
-		RootCerts:         [][]byte{[]byte(cacert)},
-		IntermediateCerts: [][]byte{[]byte(intermediatecert)},
-		SigningIdentity:   sigid,
-		Name:              "DEFAULT",
-		CryptoConfig:      cryptoConfig}
-
-	fmpsjs, _ := proto.Marshal(fmspconf)
-
-	mspconf := &msp.MSPConfig{Config: fmpsjs, Type: int32(FABRIC)}
-
-	thisMSP, err := NewBccspMsp()
-	assert.NoError(t, err)
-
-	err = thisMSP.Setup(mspconf)
-	assert.NoError(t, err)
+	// testdata/intermediate contains the credentials for a test MSP setup that has
+	// 1) a key and a signcert (used to populate the default signing identity);
+	//    signcert is not signed by a CA directly but by an intermediate CA
+	// 2) intermediatecert is an intermediate CA, signed by the CA
+	// 3) cacert is the CA that signed the intermediate
+	thisMSP := getLocalMSP(t, "testdata/intermediate")
 
 	// This MSP will trust any cert signed by the CA directly OR by the intermediate
 
@@ -136,33 +53,13 @@ func TestMSPWithIntermediateCAs(t *testing.T) {
 }
 
 func TestIntermediateCAIdentityValidity(t *testing.T) {
-	keyinfo := &msp.KeyInfo{KeyIdentifier: "PEER", KeyMaterial: []byte(key)}
-
-	sigid := &msp.SigningIdentityInfo{PublicSigner: []byte(signcert), PrivateSigner: keyinfo}
-
-	cryptoConfig := &msp.FabricCryptoConfig{
-		SignatureHashFamily:            bccsp.SHA2,
-		IdentityIdentifierHashFunction: bccsp.SHA256,
-	}
-
-	fmspconf := &msp.FabricMSPConfig{
-		RootCerts:         [][]byte{[]byte(cacert)},
-		IntermediateCerts: [][]byte{[]byte(intermediatecert)},
-		SigningIdentity:   sigid,
-		Name:              "DEFAULT",
-		CryptoConfig:      cryptoConfig}
-
-	fmpsjs, _ := proto.Marshal(fmspconf)
-
-	mspconf := &msp.MSPConfig{Config: fmpsjs, Type: int32(FABRIC)}
-
-	thisMSP, err := NewBccspMsp()
-	assert.NoError(t, err)
-
-	err = thisMSP.Setup(mspconf)
-	assert.NoError(t, err)
-
-	id, _, err := thisMSP.(*bccspmsp).getIdentityFromConf([]byte(intermediatecert))
-	assert.NoError(t, err)
+	// testdata/intermediate contains the credentials for a test MSP setup that has
+	// 1) a key and a signcert (used to populate the default signing identity);
+	//    signcert is not signed by a CA directly but by an intermediate CA
+	// 2) intermediatecert is an intermediate CA, signed by the CA
+	// 3) cacert is the CA that signed the intermediate
+	thisMSP := getLocalMSP(t, "testdata/intermediate")
+
+	id := thisMSP.(*bccspmsp).intermediateCerts[0]
 	assert.Error(t, id.Validate())
 }

msp/ouconfig_test.go

@@ -17,8 +17,6 @@ limitations under the License.
 package msp
 
 import (
-	"fmt"
-	"os"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
@@ -28,17 +26,7 @@ func TestBadConfigOU(t *testing.T) {
 	// testdata/badconfigou:
 	// the configuration is such that only identities
 	// with OU=COP2 and signed by the root ca should be validated
-	conf, err := GetLocalMspConfig("testdata/badconfigou", nil, "DEFAULT")
-	if err != nil {
-		fmt.Printf("Setup should have succeeded, got err %s instead", err)
-		os.Exit(-1)
-	}
-
-	thisMSP, err := NewBccspMsp()
-	assert.NoError(t, err)
-
-	err = thisMSP.Setup(conf)
-	assert.NoError(t, err)
+	thisMSP := getLocalMSP(t, "testdata/badconfigou")
 
 	id, err := thisMSP.GetDefaultSigningIdentity()
 	assert.NoError(t, err)

msp/revocation_test.go

@@ -23,25 +23,12 @@ import (
 	"github.com/stretchr/testify/assert"
 )
 
-func getRevocationMSP(t *testing.T) MSP {
+func TestRevocation(t *testing.T) {
 	// testdata/revocation
 	// 1) a key and a signcert (used to populate the default signing identity);
 	// 2) cacert is the CA that signed the intermediate;
 	// 3) a revocation list that revokes signcert
-	conf, err := GetLocalMspConfig("testdata/revocation", nil, "DEFAULT")
-	assert.NoError(t, err)
-
-	thisMSP, err := NewBccspMsp()
-	assert.NoError(t, err)
-
-	err = thisMSP.Setup(conf)
-	assert.NoError(t, err)
-
-	return thisMSP
-}
-
-func TestRevocation(t *testing.T) {
-	thisMSP := getRevocationMSP(t)
+	thisMSP := getLocalMSP(t, "testdata/revocation")
 
 	id, err := thisMSP.GetDefaultSigningIdentity()
 	assert.NoError(t, err)
@@ -52,7 +39,11 @@ func TestRevocation(t *testing.T) {
 }
 
 func TestIdentityPolicyPrincipalAgainstRevokedIdentity(t *testing.T) {
-	thisMSP := getRevocationMSP(t)
+	// testdata/revocation
+	// 1) a key and a signcert (used to populate the default signing identity);
+	// 2) cacert is the CA that signed the intermediate;
+	// 3) a revocation list that revokes signcert
+	thisMSP := getLocalMSP(t, "testdata/revocation")
 
 	id, err := thisMSP.GetDefaultSigningIdentity()
 	assert.NoError(t, err)