Merge pull request #11262 from backstage/freben/server-token-exp

Add an expiry time on server-to-server tokens

.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.

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<void>;
-  // (undocumented)
-  getToken: () => Promise<{
+  authenticate(token: string): Promise<void>;
+  getToken(): Promise<{
     token: string;
   }>;
 }

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(() => {

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<void>;
-  private logger: Logger;
+  private privateKeyPromise: Promise<void> | 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<void> {
     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}`);
   }
 }

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';

packages/backend-common/src/tokens/types.ts

@@ -20,6 +20,20 @@
  * @public
  */
 export interface TokenManager {
-  getToken: () => Promise<{ token: string }>;
-  authenticate: (token: string) => Promise<void>;
+  /**
+   * 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<void>;
 }