Use node:crypto instead of noble/curves (bluesky-social#2936)

* Extract verifySignatureWithKey out of verifyJwt * Accept optional verifySignatureWithKey as param * Impl. verifySignatureWithKey with native crypto * Test key validation * changesets * build * build (fix) * Move verifySig out * Trigger Build * Move test * Remove redundant check --------- Co-authored-by: Devin Ivy <[email protected]>
appview-wg-bsky · Nov 7, 2024 · 1982693 · 1982693
1 parent 9e18ab6
commit 1982693
Show file tree

Hide file tree

Showing 8 changed files with 127 additions and 11 deletions.
diff --git a/.changeset/mean-icons-argue.md b/.changeset/mean-icons-argue.md
@@ -0,0 +1,5 @@
+---
+"@atproto/crypto": patch
+---
+
+Export utils
diff --git a/.changeset/ninety-rats-reflect.md b/.changeset/ninety-rats-reflect.md
@@ -0,0 +1,5 @@
+---
+"@atproto/xrpc-server": patch
+---
+
+Accept a custom verifySignatureWithKey in verifyJwt
diff --git a/.changeset/real-needles-hunt.md b/.changeset/real-needles-hunt.md
@@ -0,0 +1,5 @@
+---
+"@atproto/bsky": patch
+---
+
+Use custom verifySignatureWithKey with native node:crypto instead of @noble/curves to evaluate performance
diff --git a/.github/workflows/build-and-push-bsky-ghcr.yaml b/.github/workflows/build-and-push-bsky-ghcr.yaml
@@ -3,7 +3,7 @@ on:
   push:
     branches:
       - main
-      - bsky-tweaks
+      - bsky-use-native-crypto
 env:
   REGISTRY: ghcr.io
   USERNAME: ${{ github.actor }}

diff --git a/packages/bsky/src/auth-verifier.ts b/packages/bsky/src/auth-verifier.ts
@@ -1,8 +1,10 @@
-import { KeyObject, createPublicKey } from 'node:crypto'
+import crypto, { KeyObject } from 'node:crypto'
 import {
   AuthRequiredError,
+  cryptoVerifySignatureWithKey,
   parseReqNsid,
   verifyJwt as verifyServiceJwt,
+  VerifySignatureWithKeyFn,
 } from '@atproto/xrpc-server'
 import KeyEncoder from 'key-encoder'
 import * as ui8 from 'uint8arrays'
@@ -16,6 +18,14 @@ import {
   unpackIdentityKeys,
 } from './data-plane'
 import { GetIdentityByDidResponse } from './proto/bsky_pb'
+import {
+  extractMultikey,
+  extractPrefixedBytes,
+  hasPrefix,
+  parseDidKey,
+  SECP256K1_DID_PREFIX,
+  SECP256K1_JWT_ALG,
+} from '@atproto/crypto'
 
 type ReqCtx = {
   req: express.Request
@@ -343,6 +353,7 @@ export class AuthVerifier {
       opts.aud,
       null,
       getSigningKey,
+      verifySignatureWithKey,
     )
     if (
       !payload.iss.endsWith('#atproto_labeler') ||
@@ -440,5 +451,48 @@ export const buildBasicAuth = (username: string, password: string): string => {
 const keyEncoder = new KeyEncoder('secp256k1')
 export const createPublicKeyObject = (publicKeyHex: string): KeyObject => {
   const key = keyEncoder.encodePublic(publicKeyHex, 'raw', 'pem')
-  return createPublicKey({ format: 'pem', key })
+  return crypto.createPublicKey({ format: 'pem', key })
+}
+
+const verifySig = (
+  publicKey: Uint8Array,
+  data: Uint8Array,
+  sig: Uint8Array,
+) => {
+  const keyEncoder = new KeyEncoder('secp256k1')
+
+  const pemKey = keyEncoder.encodePublic(
+    ui8.toString(publicKey, 'hex'),
+    'raw',
+    'pem',
+  )
+  const key = crypto.createPublicKey({ format: 'pem', key: pemKey })
+
+  return crypto.verify(
+    'sha256',
+    data,
+    {
+      key,
+      dsaEncoding: 'ieee-p1363',
+    },
+    sig,
+  )
+}
+
+export const verifySignatureWithKey: VerifySignatureWithKeyFn = async (
+  didKey: string,
+  msgBytes: Uint8Array,
+  sigBytes: Uint8Array,
+  alg: string,
+) => {
+  if (alg === SECP256K1_JWT_ALG) {
+    const parsed = parseDidKey(didKey)
+    if (alg !== parsed.jwtAlg) {
+      throw new Error(`Expected key alg ${alg}, got ${parsed.jwtAlg}`)
+    }
+
+    return verifySig(parsed.keyBytes, msgBytes, sigBytes)
+  }
+
+  return cryptoVerifySignatureWithKey(didKey, msgBytes, sigBytes, alg)
 }
diff --git a/packages/bsky/tests/auth.test.ts b/packages/bsky/tests/auth.test.ts
@@ -9,6 +9,10 @@ describe('auth', () => {
   let agent: AtpAgent
   let sc: SeedClient
 
+  // account dids, for convenience
+  let alice: string
+  let bob: string
+
   beforeAll(async () => {
     network = await TestNetwork.create({
       dbPostgresSchema: 'bsky_auth',
@@ -17,6 +21,8 @@ describe('auth', () => {
     sc = network.getSeedClient()
     await usersSeed(sc)
     await network.processAll()
+    alice = sc.dids.alice
+    bob = sc.dids.bob
   })
 
   afterAll(async () => {
@@ -63,4 +69,24 @@ describe('auth', () => {
       'jwt signature does not match jwt issuer',
     )
   })
+
+  it('throws if the user key is incorrect', async () => {
+    const bobKeypair = await network.pds.ctx.actorStore.keypair(bob)
+
+    const jwt = await createServiceJwt({
+      iss: alice,
+      aud: network.bsky.ctx.cfg.serverDid,
+      lxm: ids.AppBskyFeedGetTimeline,
+      keypair: bobKeypair,
+    })
+
+    await expect(
+      agent.api.app.bsky.actor.getProfile(
+        { actor: sc.dids.carol },
+        {
+          headers: { authorization: `Bearer ${jwt}` },
+        },
+      ),
+    ).rejects.toThrow()
+  })
 })
diff --git a/packages/crypto/src/index.ts b/packages/crypto/src/index.ts
@@ -5,6 +5,7 @@ export * from './random'
 export * from './sha'
 export * from './types'
 export * from './verify'
+export * from './utils'
 
 export * from './p256/keypair'
 export * from './p256/plugin'

diff --git a/packages/xrpc-server/src/auth.ts b/packages/xrpc-server/src/auth.ts
@@ -62,11 +62,19 @@ const jsonToB64Url = (json: Record<string, unknown>): string => {
   return common.utf8ToB64Url(JSON.stringify(json))
 }
 
+export type VerifySignatureWithKeyFn = (
+  key: string,
+  msgBytes: Uint8Array,
+  sigBytes: Uint8Array,
+  alg: string,
+) => Promise<boolean>
+
 export const verifyJwt = async (
   jwtStr: string,
   ownDid: string | null, // null indicates to skip the audience check
   lxm: string | null, // null indicates to skip the lxm check
   getSigningKey: (iss: string, forceRefresh: boolean) => Promise<string>,
+  verifySignatureWithKey: VerifySignatureWithKeyFn = cryptoVerifySignatureWithKey,
 ): Promise<ServiceJwtPayload> => {
   const parts = jwtStr.split('.')
   if (parts.length !== 3) {
@@ -116,18 +124,13 @@ export const verifyJwt = async (
 
   const msgBytes = ui8.fromString(parts.slice(0, 2).join('.'), 'utf8')
   const sigBytes = ui8.fromString(sig, 'base64url')
-  const verifySignatureWithKey = async (key: string) => {
-    return crypto.verifySignature(key, msgBytes, sigBytes, {
-      jwtAlg: header.alg,
-      allowMalleableSig: true,
-    })
-  }
 
   const signingKey = await getSigningKey(payload.iss, false)
+  const { alg } = header
 
   let validSig: boolean
   try {
-    validSig = await verifySignatureWithKey(signingKey)
+    validSig = await verifySignatureWithKey(signingKey, msgBytes, sigBytes, alg)
   } catch (err) {
     throw new AuthRequiredError(
       'could not verify jwt signature',
@@ -141,7 +144,12 @@ export const verifyJwt = async (
     try {
       validSig =
         freshSigningKey !== signingKey
-          ? await verifySignatureWithKey(freshSigningKey)
+          ? await verifySignatureWithKey(
+              freshSigningKey,
+              msgBytes,
+              sigBytes,
+              alg,
+            )
           : false
     } catch (err) {
       throw new AuthRequiredError(
@@ -161,6 +169,18 @@ export const verifyJwt = async (
   return payload
 }
 
+export const cryptoVerifySignatureWithKey: VerifySignatureWithKeyFn = async (
+  key: string,
+  msgBytes: Uint8Array,
+  sigBytes: Uint8Array,
+  alg: string,
+) => {
+  return crypto.verifySignature(key, msgBytes, sigBytes, {
+    jwtAlg: alg,
+    allowMalleableSig: true,
+  })
+}
+
 const parseB64UrlToJson = (b64: string) => {
   return JSON.parse(common.b64UrlToUtf8(b64))
 }