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

Sign/verify by digest update, StreamVerifier refactoring #556

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

mkj
Copy link
Contributor

@mkj mkj commented Jul 31, 2023

This replaces dalek-cryptography/ed25519-dalek#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 added raw_sign_byupdate() and raw_verify_byupdate() that take a closure to update the message digest.

I've included the StreamVerifier commit from #542 and moved recompute_R into its own struct RCompute. 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.

@mkj mkj mentioned this pull request Jul 31, 2023
@rozbb
Copy link
Contributor

rozbb commented Aug 12, 2023

Sorry, been sitting on this. This looks like exactly the refactor we wanted! Will take a closer look soon.

@robjtede
Copy link
Contributor

robjtede commented Aug 27, 2023

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.

@rozbb
Copy link
Contributor

rozbb commented Sep 6, 2023

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?

@mkj
Copy link
Contributor Author

mkj commented Sep 6, 2023

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)

@rozbb
Copy link
Contributor

rozbb commented Sep 6, 2023

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 ?

@robjtede
Copy link
Contributor

robjtede commented Sep 6, 2023

I'm fine seeing this feature under hazmat too, given your hesitancy for incremental verification.

My use case is demonstrated in this PR robjtede/actix-web-lab#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.

@tarcieri
Copy link
Contributor

tarcieri commented Sep 6, 2023

I'm generally fine with this, and the use cases are real

@rozbb
Copy link
Contributor

rozbb commented Sep 6, 2023

Ok cool. I'll do some cleanup and ask for a review soon

@tarcieri
Copy link
Contributor

tarcieri commented Sep 6, 2023

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

@mkj
Copy link
Contributor Author

mkj commented Dec 27, 2023

Rebased to current main

@robjtede
Copy link
Contributor

robjtede commented Feb 12, 2024

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 🙏

mkj and others added 5 commits February 14, 2024 19:26
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.
@mkj
Copy link
Contributor Author

mkj commented Feb 14, 2024

Made a few fixes to docs/comments and the changelog

mkj added a commit to mkj/sunset that referenced this pull request Mar 3, 2024
Uses ~120 bytes more stack. Can be replaced once PR is merged
dalek-cryptography/curve25519-dalek#556
@Monadic-Cat
Copy link

It's awesome to see this approved. We're looking forward to being able to use this over in twilight.

@rozbb
Copy link
Contributor

rozbb commented Apr 19, 2024

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?

@robjtede
Copy link
Contributor

Any quick thoughts on the feature gating point above @tarcieri ?

@tarcieri
Copy link
Contributor

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

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.

5 participants