Support "aes128gcm" content coding defined in the latest WEBPUSH-ENCRYPTION draft

[ShijunS]

The latest WEBPUSH-ENCRYPTION draft requires the content encoding to be "aes128gcm". This is now also reflected in the Push API spec. It'd be great if the web-push-libs can include that option in addition to other options defined in previous versions of the spec.

[gauntface]

This depends on: web-push-libs/encrypted-content-encoding#32

[gauntface]

With the range of changes to the Web Push spec we could simply add supportedContentEncodings to the options in the sendNotification() "options" parameter.

const pushSubscription = ...;
const payload = 'Example';
const options = {
  supportedContentEncodings: [....],
};
sendNotification(pushSubscription, payload, options)

Developers would need to send the encoding from a user agent to their backend.

const backendData = {
  subscription: subscription,
  supportedContentEncodings: window.PushManager.supportedContentEncodings || []
};
fetch('/api/subscriptions', {....body: backendData}

@marco-c @Minishlink @jrconlin thoughts on this approach?

[jrconlin]

I'm in favor of the client specifying the sorts of content encodings that it supports (ideally in preference order). Sending them as part of, or supplemental to, the subscription info block seems like the best approach.

My only concern about supplemental is that it does require the end dev to know that they should make this extra call.

[ShijunS]

Would it be reasonable to keep the default to "aesgcm" for now, and developer can make the extra call if they'd like to pick "aes128gcm" explicitly? The default option can be updated to "aes128gcm" once we believe it has been well tested across all major browser implementations.

[Minishlink]

It feels weird to me to decouple subscription and supportedContentEncodings.
As an app server dev, I would store the subscription info and supportedContentEncodings alongside anyway (eg. same table row on a database).

But apparently the above won't happen (cf w3c/push-api#277, I commented this), so your proposed API looks great @gauntface!

@ShijunS, web-push libraries likely will default to aesgcm for now if no supportedContentEncodings is provided.

[beverloo]

@Minishlink - the issue is that storing it as such would be the wrong thing to do, because it's a snapshot taken at time of subscription, whereas the supported content encodings may change during the lifetime of the subscription. Short-lived subscriptions (i.e. pushsubscriptionchange) would make this problem much less severe, but neither Firefox nor Chrome currently refresh subscriptions.

[jrconlin]

I think "short-lived" could also be a question of time scale. I am willing to believe that most subscriptions last around six weeks or less. It could well be that a subscription expires before the browser version. The issue is more for the app provider who may need to support subscriptions coming from multiple versions with varying degrees of support while customers get around to upgrading. Frankly, any clue is better than none, and the app could fold the info into whatever is reported back to the server. We just need to have good examples or cookbooks for how to do it, because whatever we do will be copy pasted ad infinitum.

…

On Jul 12, 2017 12:46 PM, "Peter Beverloo" ***@***.***> wrote: @Minishlink <https://github.com/minishlink> - the issue is that storing it as such would be the wrong thing to do, because it's a snapshot taken at time of subscription, whereas the supported content encodings may change during the lifetime of the subscription. Short-lived subscriptions (i.e. pushsubscriptionchange) would make this problem much less severe, but neither Firefox nor Chrome currently refresh subscriptions. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#278 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AACLq4cKyb12OePMEFWpWxt_BzSTL2pYks5sNPhegaJpZM4NaKw5> .

[Minishlink]

Thanks, it's a bit clearer. I don't quite follow though for not storing supportedContentEncodings like this... How are app servers concretely supposed to encrypt with the correct content encoding for a subscription?

I don't know if it's a best practice, but in my example, I made the app to update the stored subscription on the app server on each app load (there could be some fine-tuning for the frequency). I guess it's the least we can do right now to mitigate current lack of support of pushsubscriptionchange.

[gauntface]

@Minishlink & @jrconlin I think we all in agreement that it should be sent up with the subscription.

Would you prefer adding the supported encodings to the subscription or would you prefer to keep them seperate?

My rational for keeping them seperate was that I think it'll help developers realise it's an extra piece of information that comes from somewhere other than the subscription. But I also recognise that the saving on the backend will involve developers making an API call with the subscription and supported encoding in the same payload and to send a notification they'd have to seperate these two pieces of information.

@beverloo my main fear of "Oh it's a firebase endpoint so it must accept aes128gcm" is that what happens if the browser doesn't get updated and actually it can't decrypt an aes128gcm payload? I know this is an edge case, but if a developer stores the encodings, there is extra information that might be useful to the library, OR it might simply be ignored. Because the various spec's have changed a few times already, I'm lacking faith that a breaking change won't be waiting round the corner ;)

[jrconlin]

I will almost always vote for "simplicity", even when it not "correct". In this case, the subscription already carries extra information, the encryption keys provided by the User Agent. Technically, the endpoint is the subscription, and it doesn't care about encryption. "Extending" the subscription object to include the content type seems both reasonable and rational to me because, again, it's something that is required and restricted to the User Agent. I also agree that implying special knowledge based on endpoint kinda gets away from the idea of a standard. Firebase already makes VAPID a bit less voluntary, it'd be good to not overload users with more magic incantations to get things to work. I'm not super bothered by that, though, since it's a good behavior anyway.

…

On Fri, Jul 14, 2017 at 11:45 AM, Matt Gaunt ***@***.***> wrote: @Minishlink <https://github.com/minishlink> & @jrconlin <https://github.com/jrconlin> I think we all in agreement that it should be sent up with the subscription. Would you prefer adding the supported encodings to the subscription or would you prefer to keep them seperate? My rational for keeping them seperate was that I think it'll help developers realise it's an extra piece of information that comes from somewhere *other* than the subscription. But I also recognise that the saving on the backend will involve developers making an API call with the subscription and supported encoding in the same payload and to send a notification they'd have to seperate these two pieces of information. @beverloo <https://github.com/beverloo> my main fear of "Oh it's a firebase endpoint so it must accept aes128gcm" is that what happens if the browser doesn't get updated and actually it can't decrypt an aes128gcm payload? I know this is an edge case, but if a developer stores the encodings, there is extra information that *might* be useful to the library, OR it might simply be ignored. Because the various spec's have changed a few times already, I'm lacking faith that a breaking change won't be waiting round the corner ;) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#278 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AACLq6sdfvB0paYOGvDoyrjJXjfBYzDGks5sN7c7gaJpZM4NaKw5> .

[abdulhannanali]

@jrconlin Simplicity is better, but is the User Agent itself going to include it using .toJSON or is it going to be manually extended by the user agent?

[abdulhannanali]

In that case we can just pass the subscription object, but the problem is I do think this should be added separately. @marco-c Can I start the coding on this feature?

[aliams]

So, I took a crack at this to test out the eas128gcm in Edge. I retrofitted the library to work with the latest version of the Encryped Content-Encoding library which supports aes128gcm. Here are the affected files and lines:

1. In vapid-helper.js:

   Only return the Authorization and not 'Crypto-Key.' For example:

   return {
       Authorization: 'vapid t=' + jwt + ', k=' + urlBase64.encode(publicKey)
   };

2. In encryption-helper.js:

   Remove ece.saveKey('webpushKey', localCurve, 'P-256'); (since saveKey is not available in the latest http_ece) and instead do this:

   const cipherText = ece.encrypt(payload, {
       keyid: localPublicKey,
       dh: userPublicKey,
       privateKey: localCurve,
       salt: salt,
       authSecret: userAuth
   });

3. In web-push-lib.js:

   1. Change requestDetails.headers['Content-Encoding'] = 'aesgcm'; to 'aes128gcm'

   2. These headers are not needed for aes128gcm:

      requestDetails.headers.Encryption = 'salt=' + encrypted.salt;
      requestDetails.headers['Crypto-Key'] = 'dh=' + urlBase64.encode(encrypted.localPublicKey);

   3. Add this workaround for FCM (for now) in the else if (currentVapidDetails) { block, just before const parsedUrl = url.parse(subscription.endpoint);:

      if (subscription.endpoint.indexOf('https://fcm.googleapis.com') === 0) {
          subscription.endpoint = subscription.endpoint.replace('fcm/send', 'wp');
      }

   4. Because we're not sending the 'Crypto-Key' header, these lines can be ignored:

      if (requestDetails.headers['Crypto-Key']) {
          requestDetails.headers['Crypto-Key'] += ';' +
              vapidHeaders['Crypto-Key'];
      } else {
          requestDetails.headers['Crypto-Key'] = vapidHeaders['Crypto-Key'];
      }

[marco-c]

@aliams do you think you could work on a PR to implement this?

@abdulhannanali I hadn't noticed your offer, you're certainly free to do it if you want :)

I would add the supported encodings as an additional option and not store it in the subscription object itself. I think it doesn't add really much complexity to the users of the library and makes it more clear that they are separate pieces of information.

[aliams]

I put up a PR here: #309

[aliams]

@marco-c, can you publish a new version to npm with the aes128gcm support?

[marco-c]

Yes, good idea.

[marco-c]

Done, I've just released 3.3.0.