Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement payload verification using WebCrypto API #12

Open
gr2m opened this issue Aug 29, 2024 · 5 comments
Open

Implement payload verification using WebCrypto API #12

gr2m opened this issue Aug 29, 2024 · 5 comments
Assignees
Labels
feature New feature or request pull request welcome Issue is actionable and anyone can claim it

Comments

@gr2m
Copy link
Collaborator

gr2m commented Aug 29, 2024

The payload verification is currently implemented using node's built-in crypto module:

preview-sdk.js/index.js

Lines 17 to 19 in 06310d7

return createVerify("SHA256")
.update(rawBody)
.verify(key, signature, "base64");

But all modern JS run time environments now have the global crypto API which is WebCrypto - including Node.js.

For compatibility with @octokit which aims to be as universal as possible, we should implement that method using WebCrypto.

@gr2m gr2m added feature New feature or request pull request welcome Issue is actionable and anyone can claim it labels Aug 29, 2024
@francisfuzz
Copy link
Collaborator

the global crypto API which is WebCrypto - including Node.js.

For reference: Node.js v22.9.0 Docs: Web Crypto API

@nickytonline
Copy link

Hi @gr2m and @francisfuzz. I'd like to work on this.

@gr2m
Copy link
Collaborator Author

gr2m commented Oct 7, 2024

All yours 👍🏼

@nickytonline
Copy link

nickytonline commented Oct 8, 2024

I've been tinkering with this today using the WebCrypto API's crypto.subtle.importKey to read the public key, and crypto.subtle.verify to verify the signature, however several attempts always result in the unit tests failing that call verifySignature.

I did some digging and it seems like the test SIGNATURE in verfication.test.js seems to not work with the WebCrypto API. It considers it invalid which causes the verification to fail. It looks like node:crypto might be more forgiving.

I'm curious what method you used to generate the signature for the currrent test suite? It's also possible the WebCrypto API does not recognize the signature format. There are ways to convert it, but before I proceed down that path, I just wanted to check what method you used.

When I generate a new signature, the WebCrypto API validates it correctly. (I generated a new key pair to test this)

async function signData(privateKey, data) {
  const encoder = new TextEncoder();
  const encodedData = encoder.encode(data);

  const signature = await crypto.subtle.sign(
    {
      name: 'ECDSA',
      hash: { name: 'SHA-256' },
    },
    privateKey,
    encodedData
  );

  return arrayBufferToBase64(signature);
}

In the meantime, I'm going to run my branch with a test Copilot extension to see if the WebCrypto API changes work in a real environment.

@nickytonline
Copy link

Some other observations: Bun and Deno both support node:crypto, although it's currently not supported by Cloudlfare Workers. Cloudlfare Workers support node:crypto but only a subset of it, so the WebCrypto API is probably still the way to go.

Deno:

preview-sdk.js on  main [$?] is 📦 semantic via  v20.17.0 took 7s 
❯ deno task test
Task test npm run test:code && npm run test:tsc && npm run test:types

> @copilot-extensions/preview-sdk@0.0.0-development test:code
> c8 --100 ava test/*.test.js


  ✔ response › smoke
  ✔ response › createAckEvent()
  ✔ response › createDoneEvent()
  ✔ response › createTextEvent()
  ✔ response › createConfirmationEvent()
  ✔ response › createErrorsEvent()
  ✔ response › createReferencesEvent()
  ✔ prompt › smoke
  ✔ prompt › parsePromptArguments - uses Node fetch if no options.fetch passed as argument
  ✔ prompt › includes function calls
  ✔ prompt › does not include function calls
  ✔ verification › smoke
  ✔ verification › verifyRequestByKeyId() - invalid arguments
  ✔ verification › verifyRequest() - valid
  ✔ verification › verifyRequest() - invalid
  ✔ parse › smoke
  ✔ parse › parseRequestBody()
  ✔ parse › transformPayloadForOpenAICompatibility()
  ✔ parse › getUserMessage()
  ✔ parse › getUserConfirmation()
  ✔ parse › getUserConfirmation() with no copilot confirmations
  ✔ parse › verifyRequestByKeyId() - invalid arguments
  ✔ parse › verifyRequest() - valid
  ✔ parse › verifyRequest() - invalid
  ✔ prompt › prompt.stream
  ✔ prompt › minimal usage
  ✔ prompt › options.prompt
  ✔ prompt › options.messages
  ✔ prompt › options.endpoint
  ✔ prompt › single options argument
  ✔ prompt › function calling
  ✔ prompt › Handles error
  ✔ verification › fetchVerificationKeys() - throws on non-ok response
  ✔ verification › fetchVerificationKeys() - returns cached keys on 304 response
  ✔ parse › fetchVerificationKeys() - throws on non-ok response
  ✔ parse › fetchVerificationKeys() - returns cached keys on 304 response
  ✔ verification › fetchVerificationKeys() - without cache
  ✔ verification › fetchVerificationKeys() - with token
  ✔ verification › verifyRequestByKeyId()
  ✔ verification › verifyRequestByKeyId() - throws if keyId not present in verification keys list
  ✔ verification › fetchVerificationKeys() - populates and utilizes cache correctly
  ✔ parse › fetchVerificationKeys() - without cache
  ✔ parse › fetchVerificationKeys() - with token
  ✔ parse › verifyRequestByKeyId()
  ✔ parse › verifyAndParseRequest()
  ✔ parse › verifyRequestByKeyId() - throws if keyId not present in verification keys list
  ✔ parse › fetchVerificationKeys() - populates and utilizes cache correctly
  ─

  47 tests passed
--------------------|---------|----------|---------|---------|-------------------
File                | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s 
--------------------|---------|----------|---------|---------|-------------------
All files           |     100 |      100 |     100 |     100 |                   
 preview-sdk.js     |     100 |      100 |     100 |     100 |                   
  index.js          |     100 |      100 |     100 |     100 |                   
 preview-sdk.js/lib |     100 |      100 |     100 |     100 |                   
  parse.js          |     100 |      100 |     100 |     100 |                   
  prompt.js         |     100 |      100 |     100 |     100 |                   
  response.js       |     100 |      100 |     100 |     100 |                   
  verification.js   |     100 |      100 |     100 |     100 |                   
--------------------|---------|----------|---------|---------|-------------------

> @copilot-extensions/preview-sdk@0.0.0-development test:tsc
> tsc --allowJs --noEmit --esModuleInterop --skipLibCheck --lib es2020 index.js


> @copilot-extensions/preview-sdk@0.0.0-development test:types
> tsd

Bun:

preview-sdk.js on  main [$?] is 📦 semantic via  v20.17.0 took 4s 
❯ bun update
bun update v1.0.29 (a146856d)
[4.27ms] migrated lockfile from package-lock.json

 76 packages installed [983.00ms]

preview-sdk.js on  main [$?] is 📦 semantic via 🥟 v1.0.29 via  v20.17.0 
❯ bun test
bun test v1.0.29 (a146856d)

test/verification.test.js:

Test files must be run with the AVA CLI:

    $ ava test/verification.test.js


preview-sdk.js on  main [$?] is 📦 semantic via 🥟 v1.0.29 via  v20.17.0 
❯ bun run test
$ bun run test:code && bun run test:tsc && bun run test:types
$ c8 --100 ava test/*.test.js

  ✔ response › smoke
  ✔ response › createAckEvent()
  ✔ response › createDoneEvent()
  ✔ response › createTextEvent()
  ✔ response › createConfirmationEvent()
  ✔ response › createErrorsEvent()
  ✔ response › createReferencesEvent()
  ✔ prompt › smoke
  ✔ prompt › parsePromptArguments - uses Node fetch if no options.fetch passed as argument
  ✔ verification › smoke
  ✔ prompt › includes function calls
  ✔ prompt › does not include function calls
  ✔ verification › verifyRequestByKeyId() - invalid arguments
  ✔ verification › verifyRequest() - valid
  ✔ verification › verifyRequest() - invalid
  ✔ parse › smoke
  ✔ parse › parseRequestBody()
  ✔ parse › transformPayloadForOpenAICompatibility()
  ✔ parse › getUserMessage()
  ✔ parse › getUserConfirmation()
  ✔ parse › getUserConfirmation() with no copilot confirmations
  ✔ parse › verifyRequestByKeyId() - invalid arguments
  ✔ parse › verifyRequest() - valid
  ✔ parse › verifyRequest() - invalid
  ✔ prompt › prompt.stream
  ✔ verification › fetchVerificationKeys() - throws on non-ok response
  ✔ verification › fetchVerificationKeys() - returns cached keys on 304 response
  ✔ prompt › minimal usage
  ✔ prompt › options.prompt
  ✔ prompt › options.messages
  ✔ prompt › options.endpoint
  ✔ prompt › single options argument
  ✔ prompt › function calling
  ✔ prompt › Handles error
  ✔ parse › fetchVerificationKeys() - throws on non-ok response
  ✔ parse › fetchVerificationKeys() - returns cached keys on 304 response
  ✔ verification › fetchVerificationKeys() - without cache
  ✔ verification › fetchVerificationKeys() - with token
  ✔ verification › verifyRequestByKeyId()
  ✔ verification › verifyRequestByKeyId() - throws if keyId not present in verification keys list
  ✔ verification › fetchVerificationKeys() - populates and utilizes cache correctly
  ✔ parse › fetchVerificationKeys() - without cache
  ✔ parse › fetchVerificationKeys() - with token
  ✔ parse › verifyRequestByKeyId()
  ✔ parse › verifyAndParseRequest()
  ✔ parse › verifyRequestByKeyId() - throws if keyId not present in verification keys list
  ✔ parse › fetchVerificationKeys() - populates and utilizes cache correctly
  ─

  47 tests passed
--------------------|---------|----------|---------|---------|-------------------
File                | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s 
--------------------|---------|----------|---------|---------|-------------------
All files           |     100 |      100 |     100 |     100 |                   
 preview-sdk.js     |     100 |      100 |     100 |     100 |                   
  index.js          |     100 |      100 |     100 |     100 |                   
 preview-sdk.js/lib |     100 |      100 |     100 |     100 |                   
  parse.js          |     100 |      100 |     100 |     100 |                   
  prompt.js         |     100 |      100 |     100 |     100 |                   
  response.js       |     100 |      100 |     100 |     100 |                   
  verification.js   |     100 |      100 |     100 |     100 |                   
--------------------|---------|----------|---------|---------|-------------------
$ tsc --allowJs --noEmit --esModuleInterop --skipLibCheck --lib es2020 index.js
$ tsd

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature or request pull request welcome Issue is actionable and anyone can claim it
Projects
None yet
Development

No branches or pull requests

3 participants