From c50918c1f8c170c236d64b30cf23988e12745f6e Mon Sep 17 00:00:00 2001 From: Lina Baquero Date: Fri, 11 Oct 2024 11:52:46 -0400 Subject: [PATCH 1/3] Update clear key logic, delete keys after prevRotationDate + KEY_LIFESPAN (48H), except last uploaded key --- src/errors.ts | 7 +++ src/index.ts | 39 ++++++++------ src/utils/keyRotation.ts | 50 +++++++++++++++--- test/index.test.ts | 106 ++++++++++++++++++++++++++++++++++++++- 4 files changed, 177 insertions(+), 25 deletions(-) diff --git a/src/errors.ts b/src/errors.ts index 3c68105..fc7a4b1 100644 --- a/src/errors.ts +++ b/src/errors.ts @@ -136,3 +136,10 @@ export class BadTokenKeyRequestedError extends HTTPError { this.code = BadTokenKeyRequestedError.CODE; } } + +export class PrevRotationTimeError extends Error { + constructor(message: string) { + super(message); + this.name = 'PrevRotationTimeError'; + } +} diff --git a/src/index.ts b/src/index.ts index e650d4e..4fc1c2d 100644 --- a/src/index.ts +++ b/src/index.ts @@ -25,7 +25,7 @@ import { hexEncode } from './utils/hex'; import { DIRECTORY_CACHE_REQUEST, clearDirectoryCache, getDirectoryCache } from './cache'; const { BlindRSAMode, Issuer, TokenRequest } = publicVerif; -import { shouldRotateKey } from './utils/keyRotation'; +import { getPrevRotationTime, shouldRotateKey, shouldClearKey } from './utils/keyRotation'; const keyToTokenKeyID = async (key: Uint8Array): Promise => { const hash = await crypto.subtle.digest('SHA-256', key); @@ -38,8 +38,6 @@ interface StorageMetadata extends Record { tokenKeyID: string; } -const KEY_LIFESPAN = 48 * 60 * 60 * 1000; - export const handleTokenRequest = async (ctx: Context, request: Request) => { ctx.metrics.issuanceRequestTotal.inc({ version: ctx.env.VERSION_METADATA.id ?? RELEASE }); const contentType = request.headers.get('content-type'); @@ -231,31 +229,40 @@ export const handleRotateKey = async (ctx: Context, _request?: Request) => { const handleClearKey = async (ctx: Context, _request?: Request) => { ctx.metrics.keyClearTotal.inc(); - - const now = Date.now(); + const now = new Date(); const keys = await ctx.bucket.ISSUANCE_KEYS.list({ shouldUseCache: false }); + if (keys.objects.length === 0) { + return new Response('No keys to clear', { status: 201 }); + } + + // iterating twice over keys, because we need to know the latest upload key time to enforce key clearance + // Find the latest key based on the upload time let latestKey = keys.objects[0]; + for (const key of keys.objects) { + if (new Date(key.uploaded).getTime() > new Date(latestKey.uploaded).getTime()) { + latestKey = key; + } + } + + const effectivePrevRotationTime = getPrevRotationTime(new Date(latestKey.uploaded), ctx); + + if (effectivePrevRotationTime == null) { + return new Response('Failed to determine previous rotation time', { status: 500 }); + } + const toDelete: Set = new Set(); for (const key of keys.objects) { - const keyUploadTime = new Date(key.uploaded).getTime(); - const keyExpirationTime = keyUploadTime + KEY_LIFESPAN; - - if (keyExpirationTime < now) { + const keyUploadTime = new Date(key.uploaded); + if (shouldClearKey(keyUploadTime, now, effectivePrevRotationTime)) { toDelete.add(key.key); } - - if (latestKey.uploaded < key.uploaded) { - latestKey = key; - } } // Ensure the latest key is never deleted - if (toDelete.has(latestKey.key)) { - toDelete.delete(latestKey.key); - } + toDelete.delete(latestKey.key); if (toDelete.size === 0) { return new Response('No keys to clear', { status: 201 }); diff --git a/src/utils/keyRotation.ts b/src/utils/keyRotation.ts index 68a42d6..5f1e138 100644 --- a/src/utils/keyRotation.ts +++ b/src/utils/keyRotation.ts @@ -1,17 +1,45 @@ import { Bindings } from '../bindings'; import cronParser from 'cron-parser'; +import { Context } from '../context'; +import { PrevRotationTimeError } from '../errors'; + +interface CronParseResult { + prevTime?: number; + nextTime?: number; + match: boolean; +} + +const KEY_LIFESPAN = 48 * 60 * 60 * 1000; + +export function getPrevRotationTime(mostRecentKeyUploadTime: Date, ctx: Context): number { + let effectivePrevTime: number; + if (ctx.env.ROTATION_CRON_STRING) { + const { prevTime } = matchCronTime(ctx.env.ROTATION_CRON_STRING, mostRecentKeyUploadTime); + + if (prevTime === undefined) { + console.error('Failed to determine previous rotation time for key'); + throw new PrevRotationTimeError('Failed to determine previous rotation time'); + } + + effectivePrevTime = Math.max(prevTime, mostRecentKeyUploadTime.getTime()); + } else { + effectivePrevTime = mostRecentKeyUploadTime.getTime(); + } + return effectivePrevTime; +} export function shouldRotateKey(date: Date, env: Bindings): boolean { const utcDate = new Date(date.toISOString()); + return env.ROTATION_CRON_STRING ? matchCronTime(env.ROTATION_CRON_STRING, utcDate).match : false; +} - if (env.ROTATION_CRON_STRING) { - const result = matchCronTime(env.ROTATION_CRON_STRING, utcDate); - return result; - } - return false; +export function shouldClearKey(keyUploadTime: Date, now: Date, effectivePrevTime: number): boolean { + const keyExpirationTime = keyUploadTime.getTime() + KEY_LIFESPAN; + const rotationBasedExpirationTime = effectivePrevTime + KEY_LIFESPAN; + return now.getTime() >= Math.max(keyExpirationTime, rotationBasedExpirationTime); } -function matchCronTime(cronString: string, date: Date): boolean { +export function matchCronTime(cronString: string, date: Date): CronParseResult { // Set seconds and milliseconds to 0 to truncate to the nearest minute // https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Date/setUTCSeconds#parameters date.setUTCSeconds(0, 0); @@ -25,12 +53,18 @@ function matchCronTime(cronString: string, date: Date): boolean { try { interval = cronParser.parseExpression(cronString, options); } catch (error) { - return false; + console.error('Error parsing cron string', error); + return { match: false }; } const prevDate = interval.prev().toDate(); const nextDate = interval.next().toDate(); const result = date.getTime() === prevDate.getTime() || date.getTime() === nextDate.getTime(); - return result; + + return { + prevTime: prevDate.getTime(), + nextTime: nextDate.getTime(), + match: result, + }; } diff --git a/test/index.test.ts b/test/index.test.ts index b9d6d17..0aba33a 100644 --- a/test/index.test.ts +++ b/test/index.test.ts @@ -17,9 +17,32 @@ import { util, } from '@cloudflare/privacypass-ts'; import { getDirectoryCache } from '../src/cache'; -import { shouldRotateKey } from '../src/utils/keyRotation'; +import { + shouldRotateKey, + shouldClearKey, + matchCronTime, + getPrevRotationTime, +} from '../src/utils/keyRotation'; const { TokenRequest } = publicVerif; +// import * as keyRotation from '../src/utils/keyRotation'; // Import all as keyRotation + +// jest.unstable_mockModule('../src/utils/keyRotation', () => ({ +// matchCronTime: jest.fn(), +// })); + +import * as keyRotation from '../src/utils/keyRotation'; + +// Mock the entire module and ensure matchCronTime is mocked +jest.mock('../src/utils/keyRotation', () => { + const originalModule = jest.requireActual('../src/utils/keyRotation'); + return { + __esModule: true, // Important for ESModules + ...originalModule, + matchCronTime: jest.fn(), // mock matchCronTime + }; +}); + const sampleURL = 'http://localhost'; const keyToTokenKeyID = async (key: Uint8Array): Promise => { @@ -356,3 +379,84 @@ describe('key rotation', () => { expect(shouldRotateKey(date, ctx.env)).toBe(true); }); }); + +describe('shouldClearKey Function', () => { + it('should not clear key when neither expiration time has passed', () => { + const keyUploadTime = new Date('2023-10-01T12:00:00Z'); + const now = new Date('2023-10-02T11:59:59Z'); + const effectivePrevTime = new Date('2023-09-30T00:00:00Z').getTime(); + + const result = shouldClearKey(keyUploadTime, now, effectivePrevTime); + expect(result).toBe(false); + }); + + it('should not clear key when per-key expiration has passed but rotation-based expiration has not', () => { + const keyUploadTime = new Date('2023-09-29T12:00:00Z'); + const now = new Date('2023-10-01T12:00:01Z'); + const effectivePrevTime = new Date('2023-10-03T00:00:00Z').getTime(); + + const result = shouldClearKey(keyUploadTime, now, effectivePrevTime); + expect(result).toBe(false); + }); + + it('should not clear key when rotation-based expiration has passed but per-key expiration has not', () => { + const keyUploadTime = new Date('2023-10-03T12:00:00Z'); + const now = new Date('2023-10-05T11:59:59Z'); + const effectivePrevTime = new Date('2023-10-01T00:00:00Z').getTime(); + + const result = shouldClearKey(keyUploadTime, now, effectivePrevTime); + expect(result).toBe(false); + }); + + it('should clear key when both expiration times have passed', () => { + const keyUploadTime = new Date('2023-09-29T12:00:00Z'); + const now = new Date('2023-10-05T12:00:01Z'); + const effectivePrevTime = new Date('2023-09-30T00:00:00Z').getTime(); + + const result = shouldClearKey(keyUploadTime, now, effectivePrevTime); + expect(result).toBe(true); + }); + + it('should clear key when current time equals the maximum expiration time', () => { + const keyUploadTime = new Date('2023-10-01T12:00:00Z'); + const now = new Date('2023-10-03T12:00:00Z'); + const effectivePrevTime = new Date('2023-10-01T12:00:00Z').getTime(); + + const result = shouldClearKey(keyUploadTime, now, effectivePrevTime); + expect(result).toBe(true); + }); +}); + +describe('getPrevRotationTime Function', () => { + it('should return the maximum of prevTime and mostRecentKeyUploadTime when ROTATION_CRON_STRING is valid', () => { + const ctx = getContext({ + request: new Request('https://example.com'), + env: getEnv(), + ectx: new ExecutionContextMock(), + }); + + const mostRecentKeyUploadTime = new Date('2023-10-03T12:00:00Z'); + ctx.env.ROTATION_CRON_STRING = '* * * * *'; + + const expectedPrevTime = new Date('2023-10-03T00:00:00Z').getTime(); + const expected = Math.max(expectedPrevTime, mostRecentKeyUploadTime.getTime()); + + const result = getPrevRotationTime(mostRecentKeyUploadTime, ctx); + expect(result).toBe(expected); + }); + + it('should return mostRecentKeyUploadTime when ROTATION_CRON_STRING is not set', () => { + const ctx = getContext({ + request: new Request('https://example.com'), + env: getEnv(), + ectx: new ExecutionContextMock(), + }); + + ctx.env.ROTATION_CRON_STRING = undefined; + const mostRecentKeyUploadTime = new Date('2023-10-03T12:00:00Z'); + const expected = mostRecentKeyUploadTime.getTime(); + + const result = getPrevRotationTime(mostRecentKeyUploadTime, ctx); + expect(result).toBe(expected); + }); +}); From 5ee9493249088e6e373d9677ec7e80b6119f5e9b Mon Sep 17 00:00:00 2001 From: Thibault Meunier Date: Tue, 15 Oct 2024 09:29:21 +0200 Subject: [PATCH 2/3] Move tests to use `each` instead of copy paste Key rotation and clearance tests have a lot of copy paste, making them hard to review and update. This commit moves them to `each` format. --- src/utils/keyRotation.ts | 6 +- test/index.test.ts | 185 +++++++++++++-------------------------- 2 files changed, 64 insertions(+), 127 deletions(-) diff --git a/src/utils/keyRotation.ts b/src/utils/keyRotation.ts index 5f1e138..84cfef3 100644 --- a/src/utils/keyRotation.ts +++ b/src/utils/keyRotation.ts @@ -9,7 +9,7 @@ interface CronParseResult { match: boolean; } -const KEY_LIFESPAN = 48 * 60 * 60 * 1000; +const KEY_LIFESPAN_IN_MS = 48 * 60 * 60 * 1000; export function getPrevRotationTime(mostRecentKeyUploadTime: Date, ctx: Context): number { let effectivePrevTime: number; @@ -34,8 +34,8 @@ export function shouldRotateKey(date: Date, env: Bindings): boolean { } export function shouldClearKey(keyUploadTime: Date, now: Date, effectivePrevTime: number): boolean { - const keyExpirationTime = keyUploadTime.getTime() + KEY_LIFESPAN; - const rotationBasedExpirationTime = effectivePrevTime + KEY_LIFESPAN; + const keyExpirationTime = keyUploadTime.getTime() + KEY_LIFESPAN_IN_MS; + const rotationBasedExpirationTime = effectivePrevTime + KEY_LIFESPAN_IN_MS; return now.getTime() >= Math.max(keyExpirationTime, rotationBasedExpirationTime); } diff --git a/test/index.test.ts b/test/index.test.ts index 0aba33a..77e1e4f 100644 --- a/test/index.test.ts +++ b/test/index.test.ts @@ -25,12 +25,6 @@ import { } from '../src/utils/keyRotation'; const { TokenRequest } = publicVerif; -// import * as keyRotation from '../src/utils/keyRotation'; // Import all as keyRotation - -// jest.unstable_mockModule('../src/utils/keyRotation', () => ({ -// matchCronTime: jest.fn(), -// })); - import * as keyRotation from '../src/utils/keyRotation'; // Mock the entire module and ensure matchCronTime is mocked @@ -307,130 +301,72 @@ describe('directory', () => { }); describe('key rotation', () => { - it('should rotate key at every minute', async () => { - const ctx = getContext({ - request: new Request(sampleURL), - env: getEnv(), - ectx: new ExecutionContextMock(), - }); - ctx.env.ROTATION_CRON_STRING = '* * * * *'; // Every minute - - const date = new Date('2023-08-01T00:01:00Z'); - expect(shouldRotateKey(date, ctx.env)).toBe(true); - }); - - it('should rotate key at midnight on the first day of every month', async () => { - const ctx = getContext({ - request: new Request(sampleURL), - env: getEnv(), - ectx: new ExecutionContextMock(), - }); - ctx.env.ROTATION_CRON_STRING = '0 0 1 * *'; // At 00:00 on day-of-month 1 - - const date = new Date('2023-09-01T00:00:00Z'); - expect(shouldRotateKey(date, ctx.env)).toBe(true); - }); - - it('should rotate key at 12:30 PM every day', async () => { - const ctx = getContext({ - request: new Request(sampleURL), - env: getEnv(), - ectx: new ExecutionContextMock(), - }); - ctx.env.ROTATION_CRON_STRING = '30 12 * * *'; // At 12:30 every day - - const date = new Date('2023-08-01T12:30:00Z'); - expect(shouldRotateKey(date, ctx.env)).toBe(true); - }); - - it('should not rotate key at noon on a non-rotation day', async () => { - const ctx = getContext({ - request: new Request(sampleURL), - env: getEnv(), - ectx: new ExecutionContextMock(), - }); - ctx.env.ROTATION_CRON_STRING = '0 0 1 * *'; // At 00:00 on day-of-month 1 - - const date = new Date('2023-08-02T12:00:00Z'); // 2nd August is not the 1st - expect(shouldRotateKey(date, ctx.env)).toBe(false); - }); - - it('should rotate key at 11:59 PM on the last day of the month', async () => { - const ctx = getContext({ - request: new Request(sampleURL), - env: getEnv(), - ectx: new ExecutionContextMock(), - }); - ctx.env.ROTATION_CRON_STRING = '59 23 * * *'; // At 23:59 on the last day of the month - - const date = new Date('2023-08-31T23:59:00Z'); // 31st August 2023 is the last day of the month - expect(shouldRotateKey(date, ctx.env)).toBe(true); - }); - - it('should handle rotation with millisecond precision', async () => { - const ctx = getContext({ - request: new Request(sampleURL), - env: getEnv(), - ectx: new ExecutionContextMock(), - }); - ctx.env.ROTATION_CRON_STRING = '* * * * *'; - - const date = new Date('2023-08-01T00:01:00.010Z'); - expect(shouldRotateKey(date, ctx.env)).toBe(true); - }); + it.concurrent.each` + name | rotationCron | date | expected + ${'rotate key at every minute'} | ${'* * * * *'} | ${'2023-08-01T00:01:00Z'} | ${true} + ${'rotate key at midnight on the first day of every month'} | ${'0 0 1 * *'} | ${'2023-09-01T00:00:00Z'} | ${true} + ${'rotate key at 12:30 PM every day'} | ${'30 12 * * *'} | ${'2023-08-01T12:30:00Z'} | ${true} + ${'not rotate key at noon on a non-rotation day'} | ${'0 0 1 * *'} | ${'2023-08-02T12:00:00Z'} | ${false} + ${'rotate key at 11:59 PM on the last day of the month'} | ${'59 23 * * *'} | ${'2023-08-31T23:59:00Z'} | ${true} + ${'handle rotation with millisecond precision'} | ${'* * * * *'} | ${'2023-08-01T00:01:00.010Z'} | ${true} + `( + 'should $name', + async ({ + rotationCron, + date, + expected, + }: { + rotationCron: string; + date: string; + expected: boolean; + }) => { + const ctx = getContext({ + request: new Request(sampleURL), + env: getEnv(), + ectx: new ExecutionContextMock(), + }); + ctx.env.ROTATION_CRON_STRING = rotationCron; + + expect(shouldRotateKey(new Date(date), ctx.env)).toBe(expected); + } + ); }); describe('shouldClearKey Function', () => { - it('should not clear key when neither expiration time has passed', () => { - const keyUploadTime = new Date('2023-10-01T12:00:00Z'); - const now = new Date('2023-10-02T11:59:59Z'); - const effectivePrevTime = new Date('2023-09-30T00:00:00Z').getTime(); - - const result = shouldClearKey(keyUploadTime, now, effectivePrevTime); - expect(result).toBe(false); - }); - - it('should not clear key when per-key expiration has passed but rotation-based expiration has not', () => { - const keyUploadTime = new Date('2023-09-29T12:00:00Z'); - const now = new Date('2023-10-01T12:00:01Z'); - const effectivePrevTime = new Date('2023-10-03T00:00:00Z').getTime(); - - const result = shouldClearKey(keyUploadTime, now, effectivePrevTime); - expect(result).toBe(false); - }); - - it('should not clear key when rotation-based expiration has passed but per-key expiration has not', () => { - const keyUploadTime = new Date('2023-10-03T12:00:00Z'); - const now = new Date('2023-10-05T11:59:59Z'); - const effectivePrevTime = new Date('2023-10-01T00:00:00Z').getTime(); - - const result = shouldClearKey(keyUploadTime, now, effectivePrevTime); - expect(result).toBe(false); - }); - - it('should clear key when both expiration times have passed', () => { - const keyUploadTime = new Date('2023-09-29T12:00:00Z'); - const now = new Date('2023-10-05T12:00:01Z'); - const effectivePrevTime = new Date('2023-09-30T00:00:00Z').getTime(); - - const result = shouldClearKey(keyUploadTime, now, effectivePrevTime); - expect(result).toBe(true); - }); - - it('should clear key when current time equals the maximum expiration time', () => { - const keyUploadTime = new Date('2023-10-01T12:00:00Z'); - const now = new Date('2023-10-03T12:00:00Z'); - const effectivePrevTime = new Date('2023-10-01T12:00:00Z').getTime(); - - const result = shouldClearKey(keyUploadTime, now, effectivePrevTime); - expect(result).toBe(true); - }); + it.concurrent.each` + name | keyUpload | now | effectivePrev | expected + ${'not clear key when neither expiration time has passed'} | ${'2023-10-01T12:00:00Z'} | ${'2023-10-02T11:59:59Z'} | ${'2023-09-30T00:00:00Z'} | ${false} + ${'not clear key when per-key expiration has passed but rotation-based expiration has not'} | ${'2023-09-29T12:00:00Z'} | ${'2023-10-01T12:00:01Z'} | ${'2023-10-03T00:00:00Z'} | ${false} + ${'not clear key when rotation-based expiration has passed but per-key expiration has not'} | ${'2023-10-03T12:00:00Z'} | ${'2023-10-05T11:59:59Z'} | ${'2023-10-01T00:00:00Z'} | ${false} + ${'clear key when both expiration times have passed'} | ${'2023-09-29T12:00:00Z'} | ${'2023-10-05T12:00:01Z'} | ${'2023-09-30T00:00:00Z'} | ${true} + ${'clear key when current time equals the maximum expiration time'} | ${'2023-10-01T12:00:00Z'} | ${'2023-10-03T12:00:00Z'} | ${'2023-10-01T12:00:00Z'} | ${true} + `( + 'should $name', + ({ + keyUpload, + now, + effectivePrev, + expected, + }: { + keyUpload: string; + now: string; + effectivePrev: string; + expected: boolean; + }) => { + const keyUploadTime = new Date(keyUpload); + const nowTime = new Date(now); + const effectivePrevTime = new Date(effectivePrev).getTime(); + + const result = shouldClearKey(keyUploadTime, nowTime, effectivePrevTime); + expect(result).toBe(expected); + } + ); }); describe('getPrevRotationTime Function', () => { it('should return the maximum of prevTime and mostRecentKeyUploadTime when ROTATION_CRON_STRING is valid', () => { const ctx = getContext({ - request: new Request('https://example.com'), + request: new Request(sampleURL), env: getEnv(), ectx: new ExecutionContextMock(), }); @@ -447,13 +383,14 @@ describe('getPrevRotationTime Function', () => { it('should return mostRecentKeyUploadTime when ROTATION_CRON_STRING is not set', () => { const ctx = getContext({ - request: new Request('https://example.com'), + request: new Request(sampleURL), env: getEnv(), ectx: new ExecutionContextMock(), }); - ctx.env.ROTATION_CRON_STRING = undefined; const mostRecentKeyUploadTime = new Date('2023-10-03T12:00:00Z'); + ctx.env.ROTATION_CRON_STRING = undefined; + const expected = mostRecentKeyUploadTime.getTime(); const result = getPrevRotationTime(mostRecentKeyUploadTime, ctx); From 8df90d020e0b53a6323e3a1e1ecaff3c3e2a7f3a Mon Sep 17 00:00:00 2001 From: Thibault Date: Tue, 15 Oct 2024 09:44:53 +0200 Subject: [PATCH 3/3] Remove unecessary null check --- src/index.ts | 4 ---- 1 file changed, 4 deletions(-) diff --git a/src/index.ts b/src/index.ts index 4fc1c2d..11e4f25 100644 --- a/src/index.ts +++ b/src/index.ts @@ -248,10 +248,6 @@ const handleClearKey = async (ctx: Context, _request?: Request) => { const effectivePrevRotationTime = getPrevRotationTime(new Date(latestKey.uploaded), ctx); - if (effectivePrevRotationTime == null) { - return new Response('Failed to determine previous rotation time', { status: 500 }); - } - const toDelete: Set = new Set(); for (const key of keys.objects) {