tmolitor-stud-tu / mod_push_appserver

Simple and extendable appserver for XMPP pushes (aka. XEP-0357)
MIT License
25 stars 9 forks source link

filter required #6

Closed anurodhp closed 5 years ago

anurodhp commented 6 years ago

Having run this for a few months with actual users I have noticed that there is a need to block pushes. At the moment XMPP servers are too aggressive in what they send and in reality I just need to push out for stanzas. An ability to restrict push notifications to just a white list of stanzas would make this more usable and able to scale. Other stanzas should probably just be dropped.

tmolitor-stud-tu commented 6 years ago

The push appserver isn't able to see the stanzas that caused a particular push request. Unfortunately that's by design of XEP-0357 and nothing I can change here. There have been multiple proposals to improve this XEP, but so far the XEP maintainer didn't do anything for about a year now.

If you are willing to improve the XEP just go for it and ask if you can be the maintainer :)

anurodhp commented 6 years ago

I meant to put that on cloud notify, though we could band aid this with some better throttling

anurodhp commented 6 years ago

hmm actually talking to chris at chats secure it seems that unless there is some sort of priority on this, the push server will be flooded and the CPU load will go crazy. Apparently everyone now includes the optional last-message-body field.

Of course this isn't documented anywhere but it seems standard practice is to filter out anything where last-message-body length<=0. Is this an optional flag thing you could add? e.g. Filterlowpriority would drop anything where last-message-body length<=0

I'm looking to deploy this and at the moment the volume of junk pushes coming are already noticeable on my server and device battery use.

tmolitor-stud-tu commented 6 years ago

Well, I can add this.

But: Keep in mind that some IQ and message stanzas without a body are important too (jingle IQs for voice calls or file transfers for example). Those would not come through if you only wake up your app if a message with body is coming in. That means you essentially remove jingle filetransfer and voice call capabilities from your app.

As for the high load on your server: this could probably be due to a "bug" in prosodys mod_cloud_notify. We recently discovered that some users have a very high (about 100 or even 1000) number of push targets configured. We don't know why this happens because device ids should be stable, but for now I'm considering to add a limit of 5 push targets per user. This should lower your load significantly, once all servers are updated to this new version of mod_cloud_notify.

The appserver in this repo also has a (configurable) limit of pushes per timeframe sent to the app. You could configure a higher time limit here if you want to save battery life. The setting is named push_appserver_rate_limit and the default is one push in 5 seconds. Set this to 10 or even 15 seconds if you want.

But I doubt this the battery consumption because of pushes is very high. On my Android device Conversations is permanently connected (thus receiving every stanza) and still consumes only 1-3% of my battery.

Providing CSI functions on the xmpp server reduce pushes significantly, too. All unneeded stanzas as typing notifications etc. will be filtered out by prosody's mod_csi_battery_saver and ejabberd has a similar module, too.

anurodhp commented 6 years ago

Yeah ive seen hundreds of pushes come in in seconds. I haven't found out why. Yeah I see the jingle issue could be a problem. I think ive misunderstood how the rate limit is worked

push_appserver_rate_limit Allow this much push requests to one node per second. Default: 5. This should mitigate some DOS attacks.

I was actually reducing the number. Because I though it was # per second not number of seconds between pushes. I can try that as well. We should update the documentation if that is the case.

tmolitor-stud-tu commented 6 years ago

Well yes, it is the number of seconds...I'll try to update the doc to make that more clear.

Hundrets of pushes per second coming from one single app/user? Do you know if this user is using prosody?

anurodhp commented 6 years ago

Yes it was prosody. It was my own account I was on jabb3r.org. Right now I see that when a message arrives there appear to be about 10 devices that come in.

tmolitor-stud-tu commented 6 years ago

How much users are actually using your push enabled app? I'm trying to find the source of these huge amounts of pushes.

Are the pushes coming in in batches?

tmolitor-stud-tu commented 6 years ago

Oh well, sorry. Didn't see your last reply.

Yes, thats exactly the prosody bug I talked about. I don't know why this is happening in the first place. The device id should be constant.

Could you help me to debug this?

tmolitor-stud-tu commented 6 years ago

https://github.com/tmolitor-stud-tu/mod_push_appserver/blob/master/mod_push_appserver/mod_push_appserver.lua#L126

If a new registration for an already known device (node) is coming in, it is just updated with the new token and without touching the secret that is returned by the registration.

This is the data path:

  1. Monal gets the device id (the node) and push token and submits them to the app server through http api https://github.com/anurodhp/Monal/blob/develop/Monal/Classes/MonalAppDelegate.m#L89
  2. The appserver creates a new push target using the node as identifier (this is the device id from step 1) or updates the push token of an old entry if the node is already known https://github.com/tmolitor-stud-tu/mod_push_appserver/blob/master/mod_push_appserver/mod_push_appserver.lua#L126
  3. The appserver returns the node value unmodified alongside a new randomly generated push secret which is used by monal to register push on the xmpp server https://github.com/anurodhp/Monal/blob/develop/Monal/Classes/XMPPIQ.m#L65
  4. mod_cloud_notify on prosody sends out push notifications to the appserver using this node and secret values.

As I understand this, the device id should stay constant throughout the app lifetime and only change if you uninstall and reinstall the app again. That means: you shouldn't have 10 devices being push notified by mod_cloud_notify but only one device.

To debug this: could you investigate if all 10 "devices" are registered on the appserver and which settings they have? I need this for all 10 of them (you can anonymize the push token and/or secret if you want)

{
  type = "apns",
  token = "abc",
  last_push_error = "2017-06-07T02:31:55Z",
  last_successful_push = "2017-03-18T03:54:24Z",
  renewed = "2017-04-18T20:58:09Z",
  node = "E0FF1D8C-EB96-4E10-A912-F68B03FD8D3E",
  secret = "384e51b4b2d5e4758e5dc342b22dea9217212f2c4886e2a3dcf16f3eb0eb3807"
}

You can obtain those values if you set push_appserver_debugging to true. Afterwards go to the /v1/settings/<nodeid>/ http endpoint to get the values. /v1/settings/ lists all currently configured nodes of your server (the number of those should roughly correspond to the number of installed apps you get reported by apple).

anurodhp commented 6 years ago

hmm on my old server I have 316 devices on the new one from this morning its much less. Im going to have to hunt through my old server logs -- which I have been trying to purge for other reasons. Ill try to get this to you.

tmolitor-stud-tu commented 6 years ago

Great, thanks for helping me hunting this down! :)

tmolitor-stud-tu commented 6 years ago

316 devices of your own?? That means 316 new device ids for actually the same app/device...very strange...

anurodhp commented 6 years ago

Just found these two which had the same push token (my device) and were pushed to at the same time. It might not be what you are looking for though since the node is something the client controls so it could be something I did to produce these two. Here is the fun thing though, the node changed but the token remained the same. Even better, since both nodes are associated with the same account and share a token, this device will keep receiving two pushes here until the token changes. However, this is completely correct, there isn't an obvious way to know which device is the correct one. When I encountered something like this on other clients with APNS, the token field was unique in the db.

more 7E50E8D6%2dE80E%2d432A%2dB2F6%2d7FDF473D7E96.dat return { ["type"] = "apns"; ["last_push_error"] = "2017-12-28T04:03:55Z"; ["token"] = "token"; ["registered"] = "2017-12-28T03:38:15Z"; ["last_successful_push"] = "2018-05-22T11:20:25Z"; ["renewed"] = "2018-01-07T15:41:53Z"; ["node"] = "7E50E8D6-E80E-432A-B2F6-7FDF473D7E96"; ["secret"] = ""; }; ➜ push_appserver more 190FA133%2d8A00%2d4D76%2dBC9C%2d093C3D5FEACA.dat return { ["last_push_error"] = "2017-12-27T12:19:15Z"; ["type"] = "apns"; ["token"] = "token"; ["registered"] = "2017-12-20T23:50:40Z"; ["last_successful_push"] = "2018-05-22T11:20:24Z"; ["renewed"] = "2018-02-04T17:57:16Z"; ["node"] = "190FA133-8A00-4D76-BC9C-093C3D5FEACA"; ["secret"] = ""; };

anurodhp commented 6 years ago

316 registered devices. I dont know who's they are. there are probably about 25-30 testers.

tmolitor-stud-tu commented 6 years ago

Well, very interesting. Both seem outdated, because the renewed date is very old. Every time your app is started it registers itself for push notifications at the app server and receives the push secret back. This reregistration (using an already known device id) is detected by the appserver and the datetime value of the last reregistration is saved into renewed.

The first one has a renewed of 2018-01-07T15:41:53Z and the second one 2018-02-04T17:57:16Z (which is newer). Interestingly the first one got created later than the second one (I would have expected the opposite since the second one got used longer).

Can you try to find out why and when the device id changes? App updates or iOS updates or such.

Maybe it helps to find the currently used device id (none of the above two seems to be the one currently in use).

tmolitor-stud-tu commented 6 years ago

@anurodhp Could you try to use a self generated device id? If the iOS provided one isn't stable this should fix the issue.

I would randomly generate a uuid on first app start and save that into the database for future use.

tmolitor-stud-tu commented 6 years ago

@anurodhp I added a new "auto" priority in my last commit a few minutes ago. But as I said: this will destroy all jingle functionality inclusive VoIP support as long as the app is not foregrounded.

tmolitor-stud-tu commented 6 years ago

@anurodhp anything new regarding this?

tmolitor-stud-tu commented 5 years ago

I'm closing this now, feel free to reopen it if needed.