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

Validate parameters for vault operator init #16379

Merged
merged 10 commits into from Jul 25, 2022
Merged

Conversation

akshya96
Copy link
Contributor

@akshya96 akshya96 commented Jul 20, 2022

https://hashicorp.atlassian.net/browse/VAULT-951

The "vault operator" command should not operate if there are unused command-line flags, for example:
vault operator init --recovery-threshold=1 --recovery-shares=1
should return an error because auto unseal is not being used (the correct flags are -key-shares and -key threshold.)

@akshya96 akshya96 requested review from ncabatoff and a team July 20, 2022 17:53
@ncabatoff
Copy link
Contributor

I agree that you have to remove the defaults from the flag definitions, but I wouldn't push everything into the api as you're doing. People often use different versions of the CLI and server, and this could break backwards compat.

We might have to make the operator init command query the sys/seal-status endpoint to determine whether shamir is in use or not, so it can make the right decision about which defaults to use when the user doesn't specify any share/threshold flags.

@akshya96
Copy link
Contributor Author

I agree that you have to remove the defaults from the flag definitions, but I wouldn't push everything into the api as you're doing. People often use different versions of the CLI and server, and this could break backwards compat.

We might have to make the operator init command query the sys/seal-status endpoint to determine whether shamir is in use or not, so it can make the right decision about which defaults to use when the user doesn't specify any share/threshold flags.

I agree, I did not think of different versions of cli and server. I have changed the cli to set default values based on use of auto unseal instead of setting default values for all fields.

@akshya96 akshya96 marked this pull request as ready for review July 21, 2022 00:10
http/sys_init.go Outdated Show resolved Hide resolved
http/sys_init.go Outdated Show resolved Hide resolved
@akshya96 akshya96 requested a review from ncabatoff July 21, 2022 16:34
Copy link
Contributor

@ccapurso ccapurso left a comment

Choose a reason for hiding this comment

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

Looks good!

@akshya96
Copy link
Contributor Author

Added a new unit test for auto unseal

Copy link
Contributor

@ccapurso ccapurso left a comment

Choose a reason for hiding this comment

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

Tests look good!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants