Vault Init input validation is too strict (1.12.0)

[acidprime]

Describe the bug
Vault 1.12.0 added support to validate operator init input. However Its breaking a community terraform module and supported server side behavior.

Refs
Vault PR adding validation
Terraform Community Module

This is due to the fact the author relied on passing in 0 to the recovery_* params and then them being ignored by vault when secrets were specified as non zero. This is confusing in the code as the error does not match the expected state as defined.

This is NOT inline with the vault server code

vault/http/sys_init.go

Lines 154 to 158 in dd891bc

	if req.RecoveryShares != 0 {
	recoveryFlags = append(recoveryFlags, "recovery_shares")
	}
	if req.RecoveryThreshold != 0 {
	recoveryFlags = append(recoveryFlags, "recovery_threshold")

Which implicitly allows you to pass 0 currently.

To Reproduce
Steps to reproduce the behavior:

1. Using module write the following code

 resource "vaultoperator_init" "scenario_vault_init" {
   secret_shares    = 1
   secret_threshold = 1
 
   depends_on = [
     docker_container.scenario_vault_container,
     time_sleep.wait_for_vault_startup,
   ]
 
   lifecycle {
     ignore_changes = all
   }
 }

3. Run terraform apply
4. See error

Error: Error making API request.

URL: PUT http://127.0.0.1:8200/v1/sys/init
Code: 400. Errors:

* parameters recovery_shares,recovery_threshold not applicable to seal type shamir

  on ../modules/vault-transit/main.tf line 7, in resource "vaultoperator_init" "scenario_vault_init":
   7: resource "vaultoperator_init" "scenario_vault_init" {

Expected behavior
The provider specifiying 0 for recovery keys and >0 for secret keys should pass validation.

Based on semver this is a breaking change to the API, while I understand the need for validation I would say passing 0 to both params should negate that check given the server side code supporting that configuration. Further I think this scenario e.g. 0 for the params was missed in the go tests, and if its was an expected behavior then it should be in the go tests for linked PR. Instead the PR/tests only assume a scenario where you incorrectly pass both params in with values instead of one with 0 and one with >0 value . Most importantly its not inline with the actual server code referenced above which should be the source of truth here for validation.

Environment:

• Vault Server Version 1.12.0:
• Vault CLI Version1.12.0:
• Server Operating System/Architecture:

Docker , but its not probably not relevant to this bug.

Vault server configuration file(s):

 disable_mlock = true
 ui = true
 
 listener "tcp" {
     tls_disable = "true"
     address = "[::]:8200"
     cluster_address = "[::]:8201"
 }
 
 storage "file" {
   path = "/tmp"
 }

Additional context
I have also filed a bug on the community provider here rickardgranberg/terraform-provider-vaultoperator#7

However strictly speaking again, this is a breaking change in the API on invalid semver bump so I think vault should likely fix it. The validation while good, should not fail to validate something that would not produce an invalid configuration e.g. 0 for recovery keys.

Even if you expect the provider author to fix it, it is actually not that easy to fix for the though too:

You can see the terraform vault provider code in question here

This is essentially by design in the terraform provider code as calling Get will implicitly return a 0 and not say nil, that would work here with omitempty if the upstream vault api module supported it. However even that won't work as vault doesn't use omitempty for the init request type.

From the terraform provider docs for Get

If the key does exist in the schema but doesn't exist in the configuration, then the default value for that type will be returned. For strings, this is "", for numbers it is 0, etc.

vault/api/sys_init.go

Lines 58 to 59 in 746b089

	RecoveryShares int `json:"recovery_shares"`
	RecoveryThreshold int `json:"recovery_threshold"`

[ccapurso]

Hi, @acidprime. The Vault team is very cautious about introducing breaking changes only to major versions. Admittedly, it has happened in the past. Vault does not strictly follow semver and 1.12.0 is actually considered a major version.

I dug into terraform-provider-vaultoperator and it appears that the recovery shares and threshold are set to the secret shares and threshold if they are 0. It seems that removing those lines could resolve this issue.

[acidprime]

Hi, @acidprime. The Vault team is very cautious about introducing breaking changes only to major versions. Admittedly, it has happened in the past. Vault does not strictly follow semver and 1.12.0 is actually considered a major version.

I dug into terraform-provider-vaultoperator and it appears that the recovery shares and threshold are set to the secret shares and threshold if they are 0. It seems that removing those lines could resolve this issue.

Yeah that makes sense to me too

https://github.com/rickardgranberg/terraform-provider-vaultoperator/blob/main/internal/provider/resource_init.go#L236-L242

If those values are non-zero it makes sense validation would fail

[mladlow]

It seems like this might have been resolved, so I'm going to close it. If that's wrong, feel free to re-open!