Validate TLS certs during raft consenter addition #1223
Conversation
| return errors.WithMessage(err, "creating x509 verify options") | ||
| } | ||
|
|
||
| // following the lead of msp/mspimplvalidate.go: |
There was a problem hiding this comment.
This is for TLS, so we actually do care about the time. If the CA are expired or too early, the TLS handshake won't work.
There was a problem hiding this comment.
Ah, I see. Makes sense. I'll fix that. Thanks!
| return x509.VerifyOptions{ | ||
| Roots: tlsRoots, | ||
| Intermediates: tlsIntermediates, | ||
| KeyUsages: []x509.ExtKeyUsage{ |
There was a problem hiding this comment.
what if we only have one of the two key usages? won't this fail?
There was a problem hiding this comment.
No, this accepts certificates as long as they have any of the key usages specified.
There was a problem hiding this comment.
Thinking a little more while I'm working on the negative integration test: do/should we verify the key usages separately, i.e. check that the ServerTlsCert has the server key usage and the ClientTlsCert has the client key usage?
There was a problem hiding this comment.
I don't think so, there is a limit to which we need to coddle the users.
| return errors.Wrap(err, "parsing tls server cert") | ||
| } | ||
|
|
||
| for _, org := range ordererConfig.Organizations() { |
There was a problem hiding this comment.
why do you need to loop around the organizations? Just construct the TLS CA cert pools from the union of all the CA certs over all MSPs, and give it a go. The x509 library should be able to find a validation path if such exists.
There was a problem hiding this comment.
I did this due to the assumption that we'd want the client and server TLS certs to be valid under the same organization's MSP. I can change this if that assumption is wrong.
There was a problem hiding this comment.
In the end of the day, the check should validate the current behavior of TLS validation - which actually aggregates all TLS root/intermediate CA certificates and also adds the ones in the local config.
There was a problem hiding this comment.
Combined into a single set of VerifyOptions with all TLS root/intermediate CA certs.
integration/raft/config_test.go
Outdated
| By("Ensuring orderer3 detects leader loss") | ||
| leaderLoss := fmt.Sprintf("Raft leader changed: %d -> 0", leader) | ||
| Eventually(ordererRunners[2].Err(), network.EventuallyTimeout, time.Second).Should(gbytes.Say(leaderLoss)) | ||
| By("removing orderer3's TLS root CA certificate") |
There was a problem hiding this comment.
It's too early to remove that consenter. You just added it and you are immediately removing it from the config.
We need to wait for the consenter to onboard, and make sure it participates in consensus and only then we need to remove it.
Otherwise it might be a false positive test execution.
There was a problem hiding this comment.
I've updated this by waiting for orderer3 to see the leader but let me know if there's anything else we can/should do here.
| By("ensuring TLS handshakes fail with the other orderers") | ||
| for i, oRunner := range ordererRunners { | ||
| if i < 2 { | ||
| Eventually(oRunner.Err(), network.EventuallyTimeout).Should(gbytes.Say("TLS handshake failed with error tls: client didn't provide a certificate")) |
There was a problem hiding this comment.
Is it because the rest of the OSNs don't want to connect to complete the handshake once they see our server certificate?
There was a problem hiding this comment.
Not sure on the exact mechanics here but I can research/investigate.
There was a problem hiding this comment.
Made a quick pass, gave some comments.
I'm also missing the integration test that checks that you are trying to add a consenter with a mismatching certificate and it rejects you.
I know you did a unit test, but IMO we have too much super-glue between different components (etcdraft chain, channel configtx, etc.) so we'd want to be on the safe side.
I guess you can just addConsenter with an invalid cert and catch the return error in the config test.
wlahti
force-pushed
the
fab-17733
branch
2 times, most recently
from
806053d
to
22624e9
Compare
|
Thanks for the review and insight into Raft/TLS, Yacov! Addressed most of the comments now. Let me see what I can do for the negative case in the integration test. |
- For negative tests, test that expected error message is returned. - For positive tests, switch the order so expected/actual match the expected usage of "testify/require" package. Signed-off-by: Will Lahti <wtlahti@us.ibm.com>
FAB-17733 #done Signed-off-by: Will Lahti <wtlahti@us.ibm.com>
* [FAB-17819] Discovery returns user friendly errors Currently, the discovery service filters out peers that don't have the chaincode installed early on in the computation, and as a result - the service cannot distinguish from a case where there are not enough alive peers to satisfy the endorsement policy, or that there are enough peers but the chaincode is not installed on enough of them. This change set defers the chaincode filtering to the end of the computation, so the layouts and peer group mapping is creating without taking into account if the peers have the chaincode installed on them, and if there is no layout that can be satisfied without taking into account the chaincodes - the error that is returned now is "no peer combination can satisfy the endorsement policy", instead of "cannot satisfy any principal combination". Afterwards, the layouts are being inspected once again, and then the layouts that cannot be satisfied are filtered out, when the error returned when no layout can be satisfied is now: "required chaincodes are not installed on sufficient peers". Change-Id: I74eb29b30aec1a87842d220414c73872cdbc8304 Signed-off-by: yacovm <yacovm@il.ibm.com> * Fix docker network leak from RAFT integration test (hyperledger#1203) Signed-off-by: Matthew Sykes <matthew.sykes@gmail.com> * [FAB-17786] Update upgrade_dbs peer command to drop state couchdb (hyperledger#1187) The existing upgrade_dbs command does not automatically drop state couchdbs and therefore a separate step is required to drop couchdbs, This PR updates the command to automatically drop state couchdbs. In addition, it checks upgrade eligibility before upgrade so that it will not drop databases if it is already the expected format. Signed-off-by: Wenjian Qiao <wenjianq@gmail.com> * [FAB-17774] support orderer restart without genesis block (hyperledger#1197) * adding support for orderer restart without genesis block. Signed-off-by: Chongxin Luo <Chongxin.Luo@ibm.com> * Remove interface for block storage - Removed the interface for better code navigation, as There is only a single implementation for block storage - Merged the remaining code in the package blkstorgae with the implementation in the package fsblkstorage and used the name blkstorgae for the final package - Moved the internal single proto message with in same package Signed-off-by: manish <manish.sethi@gmail.com> * [LEDGER] Move UpdatesBytesBuilder to txmgr pkg (hyperledger#1209) - mv updateBatch bytes constructor to txmgr pkg Currently, we create the bytes representation of updateBatch to compute the hash of updateBatch which would be included in the block along with the validation results. The utility for converting a updateBatch to a deterministic bytes is present in the privacyenabledstate pkg. However, this utility is used only by the txmgr and hence, we move the utility function to txmgr pkg from privacyenabledstate pkg. - rename proto messages In the updateBatch proto, we have defined proto messages such as KVWriteProto and KVWriteBatchProto. As we shouldn't append the keyword proto to messages, we rename KVWriteProto to KVWrite and KVWriteBatchProto to Updates. - rename function names The function name such as buildForKeys(), buildForColls() are not very explicit in what they do. When the buildForColls() calls buildForKeys(), it adds even more confusion as the first-level function, i.e., deterministicBytesForPubAndHashUpdates() is also calling buildForKeys(). Hence, we have used the following function names instead: (1) genKVsFromNsUpdates() (2) genKVsFromCollsUpdates() (3) genKVs() FAB-17830 Signed-off-by: senthil <cendhu@gmail.com> * Update grpc-go to v1.29.1 (hyperledger#1213) Signed-off-by: Gari Singh <gari.r.singh@gmail.com> * [FAB-17831] Use generic constant/var names in dataformat.go (hyperledger#1212) Currently Version1x and Version20 are defined in dataformat.go. They should be renamed to more generic names such as PreviousFormat and CurrentFormat. The format values will change only when the data format is changed in ledger. If a Fabric version does not introduce a new data format, CurrentFormat will remain the same as the latest format prior to the Fabric version. Signed-off-by: Wenjian Qiao <wenjianq@gmail.com> * [LEDGER] rm interface from pvtdatastorage pkg (hyperledger#1217) rm pvtdatastorage interfaces As we have only one implementation of the pvtdatastore and not planning to any a new one, we can safely remove the interface. This also helps in code navigation and to avoid type casting such as s.(*store) in the test. FAB-17843 Signed-off-by: senthil <cendhu@gmail.com> * FAB-15710 Ch.Part.API: orderer config & hook into http server (hyperledger#1218) - Expose a configuration object for channel participation at the top level (like Metrics). Even though the channel participation API shares the same endpoint and TLS config with operations, placing it at the top level will allow us to separate these APIs to different endpoints in the future without changing the config structure. - Extend operations.System to be able to register a handler for additional APIs. - Implement a skeleton handler for the channel participation API. - Register the skeleton handler to the http server in operations.System during the server boot sequence. Signed-off-by: Yoav Tock <tock@il.ibm.com> Change-Id: I5cf15dffa29985cba60e5aaf31d189e755a3a1ef * Update the vagrant dev environment - Remove GOPATH in favor of modules - Move to Ubuntu 20.04 - Remove docker-compose as it's unnecessary for build and test Signed-off-by: Matthew Sykes <sykesmat@us.ibm.com> * Add function in blockstore to export TxIDs FAB-17837 Signed-off-by: manish <manish.sethi@gmail.com> * reduce #arguments in a few kvledger methods (hyperledger#1210) As the number of arguments passed to newKVLedger(), initTxMgr() is quite high, we reduce it by introducing initializer struct. FAB-17683 Signed-off-by: senthil <cendhu@gmail.com> * Clarify the deliver access denied message (hyperledger#1224) There are two scenarios where a deliver client could receive a 'FORBIDDEN' result when requesting blocks. Either the client was not authorized to connect to the channel initially, or, the client's access was revoked after a successful connection by some later configuration block. In both cases, we log an identical error message that "Client authorization revoked" when in fact, for the first case, the client may never have had access, so claiming it was revoked is misleading. Signed-off-by: Jason Yellick <jyellick@us.ibm.com> * Validate TLS certs during raft consenter addition (hyperledger#1223) * raft membership_test.go cleanup - For negative tests, test that expected error message is returned. - For positive tests, switch the order so expected/actual match the expected usage of "testify/require" package. Signed-off-by: Will Lahti <wtlahti@us.ibm.com> * Validate TLS certs during raft consenter addition FAB-17733 #done Signed-off-by: Will Lahti <wtlahti@us.ibm.com> * [FAB-17640] Remove pkg/configtx and import as hyperledger/fabric-config Signed-off-by: Danny Cao <dcao@us.ibm.com> * Update CONTRIBUTING guide Removed reference to Gerrit Signed-off-by: Ry Jones <ry@linux.com> Co-authored-by: yacovm <yacovm@il.ibm.com> Co-authored-by: Matthew Sykes <sykesmat@us.ibm.com> Co-authored-by: wen <wenjianq@gmail.com> Co-authored-by: Dereck <Chongxin.Luo@ibm.com> Co-authored-by: manish <manish.sethi@gmail.com> Co-authored-by: Senthil Nathan N <cendhu@users.noreply.github.com> Co-authored-by: Gari Singh <gari.r.singh@gmail.com> Co-authored-by: Yoav Tock <tock@il.ibm.com> Co-authored-by: Jason Yellick <jyellick@us.ibm.com> Co-authored-by: Will Lahti <wtlahti@us.ibm.com> Co-authored-by: Danny Cao <dcao@us.ibm.com> Co-authored-by: Ry Jones <ry@linux.com>
Type of change
Description
Related issues
FAB-17733