Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 2 commits into from
Feb 5, 2021

Conversation

sykesm
Copy link
Contributor

@sykesm sykesm commented Feb 4, 2021

These changes update tlsgen to set an SKI on generated certificates and then remove the certificate fixtures in etcdraft/consensus in favor of certificates generated at runtime.

Related to FAB-18257

@sykesm sykesm requested a review from jyellick February 4, 2021 16:48
@sykesm sykesm changed the title Ca ski Replace certificate fixtures in etcdraft/consensus with certificates generated at runtime Feb 4, 2021
@sykesm sykesm marked this pull request as ready for review February 4, 2021 17:40
@sykesm sykesm requested a review from a team as a code owner February 4, 2021 17:40
Signed-off-by: Matthew Sykes <sykesmat@us.ibm.com>
Copy link
Contributor

@jyellick jyellick left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally looks good to me -- I see no real problems with merging as is, but a few comments.

common/crypto/tlsgen/key.go Show resolved Hide resolved
common/crypto/tlsgen/ca.go Show resolved Hide resolved
orderer/consensus/etcdraft/util_test.go Outdated Show resolved Hide resolved
Signed-off-by: Matthew Sykes <sykesmat@us.ibm.com>
@yacovm
Copy link
Contributor

yacovm commented Feb 5, 2021

I don't understand why we need a valid SKI in TLS certificates.
The JIRA says:

PR #1888 introduced a number of new test fixtures because the tlsgen package was not producing certificates with a valid SKI.

But it doesn't point out what was the problem with having an invalid SKI.

Recall, this tlsgen package is used in production - the peer generates TLS certificates (client and server) for itself and the chaincode, hence if we needed SKI for TLS to work, we would have implemented that in tlsgen in the first place.

I guess this has something to do with generation of certs for orderers, but... I don't recall any check we have for SKI in the Fabric specific communication infrastructure.

@jyellick
Copy link
Contributor

jyellick commented Feb 5, 2021

@yacovm The MSPs parsing channel configuration require that the TLS CAs have an SKI set. So, if you used the tlsgen stuff without SKI, the tests fail because the MSP errors out ingesting the config.

@jyellick jyellick merged commit 964e47a into hyperledger:master Feb 5, 2021
@sykesm sykesm deleted the ca-ski branch February 20, 2021 02:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants