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/ed25519@go1.18.4#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 <johannwestphall@gmail.com>

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 <johannwestphall@gmail.com>