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
Warn instead of returning error when missing intermediate mount tune permissions #15035
Conversation
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good call - none of the other tests need it yet, but it could be relevant in future tests.
da8d6db
to
e558bcc
Compare
e558bcc
to
d122108
Compare
This PR changes the behavior added in #14516 - specifically, the fix required using the tune endpoint to update the
MaxLeaseTTL
in 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.