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

Project: Web platform compatibility (Browsers, Deno, Cloudflare Workers) #475

Closed
gr2m opened this issue Feb 17, 2021 · 8 comments · Fixed by #533
Closed

Project: Web platform compatibility (Browsers, Deno, Cloudflare Workers) #475

gr2m opened this issue Feb 17, 2021 · 8 comments · Fixed by #533
Labels
project Goes beyond a single bug fix or new feature. Help welcome by existing contributors.

Comments

@gr2m
Copy link
Contributor

gr2m commented Feb 17, 2021

@octokit/webhooks can already be used in browser's today, but the build size is huge, because Node's crypto API needs to be polyfilled. We need to create a universal library to sign a JSON payload, which uses the Web Crypto API for browser and Deno builds. This will make @octokit/webhooks and other libraries and framework that use it such as @octokit/app and probot work better with Cloudflare Workers. It is working today (example), but the build size could be reduced by 90%+. Another API change that would improve @octokit/webhooks's compatibility with browser/Deno/Cloudflare Workers is to remove the webhooks.middleware API, and replace it with a dedicated createNodeMiddleware export, similar to what we do with @octokit/app.

I created a similar package for the JSON Web Token authentication used by GitHub Apps: universal-github-app-jwt.

The challenge for this module is that different environment should receive different code. In the past, the "browse" package field was used by several build tools. But with the coming support of ES Modules by Node.js, we have a convention that the build tools are adapting now, which is to use the "exports" key, see https://docs.skypack.dev/package-authors/package-checks#export-map

It might be a good time to just create a native ES Module library without any build step at this point, so that we have full control over the package.json file published to npm. The @pika/pack build tool we are currently using in @octokit does not support the "exports" key at this point, and the module is currently not maintained by the Pika/Snowpack team

@gr2m gr2m added the project Goes beyond a single bug fix or new feature. Help welcome by existing contributors. label Feb 17, 2021
@gr2m
Copy link
Contributor Author

gr2m commented Apr 1, 2021

for future reference, here is what we do today in node

const crypto = require("crypto");

function sign(secret, data) {
  return crypto.createHmac("sha256", secret).update(data).digest("hex");
}

and here is what we need to do in browsers, and soon Deno

// credit: https://stackoverflow.com/a/47332317/206879
var enc = new TextEncoder("utf-8");

async function sign(secret, data) {
  const key = await window.crypto.subtle.importKey(
    "raw", // raw format of the key - should be Uint8Array
    enc.encode(secret),
    {
      // algorithm details
      name: "HMAC",
      hash: { name: "SHA-256" },
    },
    false, // export = false
    ["sign", "verify"] // what this key can do
  );
  const signature = await window.crypto.subtle.sign(
    "HMAC",
    key,
    enc.encode(data)
  );
  var b = new Uint8Array(signature);
  return Array.prototype.map
    .call(b, (x) => ("00" + x.toString(16)).slice(-2))
    .join("");
}

@gr2m
Copy link
Contributor Author

gr2m commented Apr 1, 2021

In node we do some fancy timesafe equal, I'm not sure if we need that in the browser, I don't fully understand what this is actually doing, so if someone knows better, please let me know if this naive version of signature string verification will suffice or not

const enc = new TextEncoder("utf-8");

async function importKey(secret) {
  return window.crypto.subtle.importKey(
    "raw", // raw format of the key - should be Uint8Array
    enc.encode(secret),
    {
      // algorithm details
      name: "HMAC",
      hash: { name: "SHA-256" },
    },
    false, // export = false
    ["sign", "verify"] // what this key can do
  );
}

function UInt8ArrayToHex(signature) {
  return Array.prototype.map
    .call(new Uint8Array(signature), (x) => x.toString(16).padStart(2, "0"))
    .join("");
}

async function sign(secret, data) {
  const signature = await window.crypto.subtle.sign(
    "HMAC",
    await importKey(secret),
    enc.encode(data)
  );
  return UInt8ArrayToHex(signature);
}

async function verify(secret, data, signature) {
  return signature === (await sign(secret, data));
}

@gr2m
Copy link
Contributor Author

gr2m commented Apr 1, 2021

Looks like webcrypto is coming to Node, too: https://nodejs.org/api/webcrypto.html

@wolfy1339
Copy link
Member

In node we do some fancy timesafe equal, I'm not sure if we need that in the browser, I don't fully understand what this is actually doing

There is a great explanation here on StackOverflow of what a time safe equal accomplishes

@gr2m
Copy link
Contributor Author

gr2m commented Apr 1, 2021

I guess this is a better way of verification? I dunno 🤷🏼

function hexToUInt8Array(string) {
  // convert string to pairs of 2 characters
  const pairs = string.match(/[\dA-F]{2}/gi);

  // convert the octets to integers
  const integers = pairs.map(function (s) {
    return parseInt(s, 16);
  });

  return new Uint8Array(integers);
}

async function verify(secret, data, signature) {
  return await window.crypto.subtle.verify(
    "HMAC",
    await importKey(secret),
    hexToUInt8Array(signature),
    enc.encode(data)
  );
}

In node we do some fancy timesafe equal, I'm not sure if we need that in the browser, I don't fully understand what this is actually doing

There is a great explanation here on StackOverflow of what a time safe equal accomplishes

this is great, I'll add a reference to the code

@gr2m
Copy link
Contributor Author

gr2m commented Apr 10, 2021

I'll start by moving sign and verify into a separate module. I want to move forward with this because I'm experimenting with running webhooks in a browser, and the usage of crypto is currently breaking the import from Skypack

@gr2m
Copy link
Contributor Author

gr2m commented Apr 10, 2021

https://github.com/octokit/webhooks-methods.js/#readme is ready. Not yet compatible with Deno, but that will be the fact as soon as Deno implements the crypto API

I'll use the package as part of #518. sign and verify now need to be asynchronous which is a breaking change.

This was referenced Apr 10, 2021
@gr2m gr2m closed this as completed in #533 Apr 14, 2021
@github-actions
Copy link
Contributor

🎉 This issue has been resolved in version 9.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
project Goes beyond a single bug fix or new feature. Help welcome by existing contributors.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants