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 multiple apple public keys #8

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

tomislavherman
Copy link

Currently we use only first public key provided by apple to verify ID
token. It seems that apple issues ID tokens with different keys (3 in
this moment). This fix uses header.kid field from ID token in order to
identify which public key will be used to verify ID token.

Currently we use only first public key provided by apple to verify ID
token. It seems that apple issues ID tokens with different keys (3 in
this moment). This fix uses `header.kid` field from ID token in order to
identify which public key will be used to verify ID token.
source/index.js Outdated
Comment on lines 97 to 98
const data = await request({ url: url.toString(), method: 'GET' });
const key = JSON.parse(data).keys[0];
const key = JSON.parse(data).keys.find(key => key.kid === kid);

Choose a reason for hiding this comment

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

request-promise-native accepts a json option to automatically parse the response as json.

Can be written as the following for a small gain in readability

const keys = await request({ url: url.toString(), method: 'GET', json: true });
const key = keys.find(k => k.kid === kid);

Copy link
Author

Choose a reason for hiding this comment

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

Nice catch! Pull request is updated.

@alexabidri
Copy link

alexabidri commented Feb 15, 2020

Can we move the discussion in #9 ?

IMO, it would be good to move to jwks-rsa and use jwks properly for the sake of clarity and simplicity

@tomislavherman
Copy link
Author

@alexabidri I applied your suggestion to this PR. I hope this is ok.
Instead of jwks-rsa I used jwks-rsa-promisified which exposes promisified method versions of jwks client so I don't have to deal with callbacks inside async verifyIdToken(...).

@alexabidri
Copy link

alexabidri commented Feb 19, 2020

@tomislavherman Good job!

Personnaly I would prefer to use jwks-rsa because jwks-rsa-promisified is not maintained a lot and we dont take advantage of recurring new releases of jwks-rsa.

As apple-signin is a package aim to be used by some startup/corporations, I would avoid using a non maintained module which doesnt provide big improvements except to promisify a already existing module.

Some hours ago, jwks-rsa got a new release (1.7.0)
This release includes a change to the default caching mechanism. Caching is on now by default, with the decrease of the default time of 10hours to 10minutes. This change introduces better support for signing key rotation.

Here is a simple wrapper to promisified jwks-rsa

// eslint-disable-next-line
const jwksClient = require('jwks-rsa');

function getApplePublicKey(kid) {
  const client = jwksClient({
    jwksUri: 'https://appleid.apple.com/auth/keys',
    cache: true,
  });

  return new Promise((resolve, reject) => {
    // eslint-disable-next-line
    client.getSigningKey(kid, (err, key) => {
      if (err) {
        reject(err);
      }
      resolve(key);
    });
  });
}

const key = await getApplePublicKey(kid);

@tomislavherman
Copy link
Author

Thanks, @alexabidri. I agree with you regarding the jwks-rsa package.

I would just instantiate jwks client in the scope of the module instead of in the scope of getApplePublicKey function. Otherwise, each call to getApplePublicKey would create a new client, and we would lose the caching capability provided by client. Do you agree?

Create wrapper around client which promisifies getSigningKey on jwks client
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.

3 participants