diff --git a/.changeset/olive-eggs-accept.md b/.changeset/olive-eggs-accept.md new file mode 100644 index 0000000000..81b1069f15 --- /dev/null +++ b/.changeset/olive-eggs-accept.md @@ -0,0 +1,13 @@ +--- +'@backstage/backend-common': minor +--- + +**BREAKING**: Server-to-server authentication tokens issued from a +`TokenManager` (specifically, `ServerTokenManager`) now has an expiry time set, +for one hour in the future from when issued. This improves security. + +It was always the case that users of this interface were expected to call its +`getToken()` method for every outgoing call and never hold on to any given token +for reuse. But this now has become even more important advice to heed, and you +should verify that you do not hold on to and reuse tokens such as these in your +own code. diff --git a/packages/backend-common/api-report.md b/packages/backend-common/api-report.md index 961973b144..a66c8de735 100644 --- a/packages/backend-common/api-report.md +++ b/packages/backend-common/api-report.md @@ -599,18 +599,20 @@ export class ServerTokenManager implements TokenManager { // (undocumented) static fromConfig( config: Config, - options: { - logger: Logger; - }, + options: ServerTokenManagerOptions, ): ServerTokenManager; // (undocumented) getToken(): Promise<{ token: string; }>; - // (undocumented) static noop(): TokenManager; } +// @public +export interface ServerTokenManagerOptions { + logger: Logger; +} + // @public export type ServiceBuilder = { loadConfig(config: Config): ServiceBuilder; @@ -669,10 +671,8 @@ export interface StatusCheckHandlerOptions { // @public export interface TokenManager { - // (undocumented) - authenticate: (token: string) => Promise; - // (undocumented) - getToken: () => Promise<{ + authenticate(token: string): Promise; + getToken(): Promise<{ token: string; }>; } diff --git a/packages/backend-common/src/tokens/ServerTokenManager.test.ts b/packages/backend-common/src/tokens/ServerTokenManager.test.ts index 0ff7bfb316..57435fe486 100644 --- a/packages/backend-common/src/tokens/ServerTokenManager.test.ts +++ b/packages/backend-common/src/tokens/ServerTokenManager.test.ts @@ -13,11 +13,12 @@ * See the License for the specific language governing permissions and * limitations under the License. */ -import { getVoidLogger } from '../logging/voidLogger'; + import { ConfigReader } from '@backstage/config'; -import { ServerTokenManager } from './ServerTokenManager'; -import { Logger } from 'winston'; import * as jose from 'jose'; +import { Logger } from 'winston'; +import { getVoidLogger } from '../logging/voidLogger'; +import { ServerTokenManager } from './ServerTokenManager'; import { TokenManager } from './types'; const emptyConfig = new ConfigReader({}); @@ -35,6 +36,7 @@ describe('ServerTokenManager', () => { afterEach(() => { process.env = env; + jest.useRealTimers(); }); describe('getToken', () => { @@ -166,6 +168,28 @@ describe('ServerTokenManager', () => { /invalid server token/i, ); }); + + it('should throw for expired tokens', async () => { + jest.useFakeTimers('modern'); + jest.setSystemTime(new Date('2020-02-02T02:00:00.0000000Z')); + + const tokenManager = ServerTokenManager.fromConfig(configWithSecret, { + logger, + }); + + const { token } = await tokenManager.getToken(); + await expect(tokenManager.authenticate(token)).resolves.not.toThrow(); + + jest.setSystemTime(new Date('2020-02-02T02:59:00.0000000Z')); + await expect(tokenManager.authenticate(token)).resolves.not.toThrow(); + + jest.setSystemTime(new Date('2020-02-02T03:00:01.0000000Z')); + await expect( + tokenManager.authenticate(token), + ).rejects.toThrowErrorMatchingInlineSnapshot( + '"Invalid server token: JWTExpired: \\"exp\\" claim timestamp check failed"', + ); + }); }); describe('fromConfig', () => { @@ -242,7 +266,7 @@ describe('ServerTokenManager', () => { }); }); - describe('ServerTokenManager.noop', () => { + describe('noop', () => { let noopTokenManager: TokenManager; beforeEach(() => { diff --git a/packages/backend-common/src/tokens/ServerTokenManager.ts b/packages/backend-common/src/tokens/ServerTokenManager.ts index 484b99ca4d..ba6a276421 100644 --- a/packages/backend-common/src/tokens/ServerTokenManager.ts +++ b/packages/backend-common/src/tokens/ServerTokenManager.ts @@ -14,12 +14,19 @@ * limitations under the License. */ -import { base64url, generateSecret, SignJWT, jwtVerify, exportJWK } from 'jose'; import { Config } from '@backstage/config'; -import { AuthenticationError } from '@backstage/errors'; -import { TokenManager } from './types'; +import { AuthenticationError, NotAllowedError } from '@backstage/errors'; +import { base64url, exportJWK, generateSecret, jwtVerify, SignJWT } from 'jose'; import { Logger } from 'winston'; +import { TokenManager } from './types'; +const TOKEN_ALG = 'HS256'; +const TOKEN_SUB = 'backstage-server'; + +/** + * A token manager that issues static dummy tokens and never fails + * authentication. This can be useful for testing. + */ class NoopTokenManager implements TokenManager { public readonly isInsecureServerTokenManager: boolean = true; @@ -30,6 +37,18 @@ class NoopTokenManager implements TokenManager { async authenticate() {} } +/** + * Options for {@link ServerTokenManager}. + * + * @public + */ +export interface ServerTokenManagerOptions { + /** + * The logger to use. + */ + logger: Logger; +} + /** * Creates and validates tokens for use during backend-to-backend * authentication. @@ -37,44 +56,48 @@ class NoopTokenManager implements TokenManager { * @public */ export class ServerTokenManager implements TokenManager { + private readonly options: ServerTokenManagerOptions; private verificationKeys: Uint8Array[]; private signingKey: Uint8Array; private privateKeyPromise?: Promise; - private logger: Logger; + /** + * Creates a token manager that issues static dummy tokens and never fails + * authentication. This can be useful for testing. + */ static noop(): TokenManager { return new NoopTokenManager(); } - static fromConfig(config: Config, options: { logger: Logger }) { - const { logger } = options; - + static fromConfig(config: Config, options: ServerTokenManagerOptions) { const keys = config.getOptionalConfigArray('backend.auth.keys'); if (keys?.length) { return new ServerTokenManager( keys.map(key => key.getString('secret')), - logger, + options, ); } + if (process.env.NODE_ENV !== 'development') { throw new Error( 'You must configure at least one key in backend.auth.keys for production.', ); } + // For development, if a secret has not been configured, we auto generate a secret instead of throwing. - logger.warn( + options.logger.warn( 'Generated a secret for backend-to-backend authentication: DEVELOPMENT USE ONLY.', ); - return new ServerTokenManager([], logger); + return new ServerTokenManager([], options); } - private constructor(secrets: string[], logger: Logger) { + private constructor(secrets: string[], options: ServerTokenManagerOptions) { if (!secrets.length && process.env.NODE_ENV !== 'development') { throw new Error( 'No secrets provided when constructing ServerTokenManager', ); } - this.logger = logger; + this.options = options; this.verificationKeys = secrets.map(s => base64url.decode(s)); this.signingKey = this.verificationKeys[0]; } @@ -86,11 +109,13 @@ export class ServerTokenManager implements TokenManager { 'Key generation is not supported outside of the dev environment', ); } + if (this.privateKeyPromise) { return this.privateKeyPromise; } + const promise = (async () => { - const secret = await generateSecret('HS256'); + const secret = await generateSecret(TOKEN_ALG); const jwk = await exportJWK(secret); this.verificationKeys.push(base64url.decode(jwk.k ?? '')); this.signingKey = this.verificationKeys[0]; @@ -98,13 +123,15 @@ export class ServerTokenManager implements TokenManager { })(); try { - // If we fail to generate a new key, we need to clear the state so that - // the next caller will try to generate another key. + this.privateKeyPromise = promise; await promise; } catch (error) { - this.logger.error(`Failed to generate new key, ${error}`); + // If we fail to generate a new key, we need to clear the state so that + // the next caller will try to generate another key. + this.options.logger.error(`Failed to generate new key, ${error}`); delete this.privateKeyPromise; } + return promise; } @@ -112,26 +139,39 @@ export class ServerTokenManager implements TokenManager { if (!this.verificationKeys.length) { await this.generateKeys(); } - const sub = 'backstage-server'; - const jwt = await new SignJWT({ alg: 'HS256' }) - .setProtectedHeader({ alg: 'HS256', sub: sub }) - .setSubject('backstage-server') + + const jwt = await new SignJWT({}) + .setProtectedHeader({ alg: TOKEN_ALG }) + .setSubject(TOKEN_SUB) + .setExpirationTime('1h') .sign(this.signingKey); + return { token: jwt }; } async authenticate(token: string): Promise { let verifyError = undefined; + for (const key of this.verificationKeys) { try { - await jwtVerify(token, key); - // If the verify succeeded, return + const result = await jwtVerify(token, key); + if (result.protectedHeader.alg !== TOKEN_ALG) { + throw new NotAllowedError( + `Illegal alg "${result.protectedHeader.alg}"`, + ); + } + if (result.payload.sub !== TOKEN_SUB) { + throw new NotAllowedError(`Illegal sub "${result.payload.sub}"`); + } + // TODO(freben): Reject missing payload.exp in the future as well + // The jose library does NOT throw if exp is not set in the token, but DOES throw if exp is set and expired return; } catch (e) { // Catch the verify exception and continue verifyError = e; } } + throw new AuthenticationError(`Invalid server token: ${verifyError}`); } } diff --git a/packages/backend-common/src/tokens/index.ts b/packages/backend-common/src/tokens/index.ts index 43ff12e597..fe1b56df80 100644 --- a/packages/backend-common/src/tokens/index.ts +++ b/packages/backend-common/src/tokens/index.ts @@ -15,4 +15,5 @@ */ export { ServerTokenManager } from './ServerTokenManager'; +export type { ServerTokenManagerOptions } from './ServerTokenManager'; export type { TokenManager } from './types'; diff --git a/packages/backend-common/src/tokens/types.ts b/packages/backend-common/src/tokens/types.ts index 1fea018db9..2fa771b7e2 100644 --- a/packages/backend-common/src/tokens/types.ts +++ b/packages/backend-common/src/tokens/types.ts @@ -20,6 +20,20 @@ * @public */ export interface TokenManager { - getToken: () => Promise<{ token: string }>; - authenticate: (token: string) => Promise; + /** + * Fetches a valid token. + * + * @remarks + * + * Tokens are valid for roughly one hour; the actual deadline is set in the + * payload `exp` claim. Never hold on to tokens for reuse; always ask for a + * new one for each outgoing request. This ensures that you always get a + * valid, fresh one. + */ + getToken(): Promise<{ token: string }>; + + /** + * Validates a given token. + */ + authenticate(token: string): Promise; }