dalek-cryptography / curve25519-dalek

A pure-Rust implementation of group operations on Ristretto and Curve25519
Other
867 stars 439 forks source link

Add StreamVerifier #542

Closed robjtede closed 12 months ago

robjtede commented 1 year ago

Context

In web applications, it is desirable to avoid storing the request payload. It is currently impossible to verify a signature based on a request payload that is receieved in a chunk stream without allocating additional memory equal to the size of the payload.

Solution

A stateful type that holds the public key, candidate signature, and hash state that can be updated incrementally.

References

elichai commented 1 year ago

Why not a function that takes impl Read instead? (and then you can use std::io::copy to copy into the hasher or use an intermediate buffer with a better size) @tarcieri Maybe it's worth adding an MAX_OPTIMAL_BUFFER_SIZE (bikeshedding aside) constant to the Update trait for those reasons? see for example the docs blake3 has:

Note that the degree of SIMD parallelism that update can use is limited by the size of this input buffer. The 8 KiB buffer currently used by std::io::copy is enough to leverage AVX2, for example, but not enough to leverage AVX-512. A 16 KiB buffer is large enough to leverage all currently supported SIMD instruction sets.

Source: https://docs.rs/blake3/latest/blake3/struct.Hasher.html#method.update

tarcieri commented 1 year ago

Why not a function that takes impl Read instead?

That requires std

Maybe it's worth adding an MAX_OPTIMAL_BUFFER_SIZE (bikeshedding aside) constant to the Update trait for those reasons?

I thought we had something like that already but now I can't find it, so maybe we need to add something.

rozbb commented 1 year ago

Maybe silly question: isn't this what prehashed signatues are for? Or is this in a setting where you don't have control over what the sender does?

robjtede commented 1 year ago

you don't have control over what the sender does

Yea exactly, my particular use case is Discord Webhooks.

There's some relevant discussion in the old PR.

robjtede commented 1 year ago

Will rebase after CI fixes here: https://github.com/dalek-cryptography/curve25519-dalek/pull/543

rozbb commented 1 year ago

Ok then I like this! One thing to avoid code duplication: do you think you could move the current VerifyingKey::{verify, verify_strict} functions to the stream verifier, and redefine the originals to use the stream version? I suspect it won't have any performance cost

robjtede commented 1 year ago

Awesome 👍🏻

redefine the originals to use the stream version

I've had a go but having trouble doing in a way that actually reduces code. From my limited knowledge of recent refactors, problem seems to be that VerifyingKey::recompute_R takes a message but we have a Scalar in the streaming case, otherwise it could be changed for the raw_verify fns. I'm hesitant to do too much refactoring in this PR.

rozbb commented 1 year ago

Ah np. I'll take a crack at it

mkj commented 1 year ago

I've pushed #556 which builds on StreamVerifier and unifies the stream/normal verifiers (as well as adding by-digest methods for signing/verify).

pinkforest commented 1 year ago

impl Read needs std

fwiw - embedded folk seems to be fan of a trait that works in no_std: https://crates.io/crates/embedded-io

but yeah that would be fragmented impl and further complicate things with endless permutations

robjtede commented 12 months ago

superseded by probably #583 or #556