Merge pull request #24077 from backstage/rugvip/exp

backend-app-api: fix plugin token public key expiration

packages/backend-app-api/src/services/implementations/auth/PluginTokenHandler.test.ts

@@ -0,0 +1,82 @@
+/*
+ * Copyright 2024 The Backstage Authors
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+import { mockServices } from '@backstage/backend-test-utils';
+import { PluginTokenHandler } from './PluginTokenHandler';
+import { decodeJwt } from 'jose';
+
+describe('PluginTokenHandler', () => {
+  beforeEach(() => {
+    jest.useRealTimers();
+  });
+
+  it('issues token with correct expiration of token and generated key', async () => {
+    jest.useFakeTimers({
+      now: new Date(0),
+    });
+
+    const addKeyMock = jest.fn();
+    const handler = PluginTokenHandler.create({
+      discovery: mockServices.discovery(),
+      keyDurationSeconds: 10,
+      logger: mockServices.logger.mock(),
+      ownPluginId: 'test',
+      publicKeyStore: {
+        addKey: addKeyMock,
+        listKeys: jest.fn(),
+      },
+    });
+
+    const { token } = await handler.issueToken({
+      pluginId: 'test',
+      targetPluginId: 'other',
+    });
+    const payload = decodeJwt(token);
+    expect(payload.iat).toBe(0);
+    expect(payload.exp).toBe(10);
+
+    expect(addKeyMock).toHaveBeenCalledTimes(1);
+    expect(addKeyMock).toHaveBeenCalledWith({
+      id: expect.any(String),
+      key: expect.any(Object),
+      expiresAt: new Date(30_000),
+    });
+
+    jest.advanceTimersByTime(5_000);
+    await handler.issueToken({
+      pluginId: 'test',
+      targetPluginId: 'other',
+    });
+    expect(addKeyMock).toHaveBeenCalledTimes(1);
+
+    jest.advanceTimersByTime(10_000);
+    await handler.issueToken({
+      pluginId: 'test',
+      targetPluginId: 'other',
+    });
+    expect(addKeyMock).toHaveBeenCalledTimes(2);
+
+    expect(addKeyMock.mock.calls[0][0].id).not.toBe(
+      addKeyMock.mock.calls[1][0].id,
+    );
+
+    expect(addKeyMock).toHaveBeenNthCalledWith(2, {
+      id: expect.any(String),
+      key: expect.any(Object),
+      expiresAt: new Date(45_000),
+    });
+  });
+});

packages/backend-app-api/src/services/implementations/auth/PluginTokenHandler.ts

@@ -24,7 +24,6 @@ import {
   SignJWT,
   decodeProtectedHeader,
 } from 'jose';
-import { DateTime } from 'luxon';
 import { v4 as uuid } from 'uuid';
 import { InternalKey, KeyStore } from './types';
 import { AuthenticationError } from '@backstage/errors';
@@ -32,6 +31,13 @@ import { jwtVerify } from 'jose';
 import { tokenTypes } from '@backstage/plugin-auth-node';
 import { JwksClient } from './JwksClient';
 
+/**
+ * The margin for how many times longer we make the public key available
+ * compared to how long we use the private key to sign new tokens.
+ */
+const KEY_EXPIRATION_MARGIN_FACTOR = 3;
+const SECONDS_IN_MS = 1000;
+
 const ALLOWED_PLUGIN_ID_PATTERN = /^[a-z0-9_-]+$/i;
 
 type Options = {
@@ -71,12 +77,12 @@ export class PluginTokenHandler {
   }
 
   private constructor(
-    readonly logger: LoggerService,
-    readonly ownPluginId: string,
-    readonly publicKeyStore: KeyStore,
-    readonly keyDurationSeconds: number,
-    readonly algorithm: string,
-    readonly discovery: DiscoveryService,
+    private readonly logger: LoggerService,
+    private readonly ownPluginId: string,
+    private readonly publicKeyStore: KeyStore,
+    private readonly keyDurationSeconds: number,
+    private readonly algorithm: string,
+    private readonly discovery: DiscoveryService,
   ) {}
 
   async verifyToken(
@@ -129,10 +135,13 @@ export class PluginTokenHandler {
 
     const sub = pluginId;
     const aud = targetPluginId;
-    const iat = Math.floor(Date.now() / 1000);
+    const iat = Math.floor(Date.now() / SECONDS_IN_MS);
     const ourExp = iat + this.keyDurationSeconds;
     const exp = onBehalfOf
-      ? Math.min(ourExp, Math.floor(onBehalfOf.expiresAt.getTime() / 1000))
+      ? Math.min(
+          ourExp,
+          Math.floor(onBehalfOf.expiresAt.getTime() / SECONDS_IN_MS),
+        )
       : ourExp;
 
     const claims = { sub, aud, iat, exp, obo: onBehalfOf?.token };
@@ -223,22 +232,16 @@ export class PluginTokenHandler {
   private async getKey(): Promise<JWK> {
     // Make sure that we only generate one key at a time
     if (this.privateKeyPromise) {
-      if (
-        this.keyExpiry &&
-        DateTime.fromJSDate(this.keyExpiry) > DateTime.local()
-      ) {
+      if (this.keyExpiry && this.keyExpiry.getTime() > Date.now()) {
         return this.privateKeyPromise;
       }
       this.logger.info(`Signing key has expired, generating new key`);
       delete this.privateKeyPromise;
     }
 
-    const keyExpiry = DateTime.utc()
-      .plus({
-        seconds: this.keyDurationSeconds,
-      })
-      .toJSDate();
-    this.keyExpiry = keyExpiry;
+    this.keyExpiry = new Date(
+      Date.now() + this.keyDurationSeconds * SECONDS_IN_MS,
+    );
 
     const promise = (async () => {
       // This generates a new signing key to be used to sign tokens until the next key rotation
@@ -260,7 +263,12 @@ export class PluginTokenHandler {
       await this.publicKeyStore.addKey({
         id: kid,
         key: publicKey as InternalKey,
-        expiresAt: keyExpiry,
+        expiresAt: new Date(
+          Date.now() +
+            this.keyDurationSeconds *
+              SECONDS_IN_MS *
+              KEY_EXPIRATION_MARGIN_FACTOR,
+        ),
       });
 
       // At this point we are allowed to start using the new key