[FAB-4003] OU certificates fail to match

When checking the OU certificate against the intermediates and roots,
the OU certificate is extracted using x509.ParseCertificate whereas the
intermediates and roots are first sanitised then reimported through
msp.bccsp.KeyImport. The reimporting has the potential to change
the signature algorithm which in turn results in certificates not
matching, even though they are the same.

There were also inconsistencies with how intermediate chains were being
validated. Some functions returned only the parent certificate.

When an OU identifiers configuration referenced an intermediate
certificate, chain validation would fail, as the root certificate was
assumed to be the signing parent.

Change-Id: If866cb4972e46a7fdb82b64947a997f563c242fe
Signed-off-by: Nick Murray <[email protected]>

Author: pkpfr

msp/msp_test.go

@@ -411,7 +411,7 @@ func TestSignAndVerify(t *testing.T) {
 func TestSignAndVerifyFailures(t *testing.T) {
 	msg := []byte("foo")
 
-	id, err := localMsp.GetDefaultSigningIdentity()
+	id, err := localMspBad.GetDefaultSigningIdentity()
 	if err != nil {
 		t.Fatalf("GetSigningIdentity should have succeeded")
 		return
@@ -512,7 +512,7 @@ func TestGetOU(t *testing.T) {
 }
 
 func TestGetOUFail(t *testing.T) {
-	id, err := localMsp.GetDefaultSigningIdentity()
+	id, err := localMspBad.GetDefaultSigningIdentity()
 	if err != nil {
 		t.Fatalf("GetSigningIdentity should have succeeded")
 		return
@@ -836,6 +836,9 @@ func TestIdentityPolicyPrincipalFails(t *testing.T) {
 
 var conf *msp.MSPConfig
 var localMsp MSP
+
+// Required because deleting the cert or msp options from localMsp causes parallel tests to fail
+var localMspBad MSP
 var mspMgr MSPManager
 
 func TestMain(m *testing.M) {
@@ -858,12 +861,24 @@ func TestMain(m *testing.M) {
 		os.Exit(-1)
 	}
 
+	localMspBad, err = NewBccspMsp()
+	if err != nil {
+		fmt.Printf("Constructor for msp should have succeeded, got err %s instead", err)
+		os.Exit(-1)
+	}
+
 	err = localMsp.Setup(conf)
 	if err != nil {
 		fmt.Printf("Setup for msp should have succeeded, got err %s instead", err)
 		os.Exit(-1)
 	}
 
+	err = localMspBad.Setup(conf)
+	if err != nil {
+		fmt.Printf("Setup for msp should have succeeded, got err %s instead", err)
+		os.Exit(-1)
+	}
+
 	mspMgr = NewMSPManager()
 	err = mspMgr.Setup([]MSP{localMsp})
 	if err != nil {

msp/mspimpl.go

@@ -680,11 +680,14 @@ func (msp *bccspmsp) getCertificationChainForBCCSPIdentity(id *identity) ([]*x50
 		return nil, errors.New("A CA certificate cannot be used directly by this MSP")
 	}
 
-	return msp.getValidationChain(id.cert)
+	return msp.getValidationChain(id.cert, false)
 }
 
 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
+	if msp.opts == nil {
+		return nil, fmt.Errorf("The supplied identity has no verify options")
+	}
 	validationChains, err := cert.Verify(*(msp.opts))
 	if err != nil {
 		return nil, fmt.Errorf("The supplied identity is not valid, Verify() returned %s", err)
@@ -700,7 +703,7 @@ func (msp *bccspmsp) getUniqueValidationChain(cert *x509.Certificate) ([]*x509.C
 	return validationChains[0], nil
 }
 
-func (msp *bccspmsp) getValidationChain(cert *x509.Certificate) ([]*x509.Certificate, error) {
+func (msp *bccspmsp) getValidationChain(cert *x509.Certificate, isIntermediateChain bool) ([]*x509.Certificate, error) {
 	validationChain, err := msp.getUniqueValidationChain(cert)
 	if err != nil {
 		return nil, fmt.Errorf("Failed getting validation chain %s", err)
@@ -712,10 +715,14 @@ func (msp *bccspmsp) getValidationChain(cert *x509.Certificate) ([]*x509.Certifi
 	}
 
 	// check that the parent is a leaf of the certification tree
-	if msp.certificationTreeInternalNodesMap[string(validationChain[1].Raw)] {
+	// if validating an intermediate chain, the first certificate will the parent
+	parentPosition := 1
+	if isIntermediateChain {
+		parentPosition = 0
+	}
+	if msp.certificationTreeInternalNodesMap[string(validationChain[parentPosition].Raw)] {
 		return nil, fmt.Errorf("Invalid validation chain. Parent certificate should be a leaf of the certification tree [%v].", cert.Raw)
 	}
-
 	return validationChain, nil
 }
 
@@ -753,14 +760,21 @@ func (msp *bccspmsp) getCertificationChainIdentifierFromChain(chain []*x509.Cert
 func (msp *bccspmsp) setupOUs(conf m.FabricMSPConfig) error {
 	msp.ouIdentifiers = make(map[string][][]byte)
 	for _, ou := range conf.OrganizationalUnitIdentifiers {
-		// 1. check that it registered in msp.rootCerts or msp.intermediateCerts
+
+		// 1. check that certificate is registered in msp.rootCerts or msp.intermediateCerts
 		cert, err := msp.getCertFromPem(ou.Certificate)
 		if err != nil {
 			return fmt.Errorf("Failed getting certificate for [%v]: [%s]", ou, err)
 		}
 
+		// 2. Sanitize it to ensure like for like comparison
+		cert, err = msp.sanitizeCert(cert)
+		if err != nil {
+			return fmt.Errorf("sanitizeCert failed %s", err)
+		}
+
 		found := false
-		root := true
+		root := false
 		// Search among root certificates
 		for _, v := range msp.rootCerts {
 			if v.(*identity).cert.Equal(cert) {
@@ -783,19 +797,19 @@ func (msp *bccspmsp) setupOUs(conf m.FabricMSPConfig) error {
 			return fmt.Errorf("Failed adding OU. Certificate [%v] not in root or intermediate certs.", ou.Certificate)
 		}
 
-		// 2. get the certification path for it
+		// 3. get the certification path for it
 		var certifiersIdentitifer []byte
 		var chain []*x509.Certificate
 		if root {
 			chain = []*x509.Certificate{cert}
 		} else {
-			chain, err = msp.getValidationChain(cert)
+			chain, err = msp.getValidationChain(cert, true)
 			if err != nil {
 				return fmt.Errorf("Failed computing validation chain for [%v]. [%s]", cert, err)
 			}
 		}
 
-		// 3. compute the hash of the certification path
+		// 4. compute the hash of the certification path
 		certifiersIdentitifer, err = msp.getCertificationChainIdentifierFromChain(chain)
 		if err != nil {
 			return fmt.Errorf("Failed computing Certifiers Identifier for [%v]. [%s]", ou.Certificate, err)
@@ -969,6 +983,9 @@ func (msp *bccspmsp) validateIdentityOUs(id *identity) error {
 		}
 
 		if !found {
+			if len(id.GetOrganizationalUnits()) == 0 {
+				return fmt.Errorf("The identity certificate does not contain an Organizational Unit (OU)")
+			}
 			return fmt.Errorf("None of the identity's organizational units [%v] are in MSP %s", id.GetOrganizationalUnits(), msp.name)
 		}
 	}

msp/mspwithintermediatecas_test.go

@@ -53,6 +53,28 @@ func TestMSPWithIntermediateCAs(t *testing.T) {
 	assert.Error(t, err)
 }
 
+func TestMSPWithExternalIntermediateCAs(t *testing.T) {
+	// testdata/external contains the credentials for a test MSP setup
+	// identical to testdata/intermediate with the exception that it has
+	// been generated independently of the fabric environment using
+	// openssl.  Sanitizing certificates may cause a change in the
+	// signature algorithm used from that used in original
+	// certificate file.  Hashes of raw certificate bytes and
+	// byte to byte comparisons between the raw certificate and the
+	// one imported into the MSP could falsely fail.
+
+	thisMSP := getLocalMSP(t, "testdata/external")
+
+	// This MSP will trust any cert signed only by the intermediate
+
+	id, err := thisMSP.GetDefaultSigningIdentity()
+	assert.NoError(t, err)
+
+	// ensure that we validate correctly the identity
+	err = thisMSP.Validate(id.GetPublicVersion())
+	assert.NoError(t, err)
+}
+
 func TestIntermediateCAIdentityValidity(t *testing.T) {
 	// 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);

msp/ouconfig_test.go

@@ -17,8 +17,10 @@ limitations under the License.
 package msp
 
 import (
+	"path/filepath"
 	"testing"
 
+	"github.com/hyperledger/fabric/bccsp/sw"
 	"github.com/stretchr/testify/assert"
 )
 
@@ -50,3 +52,30 @@ func TestBadConfigOUCert(t *testing.T) {
 	err = thisMSP.Setup(conf)
 	assert.Error(t, err)
 }
+
+func TestValidateIntermediateConfigOU(t *testing.T) {
+	// testdata/external:
+	// the configuration is such that only identities with
+	// OU=Hyperledger Testing and signed by the intermediate ca should be validated
+	thisMSP := getLocalMSP(t, "testdata/external")
+
+	id, err := thisMSP.GetDefaultSigningIdentity()
+	assert.NoError(t, err)
+
+	err = id.Validate()
+	assert.NoError(t, err)
+
+	conf, err := GetLocalMspConfig("testdata/external", nil, "DEFAULT")
+	assert.NoError(t, err)
+
+	thisMSP, err = NewBccspMsp()
+	assert.NoError(t, err)
+	ks, err := sw.NewFileBasedKeyStore(nil, filepath.Join("testdata/external", "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)
+}

msp/testdata/external/admincerts/admin.pem

@@ -0,0 +1,17 @@
+-----BEGIN CERTIFICATE-----
+MIICnTCCAkOgAwIBAgICEAEwCgYIKoZIzj0EAwIwgbkxCzAJBgNVBAYTAkhLMSMw
+IQYDVQQIDBpDZW50cmFsICYgV2VzdGVybiBEaXN0cmljdDETMBEGA1UEBwwKU2hl
+dW5nIFdhbjEWMBQGA1UECgwNUGFzc0tpdCwgSW5jLjEcMBoGA1UECwwTSHlwZXJs
+ZWRnZXIgVGVzdGluZzE6MDgGA1UEAwwxUGFzc0tpdCwgaW5jLiBIeXBlcmxlZGdl
+ciBUZXN0aW5nIEludGVybWVkaWF0ZSBDQTAeFw0xNzA1MjEwNTMzMjJaFw0yNzA1
+MjEwNTMzMjJaMIGVMQswCQYDVQQGEwJISzEjMCEGA1UECAwaQ2VudHJhbCAmIFdl
+c3Rlcm4gRGlzdHJpY3QxEzARBgNVBAcMClNoZXVuZyBXYW4xFjAUBgNVBAoMDVBh
+c3NLaXQsIEluYy4xHDAaBgNVBAsME0h5cGVybGVkZ2VyIFRlc3RpbmcxFjAUBgNV
+BAMMDVRlc3RpbmcvQWRtaW4wWTATBgcqhkjOPQIBBggqhkjOPQMBBwNCAASjcy4w
+cnFYy/gk+LhRdpDhZTVglV2K7M4neXqYn/4pdBgKgtXYXGVW4S5/hBrq3n6CfTAY
+R+VBzxk6kSzSCrWQo10wWzAJBgNVHRMEAjAAMB0GA1UdDgQWBBSyNnQXoP7Fw/ZZ
+vPYYSwrgcnGeKjAfBgNVHSMEGDAWgBRl2UHkjIPnmo8DNz0gHfpNPHJlYTAOBgNV
+HQ8BAf8EBAMCAgQwCgYIKoZIzj0EAwIDSAAwRQIgHJi//h3WQU2Oi4VRjIcCroEJ
+Kigijt/cUwWxojf+47wCIQDyUHbdA8qxW9m2CuHZbI3eKEHhqUlhemU0VbG0BIor
+Pg==
+-----END CERTIFICATE-----

msp/testdata/external/cacerts/cacert.pem

@@ -0,0 +1,14 @@
+-----BEGIN CERTIFICATE-----
+MIICNTCCAdqgAwIBAgIJAKveeQnW2k9/MAoGCCqGSM49BAMCMG0xCzAJBgNVBAYT
+AlVTMREwDwYDVQQIDAhEZWxhd2FyZTETMBEGA1UEBwwKV2lsbWluZ3RvbjEWMBQG
+A1UECgwNUGFzc0tpdCwgSW5jLjEeMBwGA1UEAwwVUGFzc0tpdCwgSW5jLiBSb290
+IENBMB4XDTE3MDUyMTA1MDc1N1oXDTQ3MDUyMTA1MDc1N1owbTELMAkGA1UEBhMC
+VVMxETAPBgNVBAgMCERlbGF3YXJlMRMwEQYDVQQHDApXaWxtaW5ndG9uMRYwFAYD
+VQQKDA1QYXNzS2l0LCBJbmMuMR4wHAYDVQQDDBVQYXNzS2l0LCBJbmMuIFJvb3Qg
+Q0EwWTATBgcqhkjOPQIBBggqhkjOPQMBBwNCAAQ4gQhu+Br8vv2X+i9bsxcdP5kk
+wG5hOyJZ5o7QwfGibeuIdno4ZvbvRoZTlJwiPXdbUhHDIpZL99UClMj2CYpPo2Mw
+YTAdBgNVHQ4EFgQUH7y0pJdCaLecYlWuwticHsa+50wwHwYDVR0jBBgwFoAUH7y0
+pJdCaLecYlWuwticHsa+50wwDwYDVR0TAQH/BAUwAwEB/zAOBgNVHQ8BAf8EBAMC
+AQYwCgYIKoZIzj0EAwIDSQAwRgIhAP2Nbs/X36XQu6saYPaUkflvFIPBi07JKnY2
+MN+LWGyKAiEAwbTlddmOZOIwI3KlAN/n5szqEFE/ksYBHPJ6+9yb4NY=
+-----END CERTIFICATE-----

msp/testdata/external/config.yaml

@@ -0,0 +1,3 @@
+OrganizationalUnitIdentifiers:
+  - Certificate: "intermediatecerts/intermediatecert.pem"
+    OrganizationalUnitIdentifier: "Hyperledger Testing"

msp/testdata/external/intermediatecerts/intermediatecert.pem

@@ -0,0 +1,17 @@
+-----BEGIN CERTIFICATE-----
+MIICtDCCAlugAwIBAgICEAEwCgYIKoZIzj0EAwIwbTELMAkGA1UEBhMCVVMxETAP
+BgNVBAgMCERlbGF3YXJlMRMwEQYDVQQHDApXaWxtaW5ndG9uMRYwFAYDVQQKDA1Q
+YXNzS2l0LCBJbmMuMR4wHAYDVQQDDBVQYXNzS2l0LCBJbmMuIFJvb3QgQ0EwHhcN
+MTcwNTIxMDUxMzE4WhcNMzcwNTIxMDUxMzE4WjCBuTELMAkGA1UEBhMCSEsxIzAh
+BgNVBAgMGkNlbnRyYWwgJiBXZXN0ZXJuIERpc3RyaWN0MRMwEQYDVQQHDApTaGV1
+bmcgV2FuMRYwFAYDVQQKDA1QYXNzS2l0LCBJbmMuMRwwGgYDVQQLDBNIeXBlcmxl
+ZGdlciBUZXN0aW5nMTowOAYDVQQDDDFQYXNzS2l0LCBpbmMuIEh5cGVybGVkZ2Vy
+IFRlc3RpbmcgSW50ZXJtZWRpYXRlIENBMFkwEwYHKoZIzj0CAQYIKoZIzj0DAQcD
+QgAERrxO6dbpvlNO+Il/dMvaJZ0lpuh94jv48DlP+hi1ekVBJa/R6/jY8HVOalG3
+EXwyeOv0DOl9KrMJcQqAIyvyKqOBnTCBmjAdBgNVHQ4EFgQUZdlB5IyD55qPAzc9
+IB36TTxyZWEwHwYDVR0jBBgwFoAUH7y0pJdCaLecYlWuwticHsa+50wwDwYDVR0T
+AQH/BAUwAwEB/zAOBgNVHQ8BAf8EBAMCAQYwNwYDVR0fBDAwLjAsoCqgKIYmaHR0
+cHM6Ly9zc2wucGFzc2tpdC5jb20vZWNyb290LmNybC5wZW0wCgYIKoZIzj0EAwID
+RwAwRAIgYJ7aV3f/e9pMT5Ozc0aK2WWM5Gl/PDQ2VuM+0faVdlICIGOvS8k09R4g
+vDx8XOyDf2igFFUQ06abcufUcf6oj70d
+-----END CERTIFICATE-----

msp/testdata/external/keystore/key.pem

@@ -0,0 +1,5 @@
+-----BEGIN EC PRIVATE KEY-----
+MHcCAQEEIHACRdTlDyxk3FDWv2ysl6OnwX5KEKS7TiCOUOGkgq70oAoGCCqGSM49
+AwEHoUQDQgAEo3MuMHJxWMv4JPi4UXaQ4WU1YJVdiuzOJ3l6mJ/+KXQYCoLV2Fxl
+VuEuf4Qa6t5+gn0wGEflQc8ZOpEs0gq1kA==
+-----END EC PRIVATE KEY-----

msp/testdata/external/signcerts/cert.pem

@@ -0,0 +1,17 @@
+-----BEGIN CERTIFICATE-----
+MIICnTCCAkOgAwIBAgICEAEwCgYIKoZIzj0EAwIwgbkxCzAJBgNVBAYTAkhLMSMw
+IQYDVQQIDBpDZW50cmFsICYgV2VzdGVybiBEaXN0cmljdDETMBEGA1UEBwwKU2hl
+dW5nIFdhbjEWMBQGA1UECgwNUGFzc0tpdCwgSW5jLjEcMBoGA1UECwwTSHlwZXJs
+ZWRnZXIgVGVzdGluZzE6MDgGA1UEAwwxUGFzc0tpdCwgaW5jLiBIeXBlcmxlZGdl
+ciBUZXN0aW5nIEludGVybWVkaWF0ZSBDQTAeFw0xNzA1MjEwNTMzMjJaFw0yNzA1
+MjEwNTMzMjJaMIGVMQswCQYDVQQGEwJISzEjMCEGA1UECAwaQ2VudHJhbCAmIFdl
+c3Rlcm4gRGlzdHJpY3QxEzARBgNVBAcMClNoZXVuZyBXYW4xFjAUBgNVBAoMDVBh
+c3NLaXQsIEluYy4xHDAaBgNVBAsME0h5cGVybGVkZ2VyIFRlc3RpbmcxFjAUBgNV
+BAMMDVRlc3RpbmcvQWRtaW4wWTATBgcqhkjOPQIBBggqhkjOPQMBBwNCAASjcy4w
+cnFYy/gk+LhRdpDhZTVglV2K7M4neXqYn/4pdBgKgtXYXGVW4S5/hBrq3n6CfTAY
+R+VBzxk6kSzSCrWQo10wWzAJBgNVHRMEAjAAMB0GA1UdDgQWBBSyNnQXoP7Fw/ZZ
+vPYYSwrgcnGeKjAfBgNVHSMEGDAWgBRl2UHkjIPnmo8DNz0gHfpNPHJlYTAOBgNV
+HQ8BAf8EBAMCAgQwCgYIKoZIzj0EAwIDSAAwRQIgHJi//h3WQU2Oi4VRjIcCroEJ
+Kigijt/cUwWxojf+47wCIQDyUHbdA8qxW9m2CuHZbI3eKEHhqUlhemU0VbG0BIor
+Pg==
+-----END CERTIFICATE-----