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.

Sign up for GitHub

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

Jump to bottom

Warn instead of returning error when missing intermediate mount tune permissions #15035

Merged
merged 1 commit into from Oct 18, 2022
Merged

Warn instead of returning error when missing intermediate mount tune permissions #15035

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 3 additions & 0 deletions .changelog/15035.txt
View file
Open in desktop
@@ -0,0 +1,3 @@
```release-note:improvement
connect/ca: Log a warning message instead of erroring when attempting to update the intermediate pki mount when using the Vault provider.
```
2 changes: 1 addition & 1 deletion agent/connect/ca/provider_vault.go
View file
Open in desktop
Expand Up @@ -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)
Copy link
Contributor

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?

Copy link
Contributor Author

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)

Copy link
Contributor Author

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
  | ```

}
}

Expand Down
114 changes: 111 additions & 3 deletions agent/connect/ca/provider_vault_test.go
View file
Open in desktop
Expand Up @@ -20,13 +20,29 @@ import (
)

const pkiTestPolicy = `
path "sys/mounts/*"
path "sys/mounts"
{
capabilities = ["create", "read", "update", "delete", "list", "sudo"]
capabilities = ["read"]
}
path "sys/mounts/pki-root"
{
capabilities = ["create", "read", "update", "delete", "list"]
}
path "sys/mounts/pki-intermediate"
{
capabilities = ["create", "read", "update", "delete", "list"]
}
path "sys/mounts/pki-intermediate/tune"
{
capabilities = ["update"]
}
path "pki-root/*"
{
capabilities = ["create", "read", "update", "delete", "list"]
}
path "pki-intermediate/*"
{
capabilities = ["create", "read", "update", "delete", "list", "sudo"]
capabilities = ["create", "read", "update", "delete", "list"]
}`

func TestVaultCAProvider_ParseVaultCAConfig(t *testing.T) {
Expand Down Expand Up @@ -794,6 +810,98 @@ func TestVaultProvider_RotateAuthMethodToken(t *testing.T) {
}, 10*time.Second, 100*time.Millisecond)
}

func TestVaultProvider_ReconfigureIntermediateTTL(t *testing.T) {
SkipIfVaultNotPresent(t)

// Set up a standard policy without any sys/mounts/pki-intermediate/tune permissions.
policy := `
Copy link
Contributor

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.

Copy link
Contributor

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?)

Copy link
Contributor Author

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.

path "sys/mounts"
{
capabilities = ["read"]
}
path "sys/mounts/pki-root"
{
capabilities = ["create", "read", "update", "delete", "list"]
}
path "sys/mounts/pki-intermediate"
{
capabilities = ["create", "read", "update", "delete", "list"]
}
path "pki-root/*"
{
capabilities = ["create", "read", "update", "delete", "list"]
}
path "pki-intermediate/*"
{
capabilities = ["create", "read", "update", "delete", "list"]
}`
testVault := NewTestVaultServer(t)

err := testVault.Client().Sys().PutPolicy("pki", policy)
require.NoError(t, err)

tcr := &vaultapi.TokenCreateRequest{
Policies: []string{"pki"},
}
secret, err := testVault.client.Auth().Token().Create(tcr)
require.NoError(t, err)
providerToken := secret.Auth.ClientToken

makeProviderConfWithTTL := func(ttl string) ProviderConfig {
conf := map[string]interface{}{
"Address": testVault.Addr,
"RootPKIPath": "pki-root/",
"IntermediatePKIPath": "pki-intermediate/",
"Token": providerToken,
"IntermediateCertTTL": ttl,
}
cfg := ProviderConfig{
ClusterID: connect.TestClusterID,
Datacenter: "dc1",
IsPrimary: true,
RawConfig: conf,
}
return cfg
}

provider := NewVaultProvider(hclog.New(nil))

// Set up the initial provider config
t.Cleanup(provider.Stop)
err = provider.Configure(makeProviderConfWithTTL("222h"))
require.NoError(t, err)
_, err = provider.GenerateRoot()
require.NoError(t, err)
_, err = provider.GenerateIntermediate()
require.NoError(t, err)

// Attempt to update the ttl without permissions for the tune endpoint - shouldn't
// return an error.
err = provider.Configure(makeProviderConfWithTTL("333h"))
require.NoError(t, err)

// Intermediate TTL shouldn't have changed
mountConfig, err := testVault.Client().Sys().MountConfig("pki-intermediate")
require.NoError(t, err)
require.Equal(t, 222*3600, mountConfig.MaxLeaseTTL)

// Update the policy and verify we can reconfigure the TTL properly.
policy += `
path "sys/mounts/pki-intermediate/tune"
{
capabilities = ["update"]
}`
err = testVault.Client().Sys().PutPolicy("pki", policy)
require.NoError(t, err)

err = provider.Configure(makeProviderConfWithTTL("333h"))
require.NoError(t, err)

mountConfig, err = testVault.Client().Sys().MountConfig("pki-intermediate")
require.NoError(t, err)
require.Equal(t, 333*3600, mountConfig.MaxLeaseTTL)
}

func getIntermediateCertTTL(t *testing.T, caConf *structs.CAConfiguration) time.Duration {
t.Helper()

Expand Down