diff --git a/changelog/_10444.txt b/changelog/_10444.txt new file mode 100644 index 0000000000..2c4e47ae8b --- /dev/null +++ b/changelog/_10444.txt @@ -0,0 +1,3 @@ +```release-note:improvement +policies: add warning about list comparison when using allowed_parameters or denied_parameters +``` \ No newline at end of file diff --git a/vault/policy_store.go b/vault/policy_store.go index 375499fae8..f805bea4c8 100644 --- a/vault/policy_store.go +++ b/vault/policy_store.go @@ -196,6 +196,9 @@ type PolicyStore struct { // logger is the server logger copied over from core logger log.Logger + + // deniedParamWarn tracks if we've already logged about the system using allowed_parameters or denied_parameters + deniedParamWarnOnce sync.Once } // PolicyEntry is used to store a policy by name @@ -412,6 +415,8 @@ func (ps *PolicyStore) setPolicyInternal(ctx context.Context, p *Policy) error { ps.tokenPoliciesLRU.Add(index, p) } + ps.logDeniedParamWarning(p) + case PolicyTypeRGP: aclView := ps.getACLView(p.namespace) acl, err := aclView.Get(ctx, entry.Key) @@ -941,3 +946,27 @@ func (ps *PolicyStore) sanitizeName(name string) string { func (ps *PolicyStore) cacheKey(ns *namespace.Namespace, name string) string { return path.Join(ns.ID, name) } + +// logDeniedParamWarning logs a warning if the given policy uses allowed_parameters or denied_parameters +// TODO (DENIED_PARAMETERS_CHANGE): Remove this function after deprecation is done +func (ps *PolicyStore) logDeniedParamWarning(p *Policy) { + if p == nil { + return + } + // check if the policy uses allowed_parameters or denied_parameters + var usesAllowedOrDenied bool + for _, path := range p.Paths { + if path.Permissions != nil { + if len(path.Permissions.AllowedParameters) > 0 || len(path.Permissions.DeniedParameters) > 0 { + usesAllowedOrDenied = true + break + } + } + } + + if usesAllowedOrDenied { + ps.deniedParamWarnOnce.Do(func() { + ps.logger.Warn("you're using 'allowed_parameters' or 'denied_parameters' in one or more policies, note that Vault 1.21 introduced a behavior change. For details see https://developer.hashicorp.com/vault/docs/v1.21.x/updates/important-changes#item-by-item-list-comparison-for-allowed_parameters-and-denied_parameters") + }) + } +} diff --git a/vault/policy_store_test.go b/vault/policy_store_test.go index 8d1b4596f0..1f7dfa88cb 100644 --- a/vault/policy_store_test.go +++ b/vault/policy_store_test.go @@ -495,3 +495,86 @@ path "foo" { require.Error(t, err) require.Contains(t, err.Error(), "failed to parse policy: failed to parse policy: The argument \"capabilities\" at 61:2 was already set. Each argument can only be defined once") } + +// TestPolicyStore_AllowedParametersWarning tests that a warning is logged when a policy containing +// allowed_parameters or denied_parameters is set in the policy store. +// TODO (DENIED_PARAMETERS_CHANGE): Remove this test after deprecation is done +func TestPolicyStore_AllowedParametersWarning(t *testing.T) { + tests := []struct { + name string + policyFragment string + expectLog bool + }{ + { + name: "allowed_parameters", + policyFragment: ` +path "foo" { + allowed_parameters = { + "param1" = ["val1", "val2"] + } + capabilities = ["read"] +} +`, + expectLog: true, + }, + { + name: "denied_parameters", + policyFragment: ` +path "foo" { + denied_parameters = { + "param1" = ["val1", "val2"] + } + capabilities = ["read"] +} +`, + expectLog: true, + }, + { + name: "no_parameters", + policyFragment: ` +path "foo" { + capabilities = ["read"] +} +`, + expectLog: false, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + logOut := new(bytes.Buffer) + conf := &CoreConfig{ + Logger: log.New(&log.LoggerOptions{ + Mutex: &sync.Mutex{}, + Level: log.Warn, + Output: logOut, + }), + } + core, _, _ := TestCoreUnsealedWithConfig(t, conf) + ps := core.policyStore + + // First policy + policy := aclPolicy + tc.policyFragment + parsedPolicy, err := ParseACLPolicy(namespace.RootNamespace, policy) + require.NoError(t, err) + + ctx := namespace.RootContext(context.Background()) + err = ps.SetPolicy(ctx, parsedPolicy) + require.NoError(t, err) + + if tc.expectLog { + require.Contains(t, logOut.String(), "you're using 'allowed_parameters' or 'denied_parameters' in one or more policies") + } else { + require.NotContains(t, logOut.String(), "you're using 'allowed_parameters' or 'denied_parameters' in one or more policies") + } + + // Reset log output and add a second policy + logOut.Reset() + err = ps.SetPolicy(ctx, parsedPolicy) + require.NoError(t, err) + + // Ensure no additional log is generated for the second policy + require.NotContains(t, logOut.String(), "you're using 'allowed_parameters' or 'denied_parameters' in one or more policies") + }) + } +}