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

Tokenutil: Perform num uses check earlier #11647

Merged
merged 3 commits into from
May 19, 2021
Merged

Tokenutil: Perform num uses check earlier #11647

merged 3 commits into from
May 19, 2021

Conversation

vishalnayak
Copy link
Member

Fixes: #11570

@vishalnayak vishalnayak added this to the 1.7.2 milestone May 18, 2021
@vishalnayak vishalnayak requested a review from a team May 18, 2021 19:18
@vercel vercel bot temporarily deployed to Preview – vault-storybook May 18, 2021 19:22 Inactive
@vercel vercel bot temporarily deployed to Preview – vault May 18, 2021 19:22 Inactive

data["token_num_uses"] = 0
data["token_type"] = "batch"
requestFunc(logical.UpdateOperation, data)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would it make sense to try using the token after making the change?

Copy link
Member Author

Choose a reason for hiding this comment

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

I am not sure I understand. This is when updating a role that is used to issue a token. Do you mean that that test should issue a token and try to use it?

Copy link
Member Author

Choose a reason for hiding this comment

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

The tests stops here because without the fix, the update step used to fail and now it doesn't.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe? I just figure if this wasn't possible before, the resulting role might not yield usable tokens due to other latent bugs, so it would be nice to validate that they work.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure. I'll add those steps.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is a backend level unit test. So I couldn't ensure that the token is usable. However the test now does ensure that a valid Auth block is returned from the engine. The token creation and usability should not be affected in my opinion. Pushed up the changes.

@vercel vercel bot temporarily deployed to Preview – vault May 19, 2021 14:17 Inactive
@vercel vercel bot temporarily deployed to Preview – vault-storybook May 19, 2021 14:17 Inactive
@vishalnayak vishalnayak merged commit d2c4fd4 into master May 19, 2021
@vishalnayak vishalnayak deleted the tokenutil-fix branch May 19, 2021 18:06
@mladlow mladlow modified the milestones: 1.7.2, 1.8, 1.7.3 May 20, 2021
vishalnayak added a commit that referenced this pull request Jun 3, 2021
* Perform num uses check earlier

* Add CL

* Ensure that login works
vishalnayak added a commit that referenced this pull request Jun 3, 2021
* Perform num uses check earlier

* Add CL

* Ensure that login works
AndreyZamyslov pushed a commit to yandex-cloud/vault that referenced this pull request Jun 10, 2021
* Perform num uses check earlier

* Add CL

* Ensure that login works
jartek pushed a commit to jartek/vault that referenced this pull request Sep 11, 2021
* Perform num uses check earlier

* Add CL

* Ensure that login works
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.

vault does not allow changing approle token_num_use=1 to batch token in a single request
3 participants