padding fixes #321

andrewkmin · 2024-08-06T19:12:27Z

Summary & Motivation

$title

There are cases where we want to manually pad buffers to get to a certain length, primarily when we're dealing with jwks.

The nature of this change is similar in spirit to another change we recently rolled out for our iframes: tkhq/frames#44

How I Tested These Changes

Unit, smoke via #322

Did you add a changeset?

If updating one of our packages, you'll likely need to add a changeset to your PR. To do so, run pnpm changeset. pnpm changeset will generate a file where you should write a human friendly message about the changes. Note how this (example) includes the package name (should be auto added by the command) along with the type of semver change (major.minor.patch) (which you should set).

These changes will be used at release time to determine what packages to publish and how to bump their version. For more context see this comment.

codesandbox-ci · 2024-08-06T19:13:36Z

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

andrewkmin · 2024-08-06T19:26:43Z

packages/http/src/webauthn-json/base64url.ts

@@ -17,7 +17,7 @@ export function base64urlToBuffer(
  for (let i = 0; i < str.length; i++) {
    byteView[i] = str.charCodeAt(i);
  }
-  return buffer;
+  return byteView;


This is unrelated to this specific change, but up until now this function was essentially just returning an empty buffer.

moe-dev

LGTM assuming tests pass!

moe-dev · 2024-08-06T20:31:45Z

packages/api-key-stamper/src/utils.ts

+  uint8ArrayToHexString,
+} from "@turnkey/encoding";
+
+const DEFAULT_JWK_MEMBER_BYTE_LENGTH = 32;


super tiny nit, if we plan on setting this in multiple files maybe we can export it from @turnkey/encoding

andrewkmin · 2024-08-07T16:18:07Z

packages/api-key-stamper/src/__tests__/utils-test.ts

+});
+
+test("correctly converts turnkey API key to JWK", function () {
+  for (let i = 0; i < 100; i++) {


Jest doesn't like indefinite (while: true) loops, so just picked an arbitrary fairly large value here

andrewkmin · 2024-08-07T16:18:36Z

packages/api-key-stamper/src/utils.ts

-    uint8ArrayToHexString(new Uint8Array(decodedX)),
-    DEFAULT_JWK_MEMBER_BYTE_LENGTH
-  );
+  // // Manipulate x and y


temporarily commenting this logic out in order to be able to reproduce the issue when converting turnkey API key to jwk

r-n-o · 2024-08-09T20:27:31Z

packages/api-key-stamper/src/utils.ts

+  // If a length is specified, ensure we sufficiently pad
+  let paddedBuffer = new Uint8Array(length);
+  paddedBuffer.set(buffer, length - buffer.length);
+  return paddedBuffer;


I think it's important to handle cases where length is less than buffer.length. In this case, maybe an error?

r-n-o · 2024-08-09T20:28:31Z

packages/api-key-stamper/src/utils.ts

+  jwkCopy.d = paddedD;
+  jwkCopy.x = paddedX;
+  jwkCopy.y = paddedY;


What led to padding (x, y, d) as opposed to just d? Given the jwk is already created I think we're good with the existing (x, y) components within it?

r-n-o · 2024-08-09T20:30:50Z

packages/api-key-stamper/src/utils.ts

@@ -9,12 +15,39 @@ export function convertTurnkeyApiKeyToJwk(input: {

  const jwk = pointDecode(hexStringToUint8Array(compressedPublicKeyHex));


If x and y aren't padded for whatever reason, let's fix this inside of pointDecode so we don't have to extract, pad, and then put things "back" in the JWK:

sdk/packages/api-key-stamper/src/tink/elliptic_curves.ts

Lines 154 to 155 in 08e2b8c

x: Bytes.toBase64(integerToByteArray(x), /* websafe */ true),

y: Bytes.toBase64(integerToByteArray(y), /* websafe */ true),

r-n-o · 2024-08-14T15:04:20Z

packages/api-key-stamper/src/tink/elliptic_curves.ts

+import {
+  DEFAULT_JWK_MEMBER_BYTE_LENGTH,
+  uint8ArrayFromHexString,
+} from "@turnkey/encoding";


Mmm, this seems a bit off because we have a vendor'd library import our encoding lib :/

r-n-o · 2024-08-14T15:05:35Z

packages/api-key-stamper/src/tink/elliptic_curves.ts

@@ -34,12 +38,20 @@ function byteArrayToInteger(bytes: Uint8Array): bigint {
  return BigInt("0x" + Bytes.toHex(bytes));
 }

-/** Converts bigint to byte array. */
-function integerToByteArray(i: bigint): Uint8Array {
+/** Converts bigint to byte array. This implementation has been modified to optionally augment the resulting byte array to a certain length. */


Let's insert this comment at the top of the file so it's obvious (in case someone is trying to update this file down the road)

r-n-o · 2024-08-14T15:15:49Z

packages/api-key-stamper/src/tink/elliptic_curves.ts

+  if (!length) {
+    return Bytes.fromHex(input);
+  }
+  if (input.length / 2 > length) {
+    throw new Error(
+      "hex value cannot fit in a buffer of " + length + " byte(s)"
+    );
+  }
+  return uint8ArrayFromHexString(input, length);


I don't think this is necessary. Let's require the length argument (integerToByteArray isn't called anywhere with an undefined length), and pad with 0 without relying on our library function. We can also remove the special case of an odd-length input and just rely on generic padding:

function integerToByteArray(i: bigint, length: number): Uint8Array { let input = i.toString(16); let numHexChars = length * 2 if numHexChars < input.length { throw new Error(`cannot pack integer with ${input.length} hex chars into$ {length} bytes`); } else { let padding = "0".repeat(numHexChars - input.length); } return Bytes.fromHex(padding + input); }

r-n-o · 2024-08-14T15:17:48Z

packages/encoding/src/index.ts

 export function stringToBase64urlString(input: string): string {
  // string to base64 -- we do not rely on the browser's btoa since it's not present in React Native environments
  const base64String = btoa(input);
  return base64StringToBase64UrlEncodedString(base64String);
 }

+export function base64urlToBuffer(baseurl64String: string): ArrayBuffer {


Aside from tests this isn't used anywhere

(same as #321 (comment))

r-n-o · 2024-08-14T15:18:28Z

packages/encoding/src/index.ts

+    baseurl64String.replace(/-/g, "+").replace(/_/g, "/") + padding;
+
+  // Base64 to binary string
+  const str = atob(base64String);


Generally we should avoid relying on things that are "magically" available. This may work in Node but might not work in React Native, for example (I'd like our core "encoding" functions to be pure JS so we don't have to deal with polyfills or worry about different implementations across JS runtimes)

That makes sense! This also was cut/pasted from packages/http/src/webauthn-json/base64url.ts / packages/webauthn-stamper/src/webauthn-json/base64url.ts . But if this is "over-consolidation", happy to take this out. That and the fact that this isn't pure JS

I see. I lean towards deleting these two functions because they come from a vendored lib https://github.com/tkhq/sdk/blob/main/packages/http/src/webauthn-json/README.md

This will make this diff smaller too! :)

r-n-o · 2024-08-14T15:19:40Z

packages/encoding/src/index.ts

+  return byteView;
+}
+
+export function bufferToBase64url(buffer: ArrayBuffer): string {


Same here. This doesn't seem to be used anywhere outside of the new tests.

I was planning on consolidating various utils in packages/encoding instead of having the same util functions in http (specifically packages/http/src/webauthn-json/base64url.ts)

ended up removing for now! maybe we can move it back, consolidate utilities in encoding down the line, along with a pureJS implementation of atob (similar to the btoa impl we have vendored in)

r-n-o · 2024-08-14T17:23:13Z

packages/api-key-stamper/src/tink/elliptic_curves.ts

@@ -1,5 +1,6 @@
 /**
 * Code modified from https://github.com/google/tink/blob/6f74b99a2bfe6677e3670799116a57268fd067fa/javascript/subtle/elliptic_curves.ts
+ * - The implementation of integerToByteArray has been modified to optionally augment the resulting byte array to a certain length.


not optionally anymore :)

r-n-o · 2024-08-14T17:26:28Z

packages/encoding/src/index.ts

+    baseurl64String.replace(/-/g, "+").replace(/_/g, "/") + padding;
+
+  // Base64 to binary string
+  const str = atob(base64String);


I see. I lean towards deleting these two functions because they come from a vendored lib https://github.com/tkhq/sdk/blob/main/packages/http/src/webauthn-json/README.md

This will make this diff smaller too! :)

r-n-o

Looks good!

andrewkmin commented Aug 6, 2024

View reviewed changes

andrewkmin changed the title ~~Andrew/padding fixes~~ padding fixes Aug 6, 2024

andrewkmin requested review from moe-dev, r-n-o and taylorjdawson August 6, 2024 20:23

moe-dev approved these changes Aug 6, 2024

View reviewed changes

andrewkmin commented Aug 7, 2024

View reviewed changes

andrewkmin force-pushed the andrew/padding-fixes branch from 2e4fa27 to 0e269bc Compare August 7, 2024 20:50

r-n-o reviewed Aug 9, 2024

View reviewed changes

andrewkmin force-pushed the andrew/padding-fixes branch 2 times, most recently from 376ac71 to 2425f82 Compare August 10, 2024 02:00

andrewkmin added 6 commits August 13, 2024 17:18

move some encoding helpers to utils

705040f

update api key stamper jwk logic

b7f1d2e

format and minor fix

a796f1b

add unit tests

7f600d2

ensure that d is padded as well

28b3cac

feedback: move+consolidate logic

c6d7a1e

andrewkmin force-pushed the andrew/padding-fixes branch from 2425f82 to c6d7a1e Compare August 14, 2024 00:18

andrewkmin requested a review from r-n-o August 14, 2024 01:05

add changeset

2d7e5a9

andrewkmin force-pushed the andrew/padding-fixes branch from 96042d0 to 2d7e5a9 Compare August 14, 2024 13:36

r-n-o reviewed Aug 14, 2024

View reviewed changes

feedback: clean up tink implementation

f158924

andrewkmin mentioned this pull request Aug 14, 2024

add example for testing padding fixes #322

Open

feedback: remove currently unused utils from packages/encoding

883dc26

r-n-o reviewed Aug 14, 2024

View reviewed changes

r-n-o approved these changes Aug 14, 2024

View reviewed changes

remove optionally comment

8c0f6ab

andrewkmin merged commit 1b0306d into main Aug 14, 2024
5 checks passed

andrewkmin deleted the andrew/padding-fixes branch August 14, 2024 20:56

omkarshanbhag mentioned this pull request Aug 15, 2024

Pad SDK browser Import Keys #323

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

padding fixes #321

padding fixes #321

andrewkmin commented Aug 6, 2024 •

edited

Loading

codesandbox-ci bot commented Aug 6, 2024 •

edited

Loading

andrewkmin Aug 6, 2024

moe-dev left a comment

moe-dev Aug 6, 2024

andrewkmin Aug 7, 2024

andrewkmin Aug 7, 2024

r-n-o Aug 9, 2024

r-n-o Aug 9, 2024 •

edited

Loading

r-n-o Aug 9, 2024

r-n-o Aug 14, 2024

r-n-o Aug 14, 2024

r-n-o Aug 14, 2024

r-n-o Aug 14, 2024

andrewkmin Aug 14, 2024

r-n-o Aug 14, 2024

andrewkmin Aug 14, 2024

r-n-o Aug 14, 2024

r-n-o Aug 14, 2024

andrewkmin Aug 14, 2024

andrewkmin Aug 14, 2024

r-n-o Aug 14, 2024

r-n-o Aug 14, 2024

r-n-o left a comment

		@@ -9,12 +15,39 @@ export function convertTurnkeyApiKeyToJwk(input: {

		const jwk = pointDecode(hexStringToUint8Array(compressedPublicKeyHex));

	x: Bytes.toBase64(integerToByteArray(x), /* websafe */ true),
	y: Bytes.toBase64(integerToByteArray(y), /* websafe */ true),

padding fixes #321

padding fixes #321

Conversation

andrewkmin commented Aug 6, 2024 • edited Loading

Summary & Motivation

How I Tested These Changes

Did you add a changeset?

codesandbox-ci bot commented Aug 6, 2024 • edited Loading

Choose a reason for hiding this comment

moe-dev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

r-n-o Aug 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

r-n-o left a comment

Choose a reason for hiding this comment

andrewkmin commented Aug 6, 2024 •

edited

Loading

codesandbox-ci bot commented Aug 6, 2024 •

edited

Loading

r-n-o Aug 9, 2024 •

edited

Loading