Skip to content

Commit

Permalink
acl: respect deny capabilities on job's own variables
Browse files Browse the repository at this point in the history
ACL policies can be associated with a job so that the job's Workload Identity
can have expanded access to other policy objects, including other
variables. Policies set on the variables the job automatically has access to
were ignored, but this includes policies with `deny` capabilities.

We originally implemented automatic workload access to Variables as a separate
code path in the Variables RPC endpoint so that we don't have to generate
on-the-fly policies that blow up the ACL policy cache. This is fairly brittle
but also the behavior around wildcard paths in policies different from the rest
of our ACL polices, which is hard to reason about.

Add an `ACLClaim` parameter to the `AllowVariableOperation` method so that we
can push all this logic into the `acl` package and the behavior can be
consistent. This will allow a `deny` policy to override automatic access (and
probably speed up checks of non-automatic variable access).
  • Loading branch information
tgross committed Mar 10, 2023
1 parent 0815277 commit 5e5140e
Show file tree
Hide file tree
Showing 8 changed files with 209 additions and 81 deletions.
3 changes: 3 additions & 0 deletions .changelog/16349.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
```release-note:security
variables: Fixed a bug where a workload-associated policy with a deny capability was ignored for the workload's own variables [CVE-2023-1296](https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2023-1296)
```
33 changes: 27 additions & 6 deletions acl/acl.go
Original file line number Diff line number Diff line change
Expand Up @@ -358,20 +358,27 @@ func (a *ACL) AllowHostVolume(ns string) bool {
return !capabilities.Check(PolicyDeny)
}

func (a *ACL) AllowVariableOperation(ns, path, op string) bool {
func (a *ACL) AllowVariableOperation(ns, path, op string, claim *ACLClaim) bool {
if a.management {
return true
}

// Check for a matching capability set
capabilities, ok := a.matchingVariablesCapabilitySet(ns, path)
capabilities, ok := a.matchingVariablesCapabilitySet(ns, path, claim)
if !ok {
return false
}

return capabilities.Check(op)
}

type ACLClaim struct {
Namespace string
Job string
Group string
Task string
}

// AllowVariableSearch is a very loose check that the token has *any* access to
// a variables path for the namespace, with an expectation that the actual
// search result will be filtered by specific paths
Expand Down Expand Up @@ -460,17 +467,31 @@ func (a *ACL) matchingHostVolumeCapabilitySet(name string) (capabilitySet, bool)
return a.findClosestMatchingGlob(a.wildcardHostVolumes, name)
}

// matchingVariablesCapabilitySet looks for a capabilitySet that matches the namespace and path,
// if no concrete definitions are found, then we return the closest matching
// glob.
var workloadVariablesCapabilitySet = capabilitySet{"read": struct{}{}, "list": struct{}{}}

// matchingVariablesCapabilitySet looks for a capabilitySet in the following order:
// - matching the namespace and path from a policy
// - automatic access based on the claim
// - closest matching glob
//
// The closest matching glob is the one that has the smallest character
// difference between the namespace and the glob.
func (a *ACL) matchingVariablesCapabilitySet(ns, path string) (capabilitySet, bool) {
func (a *ACL) matchingVariablesCapabilitySet(ns, path string, claim *ACLClaim) (capabilitySet, bool) {
// Check for a concrete matching capability set
raw, ok := a.variables.Get([]byte(ns + "\x00" + path))
if ok {
return raw.(capabilitySet), true
}
if claim != nil && ns == claim.Namespace {
switch path {
case "nomad/jobs",
fmt.Sprintf("nomad/jobs/%s", claim.Job),
fmt.Sprintf("nomad/jobs/%s/%s", claim.Job, claim.Group),
fmt.Sprintf("nomad/jobs/%s/%s/%s", claim.Job, claim.Group, claim.Task):
return workloadVariablesCapabilitySet, true
default:
}
}

// We didn't find a concrete match, so lets try and evaluate globs.
return a.findClosestMatchingGlob(a.wildcardVariables, ns+"\x00"+path)
Expand Down
34 changes: 33 additions & 1 deletion acl/acl_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -457,6 +457,7 @@ func TestVariablesMatching(t *testing.T) {
ns string
path string
op string
claim *ACLClaim
allow bool
}{
{
Expand Down Expand Up @@ -611,6 +612,36 @@ func TestVariablesMatching(t *testing.T) {
op: "list",
allow: true,
},
{
name: "claim with more specific policy",
policy: `namespace "ns" {
variables { path "nomad/jobs/example" { capabilities = ["deny"] }}}`,
ns: "ns",
path: "nomad/jobs/example",
op: "read",
claim: &ACLClaim{Namespace: "ns", Job: "example", Group: "foo", Task: "bar"},
allow: false,
},
{
name: "claim with less specific policy",
policy: `namespace "ns" {
variables { path "nomad/jobs" { capabilities = ["deny"] }}}`,
ns: "ns",
path: "nomad/jobs/example",
op: "read",
claim: &ACLClaim{Namespace: "ns", Job: "example", Group: "foo", Task: "bar"},
allow: true,
},
{
name: "claim with less specific wildcard policy",
policy: `namespace "ns" {
variables { path "nomad/jobs/*" { capabilities = ["deny"] }}}`,
ns: "ns",
path: "nomad/jobs/example",
op: "read",
claim: &ACLClaim{Namespace: "ns", Job: "example", Group: "foo", Task: "bar"},
allow: true,
},
}

for _, tc := range tests {
Expand All @@ -622,7 +653,8 @@ func TestVariablesMatching(t *testing.T) {

acl, err := NewACL(false, []*Policy{policy})
require.NoError(t, err)
require.Equal(t, tc.allow, acl.AllowVariableOperation(tc.ns, tc.path, tc.op))
allowed := acl.AllowVariableOperation(tc.ns, tc.path, tc.op, tc.claim)
require.Equal(t, tc.allow, allowed)
})
}

Expand Down
6 changes: 2 additions & 4 deletions nomad/acl.go
Original file line number Diff line number Diff line change
Expand Up @@ -70,11 +70,9 @@ func (s *Server) ResolveClaims(claims *structs.IdentityClaims) (*acl.ACL, error)
if err != nil {
return nil, err
}
if len(policies) == 0 {
return nil, nil
}

// Compile and cache the ACL object
// Compile and cache the ACL object. For many claims this will result in an
// ACL object with no policies, which can be efficiently cached.
aclObj, err := structs.CompileACLObject(s.aclCache, policies)
if err != nil {
return nil, err
Expand Down
18 changes: 12 additions & 6 deletions nomad/acl_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -457,11 +457,6 @@ func TestResolveClaims(t *testing.T) {
JobID: claims.JobID,
}

index++
err := store.UpsertACLPolicies(structs.MsgTypeTestSetup, index, []*structs.ACLPolicy{
policy0, policy1, policy2, policy3, policy4, policy5, policy6, policy7})
must.NoError(t, err)

aclObj, err := srv.ResolveClaims(claims)
must.Nil(t, aclObj)
must.EqError(t, err, "allocation does not exist")
Expand All @@ -471,11 +466,22 @@ func TestResolveClaims(t *testing.T) {
err = store.UpsertAllocs(structs.MsgTypeTestSetup, index, []*structs.Allocation{alloc})
must.NoError(t, err)

// Resolve claims and check we that the ACL object without policies provides no access
aclObj, err = srv.ResolveClaims(claims)
must.NoError(t, err)
must.NotNil(t, aclObj)
must.False(t, aclObj.AllowNamespaceOperation("default", acl.NamespaceCapabilityListJobs))

// Check that the ACL object looks reasonable
// Add the policies
index++
err = store.UpsertACLPolicies(structs.MsgTypeTestSetup, index, []*structs.ACLPolicy{
policy0, policy1, policy2, policy3, policy4, policy5, policy6, policy7})
must.NoError(t, err)

// Re-resolve and check that the resulting ACL looks reasonable
aclObj, err = srv.ResolveClaims(claims)
must.NoError(t, err)
must.NotNil(t, aclObj)
must.False(t, aclObj.IsManagement())
must.True(t, aclObj.AllowNamespaceOperation("default", acl.NamespaceCapabilityListJobs))
must.False(t, aclObj.AllowNamespaceOperation("other", acl.NamespaceCapabilityListJobs))
Expand Down
69 changes: 30 additions & 39 deletions nomad/variables_endpoint.go
Original file line number Diff line number Diff line change
Expand Up @@ -109,7 +109,7 @@ func svePreApply(sv *Variables, args *structs.VariablesApplyRequest, vd *structs
} else if aclObj != nil {
hasPerm := func(perm string) bool {
return aclObj.AllowVariableOperation(args.Var.Namespace,
args.Var.Path, perm)
args.Var.Path, perm, nil)
}
canRead = hasPerm(acl.VariablesCapabilityRead)

Expand Down Expand Up @@ -507,63 +507,54 @@ func (sv *Variables) authorize(aclObj *acl.ACL, claims *structs.IdentityClaims,
// Perform normal ACL validation. If the ACL object is nil, that means we're
// working with an identity claim.
if aclObj != nil {
if !aclObj.AllowVariableOperation(ns, pathOrPrefix, cap) {
allowed := aclObj.AllowVariableOperation(ns, pathOrPrefix, cap, nil)
if !allowed {
return structs.ErrPermissionDenied
}
return nil
}

// Check the workload-associated policies and automatic task access to
// variables.
if claims != nil {
// The workload identity gets access to paths that match its
// identity, without having to go thru the ACL system
err := sv.authValidatePrefix(claims, ns, pathOrPrefix)
if err == nil {
return nil
}

// If the workload identity doesn't match the implicit permissions
// given to paths, check for its attached ACL policies
aclObj, err = sv.srv.ResolveClaims(claims)
aclObj, err := sv.srv.ResolveClaims(claims)
if err != nil {
return err // this only returns an error when the state store has gone wrong
return err // returns internal errors only
}
if aclObj != nil && aclObj.AllowVariableOperation(
ns, pathOrPrefix, cap) {
return nil

if aclObj != nil {
group, err := sv.groupForAlloc(claims)
if err != nil {
// returns ErrPermissionDenied for claims from terminal
// allocations, otherwise only internal errors
return err
}
allowed := aclObj.AllowVariableOperation(
ns, pathOrPrefix, cap, &acl.ACLClaim{
Namespace: claims.Namespace,
Job: claims.JobID,
Group: group,
Task: claims.TaskName,
})
if allowed {
return nil
}
}
}
return structs.ErrPermissionDenied
}

// authValidatePrefix asserts that the requested path is valid for
// this allocation
func (sv *Variables) authValidatePrefix(claims *structs.IdentityClaims, ns, pathOrPrefix string) error {

func (sv *Variables) groupForAlloc(claims *structs.IdentityClaims) (string, error) {
store, err := sv.srv.fsm.State().Snapshot()
if err != nil {
return err
return "", err
}
alloc, err := store.AllocByID(nil, claims.AllocationID)
if err != nil {
return err
return "", err
}
if alloc == nil || alloc.Job == nil {
return fmt.Errorf("allocation does not exist")
}
if alloc.Job.Namespace != ns {
return fmt.Errorf("allocation is in another namespace")
}

parts := strings.Split(pathOrPrefix, "/")
expect := []string{"nomad", "jobs", claims.JobID, alloc.TaskGroup, claims.TaskName}
if len(parts) > len(expect) {
return structs.ErrPermissionDenied
return "", structs.ErrPermissionDenied
}

for idx, part := range parts {
if part != expect[idx] {
return structs.ErrPermissionDenied
}
}
return nil
return alloc.TaskGroup, nil
}
Loading

0 comments on commit 5e5140e

Please sign in to comment.