Warn instead of returning error when missing intermediate mount tune permissions #15035
Conversation
kyhavlov
added
backport/1.11
backport/1.12
kyhavlov
requested review from
a team and
kisunji
and removed request for
a team
github-actions
bot
added
the
theme/connect
kyhavlov
force-pushed
the
vault-ttl-update-warn
branch
from
7d03bad
to
da8d6db
Compare
| @@ -388,7 +388,7 @@ func (v *VaultProvider) setupIntermediatePKIPath() error { | |||
| } else { | |||
| err := v.tuneMountNamespaced(v.config.IntermediatePKINamespace, v.config.IntermediatePKIPath, &mountConfig) | |||
| if err != nil { | |||
| return err | |||
| v.logger.Warn("Could not update intermediate PKI mount settings", "path", v.config.IntermediatePKIPath, "error", err) | |||
There was a problem hiding this comment.
@kyhavlov : Does err here contain any messaging explaining that the problem is that the read capability in Vault is missing on the path?
There was a problem hiding this comment.
Yes - if it's due to permission denied, it'll have that in the error message as well as the endpoint it was trying to access (../tune in this case)
There was a problem hiding this comment.
The log message looks like this:
2022-10-18T11:38:23.609-0700 [WARN] Could not update intermediate PKI mount settings: path=pki-intermediate/
error=
| Error making API request.
|
| URL: POST http://127.0.0.1:16001/v1/sys/mounts/pki-intermediate/tune
| Code: 403. Errors:
|
| * 1 error occurred:
| * permission denied
| ```
| func TestVaultProvider_ReconfigureIntermediateTTL(t *testing.T) { | ||
| SkipIfVaultNotPresent(t) | ||
|
|
||
| policy := ` |
There was a problem hiding this comment.
@kyhavlov : Should we modify the other tests in here to use the minimal, recommended permissions (demonstrated in this test) rather than the maximal (not recommended) permissions currently used for most of the tests?
That would help us catch similar issues in the future, if the Vault CA provider code is modified to require access to a new endpoint (that needs new permissions). Without that change to the test permissions, it's easy for an issue like this one to recur.
There was a problem hiding this comment.
@kyhavlov : Should the read permissions on the tune path be included in the standard Vault policies for all tests? (I realize you'd need to temporarily remove that for the test case where you are seeing what happens if that isn't present, but it's now a part of the "standard" permissions we recommended, right?)
There was a problem hiding this comment.
Good call - none of the other tests need it yet, but it could be relevant in future tests.
kyhavlov
force-pushed
the
vault-ttl-update-warn
branch
from
da8d6db
to
e558bcc
Compare
kyhavlov
force-pushed
the
vault-ttl-update-warn
branch
from
e558bcc
to
d122108
Compare
This PR changes the behavior added in #14516 - specifically, the fix required using the tune endpoint to update the
MaxLeaseTTLin the intermediate PKI mount, which was a new permission in Vault we weren't explicitly documenting. This changes the failure case to emitting a warning log message instead of an error, which would've caused attempts to update the CA configuration to fail if this permission was missing.