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

Add a parameter to allow ExtKeyUsage field usage from a role #21702

Merged
merged 6 commits into from Jul 17, 2023
Merged

Add a parameter to allow ExtKeyUsage field usage from a role #21702

merged 6 commits into from Jul 17, 2023

Conversation

Viper61
Copy link
Contributor

@Viper61 Viper61 commented Jul 9, 2023

As discussed in #21549, there is some use cases where allowing expanding ExtKeyUsage is adequate (eg containers).
This PR adds a new configuration option on ACME to allow usage of the ExtKeyUsage field defined within a role.

@Viper61 Viper61 requested a review from a team July 9, 2023 14:23
@hashicorp-cla
Copy link

hashicorp-cla commented Jul 9, 2023

CLA assistant check
All committers have signed the CLA.

@cipherboy cipherboy self-assigned this Jul 10, 2023
@cipherboy cipherboy added enhancement secret/pki backport/1.14.x Backport changes to `release/1.14.x` labels Jul 10, 2023
@cipherboy cipherboy added this to the 1.14.1 milestone Jul 10, 2023
Copy link
Contributor

@stevendpclark stevendpclark left a comment

Choose a reason for hiding this comment

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

Thanks @Viper61 for the submission.

Looks good to me, a test for the new behavior would be nice, but we can add that after the fact, and note to myself, docs need to be updated.

changelog/21702.txt Outdated Show resolved Hide resolved
ui/app/models/pki/config/acme.js Outdated Show resolved Hide resolved
@Viper61
Copy link
Contributor Author

Viper61 commented Jul 11, 2023

Thanks for the review.

Both rewording you suggested were applied.
The new parameter was also included in website/content/api-docs/secret/pki.mdx

I'm trying to write a proper test. Sadly my lack of knowledge in Golang is causing me trouble.
I haven't been able to do something working/I'm happy with. I'll keep you posted when I'm able to do something good enough.

@stevendpclark
Copy link
Contributor

Thanks for the extra work @Viper61, we can handle writing of the test if you wish, let me know either way.

We will also tweak a few nits such as renaming the field to allow_role_ext_key_usage, and internal config variable to AllowRoleExtKeyUsage to be better aligned with existing fields/codebase. Sorry I missed these in the original review.

@Viper61
Copy link
Contributor Author

Viper61 commented Jul 13, 2023

No problem @stevendpclark. I updated the variables accordingly :)

Regarding the test, either it's always showing ok, either I'm not having it running at all.
I guess I can let you handle it if it's fine for you? I'll make sure to follow that closely. I will learn something there.

Edit : Rebased the PR on current main

@stevendpclark
Copy link
Contributor

No problem @stevendpclark. I updated the variables accordingly :)

Much appreciated!

Regarding the test, either it's always showing ok, either I'm not having it running at all. I guess I can let you handle it if it's fine for you? I'll make sure to follow that closely. I will learn something there.

No worries, I just pushed an updated unit test and I tweaked some of the UI label text a bit in a commit pushed to your branch.

@stevendpclark stevendpclark merged commit 366693c into hashicorp:main Jul 17, 2023
86 of 89 checks passed
@stevendpclark
Copy link
Contributor

Thanks again @Viper61 for the submission, this should make it into the next minor Vault release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport/1.14.x Backport changes to `release/1.14.x` enhancement secret/pki
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants