Tokens converted from legacy ACLs get their Hash computed (#8047) (#8054)

This allows new style token replication to work for legacy tokens as well when they change. Fixes #5606
2025-02-03 01:14:23 +00:00 · 2020-06-08 23:36:55 +02:00 · 2020-06-08 23:36:55 +02:00 · cfc95732f3
commit cfc95732f3
parent b4f33b52a2
6 changed files with 216 additions and 238 deletions
--- a/agent/consul/fsm/snapshot_oss.go
+++ b/agent/consul/fsm/snapshot_oss.go
@ -658,6 +658,9 @@ func restoreToken(header *snapshotHeader, restore *state.Restore, decoder *codec
 		structs.SanitizeLegacyACLToken(&req)
 	}

+	// only set if unset - mitigates a bug where converted legacy tokens could end up without a hash
+	req.SetHash(false)
+
 	return restore.ACLToken(&req)
 }

--- a/agent/consul/fsm/snapshot_oss_test.go
+++ b/agent/consul/fsm/snapshot_oss_test.go
@ -2,7 +2,6 @@ package fsm

 import (
 	"bytes"
-	"reflect"
 	"testing"
 	"time"

@ -14,21 +13,17 @@ import (
 	"github.com/hashicorp/consul/api"
 	"github.com/hashicorp/consul/lib/stringslice"
 	"github.com/hashicorp/consul/sdk/testutil"
+	"github.com/hashicorp/go-msgpack/codec"
 	"github.com/hashicorp/go-raftchunking"
-	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 )

 func TestFSM_SnapshotRestore_OSS(t *testing.T) {
 	t.Parallel()

-	assert := assert.New(t)
-	require := require.New(t)
 	logger := testutil.Logger(t)
 	fsm, err := New(nil, logger)
-	if err != nil {
-		t.Fatalf("err: %v", err)
-	}
+	require.NoError(t, err)

 	// Add some state
 	node1 := &structs.Node{
@ -49,8 +44,8 @@ func TestFSM_SnapshotRestore_OSS(t *testing.T) {
 			"testMeta": "testing123",
 		},
 	}
-	require.NoError(fsm.state.EnsureNode(1, node1))
-	require.NoError(fsm.state.EnsureNode(2, node2))
+	require.NoError(t, fsm.state.EnsureNode(1, node1))
+	require.NoError(t, fsm.state.EnsureNode(2, node2))

 	// Add a service instance with Connect config.
 	connectConf := structs.ServiceConnect{
@ -89,7 +84,7 @@ func TestFSM_SnapshotRestore_OSS(t *testing.T) {
 		Syntax:      acl.SyntaxCurrent,
 	}
 	policy.SetHash(true)
-	require.NoError(fsm.state.ACLPolicySet(1, policy))
+	require.NoError(t, fsm.state.ACLPolicySet(1, policy))

 	role := &structs.ACLRole{
 		ID:          "86dedd19-8fae-4594-8294-4e6948a81f9a",
@ -102,7 +97,7 @@ func TestFSM_SnapshotRestore_OSS(t *testing.T) {
 		},
 	}
 	role.SetHash(true)
-	require.NoError(fsm.state.ACLRoleSet(1, role))
+	require.NoError(t, fsm.state.ACLRoleSet(1, role))

 	token := &structs.ACLToken{
 		AccessorID:  "30fca056-9fbb-4455-b94a-bf0e2bc575d6",
@ -118,7 +113,7 @@ func TestFSM_SnapshotRestore_OSS(t *testing.T) {
 		// DEPRECATED (ACL-Legacy-Compat) - This is used so that the bootstrap token is still visible via the v1 acl APIs
 		Type: structs.ACLTokenTypeManagement,
 	}
-	require.NoError(fsm.state.ACLBootstrap(10, 0, token, false))
+	require.NoError(t, fsm.state.ACLBootstrap(10, 0, token, false))

 	method := &structs.ACLAuthMethod{
 		Name:        "some-method",
@ -128,7 +123,7 @@ func TestFSM_SnapshotRestore_OSS(t *testing.T) {
 			"SessionID": "952ebfa8-2a42-46f0-bcd3-fd98a842000e",
 		},
 	}
-	require.NoError(fsm.state.ACLAuthMethodSet(1, method))
+	require.NoError(t, fsm.state.ACLAuthMethodSet(1, method))

 	bindingRule := &structs.ACLBindingRule{
 		ID:          "85184c52-5997-4a84-9817-5945f2632a17",
@ -138,7 +133,7 @@ func TestFSM_SnapshotRestore_OSS(t *testing.T) {
 		BindType:    structs.BindingRuleBindTypeService,
 		BindName:    "${serviceaccount.name}",
 	}
-	require.NoError(fsm.state.ACLBindingRuleSet(1, bindingRule))
+	require.NoError(t, fsm.state.ACLBindingRuleSet(1, bindingRule))

 	fsm.state.KVSSet(11, &structs.DirEntry{
 		Key:   "/remove",
@ -146,12 +141,8 @@ func TestFSM_SnapshotRestore_OSS(t *testing.T) {
 	})
 	fsm.state.KVSDelete(12, "/remove", nil)
 	idx, _, err := fsm.state.KVSList(nil, "/remove", nil)
-	if err != nil {
-		t.Fatalf("err: %s", err)
-	}
-	if idx != 12 {
-		t.Fatalf("bad index: %d", idx)
-	}
+	require.NoError(t, err)
+	require.EqualValues(t, 12, idx, "bad index")

 	updates := structs.Coordinates{
 		&structs.Coordinate{
@ -163,9 +154,7 @@ func TestFSM_SnapshotRestore_OSS(t *testing.T) {
 			Coord: generateRandomCoordinate(),
 		},
 	}
-	if err := fsm.state.CoordinateBatchUpdate(13, updates); err != nil {
-		t.Fatalf("err: %s", err)
-	}
+	require.NoError(t, fsm.state.CoordinateBatchUpdate(13, updates))

 	query := structs.PreparedQuery{
 		ID: generateUUID(),
@ -177,18 +166,14 @@ func TestFSM_SnapshotRestore_OSS(t *testing.T) {
 			ModifyIndex: 14,
 		},
 	}
-	if err := fsm.state.PreparedQuerySet(14, &query); err != nil {
-		t.Fatalf("err: %s", err)
-	}
+	require.NoError(t, fsm.state.PreparedQuerySet(14, &query))

 	autopilotConf := &autopilot.Config{
 		CleanupDeadServers:   true,
 		LastContactThreshold: 100 * time.Millisecond,
 		MaxTrailingLogs:      222,
 	}
-	if err := fsm.state.AutopilotSetConfig(15, autopilotConf); err != nil {
-		t.Fatalf("err: %s", err)
-	}
+	require.NoError(t, fsm.state.AutopilotSetConfig(15, autopilotConf))

 	// Intentions
 	ixn := structs.TestIntention(t)
@ -197,7 +182,7 @@ func TestFSM_SnapshotRestore_OSS(t *testing.T) {
 		CreateIndex: 14,
 		ModifyIndex: 14,
 	}
-	require.NoError(fsm.state.IntentionSet(14, ixn))
+	require.NoError(t, fsm.state.IntentionSet(14, ixn))

 	// CA Roots
 	roots := []*structs.CARoot{
@ -208,16 +193,16 @@ func TestFSM_SnapshotRestore_OSS(t *testing.T) {
 		r.Active = false
 	}
 	ok, err := fsm.state.CARootSetCAS(15, 0, roots)
-	require.NoError(err)
-	assert.True(ok)
+	require.NoError(t, err)
+	require.True(t, ok)

 	ok, err = fsm.state.CASetProviderState(16, &structs.CAConsulProviderState{
 		ID:         "asdf",
 		PrivateKey: "foo",
 		RootCert:   "bar",
 	})
-	require.NoError(err)
-	assert.True(ok)
+	require.NoError(t, err)
+	require.True(t, ok)

 	// CA Config
 	caConfig := &structs.CAConfiguration{
@ -229,7 +214,7 @@ func TestFSM_SnapshotRestore_OSS(t *testing.T) {
 		},
 	}
 	err = fsm.state.CASetConfig(17, caConfig)
-	require.NoError(err)
+	require.NoError(t, err)

 	// Config entries
 	serviceConfig := &structs.ServiceConfigEntry{
@ -241,8 +226,8 @@ func TestFSM_SnapshotRestore_OSS(t *testing.T) {
 		Kind: structs.ProxyDefaults,
 		Name: "global",
 	}
-	require.NoError(fsm.state.EnsureConfigEntry(18, serviceConfig, structs.DefaultEnterpriseMeta()))
-	require.NoError(fsm.state.EnsureConfigEntry(19, proxyConfig, structs.DefaultEnterpriseMeta()))
+	require.NoError(t, fsm.state.EnsureConfigEntry(18, serviceConfig, structs.DefaultEnterpriseMeta()))
+	require.NoError(t, fsm.state.EnsureConfigEntry(19, proxyConfig, structs.DefaultEnterpriseMeta()))

 	ingress := &structs.IngressGatewayConfigEntry{
 		Kind: structs.IngressGateway,
@ -259,9 +244,9 @@ func TestFSM_SnapshotRestore_OSS(t *testing.T) {
 			},
 		},
 	}
-	require.NoError(fsm.state.EnsureConfigEntry(20, ingress, structs.DefaultEnterpriseMeta()))
+	require.NoError(t, fsm.state.EnsureConfigEntry(20, ingress, structs.DefaultEnterpriseMeta()))
 	_, gatewayServices, err := fsm.state.GatewayServices(nil, "ingress", structs.DefaultEnterpriseMeta())
-	require.NoError(err)
+	require.NoError(t, err)

 	// Raft Chunking
 	chunkState := &raftchunking.State{
@ -292,7 +277,7 @@ func TestFSM_SnapshotRestore_OSS(t *testing.T) {
 		},
 	}
 	err = fsm.chunker.RestoreState(chunkState)
-	require.NoError(err)
+	require.NoError(t, err)

 	// Federation states
 	fedState1 := &structs.FederationState{
@ -395,267 +380,269 @@ func TestFSM_SnapshotRestore_OSS(t *testing.T) {
 		},
 		UpdatedAt: time.Now().UTC(),
 	}
-	require.NoError(fsm.state.FederationStateSet(21, fedState1))
-	require.NoError(fsm.state.FederationStateSet(22, fedState2))
+	require.NoError(t, fsm.state.FederationStateSet(21, fedState1))
+	require.NoError(t, fsm.state.FederationStateSet(22, fedState2))

 	// Snapshot
 	snap, err := fsm.Snapshot()
-	if err != nil {
-		t.Fatalf("err: %v", err)
-	}
+	require.NoError(t, err)
 	defer snap.Release()

 	// Persist
 	buf := bytes.NewBuffer(nil)
 	sink := &MockSink{buf, false}
-	if err := snap.Persist(sink); err != nil {
-		t.Fatalf("err: %v", err)
+	require.NoError(t, snap.Persist(sink))
+
+	// create an encoder to handle some custom persisted data
+	// this is mainly to inject data that would no longer ever
+	// be persisted but that we still need to be able to restore
+	encoder := codec.NewEncoder(sink, structs.MsgpackHandle)
+
+	// Persist a legacy ACL token - this is not done in newer code
+	// but we want to ensure that restoring legacy tokens works as
+	// expected so we must inject one here manually
+	_, err = sink.Write([]byte{byte(structs.ACLRequestType)})
+	require.NoError(t, err)
+
+	acl := structs.ACL{
+		ID:        "1057354f-69ef-4487-94ab-aead3c755445",
+		Name:      "test-legacy",
+		Type:      "client",
+		Rules:     `operator = "read"`,
+		RaftIndex: structs.RaftIndex{CreateIndex: 1, ModifyIndex: 2},
 	}
+	require.NoError(t, encoder.Encode(&acl))
+
+	// Persist a ACLToken without a Hash - the state store will
+	// now tack these on but we want to ensure we can restore
+	// tokens without a hash and have the hash be set.
+	token2 := &structs.ACLToken{
+		AccessorID:  "4464e4c2-1c55-4c37-978a-66cb3abe6587",
+		SecretID:    "fc8708dc-c5ae-4bb2-a9af-a1ca456548fb",
+		Description: "Test No Hash",
+		CreateTime:  time.Now(),
+		Local:       false,
+		Rules:       `operator = "read"`,
+		RaftIndex:   structs.RaftIndex{CreateIndex: 1, ModifyIndex: 2},
+	}
+
+	_, err = sink.Write([]byte{byte(structs.ACLTokenSetRequestType)})
+	require.NoError(t, err)
+	require.NoError(t, encoder.Encode(&token2))

 	// Try to restore on a new FSM
 	fsm2, err := New(nil, logger)
-	if err != nil {
-		t.Fatalf("err: %v", err)
-	}
+	require.NoError(t, err)

 	// Do a restore
-	if err := fsm2.Restore(sink); err != nil {
-		t.Fatalf("err: %v", err)
-	}
+	require.NoError(t, fsm2.Restore(sink))

 	// Verify the contents
 	_, nodes, err := fsm2.state.Nodes(nil)
-	if err != nil {
-		t.Fatalf("err: %s", err)
-	}
-	if len(nodes) != 2 {
-		t.Fatalf("bad: %v", nodes)
-	}
-	if nodes[0].ID != node2.ID ||
-		nodes[0].Node != "baz" ||
-		nodes[0].Datacenter != "dc1" ||
-		nodes[0].Address != "127.0.0.2" ||
-		len(nodes[0].Meta) != 1 ||
-		nodes[0].Meta["testMeta"] != "testing123" ||
-		len(nodes[0].TaggedAddresses) != 1 ||
-		nodes[0].TaggedAddresses["hello"] != "1.2.3.4" {
-		t.Fatalf("bad: %v", nodes[0])
-	}
-	if nodes[1].ID != node1.ID ||
-		nodes[1].Node != "foo" ||
-		nodes[1].Datacenter != "dc1" ||
-		nodes[1].Address != "127.0.0.1" ||
-		len(nodes[1].TaggedAddresses) != 0 {
-		t.Fatalf("bad: %v", nodes[1])
-	}
+	require.NoError(t, err)
+	require.Len(t, nodes, 2, "incorect number of nodes: %v", nodes)
+
+	// validate the first node. Note that this test relies on stable
+	// iteration through the memdb index and the fact that node2 has
+	// a name of "baz" so it should be indexed before node1 with a
+	// name of "foo". If memdb our our indexing changes this is likely
+	// to break.
+	require.Equal(t, node2.ID, nodes[0].ID)
+	require.Equal(t, "baz", nodes[0].Node)
+	require.Equal(t, "dc1", nodes[0].Datacenter)
+	require.Equal(t, "127.0.0.2", nodes[0].Address)
+	require.Len(t, nodes[0].Meta, 1)
+	require.Equal(t, "testing123", nodes[0].Meta["testMeta"])
+	require.Len(t, nodes[0].TaggedAddresses, 1)
+	require.Equal(t, "1.2.3.4", nodes[0].TaggedAddresses["hello"])
+
+	require.Equal(t, node1.ID, nodes[1].ID)
+	require.Equal(t, "foo", nodes[1].Node)
+	require.Equal(t, "dc1", nodes[1].Datacenter)
+	require.Equal(t, "127.0.0.1", nodes[1].Address)
+	require.Empty(t, nodes[1].TaggedAddresses)

 	_, fooSrv, err := fsm2.state.NodeServices(nil, "foo", nil)
-	if err != nil {
-		t.Fatalf("err: %s", err)
-	}
-	if len(fooSrv.Services) != 2 {
-		t.Fatalf("Bad: %v", fooSrv)
-	}
-	if !stringslice.Contains(fooSrv.Services["db"].Tags, "primary") {
-		t.Fatalf("Bad: %v", fooSrv)
-	}
-	if fooSrv.Services["db"].Port != 5000 {
-		t.Fatalf("Bad: %v", fooSrv)
-	}
+	require.NoError(t, err)
+	require.Len(t, fooSrv.Services, 2)
+	require.Contains(t, fooSrv.Services["db"].Tags, "primary")
+	require.True(t, stringslice.Contains(fooSrv.Services["db"].Tags, "primary"))
+	require.Equal(t, 5000, fooSrv.Services["db"].Port)
 	connectSrv := fooSrv.Services["web"]
-	if !reflect.DeepEqual(connectSrv.Connect, connectConf) {
-		t.Fatalf("got: %v, want: %v", connectSrv.Connect, connectConf)
-	}
+	require.Equal(t, connectConf, connectSrv.Connect)

 	_, checks, err := fsm2.state.NodeChecks(nil, "foo", nil)
-	if err != nil {
-		t.Fatalf("err: %s", err)
-	}
-	if len(checks) != 1 {
-		t.Fatalf("Bad: %v", checks)
-	}
+	require.NoError(t, err)
+	require.Len(t, checks, 1)

 	// Verify key is set
 	_, d, err := fsm2.state.KVSGet(nil, "/test", nil)
-	if err != nil {
-		t.Fatalf("err: %v", err)
-	}
-	if string(d.Value) != "foo" {
-		t.Fatalf("bad: %v", d)
-	}
+	require.NoError(t, err)
+	require.EqualValues(t, "foo", d.Value)

 	// Verify session is restored
 	idx, s, err := fsm2.state.SessionGet(nil, session.ID, nil)
-	if err != nil {
-		t.Fatalf("err: %v", err)
-	}
-	if s.Node != "foo" {
-		t.Fatalf("bad: %v", s)
-	}
-	if idx <= 1 {
-		t.Fatalf("bad index: %d", idx)
-	}
+	require.NoError(t, err)
+	require.Equal(t, "foo", s.Node)
+	require.EqualValues(t, 9, idx)

 	// Verify ACL Binding Rule is restored
 	_, bindingRule2, err := fsm2.state.ACLBindingRuleGetByID(nil, bindingRule.ID, nil)
-	require.NoError(err)
-	require.Equal(bindingRule, bindingRule2)
+	require.NoError(t, err)
+	require.Equal(t, bindingRule, bindingRule2)

 	// Verify ACL Auth Method is restored
 	_, method2, err := fsm2.state.ACLAuthMethodGetByName(nil, method.Name, nil)
-	require.NoError(err)
-	require.Equal(method, method2)
+	require.NoError(t, err)
+	require.Equal(t, method, method2)

 	// Verify ACL Token is restored
-	_, token2, err := fsm2.state.ACLTokenGetByAccessor(nil, token.AccessorID, nil)
-	require.NoError(err)
-	{
-		// time.Time is tricky to compare generically when it takes a ser/deserialization round trip.
-		require.True(token.CreateTime.Equal(token2.CreateTime))
-		token2.CreateTime = token.CreateTime
-	}
-	require.Equal(token, token2)
+	_, rtoken, err := fsm2.state.ACLTokenGetByAccessor(nil, token.AccessorID, nil)
+	require.NoError(t, err)
+	require.NotNil(t, rtoken)
+	// the state store function will add on the Hash if its empty
+	require.NotEmpty(t, rtoken.Hash)
+	token.CreateTime = token.CreateTime.Round(0)
+	rtoken.CreateTime = rtoken.CreateTime.Round(0)
+
+	// note that this can work because the state store will add the Hash to the token before
+	// storing. That token just happens to be a pointer to the one in this function so it
+	// adds the Hash to our local var.
+	require.Equal(t, token, rtoken)
+
+	// Verify legacy ACL is restored
+	_, rtoken, err = fsm2.state.ACLTokenGetBySecret(nil, acl.ID, nil)
+	require.NoError(t, err)
+	require.NotNil(t, rtoken)
+	require.NotEmpty(t, rtoken.Hash)
+
+	restoredACL, err := rtoken.Convert()
+	require.NoError(t, err)
+	require.Equal(t, &acl, restoredACL)
+
+	// Verify ACLToken without hash computes the Hash during restoration
+	_, rtoken, err = fsm2.state.ACLTokenGetByAccessor(nil, token2.AccessorID, nil)
+	require.NoError(t, err)
+	require.NotNil(t, rtoken)
+	require.NotEmpty(t, rtoken.Hash)
+	// nil the Hash so we can compare them
+	rtoken.Hash = nil
+	token2.CreateTime = token2.CreateTime.Round(0)
+	rtoken.CreateTime = rtoken.CreateTime.Round(0)
+	require.Equal(t, token2, rtoken)

 	// Verify the acl-token-bootstrap index was restored
 	canBootstrap, index, err := fsm2.state.CanBootstrapACLToken()
-	require.False(canBootstrap)
-	require.True(index > 0)
+	require.False(t, canBootstrap)
+	require.True(t, index > 0)

 	// Verify ACL Role is restored
 	_, role2, err := fsm2.state.ACLRoleGetByID(nil, role.ID, nil)
-	require.NoError(err)
-	require.Equal(role, role2)
+	require.NoError(t, err)
+	require.Equal(t, role, role2)

 	// Verify ACL Policy is restored
 	_, policy2, err := fsm2.state.ACLPolicyGetByID(nil, structs.ACLPolicyGlobalManagementID, nil)
-	require.NoError(err)
-	require.Equal(policy, policy2)
+	require.NoError(t, err)
+	require.Equal(t, policy, policy2)

 	// Verify tombstones are restored
 	func() {
 		snap := fsm2.state.Snapshot()
 		defer snap.Close()
 		stones, err := snap.Tombstones()
-		if err != nil {
-			t.Fatalf("err: %s", err)
-		}
+		require.NoError(t, err)
 		stone := stones.Next().(*state.Tombstone)
-		if stone == nil {
-			t.Fatalf("missing tombstone")
-		}
-		if stone.Key != "/remove" || stone.Index != 12 {
-			t.Fatalf("bad: %v", stone)
-		}
-		if stones.Next() != nil {
-			t.Fatalf("unexpected extra tombstones")
-		}
+		require.NotNil(t, stone)
+		require.Equal(t, "/remove", stone.Key)
+		require.Nil(t, stones.Next())
 	}()

 	// Verify coordinates are restored
 	_, coords, err := fsm2.state.Coordinates(nil)
-	if err != nil {
-		t.Fatalf("err: %s", err)
-	}
-	if !reflect.DeepEqual(coords, updates) {
-		t.Fatalf("bad: %#v", coords)
-	}
+	require.NoError(t, err)
+	require.Equal(t, updates, coords)

 	// Verify queries are restored.
 	_, queries, err := fsm2.state.PreparedQueryList(nil)
-	if err != nil {
-		t.Fatalf("err: %s", err)
-	}
-	if len(queries) != 1 {
-		t.Fatalf("bad: %#v", queries)
-	}
-	if !reflect.DeepEqual(queries[0], &query) {
-		t.Fatalf("bad: %#v", queries[0])
-	}
+	require.NoError(t, err)
+	require.Len(t, queries, 1)
+	require.Equal(t, &query, queries[0])

 	// Verify autopilot config is restored.
 	_, restoredConf, err := fsm2.state.AutopilotConfig()
-	if err != nil {
-		t.Fatalf("err: %s", err)
-	}
-	if !reflect.DeepEqual(restoredConf, autopilotConf) {
-		t.Fatalf("bad: %#v, %#v", restoredConf, autopilotConf)
-	}
+	require.NoError(t, err)
+	require.Equal(t, autopilotConf, restoredConf)

 	// Verify intentions are restored.
 	_, ixns, err := fsm2.state.Intentions(nil)
-	require.NoError(err)
-	assert.Len(ixns, 1)
-	assert.Equal(ixn, ixns[0])
+	require.NoError(t, err)
+	require.Len(t, ixns, 1)
+	require.Equal(t, ixn, ixns[0])

 	// Verify CA roots are restored.
 	_, roots, err = fsm2.state.CARoots(nil)
-	require.NoError(err)
-	assert.Len(roots, 2)
+	require.NoError(t, err)
+	require.Len(t, roots, 2)

 	// Verify provider state is restored.
 	_, state, err := fsm2.state.CAProviderState("asdf")
-	require.NoError(err)
-	assert.Equal("foo", state.PrivateKey)
-	assert.Equal("bar", state.RootCert)
+	require.NoError(t, err)
+	require.Equal(t, "foo", state.PrivateKey)
+	require.Equal(t, "bar", state.RootCert)

 	// Verify CA configuration is restored.
 	_, caConf, err := fsm2.state.CAConfig(nil)
-	require.NoError(err)
-	assert.Equal(caConfig, caConf)
+	require.NoError(t, err)
+	require.Equal(t, caConfig, caConf)

 	// Verify config entries are restored
 	_, serviceConfEntry, err := fsm2.state.ConfigEntry(nil, structs.ServiceDefaults, "foo", structs.DefaultEnterpriseMeta())
-	require.NoError(err)
-	assert.Equal(serviceConfig, serviceConfEntry)
+	require.NoError(t, err)
+	require.Equal(t, serviceConfig, serviceConfEntry)

 	_, proxyConfEntry, err := fsm2.state.ConfigEntry(nil, structs.ProxyDefaults, "global", structs.DefaultEnterpriseMeta())
-	require.NoError(err)
-	assert.Equal(proxyConfig, proxyConfEntry)
+	require.NoError(t, err)
+	require.Equal(t, proxyConfig, proxyConfEntry)

 	_, ingressRestored, err := fsm2.state.ConfigEntry(nil, structs.IngressGateway, "ingress", structs.DefaultEnterpriseMeta())
-	require.NoError(err)
-	assert.Equal(ingress, ingressRestored)
+	require.NoError(t, err)
+	require.Equal(t, ingress, ingressRestored)

 	_, restoredGatewayServices, err := fsm2.state.GatewayServices(nil, "ingress", structs.DefaultEnterpriseMeta())
-	require.NoError(err)
-	require.Equal(gatewayServices, restoredGatewayServices)
+	require.NoError(t, err)
+	require.Equal(t, gatewayServices, restoredGatewayServices)

 	newChunkState, err := fsm2.chunker.CurrentState()
-	require.NoError(err)
-	assert.Equal(newChunkState, chunkState)
+	require.NoError(t, err)
+	require.Equal(t, newChunkState, chunkState)

 	// Verify federation states are restored.
 	_, fedStateLoaded1, err := fsm2.state.FederationStateGet(nil, "dc1")
-	require.NoError(err)
-	assert.Equal(fedState1, fedStateLoaded1)
+	require.NoError(t, err)
+	require.Equal(t, fedState1, fedStateLoaded1)
 	_, fedStateLoaded2, err := fsm2.state.FederationStateGet(nil, "dc2")
-	require.NoError(err)
-	assert.Equal(fedState2, fedStateLoaded2)
+	require.NoError(t, err)
+	require.Equal(t, fedState2, fedStateLoaded2)

 	// Snapshot
 	snap, err = fsm2.Snapshot()
-	if err != nil {
-		t.Fatalf("err: %v", err)
-	}
+	require.NoError(t, err)
 	defer snap.Release()

 	// Persist
 	buf = bytes.NewBuffer(nil)
 	sink = &MockSink{buf, false}
-	if err := snap.Persist(sink); err != nil {
-		t.Fatalf("err: %v", err)
-	}
+	require.NoError(t, snap.Persist(sink))

 	// Try to restore on the old FSM and make sure it abandons the old state
 	// store.
 	abandonCh := fsm.state.AbandonCh()
-	if err := fsm.Restore(sink); err != nil {
-		t.Fatalf("err: %v", err)
-	}
+	require.NoError(t, fsm.Restore(sink))
 	select {
 	case <-abandonCh:
 	default:
-		t.Fatalf("bad")
+		require.Fail(t, "Old state not abandoned")
 	}
 }

@ -664,37 +651,27 @@ func TestFSM_BadRestore_OSS(t *testing.T) {
 	// Create an FSM with some state.
 	logger := testutil.Logger(t)
 	fsm, err := New(nil, logger)
-	if err != nil {
-		t.Fatalf("err: %v", err)
-	}
+	require.NoError(t, err)
 	fsm.state.EnsureNode(1, &structs.Node{Node: "foo", Address: "127.0.0.1"})
 	abandonCh := fsm.state.AbandonCh()

 	// Do a bad restore.
 	buf := bytes.NewBuffer([]byte("bad snapshot"))
 	sink := &MockSink{buf, false}
-	if err := fsm.Restore(sink); err == nil {
-		t.Fatalf("err: %v", err)
-	}
+	require.Error(t, fsm.Restore(sink))

 	// Verify the contents didn't get corrupted.
 	_, nodes, err := fsm.state.Nodes(nil)
-	if err != nil {
-		t.Fatalf("err: %s", err)
-	}
-	if len(nodes) != 1 {
-		t.Fatalf("bad: %v", nodes)
-	}
-	if nodes[0].Node != "foo" ||
-		nodes[0].Address != "127.0.0.1" ||
-		len(nodes[0].TaggedAddresses) != 0 {
-		t.Fatalf("bad: %v", nodes[0])
-	}
+	require.NoError(t, err)
+	require.Len(t, nodes, 1)
+	require.Equal(t, "foo", nodes[0].Node)
+	require.Equal(t, "127.0.0.1", nodes[0].Address)
+	require.Empty(t, nodes[0].TaggedAddresses)

 	// Verify the old state store didn't get abandoned.
 	select {
 	case <-abandonCh:
-		t.Fatalf("bad")
+		require.FailNow(t, "FSM state was abandoned when it should not have been")
 	default:
 	}
 }
@ -702,46 +679,32 @@ func TestFSM_BadRestore_OSS(t *testing.T) {
 func TestFSM_BadSnapshot_NilCAConfig(t *testing.T) {
 	t.Parallel()

-	require := require.New(t)
-
 	// Create an FSM with no config entry.
 	logger := testutil.Logger(t)
 	fsm, err := New(nil, logger)
-	if err != nil {
-		t.Fatalf("err: %v", err)
-	}
+	require.NoError(t, err)

 	// Snapshot
 	snap, err := fsm.Snapshot()
-	if err != nil {
-		t.Fatalf("err: %v", err)
-	}
+	require.NoError(t, err)
 	defer snap.Release()

 	// Persist
 	buf := bytes.NewBuffer(nil)
 	sink := &MockSink{buf, false}
-	if err := snap.Persist(sink); err != nil {
-		t.Fatalf("err: %v", err)
-	}
+	require.NoError(t, snap.Persist(sink))

 	// Try to restore on a new FSM
 	fsm2, err := New(nil, logger)
-	if err != nil {
-		t.Fatalf("err: %v", err)
-	}
+	require.NoError(t, err)

 	// Do a restore
-	if err := fsm2.Restore(sink); err != nil {
-		t.Fatalf("err: %v", err)
-	}
+	require.NoError(t, fsm2.Restore(sink))

 	// Make sure there's no entry in the CA config table.
 	state := fsm2.State()
 	idx, config, err := state.CAConfig(nil)
-	require.NoError(err)
-	require.Equal(uint64(0), idx)
-	if config != nil {
-		t.Fatalf("config should be nil")
-	}
+	require.NoError(t, err)
+	require.EqualValues(t, 0, idx)
+	require.Nil(t, config)
 }
--- a/agent/consul/state/acl.go
+++ b/agent/consul/state/acl.go
@ -765,6 +765,9 @@ func (s *Store) aclTokenSetTxn(tx *memdb.Txn, idx uint64, token *structs.ACLToke
 		token.ModifyIndex = idx
 	}

+	// ensure that a hash is set
+	token.SetHash(false)
+
 	return s.aclTokenInsert(tx, token)
 }

--- a/agent/consul/state/acl_test.go
+++ b/agent/consul/state/acl_test.go
@ -501,6 +501,7 @@ func TestStateStore_ACLToken_SetGet(t *testing.T) {
 		idx, rtoken, err := s.ACLTokenGetByAccessor(nil, "daf37c07-d04d-4fd5-9678-a8206a57d61a", nil)
 		require.NoError(t, err)
 		require.Equal(t, uint64(2), idx)
+		require.NotEmpty(t, rtoken.Hash)
 		compareTokens(t, token, rtoken)
 		require.Equal(t, uint64(2), rtoken.CreateIndex)
 		require.Equal(t, uint64(2), rtoken.ModifyIndex)
@ -3843,6 +3844,10 @@ func stripIrrelevantTokenFields(token *structs.ACLToken) *structs.ACLToken {
 	// The raft indexes won't match either because the requester will not
 	// have access to that.
 	tokenCopy.RaftIndex = structs.RaftIndex{}
+
+	// nil out the hash - this is a computed field and we should assert
+	// elsewhere that its not empty when expected
+	tokenCopy.Hash = nil
 	return tokenCopy
 }

--- a/agent/structs/acl_legacy.go
+++ b/agent/structs/acl_legacy.go
@ -72,7 +72,7 @@ func (a *ACL) Convert() *ACLToken {
 		a.Rules = correctedRules
 	}

-	return &ACLToken{
+	token := &ACLToken{
 		AccessorID:        "",
 		SecretID:          a.ID,
 		Description:       a.Name,
@ -83,6 +83,9 @@ func (a *ACL) Convert() *ACLToken {
 		Local:             false,
 		RaftIndex:         a.RaftIndex,
 	}
+
+	token.SetHash(true)
+	return token
 }

 // Convert attempts to convert an ACLToken into an ACLCompat.
--- a/agent/structs/acl_legacy_test.go
+++ b/agent/structs/acl_legacy_test.go
@ -72,6 +72,7 @@ func TestStructs_ACL_Convert(t *testing.T) {
 	require.Equal(t, acl.Rules, token.Rules)
 	require.Equal(t, acl.CreateIndex, token.CreateIndex)
 	require.Equal(t, acl.ModifyIndex, token.ModifyIndex)
+	require.NotEmpty(t, token.Hash)
 }

 func TestStructs_ACLToken_Convert(t *testing.T) {