Adding Aes128Gcm support via `rust-ece` crate delegation

tiesselune commented 3 years ago

About

This pull request addresses #3 by using the ece crate, which is maintained by Mozilla, to implement the Aes128Gcm encoding scheme. This way, it can be easily updated and benefit from security reviews and patches on the ece crate and the crate can focus on the VAPID part of things.

For consistency purposes, it entirely delegates ECE encoding to the ece crate, including the AesGcm scheme, which implies some refactoring.

The actual cryptographic work being implemented and tested in the ece crate, the cryptographic functions and tests in the http_ece module can be entirely delegated.

Changes summary:

Re-wrote the encrypt function in the HttpEce struct implementation to support both encodings and use the ece crate, using the same interface to avoid any breaking change
Removed cryptographic structs, functions and related tests which are no longer needed
Added new tests to test crypto headers for both AesGcm and Aes128Gcm
Changed the headers property of the WebPushPayload struct from a Vec<(&'static str,String)> to a HashMap<String,String> for ease of integration and consistency with the ece crate implementation (this might be a breaking change, since WebPushPayload is published at the crate level; it needs more investigation)
Removed http-ece from the tags of the crate, since it is now delegated to another one.

Remaining work

This pull request is a draft and it needs a few things before it can be considered:

[x] It relies on the merging of this pull request on the ece crate, which I'm working on with the maintainer ( for now, cargo.toml uses my fork)
[x] The potential breaking change on the WebPushPayload struct needs to be addressed
[x] I need to do some real-world testing with major push services to ensure full compliance.
[ ] Optionnal The merging of this pull request on the ece crate would allow integrating with its headers function without extra allocation or hashing

pimeys commented 3 years ago

What? Somebody else wrote an ece crate? Oh nice! The amount of tears and pain to write it with ring and to understand the RFCs are now gone. This is sweet, waiting for the finished version :D

pimeys commented 3 years ago

Please poke me if you're done and I'm not answering soon enough. A new sprint starting and I'm in the middle of the long meetings. Also, please update the README and see that the rustdocs reflect the current state of the crate.

pimeys commented 3 years ago

For breaking changes, we go to v0.8 anyhow because of Tokio 1 and (if somebody does it) a possible support for async-std runtime. So if you need to break any apis, now's the time.

tiesselune commented 3 years ago

Great news, my ece crate pull request has been approved and merged, and the new version published to crates.io which means that this pull request is soon to get out of WIP state! 🥳

tiesselune commented 3 years ago

Ok @pimeys , responding here cause the two concerns you presented above are actually linked. I first responded with some suggestions on how to fix the problem then dove into it and found a far simpler approach that is just a bit suboptimal in regard to code duplication between the ece crate and this one, but would be better performance-wise. So I deleted my previous answers since they're no longer relevant and don't deserve to be read. 😉

The headers function from the AesGcmEncryptedBlock struct that we get from the encrypt_aesgcm function is the culprit here; it's passing self by value, effectively consuming the block and forcing a copy to avoid a double move; it's also producing the suboptimal hashmap. The good thing is: we don't need it. So we can get rid of the cloning and the hashmap in one go by re-introducing a variant of get_headers function that you originally wrote for this even if the ece crates has a function that essentially does the same thing (but with Strings and consuming Self, forcing a copy somewhere) That's definitely a design flaw that would be fixed in another pull request for the ece crate 😉, but we can circumvent it for now quite easily.

Anyway, I'm going to submit a new commit for both those issues quite soon ;)

EDIT: For reference, I've just suggested these changes to the ece crate maintainers, and might tackle it if they want me to

pimeys commented 3 years ago

Great. Probably this kind of optimization doesn't matter for most users, but before we went bankrupt with the company I was writing this crate for, we used it to send thousands and thousands notifications per second. All extra allocations start to show up, and, well, it's very cool if you can make it as fast as possible. :D

I can accept this of course as-is too, if there's no way around some of the added computation.

tiesselune commented 3 years ago

Hello @pimeys Just to let you know that I wrote a version that avoids the allocation/hashing, but in order to fix this properly, I've opened a new pull request on the ece crate for that new issue.

There would not be any performance difference, just a question of using the functions provided by the ece crate instead of re-coding them, but basically the code is the same.

Anyway, there would be no breaking change and moving from that implementation to the one in the ece crate would be very easy and could be done at any time.

(I still need to run some real-world tests before merging)

tiesselune commented 3 years ago

Hi again @pimeys ! So I have good news and bad news.

Good news: I tested the Aes128Gcm back-end with real world notifications and it works!

Bad news: for it to work, I had to downgrade hyper, hyper-tls and tokio. If I run it with the current versions, I got the following error: there is no reactor running, must be called from the context of a Tokio 1.x runtime in the hyper::Client::send method that is called in WebPushClient::send. With the old versions, I got random stack overflows, still somewhere in the WebPushClient::send request.

Would you be familiar with the Tokio problem? Does the problem occur on your end with the current master branch ?

pimeys commented 3 years ago

I'm trying to debug this on your branch, but I don't really have proper subscription info or anything, so I go with the following:

{
  "endpoint": "asdf",
  "keys": {
    "p256dh": "asdf",
    "auth": "asdf"
  }
}

I get:

    Finished dev [unoptimized + debuginfo] target(s) in 0.05s
     Running `target/debug/examples/simple_send -f info.json`
Error: Unspecified

which means that the Tokio 1.1 and Hyper 0.14 work just as they should. The error you get is that you try to run something that expects Tokio 1 in a Tokio 0.2 or 0.3 reactor. That's unfortunate how Tokio forces you to use their machinery and exact versions. Have you tried cargo clean and really be sure you run the push system at least with Tokio version 1.0 or later?

tiesselune commented 3 years ago

Hey, thanks for the quick reply. I was investigating and I came to the same conclusion. My example uses actix and it seems that the current actix_web version has a dependency that itself uses tokio 0.2. I'll need to try it with the beta version of actix.

pimeys commented 3 years ago

This is the reason I'd like this crate, a2 and fcm all to be independent on runtimes. Maybe refactoring it using https://docs.rs/async-h1/2.3.1/async_h1/ would already make it work much better with every possible system...

pimeys commented 3 years ago

Btw. I started hacking away trying to make this crate runtime-independent. This is not really in scope of this ticket, but I'm in Matrix: @oh_lawd:nauk.io if you want to chat more about the general direction of rust-web-push :)

tiesselune commented 3 years ago

I've tested against

Google's push service
Mozilla's push service
Microsoft's push service

And I am proud to announce that it works! I think this is ready for merging or at least a review ;)

pimeys commented 3 years ago

@tiesselune is it now ok to merge? I'm probably not releasing a new version until we get the runtime PR merged too. Then we go one major version up.

tiesselune commented 3 years ago

I'd like to make one more commit, but since we're going to merge your other PR and those two have a bunch of smallish formatting conflicts, it's maybe better to update it first once your other PR is merged (I did it once to test the infamous stack-overflow hyper/actix problem so the branch is basically ready, but it's on another local branch on my computer).

tiesselune commented 3 years ago

Re-converting to draft, just noticed I had some weird test failures I still have to investigate.

tiesselune commented 3 years ago

Update: the test failures come from the fact that padding has changed between the previously implemented version and the ece crate version.

Pondering if we should re-introduce padding on both schemes

pimeys / rust-web-push