From 29b45121204fd2d4219580598f1e59c2b6b51fb8 Mon Sep 17 00:00:00 2001
From: Matt Keeler <mkeeler@users.noreply.github.com>
Date: Thu, 10 Jan 2019 09:22:51 -0500
Subject: [PATCH] acl: Prevent tokens from deleting themselves (#5210)

Fixes #4897

Also apparently token deletion could segfault in secondary DCs when attempting to delete non-existant tokens. For that reason both checks are wrapped within the non-nil check.
---
 agent/consul/acl_endpoint.go      | 12 ++++--
 agent/consul/acl_endpoint_test.go | 71 +++++++++++++++++++++++++++++++
 agent/consul/helper_test.go       |  9 ++++
 3 files changed, 89 insertions(+), 3 deletions(-)

diff --git a/agent/consul/acl_endpoint.go b/agent/consul/acl_endpoint.go
index 6ebbf89f0..798b864a3 100644
--- a/agent/consul/acl_endpoint.go
+++ b/agent/consul/acl_endpoint.go
@@ -482,9 +482,15 @@ func (a *ACL) TokenDelete(args *structs.ACLTokenDeleteRequest, reply *string) er
 		return err
 	}
 
-	if !a.srv.InACLDatacenter() && !token.Local {
-		args.Datacenter = a.srv.config.ACLDatacenter
-		return a.srv.forwardDC("ACL.TokenDelete", a.srv.config.ACLDatacenter, args, reply)
+	if token != nil {
+		if args.Token == token.SecretID {
+			return fmt.Errorf("Deletion of the request's authorization token is not permitted")
+		}
+
+		if !a.srv.InACLDatacenter() && !token.Local {
+			args.Datacenter = a.srv.config.ACLDatacenter
+			return a.srv.forwardDC("ACL.TokenDelete", a.srv.config.ACLDatacenter, args, reply)
+		}
 	}
 
 	req := &structs.ACLTokenBatchDeleteRequest{
diff --git a/agent/consul/acl_endpoint_test.go b/agent/consul/acl_endpoint_test.go
index 8185bad7d..f6664183a 100644
--- a/agent/consul/acl_endpoint_test.go
+++ b/agent/consul/acl_endpoint_test.go
@@ -873,10 +873,31 @@ func TestACLEndpoint_TokenDelete(t *testing.T) {
 
 	testrpc.WaitForLeader(t, s1.RPC, "dc1")
 
+	dir2, s2 := testServerWithConfig(t, func(c *Config) {
+		c.ACLDatacenter = "dc1"
+		c.ACLsEnabled = true
+		c.Datacenter = "dc2"
+		// token replication is required to test deleting non-local tokens in secondary dc
+		c.ACLTokenReplication = true
+	})
+	defer os.RemoveAll(dir2)
+	defer s2.Shutdown()
+	codec2 := rpcClient(t, s2)
+	defer codec2.Close()
+
+	s2.tokens.UpdateACLReplicationToken("root")
+
+	testrpc.WaitForLeader(t, s1.RPC, "dc1")
+	testrpc.WaitForLeader(t, s2.RPC, "dc2")
+
+	// Try to join
+	joinWAN(t, s2, s1)
+
 	existingToken, err := upsertTestToken(codec, "root", "dc1")
 	assert.NoError(err)
 
 	acl := ACL{srv: s1}
+	acl2 := ACL{srv: s2}
 
 	// deletes a token
 	{
@@ -897,6 +918,32 @@ func TestACLEndpoint_TokenDelete(t *testing.T) {
 		assert.NoError(err)
 	}
 
+	// can't delete itself
+	{
+		readReq := structs.ACLTokenGetRequest{
+			Datacenter:   "dc1",
+			TokenID:      "root",
+			TokenIDType:  structs.ACLTokenSecret,
+			QueryOptions: structs.QueryOptions{Token: "root"},
+		}
+
+		var out structs.ACLTokenResponse
+
+		err := msgpackrpc.CallWithCodec(codec, "ACL.TokenRead", &readReq, &out)
+
+		assert.NoError(err)
+
+		req := structs.ACLTokenDeleteRequest{
+			Datacenter:   "dc1",
+			TokenID:      out.Token.AccessorID,
+			WriteRequest: structs.WriteRequest{Token: "root"},
+		}
+
+		var resp string
+		err = acl.TokenDelete(&req, &resp)
+		assert.EqualError(err, "Deletion of the request's authorization token is not permitted")
+	}
+
 	// errors when token doesn't exist
 	{
 		fakeID, err := uuid.GenerateUUID()
@@ -918,6 +965,30 @@ func TestACLEndpoint_TokenDelete(t *testing.T) {
 		assert.Nil(tokenResp.Token)
 		assert.NoError(err)
 	}
+
+	// don't segfault when attempting to delete non existant token in secondary dc
+	{
+		fakeID, err := uuid.GenerateUUID()
+		assert.NoError(err)
+
+		req := structs.ACLTokenDeleteRequest{
+			Datacenter:   "dc2",
+			TokenID:      fakeID,
+			WriteRequest: structs.WriteRequest{Token: "root"},
+		}
+
+		var resp string
+
+		waitForNewACLs(t, s2)
+
+		err = acl2.TokenDelete(&req, &resp)
+		assert.NoError(err)
+
+		// token should be nil
+		tokenResp, err := retrieveTestToken(codec2, "root", "dc1", existingToken.AccessorID)
+		assert.Nil(tokenResp.Token)
+		assert.NoError(err)
+	}
 }
 func TestACLEndpoint_TokenDelete_anon(t *testing.T) {
 	t.Parallel()
diff --git a/agent/consul/helper_test.go b/agent/consul/helper_test.go
index d19f2cfae..a488ee0ae 100644
--- a/agent/consul/helper_test.go
+++ b/agent/consul/helper_test.go
@@ -9,6 +9,7 @@ import (
 	"github.com/hashicorp/consul/testutil/retry"
 	"github.com/hashicorp/raft"
 	"github.com/hashicorp/serf/serf"
+	"github.com/stretchr/testify/require"
 )
 
 func waitForLeader(servers ...*Server) error {
@@ -152,6 +153,14 @@ func joinWAN(t *testing.T, member, leader *Server) {
 	}
 }
 
+func waitForNewACLs(t *testing.T, server *Server) {
+	retry.Run(t, func(r *retry.R) {
+		require.False(r, server.UseLegacyACLs(), "Server cannot use new ACLs")
+	})
+
+	require.False(t, server.UseLegacyACLs(), "Server cannot use new ACLs")
+}
+
 func seeEachOther(a, b []serf.Member, addra, addrb string) bool {
 	return serfMembersContains(a, addrb) && serfMembersContains(b, addra)
 }