Merge pull request #788 from MatrixAI/feature-eng-390-expired-interme…

…diate-certificates-prevents-maintaining Adding handling for expired intermediate certs when verifying client
MatrixAI · Aug 16, 2024 · 5b90f80 · 5b90f80
2 parents 76b4cf2 + df02cca
commit 5b90f80
Show file tree

Hide file tree

Showing 4 changed files with 80 additions and 10 deletions.
diff --git a/src/nodes/utils.ts b/src/nodes/utils.ts
@@ -647,10 +647,11 @@ async function verifyClientCertificateChain(
     certChain.push(cert);
   }
   const now = new Date();
+  let leafCert = true;
   for (let certIndex = 0; certIndex < certChain.length; certIndex++) {
     const cert = certChain[certIndex];
     const certNext = certChain[certIndex + 1];
-    if (now < cert.notBefore || now > cert.notAfter) {
+    if (leafCert && (now < cert.notBefore || now > cert.notAfter)) {
       return CryptoError.CertificateExpired;
     }
     const certNodeId = keysUtils.certNodeId(cert);
@@ -675,6 +676,7 @@ async function verifyClientCertificateChain(
         return CryptoError.BadCertificate;
       }
     }
+    leafCert = false;
   }
   // Undefined means success
   return undefined;

diff --git a/tests/client/utils.test.ts b/tests/client/utils.test.ts
@@ -21,9 +21,9 @@ describe('client/utils', () => {
 
   beforeAll(async () => {
     cert = await testTlsUtils.createTLSConfigWithChain([
-      keyPairRoot,
-      keyPairIntermediate,
-      keyPairLeaf,
+      [keyPairRoot, undefined],
+      [keyPairIntermediate, undefined],
+      [keyPairLeaf, undefined],
     ]);
   });
 

diff --git a/tests/nodes/utils.test.ts b/tests/nodes/utils.test.ts
@@ -9,6 +9,7 @@ import { IdInternal } from '@matrixai/id';
 import { DB } from '@matrixai/db';
 import { errors as rpcErrors } from '@matrixai/rpc';
 import { utils as wsUtils } from '@matrixai/ws';
+import { CryptoError } from '@matrixai/quic/dist/native';
 import * as nodesUtils from '@/nodes/utils';
 import * as keysUtils from '@/keys/utils';
 import * as validationErrors from '@/validation/errors';
@@ -334,9 +335,9 @@ describe('nodes/utils', () => {
 
     beforeAll(async () => {
       cert = await testTlsUtils.createTLSConfigWithChain([
-        keyPairRoot,
-        keyPairIntermediate,
-        keyPairLeaf,
+        [keyPairRoot, undefined],
+        [keyPairIntermediate, undefined],
+        [keyPairLeaf, undefined],
       ]);
     });
 
@@ -384,6 +385,51 @@ describe('nodes/utils', () => {
         if (result2.result === 'fail') fail();
         expect(Buffer.compare(result2.nodeId, nodeIdLeaf)).toBe(0);
       });
+      test('verify on leaf cert with expired intermediate certs', async () => {
+        cert = await testTlsUtils.createTLSConfigWithChain([
+          [keyPairRoot, 0],
+          [keyPairIntermediate, 0],
+          [keyPairIntermediate, 0],
+          [keyPairLeaf, undefined],
+        ]);
+        const result = await nodesUtils.verifyServerCertificateChain(
+          [nodeIdLeaf],
+          cert.certChainPem.map((v) => wsUtils.pemToDER(v)),
+        );
+        expect(result.result).toBe('success');
+        if (result.result === 'fail') fail();
+        expect(Buffer.compare(result.nodeId, nodeIdLeaf)).toBe(0);
+      });
+      test('verify on intermediate cert with expired intermediate certs', async () => {
+        cert = await testTlsUtils.createTLSConfigWithChain([
+          [keyPairRoot, 0],
+          [keyPairIntermediate, 0],
+          [keyPairIntermediate, undefined],
+          [keyPairLeaf, undefined],
+        ]);
+        const result = await nodesUtils.verifyServerCertificateChain(
+          [nodeIdIntermediate],
+          cert.certChainPem.map((v) => wsUtils.pemToDER(v)),
+        );
+        expect(result.result).toBe('success');
+        if (result.result === 'fail') fail();
+        expect(Buffer.compare(result.nodeId, nodeIdIntermediate)).toBe(0);
+      });
+      test('fails with expired intermediate before valid target', async () => {
+        cert = await testTlsUtils.createTLSConfigWithChain([
+          [keyPairRoot, 0],
+          [keyPairIntermediate, undefined],
+          [keyPairLeaf, 0],
+          [keyPairLeaf, undefined],
+        ]);
+        const result = await nodesUtils.verifyServerCertificateChain(
+          [nodeIdIntermediate],
+          cert.certChainPem.map((v) => wsUtils.pemToDER(v)),
+        );
+        expect(result.result).toBe('fail');
+        if (result.result !== 'fail') utils.never();
+        expect(result.value).toBe(CryptoError.CertificateExpired);
+      });
     });
     describe('server verifyClientCertificateChain', () => {
       test('verify with multiple certs', async () => {
@@ -392,6 +438,28 @@ describe('nodes/utils', () => {
         );
         expect(result).toBeUndefined();
       });
+      test('verify with expired intermediate certs', async () => {
+        cert = await testTlsUtils.createTLSConfigWithChain([
+          [keyPairRoot, 0],
+          [keyPairIntermediate, 0],
+          [keyPairLeaf, undefined],
+        ]);
+        const result = await nodesUtils.verifyClientCertificateChain(
+          cert.certChainPem.map((v) => wsUtils.pemToDER(v)),
+        );
+        expect(result).toBeUndefined();
+      });
+      test('fails with expired leaf cert', async () => {
+        cert = await testTlsUtils.createTLSConfigWithChain([
+          [keyPairRoot, undefined],
+          [keyPairIntermediate, undefined],
+          [keyPairLeaf, 0],
+        ]);
+        const result = await nodesUtils.verifyClientCertificateChain(
+          cert.certChainPem.map((v) => wsUtils.pemToDER(v)),
+        );
+        expect(result).toBe(CryptoError.CertificateExpired);
+      });
     });
   });
 });
diff --git a/tests/utils/tls.ts b/tests/utils/tls.ts
@@ -32,7 +32,7 @@ async function createTLSConfig(
 }
 
 async function createTLSConfigWithChain(
-  keyPairs: Array<KeyPair>,
+  keyPairs: Array<[KeyPair, number | undefined]>,
   generateCertId?: () => CertId,
 ): Promise<{
   keyPrivatePem: PrivateKeyPEM;
@@ -43,10 +43,10 @@ async function createTLSConfigWithChain(
   let previousCert: Certificate | null = null;
   let previousKeyPair: KeyPair | null = null;
   const certChain: Array<Certificate> = [];
-  for (const keyPair of keyPairs) {
+  for (const [keyPair, duration] of keyPairs) {
     const newCert = await keysUtils.generateCertificate({
       certId: generateCertId(),
-      duration: 31536000,
+      duration: duration ?? 31536000,
       issuerPrivateKey: previousKeyPair?.privateKey ?? keyPair.privateKey,
       subjectKeyPair: keyPair,
       issuerAttrsExtra: previousCert?.subjectName.toJSON(),