-
Notifications
You must be signed in to change notification settings - Fork 12
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
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some minor comments but looks good overall! 🥳 just needs some smoke testing
} | ||
if (point[0] !== 2 && point[0] !== 3) { | ||
throw new Error("invalid format"); | ||
const compressedLength = fieldSize + 1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see a test for compressed keys and invalid lengths cases. Let's cover these?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll add that in!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looking good!
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔥
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🏅
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 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.