fix: upload-api doesn't use a did:web as the serviceSigner in accessC…

…lient (#100)

Motivation:
* @alanshaw informed me via slack that upload-api signs invocations sent
to access-api. These signatures wouldn't be verifiable if their signer
did was a non-key-did.

What
* This PR makes it so the service signer for most of upload-api knows
nothing about `env.UPLOAD_API_DID`.
* `env.UPLOAD_API_DID` only is used for the Principal passed to the
ucanto server id (to verify incoming invocations 'aud')
* similar to this I did for w3protocol
storacha/w3up#303
* /version api response object has new property `aud`, which is the
expected `aud` value of any UCAN invocations sent to the ucanto server
(when env.UPLOAD_API_DID is set, it'll be that did)

Signed-off-by: Oli Evans <oli@protocol.ai>

upload-api/config.js

@@ -1,25 +1,26 @@
-/**
- * This file uses SSTs magic Config handler.
- * If you depend on it in a test then you need to use the `sst bind` CLI to setup the config object.
- *
- * see: https://docs.sst.dev/config
- * see: https://docs.sst.dev/advanced/testing#how-sst-bind-works
- */
 import * as ed25519 from '@ucanto/principal/ed25519'
-import { DID } from '@ucanto/validator'
+import * as DID from '@ipld/dag-ucan/did'
 
 /**
  * Given a config, return a ucanto Signer object representing the service
  *
  * @param {object} config
- * @param {string} [config.UPLOAD_API_DID] - public identifier of the running service. e.g. a did:key or a did:web
  * @param {string} config.PRIVATE_KEY - multiformats private key of primary signing key
  */
  export function getServiceSigner(config) {
   const signer = ed25519.parse(config.PRIVATE_KEY)
-  const did = config.UPLOAD_API_DID
-  if (!did) {
-    return signer
+  return signer
+}
+
+/**
+ * Given a config, return a ucanto principal
+ *
+ * @param {{ UPLOAD_API_DID: string } | { PRIVATE_KEY: string }} config
+ * @returns {import('@ucanto/interface').Principal}
+ */
+export function getServicePrincipal(config) {
+  if ('UPLOAD_API_DID' in config) {
+    return DID.parse(config.UPLOAD_API_DID)
   }
-  return signer.withDID(DID.match({}).from(did))
+  return getServiceSigner(config)
 }

upload-api/functions/get.js

@@ -1,7 +1,7 @@
 import * as Sentry from '@sentry/serverless'
 import { Config } from '@serverless-stack/node/config/index.js'
 
-import { getServiceSigner } from '../config.js'
+import { getServicePrincipal, getServiceSigner } from '../config.js'
 
 Sentry.AWSLambda.init({
   dsn: process.env.SENTRY_DSN,
@@ -15,16 +15,17 @@ Sentry.AWSLambda.init({
  */
  export async function versionGet (request) {
   const { NAME: name , VERSION: version, COMMIT: commit, STAGE: env } = process.env
-  const { UPLOAD_API_DID } = process.env;
   const { PRIVATE_KEY } = Config
-  const did = getServiceSigner({ UPLOAD_API_DID, PRIVATE_KEY }).did()
+  const { UPLOAD_API_DID } = process.env
+  const did = getServiceSigner({ PRIVATE_KEY }).did()
+  const aud = getServicePrincipal({ UPLOAD_API_DID, PRIVATE_KEY }).did()
   const repo = 'https://github.com/web3-storage/upload-api'
   return {
     statusCode: 200,
     headers: {
       'Content-Type': `application/json`
     },
-    body: JSON.stringify({ name, version, did, repo, commit, env })
+    body: JSON.stringify({ name, version, did, aud, repo, commit, env })
   }
 }
 
@@ -36,17 +37,18 @@ export const version = Sentry.AWSLambda.wrapHandler(versionGet)
  * @param {import('aws-lambda').APIGatewayProxyEventV2} request 
  */
 export async function homeGet (request) {
-  const { VERSION: version, STAGE: stage, UPLOAD_API_DID } = process.env
+  const { VERSION: version, STAGE: stage } = process.env
   const { PRIVATE_KEY } = Config
-  const did = getServiceSigner({ PRIVATE_KEY, UPLOAD_API_DID }).did()
+  const { UPLOAD_API_DID } = process.env
+  const aud = getServicePrincipal({ UPLOAD_API_DID, PRIVATE_KEY }).did()
   const repo = 'https://github.com/web3-storage/upload-api'
   const env = stage === 'prod' ? '' : `(${stage})`
   return {
     statusCode: 200,
     headers: {
       'Content-Type': 'text/plain; charset=utf-8'
     },
-    body: `⁂ upload-api v${version} ${env}\n- ${repo}\n- ${did}\n`
+    body: `⁂ upload-api v${version} ${env}\n- ${repo}\n- ${aud}\n`
   }
 }
 

upload-api/functions/ucan-invocation-router.js

@@ -10,7 +10,7 @@ import { createDudewhereStore } from '../buckets/dudewhere-store.js'
 import { createUcanStore } from '../buckets/ucan-store.js'
 import { createStoreTable } from '../tables/store.js'
 import { createUploadTable } from '../tables/upload.js'
-import { getServiceSigner } from '../config.js'
+import { getServicePrincipal, getServiceSigner } from '../config.js'
 import { createUcantoServer } from '../service/index.js'
 import { Config } from '@serverless-stack/node/config/index.js'
 
@@ -59,10 +59,11 @@ async function ucanInvocationRouter (request) {
 
   const { UPLOAD_API_DID } = process.env;
   const { PRIVATE_KEY } = Config
-  const serviceSigner = getServiceSigner({ UPLOAD_API_DID, PRIVATE_KEY })
+  const servicePrincipal = getServicePrincipal({ UPLOAD_API_DID, PRIVATE_KEY })
+  const serviceSigner = getServiceSigner({ PRIVATE_KEY })
   const ucanStoreBucket = createUcanStore(AWS_REGION, ucanBucketName)
 
-  const server = await createUcantoServer(serviceSigner, {
+  const server = await createUcantoServer(servicePrincipal, {
     storeTable: createStoreTable(AWS_REGION, storeTableName, {
       endpoint: dbEndpoint
     }),

upload-api/service/index.js

@@ -23,12 +23,12 @@ export function createServiceRouter (context) {
 }
 
 /**
- * @param {import('@ucanto/interface').Signer} serviceSigner
+ * @param {import('@ucanto/interface').Principal} servicePrincipal
  * @param {import('../service/types').UcantoServerContext} context
  */
-export async function createUcantoServer (serviceSigner, context) {
+export async function createUcantoServer (servicePrincipal, context) {
   const server = Server.create({
-    id: serviceSigner,
+    id: servicePrincipal,
     encoder: CBOR,
     decoder: CAR,
     service: createServiceRouter(context),

upload-api/test/config.test.js

@@ -18,31 +18,48 @@ const testKeypair = {
   },
 }
 
-test('upload-api/config getServiceSigner creates a signer using config.{UPLOAD_API_KEY,PRIVATE_KEY}', async (t) => {
+test('upload-api/config getServiceSigner creates a signer using config.PRIVATE_KEY', async (t) => {
   const config = {
     PRIVATE_KEY: testKeypair.private.multiformats,
-    UPLOAD_API_DID: 'did:web:exampe.com',
   }
   const signer = configModule.getServiceSigner(config)
   t.assert(signer)
-  t.is(signer.did().toString(), config.UPLOAD_API_DID)
+  t.is(signer.did().toString(), testKeypair.public.did)
   const { keys } = signer.toArchive()
   const didKeys = Object.keys(keys)
   t.deepEqual(didKeys, [testKeypair.public.did])
 })
-test('upload-api/config getServiceSigner errors if config.DID is provided but not a did', (t) => {
+test('upload-api/config getServiceSigner infers did from config.PRIVATE_KEY when config.UPLOAD_API_DID is omitted', async (t) => {
+  const config = {
+    PRIVATE_KEY: testKeypair.private.multiformats,
+  }
+  const signer = configModule.getServiceSigner(config)
+  t.assert(signer)
+  t.is(signer.did().toString(), testKeypair.public.did)
+})
+
+test('upload-api/config getServerPrincipal creates a signer using config.{UPLOAD_API_KEY,PRIVATE_KEY}', async (t) => {
+  const config = {
+    PRIVATE_KEY: testKeypair.private.multiformats,
+    UPLOAD_API_DID: 'did:web:exampe.com',
+  }
+  const principal = configModule.getServicePrincipal(config)
+  t.assert(principal)
+  t.is(principal.did().toString(), config.UPLOAD_API_DID)
+})
+test('upload-api/config getServerPrincipal errors if config.UPLOAD_API_DID is provided but not a did', (t) => {
   t.throws(() => {
-    configModule.getServiceSigner({
+    configModule.getServicePrincipal({
       UPLOAD_API_DID: 'not a did',
       PRIVATE_KEY: testKeypair.private.multiformats,
     })
-  }, { message: /^Expected a did: but got ".+" instead$/ })
+  }, { message: /^Invalid DID/ })
 })
-test('upload-api/config getServiceSigner infers did from config.PRIVATE_KEY when config.DID is omitted', async (t) => {
+test('upload-api/config getServerPrincipal infers did from config.PRIVATE_KEY when config.UPLOAD_API_DID is omitted', async (t) => {
   const config = {
     PRIVATE_KEY: testKeypair.private.multiformats,
   }
-  const signer = configModule.getServiceSigner(config)
-  t.assert(signer)
-  t.is(signer.did().toString(), testKeypair.public.did)
+  const principal = configModule.getServicePrincipal(config)
+  t.assert(principal)
+  t.is(principal.did().toString(), testKeypair.public.did)
 })

upload-api/test/helpers/ucanto.js

@@ -38,7 +38,7 @@ export function createTestingUcantoServer(service, ctx) {
 
 /**
  * @param {import('@ucanto/interface').Signer} service
- * @param {any} context
+ * @param {import('./context.js').UcantoServerContext & ResourcesMetadata} context
  * @returns 
  */
 export async function getClientConnection (service, context) {

upload-api/test/service/upload.test.js

@@ -9,7 +9,7 @@ import { uploadTableProps } from '../../tables/index.js'
 import { createS3, createBucket, createAccessServer, createDynamodDb, dynamoDBTableConfig } from '../helpers/resources.js'
 import { randomCAR } from '../helpers/random.js'
 import { getClientConnection, createSpace } from '../helpers/ucanto.js'
-import { getServiceSigner } from '../../config.js'
+import * as DID from '@ipld/dag-ucan/did'
 
 // https://docs.aws.amazon.com/AWSJavaScriptSDK/v3/latest/clients/client-dynamodb/classes/batchwriteitemcommand.html
 const BATCH_MAX_SAFE_LIMIT = 25
@@ -620,12 +620,9 @@ test('upload/list can be paginated with custom size', async (t) => {
 })
 
 test('can invoke when serviceSigner has a did:web did', async (t) => {
-  const serviceDid = 'did:web:example.com'
-  const servicePrivateKey = Signer.format(await Signer.generate())
-  const servicePrincipal = getServiceSigner({
-    UPLOAD_API_DID: serviceDid,
-    PRIVATE_KEY: servicePrivateKey,
-  })
+  const serviceDid = DID.parse('did:web:example.com')
+  const serviceKeySigner = (await Signer.generate())
+  const servicePrincipal = serviceKeySigner.withDID(serviceDid.did());
   const connection = await getClientConnection(servicePrincipal, {
     ...t.context,
     ...await prepareResources(t.context.dynamoClient, t.context.s3Client),
@@ -635,7 +632,7 @@ test('can invoke when serviceSigner has a did:web did', async (t) => {
   const alice = await Signer.generate()
   const inovocation = await createNoopRemoveInovocation({
     issuer: alice,
-    audience: servicePrincipal
+    audience: serviceDid
   })
   const result = await inovocation.execute(connection)
   t.falsy(result, 'result is falsy')
@@ -646,15 +643,15 @@ test('can invoke when serviceSigner has a did:web did', async (t) => {
   // Specifically, we'll use the wrong audience that corresponds to a servicePrincipal.signer key.
   // This might be a common mistake, since its a key that the serviceSigner may sign with,
   // but the `signer.did()` does not match, so we'd still expect the server to reject it.
-  const wrongAudience = Signer.parse(servicePrivateKey)
+  const wrongAudience = serviceKeySigner
   const resultOfInvocationWithWrongAudience = await (await createNoopRemoveInovocation({
     issuer: alice,
     audience: wrongAudience,
   })).execute(connection)
   t.not(resultOfInvocationWithWrongAudience, undefined, 'result is not undefined - it should be an error')
   if (resultOfInvocationWithWrongAudience?.error) {
     t.is(resultOfInvocationWithWrongAudience.name, 'InvalidAudience', 'result of sending invocation with wrong audience is InvalidAudience')
-    t.is(/** @type {import('@ucanto/server').InvalidAudience} */ (resultOfInvocationWithWrongAudience).audience?.toString(), serviceDid)
+    t.is(/** @type {import('@ucanto/server').InvalidAudience} */ (resultOfInvocationWithWrongAudience).audience?.toString(), serviceDid.did())
   }
 })