-
Notifications
You must be signed in to change notification settings - Fork 264
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Alternative: Passing custom hash functions to ECDH #180
Conversation
Arghh this still isn't safe. I need to wrap the whole |
I see two options:
@apoelstra Would love to hear your thoughts :) |
Last commit is an example of how it would look with |
Re-panicking callbacks, good catch, it's a problem I didn't consider. Going the |
You can still compile with |
Yeah, I think we should add an Concept ACK the existing code. |
5f5880f
to
eadc88f
Compare
The exact traits implementation for |
We should also implement I have a test case now (communication encryption with a Coldcard wallet) so I will try to use this PR for that today and report back. |
Tested the code; I'm happy with the API and it works (I can communicate with the Coldcard in an encrypted fashion.) Will review in detail tomorrow. |
21d304d
to
d2c2e5d
Compare
ACK d2c2e5d did some testing locally, but CI is probably more complete |
d2c2e5d
to
785a82a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me overall! 👍
src/ecdh.rs
Outdated
|
||
#[test] | ||
#[should_panic(expected = "This is UB")] | ||
// This test is technically underined behavior. but it's interesting to see if the behavior changes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/underined/undefined
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it really a good idea to have this test? I mean, it could fail on other C implementations even, no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah you're right. It was mainly me curious when/where/if this test will ever get broken (even though it's UB it's an indicator of the current instability of unwrapping through FFI, there are some proposals to just make this abort or something similar.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Heh, yeah, we should probably remove this. Or maybe comment it out or something.
&self.0[index] | ||
/// Set the length of the object. | ||
pub(crate) fn set_len(&mut self, len: usize) { | ||
self.len = len; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if this should be marked unsafe
. It is not technically unsafe, in that we're using safe code so it won't allow access to uninitialized memory or OOB access, but it seems a bit footgunny
src/ecdh.rs
Outdated
where F: FnMut([u8; 32], [u8; 32]) -> SharedSecret | ||
{ | ||
let mut ss = SharedSecret::empty(); | ||
let hashfp: ffi::EcdhHashFn = hash_callback::<F>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is really a fascinating use of Rust's type system. I did not know that generics and FFI would be allowed to interact like this.
src/ecdh.rs
Outdated
&mut hash_function as *mut F as *mut c_void, | ||
) | ||
}; | ||
debug_assert!(res >= 16); // 128 bit is the minimum for a secure hash function and the minimum we let users. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like this, though of course users are welcome to bypass it by calling set_len
(or just copying into a slice and truncating) on the result.
ACK except that I would like you to remove the UB test. (The other nits would be nice to fix, but no big deal.) |
ping @elichai |
785a82a
to
92c42ca
Compare
@apoelstra Done |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks!
This is an alternative to #164.
It's a lot more code but I feel it's more idiomatic that way.
the
ffi::SharedSecret
wasn't really needed so I removed it.Review Tip:
cargo doc
locally and compare with https://docs.rs/secp256k1/0.16.0/secp256k1/ecdh/struct.SharedSecret.htmlBasically all the user need to do is use an array and do
hash.into()
. I hope it's an easy enough to use API, I feel like the more complicated our trait system gets the harder it is for users to use, but I still think it's better than runtime checks.