search

hyperledger / fabric-gateway

Go, Node and Java client API for Hyperledger Fabric v2.4+
https://hyperledger.github.io/fabric-gateway/
Apache License 2.0
154 stars 92 forks source link

Wrong ed25519 signature due to signing identity's hash function #757

Open johannww opened 1 month ago

johannww commented 1 month ago

Following up with my pull request that implemented the support for ed25519 keys in fabric, I had trouble with signing transactions with such keys.

I believe the problem comes from ./pkg/client/transactions.go and ./pkg/client/signingidentity.go:

    digest := transaction.Digest()
    signature, err := transaction.signingID.Sign(digest)

and

func newSigningIdentity(id identity.Identity) *signingIdentity {
    return &signingIdentity{
        id: id,
        sign: func(digest []byte) ([]byte, error) {
            return nil, errors.New("no sign implementation supplied")
        },
        hash: hash.SHA256,
    }
}

In Go, ed25519 sign functions receive the message as parameter, not the digest.

PS: I realized that it is possible to pass the function hash.NONE to the gateway, and it solved my problem. However, I still think that the gateway should handle this internally, but I wait on your further analysis.

    gateway, err := gatewayClient.Connect(
    // ...
        gatewayClient.WithHash(hash.NONE),
    )

I would suggest the following changes to ./client/signingidentity.go:

func newSigningIdentity(id identity.Identity) *signingIdentity {
    hashAlg := hash.SHA256
    cert, err := identity.CertificateFromPEM([]byte(id.Credentials()))

    if err == nil && cert.PublicKeyAlgorithm == x509.Ed25519 {
        hashAlg = hash.NONE
    }

    return &signingIdentity{
        id: id,
        sign: func(digest []byte) ([]byte, error) {
            return nil, errors.New("no sign implementation supplied")
        },
        hash: hashAlg,
    }
}
bestbeforetoday commented 1 month ago

When this API was designed, it seemed to make a lot of sense for the hash and signing implementations to be decoupled:

  1. Many ECDSA signing implementations operate on a pre-computed message hash, and ECDSA was the only supported algorithm.
  2. Different hash implementations can be used for the same signing algorithm, for example SHA2 or SHA3 with ECDSA signing.
  3. PKCS11 hardware security modules used for testing typically operate on a precomputed digest.
  4. The digest provides a much smaller payload for off-line signing than the signable message bytes.

In hindsight it seems less obvious that it's the right choice, particularly as we now support Ed25519 where (in most implementations) you must pass the full message to the signing implementation. Perhaps it would have been simpler to just provide the signable message bytes and require the signing implementation (or off-line signer) to do any required hashing. In some cases it might be slightly more work for the end user, but it would avoid a lot of confusion.

Another alternative to help people avoid mistakes would be to remove the default hash implementation and require the hash to be explicitly specified, just like the signer is already.

Both of these options are breaking changes in the API. I am open to moving up a major version and implementing breaking changes, but it needs careful thought to make sure the benefits are worth the disruption.

On your proposed change, I like the idea in principle. I'm worried about the execution though. We need to maintain consistent behaviour across Go, Node and Java implementations. The Go standard library is the (only?) obvious choice for crypto so there are fewer opportunities for confusion. There are more choices when it comes to Java crypto implementation, and much more choice and variation in behaviour for Node.

I'm worried that we might not be able to reliably provide the correct defaults for all client languages. I'm also worried that having a different default hash depending on the key type is another potential source of confusion.

I did add a fair bit of explanation of the relationship between hash and signing implementation to the API documentation. I understand it's probably easy to miss though. I wonder if just having some samples that use Ed25519 (in the fabric-samples repo) for people to get started with might be enough to avoid errors. What do you think?

johannww commented 1 month ago

I think you should wait for more complaints before implementing breaking changes. Fabric v3.0.0 was just released, and people might face (or not) this type of problem. However, I have more considerations.

About choosing hash functions, according to my tests with fabric-gateway and fabric (both 2.5.9 and v3.0.0), the BCCSP hash function is determined by the "core.peer.bccsp.sw" section:

        SW:
            # TODO: The default Hash and Security level needs refactoring to be
            # fully configurable. Changing these defaults requires coordination
            # SHA2 is hardcoded in several places, not only BCCSP
            Hash: SHA2
            Security: 256

Even though there is a TODO, currently, the hash function is determined at an organization level when it joins the channel, right? Therefore, the gateway has no option to choose. It must follow the MSP.

Currently, due to fabric's go implementation, the other gateways (node and java) must use SHA512 to sign with ed25519 keys since the standard library implementation does it: (edit: afterall, it follows the specification)

func sign(signature, privateKey, message []byte) {
    if l := len(privateKey); l != PrivateKeySize {
        panic("ed25519: bad private key length: " + strconv.Itoa(l))
    }
    ...
    mh := sha512.New()
    mh.Write(prefix)
    mh.Write(message)

So, I think we are in a state where the need for changes is recognized, and the gateway is ahead of fabric, in the sense of modular hash functions. Pragmatically, there is no reason to let the user choose the hash function for ed25519 keys, but perhaps this issue is not a priority right now.