Test/Example for `sharedSecretFromKeyAgreement` with `handler`? about secp256k1.swift

[matthiasgeihs]

In fact, having looked further into this: This functionality is currently not usable as intended.
To use SHA-512 (instead of the default SHA-256), we would need the output to be of size 64 bytes.
However, currently the output is always assumed 32 bytes.

secp256k1.swift/Sources/zkp/ECDH.swift

Line 161 in 178a1a9

var sharedSecret = [UInt8](repeating: 0, count: 32)

[csjones]

Hey @matthiasgeihs 👋

Have you tried using the data argument in addition to the custom hash function? The data argument is an arbitrary data pointer that is passed through.

let privateKey = try! secp256k1.KeyAgreement.PrivateKey()
        
var dataBuffer = [UInt8](repeating: 0, count: 64)
        
_ = try! privateKey.sharedSecretFromKeyAgreement(
    with: privateKey.publicKey,
    handler: { _, x32, y32, data in
        // Copy x32 and y32 to the data buffer
        if let dataBuffer = data?.assumingMemoryBound(to: UInt8.self) {
            memcpy(dataBuffer, x32, 32)
            memcpy(dataBuffer.advanced(by: 32), y32, 32)
        }
        return 1
    },
    data: &dataBuffer
)

[matthiasgeihs]

Not sure what you are trying to do here. Your example doesn't modify the output, hence doesn't affect the shared secret.
Here is the implementation of secp256k1_ecdh for reference: https://github.com/bitcoin-core/secp256k1/blob/b10ddd2bd2bdce9ca8f2d4733636a9d9e7ac3da1/src/modules/ecdh/main_impl.h#L29C178-L29C178

The point of setting a handler is to customize the "postprocessing" of the shared secret output. The C-implementation uses SHA-256(x) as default, but does allow for a customized post-processing. However, as the Swift library assumes a fixed output length, the ways we can do post-processing is currently limited.

[csjones]

Hey @matthiasgeihs

The SharedSecret object is a thin wrapper currently and should only be a raw key ideally. If you need to customize the post-processing, it should happen on the SharedSecret itself after it has been created, not before its initialization.

let privateKey = try! secp256k1.KeyAgreement.PrivateKey()
        
let sharedSecret = try! privateKey.sharedSecretFromKeyAgreement(
    with: privateKey.publicKey,
    handler: { output, x32, _, _ in
        memcpy(output, x32, 32)
        return 1
    }
)

SHA512.hash(data: sharedSecret)

This example demonstrates there are no limitations on post-processing a SharedSecret and, again, this way would be preferable because the original DH key is preserved. The suggestion for using the data input parameter was write out an arbitrary 64 bytes from within the handler since libsecp256k1 already supports that and wouldn't require modification to support your use case.

[matthiasgeihs]

This is indeed the workaround we are currently using (no hash during secp256k1_ecdh and then hash afterwards).
However, it would be nicer if we could just replace the hash function inside secp256k1_ecdh. The C-library is designed to do that, but the Swift library disallows that. Unfortunately, you don't seem to be of the opinion that this should be possible.

To be clear, what we want is support for the following handler:

         let sha512 : secp256k1.KeyAgreement.PrivateKey.HashFunctionType = {
             (out, x, y, data) -> Int32 in
             let h = SHA512.hash(x)
             out.initialize(from: h, count: 64)
             return 1
         }

[matthiasgeihs]

Maybe for some added context:
secp256k1_ecdh by default uses a SHA-256 handler to turn the curve point into a uniformly shared secret of 32 bytes. However, for our application (compatibility with the encryption scheme described in TRON-US/go-eccrypto), we need a 64 bytes shared secret. Hence, we want to use SHA-512 instead of SHA-256. Other applications may want to use different hash functions with different output lengths as well. They may even want to change how the hash is computed. Note that your proposed workaround with computing SHA-512 after the fact works only if the computation does involve at most 32 bytes, as the output is limited to 32 bytes (this allows for copying x, but not y, for example).

[csjones]

I understand what you're getting at but I intend to keep post-processing separate from the shared secret creation. Coupling the two steps breaks the principle of single-responsibility. I appreciate you using this repo and suggesting the source code changes in a pull request. At this point though, it sounds like forking and editing the repo will give you what you want. I think it's best to close this thread. If you have any further concerns or believe there's more to address, feel free to leave a comment.

[matthiasgeihs]

My proposed change doesn't break the principle of single responsibility. The responsibility of the function is to create a shared secret and there is different ways to do that, not just SHA-256. On the contrary, the current implementation has different semantics depending on whether you want a 256-bit shared secret or not (in one case you can include the hash function in the handler, in the other case you can't.)

You can also see in the secp256k1 library, of which your library is a wrapper of, that the intention is to allow using secp256k1_ecdh with outputs of varying lengths. The following test case shows an example where the output has 65 bytes and is the raw uncompressed encoding of the shared secret point.

static int ecdh_hash_function_custom(unsigned char *output, const unsigned char *x, const unsigned char *y, void *data) {
    (void)data;
    /* Save x and y as uncompressed public key */
    output[0] = 0x04;
    memcpy(output + 1, x, 32);
    memcpy(output + 33, y, 32);
    return 1;
}

https://github.com/bitcoin-core/secp256k1/blob/ba9cb6f3789ec3ccd4012dadb878ffc4eebb6d41/src/modules/ecdh/tests_impl.h#L66

Anyhow, you don't seem to be willing to change anything about this part of your implementation and I respect that. However, your argumentation is simply not correct (data is not meant to be a buffer, there is no violation of "single responsibility principle") and the way how my request was handled leaves a bad impression on the maintenance of this library.