UI: PKI generate cert from role #18300

hashishaw · 2022-12-09T21:17:40Z

This PR adds the ability to generate, revoke, and download a certificate from a role.

hellobontempo · 2022-12-09T23:01:15Z

ui/app/components/clients/attribution.js

@@ -37,7 +37,7 @@ import { inject as service } from '@ember/service';

 export default class Attribution extends Component {
  @tracked showCSVDownloadModal = false;
-  @service downloadCsv;
+  @service download;


zofskeez

Nice work! Mostly questions or suggestions on my end and nothing really blocking. If you agree, it would be nice to move the error template over to a component in the core addon prior to merging.

zofskeez · 2022-12-12T15:44:52Z

ui/app/adapters/pki/certificate/generate.js

+    const { backend, serialNumber, certificate } = snapshot.record;
+    // Revoke certificate requires either serial_number or certificate
+    const data = serialNumber ? { serial_number: serialNumber } : { certificate };
+    return this.ajax(`${this.buildURL()}/${backend}/revoke`, 'POST', { data });


Should encodePath be used for the backend value here as well?

zofskeez · 2022-12-12T15:56:06Z

ui/app/helpers/parse-pki-cert.js

+export function parsePkiCert([model]) {
+  // model has to be the responseJSON from PKI serializer
+  // return if no certificate or if the "certificate" is actually a CRL
+  if (!model.certificate || model.certificate.includes('BEGIN X509 CRL')) {
+    return;
+  }


Would it be better to pass the certificate value in here rather than the model?

I was trying to rearrange in such a way that it was as noninvasive to other areas as possible. Given that, I think we will likely deprecate this particular method for the one that receives only the cert when we get to that area of the new PKI engine

zofskeez · 2022-12-12T15:59:36Z

ui/app/models/pki/certificate/base.js

+
+const certDisplayFields = ['certificate', 'commonName', 'serialNumber', 'notValidAfter', 'notValidBefore'];
+
+@withFormFields(certDisplayFields, null)


Small nit but the second arg to this decorator can be omitted unless it's an array.

zofskeez · 2022-12-12T16:04:23Z

ui/app/services/download.ts

@@ -0,0 +1,36 @@
+import Service from '@ember/service';
+
+export default class DownloadService extends Service {


Nice making this flexible for different file types! 🎉

zofskeez · 2022-12-12T16:05:08Z

ui/app/services/secret-mount-path.js

@@ -3,12 +3,12 @@ import Service from '@ember/service';
 // this service tracks the path of the currently viewed secret mount
 // so that we can access that inside of engines where parent route params
 // are not accessible
-export default Service.extend({
-  currentPath: null,
+export default class SecretMountPath extends Service {


zofskeez · 2022-12-12T16:09:58Z

ui/lib/pki/addon/components/pki-role-generate.ts

+  model: PkiCertificateGenerateModel;
+}
+
+interface PkiCertificateGenerateModel {


TS question. If the model was typescript would this interface belong in that file instead?

Yes, ideally once the models are TS we can just import from there! For now we're just making the component happy :)

zofskeez · 2022-12-12T16:13:14Z

ui/lib/pki/addon/components/pki-role-generate.ts

+  }
+
+  @action cancel() {
+    this.args.model.deleteRecord();


Curious if unloadRecord would be more appropriate here? deleteRecord marks the record as deleted but I don't think it will remove it from the store or do anything else unless you call save but then it would make a delete request to the server.

Ah yes thank you, I was looking for unloadRecord!

zofskeez · 2022-12-12T16:19:59Z

ui/lib/pki/addon/templates/error.hbs

+<PageHeader as |p|>
+  <p.top>
+    <Page::Breadcrumbs @breadcrumbs={{this.breadcrumbs}} />
+  </p.top>
+  <p.levelLeft>
+    <h1 class="title is-3">
+      <Icon @name="file-text" @size="24" class="has-text-grey-light" />
+      {{this.title}}
+    </h1>
+  </p.levelLeft>
+</PageHeader>
+<div class="tabs-container box is-bottomless is-marginless is-fullwidth is-paddingless">
+  <nav class="tabs" aria-label="secret tabs">
+    <ul>
+      {{#each this.tabs as |oTab|}}
+        <LinkTo @route={{oTab.route}} data-test-tab={{oTab.route}}>
+          {{oTab.label}}
+        </LinkTo>
+      {{/each}}
+    </ul>
+  </nav>
+</div>
+
+<div class="box is-sideless has-background-white-bis has-text-grey has-text-centered has-tall-padding" data-test-pki-error>
+  {{#if (eq this.model.httpStatus 404)}}
+    <h1 class="title is-3 has-text-grey">
+      404 Not Found
+    </h1>
+    <p>Sorry, we were unable to find any content at <code>{{or this.model.path this.path}}</code>.</p>
+  {{else if (eq this.model.httpStatus 403)}}
+    <h1 class="title is-3 has-text-grey">
+      Not authorized
+    </h1>
+    <p>You are not authorized to access content at <code>{{or this.model.path this.path}}</code>.</p>
+  {{else}}
+    <h1 class="title is-3 has-text-grey">
+      Error
+    </h1>
+    <p>
+      {{#if this.model.message}}
+        <p>{{this.model.message}}</p>
+      {{/if}}
+      {{#each this.model.errors as |error|}}
+        <p>{{error}}</p>
+      {{/each}}
+    </p>
+  {{/if}}
+</div>


This is something I hadn't considered within an engine. I'm wondering if this could be moved to a component in the core addon for wider use and consistency across engines that might need it?

This was sort of a rogue change on my part, and I didn't put it in the addon as a component because we already have a components in Vault that should cover this use case, except they didn't move gracefully into addons for use in engines.

I'm not opposed to making a generic-ish component, but I think it should be as a part of cleaning up the existing ones: BlockError, NotFound (why does it take an entire model 😭), Clients::Error, Error template can/should all use or be replaced by the new generic addon error component

Cool ya I agree with that but out of scope right now. I realized after the fact that the error template from the secrets backend route is being displayed in the kubernetes engine.

zofskeez · 2022-12-12T17:54:46Z

I was testing in the k8s engine and I'm seeing a generic 404 page so if your error template is specific to PKI please ignore my comment on moving to the core addon 😁

hashishaw added 13 commits December 7, 2022 08:58

Update secret-mount-path service to native class syntax

f91c3c0

Add error state for PKI engine

91f845d

Generate cert from role page

2d4102d

Refactor parse-pki-cert and download CSV service

44b05ac

Update form-field-groups-loop to take keyName for groups

c23fa77

Add PKI engine error page

2b07ddf

Role generate form and display

fda5cb2

Stub of tests

317f1bb

Update pki-role-generate test

bf9a082

Revoke certificate action

dc4772c

Add download service to pki service deps

86e4577

Revoke and download work

e87ce65

More tests

51c5c3c

hashishaw added ui pr/no-changelog labels Dec 9, 2022

hashishaw added this to the 1.13.0-rc1 milestone Dec 9, 2022

More tests

1f83f07

hashishaw marked this pull request as ready for review December 9, 2022 22:15

hellobontempo reviewed Dec 9, 2022

View reviewed changes

zofskeez approved these changes Dec 12, 2022

View reviewed changes

Address comments

29f3f38

hashishaw enabled auto-merge (squash) December 13, 2022 18:17

hashishaw merged commit 5879c61 into main Dec 13, 2022

hashishaw deleted the ui/VAULT-9350/pki-role-generate branch December 13, 2022 20:04

AnPucel pushed a commit that referenced this pull request Jan 14, 2023

UI: PKI generate cert from role (#18300)

f749f9b

hashishaw mentioned this pull request Jan 27, 2023

UI: PKI Sign Intermediate #18842

Merged

jayant07-yb pushed a commit to jayant07-yb/hashicorp-vault-integrations that referenced this pull request Mar 15, 2023

UI: PKI generate cert from role (hashicorp#18300)

f8d53a0

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

UI: PKI generate cert from role #18300

UI: PKI generate cert from role #18300

hashishaw commented Dec 9, 2022 •

edited

hellobontempo Dec 9, 2022

zofskeez left a comment

zofskeez Dec 12, 2022

zofskeez Dec 12, 2022

hashishaw Dec 12, 2022

zofskeez Dec 12, 2022

zofskeez Dec 12, 2022

zofskeez Dec 12, 2022

zofskeez Dec 12, 2022

hashishaw Dec 12, 2022

zofskeez Dec 12, 2022

hashishaw Dec 12, 2022

zofskeez Dec 12, 2022

hashishaw Dec 12, 2022

zofskeez Dec 12, 2022

zofskeez commented Dec 12, 2022


		const certDisplayFields = ['certificate', 'commonName', 'serialNumber', 'notValidAfter', 'notValidBefore'];

		@withFormFields(certDisplayFields, null)

		@@ -0,0 +1,36 @@
		import Service from '@ember/service';

		export default class DownloadService extends Service {

UI: PKI generate cert from role #18300

UI: PKI generate cert from role #18300

Conversation

hashishaw commented Dec 9, 2022 • edited

Choose a reason for hiding this comment

zofskeez left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zofskeez commented Dec 12, 2022

hashishaw commented Dec 9, 2022 •

edited