Add ed25519 support #3343

johannww · 2022-04-20T18:08:04Z

Type of change

New feature

Description

Due to some considering NIST curves insecure and
Golang having ed25519 native support, there was
not a reason for not implementing in Fabric.

Tests cases for ed25519 were also added. Since
ed25519 key derivation is not called by any
function, I left as a TODO.

As I am working on ed25519 support for node fabric-gateway,
I needed to add ed25519 support for cryptogen also,
aiming to pass tests with certificates containing
ed25519 keys. Since the node fabric-gateway tests
generate their crypto material with cryptogen, I
adapted cryptogen to support ed25519 keys.

Additional details

Some tests concerning ed25519 key generation and certificate parsing
were added.

Related issues

yacovm · 2022-04-20T18:17:54Z

I generally support this idea, and am in favor of it, but unfortunately adding support for ed25519 in a safe manner is more complex than just adding the code for generating signatures and verifying them.

If you could atomically and instantly upgrade all nodes of a Fabric network to support ed25519 then there wouldn't have been any problem at all.

But Fabric nodes may be upgraded at different times, and some nodes might be offline or unreachable and then afterwards be reachable and online again and these nodes will receive the ed25519 signatures and they will reject them.
On the other hand, the nodes that were upgraded, will not reject them, and you will have a fork in the blockchain.

The key problem is that we need a way for Fabric validation to understand that "until a certain point in the blockchain, all ed25519 signatures are invalid" and "starting from a certain point in the blockchain, all ed25519 signatures are valid".

In Fabric, this is done using the capabilities feature. Essentially, you "mark" in the channel config a certain configuration property and then the nodes know when to use that property and when not to use it.

yacovm · 2022-04-20T18:22:42Z

Aside from my comment above, the PR is missing integration tests that generate ed25519 crypto material, and run a network and transact to ensure everything is good.

yacovm · 2022-04-20T23:28:27Z

internal/cryptogen/msp/msp_test.go

@@ -110,10 +112,10 @@ func testGenerateLocalMSP(t *testing.T, nodeOUs bool) {
 	}

 	tlsCA.Name = "test/fail"
-	err = msp.GenerateLocalMSP(testDir, testName, nil, signCA, tlsCA, msp.CLIENT, nodeOUs)
+	err = msp.GenerateLocalMSP(testDir, testName, nil, signCA, tlsCA, msp.CLIENT, nodeOUs, ECDSA)


I see that you didn't changed the MSP (top level msp package).

The MSP needs to be versioned like here from the aforementioned reasons.

Thanks for all your comments! I intend to add more work to it during the following weeks.

ale-linux · 2022-04-26T08:04:00Z

Incidentally, have you checked that the verification takes care of

https://datatracker.ietf.org/doc/html/rfc8032#section-8.4

We had to do this manually for ecdsa

johannww · 2022-05-06T22:12:32Z

Incidentally, have you checked that the verification takes care of

https://datatracker.ietf.org/doc/html/rfc8032#section-8.4

We had to do this manually for ecdsa

It seems that golang has already implemented it:

https://github.com/golang/go/blob/4e79f06dac712d35d67d72777dae6df6d8c97888/src/crypto/ed25519/ed25519.go#L220

https://github.com/golang/go/blob/4e79f06dac712d35d67d72777dae6df6d8c97888/src/crypto/ed25519/ed25519_test.go#L165

In previous versions they even had a comment about preventing signature malleability:

https://github.com/golang/go/blob/go1.15/src/crypto/ed25519/ed25519.go#L235

bestbeforetoday · 2022-05-27T14:36:50Z

@johannww On ed25519 support for node fabric-gateway... I'm sure you already know the signing implementation is entirely pluggable. The client application can supply its own (ed25519) signing implementation and the API doesn't need to have an implementation built in. However, I would be quite happy to include this in the API. Just note that there need to be implementations in all three client languages (Go, Type/JavaScript and Java).

msp/factory.go

yacovm · 2022-07-19T11:00:28Z

msp/mspimplsetup.go

@@ -656,6 +659,22 @@ func (msp *bccspmsp) setupV142(conf *m.FabricMSPConfig) error {
 	return nil
 }

+func (msp *bccspmsp) setupV24(conf *m.FabricMSPConfig) error {


Lets put setupV3

yacovm · 2022-07-19T11:02:21Z

integration/nwo/standard_networks.go

@@ -181,6 +181,30 @@ func MultiChannelBasicSolo() *Config {
 	return config
 }

+func Ed25519Solo() *Config {


why do you need a config for ED25519 ? There is nothing unique about this config that is related to ed25519. In the integration test you can just take an existing config, no?

I probably don't. I only created there as a temporary solution. Peers can be added externally to this configuration function anyway.

yacovm · 2022-07-19T11:03:08Z

integration/nwo/network.go

@@ -1439,6 +1461,79 @@ func (n *Network) peerCommand(command Command, tlsDir string, env ...string) *ex
 	return cmd
 }

+func (n *Network) downloadOldBinaries(fabricVersion string) error {


why do you need this downloadOldBinaries ?

yacovm · 2022-07-19T11:03:25Z

integration/nwo/network.go

+	return nil
+}
+
+func (n *Network) oldPeerCommand(command Command, fabricVersion string, tlsDir string, env ...string) *exec.Cmd {


why do you need to have this oldPeerCommand command?

In the integration test integration/lifecycle/capabilities_endorse_test.go I upgrade the channel capabilities and intend to show that a peer that does not have source code to support it won't endorse any more transactions. Therefore, one of the peers runs with a fabric v2.4.5 binary, downloaded from Github.

did you mean to say "does not" instead of "does" ? I still don't understand, why would the peer not endorse transactions? If you enable a capability and a peer cannot support it, it simply crashes.

I assumed that the peer crash should be proven. If you consider this verification unnecessary I can remove it.

yes please remove

yacovm · 2022-07-19T11:05:46Z

integration/ed25519/config_test.go

@@ -0,0 +1,1859 @@
+/*
+Copyright IBM Corp All Rights Reserved.


I think a single integration test that:

Has peers and orderers in it

Transacts on a channel

The channel contains both ECDSA and ed25519 identities and they're both used in the endorsement policy

Is sufficient.

That is my intention. This commit was not meant as a final solution yet but a work in progress. The folder integration/ed25519 will be removed.

By "peers and odererS" do you mean I should use a raft configuration with at least two orderers?

Knowing the Raft code, you don't have to use several Raft nodes to test ed25519. One is sufficient. But you can use three, I think it's easier from a copy-paste aspect that to use one.

yacovm · 2022-07-19T11:06:56Z

integration/ed25519/client.go

+	return blk
+}
+
+func CreateBroadcastEnvelope(n *nwo.Network, entity interface{}, channel string, data []byte) *common.Envelope {


we have these functions already in the code, I don't think we need to add them again in another place. Let's just use the functions that exist

yacovm · 2022-07-19T20:13:07Z

Ok i understand this is still work in progress. So please tag me once you feel this is ready.
Take into account that it will take quite a while for this to be merged, and also even more time until this will be included into any release, because if it will go to any release it will be to Fabric v3.0

bestbeforetoday · 2022-07-26T11:18:07Z

cmd/common/signer/signer.go

+	case *ecdsa.PrivateKey:
+		return signECDSA(si.key.(*ecdsa.PrivateKey), digest)
+	case ed25519.PrivateKey:
+		return ed25519.Sign(si.key.(ed25519.PrivateKey), digest), nil


Is this correct? The godoc suggests that, while ecdsa sign takes a hash/digest, the ed25519 sign requires the entire message bytes. There is more detail in the PrivateKey Sign method doc:

https://pkg.go.dev/crypto/[email protected]#PrivateKey.Sign

based on the tests of the ed25519 package, you need to pass a message and not a hash.

And this means that the verifier needs to be changed accordingly.

I am evaluating a possible solution, as I believe these requirements will lead to changes in the Signer interface

https://github.com/johannww/fabric-1/blob/9fdc4f0c3ac92a7c95ab0099ac6535a1fc4410f0/bccsp/signer/signer.go#L76-L78

So functions like the one below can be called:

https://github.com/johannww/fabric-1/blob/9fdc4f0c3ac92a7c95ab0099ac6535a1fc4410f0/msp/identities.go#L255-L278

I think it is fixed. Plus, I forced-pushed some commits because they were not signed off.

bestbeforetoday · 2022-11-25T12:00:35Z

gossip/discovery/discovery_impl.go

+	for i := range deadMembers2Expire {
+		d.logger.Warning("Closing connection to", deadMembers2Expire[i])
+		d.comm.CloseConn(&deadMembers2Expire[i])


I don't see the benefit of changing the code to look up the value twice by index instead of just directly using the value provided by for-range, as the code did before

I don't see how this is related to the pull request's intention.

Do not consider that. This is the result of a forced push that I have done to sign my commits. I accidentally signed commits that were not mine. I will fix that and probably force-push again.

yacovm · 2022-11-25T13:20:30Z

integration/lifecycle/capabilities_endorse_test.go

@@ -0,0 +1,261 @@
+/*


Is there any specific handling for ed25519 that influences lifecycle specifically?

No. I just put it in the lifecycle folder because it seemed the most related integration test "family", considering your previous #suggestions:

I think a single integration test that:

Has peers and orderers in it

Transacts on a channel

The channel contains both ECDSA and ed25519 identities and they're both used in the endorsement policy

Is sufficient.

Warning1: This branch must be run with the fabric pull-request branch on hyperledger/fabric#3343 Warning2: The java gateway does not support ed25519 TLS certificates. This will lead to either warning users that java-gateway cannot communicate to ed25519 peers/orderers with TLS certificates OR the pull request on hyperledger/fabric#3343 fabric should disable ed25519 TLS certificates. Signed-off-by: Johann Westphall <[email protected]>

johannww · 2022-11-26T02:56:52Z

One important issue:

I am implementing Ed25519 support for fabric-gateway using the code in this pull request as my Hyperledger Fabric. I created the crypto-material by setting field PublicKeyAlgorithm to ed25519, instead of ecdsa as here:

https://github.com/johannww/fabric-gateway/blob/81f5ee44f37f7148500f1039e5b33bd4f17d8f4d/scenario/fixtures/crypto-material/crypto-config.yaml#L41-L45

This generated ed25519 TLS keys for the peers. When running the scenario-test, only the java-gateway failed in a scenario with ed25519 TLS keys, due to the library netty (see the issue). I see 2 possible ways to go from here, regarding changes to fabric and/or gateway:

Disable ed25519 certificates for TLS explicitly in fabric and prevent cryptogen from generating ed25519 TLS certificates.
Ignore Java limitation and issue a warning when errors related to the grpc and netty happen. Count on netty developers to implement ed25519 support soon.
Any other suggestions?

bestbeforetoday · 2022-11-26T10:54:41Z

When running the scenario-test, only the java-gateway failed in a scenario with ed25519 TLS keys, due to the library netty (see the issue).

I noticed this JDK issue, which says TLS support for EdDSA signatures was added in Java 16:

https://bugs.openjdk.org/browse/JDK-8254709

Any chance that it worked with Java 17?

I see 2 possible ways to go from here, regarding changes to fabric and/or gateway:

Disable ed25519 certificates for TLS explicitly in fabric and prevent cryptogen from generating ed25519 TLS certificates.

Ignore Java limitation and issue a warning when errors related to the grpc and netty happen. Count on netty developers to implement ed25519 support soon.

Any other suggestions?

I think I would be happy to proceed with only Java clients not currently able to establish TLS connections to peers that use Ed25519 keys. There is precedent for certain crypto only being supported by certain client, such as Idemix. This is clearly a bug outside of our control and should get fixed at some point.

yacovm · 2022-11-28T12:33:44Z

One important issue:

I am implementing Ed25519 support for fabric-gateway using the code in this pull request as my Hyperledger Fabric. I created the crypto-material by setting field PublicKeyAlgorithm to ed25519, instead of ecdsa as here:

https://github.com/johannww/fabric-gateway/blob/81f5ee44f37f7148500f1039e5b33bd4f17d8f4d/scenario/fixtures/crypto-material/crypto-config.yaml#L41-L45

This generated ed25519 TLS keys for the peers. When running the scenario-test, only the java-gateway failed in a scenario with ed25519 TLS keys, due to the library netty (see the issue). I see 2 possible ways to go from here, regarding changes to fabric and/or gateway:
1. Disable ed25519 certificates for TLS explicitly in fabric and prevent cryptogen from generating ed25519 TLS certificates.

2. Ignore Java limitation and issue a warning when errors related to the grpc and netty happen. Count on netty developers to implement ed25519 support soon.

3. Any other suggestions?

Fabric's TLS stack is generally up to the Go standard library, so if you use a Java client willingly, it is up to you to be able to connect correctly to the TLS stack of the nodes.

Generally, as a rule of thumb - Fabric core sets the standard, and the clients track the Fabric core, and not the other way around.

Considering the efforts to give ed25519 support in fabric and fabric-gateway, this commit intends to provide ed25519 support for fabric-ca. With these efforts, cryptogen can generate ed25519 keys for the CA, motivating these changes to keep compatibility in terms of crypto support. Fabric main branch current version (3.0.0) does not support RSA anymore. For that reason, I had to manually merge the ed25519 pull request (hyperledger/fabric#3343) with fabric v1.4.11. The adapated version replaces fabric v1.4.11 in 'go.mod'. Signed-off-by: Johann Westphall <[email protected]>

Signed-off-by: Johann Westphall <[email protected]>

- instead of changing cryptogen config, we give ed25519 keys by changing the certificate and the keys. - Now, the test is compatible with a network without a system channel. - Other improvements were made Signed-off-by: Johann Westphall <[email protected]>

Signed-off-by: Johann Westphall <[email protected]>

bestbeforetoday · 2024-05-30T13:56:57Z

@yacovm @denyeart @adecaro @ale-linux this PR is now building successfully against the BCCSP updates delivered in hyperledger/fabric-lib-go#29. Can you re-review?

yacovm · 2024-06-06T19:57:16Z

integration/nwo/cryptogen_config.go

+	Users         UsersSpec    `yaml:"Users"`
+}
+
+type CryptogenConfig struct {


where is this used?

On previous tests, I used cryptogen to give ed25519 credentials. It has no use now. It will be removed.

yacovm · 2024-06-06T20:07:54Z

integration/nwo/deploy.go

+		Value: protoutil.MarshalOrPanic(
+			&common.Capabilities{
+				Capabilities: map[string]*common.Capability{
+					capabilitiesVersion: {},


I'm confused, where do you enable the V3_0 capability here?

I followed the example of ./integration/e2e/e2e_test.go#L197-L198, which is a pattern repeated in many integration tests. V3_0 capabilities are enabled by line 518.

oh I see, the capabilitiesVersion you pass is set to V3_0

yacovm · 2024-06-06T20:09:01Z

integration/lifecycle/capabilities_endorse_test.go

+	})
+
+	It("invoke chaincode after upgrading Channel to V3_0 and add ed25519 peer and orderer", func() {
+		networkConfig := nwo.MultiNodeEtcdRaft()


So we have a single organization with 2 orderers of ECDSA and 1 orderer that is offline, and later is brought online to be ed25519 ?

Almost that. Initially, we have 3 ECDSA orderers from a single organization. Later, orderer3 is killed, given an ed25519 key, and restarted. In contrast, there are 2 ECDSA peers from different orgs and an ed25519 offline one. The ed25519 peer is only started once, directly with ed25519 keys.

yacovm · 2024-06-06T20:13:11Z

msp/identities_test.go

+// sign the DIGEST
+// The same applies to the Verify function
+func TestSignatureAlgorithms(t *testing.T) {
+	gt := gomega.NewGomegaWithT(t)


not that it matters, but, any reason you use gomega here and not "github.com/stretchr/testify/require" ?

The entire package is using the latter

I did not notice and was probably inspired by a gomega test file. I can change that to keep coherence.

denyeart · 2024-06-06T22:02:17Z

@johannww I just wanted to say thank you for sticking with this pull request. I acknowledge that it has taken a very long time to fully review it, but this is indeed on our list for the upcoming v3.0 release.

Do you have any interest in adding ed25519 support to fabric-ca for certificate issuance?

Signed-off-by: Johann Westphall <[email protected]>

johannww · 2024-06-07T04:00:04Z

@denyeart You're welcome. I remember. I tried to add ed25519 to the fabric-ca previously but stumbled into a problem with cloudfare cfssl support for ed25519: cloudflare/cfssl#1097 (comment).

Here is my old fork of fabric-ca: https://github.com/johannww/fabric-ca

This seems to be solved and merged. I should take a look into it.

yacovm · 2024-06-07T06:48:06Z

ok it looks good to me, let's have @ale-linux or @adecaro also review it.

Considering the efforts to give ed25519 support in fabric and fabric-gateway, this commit intends to provide ed25519 support for fabric-ca. With these efforts, cryptogen can generate ed25519 keys for the CA, motivating these changes to keep compatibility in terms of crypto support. Fabric main branch current version (3.0.0) does not support RSA anymore. For that reason, I had to manually merge the ed25519 pull request (hyperledger/fabric#3343) with fabric v1.4.11. The adapated version replaces fabric v1.4.11 in 'go.mod'. Signed-off-by: Johann Westphall <[email protected]>

bestbeforetoday · 2024-06-27T08:42:45Z

ok it looks good to me, let's have @ale-linux or @adecaro also review it.

How long do we wait for a review before merging?

denyeart · 2024-06-27T10:49:06Z

ok it looks good to me, let's have @ale-linux or @adecaro also review it.

How long do we wait for a review before merging?

I talked to @adecaro the other day and he confirmed he will take a look so that we can get this in for v3.0... I do think it is valuable to have one of the original MSP authors take a look.

adecaro · 2024-06-27T11:40:08Z

LGTM 👍

denyeart

@johannww Thanks again for the contribution and apologies for the delays, this is now ready for merge for inclusion in Fabric v3.0!

johannww · 2024-06-28T03:52:02Z

@denyeart you're welcome! Thank you also for reviewing big pull request.

@bestbeforetoday thank you for ensuring that this pull request remained active throughout this period.

@yacovm thank you for all the reviews and hints given.

johannww requested review from a team as code owners April 20, 2022 18:08

yacovm reviewed Apr 20, 2022

View reviewed changes

bestbeforetoday mentioned this pull request Jul 15, 2022

Ed25519 signer implementation hyperledger/fabric-gateway#468

Closed

3 tasks

johannww force-pushed the ed25519-support branch 2 times, most recently from f0ec525 to 84c4ea8 Compare July 15, 2022 20:53

yacovm reviewed Jul 19, 2022

View reviewed changes

msp/factory.go Outdated Show resolved Hide resolved

yacovm reviewed Jul 19, 2022

View reviewed changes

bestbeforetoday reviewed Jul 26, 2022

View reviewed changes

johannww force-pushed the ed25519-support branch from 7f2ffdf to 889907a Compare August 30, 2022 15:51

bestbeforetoday reviewed Nov 25, 2022

View reviewed changes

yacovm reviewed Nov 25, 2022

View reviewed changes

johannww mentioned this pull request Nov 25, 2022

TLS with EdDSA private keys not supported with OpenSSL netty/netty#10916

Open

johannww force-pushed the ed25519-support branch from a007b56 to 272fa3e Compare November 25, 2022 19:16

johannww added 9 commits May 28, 2024 16:11

fix key type error message

1bb5565

Signed-off-by: Johann Westphall <[email protected]>

remove an unnecessary test

2714ba4

Signed-off-by: Johann Westphall <[email protected]>

replace ioutil with os

4d9a50a

Signed-off-by: Johann Westphall <[email protected]>

refactor

433bfbb

Signed-off-by: Johann Westphall <[email protected]>

remove unecessary line from ed25519 test

9df7bf1

Signed-off-by: Johann Westphall <[email protected]>

fix nwo.UpdateConfig() call from discovery test

44c37b8

Signed-off-by: Johann Westphall <[email protected]>

use gomega funcs instead of println

66524a7

Signed-off-by: Johann Westphall <[email protected]>

bump fabric-lib-go to v1.1.3

2b19c89

Signed-off-by: Johann Westphall <[email protected]>

johannww force-pushed the ed25519-support branch from 30c4aae to 2b19c89 Compare May 28, 2024 22:41

yacovm reviewed Jun 6, 2024

View reviewed changes

johannww added 2 commits June 6, 2024 20:09

remove uneeded file

c3019ef

Signed-off-by: Johann Westphall <[email protected]>

fix integration test chaincode deploy

b4f60ac

Signed-off-by: Johann Westphall <[email protected]>

bestbeforetoday mentioned this pull request Jun 18, 2024

Support Chinese GM algorithm #3251

Open

denyeart approved these changes Jun 27, 2024

View reviewed changes

denyeart merged commit c27d12a into hyperledger:main Jun 27, 2024
14 checks passed

johannww mentioned this pull request Oct 2, 2024

Wrong ed25519 signature due to signing identity's hash function hyperledger/fabric-gateway#757

Open

Add ed25519 support #3343

Add ed25519 support #3343

Conversation

johannww commented Apr 20, 2022

Type of change

Description

Additional details

Related issues

yacovm commented Apr 20, 2022

yacovm commented Apr 20, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ale-linux commented Apr 26, 2022

johannww commented May 6, 2022 • edited Loading

bestbeforetoday commented May 27, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johannww Jul 19, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yacovm Jul 19, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yacovm commented Jul 19, 2022

bestbeforetoday Jul 26, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johannww commented Nov 26, 2022 • edited Loading

bestbeforetoday commented Nov 26, 2022

yacovm commented Nov 28, 2022

bestbeforetoday commented May 30, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yacovm Jun 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

denyeart commented Jun 6, 2024

johannww commented Jun 7, 2024 • edited Loading

yacovm commented Jun 7, 2024

bestbeforetoday commented Jun 27, 2024

denyeart commented Jun 27, 2024

adecaro commented Jun 27, 2024

denyeart left a comment

Choose a reason for hiding this comment

johannww commented Jun 28, 2024

johannww commented May 6, 2022 •

edited

Loading

johannww Jul 19, 2022 •

edited

Loading

yacovm Jul 19, 2022 •

edited

Loading

bestbeforetoday Jul 26, 2022 •

edited

Loading

johannww commented Nov 26, 2022 •

edited

Loading

yacovm Jun 6, 2024 •

edited

Loading

johannww commented Jun 7, 2024 •

edited

Loading