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.

Sign up for GitHub

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

Jump to bottom

Switch from node-forge to PKI.js #13894

Merged
merged 4 commits into from
Feb 4, 2022
Merged

Switch from node-forge to PKI.js #13894

merged 4 commits into from
Feb 4, 2022

Conversation

cipherboy
Copy link
Contributor

@cipherboy cipherboy commented Feb 3, 2022 •
edited
Loading

Definitely needing some review from the UI team here :-D I feel I shouldn't have so many changes to package.json, but maybe I can get some thoughts on that?

After breaking a few things, I think I ended up doing the equivalent of:

git clean -xdf # to reset node_modules to a clean checkout...
# some command to install all the missing deps again that might've also updated stuff?
npm install --save-dev asn1js pvutils pkijs

to get the dependencies to install and build fine with make static-dist.

I've roughly followed the following procedure after building Vault with the built-in UI:

source devvault
vault secrets enable pki
vault write pki/root/generate/internal common_name=example.com key_type=ed25519
# (test this change in web UI)
vault delete pki/root
vault write pki/root/generate/internal common_name=example.com key_type=ec key_bits=256
# (test this change in web UI)
vault delete pki/root
vault write pki/root/generate/internal common_name=example.com key_type=ec key_bits=384
# (test this change in web UI)
vault delete pki/root
vault write pki/root/generate/internal common_name=example.com key_type=ec key_bits=521
# (test this change in web UI)
vault delete pki/root
vault write pki/root/generate/internal common_name=example.com key_type=rsa
# (test this change in web UI)

I left the console.log in case someone has issues parsing certificates in the future, we'll at least be able to point them to a log statement they can share along with the original cert. Let me know if that should be removed.

Resolves: #13680 (if merged).

/cc @hellobontempo @hashishaw

@vercel vercel bot temporarily deployed to Preview – vault February 3, 2022 21:53 Inactive
@cipherboy
Copy link
Contributor Author

Thanks @hashishaw for helping to clean up that package.json file. :D

I should probably add a changelog now? Are there any tests or documentation that need to be updated?

@hashishaw
Copy link
Collaborator

Thanks @hashishaw for helping to clean up that package.json file. :D

I should probably add a changelog now? Are there any tests or documentation that need to be updated?

Changelog would be excellent, and it looks like our acceptance tests are still passing which means you successfully switch out the libraries while maintaining expected functionality 🎉

Thanks again for tackling this!

hashishaw
hashishaw approved these changes Feb 3, 2022
View reviewed changes
Copy link
Collaborator

@hashishaw hashishaw left a comment

Choose a reason for hiding this comment

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

Great inline comments. Add a changelog and we'll be good to go! 🚀

@hashishaw hashishaw added this to the 1.10 milestone Feb 3, 2022
@hashishaw hashishaw added bug Used to indicate a potential bug and removed bug Used to indicate a potential bug labels Feb 3, 2022
@cipherboy
Switch parse-pki-cert from node-forge to PKI.js
40e1c7c 
This replaces the implementation of parse-pki-cert to use PKI.js rather
than node-forge for two reasons:

 - PKI.js uses Web Crypto rather than maintaining a built-in
   implementation of several algorithms.
 - node-forge presently lacks support for ECDSA and Ed25519
   certificates.

Related: #13680

Signed-off-by: Alexander Scheel <[email protected]>
@cipherboy
Add dependency on PKI.js
88dda37 
$ yarn add -D asn1js pvutils pkijs

Signed-off-by: Alexander Scheel <[email protected]>
@cipherboy
Remove dependency on node-forge
8f17f8b 
$ yarn remove node-forge

Signed-off-by: Alexander Scheel <[email protected]>
@cipherboy
Add changelog entry
eae7863 
Signed-off-by: Alexander Scheel <[email protected]>
@vercel vercel bot temporarily deployed to Preview – vault February 3, 2022 23:13 Inactive
@vercel vercel bot temporarily deployed to Preview – vault-storybook February 3, 2022 23:13 Inactive
cipherboy
cipherboy commented Feb 3, 2022
View reviewed changes
ui/app/helpers/parse-pki-cert.js
// CommonName field, this is usually a PrintableString, BMPString, or a
// UTF8String. Regardless of encoding, it should be present in the
// valueBlock's value field if it is renderable.
const commonNameOID = '2.5.4.3';
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Linting hadn't run the first time, but it did now :-D

Copy link
Contributor

Choose a reason for hiding this comment

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

thank you for breaking this down and making is so clear to follow. 😍

hellobontempo
hellobontempo reviewed Feb 4, 2022
View reviewed changes
changelog/13894.txt
@@ -0,0 +1,3 @@
```release-note:improvement
Copy link
Contributor

Choose a reason for hiding this comment

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

🎉

cipherboy
cipherboy commented Feb 4, 2022
View reviewed changes
ui/app/helpers/parse-pki-cert.js
const commonNameOID = '2.5.4.3';
const commonNames = cert?.subject?.typesAndValues
.filter((rdn) => rdn?.type === commonNameOID)
.map((rdn) => rdn?.value?.valueBlock?.value);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

In case anyone was wondering, this does appear to be the correct way to parse the RDN's value: https://github.com/PeculiarVentures/PKI.js/blob/master/src/AttributeTypeAndValue.js#L213-L214

@cipherboy cipherboy merged commit 6fab385 into main Feb 4, 2022
@cipherboy
Copy link
Contributor Author

Thanks @hashishaw and @hellobontempo!

@cipherboy cipherboy deleted the cipherboy-switch-to-pki-js branch February 4, 2022 19:45
fairclothjm pushed a commit that referenced this pull request Feb 12, 2022
@cipherboy @fairclothjm
Switch from node-forge to PKI.js (#13894)
d64bac2 
* Switch parse-pki-cert from node-forge to PKI.js

This replaces the implementation of parse-pki-cert to use PKI.js rather
than node-forge for two reasons:

 - PKI.js uses Web Crypto rather than maintaining a built-in
   implementation of several algorithms.
 - node-forge presently lacks support for ECDSA and Ed25519
   certificates.

Related: #13680

Signed-off-by: Alexander Scheel <[email protected]>

* Add dependency on PKI.js

$ yarn add -D asn1js pvutils pkijs

Signed-off-by: Alexander Scheel <[email protected]>

* Remove dependency on node-forge

$ yarn remove node-forge

Signed-off-by: Alexander Scheel <[email protected]>

* Add changelog entry

Signed-off-by: Alexander Scheel <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hellobontempo hellobontempo hellobontempo left review comments

@hashishaw hashishaw hashishaw approved these changes

Assignees
No one assigned
Labels
secret/pki ui
Projects
None yet
Milestone
1.10
Development

Successfully merging this pull request may close these issues.

PKI Engine: Error parsing metadata when creating ed25519 or ec root cert
3 participants
@cipherboy @hashishaw @hellobontempo