search

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.08k stars 362 forks source link

re-export curve25519-dalek features #453

Closed nworbnhoj closed 9 months ago

nworbnhoj commented 2 years ago

Would it be possible for libsignal-client/Cargo.toml to re-export curve25519-dalek features?

nightly = [curve25519-dalek/nightly] default = [curve25519-dalek/default] std = [curve25519-dalek/std] alloc = [curve25519-dalek/alloc] u32_backend = [curve25519-dalek/u32_backend] u64_backend = [curve25519-dalek/u64_backend] simd_backend = [curve25519-dalek/simd_backend]

In particular I am seeking std and u32_backend - and include the others for sake of completeness.

jrose-signal commented 2 years ago

Hm, interesting. I was going to say you can specify this manually in Cargo.toml or in the command line, but that would only work if you could turn off the default u64_backend. (std is also included by default.) Any suggestions on how to expose that as well? And what's your use case?

nworbnhoj commented 2 years ago

thank you @jrose-signal.

As you point out, features can be specified at the command line or in Cargo.toml - but only in code with a direct dependency on curve25519-dalek. It is my understanding that libsignal-client must re-export these features for them to be available to subsequent dependents.

Also, AFAIK default = ["std", "u64_backend"] can be disabled at the command line with --no-default-features or within a [dependency] declaration with default-features = false. There is no need to expose it per-sae.

I am working to build a Signal client for the 32 bit Precursor hardware. This mobile device captured my attention with its claim of verifyable hardware. Precursor is powered by an FPGA-hosted, soft-core System-on-Chip (SoC), which means that the CPU may be verified and compiled from design source with no hidden instructions or other backdoors. Signal seems to me to be an important App to have on such a device.

nworbnhoj commented 2 years ago

While working thru other dependencies for the Precursor project I note that both zkgroup and poksho re-export the required features.


[features]
default = ["u64_backend"]
u32_backend = ["curve25519-dalek/u32_backend"]
u64_backend = ["curve25519-dalek/u64_backend"]
simd_backend = ["curve25519-dalek/simd_backend"]
nightly = ["curve25519-dalek/nightly"]```
nworbnhoj commented 2 years ago

Perhaps this should be raised as a separate issue (?) but it is closely related.

As features u32_backend and u64_backend are mutually exclusive, it would also be helpful for libsignal-client to generate a compile error (as per The Cargo Book)

#[cfg(all(feature = "u32_backend", feature = "u64_backend"))] compile_error!("feature \"u32_backend\" and feature \"u64_backend\" cannot be enabled at the same time");

rubdos commented 2 years ago

There are rare cases where features may be mutually incompatible with one another. This should be avoided if at all possible, because it requires coordinating all uses of the package in the dependency graph to cooperate to avoid enabling them together. If it is not possible, consider adding a compile error to detect this scenario. For example: [..]

So honestly, I would say this is a problem in curve25519-dalek; they should be able to expose the serial back-end in a cleaner way. As an aside, curve25519-dalek exists twice in any signal build. Once with Signal's lizard patch, and once without, so selecting u32_backend once is not even enough :-)

Anyway, I feel like we should find a solution upstream and fix the build system in curve25519-dalek. Features should be additive if possible. Since neither libsignal-client nor libsignal-service-rs have any use for exposing these features except for passing them through, I don't think like a change to our libraries is in place.

Some suggestions for a patch to curve25519:

  1. Remove the u64_backend feature. Selecting u32_backend switches off the default u64_backend. Since there are only two serial backends (and I don't see another one appearing any time soon), that should suffice. You can then just include curve25519 = { version = "3.2", features=["u32_backend"] }, which under the Law of Additive Features should work.
  2. Remove both serial backend feature flags altogether, in favour of #[cfg(target_pointer_width = "64")] guards. I see some disadvantages in here if you don't have the freedom. Maybe for some 32-bit CPUs, the u64 backend is actually faster.
  3. A hybrid of the above, where the feature overrides the cfg

Whichever solution you choose, you can use a patch section to include the patched curve25519-dalek in your experiment, @nworbnhoj.

Finally, I'd like to cc @hdevalence for their thoughts on this matter.

nworbnhoj commented 2 years ago

@jrose-signal I ran some local tests on the behaviour of --no-default-features and found that it does not flow thru as I understood. Indeed it appears that each dependency would require a default-features=false in addition to the requested re-export of u32_backend. A more impactful change unfortunately.

libsignal-client/rust/protocol/Cargo.toml

[dependencies]
curve25519-dalek = { version = "3.0", features = ["serde"], default-features = false }
x25519-dalek = { version = "1.0", default-features = false }

[features]
default = [ "u64_backend" ]
u32_backend = [ "curve25519-dalek/u32_backend", "x25519-dalek/u32_backend" ]
jrose-signal commented 2 years ago

Yeah. I think zkgroup and poksho cribbed their feature re-exporting from other crates that did this, but from libsignal-client's perspective (and zkgroup's, and poksho's) it's really an implementation detail; if we re-exported the features but changed to some other curve25519 implementation, they would no longer make sense.

As an aside, curve25519-dalek exists twice in any signal build. Once with Signal's lizard patch, and once without, so selecting u32_backend once is not even enough :-)

Within Signal's repository that's handled by a patch section in the workspace, so that even the projects that don't need the lizard APIs build with the lizard-enabled ("saurian"?) curve25519-dalek fork. Downstream projects can do that too, but it is kind of weird.

nworbnhoj commented 2 years ago

if we re-exported the features but changed to some other curve25519 implementation, they would no longer make sense.

Indeed, if there were no-longer a feature u32_backend then it would no longer make sense to develop a Signal client for the 32bit Precursor. But it sure would be nice to proceed while the u32_backend exists.

Maybe for some 32-bit CPUs, the u64 backend is actually faster.

So I am missing something important here. I can use the u64_backend on a 32-bit CPU?

rubdos commented 2 years ago

if we re-exported the features but changed to some other curve25519 implementation, they would no longer make sense.

Indeed, if there were no-longer a feature u32_backend then it would no longer make sense to develop a Signal client for the 32bit Precursor. But it sure would be nice to proceed while the u32_backend exists.

I only recommend getting rid of the feature flag, I don't recommend getting rid of the implementation. It probably has a place (and indeed is probably faster than the u64 on certain/most 32-bit CPUs). The only thing we are trying to convey here, is that the specific use of the feature flags here is non-idiomatic and should be considered "a flaw" in the design of Curve25519-dalek.

Maybe for some 32-bit CPUs, the u64 backend is actually faster.

So I am missing something important here. I can use the u64_backend on a 32-bit CPU?

I don't see why not. It just uses u64 and u128, both available. I am using the default features since forever on Whisperfish, which is compiled to i686, armv7hl and aarch64.

rubdos commented 2 years ago

Quick benchmarks of variable base scalar mult in curve25519-dalek-ng, in cooperation with @thvdveld, because you now nerd-sniped us.

  1. On a Cortex-M3 (Texas Instruments CC2538):

Thibaut does say he doesn't trust the cycle counter 100%, but at least it works and is kinda fast :-)

These things are clocked on 32MHz, so that's something like 14 ms.

  1. On my Xperia 10 with SailfishOS (armv7hl user space on aarch64 kernel, yes I know, rustflags = "-C target-feature=+v7,+neon"):

... I honestly did not expect this, and this does give me also an incentive to have the u32 backend for Whisperfish/Presage... 90% of our user base is on such a setup.

I would add a bench on a Raspberry Pi 4 on aarch64, but I have to go home now :-)

jrose-signal commented 2 years ago

That's startling, we (Signal) should probably test that for 32-bit Android as well.

rubdos commented 2 years ago

That's startling, we (Signal) should probably test that for 32-bit Android as well.

If you/us are going to patch curve25519-dalek, it might also be useful to switch to curve25519-dalek-ng.

EcoloSweet commented 2 years ago

If you/us are going to patch curve25519-dalek, it might also be useful to switch to curve25519-dalek-ng.

Could you explain why we should not use the original curve25519-dalek crate ? Because of the Dalek Github organization takeover ? as I can see, there has been no problem with this crate since it happened, and they both (dalek and zkcrypto) look like they are not actively maintained (last update 7 months ago, with pending pull requests), so I was wondering...

rubdos commented 1 year ago

Could you explain why we should not use the original curve25519-dalek crate ? Because of the Dalek Github organization takeover ? as I can see, there has been no problem with this crate since it happened, and they both (dalek and zkcrypto) look like they are not actively maintained (last update 7 months ago, with pending pull requests), so I was wondering...

Well, at the time, the zkgroup fork seemed more active, and it received some attention back then. Ultimately, it doesn't really seem to matter at this point.

Getting the lizard2 code out in one of those repos would be nice, however. I might give that a shot at some point, just to clean things up. I'm maintaining forks for Whisperfish, and now forks of forks of forks for benchmarking our neon code on Sailfish devices. :man_shrugging:

jrose-signal commented 9 months ago

Closing this since curve25519-dalek is maintained again (by folks from RustCrypto). Tying up some loose ends: