From 46e44e754aa299a97e4d51aa8762a3423255080f Mon Sep 17 00:00:00 2001
From: Filip Skokan <panva.ip@gmail.com>
Date: Wed, 13 Oct 2021 16:10:25 +0200
Subject: [PATCH] fix: do not implicitly calculate key ids for Client instances

closes #379
---
 lib/client.js                    | 19 +++++++-
 lib/helpers/client.js            |  2 +-
 test/client/implicit_kid.test.js | 76 ++++++++++++++++++++++++++++++++
 3 files changed, 95 insertions(+), 2 deletions(-)
 create mode 100644 test/client/implicit_kid.test.js

diff --git a/lib/client.js b/lib/client.js
index 487633c1..43c7524f 100644
--- a/lib/client.js
+++ b/lib/client.js
@@ -125,6 +125,17 @@ async function claimJWT(label, jwt) {
 }
 
 function getKeystore(jwks) {
+  if (!isPlainObject(jwks) || !Array.isArray(jwks.keys) || jwks.keys.some((k) => !isPlainObject(k) || !('kty' in k))) {
+    throw new TypeError('jwks must be a JSON Web Key Set formatted object');
+  }
+
+  // eslint-disable-next-line no-restricted-syntax
+  for (const jwk of jwks.keys) {
+    if (jwk.kid === undefined) {
+      jwk.kid = `DONOTUSE.${random()}`;
+    }
+  }
+
   const keystore = jose.JWKS.asKeyStore(jwks);
   if (keystore.all().some((key) => key.type !== 'private')) {
     throw new TypeError('jwks must only contain private keys');
@@ -1426,6 +1437,12 @@ module.exports = (issuer, aadIssValidation = false) => class Client extends Base
     if (jwks !== undefined && !(metadata.jwks || metadata.jwks_uri)) {
       const keystore = getKeystore.call(this, jwks);
       metadata.jwks = keystore.toJWKS(false);
+      // eslint-disable-next-line no-restricted-syntax
+      for (const jwk of metadata.jwks.keys) {
+        if (jwk.kid.startsWith('DONOTUSE.')) {
+          delete jwk.kid;
+        }
+      }
     }
 
     const response = await request.call(this, {
@@ -1525,7 +1542,7 @@ module.exports = (issuer, aadIssValidation = false) => class Client extends Base
 
       signed = jose.JWS.sign(payload, key, {
         ...header,
-        kid: symmetric ? undefined : key.kid,
+        kid: symmetric || key.kid.startsWith('DONOTUSE.') ? undefined : key.kid,
       });
     }
 
diff --git a/lib/helpers/client.js b/lib/helpers/client.js
index 82c37e2b..a9ebd135 100644
--- a/lib/helpers/client.js
+++ b/lib/helpers/client.js
@@ -47,7 +47,7 @@ async function clientAssertion(endpoint, payload) {
   if (!key) {
     throw new TypeError(`no key found in client jwks to sign a client assertion with using alg ${alg}`);
   }
-  return jose.JWS.sign(payload, key, { alg, typ: 'JWT', kid: key.kid });
+  return jose.JWS.sign(payload, key, { alg, typ: 'JWT', kid: key.kid.startsWith('DONOTUSE.') ? undefined : key.kid });
 }
 
 async function authFor(endpoint, { clientAssertionPayload } = {}) {
diff --git a/test/client/implicit_kid.test.js b/test/client/implicit_kid.test.js
new file mode 100644
index 00000000..273a360b
--- /dev/null
+++ b/test/client/implicit_kid.test.js
@@ -0,0 +1,76 @@
+const { expect } = require('chai');
+const jose = require('jose');
+const nock = require('nock');
+
+const Issuer = require('../../lib/issuer');
+const clientInternal = require('../../lib/helpers/client');
+
+async function noKidJWKS() {
+  const store = new jose.JWKS.KeyStore();
+  await store.generate('EC');
+  const jwks = store.toJWKS(true);
+  delete jwks.keys[0].kid;
+  expect(jwks.keys[0].kid).to.be.undefined;
+  return jwks;
+}
+
+describe('no implicit Key IDs (kid)', function () {
+  afterEach(nock.cleanAll);
+
+  it('is not added to client assertions', async () => {
+    const issuer = new Issuer();
+    const jwks = await noKidJWKS();
+    const client = new issuer.Client({
+      client_id: 'identifier',
+      token_endpoint_auth_method: 'private_key_jwt',
+      token_endpoint_auth_signing_alg: 'ES256',
+    }, jwks);
+
+    const { form: { client_assertion: jwt } } = await clientInternal.authFor.call(client, 'token');
+
+    const header = JSON.parse(Buffer.from(jwt.split('.')[0], 'base64'));
+    expect(header).to.have.property('alg', 'ES256');
+    expect(header).not.to.have.property('kid');
+  });
+
+  it('is not added to request objects', async () => {
+    const issuer = new Issuer();
+    const jwks = await noKidJWKS();
+    const client = new issuer.Client({
+      client_id: 'identifier',
+      request_object_signing_alg: 'ES256',
+    }, jwks);
+
+    const jwt = await client.requestObject();
+
+    const header = JSON.parse(Buffer.from(jwt.split('.')[0], 'base64'));
+    expect(header).to.have.property('alg', 'ES256');
+    expect(header).not.to.have.property('kid');
+  });
+
+  it('is not added to dynamic registration requests', async () => {
+    const issuer = new Issuer({
+      registration_endpoint: 'https://op.example.com/client/registration',
+    });
+
+    nock('https://op.example.com')
+      .filteringRequestBody(function (body) {
+        const payload = JSON.parse(body);
+        expect(payload).to.have.nested.property('jwks.keys').that.is.an('array');
+        expect(payload.jwks.keys[0]).to.be.an('object').with.property('kty');
+        expect(payload.jwks.keys[0]).not.to.have.property('kid');
+      })
+      .post('/client/registration', () => true) // to make sure filteringRequestBody works
+      .reply(201, {
+        client_id: 'identifier',
+        token_endpoint_auth_method: 'private_key_jwt',
+        jwks: {}, // ignore
+      });
+
+    await issuer.Client.register({
+      token_endpoint_auth_method: 'private_key_jwt',
+    }, { jwks: await noKidJWKS() });
+
+    expect(nock.isDone()).to.be.true;
+  });
+});