From 6b8aef90a3c7f69504ab6d4abbeb9014b4b1f009 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fredrik=20Adel=C3=B6w?= Date: Wed, 12 Jun 2024 13:41:27 +0200 Subject: [PATCH] review fixes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Fredrik Adelöw --- .gitignore | 3 - docs/auth/service-to-service-auth.md | 5 +- packages/backend-app-api/config.d.ts | 6 +- .../keys/StaticConfigPluginKeySource.test.ts | 92 +++++++++++++------ .../keys/StaticConfigPluginKeySource.ts | 24 +++-- .../plugin/keys/createPluginKeySource.test.ts | 4 +- .../auth/plugin/keys/createPluginKeySource.ts | 2 +- 7 files changed, 87 insertions(+), 49 deletions(-) diff --git a/.gitignore b/.gitignore index 01c92d5cdb..5ba3da3148 100644 --- a/.gitignore +++ b/.gitignore @@ -170,6 +170,3 @@ knip.json # Schemathesis temporary files .hypothesis/ .cassettes/ - -# Locally generated keys -*.key diff --git a/docs/auth/service-to-service-auth.md b/docs/auth/service-to-service-auth.md index fe1b6e40bb..13516a1117 100644 --- a/docs/auth/service-to-service-auth.md +++ b/docs/auth/service-to-service-auth.md @@ -110,7 +110,8 @@ accordingly: ```yaml backend: auth: - keyStore: + # This is the new section for configuring plugin-to-plugin key storage + pluginKeyStore: type: static static: keys: @@ -128,7 +129,7 @@ The first entry will always be used for signing, but any of the subsequent entries will also be used for token validation. This lets you have a period of time where tokens signed by the previous top entry are still accepted by receivers, by just inserting your new key pair as the top entry and leaving the -old ones intact. +old ones intact. You can remove old private keys however; those won't be used. ## Static Tokens diff --git a/packages/backend-app-api/config.d.ts b/packages/backend-app-api/config.d.ts index 4ddc4a61db..a23bca447b 100644 --- a/packages/backend-app-api/config.d.ts +++ b/packages/backend-app-api/config.d.ts @@ -34,7 +34,7 @@ export interface Config { dangerouslyDisableDefaultAuthPolicy?: boolean; /** Controls how to store keys for plugin-to-plugin auth */ - keyStore?: + pluginKeyStore?: | { type: 'database' } | { type: 'static'; @@ -49,8 +49,10 @@ export interface Config { publicKeyFile: string; /** * Path to the matching private key file in the PKCS#8 format. Should be an absolute path. + * + * The first array entry must specify a private key file, the rest must not. */ - privateKeyFile: string; + privateKeyFile?: string; /** * ID to uniquely identify this key within the JWK set. */ diff --git a/packages/backend-app-api/src/services/implementations/auth/plugin/keys/StaticConfigPluginKeySource.test.ts b/packages/backend-app-api/src/services/implementations/auth/plugin/keys/StaticConfigPluginKeySource.test.ts index de96a150ec..4b3260cb79 100644 --- a/packages/backend-app-api/src/services/implementations/auth/plugin/keys/StaticConfigPluginKeySource.test.ts +++ b/packages/backend-app-api/src/services/implementations/auth/plugin/keys/StaticConfigPluginKeySource.test.ts @@ -15,7 +15,7 @@ */ import { StaticConfigPluginKeySource } from './StaticConfigPluginKeySource'; -import { Config, ConfigReader } from '@backstage/config'; +import { ConfigReader } from '@backstage/config'; import { createMockDirectory } from '@backstage/backend-test-utils'; const privateKey = ` @@ -34,43 +34,39 @@ ZoUEY72HTvjIP9xSbLOhWhMREk84T0q91/m3v3sWq5EzHIWw1zRHQisKZw== `.trim(); describe('StaticConfigPluginKeySource', () => { - let sourceConfig: Config; - const sourceDir = createMockDirectory(); - - beforeAll(() => { - sourceDir.setContent({ + const mockDir = createMockDirectory({ + content: { 'public.pem': publicKey, 'private.pem': privateKey, - }); - - const publicKeyPath = sourceDir.resolve('public.pem'); - const privateKeyPath = sourceDir.resolve('private.pem'); - sourceConfig = new ConfigReader({ - type: 'static', - static: { - keys: [ - { - publicKeyFile: publicKeyPath, - privateKeyFile: privateKeyPath, - keyId: '1', - algorithm: 'ES256', - }, - { - publicKeyFile: publicKeyPath, - privateKeyFile: privateKeyPath, - keyId: '2', - // skipping explicit alg - }, - ], - }, - }); + }, }); + const publicKeyPath = mockDir.resolve('public.pem'); + const privateKeyPath = mockDir.resolve('private.pem'); it('should provide keys from disk', async () => { const staticKeyStore = await StaticConfigPluginKeySource.create({ - sourceConfig, + sourceConfig: new ConfigReader({ + type: 'static', + static: { + keys: [ + { + publicKeyFile: publicKeyPath, + privateKeyFile: privateKeyPath, + keyId: '1', + algorithm: 'ES256', + }, + { + // intentionally only private key + // skipping explicit alg + publicKeyFile: publicKeyPath, + keyId: '2', + }, + ], + }, + }), keyDuration: { hours: 1 }, }); + const keys = await staticKeyStore.listKeys(); expect(keys.keys.length).toEqual(2); expect(keys.keys[0].key).toMatchObject({ @@ -89,4 +85,40 @@ describe('StaticConfigPluginKeySource', () => { }); expect(pk.d).toBeDefined(); }); + + it('throws an error for misconfigurations', async () => { + await expect( + StaticConfigPluginKeySource.create({ + sourceConfig: new ConfigReader({ + type: 'static', + static: { + keys: [], + }, + }), + keyDuration: { hours: 1 }, + }), + ).rejects.toThrowErrorMatchingInlineSnapshot( + `"At least one key pair must be provided in static.keys, when the static key store type is used"`, + ); + + await expect( + StaticConfigPluginKeySource.create({ + sourceConfig: new ConfigReader({ + type: 'static', + static: { + keys: [ + { + publicKeyFile: publicKeyPath, + keyId: '1', + algorithm: 'ES256', + }, + ], + }, + }), + keyDuration: { hours: 1 }, + }), + ).rejects.toThrowErrorMatchingInlineSnapshot( + `"Private key for signing must be provided in the first key pair in static.keys, when the static key store type is used"`, + ); + }); }); diff --git a/packages/backend-app-api/src/services/implementations/auth/plugin/keys/StaticConfigPluginKeySource.ts b/packages/backend-app-api/src/services/implementations/auth/plugin/keys/StaticConfigPluginKeySource.ts index 9fc46aec2e..658a88a368 100644 --- a/packages/backend-app-api/src/services/implementations/auth/plugin/keys/StaticConfigPluginKeySource.ts +++ b/packages/backend-app-api/src/services/implementations/auth/plugin/keys/StaticConfigPluginKeySource.ts @@ -24,13 +24,13 @@ import { PluginKeySource } from './types'; export type KeyPair = { publicKey: JWK; - privateKey: JWK; + privateKey?: JWK; keyId: string; }; export type StaticKeyConfig = { publicKeyFile: string; - privateKeyFile: string; + privateKeyFile?: string; keyId: string; algorithm: string; }; @@ -81,7 +81,7 @@ export class StaticConfigPluginKeySource implements PluginKeySource { .map(c => { const staticKeyConfig: StaticKeyConfig = { publicKeyFile: c.getString('publicKeyFile'), - privateKeyFile: c.getString('privateKeyFile'), + privateKeyFile: c.getOptionalString('privateKeyFile'), keyId: c.getString('keyId'), algorithm: c.getOptionalString('algorithm') ?? DEFAULT_ALGORITHM, }; @@ -97,6 +97,10 @@ export class StaticConfigPluginKeySource implements PluginKeySource { throw new Error( 'At least one key pair must be provided in static.keys, when the static key store type is used', ); + } else if (!keyPairs[0].privateKey) { + throw new Error( + 'Private key for signing must be provided in the first key pair in static.keys, when the static key store type is used', + ); } return new StaticConfigPluginKeySource( @@ -106,7 +110,7 @@ export class StaticConfigPluginKeySource implements PluginKeySource { } async getPrivateSigningKey(): Promise { - return this.keyPairs[0].privateKey; + return this.keyPairs[0].privateKey!; } async listKeys(): Promise<{ keys: KeyPayload[] }> { @@ -122,11 +126,13 @@ export class StaticConfigPluginKeySource implements PluginKeySource { keyId, algorithm, ); - const privateKey = await this.loadPrivateKeyFromFile( - options.privateKeyFile, - keyId, - algorithm, - ); + const privateKey = options.privateKeyFile + ? await this.loadPrivateKeyFromFile( + options.privateKeyFile, + keyId, + algorithm, + ) + : undefined; return { publicKey, privateKey, keyId }; } diff --git a/packages/backend-app-api/src/services/implementations/auth/plugin/keys/createPluginKeySource.test.ts b/packages/backend-app-api/src/services/implementations/auth/plugin/keys/createPluginKeySource.test.ts index 638e0a0e33..68ac322da5 100644 --- a/packages/backend-app-api/src/services/implementations/auth/plugin/keys/createPluginKeySource.test.ts +++ b/packages/backend-app-api/src/services/implementations/auth/plugin/keys/createPluginKeySource.test.ts @@ -73,7 +73,7 @@ describe('createPluginKeySource', () => { const source = await createPluginKeySource({ config: new ConfigReader({ - backend: { auth: { keyStore: { type: 'database' } } }, + backend: { auth: { pluginKeyStore: { type: 'database' } } }, }), database: mockServices.database.mock({ getClient }), logger: mockServices.logger.mock(), @@ -131,7 +131,7 @@ describe('createPluginKeySource', () => { config: new ConfigReader({ backend: { auth: { - keyStore: { + pluginKeyStore: { type: 'static', static: { keys: [ diff --git a/packages/backend-app-api/src/services/implementations/auth/plugin/keys/createPluginKeySource.ts b/packages/backend-app-api/src/services/implementations/auth/plugin/keys/createPluginKeySource.ts index dc756885cd..08cd1115a5 100644 --- a/packages/backend-app-api/src/services/implementations/auth/plugin/keys/createPluginKeySource.ts +++ b/packages/backend-app-api/src/services/implementations/auth/plugin/keys/createPluginKeySource.ts @@ -24,7 +24,7 @@ import { DatabasePluginKeySource } from './DatabasePluginKeySource'; import { StaticConfigPluginKeySource } from './StaticConfigPluginKeySource'; import { PluginKeySource } from './types'; -const CONFIG_ROOT_KEY = 'backend.auth.keyStore'; +const CONFIG_ROOT_KEY = 'backend.auth.pluginKeyStore'; export async function createPluginKeySource(options: { config: RootConfigService;