Skip to content

Commit

Permalink
Allow service deregistration with node write permission
Browse files Browse the repository at this point in the history 
With ACLs enabled if an agent is wiped and restarted without a leave
it can no longer deregister the services it had previously registered
because it no longer has the tokens the services were registered with.
To remedy that we allow service deregistration from tokens with node
write permission.
  • Loading branch information
Aestek authored and Thibault Gilles committed Jun 18, 2019
1 parent 1a0a76f commit d5bae88
Show file tree
Hide file tree
Showing 3 changed files with 214 additions and 77 deletions.
7 changes: 7 additions & 0 deletions agent/consul/acl.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1267,6 +1267,13 @@ func vetDeregisterWithACL(rule acl.Authorizer, subj *structs.DeregisterRequest,
// We don't apply sentinel in this path, since at this time sentinel
// only applies to create and update operations.

// Allow service deregistration if the token has write permission on the agent.
// This accounts for cases where the agent no longer has a token with write permission
// on the service to deregister it.
if rule.NodeWrite(subj.Node, nil) {
return nil
}

// This order must match the code in applyRegister() in fsm.go since it
// also evaluates things in this order, and will ignore fields based on
// this precedence. This lets us also ignore them from an ACL perspective.
Expand Down
278 changes: 204 additions & 74 deletions agent/consul/acl_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2772,92 +2772,222 @@ func TestACL_vetDeregisterWithACL(t *testing.T) {
node "node" {
policy = "write"
}
service "service" {
policy = "write"
}
`, acl.SyntaxLegacy, nil)
if err != nil {
t.Fatalf("err %v", err)
}
perms, err := acl.NewPolicyAuthorizer(acl.DenyAll(), []*acl.Policy{policy}, nil)
nodePerms, err := acl.NewPolicyAuthorizer(acl.DenyAll(), []*acl.Policy{policy}, nil)
if err != nil {
t.Fatalf("err: %v", err)
}

// With that policy, the update should now be blocked for node reasons.
err = vetDeregisterWithACL(perms, args, nil, nil)
if !acl.IsErrPermissionDenied(err) {
t.Fatalf("bad: %v", err)
}

// Now use a permitted node name.
args.Node = "node"
if err := vetDeregisterWithACL(perms, args, nil, nil); err != nil {
t.Fatalf("err: %v", err)
}

// Try an unknown check.
args.CheckID = "check-id"
err = vetDeregisterWithACL(perms, args, nil, nil)
if err == nil || !strings.Contains(err.Error(), "Unknown check") {
t.Fatalf("bad: %v", err)
}

// Now pass in a check that should be blocked.
nc := &structs.HealthCheck{
Node: "node",
CheckID: "check-id",
ServiceID: "service-id",
ServiceName: "nope",
}
err = vetDeregisterWithACL(perms, args, nil, nc)
if !acl.IsErrPermissionDenied(err) {
t.Fatalf("bad: %v", err)
}

// Change it to an allowed service, which should go through.
nc.ServiceName = "service"
if err := vetDeregisterWithACL(perms, args, nil, nc); err != nil {
t.Fatalf("err: %v", err)
policy, err = acl.NewPolicyFromSource("", 0, `
service "my-service" {
policy = "write"
}

// Switch to a node check that should be blocked.
args.Node = "nope"
nc.Node = "nope"
nc.ServiceID = ""
nc.ServiceName = ""
err = vetDeregisterWithACL(perms, args, nil, nc)
if !acl.IsErrPermissionDenied(err) {
t.Fatalf("bad: %v", err)
`, acl.SyntaxLegacy, nil)
if err != nil {
t.Fatalf("err %v", err)
}

// Switch to an allowed node check, which should go through.
args.Node = "node"
nc.Node = "node"
if err := vetDeregisterWithACL(perms, args, nil, nc); err != nil {
servicePerms, err := acl.NewPolicyAuthorizer(acl.DenyAll(), []*acl.Policy{policy}, nil)
if err != nil {
t.Fatalf("err: %v", err)
}

// Try an unknown service.
args.ServiceID = "service-id"
err = vetDeregisterWithACL(perms, args, nil, nil)
if err == nil || !strings.Contains(err.Error(), "Unknown service") {
t.Fatalf("bad: %v", err)
}

// Now pass in a service that should be blocked.
ns := &structs.NodeService{
ID: "service-id",
Service: "nope",
}
err = vetDeregisterWithACL(perms, args, ns, nil)
if !acl.IsErrPermissionDenied(err) {
t.Fatalf("bad: %v", err)
}

// Change it to an allowed service, which should go through.
ns.Service = "service"
if err := vetDeregisterWithACL(perms, args, ns, nil); err != nil {
t.Fatalf("err: %v", err)
for _, args := range []struct {
DeregisterRequest structs.DeregisterRequest
Service *structs.NodeService
Check *structs.HealthCheck
Perms *acl.PolicyAuthorizer
Expected bool
}{
{
DeregisterRequest: structs.DeregisterRequest{
Node: "nope",
},
Perms: nodePerms,
Expected: false,
},
{
DeregisterRequest: structs.DeregisterRequest{
Node: "nope",
},
Perms: servicePerms,
Expected: false,
},
{
DeregisterRequest: structs.DeregisterRequest{
Node: "nope",
ServiceID: "my-service-id",
},
Service: &structs.NodeService{
Service: "my-service",
},
Perms: nodePerms,
Expected: false,
},
{
DeregisterRequest: structs.DeregisterRequest{
Node: "nope",
ServiceID: "my-service-id",
},
Service: &structs.NodeService{
Service: "my-service",
},
Perms: servicePerms,
Expected: true,
},
{
DeregisterRequest: structs.DeregisterRequest{
Node: "nope",
ServiceID: "my-service-id",
CheckID: "my-check",
},
Service: &structs.NodeService{
Service: "my-service",
},
Check: &structs.HealthCheck{
CheckID: "my-check",
},
Perms: nodePerms,
Expected: false,
},
{
DeregisterRequest: structs.DeregisterRequest{
Node: "nope",
ServiceID: "my-service-id",
CheckID: "my-check",
},
Service: &structs.NodeService{
Service: "my-service",
},
Check: &structs.HealthCheck{
CheckID: "my-check",
},
Perms: servicePerms,
Expected: true,
},
{
DeregisterRequest: structs.DeregisterRequest{
Node: "nope",
CheckID: "my-check",
},
Check: &structs.HealthCheck{
CheckID: "my-check",
},
Perms: nodePerms,
Expected: false,
},
{
DeregisterRequest: structs.DeregisterRequest{
Node: "nope",
CheckID: "my-check",
},
Check: &structs.HealthCheck{
CheckID: "my-check",
},
Perms: servicePerms,
Expected: false,
},
{
DeregisterRequest: structs.DeregisterRequest{
Node: "node",
},
Perms: nodePerms,
Expected: true,
},
{
DeregisterRequest: structs.DeregisterRequest{
Node: "node",
},
Perms: servicePerms,
Expected: false,
},
{
DeregisterRequest: structs.DeregisterRequest{
Node: "node",
ServiceID: "my-service-id",
},
Service: &structs.NodeService{
Service: "my-service",
},
Perms: nodePerms,
Expected: true,
},
{
DeregisterRequest: structs.DeregisterRequest{
Node: "node",
ServiceID: "my-service-id",
},
Service: &structs.NodeService{
Service: "my-service",
},
Perms: servicePerms,
Expected: true,
},
{
DeregisterRequest: structs.DeregisterRequest{
Node: "node",
ServiceID: "my-service-id",
CheckID: "my-check",
},
Service: &structs.NodeService{
Service: "my-service",
},
Check: &structs.HealthCheck{
CheckID: "my-check",
},
Perms: nodePerms,
Expected: true,
},
{
DeregisterRequest: structs.DeregisterRequest{
Node: "node",
ServiceID: "my-service-id",
CheckID: "my-check",
},
Service: &structs.NodeService{
Service: "my-service",
},
Check: &structs.HealthCheck{
CheckID: "my-check",
},
Perms: servicePerms,
Expected: true,
},
{
DeregisterRequest: structs.DeregisterRequest{
Node: "node",
CheckID: "my-check",
},
Check: &structs.HealthCheck{
CheckID: "my-check",
},
Perms: nodePerms,
Expected: true,
},
{
DeregisterRequest: structs.DeregisterRequest{
Node: "node",
CheckID: "my-check",
},
Check: &structs.HealthCheck{
CheckID: "my-check",
},
Perms: servicePerms,
Expected: false,
},
} {
err = vetDeregisterWithACL(args.Perms, &args.DeregisterRequest, args.Service, args.Check)
if !args.Expected {
if err == nil {
t.Errorf("expected error with %+v", args.DeregisterRequest)
}
if !acl.IsErrPermissionDenied(err) {
t.Errorf("expected permission denied error with %+v, instead got %+v", args.DeregisterRequest, err)
}
} else if err != nil {
t.Errorf("expected no error with %+v", args.DeregisterRequest)
}
}
}
6 changes: 3 additions & 3 deletions agent/consul/catalog_endpoint_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ import (
"github.com/hashicorp/consul/testrpc"
"github.com/hashicorp/consul/testutil/retry"
"github.com/hashicorp/consul/types"
"github.com/hashicorp/net-rpc-msgpackrpc"
msgpackrpc "github.com/hashicorp/net-rpc-msgpackrpc"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
)
Expand Down Expand Up @@ -727,7 +727,7 @@ service "service" {
err = msgpackrpc.CallWithCodec(codec, "Catalog.Deregister",
&structs.DeregisterRequest{
Datacenter: "dc1",
Node: "node",
Node: "nope",
ServiceID: "nope",
WriteRequest: structs.WriteRequest{
Token: id,
Expand All @@ -738,7 +738,7 @@ service "service" {
err = msgpackrpc.CallWithCodec(codec, "Catalog.Deregister",
&structs.DeregisterRequest{
Datacenter: "dc1",
Node: "node",
Node: "nope",
CheckID: "nope",
WriteRequest: structs.WriteRequest{
Token: id,
Expand Down

0 comments on commit d5bae88

Please sign in to comment.