Pad SDK browser Import Keys #323
Conversation
|
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. |
| } | ||
| if (point[0] !== 2 && point[0] !== 3) { | ||
| throw new Error("invalid format"); | ||
| const compressedLength = fieldSize + 1; |
There was a problem hiding this comment.
This looks like a lot of new code, and we're in a vendored file! At this point it'd be really hard for someone to understand how to upgrade tink if we ever had to.
Still, can you add a summary of what was done in this PR at the top of the file?
There was a problem hiding this comment.
Ah yeah wanted to follow the earlier suggestion of moving the uncompressed Pub key point into pointDecode -- didn't realize this is a vendored file until later will definitely add some context on top.
Also -- it seems like we've changed the file significantly from what the original implementation is for our own purposes. The original implementation also does have the capacity to check uncompressed points!
There was a problem hiding this comment.
(all this is to say that our implementation is already quite different from what's in the original, and this actually kinda brings it closer to the original)
| uint8ArrayToHexString, | ||
| } from "@turnkey/encoding"; | ||
|
|
||
| test("pointDecode -> uncompressed invalid", async function () { |
There was a problem hiding this comment.
I don't see a test for compressed keys and invalid lengths cases. Let's cover these?
There was a problem hiding this comment.
I'll add that in!
| expect(jwk.x).toBeDefined(); | ||
| expect(jwk.y).toBeDefined(); | ||
|
|
||
| // Convert x value to make sure it matches first half of uncompressed key WITHOUT truncating the first 0 bit |
There was a problem hiding this comment.
This is great! If you'd like, you could also try importing the resulting JWK as a sanity check as well (e.g. the crypto.subtle.importKey... codepath that happens in packages/sdk-browser/src/utils.ts).
Note: if you do go down this path, you might need to import crypto to have it defined in the test setting:
Object.defineProperty(globalThis, "crypto", {
value: {
getRandomValues: (arr) => crypto.randomBytes(arr.length),
subtle: crypto.subtle,
},
});
Can use packages/api-key-stamper/src/__tests__/utils-test.ts in this diff as a reference
There was a problem hiding this comment.
Totally fine if you think this is overkill given some of the manual validation you're doing here
There was a problem hiding this comment.
Thanks for the suggestion here!! I'll leave the manual validation as is for right now so we can get this over the line in a timely manner
Summary & Motivation
Some JWK buffers that we construct need to be manually padded because they may get truncated during conversion between different formats in the case that the first byte(s) are 0's. This protects against error cases where the JWK components are found to be less than 32 bytes that some clients seem to run into.
Some similar changes:
How I Tested These Changes
Did you add a changeset?
Yes -- changeset has been added
If updating one of our packages, you'll likely need to add a changeset to your PR. To do so, run
pnpm changeset.pnpm changesetwill 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.