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

ui: add params to pki parser #18760

Merged
merged 23 commits into from
Jan 24, 2023
Merged

Conversation

hellobontempo
Copy link
Contributor

@hellobontempo hellobontempo commented Jan 18, 2023

  • Deletes pki/certificate.js in favor of pki/base/certificate.js
  • Removes issueDate as an attr to consistently use notValidBefore
  • Adds parsed certificate (screenshot of console.log):

To Do:

  • Follow-on ticket to add these attributes: key_usage, ip_sans, key_bits and key_id

Screen Shot 2023-01-18 at 2 42 21 PM

@hellobontempo hellobontempo marked this pull request as ready for review January 18, 2023 22:40
Copy link
Contributor

@zofskeez zofskeez left a comment

Choose a reason for hiding this comment

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

Nice getting in those extra fields! I don't have much of an idea of how all that parsing is working but it looks good to me. I left some comments on the smaller items and one change request based on the attr type change.

ui/app/models/pki/certificate/base.js Outdated Show resolved Hide resolved
ui/app/styles/components/form-section.scss Outdated Show resolved Hide resolved
@@ -20,7 +20,6 @@ import { next } from '@ember/runloop';
* @param {string} type=array - Optional type for inputValue.
* @param {string} attrName - We use this to check the type so we can modify the tooltip content.
* @param {string} subText - Text below the label.
* @param {boolean} hideFormSection - If true do not add form-section class on surrounding div.
Copy link
Contributor

Choose a reason for hiding this comment

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

Rather than using the hideFormSection arg is the intention to now apply the is-box-shadowless class directly to the component? I'm wondering if that will be unclear for anyone who hasn't worked with this component before.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hideFormSection was added to remove the bottom shadow from string-list components within toggles
before - screenshot of before:

Screen Shot 2023-01-18 at 3 43 15 PM

I guess it depends how we want to approach component styling. It didn't seem necessary to add a whole new argument, that was passed via attr.options since instead we could add relevant css styling directly to the component using splattributes. But if the preference is to use a component arg instead, I can revert!

Copy link
Contributor

Choose a reason for hiding this comment

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

I prefer the class approach but was just looking for more context. The components that are used within FormField seem sensitive to change since they are so tightly coupled to the attr options but if we always want that shadow hidden within a form than this is a better approach.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree they're fragile! I appreciate the extra consideration - I've definitely made css changes before that were not great elsewhere

@attr('number', { formatDate: true }) notValidAfter;
@attr('number', { formatDate: true }) notValidBefore;
@attr('number', { formatDate: true, label: 'Issue date' }) notValidBefore;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we want this to still be labelled as Not Valid Before?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I pulled this from the issuer view - but I agree. And it will hopefully help mitigate any future confusion!

@zofskeez zofskeez self-requested a review January 19, 2023 15:27
Copy link
Contributor

@zofskeez zofskeez left a comment

Choose a reason for hiding this comment

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

🚀

// TODO wrap to catch errors, only parse if cross-signing?
// for cross-signing
const { alt_names, uri_sans } = parseExtensions(cert?.extensions);
const [signature_bits, use_pss] = mapSignatureBits(cert?.signatureAlgorithm);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@cipherboy can we use signature_bits to get the key_bits and key_type info?

Copy link
Contributor

@cipherboy cipherboy Jan 20, 2023

Choose a reason for hiding this comment

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

I have good news, and bad news. :-)

The bad news is, sorry that won't work. The signature's key type (RSA/...) is based on the parent issuer's CA type, which we're changing here (from an old parent issuer to a new one to form the cross-signed chain). The key_bits and key_type talk about the subject's properties (i.e., the Subject Public Key Info field), and so would be inputs for generation of the new CSR (prior to passing it to the new parent CA).

The good news is, since we're using an existing key (intermediate/generate/existing), you don't actually need to pass key_type and key_bits like @kitography does (it already exists in Vault). Kit's work is also implementing reissuance and not just cross-signing (the former can rotate key material, the latter leaves it alone).

The use_pss and signature_bits are still necessary though to tell the new parent CA what to do. :-)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah that makes sense! I totally should have realized that when I was playing around with key_type and key_bit I had to use a different endpoint, which is not what we use for cross-signing - thank you for the reminder!! (I should really not work after 8:30pm 🥲 )

YAY, I love the sound of not having to pass them 🎉 😄

@hellobontempo hellobontempo enabled auto-merge (squash) January 24, 2023 00:31
@hellobontempo hellobontempo merged commit 92cc175 into main Jan 24, 2023
@hellobontempo hellobontempo deleted the ui/VAULT-12768/add-params-to-pki-parser branch January 24, 2023 01:11
jayant07-yb pushed a commit to jayant07-yb/hashicorp-vault-integrations that referenced this pull request Mar 15, 2023
* refactor parser to pull serial number from subject

* refactor pki parser

* uninstall pvtutils

* remove hideFormSection as attr

* remove hideFormSection as attr

* add string-list

* test removing issueDate

* update tests

* final answer - make number types

* change to unix time - since valueOf() is typically used internally

* add algo mapping

* add comment to complete in followon

* add attrs to pki parser

* add conditional operands so parser continues when values dont exist

* add error handling WIP

* finish tests, add error handling

* revert to helper

* move helper to util

* add parseSubject test

* finish tests

* move certs to pki helper file

* wrap parsing functions in try...catch
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants