From 800c67c58aff28389e78f12bd3aa6f58efdbacd6 Mon Sep 17 00:00:00 2001 From: James Phillips Date: Thu, 8 Dec 2016 16:01:01 -0800 Subject: [PATCH] Adds complete ACL coverage for /v1/catalog/register. --- consul/acl.go | 100 +++++++++++++ consul/acl_test.go | 239 +++++++++++++++++++++++++++++++- consul/catalog_endpoint.go | 27 +++- consul/catalog_endpoint_test.go | 27 +++- consul/state/state_store.go | 6 + consul/structs/structs.go | 19 +++ consul/structs/structs_test.go | 38 +++++ 7 files changed, 442 insertions(+), 14 deletions(-) diff --git a/consul/acl.go b/consul/acl.go index 278395f84e..cc3b35cf3d 100644 --- a/consul/acl.go +++ b/consul/acl.go @@ -544,3 +544,103 @@ func (s *Server) filterACL(token string, subj interface{}) error { return nil } + +// vetRegisterWithACL applies the given ACL's policy to the catalog update and +// determines if it is allowed. Since the catalog register request is so +// dynamic, this is a pretty complex algorithm and was worth breaking out of the +// endpoint. +// +// This is a bit racy because we have to check the state store outside of a +// transaction. It's the best we can do because we don't want to flow ACL +// checking down there. The node information doesn't change in practice, so this +// will be fine. If we expose ways to change node addresses in a later version, +// then we should split the catalog API at the node and service level so we can +// address this race better (even then it would be super rare, and would at +// worst let a service update revert a recent node update, so it doesn't open up +// too much abuse). +func vetRegisterWithACL(acl acl.ACL, subj *structs.RegisterRequest, ns *structs.NodeServices) error { + // Fast path if ACLs are not enabled. + if acl == nil { + return nil + } + + // Vet the node info. This allows service updates to re-post the required + // node info for each request without having to have node "write" + // privileges. + needsNode := ns == nil || subj.ChangesNode(ns.Node) + if needsNode && !acl.NodeWrite(subj.Node) { + return permissionDeniedErr + } + + // Vet the service change. This includes making sure they can register + // the given service, and that we can write to any existing service that + // is being modified by id (if any). + if subj.Service != nil { + if !acl.ServiceWrite(subj.Service.Service) { + return permissionDeniedErr + } + + if ns != nil { + other, ok := ns.Services[subj.Service.ID] + if ok && !acl.ServiceWrite(other.Service) { + return permissionDeniedErr + } + } + } + + // Make sure that the member was flattened before we got there. This + // keeps us from having to verify this check as well. + if subj.Check != nil { + return fmt.Errorf("check member must be nil") + } + + // Vet the checks. Node-level checks require node write, and + // service-level checks require service write. + for _, check := range subj.Checks { + // Make sure that the node matches - we don't allow you to mix + // checks from other nodes because we'd have to pull a bunch + // more state store data to check this. If ACLs are enabled then + // we simply require them to match in a given request. There's a + // note in state_store.go to ban this down there in Consul 0.8, + // but it's good to leave this here because it's required for + // correctness wrt. ACLs. + if check.Node != subj.Node { + return fmt.Errorf("Node '%s' for check '%s' doesn't match register request node '%s'", + check.Node, check.CheckID, subj.Node) + } + + // Node-level check. + if check.ServiceID == "" { + if !acl.NodeWrite(subj.Node) { + return permissionDeniedErr + } + continue + } + + // Service-level check, check the common case where it + // matches the service part of this request, which has + // already been vetted above, and might be being registered + // along with its checks. + if subj.Service != nil && subj.Service.ID == check.ServiceID { + continue + } + + // Service-level check for some other service. Make sure they've + // got write permissions for that service. + if ns == nil { + return fmt.Errorf("Unknown service '%s' for check '%s'", + check.ServiceID, check.CheckID) + } else { + other, ok := ns.Services[check.ServiceID] + if !ok { + return fmt.Errorf("Unknown service '%s' for check '%s'", + check.ServiceID, check.CheckID) + } + if !acl.ServiceWrite(other.Service) { + return permissionDeniedErr + } + } + } + + return nil +} diff --git a/consul/acl_test.go b/consul/acl_test.go index 6eb9308bd6..1d36aaa1bb 100644 --- a/consul/acl_test.go +++ b/consul/acl_test.go @@ -5,6 +5,7 @@ import ( "fmt" "os" "reflect" + "strings" "testing" "github.com/hashicorp/consul/acl" @@ -12,6 +13,15 @@ import ( "github.com/hashicorp/consul/testutil" ) +var testACLPolicy = ` +key "" { + policy = "deny" +} +key "foo/" { + policy = "write" +} +` + func TestACL_Disabled(t *testing.T) { dir1, s1 := testServer(t) defer os.RemoveAll(dir1) @@ -1106,11 +1116,228 @@ func TestACL_unhandledFilterType(t *testing.T) { srv.filterACL(token, &structs.HealthCheck{}) } -var testACLPolicy = ` -key "" { - policy = "deny" +func TestACL_vetRegisterWithACL(t *testing.T) { + args := &structs.RegisterRequest{ + Node: "nope", + Address: "127.0.0.1", + } + + // With a nil ACL, the update should be allowed. + if err := vetRegisterWithACL(nil, args, nil); err != nil { + t.Fatalf("err: %v", err) + } + + // Create a basic node policy. + policy, err := acl.Parse(` +node "node" { + policy = "write" } -key "foo/" { - policy = "write" +`) + if err != nil { + t.Fatalf("err %v", err) + } + perms, err := acl.New(acl.DenyAll(), policy) + if err != nil { + t.Fatalf("err: %v", err) + } + + // With that policy, the update should now be blocked for node reasons. + err = vetRegisterWithACL(perms, args, nil) + if err == nil || !strings.Contains(err.Error(), permissionDenied) { + t.Fatalf("bad: %v", err) + } + + // Now use a permitted node name. + args.Node = "node" + if err := vetRegisterWithACL(perms, args, nil); err != nil { + t.Fatalf("err: %v", err) + } + + // Build some node info that matches what we have now. + ns := &structs.NodeServices{ + Node: &structs.Node{ + Node: "node", + Address: "127.0.0.1", + }, + Services: make(map[string]*structs.NodeService), + } + + // Try to register a service, which should be blocked. + args.Service = &structs.NodeService{ + Service: "service", + ID: "my-id", + } + err = vetRegisterWithACL(perms, args, ns) + if err == nil || !strings.Contains(err.Error(), permissionDenied) { + t.Fatalf("bad: %v", err) + } + + // Chain on a basic service policy. + policy, err = acl.Parse(` +service "service" { + policy = "write" +} +`) + if err != nil { + t.Fatalf("err %v", err) + } + perms, err = acl.New(perms, policy) + if err != nil { + t.Fatalf("err: %v", err) + } + + // With the service ACL, the update should go through. + if err := vetRegisterWithACL(perms, args, ns); err != nil { + t.Fatalf("err: %v", err) + } + + // Add an existing service that they are clobbering and aren't allowed + // to write to. + ns.Services["my-id"] = &structs.NodeService{ + Service: "other", + ID: "my-id", + } + err = vetRegisterWithACL(perms, args, ns) + if err == nil || !strings.Contains(err.Error(), permissionDenied) { + t.Fatalf("bad: %v", err) + } + + // Chain on a policy that allows them to write to the other service. + policy, err = acl.Parse(` +service "other" { + policy = "write" +} +`) + if err != nil { + t.Fatalf("err %v", err) + } + perms, err = acl.New(perms, policy) + if err != nil { + t.Fatalf("err: %v", err) + } + + // Now it should go through. + if err := vetRegisterWithACL(perms, args, ns); err != nil { + t.Fatalf("err: %v", err) + } + + // Try creating the node and the service at once by having no existing + // node record. This should be ok since we have node and service + // permissions. + if err := vetRegisterWithACL(perms, args, nil); err != nil { + t.Fatalf("err: %v", err) + } + + // Add a node-level check to the member, which should be rejected. + args.Check = &structs.HealthCheck{ + Node: "node", + } + err = vetRegisterWithACL(perms, args, ns) + if err == nil || !strings.Contains(err.Error(), "check member must be nil") { + t.Fatalf("bad: %v", err) + } + + // Move the check into the slice, but give a bad node name. + args.Check.Node = "nope" + args.Checks = append(args.Checks, args.Check) + args.Check = nil + err = vetRegisterWithACL(perms, args, ns) + if err == nil || !strings.Contains(err.Error(), "doesn't match register request node") { + t.Fatalf("bad: %v", err) + } + + // Fix the node name, which should now go through. + args.Checks[0].Node = "node" + if err := vetRegisterWithACL(perms, args, ns); err != nil { + t.Fatalf("err: %v", err) + } + + // Add a service-level check. + args.Checks = append(args.Checks, &structs.HealthCheck{ + Node: "node", + ServiceID: "my-id", + }) + if err := vetRegisterWithACL(perms, args, ns); err != nil { + t.Fatalf("err: %v", err) + } + + // Try creating everything at once. This should be ok since we have all + // the permissions we need. It also makes sure that we can register a + // new node, service, and associated checks. + if err := vetRegisterWithACL(perms, args, nil); err != nil { + t.Fatalf("err: %v", err) + } + + // Nil out the service registration, which'll skip the special case + // and force us to look at the ns data (it will look like we are + // writing to the "other" service which also has "my-id"). + args.Service = nil + if err := vetRegisterWithACL(perms, args, ns); err != nil { + t.Fatalf("err: %v", err) + } + + // Chain on a policy that forbids them to write to the other service. + policy, err = acl.Parse(` +service "other" { + policy = "deny" +} +`) + if err != nil { + t.Fatalf("err %v", err) + } + perms, err = acl.New(perms, policy) + if err != nil { + t.Fatalf("err: %v", err) + } + + // This should get rejected. + err = vetRegisterWithACL(perms, args, ns) + if err == nil || !strings.Contains(err.Error(), permissionDenied) { + t.Fatalf("bad: %v", err) + } + + // Change the existing service data to point to a service name they + // car write to. This should go through. + ns.Services["my-id"] = &structs.NodeService{ + Service: "service", + ID: "my-id", + } + if err := vetRegisterWithACL(perms, args, ns); err != nil { + t.Fatalf("err: %v", err) + } + + // Chain on a policy that forbids them to write to the node. + policy, err = acl.Parse(` +node "node" { + policy = "deny" +} +`) + if err != nil { + t.Fatalf("err %v", err) + } + perms, err = acl.New(perms, policy) + if err != nil { + t.Fatalf("err: %v", err) + } + + // This should get rejected because there's a node-level check in here. + err = vetRegisterWithACL(perms, args, ns) + if err == nil || !strings.Contains(err.Error(), permissionDenied) { + t.Fatalf("bad: %v", err) + } + + // Change the node-level check into a service check, and then it should + // go through. + args.Checks[0].ServiceID = "my-id" + if err := vetRegisterWithACL(perms, args, ns); err != nil { + t.Fatalf("err: %v", err) + } + + // Finally, attempt to update the node part of the data and make sure + // that gets rejected since they no longer have permissions. + args.Address = "127.0.0.2" + err = vetRegisterWithACL(perms, args, ns) + if err == nil || !strings.Contains(err.Error(), permissionDenied) { + t.Fatalf("bad: %v", err) + } } -` diff --git a/consul/catalog_endpoint.go b/consul/catalog_endpoint.go index 7b85aeec94..6c49417580 100644 --- a/consul/catalog_endpoint.go +++ b/consul/catalog_endpoint.go @@ -21,7 +21,7 @@ func (c *Catalog) Register(args *structs.RegisterRequest, reply *struct{}) error } defer metrics.MeasureSince([]string{"consul", "catalog", "register"}, time.Now()) - // Verify the args + // Verify the args. if args.Node == "" || args.Address == "" { return fmt.Errorf("Must provide node and address") } @@ -32,30 +32,31 @@ func (c *Catalog) Register(args *structs.RegisterRequest, reply *struct{}) error return err } + // Handle a service registration. if args.Service != nil { // If no service id, but service name, use default if args.Service.ID == "" && args.Service.Service != "" { args.Service.ID = args.Service.Service } - // Verify ServiceName provided if ID + // Verify ServiceName provided if ID. if args.Service.ID != "" && args.Service.Service == "" { return fmt.Errorf("Must provide service name with ID") } // Apply the ACL policy if any. The 'consul' service is excluded // since it is managed automatically internally (that behavior - // is going away after version 0.8). - if c.srv.config.ACLEnforceVersion8 || - (args.Service.Service != ConsulServiceName) { + // is going away after version 0.8). We check this same policy + // later if version 0.8 is enabled, so we can eventually just + // delete this and do all the ACL checks down there. + if args.Service.Service != ConsulServiceName { if acl != nil && !acl.ServiceWrite(args.Service.Service) { - c.srv.logger.Printf("[WARN] consul.catalog: Register of service '%s' on '%s' denied due to ACLs", - args.Service.Service, args.Node) return permissionDeniedErr } } } + // Move the old format single check into the slice, and fixup IDs. if args.Check != nil { args.Checks = append(args.Checks, args.Check) args.Check = nil @@ -69,6 +70,18 @@ func (c *Catalog) Register(args *structs.RegisterRequest, reply *struct{}) error } } + // Check the complete register request against the given ACL policy. + if c.srv.config.ACLEnforceVersion8 { + state := c.srv.fsm.State() + _, ns, err := state.NodeServices(args.Node) + if err != nil { + return fmt.Errorf("Node lookup failed: %v", err) + } + if err := vetRegisterWithACL(acl, args, ns); err != nil { + return err + } + } + _, err = c.srv.raftApply(structs.RegisterRequestType, args) if err != nil { c.srv.logger.Printf("[ERR] consul.catalog: Register failed: %v", err) diff --git a/consul/catalog_endpoint_test.go b/consul/catalog_endpoint_test.go index 4282eea031..20c5efdd0d 100644 --- a/consul/catalog_endpoint_test.go +++ b/consul/catalog_endpoint_test.go @@ -30,6 +30,9 @@ func TestCatalogRegister(t *testing.T) { Tags: []string{"master"}, Port: 8000, }, + Check: &structs.HealthCheck{ + ServiceID: "db", + }, } var out struct{} @@ -46,7 +49,7 @@ func TestCatalogRegister(t *testing.T) { }) } -func TestCatalogRegister_ACLDeny_Service(t *testing.T) { +func TestCatalogRegister_ACLDeny(t *testing.T) { dir1, s1 := testServerWithConfig(t, func(c *Config) { c.ACLDatacenter = "dc1" c.ACLMasterToken = "root" @@ -115,6 +118,28 @@ func TestCatalogRegister_ACLDeny_Service(t *testing.T) { if err == nil || !strings.Contains(err.Error(), permissionDenied) { t.Fatalf("err: %v", err) } + + // Register a db service using the root token. + argR.Service.Service = "db" + argR.Service.ID = "my-id" + argR.Token = "root" + err = msgpackrpc.CallWithCodec(codec, "Catalog.Register", &argR, &outR) + if err != nil { + t.Fatalf("err: %v", err) + } + + // Prove that we are properly looking up the node services and passing + // that to the ACL helper. We can vet the helper independently in its + // own unit test after this. This is trying to register over the db + // service we created above, which is a check that depends on looking + // at the existing registration data with that service ID. + argR.Service.Service = "foo" + argR.Service.ID = "my-id" + argR.Token = id + err = msgpackrpc.CallWithCodec(codec, "Catalog.Register", &argR, &outR) + if err == nil || !strings.Contains(err.Error(), permissionDenied) { + t.Fatalf("err: %v", err) + } } func TestCatalogRegister_ForwardLeader(t *testing.T) { diff --git a/consul/state/state_store.go b/consul/state/state_store.go index a2c94b7df6..68c786f3a9 100644 --- a/consul/state/state_store.go +++ b/consul/state/state_store.go @@ -438,6 +438,12 @@ func (s *StateStore) ensureRegistrationTxn(tx *memdb.Txn, idx uint64, watches *D } } + // TODO (slackpad) In Consul 0.8 ban checks that don't have the same + // node as the top-level registration. This is just weird to be able to + // update unrelated nodes' checks from in here. In 0.7.2 we banned this + // up in the ACL check since that's guarded behind an opt-in flag until + // Consul 0.8. + // Add the checks, if any. if req.Check != nil { if err := s.ensureCheckTxn(tx, idx, watches, req.Check); err != nil { diff --git a/consul/structs/structs.go b/consul/structs/structs.go index 9c28077c38..30b0be378c 100644 --- a/consul/structs/structs.go +++ b/consul/structs/structs.go @@ -183,6 +183,25 @@ func (r *RegisterRequest) RequestDatacenter() string { return r.Datacenter } +// ChangesNode returns true if the given register request changes the given +// node, which can be nil. This only looks for changes to the node record itself, +// not any of the health checks. +func (r *RegisterRequest) ChangesNode(node *Node) bool { + // This means it's creating the node. + if node == nil { + return true + } + + // Check if any of the node-level fields are being changed. + if r.Node != node.Node || + r.Address != node.Address || + !reflect.DeepEqual(r.TaggedAddresses, node.TaggedAddresses) { + return true + } + + return false +} + // DeregisterRequest is used for the Catalog.Deregister endpoint // to deregister a node as providing a service. If no service is // provided the entire node is deregistered. diff --git a/consul/structs/structs_test.go b/consul/structs/structs_test.go index 11c5b2aea3..efceeace3f 100644 --- a/consul/structs/structs_test.go +++ b/consul/structs/structs_test.go @@ -105,6 +105,44 @@ func TestStructs_ACL_IsSame(t *testing.T) { check(func() { other.Rules = "" }, func() { other.Rules = "service \"\" { policy = \"read\" }" }) } +func TestStructs_RegisterRequest_ChangesNode(t *testing.T) { + req := &RegisterRequest{ + Node: "test", + Address: "127.0.0.1", + TaggedAddresses: make(map[string]string), + } + + node := &Node{ + Node: "test", + Address: "127.0.0.1", + TaggedAddresses: make(map[string]string), + } + + check := func(twiddle, restore func()) { + if req.ChangesNode(node) { + t.Fatalf("should not change") + } + + twiddle() + if !req.ChangesNode(node) { + t.Fatalf("should change") + } + + restore() + if req.ChangesNode(node) { + t.Fatalf("should not change") + } + } + + check(func() { req.Node = "nope" }, func() { req.Node = "test" }) + check(func() { req.Address = "127.0.0.2" }, func() { req.Address = "127.0.0.1" }) + check(func() { req.TaggedAddresses["wan"] = "nope" }, func() { delete(req.TaggedAddresses, "wan") }) + + if !req.ChangesNode(nil) { + t.Fatalf("should change") + } +} + // testServiceNode gives a fully filled out ServiceNode instance. func testServiceNode() *ServiceNode { return &ServiceNode{