Restore ability to use RSA CA with Fabric MSP #2121
Conversation
sykesm
force-pushed
the
rsa-ca-master
branch
from
de0c320
to
194e139
Compare
| network.Peer("Org2", "peer0"), | ||
| ) | ||
| nwo.DeployChaincode(network, "testchannel", orderer, chaincode) | ||
| RunQueryInvokeQuery(network, orderer, org1Peer0, 100) |
There was a problem hiding this comment.
How about fetching the channel config and ensuring that the CAs are indeed RSA?
There was a problem hiding this comment.
Go for it. Since they were explicitly generated by the code below the test and the test failed before changing the production code (ie test-driven), I personally don't see the value.
There was a problem hiding this comment.
Thanks Matt! I'm just wondering whether it'd be actually better to exercise the existing e2e integration tests (e.g. those in integration/e2e/e2e_test.go) with the only difference that one of the CAs uses RSA. Also, shouldn't we test whether such a channel is upgradable?
| // where the CAs use RSA keys and everything else is ECDSA. It's not | ||
| // pretty but it gets the job done for testing. | ||
|
|
||
| func generateRSACACrypto(n *nwo.Network) { |
There was a problem hiding this comment.
So I don't understand why we are essentially replicating what cryptogen is doing only with RSA CAs.
Isn't it simpler to:
- Run cryptogen
- Locate all CAs, replace their public key and re-sign them.
- Locate all certificate, replace their signature with a new signature
?
There was a problem hiding this comment.
This started out with the assumption that RSA was used throughout the MSP so this code generated an entire MSP tree with RSA. When it became clear that signing certificates could not use RSA, I changed that path to use ECDSA. As you can see, that resulted in the introduction of generateECKey and a call to it from one site.
As the comment at the top of the code says, "It's not pretty but it gets the job done for testing." I stand by that. Changing this to walk through all of the crypto directories to re-sign certificates is nearly as complicated as what's here. What's here now works, is self-contained, and isolated to the single test that needs it. I do not believe rewriting it to parse and re-sign certs is worth the effort.
| Expect(err).NotTo(HaveOccurred()) | ||
| writeCertificate(filepath.Join(tlsDir, "ca.crt"), tlsCA.certBytes) | ||
|
|
||
| tlsKey := generateRSAKey() |
There was a problem hiding this comment.
I understand you're testing this, but why is this needed? BCCSP never processes any TLS certificates so the production code change is not affecting this part of the test.
There was a problem hiding this comment.
Nothing in the integration tests RSA TLS certificates. Since this test is explicitly targeted to RSA, it seemed reasonable to have something here to help prevent future regressions.
While RSA has never been supported by Fabric for transaction signing and verification, prior to version 2.x, MSPs could include CA certificates that included RSA public keys. This was broken with the removal of the unsupported RSA code. This change modifies the key import function used by the MSP to convert an RSA public key to a bccsp.Key that can then be used as part of an MSP identity. This code does *not* add support for RSA on transaction paths. Signed-off-by: Matthew Sykes <sykesmat@us.ibm.com>
Signed-off-by: Matthew Sykes <sykesmat@us.ibm.com>
Signed-off-by: Matthew Sykes <sykesmat@us.ibm.com>
sykesm
force-pushed
the
rsa-ca-master
branch
from
194e139
to
2e14af3
Compare
While RSA has never been supported by Fabric for transaction signing and verification, prior to version 2.x, MSPs could include CA certificates that included RSA public keys. This was broken with the removal of the unsupported RSA code.
This change modifies the key import function used by the MSP to convert an RSA public key to a bccsp.Key that can then be used as part of an MSP identity.
This code does not add support for RSA on transaction paths.