fix(auth): resolve JWKS URL from the OIDC discovery document

docs/adr/0006-oidc-issuer-is-configurable-signature-verification-stays-mandatory.md

@@ -7,3 +7,7 @@ status: proposed
 GitHub Actions cache tokens are verified in `lib/scope.ts` against a hard-coded issuer and JWKS endpoint (`https://token.actions.githubusercontent.com`), which is correct for github.com but rejects every token issued by a GitHub Enterprise Server instance, whose Actions tokens carry the GHES host as their issuer. To support GHES (best-effort, per issue #241) we make the issuer configurable via a single env var defaulting to the current github.com value, and derive the JWKS URL from it as `{issuer}/.well-known/jwks` — the layout GHES inherits from the same Actions stack. github.com users change nothing.
 
 We rejected the zero-code alternative of telling GHES operators to set `SKIP_TOKEN_VALIDATION=true`, because that flag disables signature verification entirely: any client that can reach the server could then forge scopes and read or poison any repository's cache. `SKIP_TOKEN_VALIDATION` stays what it is — a dev/test escape hatch, explicitly not a production deployment path. Keeping verification mandatory means a GHES deployment is exactly as authenticated as a github.com one, at the cost of the operator supplying their issuer. We deliberately derive the JWKS URL rather than accept it as a second var; if a real GHES layout ever splits the JWKS host from the issuer host, an explicit override can be added then.
+
+**Amended (issue #253):** such a layout exists, so we no longer derive the JWKS URL — we discover it. GitHub Enterprise Cloud lets an admin customize the issuer to `https://token.actions.githubusercontent.com/<enterpriseSlug>` while the JWKS stays at `https://token.actions.githubusercontent.com/.well-known/jwks`; the derived URL 404s and every token fails verification. The OIDC discovery document at `{issuer}/.well-known/openid-configuration` names the right `jwks_uri` in every case we can observe, including the custom-issuer one, so it is the default source of truth and this class of bug can't recur for any layout that publishes discovery.
+
+Discovery is fetched once, lazily, on the first token verification, and cached for the process. When it fails we log a warning and fall back to the previously derived `{issuer}/.well-known/jwks` — deliberately _without_ caching, so a discovery outage can't pin the process to a URL that may be wrong. The fallback keeps GHES deployments (whose token hosts we can't reach to confirm they serve discovery) working exactly as before, so the change can't regress the support added above. `ACTIONS_TOKEN_JWKS_URL` remains as an explicit override that skips discovery entirely, for operators whose deployment serves neither a usable discovery document nor the conventional path.

lib/db.ts

@@ -81,6 +81,37 @@ export interface Database {
 
 const dbLogger = logger.withTag('db')
 
+// mysql2 `ER_LOCK_DEADLOCK` / `ER_LOCK_WAIT_TIMEOUT`, and the SQL states for
+// postgres `deadlock_detected` / `serialization_failure`.
+const RETRYABLE_LOCK_ERRORS = new Set([
+  'ER_LOCK_DEADLOCK',
+  'ER_LOCK_WAIT_TIMEOUT',
+  '40001',
+  '40P01',
+])
+
+function isRetryableLockError(err: unknown) {
+  const code = (err as { code?: unknown } | null)?.code
+  return typeof code === 'string' && RETRYABLE_LOCK_ERRORS.has(code)
+}
+
+/**
+ * Runs a transaction, retrying it whole if the database picks it as a deadlock
+ * victim. Concurrent writers take row locks on `storage_locations` and the lease
+ * tables in differing orders, so a deadlock is expected rather than exceptional
+ * — the loser has to start over. Only wrap transactions that are safe to repeat.
+ */
+export async function retryOnLockConflict<T>(run: () => Promise<T>, attempts = 3) {
+  for (let attempt = 1; ; attempt++) {
+    try {
+      return await run()
+    } catch (err) {
+      if (attempt >= attempts || !isRetryableLockError(err)) throw err
+      dbLogger.warn(`Retrying transaction after lock conflict (attempt ${attempt})`, { error: err })
+    }
+  }
+}
+
 export const getDatabase = createSingletonPromise(async () => {
   if (process.env.NODE_CAGED === 'true' && env.DB_DRIVER === 'sqlite')
     throw new Error('SQLite is not supported with `caged` image variant.')

lib/schemas.ts

@@ -61,6 +61,7 @@ export const envBaseSchema = type({
   'DEFAULT_ACTIONS_RESULTS_URL':
     "string.url = 'https://results-receiver.actions.githubusercontent.com'",
   'ACTIONS_TOKEN_ISSUER': "string.url = 'https://token.actions.githubusercontent.com'",
+  'ACTIONS_TOKEN_JWKS_URL?': 'string.url',
   'CACHE_CLEANUP_OLDER_THAN_DAYS': 'number = 90',
   'CACHE_MAX_SIZE_BYTES?': 'number.integer > 0',
   'CACHE_FILESYSTEM_MAX_USAGE_PERCENT': 'number > 0 & number <= 100 = 90',

lib/scope.ts

@@ -4,11 +4,52 @@ import { hasAtLeast } from 'remeda'
 import { env } from './env'
 import { logger } from './logger'
 
-// ponytail: JWKS URL derived from the issuer as GitHub (and GHES, sharing the
-// same Actions stack) serves it at `{issuer}/.well-known/jwks`. Add an explicit
-// JWKS override var if a real GHES layout ever splits the JWKS host from the issuer.
 const issuer = env.ACTIONS_TOKEN_ISSUER.replace(/\/$/, '')
-const JWKS = jose.createRemoteJWKSet(new URL(`${issuer}/.well-known/jwks`))
+
+const fallbackJwksUrl = `${issuer}/.well-known/jwks`
+
+/**
+ * The JWKS URL can't be derived from the issuer: an enterprise with a custom
+ * issuer value (`{host}/{enterpriseSlug}`) still serves its JWKS at `{host}`.
+ * Ask the OIDC discovery document instead.
+ */
+export async function discoverJwksUrl() {
+  const discoveryUrl = `${issuer}/.well-known/openid-configuration`
+
+  const res = await fetch(discoveryUrl)
+  if (!res.ok) throw new Error(`Unexpected status ${res.status} ${res.statusText}`)
+
+  const config = (await res.json()) as { jwks_uri?: unknown }
+  if (typeof config.jwks_uri !== 'string')
+    throw new Error(`Discovery document at ${discoveryUrl} has no \`jwks_uri\``)
+
+  return config.jwks_uri
+}
+
+const createJwks = (url: string) => jose.createRemoteJWKSet(new URL(url))
+
+const overrideJwks = env.ACTIONS_TOKEN_JWKS_URL ? createJwks(env.ACTIONS_TOKEN_JWKS_URL) : undefined
+const fallbackJwks = createJwks(fallbackJwksUrl)
+// holder object instead of a bare `let`, which can't be assigned to from inside
+// a function (`unicorn/no-top-level-assignment-in-function`)
+const cache: { jwks?: jose.JWTVerifyGetKey } = {}
+
+async function getJwks() {
+  if (overrideJwks) return overrideJwks
+  if (cache.jwks) return cache.jwks
+
+  try {
+    return (cache.jwks = createJwks(await discoverJwksUrl()))
+  } catch (err) {
+    logger.warn(
+      `OIDC discovery failed, falling back to ${fallbackJwksUrl}. Set ACTIONS_TOKEN_JWKS_URL if token validation keeps failing.`,
+      err,
+    )
+    // Deliberately not cached, so the next request retries discovery instead of
+    // pinning the process to a URL that may well be the wrong one.
+    return fallbackJwks
+  }
+}
 
 function getBearerToken(event: H3Event) {
   const authHeader = getHeader(event, 'authorization')
@@ -23,11 +64,8 @@ async function verifyGitHubActionsToken(token: string) {
     return jose.decodeJwt(token)
   }
 
-  return jose
-    .jwtVerify(token, JWKS, {
-      issuer: env.ACTIONS_TOKEN_ISSUER,
-    })
-    .then((res) => res.payload)
+  const { payload } = await jose.jwtVerify(token, await getJwks(), { issuer })
+  return payload
 }
 
 function parseJsonScopes(json: string) {

lib/storage.ts

@@ -27,7 +27,7 @@ import { NodeHttpHandler } from '@smithy/node-http-handler'
 import { sql } from 'kysely'
 import { chunk } from 'remeda'
 import { match } from 'ts-pattern'
-import { getDatabase } from './db'
+import { getDatabase, retryOnLockConflict } from './db'
 import { env } from './env'
 import { generateNumberId } from './helpers'
 import { logger } from './logger'
@@ -475,21 +475,25 @@ export class Storage {
       const mergePromise = this.adapter
         .uploadStream(`${storageLocation.folderName}/merged`, mergerStream)
         .then(async () => {
-          await this.db.transaction().execute(async (tx) => {
-            let leaseQuery = tx
-              .selectFrom('merge_leases')
-              .select(['token', 'expiresAt'])
-              .where('storageLocationId', '=', storageLocation.id)
-            if (env.DB_DRIVER !== 'sqlite') leaseQuery = leaseQuery.forUpdate()
-            const lease = await leaseQuery.executeTakeFirst()
-            if (lease?.token !== mergeToken || lease.expiresAt <= Date.now())
-              throw new Error('Merge lease was lost before completion')
-            await tx
-              .updateTable('storage_locations')
-              .set({ mergedAt: Date.now() })
-              .where('id', '=', storageLocation.id)
-              .execute()
-          })
+          // The merged object is already written, so losing a deadlock here must
+          // not throw the merge away — the fence is re-checked on every attempt.
+          await retryOnLockConflict(() =>
+            this.db.transaction().execute(async (tx) => {
+              let leaseQuery = tx
+                .selectFrom('merge_leases')
+                .select(['token', 'expiresAt'])
+                .where('storageLocationId', '=', storageLocation.id)
+              if (env.DB_DRIVER !== 'sqlite') leaseQuery = leaseQuery.forUpdate()
+              const lease = await leaseQuery.executeTakeFirst()
+              if (lease?.token !== mergeToken || lease.expiresAt <= Date.now())
+                throw new Error('Merge lease was lost before completion')
+              await tx
+                .updateTable('storage_locations')
+                .set({ mergedAt: Date.now() })
+                .where('id', '=', storageLocation.id)
+                .execute()
+            }),
+          )
         })
         .catch(async (err) => {
           logger.error(`Merge failed for storage location ${storageLocation.id}`, { error: err })

tests/cleanup-lifecycle.test.ts

@@ -276,7 +276,9 @@ describe('cleanup lifecycle', () => {
       await db.deleteFrom('storage_locations').where('id', '=', locationId).execute()
       await storage.adapter.deleteFolder(folderName)
     }
-  })
+    // the `vi.waitFor` above may use the full 5s on its own, which is the whole
+    // default test timeout — leave room for the surrounding storage and db work
+  }, 30_000)
 
   test('storage-location cleanup waits for an active merged download', async () => {
     const db = await getDatabase()
@@ -327,7 +329,7 @@ describe('cleanup lifecycle', () => {
       },
       { timeout: 5000, interval: 100 },
     )
-  })
+  }, 30_000)
 
   test.skipIf(process.env.VITEST_STORAGE_DRIVER !== 's3')(
     'a direct-download reader lease covers the signed URL lifetime',

tests/db-lock-retry.test.ts

@@ -0,0 +1,32 @@
+import { describe, expect, test, vi } from 'vitest'
+import { retryOnLockConflict } from '~/lib/db'
+
+function lockError(code: string) {
+  return Object.assign(new Error('Deadlock found when trying to get lock'), { code })
+}
+
+describe('retryOnLockConflict', () => {
+  test('retries a deadlock victim until it succeeds', async () => {
+    const run = vi
+      .fn()
+      .mockRejectedValueOnce(lockError('ER_LOCK_DEADLOCK'))
+      .mockResolvedValueOnce('merged')
+
+    await expect(retryOnLockConflict(run)).resolves.toBe('merged')
+    expect(run).toHaveBeenCalledTimes(2)
+  })
+
+  test('gives up after the attempt limit', async () => {
+    const run = vi.fn().mockRejectedValue(lockError('40P01'))
+
+    await expect(retryOnLockConflict(run, 2)).rejects.toThrow('Deadlock')
+    expect(run).toHaveBeenCalledTimes(2)
+  })
+
+  test('rethrows anything that is not a lock conflict', async () => {
+    const run = vi.fn().mockRejectedValue(new Error('Merge lease was lost before completion'))
+
+    await expect(retryOnLockConflict(run)).rejects.toThrow('Merge lease was lost')
+    expect(run).toHaveBeenCalledTimes(1)
+  })
+})

tests/jwks-discovery.test.ts

@@ -0,0 +1,41 @@
+import { afterEach, describe, expect, test, vi } from 'vitest'
+import { discoverJwksUrl } from '~/lib/scope'
+
+function mockDiscoveryResponse(response: Response) {
+  vi.spyOn(globalThis, 'fetch').mockResolvedValue(response)
+}
+
+afterEach(() => {
+  vi.restoreAllMocks()
+})
+
+describe('discoverJwksUrl', () => {
+  test('returns the `jwks_uri` from the discovery document', async () => {
+    // an enterprise custom issuer keeps its JWKS at the root host
+    mockDiscoveryResponse(
+      Response.json({
+        issuer: 'https://token.actions.githubusercontent.com/octocat-inc',
+        jwks_uri: 'https://token.actions.githubusercontent.com/.well-known/jwks',
+      }),
+    )
+
+    await expect(discoverJwksUrl()).resolves.toBe(
+      'https://token.actions.githubusercontent.com/.well-known/jwks',
+    )
+    expect(fetch).toHaveBeenCalledWith(
+      'https://token.actions.githubusercontent.com/.well-known/openid-configuration',
+    )
+  })
+
+  test('throws when the discovery document is not served', async () => {
+    mockDiscoveryResponse(new Response('Not Found', { status: 404 }))
+
+    await expect(discoverJwksUrl()).rejects.toThrow('404')
+  })
+
+  test('throws when the discovery document has no `jwks_uri`', async () => {
+    mockDiscoveryResponse(Response.json({ issuer: 'https://token.actions.githubusercontent.com' }))
+
+    await expect(discoverJwksUrl()).rejects.toThrow('`jwks_uri`')
+  })
+})