Open mkj opened 1 year ago
Sorry, been sitting on this. This looks like exactly the refactor we wanted! Will take a closer look soon.
Since my original commit is contained in this PR, I don't mind which is merged. However, I still would like to see this functionality considered.
So, I'm wavering on this a bit. If you can help my understanding it would be much appreciated.
In what scenario would you be passing in message chunks without buffering them? I can only imagine two cases: (1) the message does not matter, or (2) the application is processing the message incrementally before the signature verification has completed. In the first case, one should simply not check the signature. In the second case, that can be very dangerous. In fact, the ed25519 RFC explicitly says not to do an incremental verification API for this reason.
I think I'd still be open to making this feature hazmat
, but it might be even more error prone than the existing hazmat functions. Thoughts?
For my use case the signed message I want to verify is not directly received on the wire - it's over a message constructed from a few sources (session_identifier
binds it to the session, the rest is from the network transport).
https://www.rfc-editor.org/rfc/rfc4252.html#page-10
The value of 'signature' is a signature by the corresponding private
key over the following data, in the following order:
string session identifier
byte SSH_MSG_USERAUTH_REQUEST
string user name
string service name
string "publickey"
boolean TRUE
string public key algorithm name
string public key to be used for authentication
(The message is inside a SSH transport so integrity is already validated, not that it's that relevant here)
Ok, so I suppose the summary of the use case is: for when you have a cobbled-together context that you'd like to check the integrity of, and don't have the buffer space to do so. I'm fine putting this in hazmat. @tarcieri ?
I'm fine seeing this feature under hazmat
too, given your hesitancy for incremental verification.
My use case is demonstrated in this PR https://github.com/robjtede/actix-web-lab/pull/34. In web framework land we're not in the business of buffering large responses due to the real risk of memory DoS attacks and I'd like to support Discord (and others') Webhooks without either opening up such attack vectors or unnecessarily limiting payload sizes.
I'm generally fine with this, and the use cases are real
Ok cool. I'll do some cleanup and ask for a review soon
The build failure looks unrelated. I guess it's another warning that was recently added to the compiler?
@mkj perhaps rebase if you haven't already. main
should be clean
Rebased to current main
Still a strong appetite from my end to see this feature released. Looks like we've avoided merge conflicts so far so I guess it's just waiting for a contributor review 🙏
Made a few fixes to docs/comments and the changelog
It's awesome to see this approved. We're looking forward to being able to use this over in twilight
.
I apologize for taking so long. I intend to catch up in 2wks.
In the meantime, would it be possible to feature-gate verify_stream()
and mod stream
? For the reasons upthread, I think they should probably be behind a streaming
feature, with some brief description of the danger in the README. Does that make sense? Are we okay with another feature @tarcieri or should it just go behind hazmat
?
Any quick thoughts on the feature gating point above @tarcieri ?
I'd be fine with either a separate feature or just putting it under hazmat
, though I'm not sure what the benefit of the former would be over the latter
This replaces https://github.com/dalek-cryptography/ed25519-dalek/pull/304
I'd like to be able to sign/verify non-prehash signatures without the whole message in memory. The use case is for running on
no_std
embedded where the message is serialized directly into the sha512 digest. It's for SSH protocol so I can't use ed25519 prehashed.The
StreamVerifier
pull request #542 provides similar functionality, though streaming is only possible for verify (signing needs two passes). Instead I've addedraw_sign_byupdate()
andraw_verify_byupdate()
that take a closure to update the message digest.I've included the
StreamVerifier
commit from #542 and movedrecompute_R
into its own structRCompute
. That lets all the verifier options use the same code path._byupdate
isn't the best name, but other names I came up with would get confused with prehashed methods. I'm open to other suggestions.