From 55647ec7df87d52bc87b4ddab7ed0da9644b756f Mon Sep 17 00:00:00 2001 From: Patrik Oldsberg Date: Thu, 2 Jun 2022 13:23:35 +0200 Subject: [PATCH] backend-common: prevent expired backend tokens from being used Signed-off-by: Patrik Oldsberg --- .changeset/metal-items-mix.md | 5 +++ .../src/tokens/ServerTokenManager.test.ts | 44 ++++++++++++++++--- .../src/tokens/ServerTokenManager.ts | 34 ++++---------- 3 files changed, 50 insertions(+), 33 deletions(-) create mode 100644 .changeset/metal-items-mix.md diff --git a/.changeset/metal-items-mix.md b/.changeset/metal-items-mix.md new file mode 100644 index 0000000000..6299e479e6 --- /dev/null +++ b/.changeset/metal-items-mix.md @@ -0,0 +1,5 @@ +--- +'@backstage/backend-common': minor +--- + +**BREAKING**: Server-to-server tokens that are authenticated by the `ServerTokenManager` now must have an `exp` claim that has not expired. Tokens where the `exp` claim is in the past or missing are considered invalid and will throw an error. This is a followup to the deprecation from the `1.2` release of Backstage where perpetual tokens were deprecated. Be sure to update any usage of the `getToken()` method to have it be called every time a token is needed. Do not store tokens for later use. diff --git a/packages/backend-common/src/tokens/ServerTokenManager.test.ts b/packages/backend-common/src/tokens/ServerTokenManager.test.ts index 6a72b99908..cc7854b2ee 100644 --- a/packages/backend-common/src/tokens/ServerTokenManager.test.ts +++ b/packages/backend-common/src/tokens/ServerTokenManager.test.ts @@ -171,7 +171,6 @@ describe('ServerTokenManager', () => { 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, @@ -179,14 +178,12 @@ describe('ServerTokenManager', () => { 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); @@ -194,14 +191,47 @@ describe('ServerTokenManager', () => { 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(token1)).rejects.toThrow( + 'Invalid server token; caused by JWTExpired: "exp" claim timestamp check failed', + ); 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"', + }); + + it('should work with a manually crafted JWT', async () => { + const secret = 'a1b2c3'; + const token = await new jose.SignJWT({}) + .setProtectedHeader({ alg: 'HS256' }) + .setSubject('backstage-server') + .setExpirationTime(Date.now() + 1000 * 60 * 60) + .sign(jose.base64url.decode(secret)); + + const tokenManager = ServerTokenManager.fromConfig( + new ConfigReader({ + backend: { auth: { keys: [{ secret }] } }, + }), + { logger }, + ); + await expect(tokenManager.authenticate(token)).resolves.toBeUndefined(); + }); + + it('should reject tokens without exp claim', async () => { + const secret = 'a1b2c3'; + const token = await new jose.SignJWT({}) + .setProtectedHeader({ alg: 'HS256' }) + .setSubject('backstage-server') + .sign(jose.base64url.decode(secret)); + + const tokenManager = ServerTokenManager.fromConfig( + new ConfigReader({ + backend: { auth: { keys: [{ secret }] } }, + }), + { logger }, + ); + await expect(tokenManager.authenticate(token)).rejects.toThrow( + 'Invalid server token; caused by AuthenticationError: Server-to-server token had no exp claim', ); }); }); diff --git a/packages/backend-common/src/tokens/ServerTokenManager.ts b/packages/backend-common/src/tokens/ServerTokenManager.ts index 329c2f164c..970dcfb83d 100644 --- a/packages/backend-common/src/tokens/ServerTokenManager.ts +++ b/packages/backend-common/src/tokens/ServerTokenManager.ts @@ -15,7 +15,7 @@ */ import { Config } from '@backstage/config'; -import { AuthenticationError, NotAllowedError } from '@backstage/errors'; +import { AuthenticationError } from '@backstage/errors'; import { base64url, exportJWK, generateSecret, jwtVerify, SignJWT } from 'jose'; import { DateTime, Duration } from 'luxon'; import { Logger } from 'winston'; @@ -64,8 +64,6 @@ export class ServerTokenManager implements TokenManager { private signingKey: Uint8Array; 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 @@ -184,36 +182,20 @@ export class ServerTokenManager implements TokenManager { 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, - }); + } = await jwtVerify(token, key); if (alg !== TOKEN_ALG) { - throw new NotAllowedError(`Illegal alg "${alg}"`); + throw new AuthenticationError(`Illegal alg "${alg}"`); } if (sub !== TOKEN_SUB) { - throw new NotAllowedError(`Illegal sub "${sub}"`); + throw new AuthenticationError(`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`, - ); - } + throw new AuthenticationError( + 'Server-to-server token had no exp claim', + ); } return; } catch (e) { @@ -222,6 +204,6 @@ export class ServerTokenManager implements TokenManager { } } - throw new AuthenticationError(`Invalid server token: ${verifyError}`); + throw new AuthenticationError('Invalid server token', verifyError); } }