add posibility to create SharedSecret with applied custom hash function

[shmishleniy]

#161 alternative

[elichai]

Can you please share the use case on using a different hash function here?

[shmishleniy]

Hi,

https://github.com/flyingw/umbrella/blob/4840a661afac0ec461eccf0d2fc04928f51198ec/src/ecies.rs#L12-L17

1. create unencrypted shared as z.
2. apply kdf
3. apply sha256

First step is broken now. We will use ffi::secp256k1_ecdh(..) directly to create unencrypted shared.

[shmishleniy]

here is current implementation

ecdh::SharedSecret::new_with_hash
https://github.com/flyingw/umbrella/blob/344c5e7b3d0560e744e3ad30bbe9426044970a19/src/ecies.rs#L27

fn hash
https://github.com/flyingw/umbrella/blob/344c5e7b3d0560e744e3ad30bbe9426044970a19/src/ecies.rs#L13

[apoelstra]

concept ACK. There were a few usecases listed when we added this support upstream (and we took forever on that ... let's try to be faster in rust-secp :)).

I'm very nervous about this business of casting a rust closure to a c_void pointer. I'm surprised this works at all actually. Can you cite something that says we're allowed to do this?

[dvdplm]

We are trying to get decommission our fork of secp256k1 and one of the hurdles to do that is this exact issue: we need the bare shared secret. To work around this we currently have this, but that makes switching libraries problematic.

The "raw" variant is used in parity-crypto and then used in many places in parity-ethereum, e.g. for the devp2p handshake.
Having a SharedSecret::new_with_hash() like suggested here would work for us, as would SharedSecret::new_raw() or something similar.

[elichai]

I have not reviewed this thoroughly but I'm pretty sure closures can't be safely casted to fn pointers.

You need an fn() type for that.

https://rust-lang.github.io/unsafe-code-guidelines/layout/function-pointers.html

Fn traits don't promise to be function pointers. They are just types that impl the trait.

[elichai]

I just realized I misunderstood the code.

the FnMut trait is used as data. so C code never dereference it.
If we can agree that this is equivalent to the following code: [1]

fn hey<F: FnMut()>(func: &mut F){
    let ptr: *mut F = func;
    let void_ptr = ptr as *mut c_void;
    let mut_ref = unsafe { &mut *(void_ptr as *mut F)};
    mut_ref();
}

Then I think that code is completely valid (though I'm not sure if I like it yet) for the following reasons:

1. The layouts of &T, &mut T, *const T and *mut T are the same. [2]
2. We're allowed to cast any pointer to *const c_void and back. which I actually can't find any documentation that says this is true.

[1] https://play.rust-lang.org/?gist=cac9fbb4766cfda4be5f9bbefc0a0878
[2] https://rust-lang.github.io/unsafe-code-guidelines/layout/pointers.html#notes

[dvdplm]

2\. We're allowed to cast any pointer to `*const c_void` and back. which I actually can't find any documentation that says this is true.

Maybe this SO post is relevant to this? Seems to suggest that as long as C doesn't do anything funny with the pointer it should be fine. Sure would be nice to see more "official" documentation asserting that though.

[real-or-random]

Given that this is a Rust library, wouldn't it be much cleaner to expect a Rust function instead of a C function? Then we simply need a C function that passes us x and y, and then we apply a user-provided Rust function to x and y. The Rust function can take x and y as bytestrings.

[elichai]

I would consider actually using i16 here. and casting it to c_int in the function. that way it should be compatible with any rust supporting architecture while being lossless.

[elichai]

Please minimize the code inside the unsafe block to the minimum possible needed (i.e. the ffi call only)

[shmishleniy]

I have not reviewed this thoroughly but I'm pretty sure closures can't be safely casted to fn pointers.

You need an fn() type for that.

https://rust-lang.github.io/unsafe-code-guidelines/layout/function-pointers.html

Fn traits don't promise to be function pointers. They are just types that impl the trait.

Hi,

in this code we just cast FnMut pointer to * c_void and pass it as data argument.
then hash_callback works as a call-back function that will receive our FnMut as * c_void data argument, then cast it back to FnMut and invoke. so basically it absolutely safe because nothing is happened with our FnMut.

[apoelstra]

Since a pointer to FnMut is a trait object it is two words. A *c_void is one word. I am surprised this cast even compiles.

[elichai]

@apoelstra Generics aren't trait objects.
A trait object is only if you do something like ptr: &FnMut(u8) but if you have <F: FnMut(u8)>(ptr: &F...) then it's a generic over the trait. There's no dynamic dyspatch involved. (not a trait object)

The monomorphisim will just replace that F with some type that implements the FnMut(probably a function or another auto generated type, unless in nightly where you can basically implement it yourself on whatever)

[real-or-random]

Given that this is a Rust library, wouldn't it be much cleaner to expect a Rust function instead of a C function? Then we simply need a C function that passes us x and y, and then we apply a user-provided Rust function to x and y. The Rust function can take x and y as bytestrings.

Do people have opinions on this?

[dvdplm]

@real-or-random maybe it’s would lead to a slightly awkward API unless we change the C interface to stop taking a callback? With the current API, wouldn’t we have to pass in a dummy callback and then apply the user-provided rust function to the result of that? It’d work, but the code would look a bit odd perhaps?
Or maybe I misunderstand your suggestion?

[real-or-random]

I think it leads to a less awkward API: Our user does not expect to provide a function hash: (&mut [u8], &[u8], &[u8]) -> i32 because this not not all idiomatic in Rust, in particular the -> i32 part.

I don't care that we internally provide a dummy callback. It may look odd but we have lots of odd code in this library. In fact, I think calling a dummy function would looks much less odd than the current suggestion in PR, and the long discussion about whether the current code is legal supports my point.

[elichai]

I think it leads to a less awkward API: Our user does not expect to provide a function hash: (&mut [u8], &[u8], &[u8]) -> i32 because this not not all idiomatic in Rust, in particular the -> i32 part.

I don't care that we internally provide a dummy callback. It may look odd but we have lots of odd code in this library. In fact, I think calling a dummy function would looks much less odd than the current suggestion in PR, and the long discussion about whether the current code is legal supports my point.

You're right. idk how I missed that glaring thing.
at the very least it should be FnMut([u8; 32], [u8;32]) -> [u8;32]
and even yet it's awkward. because upstream allows variable size hashes (to support protocols with shaX-512) but we invented this whole SharedSecret struct that constrains it to 32 bytes.

A note, our current API doesn't allow for failure. and honestly? hash functions should never fail. (well unless you pass the max limit allowed by some hash functions, even then if that limit is 64 bytes then it sucks)

[elichai]

Too bad there are no const generics yet.
otherwise we could've made this function return [u8; T]

[real-or-random]

at the very least it should be FnMut([u8; 32], [u8;32]) -> [u8;32]
and even yet it's awkward. because upstream allows variable size hashes (to support protocols with shaX-512) but we invented this whole SharedSecret struct that constrains it to 32 bytes.

Yeah, hm. If we want to support variable-length hash functions, then hash: (&mut [u8], [u8:32], [u8:32]) is fine I guess. But you're right, that's not compatible with SharedSecret... What's the current reason for SharedSecret?

A note, our current API doesn't allow for failure. and honestly? hash functions should never fail. (well unless you pass the max limit allowed by some hash functions, even then if that limit is 64 bytes then it sucks)

Yes, a hash function should not fail. Maybe the more interesting question is if the user considers certain coordinate pairs invalid. But I don't think that's reasonable here. We promise to give a valid non-zero point to the callback (and there are no "weak" points / small subgroups whatever), so I agree: failures should not be allowed.

[elichai]

@apoelstra might be able to respond to the current design choices for SharedSecret.
but as it is, it's really hard in rust right now to return variable length arrays, without heap allocation or some funky magic tricks(i.e. ArrayVec).
or at the very least setting an upper limit and returning the right length (similar to what we did with the DER signature https://docs.rs/secp256k1/0.16.0/secp256k1/struct.SerializedSignature.html)

I see 3 reasonable options:

1. Stick with 32 byte hashes.

2. We can do something like SerializedSignature and set a reasonable future proof max length (256 byte?) with a hash function that needs to return the right length. (or return T: AsRef<[u8]> and then it can return any type that will give us the data+length)

3. We wait for const generics. which we probably won't see in stable for a ~year Tracking issue for const generics (RFC 2000) rust-lang/rust#44580

[apoelstra]

Can't we just genericize over SharedSecret? And provide a convenience function to get the existing behaviour.

[elichai]

Well we can keep SharedSecret and make it more like SerializedSignature

But the question is how do we genericize the closure passed in. What should that return? Should we make the user create a SharedSecret? (don't think so)

[dvdplm]

If we're fine passing in a dummy callback internally, then maybe having a SharedSecret::new_raw() allows us to punt on figuring out the best/idiomatic way to pass in a callback; the caller simply gets the raw secret and does whatever they need to do with it?

[elichai]

If the author is fine with it I can try making an alternative for this that will try to keep idiomaticity (with some generic functions over SharedSecret) @shmishleniy

[shmishleniy]

I'm ok with any solution that will allow to get raw signature. Thanks for support.

[elichai]

FWIW you can already unsafely do this using https://docs.rs/secp256k1/0.16.0/secp256k1/ffi/fn.secp256k1_ecdh.html (Not that I recommend doing it, as you can see there's a lot of hard safety questions here)

[apoelstra]

Closing in favor of #180