Skip to content
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

Add StreamVerifier #542

Closed

Conversation

robjtede
Copy link
Contributor

@robjtede robjtede commented Jul 2, 2023

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

@robjtede robjtede marked this pull request as ready for review July 2, 2023 17:40
@robjtede robjtede force-pushed the streaming-verifier branch 2 times, most recently from ad3c67f to 230a842 Compare July 2, 2023 17:44
@elichai
Copy link
Contributor

elichai commented Jul 27, 2023

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
Copy link
Contributor

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
Copy link
Contributor

rozbb commented Jul 30, 2023

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
Copy link
Contributor Author

robjtede commented Jul 30, 2023

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
Copy link
Contributor Author

Will rebase after CI fixes here: #543

@rozbb
Copy link
Contributor

rozbb commented Jul 30, 2023

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
Copy link
Contributor Author

robjtede commented Jul 30, 2023

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
Copy link
Contributor

rozbb commented Jul 30, 2023

Ah np. I'll take a crack at it

@mkj
Copy link
Contributor

mkj commented Jul 31, 2023

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
Copy link
Contributor

pinkforest commented Aug 8, 2023

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
Copy link
Contributor Author

superseded by probably #583 or #556

@robjtede robjtede closed this Sep 12, 2023
@robjtede robjtede deleted the streaming-verifier branch September 12, 2023 18:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants