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.

Sign up for GitHub

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

Jump to bottom

Adding handling for expired intermediate certs when verifying client #788

Merged
merged 2 commits into from
Aug 16, 2024
Merged

Adding handling for expired intermediate certs when verifying client #788

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
09ecd40
fix: fixed problem with `verifyClientCertificateChain` not handling e…
tegefaulkes Aug 15, 2024
df02cca
tests: added test checking if certificate before target node is expired
tegefaulkes Aug 16, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion src/nodes/utils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)) {
Comment on lines +650 to +654
Copy link
Member

Choose a reason for hiding this comment

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

This is a bit confusing when it's set to true and then checked. The order of switching booleans may be clearer if it was the other way around.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As in, started as false and had a different name?

return CryptoError.CertificateExpired;
}
const certNodeId = keysUtils.certNodeId(cert);
Expand All @@ -675,6 +676,7 @@ async function verifyClientCertificateChain(
return CryptoError.BadCertificate;
}
}
leafCert = false;
}
// Undefined means success
return undefined;
Expand Down
6 changes: 3 additions & 3 deletions tests/client/utils.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,9 +21,9 @@ describe('client/utils', () => {

beforeAll(async () => {
cert = await testTlsUtils.createTLSConfigWithChain([
keyPairRoot,
keyPairIntermediate,
keyPairLeaf,
[keyPairRoot, undefined],
[keyPairIntermediate, undefined],
[keyPairLeaf, undefined],
]);
});

Expand Down
74 changes: 71 additions & 3 deletions tests/nodes/utils.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -334,9 +335,9 @@ describe('nodes/utils', () => {

beforeAll(async () => {
cert = await testTlsUtils.createTLSConfigWithChain([
keyPairRoot,
keyPairIntermediate,
keyPairLeaf,
[keyPairRoot, undefined],
[keyPairIntermediate, undefined],
[keyPairLeaf, undefined],
]);
});

Expand Down Expand Up @@ -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 () => {
Expand All @@ -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);
});
});
});
});
6 changes: 3 additions & 3 deletions tests/utils/tls.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ async function createTLSConfig(
}

async function createTLSConfigWithChain(
keyPairs: Array<KeyPair>,
keyPairs: Array<[KeyPair, number | undefined]>,
generateCertId?: () => CertId,
): Promise<{
keyPrivatePem: PrivateKeyPEM;
Expand All @@ -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(),
Expand Down