Add Apple JWT Token based authentication

[sideshow]

Work in progress for the purpose of gathering feedback on the API

• Needs tests

[coveralls]

Coverage decreased (-15.6%) to 70.833% when pulling 892c8a4 on feature-auth-token into 741a20d on master.

[vivangkumar]

Nice! I think the changes are simple and easy to understand! The API hasn't changed significantly either, a new method to get a client based on Token is as simple as it can get. I can look into helping out with the tests, however, it does depend on how much time I get :(

[vivangkumar]

@sideshow Any idea when this will be merged in?

[sideshow]

Within the next week hopefully! Just adding some tests.

[xjewer]

Within the next week hopefully! Just adding some tests.

Do you have some news about tests? 😀

[coveralls]

Coverage decreased (-2.5%) to 87.118% when pulling 59dc349 on feature-auth-token into e42f05d on master.

[coveralls]

Coverage increased (+1.7%) to 91.304% when pulling 40fc7ef on feature-auth-token into e42f05d on master.

[coveralls]

Coverage increased (+1.7%) to 91.304% when pulling 7f17f3a on feature-auth-token into e42f05d on master.

[vivangkumar]

@sideshow Tests look 👌 I expect this will be merged soon, then? If you want a final review or would like me to look at anything in particular, I'm happy to help!

[sideshow]

Thanks @vivangkumar

Yeah it's close. Two things i would appreciate help with;

1. Running it in production. I have done so but would be good for others to check before we merge into master.
2. I'm not too happy with the way we check every push if the token is expired and was considering changing to catch the 403 from apple and regenerate then instead, So any input/thoughts round this would be great https://github.com/sideshow/apns2/blob/feature-auth-token/client.go#L156

And of course anything else i've missed :)

[xjewer]

I'm not too happy with the way we check every push if the token is expired and was considering changing to catch the 403 from apple and regenerate then instead, So any input/thoughts round this would be great https://github.com/sideshow/apns2/blob/feature-auth-token/client.go#L156

You also can change to catch the 403 from apple and add the autogenerator to update token before expiration. So it will call token.Generate on each time.Tick

[knousere]

We have been running in production for a year and a half using the legacy apns against multiple app targets. Not high volume but high variability. We already use dgrijalva/jwt-go for other authentication purposes. We most eagerly await your release of token based authentication. We are willing to be production test agents for you.
The connection must be fail-safe unattended. Recovery must be automatic. Anything that can be done locally should be done locally.
According to Apple documentation, a token expires after an hour.
Checking should proceed in the following order: 1. Is the token nearing expiration? (Creation time should be locally cached. Caching can be a user responsibility. Near should be a user settable parameter) 2. Catch the 403 from Apple as a safety. Apple does things for their own reasons, so sanity can not be assumed.
I always wrap connections with Apple in an object so that anything that needs to be cached is not globally accessible. I open multiple sockets per connection, and process everything asynchronously via channels so would find the 403 disruptive as it might cause redundant recovery efforts.

[sideshow]

I have run into the same issue as @nathany did in his buford library golang/go#13774

We need to find a decent enough workaround to this issue golang/go#13774 before I can merge this.

The problem is that Apple for some reason send a MAX_CONCURRENT_STREAMS=1 at the start which means they are limiting to one stream only. After you have sent one push they update the setting with MAX_CONCURRENT_STREAMS=500.

The problem is that if you send a second push notification (which opens another stream) before they have adjusted the setting, it will result in errors.

And unfortunately because the http2 transport doesn't yet respect the MAX_CONCURRENT_STREAMS we don't have a decent way to pause or lock until we are allowed to send.

I have tested this on the development and production APNs gateways and the same happens for both.

[mitkodev]

hey, just checking, what is the status of this pull request?

[nathany]

@ddimitrov90 We are blocked on not having access to the number of allowed concurrent streams sent from Apple. That's going to require some work on Go's x/net/http2 library, which will first require learning how it works. So it's a ways off 😦

"The problem is that Apple for some reason send a MAX_CONCURRENT_STREAMS=1"

The reason is to do with authentication:

"When you connect to APNs without a client certificate, the connection is essentially unauthenticated until APNs receives a push with a token authenticating the provider. This is why the connection is limited to a single stream initially." - Mayur Mahajan, Apple

It makes sense, it's just makes the API somewhat difficult to work with.

[coveralls]

Coverage increased (+1.6%) to 91.543% when pulling b2315eb on feature-auth-token into 7588e8f on master.

[nordicdyno]

appleboy/gorush#247 (comment)

@sideshow My pardon, but code that works for most cases until it's not – it sounds very dangerous for me. Personally, I think the merge of this code is a trap for newcomers and their production environments.

[luismfonseca]

So, this has been merged and is now part of the master branch golang/net@1c05540.

@sideshow, how does that affect this ticket?

[felipejfc]

@sideshow any updates here?

[sideshow]

@nordicdyno I agree with you, it would be a trap and would be painful to explain. Decided against it in the end.

@luismfonseca @felipejfc - Cool good to see progress. I'm not sure if this gets us all the way there but will try it out later this week to determine exactly what we need to do.

[sideshow]

@luismfonseca @felipejfc
Thanks, i was able to check this branch and can confirm it fixes the issue.
We are close. The only outstanding thing is that golang/net@1c05540 modifies the golang connection pool so that it blocks and doesnt dial a new connection. This is good in terms of fixing the above problem, however may have negative implications on throughput. Trying to evaluate this over the coming week before i merge.

[ehsannm]

@sideshow why you don't merge master with the Key Auth ?!

[Xzya]

Any update on this? Thanks!

[coveralls]

Coverage increased (+1.5%) to 91.667% when pulling 2cf793a on feature-auth-token into d8025ed on master.

[coveralls]

Coverage increased (+1.5%) to 91.667% when pulling 3afa5a2 on feature-auth-token into d8025ed on master.

[coveralls]

Coverage increased (+1.5%) to 91.667% when pulling 8d55eb2 on feature-auth-token into d8025ed on master.

[sideshow]

Finally merged!! Have tested this on production and development gateways and it no longer returns errors as it was before.

cc/ @vivangkumar @xjewer @ddimitrov90 @nordicdyno @luismfonseca @felipejfc @ehsannm @Xzya

[felipejfc]

Awesome @sideshow!

Thanks!

[alissonsales]

💃

[nathany]

FYI

https://go-review.googlesource.com/c/net/+/151857/ "http2: revert Transport's strict interpretation of MAX_CONCURRENT_STREAMS"

golang/go#27753