Port chromium message parser and refactor

[ibash]

This commit:

1. Adds sanity tests against chrome gcm. To use the tests copy test/keys.template.js to test/keys.js and populate it with gcm credentials / sender id. Then do npm run test

2. Ports chromium's ConnectionHandlerImpl::WaitForData. This fixes the problem of not parsing the wire format correctly. There are two more things here that would be nice to port:

   • Add timeouts while waiting for message bytes
   • Error handling around protobufs

   I did make the simplification of not supporting the kDefaultDataPacketLimit limit (it shouldn't affect correctness)

   ref: https://cs.chromium.org/chromium/src/google_apis/gcm/engine/connection_handler_impl.cc?rcl=dc7c41bc0ee5fee0ed269495dde6b8c40df43e40&l=178

3. Refactors client and socket into a class. Combining them made it easier to cleanup the socket at the end of the test. It also has the benefit of making the retrying a bit cleaner.

For the code reviewer: I'd love to have a second pair of eyes comparing the port to the original from https://cs.chromium.org/chromium/src/google_apis/gcm/engine/connection_handler_impl.cc?rcl=dc7c41bc0ee5fee0ed269495dde6b8c40df43e40&l=178. In particular - is there anything major I missed? Is there anything slightly different? Any off by one errors?

[MatthieuLemoine]

Thanks @ibash ! I will look at it this weekend

[MatthieuLemoine]

@ibash Nice work ! Also thanks for the tests. push-receiver is way more reliable now 👍 I just made a few format changes using prettier & replace mocha with jest. I also added a precommit hook to run eslint & jest.

Could you update the documentation too ? Now that client is returned by listen we can subscribe to some events like 'connect' & we can call destroy to stop listening.

[ibash]

Hey @MatthieuLemoine we're testing this internally - since I removed the try/catch around decrypt I'm seeing errors like this:

I plan on digging into it this week, but am wondering if you've seen similar ones before?

[yurynix]

When I got pushes from GCM it wasn't encrypted at all, so, decrypt was failing as well for me, I just removed it.

So, we should check if push contains encryption and crypto-key and not assume it has them.

Here's an example GCM DataMessageStanza

{
  "appData": [
    {
      "key": "expires",
      "value": "1518341280"
    },
    {
      "key": "<some app specific key>",
      "value": "<some app specific data>"
    }
  ],
  "category": "com.company.android",
  "from": "<sender id>",
  "fromTrustedServer": false,
  "id": "850A672B",
  "persistentId": "0:1518341028984788%8d385070f9fd7ecd",
  "sent": "1518341028979",
  "ttl": 2419200
}

[MatthieuLemoine]

@ibash could you check if the messages you receive that throw contains a cryptoKey? If not we could add a check

[MatthieuLemoine]

Also see #8

[ibash]

@MatthieuLemoine / @yurynix I'm adding some logging to check if that's the case for when I'm seeing the error, will let you know when I get these errors in

[ibash]

So - the crypto-key / salt definitely exist on the notifications that are failing to decrypt. I spent some time digging through how chromium encrypts/decrypts gcm messages. I didn't see anything obvious, I think my next step is to log whenever the keys change just to make sure it's not something like the notification being encrypted for older keys.

[ibash]

Putting this on the backburner on our side as it's not a hard requirement, I'll probably pick this back up later this week or mid next week

[MatthieuLemoine]

@ibash any news ?

[ibash]

@MatthieuLemoine I've been digging into this over the last week or so. So far I've confirmed that:

1. The keys are fine, I'm able to receive other notifications using the same keys.
2. Chrome's own crypto code can't decrypt the notification with the keys / cipher text (I made a unit test in chrome's code and hardcoded values that should work, and then tried the values that weren't working for me). This implies that the http_ece code is correct, too.
3. I tried decrypting the notification that fails with other keys used on my machine, and it also fails, so I don't think it's a problem with the notification being for the wrong key.
4. I did some digging through chrome's code and it looks like if they fail to decrypt a notification they just log it and drop the notifications.
5. I added some more logging to see if after getting a notification that fails to decrypt, all future notifications fail, or if the code can recover from this error. So far it looks like the code can recover, but I'm waiting to get more data to see how often decrypting fails after that.

This makes me think that it's a problem with either:

1. The ciphertext is not being parsed corrrectly (and so it fails when trying to decrypt).
2. Chrome's server is sending something that we can't decrypt.

Any thoughts / ideas?

[MatthieuLemoine]

@ibash Are you sure that you're missing a notification ? Maybe this message is not a valid notification.

[ibash]

@MatthieuLemoine I'm fairly sure - they have a category that we set. In anycase I'm deciding to silently drop it, since we still get most notifications.

[ibash]

@MatthieuLemoine / @yurynix updated the PR, WDYT?

[ibash]

I started clearing persistent ids on our fork: https://github.com/superhuman/push-receiver/pull/6/files

[yurynix]

Regarding the decryption issue, What's the steps to reproduce it?

Just run this branch and send notifications from FCM? After how many I should get an error?

[MatthieuLemoine]

@ibash Can you add the persistentIds clearing in this PR ?

[MatthieuLemoine]

@ibash Is this PR ready to be merged ?

[ibash]

@MatthieuLemoine I think your test changes made them more leaky, as they now reach into global scope. Going to change them back (as it's making the tests less reliable for me...)

[ibash]

@yurynix I haven't found a way to reproduce the encryption issue reliably, it seems random AFAICT (maybe upon initial connection? I saw it once in dev mode right after booting the app).

[ibash]

@MatthieuLemoine Ah - didn't realize jest doesn't have a context for each test, going to make it just clear credentials.

[ibash]

I think so, thank you!

Islam Sharabash 217-377-9657

Sent via Superhuman iOS ( https://sprh.mn/?vip=islam.sharabash@gmail.com )

On Thu, Mar 8 2018 at 1:02 AM, Conrad Irwin < notifications@github.com > wrote:

@MatthieuLemoine approved this pull request.

Nice work @ibash ( https://github.com/ibash ) ! Are we good to merge ?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub ( https://github.com/MatthieuLemoine/push-receiver/pull/13#pullrequestreview-102227068 ) , or mute the thread ( https://github.com/notifications/unsubscribe-auth/AAMfFnuixqiozVwxutMhTMthm4JLVcfkks5tcPOygaJpZM4R8CqG ).