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

UI/Fix node-forge EC error #13238

Merged
merged 12 commits into from
Nov 23, 2021
Merged

UI/Fix node-forge EC error #13238

merged 12 commits into from
Nov 23, 2021

Conversation

hellobontempo
Copy link
Contributor

@hellobontempo hellobontempo commented Nov 22, 2021 •
edited

Fix for #13219

Adds a catch for when node-forge returns an error converting or parsing a certificate, and returns unparsed certificate (so metadata will not display).

Addresses error returned when trying to read EC certs. Error: Cannot read public key. OID is not RSA.

When unable to parse certificate metadata, UI reverts to original behavior and just displays serial number and certificate:

Screen Shot 2021-11-22 at 4 19 04 PM

@vercel vercel bot temporarily deployed to Preview – vault November 22, 2021 17:13 Inactive
@vercel vercel bot temporarily deployed to Preview – vault-storybook November 22, 2021 17:13 Inactive
hashishaw
hashishaw approved these changes Nov 22, 2021
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.

Beautiful, thanks for the quick fix!

@cipherboy
Copy link
Contributor

Just to mention, we tested this via setting up two secrets engines:

$ source devvault

$ vault secrets enable pki
$ vault secrets tune -max-lease-ttl=8760h pki 
$ vault write pki/root/generate/internal common_name=my-website.com ttl=8760h key_type=ec key_bits=256

$ vault secrets enable  -path=pki2/ pki
$ vault secrets tune -max-lease-ttl=8760h pki2 
$ vault write pki2/root/generate/internal common_name=my-website.com ttl=8760h key_type=rsa

One CA was with RSA and the other was with EC. Since the root CA certificate for the mount is visible from the secrets page, we were able to confirm the original issue and that this fixed it.

@vercel vercel bot temporarily deployed to Preview – vault November 22, 2021 21:21 Inactive
@vercel vercel bot temporarily deployed to Preview – vault November 22, 2021 21:26 Inactive
@vercel vercel bot temporarily deployed to Preview – vault November 22, 2021 22:01 Inactive
@vercel vercel bot temporarily deployed to Preview – vault November 22, 2021 22:06 Inactive
@vercel vercel bot temporarily deployed to Preview – vault November 23, 2021 04:12 Inactive
@vercel vercel bot temporarily deployed to Preview – vault November 23, 2021 14:37 Inactive
@vercel vercel bot temporarily deployed to Preview – vault November 23, 2021 16:40 Inactive
Monkeychip
Monkeychip approved these changes Nov 23, 2021
View reviewed changes
Copy link
Contributor

@Monkeychip Monkeychip left a comment

Choose a reason for hiding this comment

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

Looks great. Don't forget the bug and ui label and the milestone. (unsure if this is being backported to 1.9, guessing it is?)

@vercel vercel bot temporarily deployed to Preview – vault November 23, 2021 17:15 Inactive
@hellobontempo hellobontempo linked an issue Nov 23, 2021 that may be closed by this pull request
Web-UI errors with EC certs on PKI backend #13219
Closed
@hellobontempo hellobontempo added backport bug Used to indicate a potential bug ui labels Nov 23, 2021
@hellobontempo hellobontempo added this to the 1.9.1 milestone Nov 23, 2021
@vercel vercel bot temporarily deployed to Preview – vault November 23, 2021 17:35 Inactive
@hellobontempo hellobontempo merged commit 34c16a0 into main Nov 23, 2021
hellobontempo added a commit that referenced this pull request Nov 23, 2021
@hellobontempo
UI/Fix node-forge EC error (#13238)
3dbcc65 
* add catch for node-forge error handling

* update comment

* adds changelog

* alphabetize attrs and add canParse attr

* show alert banner if unable to parse metadata

* add test to check info banner renders
@hellobontempo hellobontempo mentioned this pull request Nov 23, 2021
Merged
@hellobontempo hellobontempo deleted the ui/fix-node-forge-error branch November 23, 2021 20:17
hellobontempo added a commit that referenced this pull request Nov 23, 2021
@hellobontempo
UI/Fix node-forge EC error (#13238) (#13261)
e4bb8f7 
* add catch for node-forge error handling

* update comment

* adds changelog

* alphabetize attrs and add canParse attr

* show alert banner if unable to parse metadata

* add test to check info banner renders
@cipherboy cipherboy mentioned this pull request Feb 3, 2022
Closed
qk4l pushed a commit to qk4l/vault that referenced this pull request Feb 4, 2022
@hellobontempo
UI/Fix node-forge EC error (hashicorp#13238)
a695ea1 
* add catch for node-forge error handling

* update comment

* adds changelog

* alphabetize attrs and add canParse attr

* show alert banner if unable to parse metadata

* add test to check info banner renders
@thiskevinwang thiskevinwang mentioned this pull request Apr 25, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cipherboy cipherboy cipherboy approved these changes

@Monkeychip Monkeychip Monkeychip approved these changes

@hashishaw hashishaw hashishaw approved these changes

Assignees
No one assigned
Labels
backport bug Used to indicate a potential bug ui
Projects
None yet
Milestone
1.9.1
Development

Successfully merging this pull request may close these issues.

Web-UI errors with EC certs on PKI backend
4 participants
@hellobontempo @cipherboy @Monkeychip @hashishaw