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

cfssl to generating certificates #1001

Merged
merged 2 commits into from
Sep 27, 2023
Merged

Conversation

adityajoshi12
Copy link
Contributor

This PR adds a new example to support pluggable CA in hyperledger fabric. Current examples in the fabric-samples provide two implementations for cert generation i.e cryptogen and fabric-ca.

This PR will help the developers to understand more about bring your own identity. These changesets will allow developers to generate certificates using the cfssl tool, which is an opensource tool by cloudflare for cert management.

Generate certs using cfssl

./network.sh up -cfssl

@adityajoshi12 adityajoshi12 requested a review from a team as a code owner March 12, 2023 09:20
@ryjones
Copy link
Member

ryjones commented Apr 12, 2023

@lehors @denyeart

@denyeart
Copy link
Contributor

Thanks for the contribution! Sorry I missed this one earlier...

It looks really good. Could you also update the network.sh help to mention the new flag?
For example see:
https://github.com/hyperledger/fabric-samples/blob/main/test-network/scripts/utils.sh#L17
and
https://github.com/hyperledger/fabric-samples/blob/main/test-network/scripts/utils.sh#L107

@adityajoshi12
Copy link
Contributor Author

Thanks for the contribution! Sorry I missed this one earlier...

It looks really good. Could you also update the network.sh help to mention the new flag? For example see: https://github.com/hyperledger/fabric-samples/blob/main/test-network/scripts/utils.sh#L17 and https://github.com/hyperledger/fabric-samples/blob/main/test-network/scripts/utils.sh#L107

Thanks @denyeart , I have updated the help message

Comment on lines +16 to +21
"hosts": [
"{USER}",
"localhost",
"127.0.0.1",
"0.0.0.0"
]
Copy link
Contributor

Choose a reason for hiding this comment

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

I just realized that you generate a single certificate and use it for both the MSP enrollment cert (e.g. role 'admin' above) and the TLS cert (hosts defined for tls cert SAN here).
While this will technically work, it is not a good practice, production environments should have separate certs for msp and tls. And while the samples here are not expected to be used in production environments, the samples are intended to be an educational resource and there is a lot of confusion about proper use of certificates.
I'd suggest to split the certificates to make their usage more clear - have one msp enrollment cert with the OU role defined, and a seprate tls cert with the hosts SAN entries.
What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Certs will be different as we are generating MSP and TLS certs with different profiles, we have two profiles (sign and tls) that governs if the certificates are for signing or TLS purpose.

@ryjones
Copy link
Member

ryjones commented Aug 17, 2023

@denyeart thoughts?

Copy link
Contributor

@denyeart denyeart left a comment

Choose a reason for hiding this comment

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

Ok, sorry for the delay... I've finally confirmed the certificate generation works when bringing up test-network. And I confirmed that the msp and tls certificates are indeed different.

We may want to remove the SANS "hosts" for the msp certificates, they are unnecessary and may confuse people, but if that is difficult we could skip it.

Overall this is a really great enhancement, many people have had the question of how to bring your own CA and this sample extension will really help.

This is probably good enough to merge as-is, but could you double check the names of all the files relative to the prior generation methods. I found at least one file that is named different that causes the test network tutorial to not work, there may be some others.

Also I found one other typo.

cp "$CERT_DIR/ca/ca.pem" "$CERT_DIR/tlsca/tlsca.example.com-cert.pem"

cp "$CERT_DIR/ca/ca.pem" "$CERT_DIR/msp/cacerts/"
cp "$CERT_DIR/ca/ca.pem" "$CERT_DIR/msp/tlscacerts/"
Copy link
Contributor

Choose a reason for hiding this comment

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

This needs to be called tlsca.example.com-cert.pem for the test-network instructions to work, e.g.

https://hyperledger-fabric.readthedocs.io/en/latest/test_network.html

peer chaincode invoke -o localhost:7050 --ordererTLSHostnameOverride orderer.example.com --tls --cafile "${PWD}/organizations/ordererOrganizations/example.com/orderers/orderer.example.com/msp/tlscacerts/tlsca.example.com-cert.pem" -C mychannel -n basic --peerAddresses localhost:7051 --tlsRootCertFiles "${PWD}/organizations/peerOrganizations/org1.example.com/peers/peer0.org1.example.com/tls/ca.crt" --peerAddresses localhost:9051 --tlsRootCertFiles "${PWD}/organizations/peerOrganizations/org2.example.com/peers/peer0.org2.example.com/tls/ca.crt" -c '{"function":"InitLedger","Args":[]}'

cfssl gencert \
-ca="$CERT_DIR/ca/ca.pem" \
-ca-key="$CERT_DIR/ca/ca-key.pem" \
-config="$PWD/organizations/cfssl/cert-signing-config.jso"n \
Copy link
Contributor

Choose a reason for hiding this comment

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

Somehow the generation worked even with this typo, but I assume it needs to be fixed.

@ryjones
Copy link
Member

ryjones commented Sep 19, 2023

@adityajoshi12 could you address these remaining issues?

@adityajoshi12
Copy link
Contributor Author

@adityajoshi12 could you address these remaining issues?

Sure, I will update my PR

@denyeart
Copy link
Contributor

Also, the PR will need to be rebased against latest main.

Signed-off-by: Aditya Joshi <[email protected]>
@adityajoshi12
Copy link
Contributor Author

Also, the PR will need to be rebased against latest main.

@denyeart update the PR

@ryjones
Copy link
Member

ryjones commented Sep 23, 2023

@denyeart it is rebased against the head of main

Copy link
Contributor

@denyeart denyeart left a comment

Choose a reason for hiding this comment

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

Thank you, it works well now!

@denyeart denyeart merged commit 9441772 into hyperledger:main Sep 27, 2023
37 of 40 checks passed
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.

None yet

3 participants