From c8731ae67ce3fc5af291b96a61c0696dd5970877 Mon Sep 17 00:00:00 2001 From: Nick Murray Date: Thu, 18 May 2017 18:37:55 +0800 Subject: [PATCH] [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 --- msp/msp_test.go | 19 ++++++++-- msp/mspimpl.go | 35 ++++++++++++++----- msp/mspwithintermediatecas_test.go | 22 ++++++++++++ msp/ouconfig_test.go | 29 +++++++++++++++ msp/testdata/external/admincerts/admin.pem | 17 +++++++++ msp/testdata/external/cacerts/cacert.pem | 14 ++++++++ msp/testdata/external/config.yaml | 3 ++ .../intermediatecerts/intermediatecert.pem | 17 +++++++++ msp/testdata/external/keystore/key.pem | 5 +++ msp/testdata/external/signcerts/cert.pem | 17 +++++++++ 10 files changed, 167 insertions(+), 11 deletions(-) create mode 100644 msp/testdata/external/admincerts/admin.pem create mode 100644 msp/testdata/external/cacerts/cacert.pem create mode 100644 msp/testdata/external/config.yaml create mode 100644 msp/testdata/external/intermediatecerts/intermediatecert.pem create mode 100644 msp/testdata/external/keystore/key.pem create mode 100644 msp/testdata/external/signcerts/cert.pem diff --git a/msp/msp_test.go b/msp/msp_test.go index d2ddce4e528..90d62405f75 100644 --- a/msp/msp_test.go +++ b/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 { diff --git a/msp/mspimpl.go b/msp/mspimpl.go index dc54e2c2bb4..e896a54a5a3 100644 --- a/msp/mspimpl.go +++ b/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) } } diff --git a/msp/mspwithintermediatecas_test.go b/msp/mspwithintermediatecas_test.go index 5cc3de361a8..eb5c160f470 100644 --- a/msp/mspwithintermediatecas_test.go +++ b/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); diff --git a/msp/ouconfig_test.go b/msp/ouconfig_test.go index b74bbfbf5f5..0980145e308 100644 --- a/msp/ouconfig_test.go +++ b/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) +} diff --git a/msp/testdata/external/admincerts/admin.pem b/msp/testdata/external/admincerts/admin.pem new file mode 100644 index 00000000000..43aaa90e399 --- /dev/null +++ b/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----- \ No newline at end of file diff --git a/msp/testdata/external/cacerts/cacert.pem b/msp/testdata/external/cacerts/cacert.pem new file mode 100644 index 00000000000..fa5b35362ab --- /dev/null +++ b/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----- \ No newline at end of file diff --git a/msp/testdata/external/config.yaml b/msp/testdata/external/config.yaml new file mode 100644 index 00000000000..8431ec34b0e --- /dev/null +++ b/msp/testdata/external/config.yaml @@ -0,0 +1,3 @@ +OrganizationalUnitIdentifiers: + - Certificate: "intermediatecerts/intermediatecert.pem" + OrganizationalUnitIdentifier: "Hyperledger Testing" \ No newline at end of file diff --git a/msp/testdata/external/intermediatecerts/intermediatecert.pem b/msp/testdata/external/intermediatecerts/intermediatecert.pem new file mode 100644 index 00000000000..d5c45d4d9bb --- /dev/null +++ b/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----- \ No newline at end of file diff --git a/msp/testdata/external/keystore/key.pem b/msp/testdata/external/keystore/key.pem new file mode 100644 index 00000000000..77ff6efdda9 --- /dev/null +++ b/msp/testdata/external/keystore/key.pem @@ -0,0 +1,5 @@ +-----BEGIN EC PRIVATE KEY----- +MHcCAQEEIHACRdTlDyxk3FDWv2ysl6OnwX5KEKS7TiCOUOGkgq70oAoGCCqGSM49 +AwEHoUQDQgAEo3MuMHJxWMv4JPi4UXaQ4WU1YJVdiuzOJ3l6mJ/+KXQYCoLV2Fxl +VuEuf4Qa6t5+gn0wGEflQc8ZOpEs0gq1kA== +-----END EC PRIVATE KEY----- \ No newline at end of file diff --git a/msp/testdata/external/signcerts/cert.pem b/msp/testdata/external/signcerts/cert.pem new file mode 100644 index 00000000000..43aaa90e399 --- /dev/null +++ b/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----- \ No newline at end of file