Techofficer / node-apple-signin

Node.JS wrapper around Sign In with Apple REST API
MIT License
53 stars 40 forks source link

Add support for multiple apple public keys #8

Open tomislavherman opened 4 years ago

tomislavherman commented 4 years ago

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.

alexabidri commented 4 years ago

Can we move the discussion in https://github.com/Techofficer/node-apple-signin/pull/9 ?

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

tomislavherman commented 4 years ago

@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 commented 4 years ago

@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 commented 4 years ago

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?