Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for Apple APNS AuthKey #101

Merged
merged 2 commits into from
Apr 18, 2023
Merged

Add support for Apple APNS AuthKey #101

merged 2 commits into from
Apr 18, 2023

Conversation

enahum
Copy link
Contributor

@enahum enahum commented Apr 14, 2022

Summary

This PR adds support for Apple AuthKey with this we will be using the same key for all Apple topics (one per-app) which means that the same key will WORK with every app we ship, also this removes the need to having to renew certificates every year. Also for customers the setup for APN is going to be much easier than the current certificate conversion to PEM.

Ticket Link

N/A

@enahum enahum added the 1: Dev Review Requires review by a core commiter label Apr 14, 2022
@github-actions
Copy link

github-actions bot commented Apr 14, 2022

Unit Test Results

  1 files  ±0    5 suites  ±0   16s ⏱️ ±0s
18 tests ±0  18 ✔️ ±0  0 💤 ±0  0 ±0 

Results for commit 5895656. ± Comparison against base commit be6dd11.

♻️ This comment has been updated with latest results.

Copy link
Member

@agnivade agnivade left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking great Elias. Just some small suggestions.

server/apple_notification_server.go Outdated Show resolved Hide resolved
server/apple_notification_server.go Outdated Show resolved Hide resolved
server/apple_notification_server.go Outdated Show resolved Hide resolved
@enahum enahum requested a review from agnivade April 15, 2022 15:14
Copy link
Member

@agnivade agnivade left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Superb work 👌

@enahum
Copy link
Contributor Author

enahum commented Apr 29, 2022

@spirosoik friendly reminder to review

Copy link
Member

@spirosoik spirosoik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

great work thanks @enahum

@enahum
Copy link
Contributor Author

enahum commented Apr 8, 2023

@spirosoik @stylianosrigas the current apple certificate will expire in the next 30 days, this PR has been sitting here for a long while now, I'll be back from PTO on the 14th of April, can we coordinate so that this PR gets merged and deployed to our HPNS and TPNS as well as start using the API keys instead of the certs. The benefit of doing that is that we won't need to deploy updated certs every year.

The one downside would be that we would need to rotate the API in case there is a security breach of some sort.

Note: The previous method of using the cert is still valid after this PR gets merged and deployed

@stylianosrigas
Copy link

stylianosrigas commented Apr 10, 2023

@enahum Sounds good to me. Once we merge and cut a release, SRE Team can work on updating the Push Proxy helm chart and deploy where needed in cloud.

@spirosoik
Copy link
Member

@enahum is security team aware for this change?

cc @iyampaul @DSchalla

@iyampaul
Copy link

@enahum is security team aware for this change?

cc @iyampaul @DSchalla

News to me, but I have no issues with this change 👍

@enahum enahum added 2: Reviews Complete All reviewers have approved the pull request and removed 1: Dev Review Requires review by a core commiter labels Apr 18, 2023
@enahum enahum merged commit 3da98c5 into master Apr 18, 2023
@enahum enahum deleted the apple-authKey branch April 18, 2023 13:33
agnivade added a commit that referenced this pull request Sep 6, 2023
With the introduction of APNS auth key (#101),
there is no need to renew certs any more. We clean it up.
agnivade added a commit that referenced this pull request Sep 8, 2023
With the introduction of APNS auth key (#101),
there is no need to renew certs any more. We clean it up.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2: Reviews Complete All reviewers have approved the pull request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants