Merge pull request #10218 from hashicorp/dnephin/backport-local-agent-fix

[1.9.x] agent/local: do not persist the agent or user token

.changelog/10188.txt

@@ -0,0 +1,3 @@
+```release-note:bug
+local: agents will no longer persist the default user token along with a service or check.
+```

agent/local/state.go

@@ -227,25 +227,25 @@ func (l *State) SetDiscardCheckOutput(b bool) {
 	l.discardCheckOutput.Store(b)
 }
 
-// ServiceToken returns the configured ACL token for the given
-// service ID. If none is present, the agent's token is returned.
+// ServiceToken returns the ACL token associated with the service. If the service is
+// not found, or does not have a token, the empty string is returned.
 func (l *State) ServiceToken(id structs.ServiceID) string {
 	l.RLock()
 	defer l.RUnlock()
-	return l.serviceToken(id)
+	if s := l.services[id]; s != nil {
+		return s.Token
+	}
+	return ""
 }
 
-// serviceToken returns an ACL token associated with a service.
+// aclTokenForServiceSync returns an ACL token associated with a service. If there is
+// no ACL token associated with the service, fallback is used to return a value.
 // This method is not synchronized and the lock must already be held.
-func (l *State) serviceToken(id structs.ServiceID) string {
-	var token string
-	if s := l.services[id]; s != nil {
-		token = s.Token
+func (l *State) aclTokenForServiceSync(id structs.ServiceID, fallback func() string) string {
+	if s := l.services[id]; s != nil && s.Token != "" {
+		return s.Token
 	}
-	if token == "" {
-		token = l.tokens.AgentToken()
-	}
-	return token
+	return fallback()
 }
 
 // AddService is used to add a service entry to the local state.
@@ -440,26 +440,25 @@ func (l *State) ServiceStates(entMeta *structs.EnterpriseMeta) map[structs.Servi
 	return m
 }
 
-// CheckToken is used to return the configured health check token for a
-// Check, or if none is configured, the default agent ACL token.
-func (l *State) CheckToken(checkID structs.CheckID) string {
+// CheckToken returns the ACL token associated with the check. If the check is
+// not found, or does not have a token, the empty string is returned.
+func (l *State) CheckToken(id structs.CheckID) string {
 	l.RLock()
 	defer l.RUnlock()
-	return l.checkToken(checkID)
+	if c := l.checks[id]; c != nil {
+		return c.Token
+	}
+	return ""
 }
 
-// checkToken returns an ACL token associated with a check.
+// aclTokenForCheckSync returns an ACL token associated with a check. If there is
+// no ACL token associated with the check, the callback is used to return a value.
 // This method is not synchronized and the lock must already be held.
-func (l *State) checkToken(id structs.CheckID) string {
-	var token string
-	c := l.checks[id]
-	if c != nil {
-		token = c.Token
+func (l *State) aclTokenForCheckSync(id structs.CheckID, fallback func() string) string {
+	if c := l.checks[id]; c != nil && c.Token != "" {
+		return c.Token
 	}
-	if token == "" {
-		token = l.tokens.AgentToken()
-	}
-	return token
+	return fallback()
 }
 
 // AddCheck is used to add a health check to the local state.
@@ -1132,8 +1131,7 @@ func (l *State) deleteService(key structs.ServiceID) error {
 		return fmt.Errorf("ServiceID missing")
 	}
 
-	st := l.serviceToken(key)
-
+	st := l.aclTokenForServiceSync(key, l.tokens.AgentToken)
 	req := structs.DeregisterRequest{
 		Datacenter:     l.config.Datacenter,
 		Node:           l.config.NodeName,
@@ -1182,7 +1180,7 @@ func (l *State) deleteCheck(key structs.CheckID) error {
 		return fmt.Errorf("CheckID missing")
 	}
 
-	ct := l.checkToken(key)
+	ct := l.aclTokenForCheckSync(key, l.tokens.AgentToken)
 	req := structs.DeregisterRequest{
 		Datacenter:     l.config.Datacenter,
 		Node:           l.config.NodeName,
@@ -1226,7 +1224,7 @@ func (l *State) pruneCheck(id structs.CheckID) {
 
 // syncService is used to sync a service to the server
 func (l *State) syncService(key structs.ServiceID) error {
-	st := l.serviceToken(key)
+	st := l.aclTokenForServiceSync(key, l.tokens.UserToken)
 
 	// If the service has associated checks that are out of sync,
 	// piggyback them on the service sync so they are part of the
@@ -1242,7 +1240,7 @@ func (l *State) syncService(key structs.ServiceID) error {
 		if !key.Matches(c.Check.CompoundServiceID()) {
 			continue
 		}
-		if st != l.checkToken(checkKey) {
+		if st != l.aclTokenForCheckSync(checkKey, l.tokens.UserToken) {
 			continue
 		}
 		checks = append(checks, c.Check)
@@ -1308,7 +1306,7 @@ func (l *State) syncService(key structs.ServiceID) error {
 // syncCheck is used to sync a check to the server
 func (l *State) syncCheck(key structs.CheckID) error {
 	c := l.checks[key]
-	ct := l.checkToken(key)
+	ct := l.aclTokenForCheckSync(key, l.tokens.UserToken)
 	req := structs.RegisterRequest{
 		Datacenter:      l.config.Datacenter,
 		ID:              l.config.NodeID,

agent/local/state_test.go

@@ -1720,61 +1720,72 @@ func TestAgentAntiEntropy_NodeInfo(t *testing.T) {
 	}
 }
 
-func TestAgent_ServiceTokens(t *testing.T) {
-	t.Parallel()
-
+func TestState_ServiceTokens(t *testing.T) {
 	tokens := new(token.Store)
-	tokens.UpdateUserToken("default", token.TokenSourceConfig)
 	cfg := config.DefaultRuntimeConfig(`bind_addr = "127.0.0.1" data_dir = "dummy"`)
 	l := local.NewState(agent.LocalConfig(cfg), nil, tokens)
 	l.TriggerSyncChanges = func() {}
 
-	l.AddService(&structs.NodeService{ID: "redis"}, "")
+	id := structs.NewServiceID("redis", nil)
 
-	// Returns default when no token is set
-	if token := l.ServiceToken(structs.NewServiceID("redis", nil)); token != "default" {
-		t.Fatalf("bad: %s", token)
+	t.Run("defaults to empty string", func(t *testing.T) {
+		require.Equal(t, "", l.ServiceToken(id))
+	})
+
+	t.Run("empty string when there is no token", func(t *testing.T) {
+		err := l.AddService(&structs.NodeService{ID: "redis"}, "")
+		require.NoError(t, err)
+
+		require.Equal(t, "", l.ServiceToken(id))
+	})
+
+	t.Run("returns configured token", func(t *testing.T) {
+		err := l.AddService(&structs.NodeService{ID: "redis"}, "abc123")
+		require.NoError(t, err)
+
+		require.Equal(t, "abc123", l.ServiceToken(id))
+	})
+
+	t.Run("RemoveCheck keeps token around for the delete", func(t *testing.T) {
+		err := l.RemoveService(structs.NewServiceID("redis", nil))
+		require.NoError(t, err)
+
+		require.Equal(t, "abc123", l.ServiceToken(id))
+	})
 }
 
-	// Returns configured token
-	l.AddService(&structs.NodeService{ID: "redis"}, "abc123")
-	if token := l.ServiceToken(structs.NewServiceID("redis", nil)); token != "abc123" {
-		t.Fatalf("bad: %s", token)
-	}
-
-	// Keeps token around for the delete
-	l.RemoveService(structs.NewServiceID("redis", nil))
-	if token := l.ServiceToken(structs.NewServiceID("redis", nil)); token != "abc123" {
-		t.Fatalf("bad: %s", token)
-	}
-}
-
-func TestAgent_CheckTokens(t *testing.T) {
-	t.Parallel()
-
+func TestState_CheckTokens(t *testing.T) {
 	tokens := new(token.Store)
-	tokens.UpdateUserToken("default", token.TokenSourceConfig)
 	cfg := config.DefaultRuntimeConfig(`bind_addr = "127.0.0.1" data_dir = "dummy"`)
 	l := local.NewState(agent.LocalConfig(cfg), nil, tokens)
 	l.TriggerSyncChanges = func() {}
 
-	// Returns default when no token is set
-	l.AddCheck(&structs.HealthCheck{CheckID: types.CheckID("mem")}, "")
-	if token := l.CheckToken(structs.NewCheckID("mem", nil)); token != "default" {
-		t.Fatalf("bad: %s", token)
-	}
+	id := structs.NewCheckID("mem", nil)
 
-	// Returns configured token
-	l.AddCheck(&structs.HealthCheck{CheckID: types.CheckID("mem")}, "abc123")
-	if token := l.CheckToken(structs.NewCheckID("mem", nil)); token != "abc123" {
-		t.Fatalf("bad: %s", token)
-	}
+	t.Run("defaults to empty string", func(t *testing.T) {
+		require.Equal(t, "", l.CheckToken(id))
+	})
 
-	// Keeps token around for the delete
-	l.RemoveCheck(structs.NewCheckID("mem", nil))
-	if token := l.CheckToken(structs.NewCheckID("mem", nil)); token != "abc123" {
-		t.Fatalf("bad: %s", token)
-	}
+	t.Run("empty string when there is no token", func(t *testing.T) {
+		err := l.AddCheck(&structs.HealthCheck{CheckID: "mem"}, "")
+		require.NoError(t, err)
+
+		require.Equal(t, "", l.CheckToken(id))
+	})
+
+	t.Run("returns configured token", func(t *testing.T) {
+		err := l.AddCheck(&structs.HealthCheck{CheckID: "mem"}, "abc123")
+		require.NoError(t, err)
+
+		require.Equal(t, "abc123", l.CheckToken(id))
+	})
+
+	t.Run("RemoveCheck keeps token around for the delete", func(t *testing.T) {
+		err := l.RemoveCheck(structs.NewCheckID("mem", nil))
+		require.NoError(t, err)
+
+		require.Equal(t, "abc123", l.CheckToken(id))
+	})
 }
 
 func TestAgent_CheckCriticalTime(t *testing.T) {