diff --git a/.changeset/olive-eggs-accept.md b/.changeset/olive-eggs-accept.md new file mode 100644 index 0000000000..66d44d9c3e --- /dev/null +++ b/.changeset/olive-eggs-accept.md @@ -0,0 +1,16 @@ +--- +'@backstage/backend-common': patch +--- + +**DEPRECATION**: 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. The ability +to pass in and validate tokens that either have a missing `exp` claim, or an +`exp` claim that expired in the past, is now deprecated. Trying to do so will +lead to logged warnings, and in a future release will instead lead to errors. + +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 13c4ed62ce..6059e68792 100644 --- a/packages/backend-common/api-report.md +++ b/packages/backend-common/api-report.md @@ -617,18 +617,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; @@ -687,10 +689,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..6a72b99908 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,42 @@ describe('ServerTokenManager', () => { /invalid server token/i, ); }); + + it('should throw for expired tokens, and re-issue new ones', async () => { + jest.useFakeTimers(); + const warn = jest.spyOn(logger, 'warn'); + + const tokenManager = ServerTokenManager.fromConfig(configWithSecret, { + logger, + }); + + const { token: token1 } = await tokenManager.getToken(); + await expect(tokenManager.authenticate(token1)).resolves.not.toThrow(); + expect(warn.mock.calls.length).toBe(0); + + // Right before the reissue timeout, it still returns the same token + jest.advanceTimersByTime(9 * 60 * 1000); + const { token: token1Again } = await tokenManager.getToken(); + expect(token1).toEqual(token1Again); + await expect(tokenManager.authenticate(token1)).resolves.not.toThrow(); + expect(warn.mock.calls.length).toBe(0); + + // Right after the reissue timeout, the old ones are still valid but returning a new token + jest.advanceTimersByTime(2 * 60 * 1000); + const { token: token2 } = await tokenManager.getToken(); + expect(token1).not.toEqual(token2); + await expect(tokenManager.authenticate(token1)).resolves.not.toThrow(); + await expect(tokenManager.authenticate(token2)).resolves.not.toThrow(); + expect(warn.mock.calls.length).toBe(0); + + // After expiry of the first one, it gets warnings but the newest one is still valid + jest.advanceTimersByTime(52 * 60 * 1000); + await expect(tokenManager.authenticate(token1)).resolves.not.toThrow(); + await expect(tokenManager.authenticate(token2)).resolves.not.toThrow(); + expect(warn.mock.calls[0][0]).toMatchInlineSnapshot( + '"#### DEPRECATION WARNING: #### Server-to-server token had an expired exp claim, support for this has been deprecated and will result in errors in a future release"', + ); + }); }); describe('fromConfig', () => { @@ -242,7 +280,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..329c2f164c 100644 --- a/packages/backend-common/src/tokens/ServerTokenManager.ts +++ b/packages/backend-common/src/tokens/ServerTokenManager.ts @@ -14,12 +14,22 @@ * 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 { DateTime, Duration } from 'luxon'; import { Logger } from 'winston'; +import { TokenManager } from './types'; +const TOKEN_ALG = 'HS256'; +const TOKEN_SUB = 'backstage-server'; +const TOKEN_EXPIRY_AFTER = Duration.fromObject({ hours: 1 }); +const TOKEN_REISSUE_AFTER = Duration.fromObject({ minutes: 10 }); + +/** + * 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 +40,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 +59,51 @@ class NoopTokenManager implements TokenManager { * @public */ export class ServerTokenManager implements TokenManager { - private verificationKeys: Uint8Array[]; + private readonly options: ServerTokenManagerOptions; + private readonly verificationKeys: Uint8Array[]; private signingKey: Uint8Array; - private privateKeyPromise?: Promise; - private logger: Logger; + private privateKeyPromise: Promise | undefined; + private currentTokenPromise: Promise<{ token: string }> | undefined; + private warnedForMissingExpClaim = false; + private warnedForExpiredExpClaim = false; + /** + * 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 +115,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 +129,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 +145,83 @@ 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') - .sign(this.signingKey); - return { token: jwt }; + + if (this.currentTokenPromise) { + return this.currentTokenPromise; + } + + const result = Promise.resolve().then(async () => { + const jwt = await new SignJWT({}) + .setProtectedHeader({ alg: TOKEN_ALG }) + .setSubject(TOKEN_SUB) + .setExpirationTime( + DateTime.now().plus(TOKEN_EXPIRY_AFTER).toUnixInteger(), + ) + .sign(this.signingKey); + return { token: jwt }; + }); + + this.currentTokenPromise = result; + + result + .then(() => { + setTimeout(() => { + this.currentTokenPromise = undefined; + }, TOKEN_REISSUE_AFTER.toMillis()); + }) + .catch(() => { + this.currentTokenPromise = undefined; + }); + + return result; } 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 { + protectedHeader: { alg }, + payload: { sub, exp }, + } = await jwtVerify(token, key, { + // TODO(freben): Holding on to tokens and reusing them is deprecated; remove this tolerance in a future release + clockTolerance: 3e9, + }); + + if (alg !== TOKEN_ALG) { + throw new NotAllowedError(`Illegal alg "${alg}"`); + } + + if (sub !== TOKEN_SUB) { + throw new NotAllowedError(`Illegal sub "${sub}"`); + } + + // TODO(freben): Passing in tokens without an exp is deprecated; change this warning to an error in a future release + if (typeof exp !== 'number') { + if (!this.warnedForMissingExpClaim) { + this.warnedForMissingExpClaim = true; + this.options.logger.warn( + `#### DEPRECATION WARNING: #### Server-to-server token had no exp claim, support for this has been deprecated and will result in errors in a future release`, + ); + } + } + // TODO(freben): Holding on to tokens and reusing them is deprecated; remove this tolerance in a future release + else if (exp * 1000 < Date.now()) { + if (!this.warnedForExpiredExpClaim) { + this.warnedForExpiredExpClaim = true; + this.options.logger.warn( + `#### DEPRECATION WARNING: #### Server-to-server token had an expired exp claim, support for this has been deprecated and will result in errors in a future release`, + ); + } + } 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; }