search

pimeys / rust-web-push

A Web Push library for Rust
Apache License 2.0
113 stars 21 forks source link

Doing some updating / maybe a fork #21

Closed platy closed 3 years ago

platy commented 3 years ago

@pimeys I'm finding this library useful right now and so I'm making some updates. Since you wrote this the web push has become more standardised and so I don't think any of the special cases for firebase are needed and so most of it can be simpler (thankfully the encryption stuff can all stay as it is though).

Also, I'm not using tokio on my project and so I'm switching out hyper for a syncronous http client.

It's currently WIP, I wonder whether you would be interested in merging this or whether I should make a fork?

pimeys commented 3 years ago

Would you like to take the whole crate for yourself? I can give you the keys for the project! I'm currently not working on push notifications and I have no time to setup an env and test things.

And making this runtime-independent is an excellent idea, I'd do the same if I'd still worked with this!

platy commented 3 years ago

Thanks for the quick reply! Sure - I would be interested, it's quite a nice little library to take over :)

pimeys commented 3 years ago

Just fork and make your changes, when you need to publish something we make the transition and ask GitHub to make yours the main fork.

platy commented 3 years ago

Made the fork, and it seems to be going pretty well, I'll probably be up for releasing it Friday. To make it runtime independent I've added 2 features, http-ureq and http-hyper to decide which http client to use - it works fine but is a bit clunky - does any other method come to mind for you? Or another method that would be less surprising? https://github.com/platy/rust-web-push

tiesselune commented 3 years ago

Hi! Ive been working on this crate as well on the last few days, you both might be interested in this pull request : #23, which addresses #3 (The Aes128Gcm scheme).

It's not ready to be merged yet for several reasons but I was hoping to make it ready in the coming week, and that might become hard if our forks diverge too much (hence this little poke to make myself known)

platy commented 3 years ago

Hi! I just started to look at the aes128gcm yesterday, I was thinking to delegate to the https://crates.io/crates/aes-gcm crate but the ece crate looks like It makes more sense as it can delegate more. I will check out your branch and look at merging it into mine this week. My change will break some things anyway.

On Sun, 31 Jan 2021, at 02:17, Julien SOSTHENE wrote:

Hi! Ive been working on this crate as well on the last few days, you both might be interested in this pull request : #23 https://github.com/pimeys/rust-web-push/pull/23, which addresses #3 https://github.com/pimeys/rust-web-push/issues/3 (The Aes128Gcm scheme).

It's not ready to be merged yet for several reasons but I was hoping to make it ready in the coming week, and that might become hard if our forks diverge too much (hence this little poke to make myself known)

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/pimeys/rust-web-push/issues/21#issuecomment-770308545, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAATBT3A6KD3BOQGZYS53W3S4SVSNANCNFSM4WVIVUJA.

tiesselune commented 3 years ago

Hi! Thanks a lot. I'm currently working with Mozilla to make the ece crate actually usable for this use case (because there was a flaw in their API that prevented the AesGcm scheme to be accessed from outside of the crate), so it depends on a personal fork of the ece crate until the changes to the ece crate are merged in (which is on the right track) and I should do some more testing first (that's why it's still in draft state).

The cool thing about the ece crate is that's it's made for Web Push, so it's really easy to use and abstracts most of the complex crypto part behind a simple API. So the new code for the http_ece module becomes really lightweight.

Anyway I should have time to do some real world testing with major browser push services this week. Would you rather I tried merging your updated version into mine? I feel like the conflicts are not going to be very difficult to resolve.

pimeys commented 3 years ago

Yeah. Writing that ece api here was very painful. Remember, this crate is from the Futures 0.1 time, with Tokio 0.1 and so on, using futures combinators. Writing the ece encryption always either worked or not, and I remember back then the right RFC was like five versions behind the latest that actually worked. It's great news for maintaining rust-web-push if you don't need to care about encryption algorithms.

Also, be aware that if we continue to depend on ring, I've spent a few long nights trying to upgrade it to the latest version. Would be cool if there'd be an actual automated test that'd show us if things really break.

tiesselune commented 3 years ago

Hum I believe ring is used for the VAPID signing part, is it not? Cause the aesgcm/aes128gcm are already tested as part of the ece crate, but not the VAPID part. I'll take a look to see if there's anything I would feel comfortable doing.

platy commented 3 years ago

@tiesselune :

Would you rather I tried merging your updated version into mine?

Whatever you think is best. Unfortunately I haven't manged to work on any web push this week, I should get back to it next week. I see your rust-ece PR is merged and released - I look forward to trying it out!

tiesselune commented 3 years ago

There has been some discussions on #23 , and some changes as well, leading to another pull request on the ece crate, but it's not necessary for this one to be merged in.

What I might do is: Test my branch the way it is so #23 can be merged, then merge yours in on another branch so you can test it out!

platy commented 3 years ago

Our work is converging on this repository now - and it looks like everything that was missing will be fixed in a next release which shouldn't be far off. Closing