signalapp / libsignal

Home to the Signal Protocol as well as other cryptographic primitives which make Signal possible.
GNU Affero General Public License v3.0
3.05k stars 359 forks source link

Seperate Lizard into its own crate. #540

Closed kotval closed 1 month ago

kotval commented 8 months ago

I am attempting write a signal client for the precursor (https://betrusted.io/) (running xous. This device has hardware acceleration for curve 255519, and provides a binding for rust through the curve25519-dalek crate. In order to build libsignal for inclusion in an app image for this operating system, I have to apply a patch pointing at your fork of curve25519-dalek (as you do here: https://github.com/signalapp/libsignal/blob/d1f9dff273e6da059af699c6afe860fb93406032/Cargo.toml#L32), but because cargo applies patches to all transitive dependencies, this breaks being able to use the hardware acceleration binding. It was designed so that any rust code which needed curve25519-dalek could simply swap out the dependencies and achieve hardware acceleration.

So, I am left with a few options. 1) change the OS version of the crate to signal's fork, while this would work, it would mean that we'd force all other applications to use your version, which is not ideal since its not published on crates.io and we'd like to keep vendored code to a minimum. 2) Ask you to change the name of the the curve25519-dalek module when you import it rather than patching it, since that seems to be the way to use a package which is not available without forcing all other crates which use it to be patched as well. 3) Fork libsignal and make this change, and have to keep my fork up to date. 4) Make the Lizard code it's own crate.

Note that this is a particular problem for our project since we need to link with the hardware accelerator. I'm sure that all other users are happy compiling libsignal as a binary and linking to it, but I cannot even build the appropriate binary.

To consider which of the above options is best, I began looking at the lizard changes I realized that 1) they are completely orthogonal to the rest of the crate, and could just as easily be in their own crate, 2) it doesn't seem like there is any effort to get these features merged into the upstream curve25519-dalek, 3) it seems that the only place in libsignal that these features are used is currently a stub indicating that is in fact not used (https://github.com/signalapp/libsignal/blob/d1f9dff273e6da059af699c6afe860fb93406032/rust/zkgroup/src/crypto/uid_struct.rs#L17). These lead me to the conclusion that the best solution would be to make Lizard its own crate.

While I know you clearly state that you don't intend to support a public api in libsignal, it would be nice if I didn't have to maintain a fork of libsignal simply to remove this patch, so I have a proposal, and I am willing to do all the work as long as you will review my PRs.

I would like to 1) separate out the Lizard code into a separate crate and publish it on crates.io, 2) modify libsignal to use this lizard crate and the standard version of curve25519-dalek. That is all. If you are willing to accept such a PR, I will open one. If not, let me know, and I'll just maintain a fork.

jrose-signal commented 8 months ago

We can't make Lizard its own crate because it pulls from the implementation details of curve25519-dalek. We've considered upstreaming it, or rather its base APIs from_uniform_bytes_single_elligator and decode_253_bits, but haven't gotten around to it because it's pretty niche, and because upstreaming is always additional work. (Our implementation doesn't currently work with the fiat-crypto backends, for example.)

Only zkgroup actually depends on the Lizard extensions to curve25519-dalek, but that's no help for you if you're trying to make a full Signal clone—something that Signal-the-organization does not support, but very understandable for an OS that isn't one of the Big Five. (That "currently unused" comment applies only to the raw_uuid_bytes field; the rest of UidStruct is still used.) Unfortunately, the other crates in libsignal are also why we aren't renaming the crate: we want to use our fork for "normal" curve25519-dalek uses in libsignal-protocol and attest and elsewhere, to prevent having two copies of the crate's code in the final build of the app.

I realize this doesn't leave any good paths forward for you. :-/ If you want to assist our upstreaming efforts, getting the Lizard extensions working on all of curve25519-dalek's backends would be a big help, but that's entirely up to you, and there are still no guarantees—the current maintainers could easily decide they aren't interested in supporting this use case.

kotval commented 8 months ago

Thanks for the clarification. For now, I will fork the crate so that I can keep working. I will look into helping upstream the changes to curve25519-dalek, as that might be educational for me, but no promises.

stale[bot] commented 5 months ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

kotval commented 5 months ago

Update as this was marked as stale. It seems likely easiest to vendor lizard2 into our own fork of curve25519-dalek as well. The orthogonality of it to the rest of the code makes keeping up to date with your fork not much of a burden. As long as that works for us, we will likely not attempt get lizard2 separated out.

jrose-signal commented 5 months ago

We haven't gotten any response on the upstream issue (https://github.com/dalek-cryptography/curve25519-dalek/issues/533#issuecomment-1850590569), so that's probably your best bet, yeah.

kotval commented 5 months ago

I had briefly considered asking them to make a hazmat module similar to their (https://docs.rs/ed25519-dalek/latest/ed25519_dalek/hazmat/index.html) for curve25519-dalek which would permit it to at least be its own crate, but I am not sure if they would be amenable to that either.

stale[bot] commented 2 months ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

stale[bot] commented 1 month ago

This issue has been closed due to inactivity.