diff --git a/acl/acl_test.go b/acl/acl_test.go index 57c3cfa2b..51c1a6189 100644 --- a/acl/acl_test.go +++ b/acl/acl_test.go @@ -586,6 +586,31 @@ func TestSecureVariablesMatching(t *testing.T) { op: "read", allow: false, }, + { + name: "wildcard with more specific denied path", + policy: `namespace "ns" { + secure_variables { + path "*" { capabilities = ["list"] } + path "system/*" { capabilities = ["deny"] }}}`, + ns: "ns", + path: "system/not-allowed", + op: "list", + allow: false, + }, + { + name: "multiple namespace with overlapping paths", + policy: `namespace "ns" { + secure_variables { + path "*" { capabilities = ["list"] } + path "system/*" { capabilities = ["deny"] }}} + namespace "prod" { + secure_variables { + path "*" { capabilities = ["list"]}}}`, + ns: "prod", + path: "system/is-allowed", + op: "list", + allow: true, + }, } for _, tc := range tests { diff --git a/acl/policy.go b/acl/policy.go index d74df4437..d52d4ee15 100644 --- a/acl/policy.go +++ b/acl/policy.go @@ -75,6 +75,7 @@ const ( SecureVariablesCapabilityRead = "read" SecureVariablesCapabilityWrite = "write" SecureVariablesCapabilityDestroy = "destroy" + SecureVariablesCapabilityDeny = "deny" ) // Policy represents a parsed HCL or JSON policy. @@ -187,7 +188,7 @@ func isNamespaceCapabilityValid(cap string) bool { func isPathCapabilityValid(cap string) bool { switch cap { case SecureVariablesCapabilityWrite, SecureVariablesCapabilityRead, - SecureVariablesCapabilityList, SecureVariablesCapabilityDestroy: + SecureVariablesCapabilityList, SecureVariablesCapabilityDestroy, SecureVariablesCapabilityDeny: return true default: return false @@ -269,6 +270,8 @@ func expandSecureVariablesCapabilities(caps []string) []string { var foundRead, foundList bool for _, cap := range caps { switch cap { + case SecureVariablesCapabilityDeny: + return []string{SecureVariablesCapabilityDeny} case SecureVariablesCapabilityRead: foundRead = true case SecureVariablesCapabilityList: diff --git a/nomad/secure_variables_endpoint.go b/nomad/secure_variables_endpoint.go index d68238c3c..c1b15446b 100644 --- a/nomad/secure_variables_endpoint.go +++ b/nomad/secure_variables_endpoint.go @@ -210,7 +210,7 @@ func (sv *SecureVariables) Read(args *structs.SecureVariablesReadRequest, reply } defer metrics.MeasureSince([]string{"nomad", "secure_variables", "read"}, time.Now()) - err := sv.handleMixedAuthEndpoint(args.QueryOptions, + _, err := sv.handleMixedAuthEndpoint(args.QueryOptions, acl.PolicyRead, args.Path) if err != nil { return err @@ -261,7 +261,7 @@ func (sv *SecureVariables) List( return sv.listAllSecureVariables(args, reply) } - err := sv.handleMixedAuthEndpoint(args.QueryOptions, + aclObj, err := sv.handleMixedAuthEndpoint(args.QueryOptions, acl.PolicyList, args.Prefix) if err != nil { return err @@ -291,13 +291,23 @@ func (sv *SecureVariables) List( }, ) + filters := []paginator.Filter{ + paginator.GenericFilter{ + Allow: func(raw interface{}) (bool, error) { + sv := raw.(*structs.SecureVariableEncrypted) + return strings.HasPrefix(sv.Path, args.Prefix) && + (aclObj == nil || aclObj.AllowSecureVariableOperation(sv.Namespace, sv.Path, acl.PolicyList)), nil + }, + }, + } + // Set up our output after we have checked the error. var svs []*structs.SecureVariableMetadata // Build the paginator. This includes the function that is // responsible for appending a variable to the secure variables // stubs slice. - paginatorImpl, err := paginator.NewPaginator(iter, tokenizer, nil, args.QueryOptions, + paginatorImpl, err := paginator.NewPaginator(iter, tokenizer, filters, args.QueryOptions, func(raw interface{}) error { sv := raw.(*structs.SecureVariableEncrypted) svStub := sv.SecureVariableMetadata @@ -356,7 +366,7 @@ func (s *SecureVariables) listAllSecureVariables( // Identify which namespaces the caller has access to. If they do // not have access to any, send them an empty response. Otherwise, // handle any error in a traditional manner. - allowedNSes, err := allowedNSes(aclObj, stateStore, allowFunc) + _, err := allowedNSes(aclObj, stateStore, allowFunc) switch err { case structs.ErrPermissionDenied: reply.Data = make([]*structs.SecureVariableMetadata, 0) @@ -384,24 +394,19 @@ func (s *SecureVariables) listAllSecureVariables( }, ) - // Wrap the SecureVariables iterator with a FilterIterator to - // eliminate invalid values before sending them to the paginator. - fltrIter := memdb.NewFilterIterator(iter, func(raw interface{}) bool { - - // Values are filtered when the func returns true. - sv := raw.(*structs.SecureVariableEncrypted) - if allowedNSes != nil && !allowedNSes[sv.Namespace] { - return true - } - if !strings.HasPrefix(sv.Path, args.Prefix) { - return true - } - return false - }) + filters := []paginator.Filter{ + paginator.GenericFilter{ + Allow: func(raw interface{}) (bool, error) { + sv := raw.(*structs.SecureVariableEncrypted) + return strings.HasPrefix(sv.Path, args.Prefix) && + (aclObj == nil || aclObj.AllowSecureVariableOperation(sv.Namespace, sv.Path, acl.PolicyList)), nil + }, + }, + } // Build the paginator. This includes the function that is // responsible for appending a variable to the stubs array. - paginatorImpl, err := paginator.NewPaginator(fltrIter, tokenizer, nil, args.QueryOptions, + paginatorImpl, err := paginator.NewPaginator(iter, tokenizer, filters, args.QueryOptions, func(raw interface{}) error { sv := raw.(*structs.SecureVariableEncrypted) svStub := sv.SecureVariableMetadata @@ -465,7 +470,7 @@ func (sv *SecureVariables) decrypt(v *structs.SecureVariableEncrypted) (*structs // handleMixedAuthEndpoint is a helper to handle auth on RPC endpoints that can // either be called by external clients or by workload identity -func (sv *SecureVariables) handleMixedAuthEndpoint(args structs.QueryOptions, cap, pathOrPrefix string) error { +func (sv *SecureVariables) handleMixedAuthEndpoint(args structs.QueryOptions, cap, pathOrPrefix string) (*acl.ACL, error) { // Perform the initial token resolution. aclObj, err := sv.srv.ResolveToken(args.AuthToken) @@ -474,15 +479,15 @@ func (sv *SecureVariables) handleMixedAuthEndpoint(args structs.QueryOptions, ca // are not enabled, otherwise trigger the allowed namespace function. if aclObj != nil { if !aclObj.AllowSecureVariableOperation(args.RequestNamespace(), pathOrPrefix, cap) { - return structs.ErrPermissionDenied + return nil, structs.ErrPermissionDenied } } - return nil + return aclObj, nil } if helper.IsUUID(args.AuthToken) { // early return for ErrNotFound or other errors if it's formed // like an ACLToken.SecretID - return err + return nil, err } // Attempt to verify the token as a JWT with a workload @@ -492,27 +497,27 @@ func (sv *SecureVariables) handleMixedAuthEndpoint(args structs.QueryOptions, ca metrics.IncrCounter([]string{ "nomad", "secure_variables", "invalid_allocation_identity"}, 1) sv.logger.Trace("allocation identity was not valid", "error", err) - return structs.ErrPermissionDenied + return nil, structs.ErrPermissionDenied } // The workload identity gets access to paths that match its // identity, without having to go thru the ACL system err = sv.authValidatePrefix(claims, args.RequestNamespace(), pathOrPrefix) if err == nil { - return nil + return aclObj, 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) if err != nil { - return err // this only returns an error when the state store has gone wrong + return nil, err // this only returns an error when the state store has gone wrong } if aclObj != nil && aclObj.AllowSecureVariableOperation( args.RequestNamespace(), pathOrPrefix, cap) { - return nil + return aclObj, nil } - return structs.ErrPermissionDenied + return nil, structs.ErrPermissionDenied } // authValidatePrefix asserts that the requested path is valid for diff --git a/nomad/secure_variables_endpoint_test.go b/nomad/secure_variables_endpoint_test.go index 15b2a1db2..58aafd892 100644 --- a/nomad/secure_variables_endpoint_test.go +++ b/nomad/secure_variables_endpoint_test.go @@ -7,6 +7,7 @@ import ( "testing" msgpackrpc "github.com/hashicorp/net-rpc-msgpackrpc" + "github.com/shoenig/test" "github.com/shoenig/test/must" "github.com/hashicorp/nomad/acl" @@ -91,7 +92,7 @@ func TestSecureVariablesEndpoint_auth(t *testing.T) { must.NoError(t, err) t.Run("terminal alloc should be denied", func(t *testing.T) { - err = srv.staticEndpoints.SecureVariables.handleMixedAuthEndpoint( + _, err = srv.staticEndpoints.SecureVariables.handleMixedAuthEndpoint( structs.QueryOptions{AuthToken: idToken, Namespace: ns}, "n/a", fmt.Sprintf("nomad/jobs/%s/web/web", jobID)) must.EqError(t, err, structs.ErrPermissionDenied.Error()) @@ -103,7 +104,7 @@ func TestSecureVariablesEndpoint_auth(t *testing.T) { structs.MsgTypeTestSetup, 1200, []*structs.Allocation{alloc1})) t.Run("wrong namespace should be denied", func(t *testing.T) { - err = srv.staticEndpoints.SecureVariables.handleMixedAuthEndpoint( + _, err = srv.staticEndpoints.SecureVariables.handleMixedAuthEndpoint( structs.QueryOptions{AuthToken: idToken, Namespace: structs.DefaultNamespace}, "n/a", fmt.Sprintf("nomad/jobs/%s/web/web", jobID)) must.EqError(t, err, structs.ErrPermissionDenied.Error()) @@ -246,7 +247,7 @@ func TestSecureVariablesEndpoint_auth(t *testing.T) { for _, tc := range testCases { tc := tc t.Run(tc.name, func(t *testing.T) { - err := srv.staticEndpoints.SecureVariables.handleMixedAuthEndpoint( + _, err := srv.staticEndpoints.SecureVariables.handleMixedAuthEndpoint( structs.QueryOptions{AuthToken: tc.token, Namespace: ns}, tc.cap, tc.path) if tc.expectedErr == nil { must.NoError(t, err) @@ -438,3 +439,124 @@ func TestSecureVariablesEndpoint_Apply_ACL(t *testing.T) { must.Equals(t, sv.Items, applyResp.Output.Items) }) } + +func TestSecureVariablesEndpoint_ComplexACLPolicies(t *testing.T) { + + ci.Parallel(t) + srv, _, shutdown := TestACLServer(t, func(c *Config) { + c.NumSchedulers = 0 // Prevent automatic dequeue + }) + defer shutdown() + testutil.WaitForLeader(t, srv.RPC) + codec := rpcClient(t, srv) + + idx := uint64(1000) + + policyRules := ` +namespace "dev" { + secure_variables { + path "*" { capabilities = ["list", "read"] } + path "system/*" { capabilities = ["deny"] } + path "config/system/*" { capabilities = ["deny"] } + } +} + +namespace "prod" { + secure_variables { + path "*" { + capabilities = ["list"] + } + } +} + +namespace "*" {} +` + + store := srv.fsm.State() + + must.NoError(t, store.UpsertNamespaces(1000, []*structs.Namespace{ + {Name: "dev"}, {Name: "prod"}, {Name: "other"}})) + + idx++ + token := mock.CreatePolicyAndToken(t, store, idx, "developer", policyRules) + + writeVar := func(ns, path string) { + idx++ + sv := mock.SecureVariableEncrypted() + sv.Namespace = ns + sv.Path = path + resp := store.SVESet(idx, &structs.SVApplyStateRequest{ + Op: structs.SVOpSet, + Var: sv, + }) + must.NoError(t, resp.Error) + } + + writeVar("dev", "system/never-list") + writeVar("dev", "config/system/never-list") + writeVar("dev", "config/can-read") + writeVar("dev", "project/can-read") + + writeVar("prod", "system/can-list") + writeVar("prod", "config/system/can-list") + writeVar("prod", "config/can-list") + writeVar("prod", "project/can-list") + + writeVar("other", "system/never-list") + writeVar("other", "config/system/never-list") + writeVar("other", "config/never-list") + writeVar("other", "project/never-list") + + testListPrefix := func(ns, prefix string, expectedCount int, expectErr error) { + t.Run(fmt.Sprintf("ns=%s-prefix=%s", ns, prefix), func(t *testing.T) { + req := &structs.SecureVariablesListRequest{ + QueryOptions: structs.QueryOptions{ + Namespace: ns, + Prefix: prefix, + AuthToken: token.SecretID, + Region: "global", + }, + } + var resp structs.SecureVariablesListResponse + + if expectErr != nil { + must.EqError(t, + msgpackrpc.CallWithCodec(codec, "SecureVariables.List", req, &resp), + expectErr.Error()) + return + } + must.NoError(t, msgpackrpc.CallWithCodec(codec, "SecureVariables.List", req, &resp)) + + found := "found:\n" + for _, sv := range resp.Data { + found += fmt.Sprintf(" ns=%s path=%s\n", sv.Namespace, sv.Path) + } + must.Len(t, expectedCount, resp.Data, test.Sprintf("%s", found)) + }) + } + + testListPrefix("dev", "system", 0, nil) + testListPrefix("dev", "config/system", 0, nil) + testListPrefix("dev", "config", 1, nil) + testListPrefix("dev", "project", 1, nil) + testListPrefix("dev", "", 2, nil) + + testListPrefix("prod", "system", 1, nil) + testListPrefix("prod", "config/system", 1, nil) + testListPrefix("prod", "config", 2, nil) + testListPrefix("prod", "project", 1, nil) + testListPrefix("prod", "", 4, nil) + + testListPrefix("other", "system", 0, structs.ErrPermissionDenied) + testListPrefix("other", "config/system", 0, structs.ErrPermissionDenied) + testListPrefix("other", "config", 0, structs.ErrPermissionDenied) + testListPrefix("other", "project", 0, structs.ErrPermissionDenied) + testListPrefix("other", "", 0, structs.ErrPermissionDenied) + + testListPrefix("*", "system", 1, nil) + testListPrefix("*", "config/system", 1, nil) + testListPrefix("*", "config", 3, nil) + testListPrefix("*", "project", 2, nil) + testListPrefix("*", "", 6, nil) + +} diff --git a/website/content/docs/other-specifications/acl-policy.mdx b/website/content/docs/other-specifications/acl-policy.mdx index 3375e005b..0e052be2a 100644 --- a/website/content/docs/other-specifications/acl-policy.mdx +++ b/website/content/docs/other-specifications/acl-policy.mdx @@ -202,6 +202,7 @@ Variables are as follows: | read | Read the decrypted contents of Secure Variables at this path. Also includes the "list" capability | | list | List the metadata but not contents of Secure Variables at this path. | | destroy | Delete Secure Variables at this path. | +| deny | No permissions at this path. Deny takes precedence over other capabilities. | For example, the policy below allows full access to secure variables at all paths in the "dev" namespace that are prefixed with "project/", but only read