Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow service deregistration with node write permission #5217

Merged
merged 2 commits into from
Jun 27, 2019
Merged

Allow service deregistration with node write permission #5217

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ff4150d
Allow service deregistration with node write permission
Jan 11, 2019
95457a3
improve code and update comment.
hanshasselberg Jun 27, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions agent/consul/acl.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1592,6 +1592,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 for the node.
// 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
297 changes: 223 additions & 74 deletions agent/consul/acl_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3268,93 +3268,242 @@ 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
Name string
}{
{
DeregisterRequest: structs.DeregisterRequest{
Node: "nope",
},
Perms: nodePerms,
Expected: false,
Name: "no right on node",
},
{
DeregisterRequest: structs.DeregisterRequest{
Node: "nope",
},
Perms: servicePerms,
Expected: false,
Name: "right on service but node dergister request",
},
{
DeregisterRequest: structs.DeregisterRequest{
Node: "nope",
ServiceID: "my-service-id",
},
Service: &structs.NodeService{
Service: "my-service",
},
Perms: nodePerms,
Expected: false,
Name: "no rights on node nor service",
},
{
DeregisterRequest: structs.DeregisterRequest{
Node: "nope",
ServiceID: "my-service-id",
},
Service: &structs.NodeService{
Service: "my-service",
},
Perms: servicePerms,
Expected: true,
Name: "no rights on node but rights on service",
},
{
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,
Name: "no right on node nor service for check",
},
{
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,
Name: "no rights on node but rights on service for check",
},
{
DeregisterRequest: structs.DeregisterRequest{
Node: "nope",
CheckID: "my-check",
},
Check: &structs.HealthCheck{
CheckID: "my-check",
},
Perms: nodePerms,
Expected: false,
Name: "no right on node for node check",
},
{
DeregisterRequest: structs.DeregisterRequest{
Node: "nope",
CheckID: "my-check",
},
Check: &structs.HealthCheck{
CheckID: "my-check",
},
Perms: servicePerms,
Expected: false,
Name: "rights on service but no right on node for node check",
},
{
DeregisterRequest: structs.DeregisterRequest{
Node: "node",
},
Perms: nodePerms,
Expected: true,
Name: "rights on node for node",
},
{
DeregisterRequest: structs.DeregisterRequest{
Node: "node",
},
Perms: servicePerms,
Expected: false,
Name: "rights on service but not on node for node",
},
{
DeregisterRequest: structs.DeregisterRequest{
Node: "node",
ServiceID: "my-service-id",
},
Service: &structs.NodeService{
Service: "my-service",
},
Perms: nodePerms,
Expected: true,
Name: "rights on node for service",
},
{
DeregisterRequest: structs.DeregisterRequest{
Node: "node",
ServiceID: "my-service-id",
},
Service: &structs.NodeService{
Service: "my-service",
},
Perms: servicePerms,
Expected: true,
Name: "rights on service for service",
},
{
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,
Name: "right on node for check",
},
{
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,
Name: "rights on service for check",
},
{
DeregisterRequest: structs.DeregisterRequest{
Node: "node",
CheckID: "my-check",
},
Check: &structs.HealthCheck{
CheckID: "my-check",
},
Perms: nodePerms,
Expected: true,
Name: "rights on node for check",
},
{
DeregisterRequest: structs.DeregisterRequest{
Node: "node",
CheckID: "my-check",
},
Check: &structs.HealthCheck{
CheckID: "my-check",
},
Perms: servicePerms,
Expected: false,
Name: "rights on service for node check",
},
} {
t.Run(args.Name, func(t *testing.T) {
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)
}
})
}
}

Expand Down
6 changes: 3 additions & 3 deletions agent/consul/catalog_endpoint_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ import (
"github.com/hashicorp/consul/sdk/testutil/retry"
"github.com/hashicorp/consul/testrpc"
"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",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why did this file change?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With the previous behavior this call (write access on node but not on service) would fail. It now succeeds if the token has write access on the node, which is the case here, so I changed the test to target a node the token does not has write access to.

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