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

UI: add allow_empty_principals to ssh engine (fixes failing test) #28484

Merged
merged 5 commits into from
Sep 24, 2024
Merged

UI: add allow_empty_principals to ssh engine (fixes failing test) #28484

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
c30c487
remove superfluous divs
hellobontempo Sep 24, 2024
d890aa1
add attrs to ssh sign form
hellobontempo Sep 24, 2024
c8c71a6
oops, allowEmptyPrincipals should be on the role, not the signing form
hellobontempo Sep 24, 2024
42ea082
update test
hellobontempo Sep 24, 2024
a50a2e7
add ssh allowEmptyPrincipals to openapi
hellobontempo Sep 24, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
137 changes: 63 additions & 74 deletions ui/app/components/secret-engine/configure-ssh.hbs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,84 +3,73 @@
SPDX-License-Identifier: BUSL-1.1
~}}

<form {{on "submit" (perform this.save)}} aria-label="save ssh creds" data-test-configure-form>
<div class="box is-fullwidth is-shadowless is-marginless">
Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed lots of extra divs here, also the the box shadow above the input is anti-pattern so I removed it

before/after

Screenshot 2024-09-23 at 5 02 02 PM

<NamespaceReminder @mode="save" @noun="configuration" />
<MessageError @errorMessage={{this.errorMessage}} />
{{#unless @model.isNew}}
<p class="has-text-grey-dark">
NOTE: You must delete your existing certificate and key before saving new values.
</p>
Comment on lines -11 to -13
Copy link
Contributor Author

@hellobontempo hellobontempo Sep 24, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

moved this down, inside the {{else}} block

{{/unless}}
</div>
<form {{on "submit" (perform this.save)}} aria-label="save ssh creds" class="has-top-padding-m" data-test-configure-form>
<NamespaceReminder @mode="save" @noun="configuration" />
<MessageError @errorMessage={{this.errorMessage}} />
{{#if @model.isNew}}
<div class="box is-fullwidth is-sideless">
{{#each @model.formFields as |attr|}}
<FormField @attr={{attr}} @model={{@model}} @modelValidations={{this.modelValidations}} />
{{/each}}
</div>
<div class="box is-fullwidth is-bottomless">
<div class="control">
<Hds::Button
@text="Save"
@icon={{if this.save.isRunning "loading"}}
type="submit"
disabled={{this.save.isRunning}}
data-test-configure-save-button
/>
<Hds::Button
@text="Cancel"
@color="secondary"
class="has-left-margin-s"
disabled={{this.save.isRunning}}
{{on "click" this.onCancel}}
data-test-cancel-button
/>
</div>
{{#if this.invalidFormAlert}}
<AlertInline
data-test-invalid-form-alert
class="has-top-padding-s"
@type="danger"
@message={{this.invalidFormAlert}}
/>
{{/if}}
</div>
{{#each @model.formFields as |attr|}}
<FormField @attr={{attr}} @model={{@model}} @modelValidations={{this.modelValidations}} />
{{/each}}
<hr class="has-background-gray-300" />
<Hds::ButtonSet>
<Hds::Button
@text="Save"
@icon={{if this.save.isRunning "loading"}}
type="submit"
disabled={{this.save.isRunning}}
data-test-configure-save-button
/>
<Hds::Button
@text="Cancel"
@color="secondary"
disabled={{this.save.isRunning}}
{{on "click" this.onCancel}}
data-test-cancel-button
/>
</Hds::ButtonSet>
{{#if this.invalidFormAlert}}
<AlertInline
data-test-invalid-form-alert
class="has-top-padding-s"
@type="danger"
@message={{this.invalidFormAlert}}
/>
{{/if}}
{{else}}
{{! Model is not new and keys have already been created. Require user deletes the keys before creating new ones }}
<div class="box is-fullwidth is-sideless is-marginless" data-test-edit-config-section>
Copy link
Contributor Author

@hellobontempo hellobontempo Sep 24, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

before/after

Screenshot 2024-09-23 at 5 02 31 PM

<div class="field">
<label for="publicKey" class="is-label">
Public key
</label>
<div class="control">
<MaskedInput
@name="publickey"
@id="publicKey"
@value={{@model.publicKey}}
@displayOnly={{true}}
@allowCopy={{true}}
data-test-input="public-key"
/>
</div>
</div>
</div>
<div class="field is-grouped-split box is-fullwidth is-bottomless">
<Hds::ButtonSet>
<Hds::Copy::Button
@text="Copy"
@textToCopy={{@model.publicKey}}
@onError={{fn (set-flash-message "Clipboard copy failed. The Clipboard API requires a secure context." "danger")}}
class="primary"
/>
<ConfirmAction
@buttonText="Delete"
@buttonColor="secondary"
@confirmMessage="Confirming will remove the CA certificate information."
@onConfirmAction={{this.deleteCaConfig}}
data-test-delete-public-key
<p class="has-text-grey-dark has-top-bottom-margin">
NOTE: You must delete your existing certificate and key before saving new values.
</p>

<div class="box is-fullwidth is-sideless" data-test-edit-config-section>
<label for="publicKey" class="is-label">
Public key
</label>
<div class="control">
<MaskedInput
@name="publickey"
@id="publicKey"
@value={{@model.publicKey}}
@displayOnly={{true}}
@allowCopy={{true}}
data-test-input="public-key"
/>
</Hds::ButtonSet>
</div>
</div>
<Hds::ButtonSet>
<Hds::Copy::Button
@text="Copy"
@textToCopy={{@model.publicKey}}
@onError={{fn (set-flash-message "Clipboard copy failed. The Clipboard API requires a secure context." "danger")}}
class="primary"
/>
<ConfirmAction
@buttonText="Delete"
@buttonColor="secondary"
@confirmMessage="Confirming will remove the CA certificate information."
@onConfirmAction={{this.deleteCaConfig}}
data-test-delete-public-key
/>
</Hds::ButtonSet>
{{/if}}
</form>
5 changes: 5 additions & 0 deletions ui/app/models/role-ssh.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ const CA_FIELDS = [
'defaultExtensions',
'allowBareDomains',
'allowSubdomains',
'allowEmptyPrincipals',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

added to CA roles and in the more options toggle
Screenshot 2024-09-23 at 6 01 59 PM

'allowUserKeyIds',
'keyIdFormat',
'notBeforeDuration',
Expand Down Expand Up @@ -118,6 +119,10 @@ export default Model.extend({
helpText:
'Specifies if host certificates that are requested are allowed to be subdomains of those listed in Allowed Domains',
}),
allowEmptyPrincipals: attr('boolean', {
helpText:
'Allow signing certificates with no valid principals (e.g. any valid principal). For backwards compatibility only. The default of false is highly recommended.',
}),
allowUserKeyIds: attr('boolean', {
helpText: 'Specifies if users can override the key ID for a signed certificate with the "key_id" field',
}),
Expand Down
5 changes: 4 additions & 1 deletion ui/app/models/ssh-sign.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,10 @@ export default Model.extend({
label: 'TTL',
editType: 'ttl',
}),
validPrincipals: attr('string'),
validPrincipals: attr('string', {
helpText:
'Specifies valid principals, either usernames or hostnames, that the certificate should be signed for. Required unless the role has specified allow_empty_principals.',
}),
certType: attr('string', {
defaultValue: 'user',
label: 'Certificate Type',
Expand Down
63 changes: 28 additions & 35 deletions ui/app/templates/vault/cluster/secrets/backend/sign.hbs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,48 +76,41 @@
<MessageError @model={{this.model}} />
<NamespaceReminder @mode="sign" @noun="SSH key" />
{{#if this.model.attrs}}
{{#each (take 1 this.model.attrs) as |attr|}}
Copy link
Contributor Author

@hellobontempo hellobontempo Sep 24, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

cleaned up divs, added subtext and moved valid_principals outside of the more options toggle because typically it will be required

Screenshot 2024-09-23 at 6 00 10 PM

<FormFieldFromModel
@attr={{attr}}
@model={{this.model}}
@updateTtl={{action "updateTtl" attr.name}}
@emptyData={{this.emptyData}}
@codemirrorUpdated={{action "codemirrorUpdated" attr.name}}
/>
{{/each}}
{{#let (find-by "name" "publicKey" this.model.attrs) as |attr|}}
<FormFieldFromModel @attr={{attr}} @model={{this.model}} />
{{/let}}
{{! valid_principals is required unless allow_empty_principals is true (not recommended) }}
{{#let (find-by "name" "validPrincipals" this.model.attrs) as |attr|}}
<FormFieldFromModel @attr={{attr}} @model={{this.model}} />
{{/let}}
<ToggleButton @isOpen={{this.showOptions}} @onClick={{fn (mut this.showOptions)}} data-test-toggle-button />
{{#if this.showOptions}}
<div class="box is-marginless">
{{#each (drop 1 this.model.attrs) as |attr|}}
<FormFieldFromModel
@attr={{attr}}
@model={{this.model}}
@updateTtl={{action "updateTtl" attr.name}}
@emptyData={{this.emptyData}}
@codemirrorUpdated={{action "codemirrorUpdated" attr.name}}
/>
{{#each this.model.attrs as |attr|}}
{{! These attrs render above, outside of the "More options" toggle }}
{{#if (not (includes attr.name (array "publicKey" "validPrincipals")))}}
<FormFieldFromModel
@attr={{attr}}
@model={{this.model}}
@updateTtl={{action "updateTtl" attr.name}}
@emptyData={{this.emptyData}}
@codemirrorUpdated={{action "codemirrorUpdated" attr.name}}
/>
{{/if}}
{{/each}}
</div>
{{/if}}
{{/if}}
</div>
<div class="field is-grouped box is-fullwidth is-bottomless">
<Hds::ButtonSet>
<Hds::Button
@text="Sign"
@icon={{if this.loading "loading"}}
type="submit"
disabled={{this.loading}}
data-test-save
/>
<Hds::Button
@text="Cancel"
@color="secondary"
@route="vault.cluster.secrets.backend.list-root"
@model={{this.backend.id}}
data-test-cancel
/>
</Hds::ButtonSet>
</div>
<Hds::ButtonSet class="has-top-bottom-margin">
<Hds::Button @text="Sign" @icon={{if this.loading "loading"}} type="submit" disabled={{this.loading}} data-test-save />
<Hds::Button
@text="Cancel"
@color="secondary"
@route="vault.cluster.secrets.backend.list-root"
@model={{this.backend.id}}
data-test-cancel
/>
</Hds::ButtonSet>
</form>
{{/if}}
3 changes: 3 additions & 0 deletions ui/tests/acceptance/secrets/backend/ssh/roles-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,9 @@ module('Acceptance | ssh | roles', function (hooks) {
credsRoute: 'vault.cluster.secrets.backend.sign',
async fillInCreate() {
await click(GENERAL.inputByAttr('allowUserCertificates'));
await click(GENERAL.toggleGroup('Options'));
// it's recommended to keep allow_empty_principals false, check for testing so we don't have to input an extra field when signing a key
await click(GENERAL.inputByAttr('allowEmptyPrincipals'));
},
async fillInGenerate() {
await fillIn(GENERAL.inputByAttr('publicKey'), PUB_KEY);
Expand Down
7 changes: 7 additions & 0 deletions ui/tests/helpers/openapi/expected-secret-attrs.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,13 @@ const ssh = {
fieldGroup: 'default',
type: 'boolean',
},
allowEmptyPrincipals: {
editType: 'boolean',
fieldGroup: 'default',
helpText:
'Whether to allow issuing certificates with no valid principals (meaning any valid principal). Exists for backwards compatibility only, the default of false is highly recommended.',
type: 'boolean',
},
allowHostCertificates: {
editType: 'boolean',
helpText:
Expand Down
Loading