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 #196

Closed
wants to merge 1 commit into from
Closed

Add StreamVerifier #196

wants to merge 1 commit into from

Conversation

robjtede
Copy link
Contributor

@robjtede robjtede commented Apr 23, 2022

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.

To Do

  • decide if this type is appropriate for ed25519-dalek
  • type name
  • add verify_stream method to Keypair
  • documentation

References

@tarcieri
Copy link
Contributor

tarcieri commented Apr 23, 2022

Note that there's a DigestVerifier trait (along with a DigestSigner trait) in the signature crate, which ed25519-dalek uses:

https://docs.rs/signature/latest/signature/trait.DigestVerifier.html

I think it would probably make sense for those to map to Ed25519ph though (although context-handling presents a complication).

@robjtede
Copy link
Contributor Author

robjtede commented Apr 23, 2022

Yeah I saw that trait but it doesn't seem to map well to ed25519 since it receives the signature after the message digest has been created and we need to add parts from the public key and signature as the initially digested parts.

@tarcieri
Copy link
Contributor

Ed25519ph is intended to be the IUF mode for Ed25519, and is already implemented by ed25519-dalek in the form of sign_prehashed and verify_prehashed.

Personally I would find it confusing to encounter an IUF mode for Ed25519 that wasn't Ed25519ph.

@robjtede
Copy link
Contributor Author

If ed25519 and ed25519ph verifications algorithms were compatible, I wouldn't have an issue.

What do you think is the best course of action for the use case listed of more-memory-efficient Discord webhook verification given I cannot change Discord itself to sign its requests using the *ph variant? This is clearly possible to acomplish unless the approach of this PR has security downsides I'm not aware of.

@tarcieri
Copy link
Contributor

You could potentially give it a different name which is less likely to be confused with Ed25519ph, like "StreamingVerifier"

@robjtede
Copy link
Contributor Author

Ahh. If this is just a naming issue then sure that seems very appropriate.

@tarcieri
Copy link
Contributor

Yeah, I think it might be nice for ed25519-dalek to eventually implement the DigestSigner and DigestVerifier traits from the signature crate for Ed25519ph, in which case it'd be nice to have a different name for an IUF verifier for traditional Ed25519

@robjtede robjtede marked this pull request as ready for review April 23, 2022 15:41
@robjtede robjtede changed the title Add DigestVerifier Add StreamVerifier Apr 23, 2022
@Monadic-Cat
Copy link

Heya! What's the status of this PR? It'd be nice to have for twilight-rs, so I'd like to help if there's anything I can do.

@tarcieri
Copy link
Contributor

Could you potentially use the hazmat low-level verifier API that was just added in #299?

Something like this might be nice as a safer API, but it should probably be (re)built in terms of the APIs introduced in #299.

@Monadic-Cat
Copy link

Monadic-Cat commented May 15, 2023

Could you potentially use the hazmat low-level verifier API that was just added in #299?

Something like this might be nice as a safer API, but it should probably be (re)built in terms of the APIs introduced in #299.

Sure! I'll try and do that today.

@Monadic-Cat
Copy link

So I did attempt redoing this PR on top of hazmat and, while I did manage to make it work, it required a dirty workaround to avoid adding any API surface to hazmat: It's a bit hard to precompute the hash used in raw_verify, as it drives the hashing itself.

I got around that by making a DirtyWorkaround struct which implements Digest, and knows that raw_verify ends up passing its message argument to the digest after 64 bytes of data, and so we call raw_verify with the precomputed hash in the message argument and extract it at DirtyWorkaround::finalize().
In the end it looks like this:

impl StreamingVerifier {
    // ...

    pub fn finalize_and_verify(self) -> Result<(), SignatureError> {
        hazmat::raw_verify::<dirty_workaround::DirtyWorkaround>(&self.public_key, self.hasher.finalize().as_slice(), &self.signature)
    }
}

This seems like an incredibly bad idea on my part, but it works, so I felt obligated to share it.
(Incidentally, I think, the way I wrote it, the whole module uses only public APIs of ed25519_dalek? So that's neat.)

@tarcieri
Copy link
Contributor

@Monadic-Cat can you open a PR so we can discuss options?

src/stream_verifier.rs Outdated Show resolved Hide resolved
Comment on lines +47 to +50
let minus_A: EdwardsPoint = -self.public_key.point;
let k = Scalar::from_hash(self.hasher);
let R =
EdwardsPoint::vartime_double_scalar_mul_basepoint(&k, &(minus_A), &self.signature.s);
Copy link
Contributor

@tarcieri tarcieri May 17, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if VerifyingKey::recover_R could be further decomposed into VerifyingKey::recover_R_from_k or thereabouts which captures this functionality.

cc @rozbb

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've made that change along with some tangential work in #304

@robjtede
Copy link
Contributor Author

robjtede commented Jul 2, 2023

migrated to dalek-cryptography/curve25519-dalek#542

@robjtede robjtede closed this Jul 2, 2023
@robjtede robjtede deleted the verify-digest branch July 2, 2023 17:34
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.

4 participants