feat(upload): signed upload URLs

docs/adr/0011-upload-urls-carry-a-capability.md

@@ -0,0 +1,11 @@
+---
+status: proposed
+---
+
+# Upload URLs carry a capability
+
+Twirp calls are authorized by the Actions runtime token (`getCacheScope`), but the Upload URL returned in `signed_upload_url` is fetched by Azure Blob-style clients that send no token. Until now that URL held only the numeric Upload id, so it was the sole thing standing between any client that can reach the server and the Upload's Parts. We now issue a random 256-bit capability with each Upload, put it in the URL's query (`?sig=…`), and require it on every block, single-shot and blocklist request. A missing or wrong capability and an unknown Upload all get the same `403`, so the response reveals nothing about which Uploads exist. An authorized Part whose Upload is deleted while it streams still gets `404`.
+
+The capability lives in the query because every client we support treats the signed URL as an Azure SAS URL: `@actions/cache` and `go-actions-cache` keep the query and append `comp`/`blockid`, and OpenDAL (sccache) requires a query and re-sends it as the SAS. Only its SHA-256 is stored (`uploads.capabilityHash`), compared in constant time, so neither the database nor its debug query log can reproduce a working URL. Request logs record paths without their query string, and upload errors are logged by the route rather than left to Nitro, whose unhandled-error log includes the request URL. Upload URLs requested with a method other than `PUT` get `405` instead of falling through to the Results Passthrough, which would forward the capability upstream.
+
+We rejected an HMAC over the Upload id: it needs a secret shared by every worker and kept across restarts, which this server has no place for, while a stored random value needs nothing new. The column comes from migration `$9_upload_capability_hash`, which keeps a `capabilityHash` column that already exists rather than failing on it. Uploads created before the migration have no capability, so their URLs stop accepting Parts; they are short-lived and `cleanup:uploads` removes them once idle.

lib/db.ts

@@ -69,6 +69,8 @@ export const uploadSchema = z.object({
   finishedPartUploadCount: z.number(),
   folderName: z.string(),
   reservationHash: z.string().nullable(),
+  // SHA-256 of the Upload capability; `null` for Uploads created without one
+  capabilityHash: z.string().nullable(),
 })
 type Upload = z.infer<typeof uploadSchema>
 

lib/helpers.ts

@@ -1,5 +1,6 @@
 import type { H3Event } from 'h3'
-import { createHash } from 'node:crypto'
+import { Buffer } from 'node:buffer'
+import { createHash, randomBytes, timingSafeEqual } from 'node:crypto'
 import { customAlphabet } from 'nanoid'
 
 const generate = customAlphabet('0123456789', 10)
@@ -29,3 +30,28 @@ export function reservationHash({ key, version, scope, repoId }: Reservation) {
     .update(JSON.stringify([key, version, scope, repoId]))
     .digest('hex')
 }
+
+/**
+ * A new Upload capability: 256 random bits, sent in the Upload URL's query
+ * (`?sig=`). Upload URLs are fetched by Azure Blob-style clients that send no
+ * Actions token, so the capability is their only authorization.
+ */
+export function generateUploadCapability() {
+  return randomBytes(32).toString('hex')
+}
+
+/** What is stored of a capability, so neither the database nor its query log can reproduce a working Upload URL. */
+export function hashUploadCapability(capability: string) {
+  return createHash('sha256').update(capability).digest('hex')
+}
+
+/** Whether `capability` is the one whose hash was stored, compared in constant time. */
+export function uploadCapabilityMatches(
+  capabilityHash: string | null | undefined,
+  capability: string | undefined,
+) {
+  if (!capabilityHash || !capability) return false
+  const expected = Buffer.from(capabilityHash, 'hex')
+  const actual = createHash('sha256').update(capability).digest()
+  return expected.length === actual.length && timingSafeEqual(expected, actual)
+}

lib/migrations.ts

@@ -285,5 +285,25 @@ export function migrations(
             .execute()
       },
     },
+    $9_upload_capability_hash: {
+      // SHA-256 of the capability in an Upload's URL (`?sig=`, ADR-0011). Nullable:
+      // Uploads created before this migration have none and accept no parts.
+      async up(db) {
+        // A database can already have the column, e.g. from a build that added it
+        // before this migration existed; keep it and the hashes it holds.
+        const tables = await db.introspection.getTables()
+        const exists = tables.some(
+          (table) =>
+            table.name === 'uploads' &&
+            table.columns.some((column) => column.name === 'capabilityHash'),
+        )
+        if (exists) return
+
+        await db.schema.alterTable('uploads').addColumn('capabilityHash', 'varchar(64)').execute()
+      },
+      async down(db) {
+        await db.schema.alterTable('uploads').dropColumn('capabilityHash').execute()
+      },
+    },
   } satisfies Record<string, Migration>
 }

lib/storage.ts

@@ -41,7 +41,13 @@ import { chunk } from 'remeda'
 import { match } from 'ts-pattern'
 import { getDatabase, isUniqueViolation, retryOnLockConflict } from './db'
 import { env } from './env'
-import { generateNumberId, reservationHash } from './helpers'
+import {
+  generateNumberId,
+  generateUploadCapability,
+  hashUploadCapability,
+  reservationHash,
+  uploadCapabilityMatches,
+} from './helpers'
 import { logger } from './logger'
 import {
   acquireMergeLease,
@@ -377,6 +383,21 @@ export class Storage {
     return Promise.all(this.mergeStreamPromises)
   }
 
+  /**
+   * Whether `capability` is the one issued with the Upload. Upload URLs are not
+   * covered by the Actions runtime token, so this is their only authorization.
+   * Unknown Uploads, Uploads created without a capability, and missing or wrong
+   * capabilities are indistinguishable.
+   */
+  async authorizeUpload(uploadId: number, capability: string | undefined) {
+    const upload = await this.db
+      .selectFrom('uploads')
+      .where('id', '=', uploadId)
+      .select('capabilityHash')
+      .executeTakeFirst()
+    return uploadCapabilityMatches(upload?.capabilityHash, capability)
+  }
+
   /**
    * `false` when the upload does not exist, or was deleted while the part was streaming
    * (the written part is then Orphaned Storage, ADR-0001).
@@ -636,6 +657,7 @@ export class Storage {
 
   async createUpload(reservation: Reservation) {
     const uploadId = generateNumberId()
+    const capability = generateUploadCapability()
     try {
       await this.db
         .insertInto('uploads')
@@ -645,6 +667,7 @@ export class Storage {
           folderName: uploadId.toString(),
           createdAt: Date.now(),
           reservationHash: reservationHash(reservation),
+          capabilityHash: hashUploadCapability(capability),
           lastPartUploadedAt: null,
           finishedPartUploadCount: 0,
           startedPartUploadCount: 0,
@@ -655,7 +678,7 @@ export class Storage {
       throw err
     }
 
-    return { id: uploadId }
+    return { id: uploadId, capability }
   }
 
   async matchCacheEntry({

routes/[...path].ts

@@ -2,7 +2,15 @@ import { env } from '~/lib/env'
 import { loggedPath } from '~/lib/helpers'
 import { logger } from '~/lib/logger'
 
+// This server's own Upload URLs. Only `PUT` is routed; any other method lands
+// here, and forwarding it would hand the URL's capability to the upstream.
+const UPLOAD_PATH = /^\/(?:devstoreaccount1\/)?upload\//
+
 export default defineEventHandler(async (event) => {
-  logger.debug('proxying unknown path', loggedPath(event), 'to', env.DEFAULT_ACTIONS_RESULTS_URL)
+  const path = loggedPath(event)
+  if (UPLOAD_PATH.test(path))
+    throw createError({ statusCode: 405, statusMessage: 'Method Not Allowed' })
+
+  logger.debug('proxying unknown path', path, 'to', env.DEFAULT_ACTIONS_RESULTS_URL)
   return proxyRequest(event, `${env.DEFAULT_ACTIONS_RESULTS_URL}${event.path}`)
 })

routes/devstoreaccount1/upload/[uploadId].put.ts

@@ -18,36 +18,43 @@ export default defineEventHandler(async (event) => {
       statusMessage: `Invalid path parameters: ${parsedPathParams.error.message}`,
     })
 
-  if (getQuery(event).comp === 'blocklist') {
-    setResponseStatus(event, 201)
-    // prevent random EOF error with in tonistiigi/go-actions-cache caused by missing request id
-    setHeader(event, 'x-ms-request-id', randomUUID())
-    return
-  }
-
-  const blockId = getQuery(event)?.blockid as string
-  // if no block id, upload smaller than chunk size
-  const chunkIndex = blockId ? getChunkIndexFromBlockId(blockId) : 0
-  if (chunkIndex === undefined)
-    throw createError({
-      statusCode: 400,
-      statusMessage: `Invalid block id: ${blockId}`,
-    })
-
   const { uploadId } = parsedPathParams.data
+  const query = getQuery(event)
+  // The capability issued in `signed_upload_url`. Clients keep the URL's query
+  // and append their own parameters, as they do with an Azure SAS.
+  const capability = typeof query.sig === 'string' ? query.sig : undefined
+
+  // A blocklist commits the blocks already stored; there is no part to write.
+  let chunkIndex: number | undefined
+  if (query.comp !== 'blocklist') {
+    const blockId = query.blockid as string
+    // if no block id, upload smaller than chunk size
+    chunkIndex = blockId ? getChunkIndexFromBlockId(blockId) : 0
+    if (chunkIndex === undefined)
+      throw createError({
+        statusCode: 400,
+        statusMessage: `Invalid block id: ${blockId}`,
+      })
+  }
 
-  let stored: boolean
+  let authorized: boolean
+  let stored = true
   try {
+    const storage = await getStorage()
+    authorized = await storage.authorizeUpload(uploadId, capability)
     // Not h3's `getRequestWebStream`: it enqueues every `data` event and never pauses the
     // request, so a body arriving faster than storage consumes it is buffered in full.
-    const storage = await getStorage()
-    stored = await storage.uploadPart(uploadId, chunkIndex, event.node.req)
+    if (authorized && chunkIndex !== undefined)
+      stored = await storage.uploadPart(uploadId, chunkIndex, event.node.req)
   } catch (err) {
     // Logged here and rethrown as handled: Nitro logs unhandled errors with the
     // full request URL, query string included.
     logger.error(`Failed to store upload ${uploadId}`, err)
     throw createError({ statusCode: 500, statusMessage: 'Failed to store upload' })
   }
+  // Unknown Uploads and missing or wrong capabilities get the same answer.
+  if (!authorized) throw createError({ statusCode: 403, statusMessage: 'Forbidden' })
+  // The Upload was deleted while the part was streaming (e.g. finalized or cleaned up).
   if (!stored)
     throw createError({
       statusCode: 404,

routes/twirp/github.actions.results.api.v1.CacheService/CreateCacheEntry.post.ts

@@ -39,7 +39,10 @@ export default defineEventHandler(async (event) => {
 
   return sendTwirpResponse(
     event,
-    { ok: true, signed_upload_url: `${env.API_BASE_URL}/devstoreaccount1/upload/${upload.id}` },
+    {
+      ok: true,
+      signed_upload_url: `${env.API_BASE_URL}/devstoreaccount1/upload/${upload.id}?sig=${upload.capability}`,
+    },
     TwirpMessage.CreateCacheEntryResponse,
   )
 })

tests/proxy-backpressure.test.ts

@@ -80,10 +80,13 @@ describe('proxy routes apply backpressure', () => {
 
     const { port } = server.address() as AddressInfo
     const serverSide = once(server, 'request') as Promise<[IncomingMessage, ServerResponse]>
-    const req = http.request(`http://localhost:${port}/upload/${created.id}`, {
-      method: 'PUT',
-      headers: { 'content-length': ENTRY_BYTES },
-    })
+    const req = http.request(
+      `http://localhost:${port}/upload/${created.id}?sig=${created.capability}`,
+      {
+        method: 'PUT',
+        headers: { 'content-length': ENTRY_BYTES },
+      },
+    )
     const response = once(req, 'response') as Promise<[IncomingMessage]>
     const chunk = Buffer.alloc(CHUNK_BYTES)
     let sent = 0

tests/request-logging.test.ts

@@ -73,7 +73,7 @@ afterAll(async () => {
   await once(server, 'close')
 })
 
-test('query strings stay out of request, error and passthrough logs', async () => {
+test('upload capabilities stay out of request, error and passthrough logs', async () => {
   const storage = await getStorage()
   const created = await storage.createUpload({
     key: `request-logging-${randomUUID()}`,
@@ -83,12 +83,16 @@ test('query strings stay out of request, error and passthrough logs', async () =
   })
 
   const { port } = server.address() as AddressInfo
-  // Azure Blob clients send SAS-style credentials in the query.
-  const secret = randomBytes(32).toString('hex')
-  const url = `http://localhost:${port}/upload/${created.id}?sig=${secret}`
+  const url = `http://localhost:${port}/upload/${created.id}?sig=${created.capability}`
+  const forged = randomBytes(32).toString('hex')
 
   const stored = await fetch(url, { method: 'PUT', body: 'part' })
   expect(stored.status).toBe(201)
+  const rejected = await fetch(`http://localhost:${port}/upload/${created.id}?sig=${forged}`, {
+    method: 'PUT',
+    body: 'part',
+  })
+  expect(rejected.status).toBe(403)
 
   // Storage failing mid-upload (e.g. the backend is unreachable) is logged as an error.
   vi.spyOn(storage.adapter, 'uploadStream').mockRejectedValue(new Error('storage unavailable'))
@@ -100,15 +104,25 @@ test('query strings stay out of request, error and passthrough logs', async () =
   expect(failed.status).toBe(500)
   await vi.waitFor(() => expect(logs.join('\n')).toContain('> 500'))
 
-  // An unknown path falls through to the Results Passthrough.
-  const unknown = await fetch(`http://localhost:${port}/uploads/${created.id}?sig=${secret}`)
-  expect(unknown.status).toBe(204)
+  // A mistyped Upload URL falls through to the Results Passthrough.
+  const mistyped = await fetch(
+    `http://localhost:${port}/uploads/${created.id}?sig=${created.capability}`,
+  )
+  expect(mistyped.status).toBe(204)
   await vi.waitFor(() => expect(logs.join('\n')).toContain(`proxying unknown path`))
   expect(proxyRequest).toHaveBeenCalledTimes(1)
 
+  // An Upload URL with an unsupported method is refused, not forwarded upstream.
+  for (const method of ['POST', 'HEAD', 'GET']) {
+    const response = await fetch(url, { method })
+    expect(response.status).toBe(405)
+  }
+  expect(proxyRequest).toHaveBeenCalledTimes(1)
+
   const logged = logs.join('\n')
   expect(logged).toContain(`/upload/${created.id}`)
   expect(logged).toContain('storage unavailable')
   expect(logged).not.toContain('sig=')
-  expect(logged).not.toContain(secret)
+  expect(logged).not.toContain(created.capability)
+  expect(logged).not.toContain(forged)
 })