diff --git a/.changeset/olive-eggs-accept.md b/.changeset/olive-eggs-accept.md index 81b1069f15..66d44d9c3e 100644 --- a/.changeset/olive-eggs-accept.md +++ b/.changeset/olive-eggs-accept.md @@ -1,10 +1,13 @@ --- -'@backstage/backend-common': minor +'@backstage/backend-common': patch --- -**BREAKING**: Server-to-server authentication tokens issued from a +**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. +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 diff --git a/packages/backend-common/src/tokens/ServerTokenManager.test.ts b/packages/backend-common/src/tokens/ServerTokenManager.test.ts index 30d3571606..6a72b99908 100644 --- a/packages/backend-common/src/tokens/ServerTokenManager.test.ts +++ b/packages/backend-common/src/tokens/ServerTokenManager.test.ts @@ -171,6 +171,7 @@ 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, @@ -178,12 +179,14 @@ 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); @@ -191,15 +194,15 @@ 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, the newest one is still valid + // 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), - ).rejects.toThrowErrorMatchingInlineSnapshot( - '"Invalid server token: JWTExpired: \\"exp\\" claim timestamp check failed"', - ); + 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"', + ); }); }); diff --git a/packages/backend-common/src/tokens/ServerTokenManager.ts b/packages/backend-common/src/tokens/ServerTokenManager.ts index 1915330d26..329c2f164c 100644 --- a/packages/backend-common/src/tokens/ServerTokenManager.ts +++ b/packages/backend-common/src/tokens/ServerTokenManager.ts @@ -64,6 +64,8 @@ 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 @@ -179,17 +181,40 @@ export class ServerTokenManager implements TokenManager { for (const key of this.verificationKeys) { try { - const result = await jwtVerify(token, key); - if (result.protectedHeader.alg !== TOKEN_ALG) { - throw new NotAllowedError( - `Illegal alg "${result.protectedHeader.alg}"`, - ); + 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 (result.payload.sub !== TOKEN_SUB) { - throw new NotAllowedError(`Illegal sub "${result.payload.sub}"`); + + 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`, + ); + } } - // 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