From ac0697ac48804d3c4c03bb2b013a6a4ffef36d2d Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Mon, 10 May 2021 15:58:20 -0400 Subject: [PATCH] Merge pull request #10188 from hashicorp/dnephin/dont-persist-agent-tokens agent/local: do not persist the agent or user token --- .changelog/10188.txt | 3 ++ agent/local/state.go | 62 ++++++++++++++-------------- agent/local/state_test.go | 87 ++++++++++++++++++++++----------------- 3 files changed, 82 insertions(+), 70 deletions(-) create mode 100644 .changelog/10188.txt diff --git a/.changelog/10188.txt b/.changelog/10188.txt new file mode 100644 index 0000000000..376a1484a2 --- /dev/null +++ b/.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. +``` diff --git a/agent/local/state.go b/agent/local/state.go index b2382ed5b1..5b9d0cc7f7 100644 --- a/agent/local/state.go +++ b/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 - } - if token == "" { - token = l.tokens.AgentToken() +func (l *State) aclTokenForServiceSync(id structs.ServiceID, fallback func() string) string { + if s := l.services[id]; s != nil && s.Token != "" { + return s.Token } - 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, diff --git a/agent/local/state_test.go b/agent/local/state_test.go index a8fc83be24..c990cc787a 100644 --- a/agent/local/state_test.go +++ b/agent/local/state_test.go @@ -1768,33 +1768,38 @@ 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 := loadRuntimeConfig(t, `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)) + }) - // 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) - } + t.Run("empty string when there is no token", func(t *testing.T) { + err := l.AddService(&structs.NodeService{ID: "redis"}, "") + require.NoError(t, err) - // 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) - } + 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)) + }) } func loadRuntimeConfig(t *testing.T, hcl string) *config.RuntimeConfig { @@ -1805,32 +1810,38 @@ func loadRuntimeConfig(t *testing.T, hcl string) *config.RuntimeConfig { return result.RuntimeConfig } -func TestAgent_CheckTokens(t *testing.T) { - t.Parallel() - +func TestState_CheckTokens(t *testing.T) { tokens := new(token.Store) - tokens.UpdateUserToken("default", token.TokenSourceConfig) cfg := loadRuntimeConfig(t, `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) {