FAB-18192 Fixed TLS certs validation for consenters. #1888

kopaygorodsky · 2020-09-17T00:46:45Z

Signed-off-by: kopaygorodsky vlad.kopaygorodsky@gmail.com

Type of change

Bug fix

Description

Fixes usecase when you add new org with a consenter.
Verification of consenter's TLS certs against simulated config, not the last one. To achieve that, metadata validator interface was changed, now it requires orderer config instead of just consensus metadata. Also, TLS verification was moved to VerifyConfigMetadata function. Added ignoreCertExpiration option to ignore expiration errors when validating config metadata.

Additional details

1.consenter.IsChannelMember returns an error if metadata has a consenter with an invalid certificate or signed by unknown MSP.
2.Added new tests, fixed existing tests.
3.Checked the fix on a test network.

Related issues

https://jira.hyperledger.org/browse/FAB-18192

wlahti

Thanks for picking this task up! A few general comments that stood out on a first pass.

Going to take a deeper look at the more substantial changes in etcdraft/util.go.

integration/raft/config_test.go

internal/cryptogen/msp/msp.go

integration/raft/config_test.go

orderer/consensus/etcdraft/chain.go

orderer/consensus/etcdraft/consenter_test.go

orderer/consensus/etcdraft/util.go

orderer/consensus/etcdraft/validator_test.go

Signed-off-by: kopaygorodsky <vlad.kopaygorodsky@gmail.com>

jyellick

Thanks for this pull request! It's definitely a bug that we're not validating using the latest config. There are a few points, in particular since we are looking at the full config, and not performing the validation only on the update information, I'm a little concerned about unrelated config updates being rejected because of temporal checks. I've included a comment at the spot, but, I think we could simplify this code by simply skipping the expiration checks. If we are really convinced that they are the right thing to do, then we need to be careful that we only apply them to new certificates in the channel config, and not existing ones. If we apply it to existing ones, then as soon any orderer TLS cert expires, all channel updates would begin failing until the issue is fixed.

orderer/consensus/etcdraft/chain.go

jyellick · 2020-10-05T13:49:47Z

orderer/consensus/etcdraft/chain.go

 	if err != nil {
 		return err
 	}

+	//new config metadata was verified above. Additionally need to check new consenters for certificates expiration
+	for _, c := range changes.AddedNodes {
+		if err := validateConsenterTLSCerts(c, verifyOpts, false); err != nil {


I'm not sure that we should be checking certificate expiration here. For instance, we could have an expired certificate that we want to leave in place while we grow or shrink the consenter set. If we want to check expiration I'd say we need to only check expiration of new certificates.

I'd also note, that the orderer has been modified to perform ID validation not based on cert literals anymore, but rather based on public key, so, in some cases, even if the TLS certificate is expired in the channel config, an orderer with a valid and unexpired TLS certificate with matching public key may successfully be validated using this information.

@jyellick but slicechanges.AddedNodes contains only new nodes(only one node actually), doesn't it?

You're correct, sorry for missing this.

orderer/consensus/etcdraft/util_test.go

orderer/consensus/etcdraft/membership.go

…expiration is checked only on new consenters. Improved tests. Signed-off-by: kopaygorodsky <vlad.kopaygorodsky@gmail.com>

jyellick

I think this is in good enough shape to merge. I've opened https://jira.hyperledger.org/browse/FAB-18257 to address removing the test fixtures once go v1.15 is being used for Fabric.

wlahti · 2020-10-06T16:07:50Z

@jyellick Can you run the backport command to get this in the release-2.2 branch? Thanks! (I assume only maintainers are allowed to do that based on my previous failed attempts to run it myself)

@Mergifyio backport release-2.2

mergify · 2020-10-06T16:08:24Z

@wlahti is not allowed to run commands

jyellick · 2020-10-06T17:23:19Z

@Mergifyio backport release-2.2

* FAB-18192 Fixed TLS certs validation for consenters. Verification of TLS cert against simulated config, not the last one. To achieve that, metadata validator interface was changed, now it requires orderer config instead of just consensus metadata. Also, TLS verification was moved to VerifyMetadata function, it shouldn't have been part of ComputeMembershipChanges. Fixed tests. Signed-off-by: kopaygorodsky <vlad.kopaygorodsky@gmail.com> * fixed consenters tests Signed-off-by: kopaygorodsky <vlad.kopaygorodsky@gmail.com> * modified VerifyConfigMetadata with ignoreCertExpiration option Signed-off-by: kopaygorodsky <vlad.kopaygorodsky@gmail.com> * generation of verifying options based only on simulated config, fixes Signed-off-by: kopaygorodsky <vlad.kopaygorodsky@gmail.com> * fixed consenters map Signed-off-by: kopaygorodsky <vlad.kopaygorodsky@gmail.com> * Removed ignoreCertExpiration option from VerifyConfigMetadata, certs expiration is checked only on new consenters. Improved tests. Signed-off-by: kopaygorodsky <vlad.kopaygorodsky@gmail.com> (cherry picked from commit 886d3cc) # Conflicts: # orderer/common/msgprocessor/systemchannelfilter_test.go # orderer/common/multichannel/chainsupport_test.go # orderer/consensus/etcdraft/consenter.go # orderer/consensus/etcdraft/consenter_test.go # orderer/consensus/etcdraft/util_test.go

mergify · 2020-10-06T17:24:30Z

Command backport release-2.2: success

Backports have been created

#1971 FAB-18192 Fixed TLS certs validation for consenters. (bp #1888) has been created for branch release-2.2

kopaygorodsky · 2020-10-06T17:41:44Z

woohoo, my first PR to fabric is merged :)

robmurgai · 2020-10-07T16:09:14Z

Congratulations, Nice work :) S Rob Murgai He / Him / His Program Director - Hyperledger Fabric IBM Blockchain Raleigh-Durham, NC M: 919.342.8432 | LinkedIn: Rob-Murgai ----- Original message -----From: Vladyslav Kopaihorodskyi <notifications@github.com>To: hyperledger/fabric <fabric@noreply.github.com>Cc: Subscribed <subscribed@noreply.github.com>Subject: [EXTERNAL] Re: [hyperledger/fabric] FAB-18192 Fixed TLS certs validation for consenters. (#1888)Date: Tue, Oct 6, 2020 1:42 PM woohoo, my first PR to fabric is merged :) —You are receiving this because you are subscribed to this thread.Reply to this email directly, view it on GitHub, or unsubscribe.

* FAB-18192 Fixed TLS certs validation for consenters. Verification of TLS cert against simulated config, not the last one. To achieve that, metadata validator interface was changed, now it requires orderer config instead of just consensus metadata. Also, TLS verification was moved to VerifyMetadata function, it shouldn't have been part of ComputeMembershipChanges. Fixed tests. Signed-off-by: Vladyslav Kopaihorodskyi <vlad.kopaygorodsky@gmail.com>

* FAB-18192 Fixed TLS certs validation for consenters. (#1888) * FAB-18192 Fixed TLS certs validation for consenters. Verification of TLS cert against simulated config, not the last one. To achieve that, metadata validator interface was changed, now it requires orderer config instead of just consensus metadata. Also, TLS verification was moved to VerifyMetadata function, it shouldn't have been part of ComputeMembershipChanges. Fixed tests. Signed-off-by: Vladyslav Kopaihorodskyi <vlad.kopaygorodsky@gmail.com> * IT - update MSP and consenters set in single config update FAB-18192 Signed-off-by: Will Lahti <wtlahti@us.ibm.com> * Fix bug in consenter cert validation logic It was accidentally verifying only the clientCert and not the cert that was passed in. FAB-18269 Signed-off-by: Will Lahti <wtlahti@us.ibm.com> * Update release notes for 2.2.2 for FAB-18192 bug fix Signed-off-by: Will Lahti <wtlahti@us.ibm.com> Co-authored-by: Vladyslav Kopaihorodskyi <vlad.kopaygorodsky@gmail.com>

lucianoacsilva · 2020-10-23T13:18:55Z

Hello! Is that the new version with this latest fix already available in Docker Hub?

lindluni · 2020-10-23T14:53:41Z

There is not yet, you will need to build it yourself, or pull it from hyperledger-fabric.jfrog.io/fabric-peer:amd64-2.2-stable which is the unreleased artifact, but the current tip of the 2.2 branch. If you want the latest master you can use the amd64-latest tag instead.

lucianoacsilva · 2020-10-26T00:39:03Z

Hello, @btl5037 ,

I've pulled here the hyperledger-fabric.jfrog.io/fabric-peer:amd64-2.2-stable, as you mentioned, but the bug still persists when I try to update the config block with a new adding org:

Error: got unexpected status: BAD_REQUEST -- error applying config update to existing channel 'system-channel': consensus metadata update for channel config update is invalid: verifying tls client cert with serial number 84262169678602556738840170285785102293826956193: x509: certificate signed by unknown authority

Is this bug still persisting with you?

lindluni · 2020-10-26T00:52:52Z

@lucianoacsilva Can you pull the image again, artifactory was receiving, but not publishing the updates, it's published now.

lucianoacsilva · 2020-10-26T01:28:53Z

@btl5037 , removed the image, pulled it again when starting main network, error still persists:

Error: got unexpected status: BAD_REQUEST -- error applying config update to existing channel 'meat-trace': consensus metadata update for channel config update is invalid: verifying tls client cert with serial number 339235991777995742225504705667499867821380891148: x509: certificate signed by unknown authority

lindluni · 2020-10-26T01:31:53Z

@lucianoacsilva Can you ping me on chat.hyperledger.org, my ID is BrettLogan

kopaygorodsky · 2020-10-26T09:03:41Z

@lucianoacsilva @btl5037 I've built an image from master branch and this fix works. On the other hand, I've tried hyperledger-fabric.jfrog.io/fabric-orderer:amd64-2.2-stable and see this issue.

lucianoacsilva · 2020-10-26T13:19:04Z

I see, @kopaygorodsky . How could I build those images from the master branch?

kopaygorodsky · 2020-10-26T13:23:14Z

@lucassaldanha
try hyperledger-fabric.jfrog.io/fabric-orderer:amd64-latest
or
git clone fabric
make orderer-docker
and push docker image to your docker registry

lucianoacsilva · 2020-10-26T13:28:00Z

Wait, is that the image of the peer or the orderer? @btl5037 mentioned me the hyperledger-fabric.jfrog.io/fabric-peer:amd64-2.2-stable image, not the hyperledger-fabric.jfrog.io/fabric-orderer:amd64-latestone.

kopaygorodsky · 2020-10-26T14:13:12Z

orderer validates block, so you need to use orderer image

kopaygorodsky requested a review from a team as a code owner September 17, 2020 00:46

kopaygorodsky force-pushed the FAB-18192 branch 16 times, most recently from e0dba77 to 22d6046 Compare September 21, 2020 21:03

kopaygorodsky force-pushed the FAB-18192 branch 10 times, most recently from ad5d649 to 749bdfa Compare September 25, 2020 13:19

wlahti reviewed Sep 25, 2020

View reviewed changes

kopaygorodsky force-pushed the FAB-18192 branch 2 times, most recently from 3d79ab7 to f0325f9 Compare September 28, 2020 11:18

kopaygorodsky force-pushed the FAB-18192 branch from 7481ac3 to 5ccc63f Compare October 3, 2020 15:47

fixed consenters map

ef03b56

Signed-off-by: kopaygorodsky <vlad.kopaygorodsky@gmail.com>

jyellick reviewed Oct 5, 2020

View reviewed changes

Removed ignoreCertExpiration option from VerifyConfigMetadata, certs …

f209dda

…expiration is checked only on new consenters. Improved tests. Signed-off-by: kopaygorodsky <vlad.kopaygorodsky@gmail.com>

jyellick approved these changes Oct 6, 2020

View reviewed changes

jyellick merged commit 886d3cc into hyperledger:master Oct 6, 2020

mergify bot mentioned this pull request Oct 6, 2020

FAB-18192 Fixed TLS certs validation for consenters. (bp #1888) #1971

Closed

yacovm mentioned this pull request Feb 5, 2021

Replace certificate fixtures in etcdraft/consensus with certificates generated at runtime #2370

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

FAB-18192 Fixed TLS certs validation for consenters. #1888

FAB-18192 Fixed TLS certs validation for consenters. #1888

kopaygorodsky commented Sep 17, 2020 •

edited

Loading

wlahti left a comment

jyellick left a comment

jyellick Oct 5, 2020 •

edited

Loading

kopaygorodsky Oct 5, 2020 •

edited

Loading

jyellick Oct 6, 2020

jyellick left a comment

wlahti commented Oct 6, 2020

mergify bot commented Oct 6, 2020

jyellick commented Oct 6, 2020

mergify bot commented Oct 6, 2020

kopaygorodsky commented Oct 6, 2020

robmurgai commented Oct 7, 2020 via email

lucianoacsilva commented Oct 23, 2020

lindluni commented Oct 23, 2020

lucianoacsilva commented Oct 26, 2020

lindluni commented Oct 26, 2020 •

edited

Loading

lucianoacsilva commented Oct 26, 2020

lindluni commented Oct 26, 2020

kopaygorodsky commented Oct 26, 2020 •

edited

Loading

lucianoacsilva commented Oct 26, 2020

kopaygorodsky commented Oct 26, 2020 •

edited

Loading

lucianoacsilva commented Oct 26, 2020

kopaygorodsky commented Oct 26, 2020

FAB-18192 Fixed TLS certs validation for consenters. #1888

FAB-18192 Fixed TLS certs validation for consenters. #1888

Conversation

kopaygorodsky commented Sep 17, 2020 • edited Loading

Type of change

Description

Additional details

Related issues

wlahti left a comment

Choose a reason for hiding this comment

jyellick left a comment

Choose a reason for hiding this comment

jyellick Oct 5, 2020 • edited Loading

Choose a reason for hiding this comment

kopaygorodsky Oct 5, 2020 • edited Loading

Choose a reason for hiding this comment

jyellick Oct 6, 2020

Choose a reason for hiding this comment

jyellick left a comment

Choose a reason for hiding this comment

wlahti commented Oct 6, 2020

mergify bot commented Oct 6, 2020

jyellick commented Oct 6, 2020

mergify bot commented Oct 6, 2020

kopaygorodsky commented Oct 6, 2020

robmurgai commented Oct 7, 2020 via email

lucianoacsilva commented Oct 23, 2020

lindluni commented Oct 23, 2020

lucianoacsilva commented Oct 26, 2020

lindluni commented Oct 26, 2020 • edited Loading

lucianoacsilva commented Oct 26, 2020

lindluni commented Oct 26, 2020

kopaygorodsky commented Oct 26, 2020 • edited Loading

lucianoacsilva commented Oct 26, 2020

kopaygorodsky commented Oct 26, 2020 • edited Loading

lucianoacsilva commented Oct 26, 2020

kopaygorodsky commented Oct 26, 2020

kopaygorodsky commented Sep 17, 2020 •

edited

Loading

jyellick Oct 5, 2020 •

edited

Loading

kopaygorodsky Oct 5, 2020 •

edited

Loading

lindluni commented Oct 26, 2020 •

edited

Loading

kopaygorodsky commented Oct 26, 2020 •

edited

Loading

kopaygorodsky commented Oct 26, 2020 •

edited

Loading