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

Make raw p256 private key import work correctly #4

Merged
merged 3 commits into from
Oct 13, 2023
Merged

Make raw p256 private key import work correctly #4

merged 3 commits into from
Oct 13, 2023

Conversation

r-n-o
Copy link
Collaborator

@r-n-o r-n-o commented Oct 13, 2023

Finally! I think p256 raw private key import works correctly. After a false start (nodejs/node#50174) I realized I had to bite the bullet and implement p256 derivation. Luckily it's something I've implemented in raw JS before, for Bitcoin! https://github.com/ArnaudBrousseau/arnaudbrousseau.com/blob/master/static/labs/keys.deconstructed/keys.deconstructed.js

And because this derivation isn't security sensitive (the only "bad" thing that can happen is the actual call to crypto.subtle.importKey will fail), I feel confident about this approach (as opposed to importing a library, which means we lose the self-contained nature of this page)

Note that we already test this with the unit tests imports recovery credentials correctly. They used to pass because nodeJS is able to import bare pkcs8 private keys. Now they pass because the key derivation logic is sound/working!

@r-n-o r-n-o requested a review from emostov October 13, 2023 16:45
index.html Outdated Show resolved Hide resolved
@cloudflare-workers-and-pages
Copy link

cloudflare-workers-and-pages bot commented Oct 13, 2023

Deploying with  Cloudflare Pages  Cloudflare Pages

Latest commit: af93206
Status: ✅  Deploy successful!
Preview URL: https://024d049f.recovery-br3.pages.dev
Branch Preview URL: https://rno-p256.recovery-br3.pages.dev

View logs

index.html Show resolved Hide resolved
index.html Show resolved Hide resolved
index.html Show resolved Hide resolved
index.html Show resolved Hide resolved
Copy link
Contributor

@emostov emostov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall looks good! The comments for the p256 stuff where very helpful

@r-n-o r-n-o merged commit ca23781 into main Oct 13, 2023
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants