diff --git a/.changelog/_1253.txt b/.changelog/_1253.txt new file mode 100644 index 0000000000..dbe9453d48 --- /dev/null +++ b/.changelog/_1253.txt @@ -0,0 +1,3 @@ +```release-note:security +agent: Use SHA256 instead of MD5 to generate persistence file names. +``` diff --git a/agent/agent.go b/agent/agent.go index 2c254e251d..ba4bb650fd 100644 --- a/agent/agent.go +++ b/agent/agent.go @@ -1775,10 +1775,14 @@ type persistedService struct { LocallyRegisteredAsSidecar bool `json:",omitempty"` } +func (a *Agent) makeServiceFilePath(svcID structs.ServiceID) string { + return filepath.Join(a.config.DataDir, servicesDir, svcID.StringHashSHA256()) +} + // persistService saves a service definition to a JSON file in the data dir func (a *Agent) persistService(service *structs.NodeService, source configSource) error { svcID := service.CompoundServiceID() - svcPath := filepath.Join(a.config.DataDir, servicesDir, svcID.StringHash()) + svcPath := a.makeServiceFilePath(svcID) wrapped := persistedService{ Token: a.State.ServiceToken(service.CompoundServiceID()), @@ -1796,7 +1800,7 @@ func (a *Agent) persistService(service *structs.NodeService, source configSource // purgeService removes a persisted service definition file from the data dir func (a *Agent) purgeService(serviceID structs.ServiceID) error { - svcPath := filepath.Join(a.config.DataDir, servicesDir, serviceID.StringHash()) + svcPath := a.makeServiceFilePath(serviceID) if _, err := os.Stat(svcPath); err == nil { return os.Remove(svcPath) } @@ -1806,7 +1810,7 @@ func (a *Agent) purgeService(serviceID structs.ServiceID) error { // persistCheck saves a check definition to the local agent's state directory func (a *Agent) persistCheck(check *structs.HealthCheck, chkType *structs.CheckType, source configSource) error { cid := check.CompoundCheckID() - checkPath := filepath.Join(a.config.DataDir, checksDir, cid.StringHash()) + checkPath := filepath.Join(a.config.DataDir, checksDir, cid.StringHashSHA256()) // Create the persisted check wrapped := persistedCheck{ @@ -1826,7 +1830,7 @@ func (a *Agent) persistCheck(check *structs.HealthCheck, chkType *structs.CheckT // purgeCheck removes a persisted check definition file from the data dir func (a *Agent) purgeCheck(checkID structs.CheckID) error { - checkPath := filepath.Join(a.config.DataDir, checksDir, checkID.StringHash()) + checkPath := filepath.Join(a.config.DataDir, checksDir, checkID.StringHashSHA256()) if _, err := os.Stat(checkPath); err == nil { return os.Remove(checkPath) } @@ -1842,6 +1846,10 @@ type persistedServiceConfig struct { structs.EnterpriseMeta } +func (a *Agent) makeServiceConfigFilePath(serviceID structs.ServiceID) string { + return filepath.Join(a.config.DataDir, serviceConfigDir, serviceID.StringHashSHA256()) +} + func (a *Agent) persistServiceConfig(serviceID structs.ServiceID, defaults *structs.ServiceConfigResponse) error { // Create the persisted config. wrapped := persistedServiceConfig{ @@ -1856,7 +1864,7 @@ func (a *Agent) persistServiceConfig(serviceID structs.ServiceID, defaults *stru } dir := filepath.Join(a.config.DataDir, serviceConfigDir) - configPath := filepath.Join(dir, serviceID.StringHash()) + configPath := a.makeServiceConfigFilePath(serviceID) // Create the config dir if it doesn't exist if err := os.MkdirAll(dir, 0700); err != nil { @@ -1867,7 +1875,7 @@ func (a *Agent) persistServiceConfig(serviceID structs.ServiceID, defaults *stru } func (a *Agent) purgeServiceConfig(serviceID structs.ServiceID) error { - configPath := filepath.Join(a.config.DataDir, serviceConfigDir, serviceID.StringHash()) + configPath := a.makeServiceConfigFilePath(serviceID) if _, err := os.Stat(configPath); err == nil { return os.Remove(configPath) } @@ -1914,7 +1922,18 @@ func (a *Agent) readPersistedServiceConfigs() (map[structs.ServiceID]*structs.Se ) continue } - out[structs.NewServiceID(p.ServiceID, &p.EnterpriseMeta)] = p.Defaults + + serviceID := structs.NewServiceID(p.ServiceID, &p.EnterpriseMeta) + + // Rename files that used the old md5 hash to the new sha256 name; only needed when upgrading from 1.10 and before. + newPath := a.makeServiceConfigFilePath(serviceID) + if file != newPath { + if err := os.Rename(file, newPath); err != nil { + a.logger.Error("Failed renaming service config file from %s to %s", file, newPath, err) + } + } + + out[serviceID] = p.Defaults } return out, nil @@ -2983,7 +3002,7 @@ func (a *Agent) persistCheckState(check *checks.CheckTTL, status, output string) } // Write the state to the file - file := filepath.Join(dir, check.CheckID.StringHash()) + file := filepath.Join(dir, check.CheckID.StringHashSHA256()) // Create temp file in same dir, to make more likely atomic tempFile := file + ".tmp" @@ -3003,13 +3022,26 @@ func (a *Agent) persistCheckState(check *checks.CheckTTL, status, output string) func (a *Agent) loadCheckState(check *structs.HealthCheck) error { cid := check.CompoundCheckID() // Try to read the persisted state for this check - file := filepath.Join(a.config.DataDir, checkStateDir, cid.StringHash()) + file := filepath.Join(a.config.DataDir, checkStateDir, cid.StringHashSHA256()) buf, err := ioutil.ReadFile(file) if err != nil { if os.IsNotExist(err) { - return nil + // try the md5 based name. This can be removed once we no longer support upgrades from versions that use MD5 hashing + oldFile := filepath.Join(a.config.DataDir, checkStateDir, cid.StringHashMD5()) + buf, err = ioutil.ReadFile(oldFile) + if err != nil { + if os.IsNotExist(err) { + return nil + } else { + return fmt.Errorf("failed reading file %q: %s", file, err) + } + } + if err := os.Rename(oldFile, file); err != nil { + a.logger.Error("Failed renaming service file from %s to %s", oldFile, file, err) + } + } else { + return fmt.Errorf("failed reading file %q: %s", file, err) } - return fmt.Errorf("failed reading file %q: %s", file, err) } // Decode the state data @@ -3033,7 +3065,7 @@ func (a *Agent) loadCheckState(check *structs.HealthCheck) error { // purgeCheckState is used to purge the state of a check from the data dir func (a *Agent) purgeCheckState(checkID structs.CheckID) error { - file := filepath.Join(a.config.DataDir, checkStateDir, checkID.StringHash()) + file := filepath.Join(a.config.DataDir, checkStateDir, checkID.StringHashSHA256()) err := os.Remove(file) if os.IsNotExist(err) { return nil @@ -3232,6 +3264,14 @@ func (a *Agent) loadServices(conf *config.RuntimeConfig, snap map[structs.CheckI } } + // Rename files that used the old md5 hash to the new sha256 name; only needed when upgrading from 1.10 and before. + newPath := a.makeServiceFilePath(p.Service.CompoundServiceID()) + if file != newPath { + if err := os.Rename(file, newPath); err != nil { + a.logger.Error("Failed renaming service file from %s to %s", file, newPath, err) + } + } + // Restore LocallyRegisteredAsSidecar, see persistedService.LocallyRegisteredAsSidecar p.Service.LocallyRegisteredAsSidecar = p.LocallyRegisteredAsSidecar @@ -3362,6 +3402,14 @@ func (a *Agent) loadChecks(conf *config.RuntimeConfig, snap map[structs.CheckID] } checkID := p.Check.CompoundCheckID() + // Rename files that used the old md5 hash to the new sha256 name; only needed when upgrading from 1.10 and before. + newPath := filepath.Join(a.config.DataDir, checksDir, checkID.StringHashSHA256()) + if file != newPath { + if err := os.Rename(file, newPath); err != nil { + a.logger.Error("Failed renaming service file from %s to %s", file, newPath, err) + } + } + source, ok := ConfigSourceFromName(p.Source) if !ok { a.logger.Warn("check exists with invalid source, purging", diff --git a/agent/agent_test.go b/agent/agent_test.go index e0bcc15923..6b3ea8cfbd 100644 --- a/agent/agent_test.go +++ b/agent/agent_test.go @@ -3,6 +3,7 @@ package agent import ( "bytes" "context" + "crypto/md5" "crypto/tls" "crypto/x509" "encoding/base64" @@ -2254,7 +2255,7 @@ func testAgent_PersistService(t *testing.T, extraHCL string) { Port: 8000, } - file := filepath.Join(a.Config.DataDir, servicesDir, stringHash(svc.ID)) + file := filepath.Join(a.Config.DataDir, servicesDir, structs.NewServiceID(svc.ID, nil).StringHashSHA256()) // Check is not persisted unless requested if err := a.addServiceFromSource(svc, nil, false, "", ConfigSourceLocal); err != nil { @@ -2365,7 +2366,7 @@ func testAgent_persistedService_compat(t *testing.T, extraHCL string) { } // Write the content to the file - file := filepath.Join(a.Config.DataDir, servicesDir, stringHash(svc.ID)) + file := filepath.Join(a.Config.DataDir, servicesDir, structs.NewServiceID(svc.ID, nil).StringHashSHA256()) if err := os.MkdirAll(filepath.Dir(file), 0700); err != nil { t.Fatalf("err: %s", err) } @@ -2383,6 +2384,85 @@ func testAgent_persistedService_compat(t *testing.T, extraHCL string) { require.Equal(t, svc, result) } +func TestAgent_persistedService_compat_hash(t *testing.T) { + t.Run("normal", func(t *testing.T) { + t.Parallel() + testAgent_persistedService_compat_hash(t, "enable_central_service_config = false") + }) + t.Run("service manager", func(t *testing.T) { + t.Parallel() + testAgent_persistedService_compat_hash(t, "enable_central_service_config = true") + }) + +} + +func testAgent_persistedService_compat_hash(t *testing.T, extraHCL string) { + t.Helper() + + // Tests backwards compatibility of persisted services from pre-0.5.1 + a := NewTestAgent(t, extraHCL) + defer a.Shutdown() + + svc := &structs.NodeService{ + ID: "redis", + Service: "redis", + Tags: []string{"foo"}, + Port: 8000, + TaggedAddresses: map[string]structs.ServiceAddress{}, + Weights: &structs.Weights{Passing: 1, Warning: 1}, + EnterpriseMeta: *structs.DefaultEnterpriseMetaInDefaultPartition(), + } + + // Encode the NodeService directly. This is what previous versions + // would serialize to the file (without the wrapper) + encoded, err := json.Marshal(svc) + if err != nil { + t.Fatalf("err: %s", err) + } + + // Write the content to the file using the old md5 based path + file := filepath.Join(a.Config.DataDir, servicesDir, stringHashMD5(svc.ID)) + if err := os.MkdirAll(filepath.Dir(file), 0700); err != nil { + t.Fatalf("err: %s", err) + } + if err := ioutil.WriteFile(file, encoded, 0600); err != nil { + t.Fatalf("err: %s", err) + } + + wrapped := persistedServiceConfig{ + ServiceID: "redis", + Defaults: &structs.ServiceConfigResponse{}, + EnterpriseMeta: *structs.DefaultEnterpriseMetaInDefaultPartition(), + } + + encodedConfig, err := json.Marshal(wrapped) + if err != nil { + t.Fatalf("err: %s", err) + } + + configFile := filepath.Join(a.Config.DataDir, serviceConfigDir, stringHashMD5(svc.ID)) + if err := os.MkdirAll(filepath.Dir(configFile), 0700); err != nil { + t.Fatalf("err: %s", err) + } + if err := ioutil.WriteFile(configFile, encodedConfig, 0600); err != nil { + t.Fatalf("err: %s", err) + } + + // Load the services + if err := a.loadServices(a.Config, nil); err != nil { + t.Fatalf("err: %s", err) + } + + // Ensure the service was restored + result := requireServiceExists(t, a, "redis") + require.Equal(t, svc, result) +} + +// Exists for backwards compatibility testing +func stringHashMD5(s string) string { + return fmt.Sprintf("%x", md5.Sum([]byte(s))) +} + func TestAgent_PurgeService(t *testing.T) { if testing.Short() { t.Skip("too slow for testing.Short") @@ -2411,7 +2491,7 @@ func testAgent_PurgeService(t *testing.T, extraHCL string) { Port: 8000, } - file := filepath.Join(a.Config.DataDir, servicesDir, stringHash(svc.ID)) + file := filepath.Join(a.Config.DataDir, servicesDir, structs.NewServiceID(svc.ID, nil).StringHashSHA256()) if err := a.addServiceFromSource(svc, nil, true, "", ConfigSourceLocal); err != nil { t.Fatalf("err: %v", err) } @@ -2491,7 +2571,7 @@ func testAgent_PurgeServiceOnDuplicate(t *testing.T, extraHCL string) { defer a2.Shutdown() sid := svc1.CompoundServiceID() - file := filepath.Join(a.Config.DataDir, servicesDir, sid.StringHash()) + file := filepath.Join(a.Config.DataDir, servicesDir, sid.StringHashSHA256()) _, err := os.Stat(file) require.Error(t, err, "should have removed persisted service") result := requireServiceExists(t, a, "redis") @@ -2525,7 +2605,7 @@ func TestAgent_PersistCheck(t *testing.T) { } cid := check.CompoundCheckID() - file := filepath.Join(a.Config.DataDir, checksDir, cid.StringHash()) + file := filepath.Join(a.Config.DataDir, checksDir, cid.StringHashSHA256()) // Not persisted if not requested require.NoError(t, a.AddCheck(check, chkType, false, "", ConfigSourceLocal)) @@ -2671,7 +2751,7 @@ func TestAgent_PurgeCheckOnDuplicate(t *testing.T) { defer a2.Shutdown() cid := check1.CompoundCheckID() - file := filepath.Join(a.DataDir, checksDir, cid.StringHash()) + file := filepath.Join(a.DataDir, checksDir, cid.StringHashSHA256()) if _, err := os.Stat(file); err == nil { t.Fatalf("should have removed persisted check") } @@ -3452,6 +3532,69 @@ func TestAgent_checkStateSnapshot(t *testing.T) { } } +func TestAgent_checkStateSnapshot_backcompat(t *testing.T) { + if testing.Short() { + t.Skip("too slow for testing.Short") + } + + t.Parallel() + a := NewTestAgent(t, "") + defer a.Shutdown() + + // First register a service + svc := &structs.NodeService{ + ID: "redis", + Service: "redis", + Tags: []string{"foo"}, + Port: 8000, + } + if err := a.addServiceFromSource(svc, nil, false, "", ConfigSourceLocal); err != nil { + t.Fatalf("err: %v", err) + } + + // Register a check + check1 := &structs.HealthCheck{ + Node: a.Config.NodeName, + CheckID: "service:redis", + Name: "redischeck", + Status: api.HealthPassing, + ServiceID: "redis", + ServiceName: "redis", + } + if err := a.AddCheck(check1, nil, true, "", ConfigSourceLocal); err != nil { + t.Fatalf("err: %s", err) + } + + // Snapshot the state + snap := a.snapshotCheckState() + + // Unload all of the checks + if err := a.unloadChecks(); err != nil { + t.Fatalf("err: %s", err) + } + + // Mutate the path to look like the old md5 checksum + dir := filepath.Join(a.config.DataDir, checksDir) + new_path := filepath.Join(dir, check1.CompoundCheckID().StringHashSHA256()) + old_path := filepath.Join(dir, check1.CompoundCheckID().StringHashMD5()) + if err := os.Rename(new_path, old_path); err != nil { + t.Fatalf("err: %s", err) + } + + // Reload the checks and restore the snapshot. + if err := a.loadChecks(a.Config, snap); err != nil { + t.Fatalf("err: %s", err) + } + + // Search for the check + out := requireCheckExists(t, a, check1.CheckID) + + // Make sure state was restored + if out.Status != api.HealthPassing { + t.Fatalf("should have restored check state") + } +} + func TestAgent_loadChecks_checkFails(t *testing.T) { if testing.Short() { t.Skip("too slow for testing.Short") @@ -3514,7 +3657,7 @@ func TestAgent_persistCheckState(t *testing.T) { } // Check the persisted file exists and has the content - file := filepath.Join(a.Config.DataDir, checkStateDir, cid.StringHash()) + file := filepath.Join(a.Config.DataDir, checkStateDir, cid.StringHashSHA256()) buf, err := ioutil.ReadFile(file) if err != nil { t.Fatalf("err: %s", err) @@ -3582,7 +3725,7 @@ func TestAgent_loadCheckState(t *testing.T) { } // Should have purged the state - file := filepath.Join(a.Config.DataDir, checksDir, stringHash("check1")) + file := filepath.Join(a.Config.DataDir, checksDir, structs.NewCheckID("check1", nil).StringHashSHA256()) if _, err := os.Stat(file); !os.IsNotExist(err) { t.Fatalf("should have purged state") } @@ -3639,7 +3782,7 @@ func TestAgent_purgeCheckState(t *testing.T) { } // Removed the file - file := filepath.Join(a.Config.DataDir, checkStateDir, cid.StringHash()) + file := filepath.Join(a.Config.DataDir, checkStateDir, cid.StringHashSHA256()) if _, err := os.Stat(file); !os.IsNotExist(err) { t.Fatalf("should have removed file") } diff --git a/agent/service_manager_test.go b/agent/service_manager_test.go index 9831144d09..e0d214b863 100644 --- a/agent/service_manager_test.go +++ b/agent/service_manager_test.go @@ -396,8 +396,8 @@ func TestServiceManager_PersistService_API(t *testing.T) { svc := newNodeService() svcID := svc.CompoundServiceID() - svcFile := filepath.Join(a.Config.DataDir, servicesDir, svcID.StringHash()) - configFile := filepath.Join(a.Config.DataDir, serviceConfigDir, svcID.StringHash()) + svcFile := filepath.Join(a.Config.DataDir, servicesDir, svcID.StringHashSHA256()) + configFile := filepath.Join(a.Config.DataDir, serviceConfigDir, svcID.StringHashSHA256()) // Service is not persisted unless requested, but we always persist service configs. err = a.AddService(AddServiceRequest{Service: svc, Source: ConfigSourceRemote}) @@ -643,8 +643,8 @@ func TestServiceManager_PersistService_ConfigFiles(t *testing.T) { require.Equal(r, expectState, current) }) - svcFile := filepath.Join(a.Config.DataDir, servicesDir, stringHash(svcID)) - configFile := filepath.Join(a.Config.DataDir, serviceConfigDir, stringHash(svcID)) + svcFile := filepath.Join(a.Config.DataDir, servicesDir, stringHashSHA256(svcID)) + configFile := filepath.Join(a.Config.DataDir, serviceConfigDir, stringHashSHA256(svcID)) // Service is never persisted, but we always persist service configs. requireFileIsAbsent(t, svcFile) diff --git a/agent/structs/structs.go b/agent/structs/structs.go index 7f3bd3369e..aeb3d8e446 100644 --- a/agent/structs/structs.go +++ b/agent/structs/structs.go @@ -3,6 +3,7 @@ package structs import ( "bytes" "crypto/md5" + "crypto/sha256" "encoding/json" "fmt" "math/rand" @@ -1876,15 +1877,25 @@ func NewCheckID(id types.CheckID, entMeta *EnterpriseMeta) CheckID { return cid } -// StringHash is used mainly to populate part of the filename of a check -// definition persisted on the local agent -func (cid CheckID) StringHash() string { +// StringHashMD5 is used mainly to populate part of the filename of a check +// definition persisted on the local agent (deprecated in favor of StringHashSHA256) +// Kept around for backwards compatibility +func (cid CheckID) StringHashMD5() string { hasher := md5.New() hasher.Write([]byte(cid.ID)) cid.EnterpriseMeta.addToHash(hasher, true) return fmt.Sprintf("%x", hasher.Sum(nil)) } +// StringHashSHA256 is used mainly to populate part of the filename of a check +// definition persisted on the local agent +func (cid CheckID) StringHashSHA256() string { + hasher := sha256.New() + hasher.Write([]byte(cid.ID)) + cid.EnterpriseMeta.addToHash(hasher, true) + return fmt.Sprintf("%x", hasher.Sum(nil)) +} + type ServiceID struct { ID string EnterpriseMeta @@ -1906,10 +1917,10 @@ func (sid ServiceID) Matches(other ServiceID) bool { return sid.ID == other.ID && sid.EnterpriseMeta.Matches(&other.EnterpriseMeta) } -// StringHash is used mainly to populate part of the filename of a service +// StringHashSHA256 is used mainly to populate part of the filename of a service // definition persisted on the local agent -func (sid ServiceID) StringHash() string { - hasher := md5.New() +func (sid ServiceID) StringHashSHA256() string { + hasher := sha256.New() hasher.Write([]byte(sid.ID)) sid.EnterpriseMeta.addToHash(hasher, true) return fmt.Sprintf("%x", hasher.Sum(nil)) diff --git a/agent/util.go b/agent/util.go index 2649563619..55688fe308 100644 --- a/agent/util.go +++ b/agent/util.go @@ -1,7 +1,7 @@ package agent import ( - "crypto/md5" + "crypto/sha256" "fmt" "os" "os/exec" @@ -14,14 +14,13 @@ import ( "github.com/hashicorp/consul/types" ) -// stringHash returns a simple md5sum for a string. -func stringHash(s string) string { - return fmt.Sprintf("%x", md5.Sum([]byte(s))) +func stringHashSHA256(s string) string { + return fmt.Sprintf("%x", sha256.Sum256([]byte(s))) } // checkIDHash returns a simple md5sum for a types.CheckID. func checkIDHash(checkID types.CheckID) string { - return stringHash(string(checkID)) + return stringHashSHA256(string(checkID)) } // setFilePermissions handles configuring ownership and permissions diff --git a/agent/util_test.go b/agent/util_test.go index 567ed6a71b..c2c49648a0 100644 --- a/agent/util_test.go +++ b/agent/util_test.go @@ -14,13 +14,13 @@ import ( "github.com/stretchr/testify/require" ) -func TestStringHash(t *testing.T) { +func TestStringHashSHA256(t *testing.T) { t.Parallel() - in := "hello world" - expected := "5eb63bbbe01eeed093cb22bb8f5acdc3" + in := "hello world\n" + expected := "a948904f2f0f479b8f8197694b30184b0d2ed1c1cd2a1ec0fb85d299a192a447" - if out := stringHash(in); out != expected { - t.Fatalf("bad: %s", out) + if out := stringHashSHA256(in); out != expected { + t.Fatalf("bad: %s expected %s", out, expected) } }