Implement VAPID authorization #26

rossta · 2016-10-07T06:17:34Z

This PR is ~~a work-in-progress and not ready to merge~~ is ready for review. It is a fairly large set of changes internally that should be backwards compatible with webpush using GCM API key, while providing support for VAPID, now available in FF and Chrome, as the way moving forward for server identification.

This is a rather large PR. I'm happy to take suggestions to break it down into smaller parts.

Fixes #25 Fixes #13

Usage

The server must store public and private VAPID keys. To generate (one-time):

# Save these values in application settings, such as 
# ENV['VAPID_PUBLIC_KEY'] and ENV['VAPID_PRIVATE_KEY']
vapid_key = Webpush.generate_key
vapid_key.public_key  # => A base64-encoded representation of a 65-byte binary key
vapid_key.private_key # => A base64-encoded representation of a 32-byte binary key

The client needs the public key as a JavaScript Uint8Array initialized with the decoded bytes.

window.vapidPublicKey = new Uint8Array(<%= Base64.urlsafe_decode64(ENV['VAPID_PUBLIC_KEY']).bytes %>);

Provide the public key with the pushManager.subscribe options.

navigator.serviceWorker.ready.then((serviceWorkerRegistration) => {
   serviceWorkerRegistration.pushManager
   .subscribe({
     userVisibleOnly: true,
     applicationServerKey: window.vapidPublicKey
   });
 });

Provide VAPID details when sending a push notification from the backend along with the payload and subscription settings.

Webpush.payload_send(
  message: params[:message]
  endpoint: params[:subscription][:endpoint],
  p256dh: params[:subscription][:keys][:p256dh],
  auth: params[:subscription][:keys][:p256dh],
  vapid: {
    subject: "mailto:[email protected]",
    public_key: ENV['VAPID_PUBLIC_KEY'],
    private_key: ENV['VAPID_PRIVATE_KEY']
  }
)

Notes

I have rewritten the Webpush::Request to set 'Authorization and Crypto-Key headers using the VAPID protocol. I can successfully send push requests with VAPID through Firefox and Chrome.

~~In Chrome, I frequently get a #<Net::HTTPBadRequest 400 UnauthorizedRegistration readbody=true> response. I haven't found the root cause but would be interested in help to troubleshoot.~~

Checklist

get working in FFNightly, FF
get working (consistently) in Chrome Canary, Chrome
Retain GCM api key 'Authorization' for backwards compatibility
add tests for the Vapid module
add README usage instructions for VAPID

comp615

Tried to help by looking at a few issues I ran into implementing similar functionality.

One other note is that if these changes are taken, the README and example should be updated to reflect the need to .subscribe({}) with the server's public key and to pass the same public/private key into the VAPID stuff added here

comp615 · 2016-10-09T22:36:25Z

lib/webpush/request.rb

-        headers["Encryption"] = "salt=#{salt_param}"
-        headers["Crypto-Key"] = "dh=#{dh_param}"
+        headers["Encryption"] = "keyid=p256dh;salt=#{salt_param}"
+        headers["Crypto-Key"] = "keyid=p256dh;dh=#{dh_param}"


keyid is no longer needed here

Thanks, removed!

comp615 · 2016-10-09T22:37:06Z

lib/webpush/request.rb

+      headers["Crypto-Key"] = [
+        headers["Crypto-Key"],
+        vapid_headers["Crypto-Key"]
+      ].compact.join(";")


Word is the ';' join was a bug in Chrome 52, should be using ',' here now I think :-?

comp615 · 2016-10-09T22:43:07Z

lib/webpush/request.rb

@@ -36,17 +44,26 @@ def headers
      headers["Content-Type"] = "application/octet-stream"
      headers["Ttl"]          = ttl

-      if encrypted_payload?
+      if @payload.has_key?(:server_public_key)
        headers["Content-Encoding"] = "aesgcm"


https://tools.ietf.org/html/draft-ietf-webpush-encryption-03 says "aesgcm128"

Thanks for checking, made the change

comp615 · 2016-10-09T22:51:49Z

lib/webpush/request.rb

      end

-      headers["Authorization"] = "key=#{api_key}" if api_key?
+      vapid_headers = build_vapid_headers
+      headers["Authorization"] = vapid_headers["Authorization"]


I think this still needs the previous case matching statement, or something to the effect of:

if (@endpoint =~ /\Ahttps:\/\/(android|gcm-http)\.googleapis\.com/) { vapid_headers = api_key? ? {"Authorization": "key=#{api_key}" } : {} } else { vapid_headers = build_vapid_headers }

Reason being that for backwards compatibility, people may still have old tokens saved. Basically iff the subscription is created with a VAPID public key in JS on the client, an FCM endpoint will be returned. Otherwise, you still get the old style and wouldn't be able to perform VAPID with it.

Good point. It was perhaps aggressive of me to remove this functionality so I've added an item to my checklist to reinstate.

Set 'Content-Encoding': 'aesgcm128' and remove 'keyid=p256dh' from 'Encryption' and 'Crypto-Key' headers

…eader

rossta · 2016-10-10T04:12:30Z

Quick update: I now have the VAPID implementation working in Chrome. I had to fix the audience value in the JWT payload and my own manifest.json settings for the website I've been using to test.

The audience value of the VAPID JWT payload is now automatically set to the push service host from the subscription endpoint. I also learned it (apparently) is still necessary to declare in the manifest.json the gcm_sender_id, which you get with a registered FCM app on the Google developer console.

gazay · 2016-10-10T18:01:46Z

@rossta can you please help me. I'm not sure that I'm trying your branch right. Because when I try to push to new subscription from FF 49.0.1 I receive 400:

Webpush::ResponseError: host: updates.push.services.mozilla.com, #<Net::HTTPBadRequest 400 Bad Request readbody=true>
body:
{"errno": 110, "message": "Request did not validate You're using outdated encryption; Please update to the format described in https://developers.google.com/web/updates/2016/03/web-push-encryption", "code": 400, "more_info": "http://autopush.readthedocs.io/en/latest/http.html#error-codes", "error": "Bad Request"}

The code I use is the same for Chrome (which is works) and for FF subscriptions:

    Webpush.payload_send(
      endpoint: @subscription.endpoint,
      message: payload,
      p256dh: @subscription.p256dh,
      auth: @subscription.auth,
      vapid: {
        public_key: Settings.vapid_keys.p256dh,
        private_key: Settings.vapid_keys.auth,
        audience: @subscription.endpoint.split('/')[0..2].join('/'),
        subject: 'mailto:[email protected]'
      }
    )

Extensive setup tutorial for using webpush with VAPID

rossta · 2016-10-11T00:40:18Z

@gazay Thanks for alerting me. I was able to reproduce in FF as well while it works in Chrome. Looking into it.

rossta · 2016-10-11T00:48:38Z

@gazay Looks FF still expects the 'Content-Encoding' to be 'aesgcm' despite the spec change noted by @comp615. I was able to send messages in both Chrome and FF with this change reverted in e4f5b1e.

Let me know if you have a chance to re-test. Thanks!

comp615 · 2016-10-11T00:57:58Z

@rossta I'll reach out to FF and see what the deal is with the content encoding header and try to get a more definitive answer.

rossta · 2016-10-11T04:27:52Z

Update: This PR is now feature-complete and ready for closer review. I updated the description with more details about the changes and usage.

comp615 · 2016-10-11T16:41:07Z

@rossta when you were doing aesgcm128, which version of FF were you using? They said it should work, but may not in 46 or lower since the way encryption was done was different. I'm getting the sense that's the case for a lot of this (the ; vs ,'s, legacy GCM, etc.) so maybe we should just suggest that people do browser detection for appropriate versions of Chrome and FF?

rossta · 2016-10-11T20:15:33Z

@comp615 Thanks for looking into it. I've tried in Firefox 49.0.1 and FirefoxNightly 52.0a1 with aesgcm128 and I get the same error message noted by @gazay above each time I attempt to send a push request with a valid subscription. I've tried a number of tacts to troubleshoot including resubscribing, generating new app keys, resetting Notification permissions, running Firefox in different profiles... all the same result. Both FF and FFNightly work when I make no other changes besides aesgcm.

I haven't ruled out the possibility that perhaps something else might be incorrect about our encryption and encoding logic, but I haven't come across anything yet, other the changes we've already noted in this thread. I can continue trying to debug, but the response messages from both Chrome and FF don't always help us determine the root cause.

comp615 · 2016-10-11T20:33:03Z

I'm a dummy. Apparently the newer spec version is aesgcm, so that's definitely the correct way how you have it now. Sorry for the runaround.

rossta · 2016-10-11T21:58:45Z

Ha, nice catch! I also missed that it wasn't the most recent spec. Glad you
double-checked.

On Tuesday, October 11, 2016, Charlie Croom [email protected]
wrote:

I'm a dummy. Apparently the newer spec version is aesgcm, so that's
definitely the correct way how you have it now. Sorry for the runaround.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#26 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAAtmQBHFY1HJnMdCOom8QDX1nvLRXyfks5qy_KAgaJpZM4KQtXi
.

Ross Kaffenberger
@rossta

zaru · 2016-10-13T11:49:26Z

lib/webpush/request.rb

        headers["Content-Encoding"] = "aesgcm"
        headers["Encryption"] = "salt=#{salt_param}"
        headers["Crypto-Key"] = "dh=#{dh_param}"
      end

-      headers["Authorization"] = "key=#{api_key}" if api_key?
+      if api_key?


@rossta Thank you for your great PR! 😀
I think that it cannot be sent to the FF endpoint is not a VAPID.
I think that judging by the vapid options.
What do you think?

Hi @zaru - thanks!

I'm not sure I understand your question completely so please correct me if my answer is misdirected:

I believe one thing the PR is missing is to account for the fact that VAPID is optional so I'll need to add logic to allow the request to succeed if :vapid_options and :api_key are missing from the request as it should still be possible to send the notification. I've added that along with README updates as of de87c4c.

@rossta Thanks!
I tested your PR and it looks fine.
So I'm gonna merge them if you've got nothing else to check. Is that okay?

@zaru Yes, thank you! I'm sure there's a lot to improve on here, especially with request error handling, but I hope the community will find this changeset useful as a starting point with VAPID.

rossta added 5 commits October 6, 2016 17:49

Remove google api_key references

289ecb6

Working implementation of vapid webpush in FF

e1c326f

WIP refactor request

d664790

Require vapid options to send notification

53a54cc

WIP refactor request

0961b6f

rossta mentioned this pull request Oct 7, 2016

WebPush, 0.2.5, Webpush::InvalidSubscription (#<Net::HTTPBadRequest 400 UnauthorizedRegistration readbody=true>): #25

Closed

comp615 suggested changes Oct 9, 2016

View reviewed changes

rossta added 5 commits October 9, 2016 23:20

Use Urlsafe module in Request class

6375293

Vapid audience should represent push host

0071838

Update to request headers per spec

5f652f8

Set 'Content-Encoding': 'aesgcm128' and remove 'keyid=p256dh' from 'Encryption' and 'Crypto-Key' headers

Update specs to address header fixes for VAPID

9b1a78e

Add GCM api key specs and functionality for setting 'Authorization' h…

50bbd01

…eader

rossta changed the title ~~Switch to VAPID authorization~~ Implement VAPID authorization Oct 10, 2016

Add README introduction for VAPID and webpush setup

9bce0ff

Update README.md

be4fc16

Extensive setup tutorial for using webpush with VAPID

Switch back to aesgcm Content-Encoding

e4f5b1e

rossta added 2 commits October 11, 2016 00:05

Update README with edits and additional information on VAPID

c802fe7

Add specs for VAPID headers and key generation with refactoring

2b9ceef

rossta added 2 commits October 11, 2016 08:41

Edit README

b89112a

Add convenience methods for inspecting vapid key

d4d5a6b

rossta force-pushed the vapid branch from 410e879 to d4d5a6b Compare October 11, 2016 12:57

zaru reviewed Oct 13, 2016

View reviewed changes

rossta added 2 commits October 13, 2016 23:39

Allow for VAPID details to be optional

fd4f283

Add README notes about making VAPID optional

de87c4c

zaru merged commit 15b68f5 into zaru:master Oct 14, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement VAPID authorization #26

Implement VAPID authorization #26

rossta commented Oct 7, 2016 •

edited

Loading

comp615 left a comment

comp615 Oct 9, 2016

rossta Oct 10, 2016

comp615 Oct 9, 2016

comp615 Oct 9, 2016

rossta Oct 10, 2016

comp615 Oct 9, 2016 •

edited

Loading

rossta Oct 10, 2016

rossta commented Oct 10, 2016

gazay commented Oct 10, 2016 •

edited

Loading

rossta commented Oct 11, 2016

rossta commented Oct 11, 2016

comp615 commented Oct 11, 2016

rossta commented Oct 11, 2016

comp615 commented Oct 11, 2016

rossta commented Oct 11, 2016

comp615 commented Oct 11, 2016

rossta commented Oct 11, 2016

zaru Oct 13, 2016

rossta Oct 14, 2016 •

edited

Loading

zaru Oct 14, 2016

rossta Oct 14, 2016

Implement VAPID authorization #26

Implement VAPID authorization #26

Conversation

rossta commented Oct 7, 2016 • edited Loading

Usage

Notes

Checklist

comp615 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

comp615 Oct 9, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rossta commented Oct 10, 2016

gazay commented Oct 10, 2016 • edited Loading

rossta commented Oct 11, 2016

rossta commented Oct 11, 2016

comp615 commented Oct 11, 2016

rossta commented Oct 11, 2016

comp615 commented Oct 11, 2016

rossta commented Oct 11, 2016

comp615 commented Oct 11, 2016

rossta commented Oct 11, 2016

Choose a reason for hiding this comment

rossta Oct 14, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rossta commented Oct 7, 2016 •

edited

Loading

comp615 Oct 9, 2016 •

edited

Loading

gazay commented Oct 10, 2016 •

edited

Loading

rossta Oct 14, 2016 •

edited

Loading