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.

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

Add cache issuer directory #15

Merged
merged 6 commits into from
Feb 13, 2024
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
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
1 change: 1 addition & 0 deletions src/bindings.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import type { R2Bucket } from '@cloudflare/workers-types/2023-07-01';

export interface Bindings {
// variables and secrets
DIRECTORY_CACHE_MAX_AGE_SECONDS: string;
ENVIRONMENT: string;
LOGGING_SHIM_TOKEN: string;
SENTRY_ACCESS_CLIENT_ID: string;
Expand Down
22 changes: 14 additions & 8 deletions src/context/metrics.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,23 +13,33 @@ export class MetricsRegistry {
registry: RegistryType;
options: RegistryOptions;

requestsTotal: CounterType;
directoryCacheMissTotal: CounterType;
erroredRequestsTotal: CounterType;
issuanceRequestTotal: CounterType;
keyRotationTotal: CounterType;
keyClearTotal: CounterType;
issuanceRequestTotal: CounterType;
requestsTotal: CounterType;
signedTokenTotal: CounterType;

constructor(options: RegistryOptions) {
this.options = options;
this.registry = new Registry();

this.requestsTotal = this.registry.create('counter', 'requests_total', 'total requests');
this.directoryCacheMissTotal = this.registry.create(
'counter',
'directory_cache_miss_total',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

the metric counts cache miss. It is also possible to devise a metric directory_cache_request_total{status="hit|miss"}. However, I feel this would add uneccessary weight.

'Number of requests for private token issuer directory which are not served by the cache.'
);
this.erroredRequestsTotal = this.registry.create(
'counter',
'errored_requests_total',
'Errored requests served to eyeball'
);
this.issuanceRequestTotal = this.registry.create(
'counter',
'issuance_request_total',
'Number of requests for private token issuance.'
Copy link
Contributor

Choose a reason for hiding this comment

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

successful?

Suggested change
'Number of requests for private token issuance.'
'Number of successful requests for private token issuance.'

Copy link
Contributor Author

Choose a reason for hiding this comment

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

not necessary successful. the metric is increased at the start of handleTokenRequest, no matter the response.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

side: this is also not a new metrics. The metrics have been sorted by alphabetical orders, and it's part of this change.

);
this.keyRotationTotal = this.registry.create(
'counter',
'key_rotation_total',
Expand All @@ -40,11 +50,7 @@ export class MetricsRegistry {
'key_clear_total',
'Number of key clear performed.'
);
this.issuanceRequestTotal = this.registry.create(
'counter',
'issuance_request_total',
'Number of requests for private token issuance.'
);
this.requestsTotal = this.registry.create('counter', 'requests_total', 'total requests');
this.signedTokenTotal = this.registry.create(
'counter',
'signed_token_total',
Expand Down
64 changes: 61 additions & 3 deletions src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import {
} from '@cloudflare/privacypass-ts';
import { ConsoleLogger } from './context/logging';
import { MetricsRegistry } from './context/metrics';
import { hexEncode } from './utils/hex';
const { BlindRSAMode, Issuer, TokenRequest } = publicVerif;

const keyToTokenKeyID = async (key: Uint8Array): Promise<number> => {
Expand Down Expand Up @@ -79,7 +80,39 @@ export const handleTokenRequest = async (ctx: Context, request: Request) => {
});
};

export const handleTokenDirectory = async (ctx: Context, request?: Request) => {
const getDirectoryCache = async (): Promise<Cache> => {
return caches.open('response/issuer-directory');
};

const FAKE_DOMAIN_CACHE = 'cache.local';
const DIRECTORY_CACHE_REQUEST = new Request(
`https://${FAKE_DOMAIN_CACHE}${PRIVATE_TOKEN_ISSUER_DIRECTORY}`
);

export const handleHeadTokenDirectory = async (ctx: Context, request: Request) => {
const getResponse = await handleTokenDirectory(ctx, request);

return new Response(undefined, {
status: getResponse.status,
headers: getResponse.headers,
});
};

// eslint-disable-next-line @typescript-eslint/no-unused-vars
export const handleTokenDirectory = async (ctx: Context, request: Request) => {
const cache = await getDirectoryCache();
const cachedResponse = await cache.match(DIRECTORY_CACHE_REQUEST);
if (cachedResponse) {
if (request.headers.get('if-none-match') === cachedResponse.headers.get('etag')) {
return new Response(undefined, {
status: 304,
headers: cachedResponse.headers,
});
}
return cachedResponse;
}
ctx.metrics.directoryCacheMissTotal.inc({ env: ctx.env.ENVIRONMENT });

const keys = await ctx.env.ISSUANCE_KEYS.list({ include: ['customMetadata'] });

if (keys.objects.length === 0) {
Expand All @@ -95,14 +128,32 @@ export const handleTokenDirectory = async (ctx: Context, request?: Request) => {
})),
};

return new Response(JSON.stringify(directory), {
const body = JSON.stringify(directory);
const digest = new Uint8Array(
await crypto.subtle.digest('SHA-256', new TextEncoder().encode(body))
);
const etag = `"${hexEncode(digest)}"`;

const response = new Response(body, {
headers: {
'content-type': MediaType.PRIVATE_TOKEN_ISSUER_DIRECTORY,
'cache-control': 'public, max-age=86400',
'cache-control': `public, max-age=${ctx.env.DIRECTORY_CACHE_MAX_AGE_SECONDS}`,
'content-length': body.length.toString(),

Choose a reason for hiding this comment

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

Question: Isn't content-length automatically set? Or is it missing due to chunked encoding being used?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

when I first tested, it did not seem to be set automatically. therefore adding it here. The question I have is if the request is compressed, I'm not sure if it's modified automatically.

'date': new Date().toUTCString(),

Choose a reason for hiding this comment

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

Date is not that important for caching, has this been added for debugging purposes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it's been added to help debugging if there are caching issues in the future.

etag,
},
});
ctx.waitUntil(cache.put(DIRECTORY_CACHE_REQUEST, response.clone()));

return response;
};

const clearDirectoryCache = async (): Promise<boolean> => {
const cache = await getDirectoryCache();
return cache.delete(DIRECTORY_CACHE_REQUEST);
};

// eslint-disable-next-line @typescript-eslint/no-unused-vars
export const handleRotateKey = async (ctx: Context, request?: Request) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

You can remove the linter exception and prepend variable's name with _.

Suggested change
// eslint-disable-next-line @typescript-eslint/no-unused-vars
export const handleRotateKey = async (ctx: Context, request?: Request) => {
export const handleRotateKey = async (ctx: Context, _request?: Request) => {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the rule triggered a warning in both case. I've added a rule configuration to ignore ^_, which _request is.

ctx.metrics.keyRotationTotal.inc({ env: ctx.env.ENVIRONMENT });

Expand Down Expand Up @@ -148,9 +199,12 @@ export const handleRotateKey = async (ctx: Context, request?: Request) => {
customMetadata: metadata,
});

ctx.waitUntil(clearDirectoryCache());

return new Response(`New key ${publicKeyEnc}`, { status: 201 });
};

// eslint-disable-next-line @typescript-eslint/no-unused-vars
const handleClearKey = async (ctx: Context, request?: Request) => {
ctx.metrics.keyClearTotal.inc({ env: ctx.env.ENVIRONMENT });
const keys = await ctx.env.ISSUANCE_KEYS.list();
Expand All @@ -169,6 +223,9 @@ const handleClearKey = async (ctx: Context, request?: Request) => {
}
const toDeleteArray = [...toDelete];
await ctx.env.ISSUANCE_KEYS.delete(toDeleteArray);

ctx.waitUntil(clearDirectoryCache());

return new Response(`Keys cleared: ${toDeleteArray.join('\n')}`, { status: 201 });
};

Expand All @@ -179,6 +236,7 @@ export default {
const router = new Router();

router
.head(PRIVATE_TOKEN_ISSUER_DIRECTORY, handleHeadTokenDirectory)

Choose a reason for hiding this comment

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

I think that head method should be dropped, head should be automatically added for all GET requests by the underlying router as suggested in #16 with appropriate tests verifying that behavior.

You do not have to go our of your way to omit the body in the Response constructor. The underlying Workers runtime will skip inclusion of the the body for HEAD requests.

The advantage of using the same implementation is that your response (headers) will be the same with no extra effort.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated

.get(PRIVATE_TOKEN_ISSUER_DIRECTORY, handleTokenDirectory)
.post('/token-request', handleTokenRequest)
.post('/admin/rotate', handleRotateKey)
Expand Down
12 changes: 12 additions & 0 deletions src/utils/hex.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
export const hexDecode = (s: string) => {
const bytes = s.match(/.{1,2}/g);
if (!bytes) {
return new Uint8Array(0);
}
return Uint8Array.from(bytes.map(b => parseInt(b, 16)));
};

export const hexEncode = (u: Uint8Array) =>
Array.from(u)
.map(b => b.toString(16).padStart(2, '0'))
.join('');
61 changes: 60 additions & 1 deletion test/index.test.ts
Original file line number Diff line number Diff line change
@@ -1,8 +1,10 @@
import { jest } from '@jest/globals';

import { Context } from '../src/context';
import { handleTokenRequest, default as workerObject } from '../src/index';
import { IssuerConfigurationResponse } from '../src/types';
import { b64ToB64URL, u8ToB64 } from '../src/utils/base64';
import { ExecutionContextMock, getContext, getEnv } from './mocks';
import { ExecutionContextMock, MockCache, getContext, getEnv } from './mocks';
import { RSABSSA } from '@cloudflare/blindrsa-ts';
import {
MediaType,
Expand Down Expand Up @@ -147,3 +149,60 @@ describe('rotate and clear key', () => {
expect(directory['token-keys']).toHaveLength(1);
});
});

describe('cache directory response', () => {
const rotateURL = `${sampleURL}/admin/rotate`;
const directoryURL = `${sampleURL}${PRIVATE_TOKEN_ISSUER_DIRECTORY}`;

const initializeKeys = async (numberOfKeys = 1): Promise<void> => {
const rotateRequest = new Request(rotateURL, { method: 'POST' });

for (let i = 0; i < numberOfKeys; i += 1) {
await workerObject.fetch(rotateRequest, getEnv(), new ExecutionContextMock());
}
};

it('should cache the directory response', async () => {
await initializeKeys();

const mockCache = new MockCache();
const spy = jest.spyOn(caches, 'open').mockResolvedValue(mockCache);
const directoryRequest = new Request(directoryURL);

let response = await workerObject.fetch(directoryRequest, getEnv(), new ExecutionContextMock());
expect(response.ok).toBe(true);
expect(Object.entries(mockCache.cache)).toHaveLength(1);

const [cachedURL, _] = Object.entries(mockCache.cache)[0];
const sampleEtag = '"sampleEtag"';
const cachedResponse = new Response('cached response', { headers: { etag: sampleEtag } });
mockCache.cache[cachedURL] = cachedResponse;

response = await workerObject.fetch(directoryRequest, getEnv(), new ExecutionContextMock());
expect(response.ok).toBe(true);
expect(response).toBe(cachedResponse);

const cachedDirectoryRequest = new Request(directoryURL, {
headers: { 'if-none-match': sampleEtag },
});
response = await workerObject.fetch(
cachedDirectoryRequest,
getEnv(),
new ExecutionContextMock()
);
expect(response.status).toBe(304);

const headCachedDirectoryRequest = new Request(directoryURL, {
method: 'HEAD',
headers: { 'if-none-match': sampleEtag },
});
response = await workerObject.fetch(
headCachedDirectoryRequest,
getEnv(),
new ExecutionContextMock()
);
expect(response.status).toBe(304);

spy.mockClear();
});
});
25 changes: 25 additions & 0 deletions test/mocks.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,31 @@ import { Context, WaitUntilFunc } from '../src/context';
import { ConsoleLogger, Logger } from '../src/context/logging';
import { MetricsRegistry } from '../src/context/metrics';

export class MockCache implements Cache {
public cache: Record<string, Response> = {};

async match(info: RequestInfo, options?: CacheQueryOptions): Promise<Response | undefined> {
if (options) {
throw new Error('CacheQueryOptions not supported');
}
const url = new URL(info instanceof Request ? info.url : info).href;
return this.cache[url];
}

async delete(info: RequestInfo, options?: CacheQueryOptions): Promise<boolean> {
if (options) {
throw new Error('CacheQueryOptions not supported');
}
const url = new URL(info instanceof Request ? info.url : info).href;
return delete this.cache[url];
}

async put(info: RequestInfo, response: Response): Promise<void> {
const url = new URL(info instanceof Request ? info.url : info).href;
this.cache[url] = response;
}
}

export class ExecutionContextMock implements ExecutionContext {
waitUntils: Promise<any>[] = [];
passThrough = false;
Expand Down
1 change: 1 addition & 0 deletions wrangler.toml
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ route = { pattern = "pp-issuer.example.test", custom_domain=true }
crons = ["0 0 * * *"]

[env.production.vars]
DIRECTORY_CACHE_MAX_AGE_SECONDS = "86400"
ENVIRONMENT = "production"
SENTRY_SAMPLE_RATE = "0" # Between 0-1 if you log errors on Sentry. 0 disables Sentry logging. Configuration is done through Workers Secrets

Expand Down
Loading