added support for aes128gcm

[mplatov]

This adds support for aes128gcm as it was described here: https://github.com/zaru/webpush/issues/60

[zaru]

@mplatov Thanks! Please add spec.

[mohamedhafez]

@mplatov I tried testing this code change with aes128gcm enabled, and found that while everything works fine in the master branch, when I try to send a push notification to Chrome with VAPID using your code, while I do get a 201 response, no push notification is ever delivered. When I switch back to the master branch, everything works fine again, so I'm guessing there is something wrong with this implementation. Sorry I don't know enough about any of this encryption stuff to fix things up myself.

Note: I'm using the code at https://github.com/mohamedhafez/webpush/blob/aes128gcm/lib/webpush/encryption.rb, which is your code change minus the ability to choose between aesgcm and aes128gcm, it just uses aes128gcm every time since there is no need to fall back to anything else. Also its merged with the latest master at the time of this comment

[mplatov]

Replacing encryption implementation is not enough. As I mentioned here, more changes are needed to indicate that encrypted payload uses this type of encryption.

[mohamedhafez]

@mplatov I made the changes you mentioned, namely removing the Crypto-Key and Encryption headers, however now I get the following error when I try to push:

Webpush::Unauthorized (FOR WEBPUSH ID 13466: host: fcm.googleapis.com, #<Net::HTTPForbidden 403 Forbidden readbody=true>)
body:
crypto-key header had no public application server key specified. crypto-key header should have the following format: p256ecdsa=base64(publicApplicationServerKey)

I think the problem is the Authorization header: I think that needs to be inside the encrypted payload as well somehow: in the VAPID spec, the example it gives doesn't have one, but simply removing it doesn't work either.

(The code I'm working on is at https://github.com/mohamedhafez/webpush/tree/aes128gcm btw)

[mplatov]

Authorization data should be prepended to the encrypted payload. This link describes how this data should be prepared.

[mohamedhafez]

@mplatov I see, unfortunately I'm not really familiar with how to work with encryption byte-level stuff like that, if you or anyone else, maybe @collimarco, @rossta, or @zaru would be willing to take care of that I'd be happy to do the other legwork like fixing the specs (sorry, I'm just swamped with work right now)

The code I'm working with is at https://github.com/mohamedhafez/webpush/tree/aes128gcm

[mplatov]

It's a bit more tricky than just removing headers... and a bit easier if supporting old encryption method via configuration option is not needed. I've made other necessary changes in this pull request.

With these changes the gem works for me when sending for Firefox and Chrome (I believe you have to use VAPID keys for aes128gcm encryption).

PS: Pretty sure it will break some of the gem's tests.

[mohamedhafez]

@mplatov, it looks like you branched off of the master branch of my fork, which hadn't be updated since 2017, I was working out of the aes128gcm branch of mohamedhafez/webpush, would you mind basing the changes off of that and re-sending that pull request to that branch? Or better yet just fork off the main repo and I can fork from you when I fix up the tests.

[mohamedhafez]

Also @mplatov, I'm pretty sure aes128gcm is supposed to work with or without VAPID, though what that entails I don't know since it does seem like a lot of aes128gcm is putting in VAPID headers into the payload. https://www.w3.org/TR/push-api/ says that aes128gcm is the only encoding that user agents are required to support, and VAPID is voluntary... @jrconlin maybe you can help clarify?

[jrconlin]

Just to be clear, aes128gcm doesn't require VAPID. Google does. aes128gcm actually gets rid of a couple of headers by putting that extra stuff inside the payload. I'm guessing that's part of what might be confusing.

The fact that you're getting a 200 back means that GCM/FCM is accepting the payload. Of course, your app could be having a problem decrypting it, or the service worker might not be handling the payload correctly, or any number of other reasons.

I've not tried it with Chrome, but I know that in Firefox, you can turn on the Browser Console [Ctrl+Shift+J] (which is different than just the Web Console). The Browser Console displays messages from the browser chrome and will show if the notification was received, and if there were any problems decrypting it. It's quiet if there aren't any problems. You might want to test against Firefox to see what's up and then switch to Chrome to get their bits in order. (Added bonus, Mozilla doesn't require VAPID, so you can just test the protocol directly.)

[mohamedhafez]

I think when you aren't using VAPID, Google will still let you send but there is some non-standard stuff that you have to do like putting an api key in the header that you got from the Firebase Console. We'll have to continue supporting that I suppose, but the last time I took a look at it I got the impression they would accept aes128gcm even on their non-standard non-VAPID implementation. I'm just hoping we don't have to keep around the old aesgcm code just for that, since Google has said eventually they'll switch over to the standard API even for non-VAPID

[mplatov]

@mohamedhafez I've merged the changes to aes128gcm branch and made a pull request from there. Is still works for me both in Firefox and Chrome.

Added bonus, Mozilla doesn't require VAPID, so you can just test the protocol directly.

I can't really test that now, because push tokens that I normally use have VAPID keys, which means they can't be used without it. However, if one omits :vapid key from payload_send arguments the request will be send without Authorization header, so it should be easy to test with a fresh token.

but the last time I took a look at it I got the impression they would accept aes128gcm even on their non-standard non-VAPID implementation.

It didn't work like that when I was developing this feature few months ago. I've just tested it now (using GCM API key instead of VAPID authorization header) and FCM returned this error message: the key in the authorization header does not correspond to the sender ID used to subscribe this user. Please ensure you are using the correct sender ID and server Key from the Firebase console.

But maybe it's also related to fact that my browser push token was requested with a VAPID key. The current gem's code is such, so it will prefer GCM API key if both GCM API key and VAPID credentials are provided, so it should be possible to test with a fresh token without VAPID keys.

[mohamedhafez]

@mplatov Good news! Yeah it definitely seems to work with Google's non-standard non-VAPID implementation, you are correct you just need to request the browser push token without VAPID, otherwise you have to sign with VAPID on every push. I also tried Firefox with and without VAPID, and Chrome with VAPID, and everything worked just fine!

Before I get started with fixing the tests, does @collimarco or anyone else want to test it out real quick and see if things are working smoothy for all their use cases?

[collimarco]

@mohamedhafez Great, if it works on Chrome and Firefox with VAPID, which are the two major implementation, I think that you can safely proceed. If you want to make an additional verification you can test with Edge, just to be 100% sure.

In any case, if there are problems in production, we could always stick to a previous version, until the bugs are fixed.

[mohamedhafez]

Ok, almost there! I fixed up all but 2 of the tests, and fixed a small regression where the an empty message caused an error. Also, I tested with Edge with VAPID and it works there too. (I was unable to do a test for Edge without VAPID unfortunately)

The two remaining failing tests are in encryption_spec.rb here and here, the issue is the same in both tests, namely that the tests aren't accounting for the fact that the payload has a header now.

I could have just stripped off the header and I'm sure it would have worked, but we also should be testing that the header is containing the correct info. My thinking is that we should

1) have Webpush::Encryption.encrypt just return the string ciphertext_with_header, and leave out all the other stuff in the hash it currently returns, since none of that stuff is needed anymore outside of those two failing tests.

and

2) for those tests, extract all the info we need just from the payload header and then decrypt the payload using that info, exactly like a browser will have to do.

I could do 1) easily enough if you could do 2) @mplatov, sorry again I'm just not familiar with byte-level stuff and working with the encryption libraries.

(The lastest code is still in the aes128gcm branch of my fork at https://github.com/mohamedhafez/webpush/tree/aes128gcm)

[mohamedhafez]

Ok I implemented the changes from bullet point 1 in my last comment, @mplatov or anyone else that knows how if you could take care of bullet point 2, fixing the two broken tests by decrypting the new payload with just the information a browser will have, without the benefit of all the intermediate values we used to pass along in @payload (everything we need to decrypt is now in the payload header, and using that to successfully decrypt will test out that it is being formed correctly)

The code is in the latest commit to the aes128gcm branch of my fork at: https://github.com/mohamedhafez/webpush/tree/aes128gcm

[mohamedhafez]

So I've actually gotten pretty far trying to fix up the tests myself, in encryption_spec.rb I'm able to recalculate all the intermediate values that we used to return as part of the hash returned from encrypt, however I'm just hitting a snag trying to get ECE to decrypt with aes128gcm. For the code

      decrypted_data = ECE.decrypt(ciphertext,
                                   key: shared_secret,
                                   salt: salt,
                                   server_public_key: server_public_key_bn,
                                   user_public_key: decode64(p256dh),
                                   auth: decode64(auth))

ECE automatically uses aesgcm instead of aes128gcm because of the auth parameter, but when I take it out it throws an error. I'll play around with this a bit more and see if I cant figure it out, but just wanted to update here that I may be able to polish this off myself, if not I may just need a bit of help trying to get ECE to decrypt and thats all

[mohamedhafez]

ok I'm pretty stumped on how to get ECE to decrypt the payload, and thats the last stop to finish off fixing the specs for this PR, if anyone else who might know how could take over from here or even just offer a tip that would be great! @mplatov, @zaru, @rossta

(again me and @mplatov have been working off of this branch: https://github.com/mohamedhafez/webpush/tree/aes128gcm)

[mohamedhafez]

I've opened up https://github.com/zaru/webpush/pull/84 to request a pull directly from the branch we are actually working on, just to avoid confusion and so we can all see the commits being made to it and all that