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 fields 'ttl' and 'num_uses' to SecretID generation. #14474

Merged

Conversation

RemcoBuddelmeijer
Copy link
Contributor

@RemcoBuddelmeijer RemcoBuddelmeijer commented Mar 12, 2022

Add the fields 'ttl' and 'num_uses' to the endpoints /auth/approle/role/<role_name>/secret-id and /auth/approle/role/<role_name>/custom-secret-id.
Edited the help synopsis and description to refer to these fields. Both for the endpoints respectively but also for the secret-id-num-uses and secret-id-ttl endpoints.

Fixes #14390

@RemcoBuddelmeijer RemcoBuddelmeijer marked this pull request as ready for review March 12, 2022 21:25
@swayne275
Copy link
Contributor

thanks! it appears we have some formatting issues

@RemcoBuddelmeijer
Copy link
Contributor Author

thanks! it appears we have some formatting issues

@swayne275 This is correct! I looked at them before and they seem to not originate from my code.
I can go ahead and rebase on the main branch if necessary?

@swayne275
Copy link
Contributor

sure, give it a shot and we can take a deeper look

@RemcoBuddelmeijer RemcoBuddelmeijer force-pushed the feature/numuses-and-ttl-appsecret branch from 7854530 to 215f35d Compare March 14, 2022 17:51
Add fields 'ttl' and 'num_uses' when generating/obtaining a SecretID.
Rather than just being able to use the Role's SecretID ttl and num uses. hashicorp#14390
Add the response field secret_id_num_uses to the endpoints for generating
SecretIDs. Used in testing but also to supply the vendor with this variable.
Add tests to assert the new TTL and NumUses option in the SecretID entry.
Separate test for testing with just parameters vs a -force example.
@RemcoBuddelmeijer RemcoBuddelmeijer force-pushed the feature/numuses-and-ttl-appsecret branch from 215f35d to 0f1c941 Compare March 14, 2022 18:01
@RemcoBuddelmeijer
Copy link
Contributor Author

RemcoBuddelmeijer commented Mar 14, 2022

The test seem to be passing now!

@kent007
Copy link

kent007 commented Mar 24, 2022

Hi everyone,

Just want to keep the conversation moving on this feature. I see Vault 1.10 just rolled out, any idea when these changes might be generally released?

builtin/credential/approle/path_role.go Show resolved Hide resolved
builtin/credential/approle/path_role.go Outdated Show resolved Hide resolved
builtin/credential/approle/path_role.go Outdated Show resolved Hide resolved
builtin/credential/approle/path_role_test.go Outdated Show resolved Hide resolved
builtin/credential/approle/path_role_test.go Outdated Show resolved Hide resolved
builtin/credential/approle/path_role_test.go Outdated Show resolved Hide resolved
changelog/14474.txt Outdated Show resolved Hide resolved
changelog/14474.txt Outdated Show resolved Hide resolved
website/content/api-docs/auth/approle.mdx Outdated Show resolved Hide resolved
website/content/api-docs/auth/approle.mdx Outdated Show resolved Hide resolved
Change the error message produced when a test fails due to a missing field.
Previous values did not map to correct fields.
Unnecessary cast to int where type already is int.
Remove metadata entry in sample payload for custom-secret-id. The metadata was not
changed in the features pull request.
Rather than implicitly overriding, error when the ttl is lower than and the num
uses higher than the role's configuration. hashicorp#14390
More elaborate description for the changelog. Specifying the per-request based fields.
@RemcoBuddelmeijer RemcoBuddelmeijer force-pushed the feature/numuses-and-ttl-appsecret branch from 900e726 to 772adde Compare May 31, 2022 21:25
Specify in both the api-docs and the CLI the limits of the fields.
Specify that the role's configuration is still the leading factor.
Upper bound ttl with role secret id ttl when creating a secret id
Adding test cases for infinite ttl and num uses
Adding test cases for negative ttl and num uses
Validation on infinite ttl and num uses
@hashicorp-cla
Copy link

hashicorp-cla commented Jun 3, 2022

CLA assistant check
All committers have signed the CLA.

@RemcoBuddelmeijer RemcoBuddelmeijer force-pushed the feature/numuses-and-ttl-appsecret branch from 1875e77 to cba46ac Compare June 3, 2022 09:08
Changed that TTL is not allowed to be shorter to longer
Define ttl and num_uses in every test despite them not being tested.
This is to ensure that no unexpected behaviour comes to mind.
@RemcoBuddelmeijer
Copy link
Contributor Author

Hello all! I was hoping to bump up this PR to get this looked at and possibly merged. Thank you! 😄

@kent007
Copy link

kent007 commented Jun 14, 2022

Hi everyone, also bumping this PR. Would love to see this feature merged.

builtin/credential/approle/path_role.go Show resolved Hide resolved
builtin/credential/approle/path_role.go Show resolved Hide resolved
builtin/credential/approle/path_role.go Show resolved Hide resolved
builtin/credential/approle/path_role.go Outdated Show resolved Hide resolved
builtin/credential/approle/path_role.go Outdated Show resolved Hide resolved
builtin/credential/approle/path_role_test.go Show resolved Hide resolved
builtin/credential/approle/path_role_test.go Outdated Show resolved Hide resolved
@kent007
Copy link

kent007 commented Jul 12, 2022

Hi everyone, are there still more discussions to be resolved on this feature?

@RemcoBuddelmeijer
Copy link
Contributor Author

Hello everyone, periodic bump up of the PR to see if it can get merged. Thank you!

@raskchanky raskchanky removed their request for review August 16, 2022 18:26
@kent007
Copy link

kent007 commented Aug 24, 2022

Periodic PR bump, this is still a feature I'm very interested in!

Copy link
Contributor

@HridoyRoy HridoyRoy left a comment

Choose a reason for hiding this comment

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

Lgtm

@HridoyRoy HridoyRoy merged commit 3e6f7a3 into hashicorp:main Sep 2, 2022
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.

Customizing num_uses and ttl per approle secret id
7 participants