hashicorp / vault

A tool for secrets management, encryption as a service, and privileged access management
https://www.vaultproject.io/
Other
30.84k stars 4.17k forks source link

vault auth tune: unable to reset listing-visibility to default (empty string) #15209

Closed candlerb closed 2 years ago

candlerb commented 2 years ago

Describe the bug Having used vault auth tune -listing-visibility=unauth to make an auth method visible in the web UI, I cannot reset this to empty string. The command is accepted but does not make a change.

To Reproduce

$ vault read sys/auth/oidc
Key                        Value
---                        -----
accessor                   auth_oidc_43c2410e
config                     map[default_lease_ttl:0 force_no_cache:false listing_visibility:unauth max_lease_ttl:0 token_type:default-service]
description                Keycloak
external_entropy_access    false
local                      false
options                    <nil>
seal_wrap                  false
type                       oidc
uuid                       fe03f5d3-cecf-70e0-2aa4-f5a887ceb29b
$ vault auth tune -listing-visibility="" oidc/
Success! Tuned the auth method at: oidc/
$ vault read sys/auth/oidc
Key                        Value
---                        -----
accessor                   auth_oidc_43c2410e
config                     map[default_lease_ttl:0 force_no_cache:false listing_visibility:unauth max_lease_ttl:0 token_type:default-service]
description                Keycloak
external_entropy_access    false
local                      false
options                    <nil>
seal_wrap                  false
type                       oidc
uuid                       fe03f5d3-cecf-70e0-2aa4-f5a887ceb29b

Expected behavior It should be possible to change listing-visibility back to its default (empty string) state.

Environment:

Vault server configuration file(s): N/A

Additional context N/A

ccapurso commented 2 years ago

@candlerb, this looks to be an inconsistency between the CLI and hitting the API directly. I have reproduced the issue locally using your CLI example. Performing the same steps with the curl, however, I was able to reset the field as expected.

❯ curl -X POST -H "X-Vault-Token: $VAULT_TOKEN" -d '{"listing_visibility": "unauth"}' "$VAULT_ADDR/v1/sys/mounts/auth/oidc/tune"

❯ vault read sys/auth/oidc
Key                        Value
---                        -----
accessor                   auth_oidc_cf06863b
config                     map[default_lease_ttl:0 force_no_cache:false listing_visibility:unauth max_lease_ttl:0 token_type:default-service]
description                n/a
external_entropy_access    false
local                      false
options                    <nil>
seal_wrap                  false
type                       oidc
uuid

❯ curl -X POST -H "X-Vault-Token: $VAULT_TOKEN" -d '{"listing_visibility": ""}' "$VAULT_ADDR/v1/sys/mounts/auth/oidc/tune"

❯ vault read sys/auth/oidc
Key                        Value
---                        -----
accessor                   auth_oidc_cf06863b
config                     map[default_lease_ttl:0 force_no_cache:false max_lease_ttl:0 token_type:default-service]
description                n/a
external_entropy_access    false
local                      false
options                    <nil>
seal_wrap                  false
type                       oidc
uuid                       173e582d-6c00-bdca-d7c4-50b5eda200d9

I believe the fix is likely pretty straightforward and is caused by the omitempty JSON tag for the MountConfigInput struct which the CLI uses for marshaling the input/output. I will verify this with some tests to ensure that removing that tag is safe.

candlerb commented 2 years ago

Many thanks. I can confirm the following workaround is successful:

curl -X POST -H "X-Vault-Token: $(cat ~/.vault-token)" \
  -d '{"listing_visibility": ""}' "$VAULT_ADDR/v1/sys/mounts/auth/oidc/tune"

In the case of the CLI, I wonder if that string needs to become a string pointer? It needs to distinguish between "no change to listing_visibility" and "set listing_visibility to empty string".

ccapurso commented 2 years ago

Ah, yes there is not currently a CLI flag type for a string pointer that allows for specifying between a deliberate empty string and the absence of the flag. It would be fairly straightforward to add but I think we would then be faced with a similar issue due to the fact that the data is provided as a struct that is then marshaled into JSON. The omitempty JSON tag would allow for the ability to ignore if not specified but not allow to override as an empty string.

The inconsistency between the API and the CLI is unfortunate and might exist elsewhere for string fields where omitempty JSON tags are used. For now, I would recommend using the workaround discussed. I will bring this up as a discussion topic with our team to determine what paths exist to mitigate such inconsistencies. Thank you for bringing it to our attention!

cipherboy commented 2 years ago

Hi @candlerb I believe listing_visibility=hidden solves this. It was added in a past release. I think there might be some documentation missing though; feel free to add a PR for that if its not clear where that should be.

[cipherboy@xps15 vault]$ vault read sys/auth/cert-b
Key                        Value
---                        -----
accessor                   auth_cert_ee4ca3c3
config                     map[default_lease_ttl:0 force_no_cache:false listing_visibility:unauth max_lease_ttl:0 token_type:default-service]
description                n/a
external_entropy_access    false
local                      false
options                    <nil>
seal_wrap                  false
type                       cert
uuid                       0cf513c2-06b8-096b-d723-26748191f038
[cipherboy@xps15 vault]$ vault auth tune -listing-visibility="hidden" cert-b
Success! Tuned the auth method at: cert-b/
[cipherboy@xps15 vault]$ vault read sys/auth/cert-b
Key                        Value
---                        -----
accessor                   auth_cert_ee4ca3c3
config                     map[default_lease_ttl:0 force_no_cache:false listing_visibility:hidden max_lease_ttl:0 token_type:default-service]
description                n/a
external_entropy_access    false
local                      false
options                    <nil>
seal_wrap                  false
type                       cert
uuid                       0cf513c2-06b8-096b-d723-26748191f038
candlerb commented 2 years ago

I can confirm that "hidden" works as a value, thank you. Ref: https://github.com/hashicorp/vault/pull/15833#issuecomment-1147956548