Sign/verify by digest update + StreamVerifier #735
Conversation
This struct can be use to implement verifiers with incremental updates
These allow signing/verifying a non-prehashed message but don't require the whole message to be provided at once.
This allows it to use the same implementation as non-stream signature verification.
There was a problem hiding this comment.
I'll give a tentative utACK with questions about if this has impact on benchmarks.
The main notable thing about this PR IMO is it refactors the recompute_R function into a stateful/IUF-ish RCompute struct. That seems fine to me.
I'll also argue the core rationale for this PR is similar to the "external mu" approach of ML-DSA in that rather than making a mutant IUF variant of an algorithm (i.e. Ed25519ph) you instead expose an API which allows IUF-like operations on standard Ed25519 signatures. Though "external mu" is perhaps a lot more elegant than this, but I digress.
Co-authored-by: Tony Arcieri <[email protected]>
ed25519-dalek/src/hazmat.rs
Outdated
| /// Do NOT use this function unless you absolutely must. Using the wrong values in | ||
| /// `ExpandedSecretKey` can leak your signing key. See | ||
| /// [here](https://github.com/MystenLabs/ed25519-unsafe-libs) for more details on this attack. | ||
| pub fn raw_sign_byupdate<CtxDigest, F>( |
There was a problem hiding this comment.
Are you sure it makes sense to have this function? The (prefixed) hash of M needs to be computed twice for signing. This restricts the kinds of things the msg_update closure can do. Eg it cannot consume an incoming network stream, since it'll have nothing to hash the second time it's called.
So I propose: don't make new signing procedures, and leave the current signing procedures alone. Either that, or, if we really want, we can make it take msgs: &[&[u8]] to handle non-continguous messages (as suggested below). What do you think?
There was a problem hiding this comment.
happy to remove this one too as my main interest is the new StreamVerifier which works fine without these new functions
There was a problem hiding this comment.
Ty! Can you also remove the second line of the changelog entry and raw_sign_byupdate?
There was a problem hiding this comment.
Sorry, should have been clearer. All of the raw_sign_byupdate methods. Including this one, and revert raw_sign to the original
There was a problem hiding this comment.
The rationale is in #556 (comment)
The signature is over a message made up of various parts, I'm using the serialiser to feed directly into the hash function. This is an example of using it, instead I've had to allocate a few hundred byte buffer to write into then sign (and it has fixed username length limitation assumptions). A &[&[u8]] wouldn't work for my case.
There was a problem hiding this comment.
Ahh ok I’m open to this. I feel bad for requesting so many changes. I can break this into a separate PR today
|
@tarcieri re performance: I see no statistically significan't changes on my M1 Air, EXCEPT I get an 11% slowdown on strict signature verification. Regular signature verification is unaffected though. I'm baffled at that |
|
See also: #763 |
|
Ok I tested on an M3 MBP and can't observe a performance difference at all. I'm willing to believe that the regression is spurious. |
see description and discussion in #556
this PR reuses all commits from that one, and adds in the feature guard we talked about
nightly CI failure is unrelated