Skip to content

fix apple auth adapter to verify using the correct public key for token #6410

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

Closed
wants to merge 3 commits into from

Conversation

andrewking0207
Copy link
Contributor

@andrewking0207 andrewking0207 commented Feb 14, 2020

Fixes: #6408

The appleid auth token endpoint https://appleid.apple.com/auth/keys returns three different keys. The current apple auth adapter implementation always selects the first key to verify the token. This works often but if the client did not encode using the first key then the verification will fail. This fix checks the header of the token to get the key ID used for the encoding and then selects the correct public key with which to perform verification of the token.

@andrewking0207
Copy link
Contributor Author

looks like I was running the tests wrong locally. I'll fix these up

@codecov
Copy link

codecov bot commented Feb 15, 2020

Codecov Report

Merging #6410 into master will decrease coverage by 0.14%.
The diff coverage is 60%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #6410      +/-   ##
==========================================
- Coverage   93.94%   93.79%   -0.15%     
==========================================
  Files         169      169              
  Lines       11734    11736       +2     
==========================================
- Hits        11023    11008      -15     
- Misses        711      728      +17
Impacted Files Coverage Δ
src/Adapters/Auth/apple.js 42.42% <60%> (-44.68%) ⬇️
src/Adapters/Storage/Mongo/MongoStorageAdapter.js 92.79% <0%> (-0.68%) ⬇️
src/RestWrite.js 93.81% <0%> (+0.16%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fd0b535...6057c41. Read the comment docs.

@dplewis
Copy link
Member

dplewis commented Feb 15, 2020

Nice catch! Let me run this locally.


const decodedToken = jwt.decode(token, { complete: true });
const keyId = decodedToken.header.kid;
const applePublicKey = await getApplePublicKey(keyId);
const jwtClaims = jwt.verify(token, applePublicKey, { algorithms: 'RS256' });

Choose a reason for hiding this comment

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

We should probably parse the "alg" key from the JWT dict for the used algorithm and pass it in here, to avoid hardcoding RS256 in case Apple changes this at some point

Copy link
Member

Choose a reason for hiding this comment

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

Nice catch! We will open a new PR with these changes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll get the new PR submitted today, sorry I broke the source branch for this one working on a different thing.

Copy link
Member

Choose a reason for hiding this comment

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

There is an existing issue #6394. I'll get that in shortly so you won't have merge conflicts.

@tealshift
Copy link

Hi, I'm just chiming in here to say I'm also experiencing random auth failures on my server through Back4app, so thank you for submitting your patch! It would be amazing to get this merged so back4app can easily update my backend. 👍
Let me know if there's anything I can do to help.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Sign in With Apple token verification fails on occassion
4 participants