From 874d8459ce53951ab0f1047c84757a40325e3c21 Mon Sep 17 00:00:00 2001 From: Fredrik Strand Oseberg Date: Fri, 19 Aug 2022 10:48:33 +0200 Subject: [PATCH] Feat/add alias to api tokens (#1931) * refactor: remove unused API definition routes * feat: embed proxy endpoints * feat: check token metadata for alias if none is found * fix: rename param * feat: add test for retrieving token by alias * fix: update schema * fix: refactor to alias * fix: refactor to null * fix: update snapshot * fix: update openapi snapshot * fix: add check to getUserForToken * refactor: add more token alias tests * refactor: use timingSafeEqual for token comparisons Co-authored-by: olav --- src/lib/db/api-token-store.ts | 8 +- .../api-token-schema.test.ts.snap | 17 ---- src/lib/openapi/spec/api-token-schema.test.ts | 38 --------- src/lib/openapi/spec/api-token-schema.ts | 16 +--- src/lib/schema/api-token-schema.test.ts | 21 +---- src/lib/services/api-token-service.ts | 18 ++-- src/lib/types/models/api-token.ts | 6 +- src/lib/util/constantTimeCompare.test.ts | 55 ++++++++++++ src/lib/util/constantTimeCompare.ts | 12 +++ .../20220817130250-alter-api-tokens.js | 17 ++++ .../__snapshots__/openapi.e2e.test.ts.snap | 20 +---- src/test/e2e/api/proxy/proxy.e2e.test.ts | 83 +++++++++++++++++++ src/test/fixtures/fake-api-token-store.ts | 2 +- 13 files changed, 194 insertions(+), 119 deletions(-) create mode 100644 src/lib/util/constantTimeCompare.test.ts create mode 100644 src/lib/util/constantTimeCompare.ts create mode 100644 src/migrations/20220817130250-alter-api-tokens.js diff --git a/src/lib/db/api-token-store.ts b/src/lib/db/api-token-store.ts index 16d5162330..fdd9d5ce8b 100644 --- a/src/lib/db/api-token-store.ts +++ b/src/lib/db/api-token-store.ts @@ -45,7 +45,7 @@ const tokenRowReducer = (acc, tokenRow) => { environment: token.environment ? token.environment : ALL, expiresAt: token.expires_at, createdAt: token.created_at, - metadata: token.metadata, + alias: token.alias, }; } const currentToken = acc[tokenRow.secret]; @@ -66,7 +66,7 @@ const toRow = (newToken: IApiTokenCreate) => ({ environment: newToken.environment === ALL ? undefined : newToken.environment, expires_at: newToken.expiresAt, - metadata: newToken.metadata || {}, + alias: newToken.alias || null, }); const toTokens = (rows: any[]): IApiToken[] => { @@ -126,7 +126,7 @@ export class ApiTokenStore implements IApiTokenStore { 'type', 'expires_at', 'created_at', - 'metadata', + 'alias', 'seen_at', 'environment', 'token_project_link.project', @@ -153,7 +153,7 @@ export class ApiTokenStore implements IApiTokenStore { await Promise.all(updateProjectTasks); return { ...newToken, - metadata: newToken.metadata || {}, + alias: newToken.alias || null, project: newToken.projects?.join(',') || '*', createdAt: row.created_at, }; diff --git a/src/lib/openapi/spec/__snapshots__/api-token-schema.test.ts.snap b/src/lib/openapi/spec/__snapshots__/api-token-schema.test.ts.snap index 4e9be0e49d..7a3c062650 100644 --- a/src/lib/openapi/spec/__snapshots__/api-token-schema.test.ts.snap +++ b/src/lib/openapi/spec/__snapshots__/api-token-schema.test.ts.snap @@ -16,20 +16,3 @@ Object { "schema": "#/components/schemas/apiTokenSchema", } `; - -exports[`apiTokenSchema metadata - does not allow custom metadata parameters 1`] = ` -Object { - "errors": Array [ - Object { - "instancePath": "/metadata", - "keyword": "additionalProperties", - "message": "must NOT have additional properties", - "params": Object { - "additionalProperty": "arbitraryParameter", - }, - "schemaPath": "#/properties/metadata/additionalProperties", - }, - ], - "schema": "#/components/schemas/apiTokenSchema", -} -`; diff --git a/src/lib/openapi/spec/api-token-schema.test.ts b/src/lib/openapi/spec/api-token-schema.test.ts index 7a5efc269f..610b29ca86 100644 --- a/src/lib/openapi/spec/api-token-schema.test.ts +++ b/src/lib/openapi/spec/api-token-schema.test.ts @@ -22,44 +22,6 @@ test('apiTokenSchema', () => { ).toBeUndefined(); }); -test('apiTokenSchema metadata - should allow empty object', () => { - const data: ApiTokenSchema = { ...defaultData, metadata: {} }; - - expect( - validateSchema('#/components/schemas/apiTokenSchema', data), - ).toBeUndefined(); -}); - -test('apiTokenSchema metadata - allows for corsOrigins and/or alias', () => { - expect( - validateSchema('#/components/schemas/apiTokenSchema', { - ...defaultData, - metadata: { corsOrigins: ['*'] }, - }), - ).toBeUndefined(); - expect( - validateSchema('#/components/schemas/apiTokenSchema', { - ...defaultData, - metadata: { alias: 'secret' }, - }), - ).toBeUndefined(); - expect( - validateSchema('#/components/schemas/apiTokenSchema', { - ...defaultData, - metadata: { corsOrigins: ['*'], alias: 'abc' }, - }), - ).toBeUndefined(); -}); - -test('apiTokenSchema metadata - does not allow custom metadata parameters', () => { - expect( - validateSchema('#/components/schemas/apiTokenSchema', { - ...defaultData, - metadata: { arbitraryParameter: true }, - }), - ).toMatchSnapshot(); -}); - test('apiTokenSchema empty', () => { expect( validateSchema('#/components/schemas/apiTokenSchema', {}), diff --git a/src/lib/openapi/spec/api-token-schema.ts b/src/lib/openapi/spec/api-token-schema.ts index cda8528f71..579b3b6c22 100644 --- a/src/lib/openapi/spec/api-token-schema.ts +++ b/src/lib/openapi/spec/api-token-schema.ts @@ -44,20 +44,8 @@ export const apiTokenSchema = { format: 'date-time', nullable: true, }, - metadata: { - type: 'object', - additionalProperties: false, - properties: { - corsOrigins: { - type: 'array', - items: { - type: 'string', - }, - }, - alias: { - type: 'string', - }, - }, + alias: { + type: 'string', nullable: true, }, }, diff --git a/src/lib/schema/api-token-schema.test.ts b/src/lib/schema/api-token-schema.test.ts index 5e74a42eba..9fed583f7e 100644 --- a/src/lib/schema/api-token-schema.test.ts +++ b/src/lib/schema/api-token-schema.test.ts @@ -43,27 +43,11 @@ test('should not have projects set if project is present', async () => { expect(token.projects).not.toBeDefined(); }); -test('should set metadata', async () => { - let token = await createApiToken.validateAsync({ - username: 'test', - type: 'admin', - project: 'default', - metadata: { - corsOrigins: ['*'], - alias: 'secret', - }, - }); - expect(token.projects).toBeUndefined(); -}); - -test('should allow for frontend key (embedded proxy)', async () => { +test('should allow for embedded proxy (frontend) key', async () => { let token = await createApiToken.validateAsync({ username: 'test', type: 'frontend', project: 'default', - metadata: { - corsOrigins: ['*'], - }, }); expect(token.error).toBeUndefined(); }); @@ -73,9 +57,6 @@ test('should set environment to default for frontend key', async () => { username: 'test', type: 'frontend', project: 'default', - metadata: { - corsOrigins: ['*'], - }, }); expect(token.environment).toEqual('default'); }); diff --git a/src/lib/services/api-token-service.ts b/src/lib/services/api-token-service.ts index 1517d2dbf5..e6129611b2 100644 --- a/src/lib/services/api-token-service.ts +++ b/src/lib/services/api-token-service.ts @@ -19,6 +19,7 @@ import { FOREIGN_KEY_VIOLATION } from '../error/db-error'; import BadDataError from '../error/bad-data-error'; import { minutesToMilliseconds } from 'date-fns'; import { IEnvironmentStore } from 'lib/types/stores/environment-store'; +import { constantTimeCompare } from '../util/constantTimeCompare'; const resolveTokenPermissions = (tokenType: string) => { if (tokenType === ApiTokenType.ADMIN) { @@ -106,13 +107,19 @@ export class ApiTokenService { return undefined; } - let token = this.activeTokens.find((t) => t.secret === secret); + let token = this.activeTokens.find( + (activeToken) => + Boolean(activeToken.secret) && + constantTimeCompare(activeToken.secret, secret), + ); - // If the token is not found, try to find it in the legacy format with the metadata alias - // This is to ensure that previous proxies we set up for our customers continue working - if (!token && secret) { + // If the token is not found, try to find it in the legacy format with alias. + // This allows us to support the old format of tokens migrating to the embedded proxy. + if (!token) { token = this.activeTokens.find( - (t) => t.metadata.alias && t.metadata.alias === secret, + (activeToken) => + Boolean(activeToken.alias) && + constantTimeCompare(activeToken.alias, secret), ); } @@ -126,6 +133,7 @@ export class ApiTokenService { secret: token.secret, }); } + return undefined; } diff --git a/src/lib/types/models/api-token.ts b/src/lib/types/models/api-token.ts index 9e35d31d00..97d2a68372 100644 --- a/src/lib/types/models/api-token.ts +++ b/src/lib/types/models/api-token.ts @@ -22,21 +22,19 @@ export interface ILegacyApiTokenCreate { export interface IApiTokenCreate { secret: string; username: string; - metadata?: Metadata; + alias?: string; type: ApiTokenType; environment: string; projects: string[]; expiresAt?: Date; } -type Metadata = { [key: string]: unknown }; - export interface IApiToken extends IApiTokenCreate { createdAt: Date; seenAt?: Date; environment: string; project: string; - metadata: Metadata; + alias: string | null; } export const isAllProjects = (projects: string[]): boolean => { diff --git a/src/lib/util/constantTimeCompare.test.ts b/src/lib/util/constantTimeCompare.test.ts new file mode 100644 index 0000000000..dab4a9dc45 --- /dev/null +++ b/src/lib/util/constantTimeCompare.test.ts @@ -0,0 +1,55 @@ +import { constantTimeCompare } from './constantTimeCompare'; + +test('constantTimeCompare', () => { + expect(constantTimeCompare('', '')).toEqual(false); + expect(constantTimeCompare(' ', '')).toEqual(false); + expect(constantTimeCompare('a', '')).toEqual(false); + expect(constantTimeCompare('', 'b')).toEqual(false); + expect(constantTimeCompare('a', 'b')).toEqual(false); + + expect(constantTimeCompare(' ', ' ')).toEqual(true); + expect(constantTimeCompare('a', 'a')).toEqual(true); + expect(constantTimeCompare('b', 'b')).toEqual(true); + + expect( + constantTimeCompare( + '*:*.63df5492f027129de9c9368bc80fb8200677e97fb107455497dc42d6', + '*:production.431c724bd84fbe8484bc6437d8e189f0ee288ebee6332bd030a539f5', + ), + ).toEqual(false); + + expect( + constantTimeCompare( + '*:production.559520cc3c1a3b071260f77d80c4650a08699a1d918ea4e7b18c487e', + 'default:development.148bbfa96e91ca41d6580232b331bbbdbc40fcc3626a815055ba79b5', + ), + ).toEqual(false); + + expect( + constantTimeCompare( + '*:production.559520cc3c1a3b071260f77d80c4650a08699a1d918ea4e7b18c487e', + '*:production.431c724bd84fbe8484bc6437d8e189f0ee288ebee6332bd030a539f5', + ), + ).toEqual(false); + + expect( + constantTimeCompare( + '*:production.431c724bd84fbe8484bc6437d8e189f0ee288ebee6332bd030a539f5', + '*:production.559520cc3c1a3b071260f77d80c4650a08699a1d918ea4e7b18c487e', + ), + ).toEqual(false); + + expect( + constantTimeCompare( + '*:*.63df5492f027129de9c9368bc80fb8200677e97fb107455497dc42d6', + '*:*.63df5492f027129de9c9368bc80fb8200677e97fb107455497dc42d6', + ), + ).toEqual(true); + + expect( + constantTimeCompare( + '*:production.431c724bd84fbe8484bc6437d8e189f0ee288ebee6332bd030a539f5', + '*:production.431c724bd84fbe8484bc6437d8e189f0ee288ebee6332bd030a539f5', + ), + ).toEqual(true); +}); diff --git a/src/lib/util/constantTimeCompare.ts b/src/lib/util/constantTimeCompare.ts new file mode 100644 index 0000000000..e5acd7a185 --- /dev/null +++ b/src/lib/util/constantTimeCompare.ts @@ -0,0 +1,12 @@ +import crypto from 'crypto'; + +export const constantTimeCompare = (a: string, b: string): boolean => { + if (!a || !b || a.length !== b.length) { + return false; + } + + return crypto.timingSafeEqual( + Buffer.from(a, 'utf8'), + Buffer.from(b, 'utf8'), + ); +}; diff --git a/src/migrations/20220817130250-alter-api-tokens.js b/src/migrations/20220817130250-alter-api-tokens.js new file mode 100644 index 0000000000..0a5546776c --- /dev/null +++ b/src/migrations/20220817130250-alter-api-tokens.js @@ -0,0 +1,17 @@ +'use strict'; + +exports.up = function (db, cb) { + db.runSql( + `ALTER TABLE api_tokens DROP COLUMN metadata; + ALTER TABLE api_tokens ADD COLUMN alias text;`, + cb, + ); +}; + +exports.down = function (db, cb) { + db.runSql( + `ALTER TABLE api_tokens ADD COLUMN metadata JSONB NOT NULL DEFAULT '{}'::jsonb; + ALTER TABLE api_tokens DROP COLUMN alias;`, + cb, + ); +}; diff --git a/src/test/e2e/api/openapi/__snapshots__/openapi.e2e.test.ts.snap b/src/test/e2e/api/openapi/__snapshots__/openapi.e2e.test.ts.snap index 9f4d304572..ae38379ffc 100644 --- a/src/test/e2e/api/openapi/__snapshots__/openapi.e2e.test.ts.snap +++ b/src/test/e2e/api/openapi/__snapshots__/openapi.e2e.test.ts.snap @@ -199,6 +199,10 @@ Object { "apiTokenSchema": Object { "additionalProperties": false, "properties": Object { + "alias": Object { + "nullable": true, + "type": "string", + }, "createdAt": Object { "format": "date-time", "nullable": true, @@ -212,22 +216,6 @@ Object { "nullable": true, "type": "string", }, - "metadata": Object { - "additionalProperties": false, - "nullable": true, - "properties": Object { - "alias": Object { - "type": "string", - }, - "corsOrigins": Object { - "items": Object { - "type": "string", - }, - "type": "array", - }, - }, - "type": "object", - }, "project": Object { "type": "string", }, diff --git a/src/test/e2e/api/proxy/proxy.e2e.test.ts b/src/test/e2e/api/proxy/proxy.e2e.test.ts index 4811c9a6e5..817e3e441c 100644 --- a/src/test/e2e/api/proxy/proxy.e2e.test.ts +++ b/src/test/e2e/api/proxy/proxy.e2e.test.ts @@ -106,6 +106,89 @@ test('should not allow requests with a client token', async () => { .expect(403); }); +test('should allow requests with a token secret alias', async () => { + const featureA = randomId(); + const featureB = randomId(); + const envA = randomId(); + const envB = randomId(); + await db.stores.environmentStore.create({ name: envA, type: 'test' }); + await db.stores.environmentStore.create({ name: envB, type: 'test' }); + await db.stores.projectStore.addEnvironmentToProject('default', envA); + await db.stores.projectStore.addEnvironmentToProject('default', envB); + await createFeatureToggle({ + name: featureA, + enabled: true, + environment: envA, + strategies: [{ name: 'default', constraints: [], parameters: {} }], + }); + await createFeatureToggle({ + name: featureB, + enabled: true, + environment: envB, + strategies: [{ name: 'default', constraints: [], parameters: {} }], + }); + const tokenA = await createApiToken(ApiTokenType.FRONTEND, { + alias: randomId(), + environment: envA, + }); + const tokenB = await createApiToken(ApiTokenType.FRONTEND, { + alias: randomId(), + environment: envB, + }); + await app.request + .get('/api/frontend') + .expect('Content-Type', /json/) + .expect(401); + await app.request + .get('/api/frontend') + .set('Authorization', '') + .expect('Content-Type', /json/) + .expect(401); + await app.request + .get('/api/frontend') + .set('Authorization', 'null') + .expect('Content-Type', /json/) + .expect(401); + await app.request + .get('/api/frontend') + .set('Authorization', randomId()) + .expect('Content-Type', /json/) + .expect(401); + await app.request + .get('/api/frontend') + .set('Authorization', tokenA.secret.slice(0, -1)) + .expect('Content-Type', /json/) + .expect(401); + await app.request + .get('/api/frontend') + .set('Authorization', tokenA.secret) + .expect('Content-Type', /json/) + .expect(200) + .expect((res) => expect(res.body.toggles).toHaveLength(1)) + .expect((res) => expect(res.body.toggles[0].name).toEqual(featureA)); + await app.request + .get('/api/frontend') + .set('Authorization', tokenB.secret) + .expect('Content-Type', /json/) + .expect(200) + .expect((res) => expect(res.body.toggles).toHaveLength(1)) + .expect((res) => expect(res.body.toggles[0].name).toEqual(featureB)); + await app.request + .get('/api/frontend') + .set('Authorization', tokenA.alias) + .expect('Content-Type', /json/) + .expect(200) + .expect((res) => expect(res.body.toggles).toHaveLength(1)) + .expect((res) => expect(res.body.toggles[0].name).toEqual(featureA)); + await app.request + .get('/api/frontend') + .set('Authorization', tokenB.alias) + .expect('Content-Type', /json/) + .expect(200) + .expect((res) => expect(res.body.toggles).toHaveLength(1)) + .expect((res) => expect(res.body.toggles[0].name).toEqual(featureB)); +}); + test('should allow requests with an admin token', async () => { const adminToken = await createApiToken(ApiTokenType.ADMIN, { projects: ['*'], diff --git a/src/test/fixtures/fake-api-token-store.ts b/src/test/fixtures/fake-api-token-store.ts index cddc36eb3d..6064700eb8 100644 --- a/src/test/fixtures/fake-api-token-store.ts +++ b/src/test/fixtures/fake-api-token-store.ts @@ -53,8 +53,8 @@ export default class FakeApiTokenStore const apiToken = { createdAt: new Date(), project: newToken.projects?.join(',') || '*', + alias: null, ...newToken, - metadata: {}, }; this.tokens.push(apiToken); this.emit('insert');