From 1782b4e88042a7b7487aecca7c44755767b32d03 Mon Sep 17 00:00:00 2001 From: Hridoy Roy Date: Mon, 7 Jun 2021 09:15:35 -0700 Subject: [PATCH] oss part of control groups upgrade (#11772) * oss part of control groups upgrade * changelog and docs * formatting * formatting --- changelog/_1819.txt | 3 + vault/acl.go | 3 +- vault/policy.go | 35 ++++++- vault/policy_test.go | 71 ++++++++++++-- .../docs/enterprise/control-groups.mdx | 92 +++++++++++++++++++ 5 files changed, 190 insertions(+), 14 deletions(-) create mode 100644 changelog/_1819.txt diff --git a/changelog/_1819.txt b/changelog/_1819.txt new file mode 100644 index 000000000..9e6cb9069 --- /dev/null +++ b/changelog/_1819.txt @@ -0,0 +1,3 @@ +```release-note:feature +core: Add controlled capabilities to control group policy stanza +``` diff --git a/vault/acl.go b/vault/acl.go index 78a37dd3f..38fa5efc9 100644 --- a/vault/acl.go +++ b/vault/acl.go @@ -246,7 +246,8 @@ func NewACL(ctx context.Context, policies []*Policy) (*ACL, error) { existingPerms.MFAMethods = strutil.RemoveDuplicates(existingPerms.MFAMethods, false) } - // No need to dedupe this list since any authorization can satisfy any factor + // No need to dedupe this list since any authorization can satisfy any factor, so long as + // the factor matches the specified permission requested. if pc.Permissions.ControlGroup != nil { if len(pc.Permissions.ControlGroup.Factors) > 0 { if existingPerms.ControlGroup == nil { diff --git a/vault/policy.go b/vault/policy.go index e87372933..a99399d3c 100644 --- a/vault/policy.go +++ b/vault/policy.go @@ -44,6 +44,13 @@ const ( SudoCapabilityInt ) +// Error constants for testing +const ( + // ControlledCapabilityPolicySubsetError is thrown when a control group's controlled capabilities + // are not a subset of the policy's capabilities. + ControlledCapabilityPolicySubsetError = "control group factor capabilities must be a subset of the policy's capabilities" +) + type PolicyType uint32 const ( @@ -139,8 +146,9 @@ type ControlGroup struct { } type ControlGroupFactor struct { - Name string - Identity *IdentityFactor `hcl:"identity"` + Name string + Identity *IdentityFactor `hcl:"identity"` + ControlledCapabilities []string `hcl:"controlled_capabilities"` } type IdentityFactor struct { @@ -431,7 +439,6 @@ func parsePaths(result *Policy, list *ast.ObjectList, performTemplating bool, en } pc.Permissions.ControlGroup.TTL = dur } - var factors []*ControlGroupFactor if pc.ControlGroupHCL.Factors != nil { for key, factor := range pc.ControlGroupHCL.Factors { @@ -446,9 +453,27 @@ func parsePaths(result *Policy, list *ast.ObjectList, performTemplating bool, en return errors.New("must provide more than one identity group and approvals > 0") } + // Ensure that configured ControlledCapabilities for factor are a subset of the + // Capabilities of the policy. + if len(factor.ControlledCapabilities) > 0 { + var found bool + for _, controlledCapability := range factor.ControlledCapabilities { + found = false + for _, policyCap := range pc.Capabilities { + if controlledCapability == policyCap { + found = true + } + } + if !found { + return errors.New(ControlledCapabilityPolicySubsetError) + } + } + } + factors = append(factors, &ControlGroupFactor{ - Name: key, - Identity: factor.Identity, + Name: key, + Identity: factor.Identity, + ControlledCapabilities: factor.ControlledCapabilities, }) } } diff --git a/vault/policy_test.go b/vault/policy_test.go index d464dcc6c..045fae865 100644 --- a/vault/policy_test.go +++ b/vault/policy_test.go @@ -12,22 +12,18 @@ import ( var rawPolicy = strings.TrimSpace(` # Developer policy name = "dev" - # Deny all paths by default path "*" { policy = "deny" } - # Allow full access to staging path "stage/*" { policy = "sudo" } - # Limited read privilege to production path "prod/version" { policy = "read" } - # Read access to foobar # Also tests stripping of leading slash and parsing of min/max as string and # integer @@ -36,7 +32,6 @@ path "/foo/bar" { min_wrapping_ttl = 300 max_wrapping_ttl = "1h" } - # Add capabilities for creation and sudo to foobar # This will be separate; they are combined when compiled into an ACL # Also tests reverse string/int handling to the above @@ -45,7 +40,6 @@ path "foo/bar" { min_wrapping_ttl = "300s" max_wrapping_ttl = 3600 } - # Check that only allowed_parameters are being added to foobar path "foo/bar" { capabilities = ["create", "sudo"] @@ -54,7 +48,6 @@ path "foo/bar" { "zap" = [] } } - # Check that only denied_parameters are being added to bazbar path "baz/bar" { capabilities = ["create", "sudo"] @@ -63,7 +56,6 @@ path "baz/bar" { "zap" = [] } } - # Check that both allowed and denied parameters are being added to bizbar path "biz/bar" { capabilities = ["create", "sudo"] @@ -351,6 +343,69 @@ nope = "yes" } } +// TestPolicy_ParseControlGroupWrongCaps makes sure an appropriate error is +// thrown when a factor's controlled_capabilities are not a subset of +// the path capabilities. +func TestPolicy_ParseControlGroupWrongCaps(t *testing.T) { + _, err := ParseACLPolicy(namespace.RootNamespace, strings.TrimSpace(` + name = "controlgroups" + path "secret/*" { + capabilities = ["create", "read"] + control_group = { + max_ttl = "1h" + factor "ops_manager" { + controlled_capabilities = ["read", "write"] + identity { + group_names = ["blah"] + approvals = 1 + } + } + } + } + `)) + if err == nil { + t.Fatalf("Bad policy was successfully parsed") + } + if !strings.Contains(err.Error(), ControlledCapabilityPolicySubsetError) { + t.Fatalf("Wrong error returned when control group's controlled capabilities are not a subset of the path capabilities: error was %s", err.Error()) + } +} + +func TestPolicy_ParseControlGroup(t *testing.T) { + pol, err := ParseACLPolicy(namespace.RootNamespace, strings.TrimSpace(` + name = "controlgroups" + path "secret/*" { + capabilities = ["create", "read"] + control_group = { + max_ttl = "1h" + factor "ops_manager" { + controlled_capabilities = ["create"] + identity { + group_names = ["blah"] + approvals = 1 + } + } + } + } + `)) + if err != nil { + t.Fatalf("Policy could not be parsed") + } + + // At this point paths haven't been merged yet. We must simply make sure + // that each factor has the correct associated permissions. + + permFactors := pol.Paths[0].Permissions.ControlGroup.Factors + + if len(permFactors) != 1 { + t.Fatalf("Expected 1 control group factor: got %d", len(permFactors)) + } + + if len(permFactors[0].ControlledCapabilities) != 1 && permFactors[0].ControlledCapabilities[0] != "create" { + t.Fatalf("controlled_capabilities on the first factor was not correct: %+v", permFactors[0].ControlledCapabilities) + } +} + func TestPolicy_ParseBadPath(t *testing.T) { // The wrong spelling is intended here _, err := ParseACLPolicy(namespace.RootNamespace, strings.TrimSpace(` diff --git a/website/content/docs/enterprise/control-groups.mdx b/website/content/docs/enterprise/control-groups.mdx index cce165921..24b55e1e0 100644 --- a/website/content/docs/enterprise/control-groups.mdx +++ b/website/content/docs/enterprise/control-groups.mdx @@ -25,6 +25,20 @@ Control Groups can verify the following factors: - `Identity Groups` - Require an authorizer to be in a specific set of identity groups. +### Controlled capabilities +Control group factors can be configured to trigger the control group workflow +on specific capabilities. This is done with the `controlled_capabilities` field. +Not specifying the `controlled_capabilities` field will necessitate the factor to be +checked for all operations to the specified policy path. The `controlled_capabilities` +field can differ per factor, so that different factors can be required for different +operations. + +Finally, the capabilities in the `controlled_capabilities` stanza must be a subset of the +`capabilities` specifed in the policy itself. For example, a policy giving only `read` access to +the path `secret/foo` cannot specify a control group factor with `list` as a controlled capability. + +Please see the following section for example ACL Policies. + ## Control Groups In ACL Policies Control Group requirements on paths are specified as `control_group` along @@ -76,6 +90,84 @@ group authorizes the request. If an authorizer is a member of both the "managers" and "superusers" group, one authorization for both factors will be satisfied. +``` +path "secret/foo" { + capabilities = ["write","read"] + control_group = { + factor "admin" { + controlled_capabilities = ["write"] + identity { + group_names = ["admin"] + approvals = 1 + } + } + } +} +``` + +The above policy grants `read` access to `secret/foo` for anyone that has a vault token +with this policy. It grants `write` access to `secret/foo` only after one member from the +admin group authorizes the request. + +``` +path "kv/*" { + capabilities = ["create", "update","delete","list","sudo"] + control_group = { + factor "admin" { + controlled_capabilities = ["delete","list","sudo"] + identity { + group_names = ["admin"] + approvals = 1 + } + } + } +} +path "kv/*" { + capabilities = ["create"] + control_group = { + factor "superuser" { + identity { + group_names = ["superuser"] + approvals = 2 + } + } + } +} + +``` + +Because the second path stanza has a control group factor with no `controlled_capabilities` field, +any token with this policy will be required to get 2 approvals from the `superuser` group before executing +any operation against `kv/*`. In addition, by virtue of the `controlled_capablities` field in the first +path stanza, `delete`,`list`, and `sudo` operations will require an additional approval from the `admin` group. + +``` +path "kv/*" { + capabilities = ["read", "list", "create"] + control_group = { + controlled_capabilities = ["read"] + factor "admin" { + identity { + group_names = ["admin"] + approvals = 1 + } + } + factor "superuser" { + controlled_capabilities = ["create"] + identity { + group_names = ["superuser"] + approvals = 1 + } + } + } +} +``` + +In this case, `read` will require one admin approval and `create` will require +one superuser approval and one admin approval. `List` will require no extra approvals +from any of the control group factors, and a token with this policy will not be required +to go through the control group workflow in order to execute a read operation against `kv/*`. + ## Control Groups in Sentinel Control Groups are also supported in Sentinel policies using the `controlgroup`