From fb9ef29af9a11a785765dcab7f5cc2f16ce50618 Mon Sep 17 00:00:00 2001 From: Dimitar Dimitrov Date: Wed, 8 Jan 2025 17:08:42 +0200 Subject: [PATCH 1/7] Fix G306 gosec issues > G306: Expect WriteFile permissions to be 0600 or less The mimirtool commands should be ok. Makes using the tool a bit more restrictive, but linters gotta lint. My understanding of the AM changes is that they're safe too because the files written are only used immediately after that to validate them and never again, so they'll never be used by another user. But if would be nice if someone from AM-maintainers can pitch in Signed-off-by: Dimitar Dimitrov --- .golangci.yml | 1 + pkg/alertmanager/multitenant.go | 2 +- pkg/mimirtool/commands/rules.go | 2 +- 3 files changed, 3 insertions(+), 2 deletions(-) diff --git a/.golangci.yml b/.golangci.yml index 4fc058d727b..9dd6668df20 100644 --- a/.golangci.yml +++ b/.golangci.yml @@ -71,6 +71,7 @@ linters-settings: - G114 - G301 - G302 + - G306 run: timeout: 10m diff --git a/pkg/alertmanager/multitenant.go b/pkg/alertmanager/multitenant.go index f575811c48b..41093b519b0 100644 --- a/pkg/alertmanager/multitenant.go +++ b/pkg/alertmanager/multitenant.go @@ -1380,7 +1380,7 @@ func storeTemplateFile(templateFilepath, content string) (bool, error) { return false, err } - if err := os.WriteFile(templateFilepath, []byte(content), 0644); err != nil { + if err := os.WriteFile(templateFilepath, []byte(content), 0600); err != nil { return false, fmt.Errorf("unable to create Alertmanager template file %q: %s", templateFilepath, err) } diff --git a/pkg/mimirtool/commands/rules.go b/pkg/mimirtool/commands/rules.go index 81ed385d15b..7971050c97e 100644 --- a/pkg/mimirtool/commands/rules.go +++ b/pkg/mimirtool/commands/rules.go @@ -885,7 +885,7 @@ func save(nss map[string]rules.RuleNamespace, i bool) error { filepath = filepath + ".result" } - if err := os.WriteFile(filepath, payload, 0644); err != nil { + if err := os.WriteFile(filepath, payload, 0600); err != nil { return err } } From 08a0c47a5961dcb0339cea65fdd7f79f09b53aa9 Mon Sep 17 00:00:00 2001 From: Dimitar Dimitrov Date: Wed, 8 Jan 2025 17:14:03 +0200 Subject: [PATCH 2/7] Fix tests too Signed-off-by: Dimitar Dimitrov --- pkg/storegateway/indexheader/encoding/encoding_test.go | 2 +- pkg/util/process/collector_test.go | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/pkg/storegateway/indexheader/encoding/encoding_test.go b/pkg/storegateway/indexheader/encoding/encoding_test.go index 04d7eedb210..5a3a8ce6184 100644 --- a/pkg/storegateway/indexheader/encoding/encoding_test.go +++ b/pkg/storegateway/indexheader/encoding/encoding_test.go @@ -711,7 +711,7 @@ func TestDecbuf_Crc32(t *testing.T) { func createDecbufWithBytes(t testing.TB, b []byte) Decbuf { dir := t.TempDir() filePath := path.Join(dir, "test-file") - require.NoError(t, os.WriteFile(filePath, b, 0700)) + require.NoError(t, os.WriteFile(filePath, b, 0600)) reg := prometheus.NewPedanticRegistry() factory := NewDecbufFactory(filePath, 0, NewDecbufFactoryMetrics(reg)) diff --git a/pkg/util/process/collector_test.go b/pkg/util/process/collector_test.go index b38a1c04f85..279fee4fe85 100644 --- a/pkg/util/process/collector_test.go +++ b/pkg/util/process/collector_test.go @@ -29,8 +29,8 @@ func TestProcessCollector(t *testing.T) { require.NoError(t, os.MkdirAll(filepath.Dir(mapsPath), os.ModePerm)) require.NoError(t, os.MkdirAll(filepath.Dir(mapsLimitPath), os.ModePerm)) - require.NoError(t, os.WriteFile(mapsPath, []byte("1\n2\n3\n4\n5\n"), os.ModePerm)) - require.NoError(t, os.WriteFile(mapsLimitPath, []byte("262144\n"), os.ModePerm)) + require.NoError(t, os.WriteFile(mapsPath, []byte("1\n2\n3\n4\n5\n"), 0600)) + require.NoError(t, os.WriteFile(mapsLimitPath, []byte("262144\n"), 0600)) // Create a new collector and test metrics. c, err := newProcessCollector(pid, procDir) From 683dcbcb3729d0fe975a515cc282eba85958d62a Mon Sep 17 00:00:00 2001 From: Dimitar Dimitrov Date: Wed, 8 Jan 2025 17:25:44 +0200 Subject: [PATCH 3/7] Fix more tests ??? Signed-off-by: Dimitar Dimitrov --- pkg/storegateway/indexheader/encoding/factory_test.go | 2 +- pkg/storegateway/indexheader/encoding/reader_test.go | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/pkg/storegateway/indexheader/encoding/factory_test.go b/pkg/storegateway/indexheader/encoding/factory_test.go index 57e100bad8d..4fb6daad81d 100644 --- a/pkg/storegateway/indexheader/encoding/factory_test.go +++ b/pkg/storegateway/indexheader/encoding/factory_test.go @@ -220,7 +220,7 @@ func createDecbufFactoryWithBytes(t testing.TB, filePoolSize uint, len int, enc dir := t.TempDir() filePath := path.Join(dir, "test-file") - require.NoError(t, os.WriteFile(filePath, bytes, 0700)) + require.NoError(t, os.WriteFile(filePath, bytes, 0600)) reg := prometheus.NewPedanticRegistry() factory := NewDecbufFactory(filePath, filePoolSize, NewDecbufFactoryMetrics(reg)) diff --git a/pkg/storegateway/indexheader/encoding/reader_test.go b/pkg/storegateway/indexheader/encoding/reader_test.go index b42f552e43e..10ce2c50668 100644 --- a/pkg/storegateway/indexheader/encoding/reader_test.go +++ b/pkg/storegateway/indexheader/encoding/reader_test.go @@ -231,7 +231,7 @@ func testReaders(t *testing.T, test func(t *testing.T, r *fileReader)) { t.Run("FileReaderWithZeroOffset", func(t *testing.T) { dir := t.TempDir() filePath := path.Join(dir, "test-file") - require.NoError(t, os.WriteFile(filePath, testReaderContents, 0700)) + require.NoError(t, os.WriteFile(filePath, testReaderContents, 0600)) f, err := os.Open(filePath) require.NoError(t, err) @@ -251,7 +251,7 @@ func testReaders(t *testing.T, test func(t *testing.T, r *fileReader)) { dir := t.TempDir() filePath := path.Join(dir, "test-file") - require.NoError(t, os.WriteFile(filePath, fileBytes, 0700)) + require.NoError(t, os.WriteFile(filePath, fileBytes, 0600)) f, err := os.Open(filePath) require.NoError(t, err) From bdd8bdd82cb4414550362ca765683f6a366e52d3 Mon Sep 17 00:00:00 2001 From: Dimitar Dimitrov Date: Wed, 8 Jan 2025 17:44:55 +0200 Subject: [PATCH 4/7] Fix even more tests why wasn't this in the output of the liner the first time ??? Signed-off-by: Dimitar Dimitrov --- pkg/alertmanager/alertstore/local/store_test.go | 14 +++++++------- pkg/ingester/ingester_test.go | 8 ++++---- pkg/ingester/shipper_test.go | 4 ++-- pkg/ruler/rulestore/local/local_test.go | 2 +- .../indexheader/encoding/reader_test.go | 2 +- pkg/storegateway/indexheader/index/symbols_test.go | 4 ++-- 6 files changed, 17 insertions(+), 17 deletions(-) diff --git a/pkg/alertmanager/alertstore/local/store_test.go b/pkg/alertmanager/alertstore/local/store_test.go index 8db63594cab..a90d90fbda7 100644 --- a/pkg/alertmanager/alertstore/local/store_test.go +++ b/pkg/alertmanager/alertstore/local/store_test.go @@ -33,11 +33,11 @@ func TestStore_ListAllUsers(t *testing.T) { { user1Cfg := prepareAlertmanagerConfig("user-1") user2Cfg := prepareAlertmanagerConfig("user-2") - require.NoError(t, os.WriteFile(filepath.Join(storeDir, "user-1.yaml"), []byte(user1Cfg), os.ModePerm)) - require.NoError(t, os.WriteFile(filepath.Join(storeDir, "user-2.yaml"), []byte(user2Cfg), os.ModePerm)) + require.NoError(t, os.WriteFile(filepath.Join(storeDir, "user-1.yaml"), []byte(user1Cfg), 0600)) + require.NoError(t, os.WriteFile(filepath.Join(storeDir, "user-2.yaml"), []byte(user2Cfg), 0600)) // The following file is expected to be skipped. - require.NoError(t, os.WriteFile(filepath.Join(storeDir, "user-3.unsupported-extension"), []byte{}, os.ModePerm)) + require.NoError(t, os.WriteFile(filepath.Join(storeDir, "user-3.unsupported-extension"), []byte{}, 0600)) users, err := store.ListAllUsers(ctx) require.NoError(t, err) @@ -59,8 +59,8 @@ func TestStore_GetAlertConfig(t *testing.T) { { user1Cfg := prepareAlertmanagerConfig("user-1") user2Cfg := prepareAlertmanagerConfig("user-2") - require.NoError(t, os.WriteFile(filepath.Join(storeDir, "user-1.yaml"), []byte(user1Cfg), os.ModePerm)) - require.NoError(t, os.WriteFile(filepath.Join(storeDir, "user-2.yaml"), []byte(user2Cfg), os.ModePerm)) + require.NoError(t, os.WriteFile(filepath.Join(storeDir, "user-1.yaml"), []byte(user1Cfg), 0600)) + require.NoError(t, os.WriteFile(filepath.Join(storeDir, "user-2.yaml"), []byte(user2Cfg), 0600)) config, err := store.GetAlertConfig(ctx, "user-1") require.NoError(t, err) @@ -86,7 +86,7 @@ func TestStore_GetAlertConfigs(t *testing.T) { // The storage contains some configs. { user1Cfg := prepareAlertmanagerConfig("user-1") - require.NoError(t, os.WriteFile(filepath.Join(storeDir, "user-1.yaml"), []byte(user1Cfg), os.ModePerm)) + require.NoError(t, os.WriteFile(filepath.Join(storeDir, "user-1.yaml"), []byte(user1Cfg), 0600)) configs, err := store.GetAlertConfigs(ctx, []string{"user-1", "user-2"}) require.NoError(t, err) @@ -96,7 +96,7 @@ func TestStore_GetAlertConfigs(t *testing.T) { // Add another user config. user2Cfg := prepareAlertmanagerConfig("user-2") - require.NoError(t, os.WriteFile(filepath.Join(storeDir, "user-2.yaml"), []byte(user2Cfg), os.ModePerm)) + require.NoError(t, os.WriteFile(filepath.Join(storeDir, "user-2.yaml"), []byte(user2Cfg), 0600)) configs, err = store.GetAlertConfigs(ctx, []string{"user-1", "user-2"}) require.NoError(t, err) diff --git a/pkg/ingester/ingester_test.go b/pkg/ingester/ingester_test.go index 96ebf4f6165..a753577b5ba 100644 --- a/pkg/ingester/ingester_test.go +++ b/pkg/ingester/ingester_test.go @@ -6386,10 +6386,10 @@ func TestIngester_OpenExistingTSDBOnStartup(t *testing.T) { setup: func(t *testing.T, dir string) { // Create a fake TSDB on disk with an empty chunks head segment file (it's invalid unless // it's the last one and opening TSDB should fail). - require.NoError(t, os.MkdirAll(filepath.Join(dir, "user0", "wal", ""), 0700)) - require.NoError(t, os.MkdirAll(filepath.Join(dir, "user0", "chunks_head", ""), 0700)) - require.NoError(t, os.WriteFile(filepath.Join(dir, "user0", "chunks_head", "00000001"), nil, 0700)) - require.NoError(t, os.WriteFile(filepath.Join(dir, "user0", "chunks_head", "00000002"), nil, 0700)) + require.NoError(t, os.MkdirAll(filepath.Join(dir, "user0", "wal", ""), 0600)) + require.NoError(t, os.MkdirAll(filepath.Join(dir, "user0", "chunks_head", ""), 0600)) + require.NoError(t, os.WriteFile(filepath.Join(dir, "user0", "chunks_head", "00000001"), nil, 0600)) + require.NoError(t, os.WriteFile(filepath.Join(dir, "user0", "chunks_head", "00000002"), nil, 0600)) require.NoError(t, os.MkdirAll(filepath.Join(dir, "user1", "dummy"), 0700)) }, diff --git a/pkg/ingester/shipper_test.go b/pkg/ingester/shipper_test.go index 3bcc94a4d41..ef23b3ef8a9 100644 --- a/pkg/ingester/shipper_test.go +++ b/pkg/ingester/shipper_test.go @@ -292,9 +292,9 @@ func TestShipperAddsSegmentFiles(t *testing.T) { }, }, }.WriteToDir(log.NewNopLogger(), path.Join(dir, id.String()))) - require.NoError(t, os.WriteFile(filepath.Join(blockDir, "index"), []byte("index file"), 0666)) + require.NoError(t, os.WriteFile(filepath.Join(blockDir, "index"), []byte("index file"), 0600)) segmentFile := "00001" - require.NoError(t, os.WriteFile(filepath.Join(chunksDir, segmentFile), []byte("hello world"), 0666)) + require.NoError(t, os.WriteFile(filepath.Join(chunksDir, segmentFile), []byte("hello world"), 0600)) uploaded, err := s.Sync(context.Background()) require.NoError(t, err) diff --git a/pkg/ruler/rulestore/local/local_test.go b/pkg/ruler/rulestore/local/local_test.go index 5adf293c422..7fc041c10de 100644 --- a/pkg/ruler/rulestore/local/local_test.go +++ b/pkg/ruler/rulestore/local/local_test.go @@ -56,7 +56,7 @@ func TestClient_LoadRuleGroups(t *testing.T) { err = os.Symlink(user1, path.Join(dir, user2)) require.NoError(t, err) - err = os.WriteFile(path.Join(dir, user1, namespace1), b, 0777) + err = os.WriteFile(path.Join(dir, user1, namespace1), b, 0600) require.NoError(t, err) const ignoredDir = "ignored-dir" diff --git a/pkg/storegateway/indexheader/encoding/reader_test.go b/pkg/storegateway/indexheader/encoding/reader_test.go index 10ce2c50668..0c6f6ca5917 100644 --- a/pkg/storegateway/indexheader/encoding/reader_test.go +++ b/pkg/storegateway/indexheader/encoding/reader_test.go @@ -210,7 +210,7 @@ func TestReaders_CreationWithEmptyContents(t *testing.T) { t.Run("fileReader", func(t *testing.T) { dir := t.TempDir() filePath := path.Join(dir, "test-file") - require.NoError(t, os.WriteFile(filePath, nil, 0700)) + require.NoError(t, os.WriteFile(filePath, nil, 0600)) f, err := os.Open(filePath) require.NoError(t, err) diff --git a/pkg/storegateway/indexheader/index/symbols_test.go b/pkg/storegateway/indexheader/index/symbols_test.go index 81d1fc42a56..0215e3e1c46 100644 --- a/pkg/storegateway/indexheader/index/symbols_test.go +++ b/pkg/storegateway/indexheader/index/symbols_test.go @@ -64,7 +64,7 @@ func TestSymbolsV1(t *testing.T) { dir := t.TempDir() filePath := path.Join(dir, "index") - require.NoError(t, os.WriteFile(filePath, buf.Get(), 0700)) + require.NoError(t, os.WriteFile(filePath, buf.Get(), 0600)) reg := prometheus.NewPedanticRegistry() df := streamencoding.NewDecbufFactory(filePath, 0, streamencoding.NewDecbufFactoryMetrics(reg)) @@ -166,7 +166,7 @@ func TestSymbolsV2(t *testing.T) { dir := t.TempDir() filePath := path.Join(dir, "index") - require.NoError(t, os.WriteFile(filePath, buf.Get(), 0700)) + require.NoError(t, os.WriteFile(filePath, buf.Get(), 0600)) reg := prometheus.NewPedanticRegistry() df := streamencoding.NewDecbufFactory(filePath, 0, streamencoding.NewDecbufFactoryMetrics(reg)) From 463fa813ad78a68deec118750406d9485b15bd9c Mon Sep 17 00:00:00 2001 From: Dimitar Dimitrov Date: Wed, 8 Jan 2025 17:45:51 +0200 Subject: [PATCH 5/7] More tests mate Signed-off-by: Dimitar Dimitrov --- pkg/alertmanager/multitenant_test.go | 2 +- pkg/ingester/ingester_test.go | 8 ++++---- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/pkg/alertmanager/multitenant_test.go b/pkg/alertmanager/multitenant_test.go index 92b83ee3f7e..f61052b7733 100644 --- a/pkg/alertmanager/multitenant_test.go +++ b/pkg/alertmanager/multitenant_test.go @@ -2492,7 +2492,7 @@ func TestMultitenantAlertmanager_computeFallbackConfig(t *testing.T) { // If a fallback configuration file is set, it returns its content. configDir := t.TempDir() configFile := filepath.Join(configDir, "test.yaml") - err = os.WriteFile(configFile, []byte(simpleConfigOne), 0664) + err = os.WriteFile(configFile, []byte(simpleConfigOne), 0600) assert.NoError(t, err) fallbackConfig, err = ComputeFallbackConfig(configFile) diff --git a/pkg/ingester/ingester_test.go b/pkg/ingester/ingester_test.go index a753577b5ba..8b043cf93ef 100644 --- a/pkg/ingester/ingester_test.go +++ b/pkg/ingester/ingester_test.go @@ -6410,10 +6410,10 @@ func TestIngester_OpenExistingTSDBOnStartup(t *testing.T) { // Create a fake TSDB on disk with an empty chunks head segment file (it's invalid unless // it's the last one and opening TSDB should fail). - require.NoError(t, os.MkdirAll(filepath.Join(dir, "user2", "wal", ""), 0700)) - require.NoError(t, os.MkdirAll(filepath.Join(dir, "user2", "chunks_head", ""), 0700)) - require.NoError(t, os.WriteFile(filepath.Join(dir, "user2", "chunks_head", "00000001"), nil, 0700)) - require.NoError(t, os.WriteFile(filepath.Join(dir, "user2", "chunks_head", "00000002"), nil, 0700)) + require.NoError(t, os.MkdirAll(filepath.Join(dir, "user2", "wal", ""), 0600)) + require.NoError(t, os.MkdirAll(filepath.Join(dir, "user2", "chunks_head", ""), 0600)) + require.NoError(t, os.WriteFile(filepath.Join(dir, "user2", "chunks_head", "00000001"), nil, 0600)) + require.NoError(t, os.WriteFile(filepath.Join(dir, "user2", "chunks_head", "00000002"), nil, 0600)) }, check: func(t *testing.T, i *Ingester) { for _, userID := range []string{"user0", "user1"} { From a5d4a1c143752804d8eab389ece6c526f4915ef3 Mon Sep 17 00:00:00 2001 From: Dimitar Dimitrov Date: Wed, 8 Jan 2025 17:49:56 +0200 Subject: [PATCH 6/7] i'm speechless Signed-off-by: Dimitar Dimitrov --- integration/util.go | 2 +- pkg/compactor/bucket_compactor_e2e_test.go | 2 +- pkg/storegateway/indexheader/lazy_binary_reader_test.go | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/integration/util.go b/integration/util.go index 93b61e6750d..4f3bcbda45e 100644 --- a/integration/util.go +++ b/integration/util.go @@ -87,7 +87,7 @@ func writeFileToSharedDir(s *e2e.Scenario, dst string, content []byte) error { return os.WriteFile( dst, content, - os.ModePerm) + 0600) } func copyFileToSharedDir(s *e2e.Scenario, src, dst string) error { diff --git a/pkg/compactor/bucket_compactor_e2e_test.go b/pkg/compactor/bucket_compactor_e2e_test.go index ded4174523d..b555f5e3736 100644 --- a/pkg/compactor/bucket_compactor_e2e_test.go +++ b/pkg/compactor/bucket_compactor_e2e_test.go @@ -614,7 +614,7 @@ func createEmptyBlock(dir string, mint, maxt int64, extLset labels.Labels, resol return ulid.ULID{}, err } - if err := os.WriteFile(path.Join(dir, uid.String(), "meta.json"), b, os.ModePerm); err != nil { + if err := os.WriteFile(path.Join(dir, uid.String(), "meta.json"), b, 0600); err != nil { return ulid.ULID{}, errors.Wrap(err, "saving meta.json") } diff --git a/pkg/storegateway/indexheader/lazy_binary_reader_test.go b/pkg/storegateway/indexheader/lazy_binary_reader_test.go index 13e79f92b5e..e8a718fb533 100644 --- a/pkg/storegateway/indexheader/lazy_binary_reader_test.go +++ b/pkg/storegateway/indexheader/lazy_binary_reader_test.go @@ -77,7 +77,7 @@ func TestNewLazyBinaryReader_ShouldRebuildCorruptedIndexHeader(t *testing.T) { // Write a corrupted index-header for the block. headerFilename := filepath.Join(tmpDir, blockID.String(), block.IndexHeaderFilename) - require.NoError(t, os.WriteFile(headerFilename, []byte("xxx"), os.ModePerm)) + require.NoError(t, os.WriteFile(headerFilename, []byte("xxx"), 0600)) testLazyBinaryReader(t, bkt, tmpDir, blockID, func(t *testing.T, r *LazyBinaryReader, err error) { require.NoError(t, err) From 6fa1d48235458e9a21784a9220697c375876ab87 Mon Sep 17 00:00:00 2001 From: Dimitar Dimitrov Date: Thu, 9 Jan 2025 10:41:21 +0200 Subject: [PATCH 7/7] Update CHANGELOG.md entry Signed-off-by: Dimitar Dimitrov --- CHANGELOG.md | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c66684fabc7..b87f7943a45 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,10 +7,10 @@ ### Grafana Mimir * [CHANGE] Distributor: OTLP and push handler replace all non-UTF8 characters with the unicode replacement character `\uFFFD` in error messages before propagating them. #10236 -* [CHANGE] Make default directory file permissions more restrictive: #10379 +* [CHANGE] Make default directory file permissions more restrictive: #10379 #10380 * Directories are only read-write-executable by the Mimir user and readable by users in the same group. * Files are read-writable only by the Mimir user. - * The following files and directories are affected: Alertmanager data directory, blocks exported by `mimirtool remote-read export`, locally cached meta files in the compactor, and index headers in the store-gateway. + * The following files and directories are affected: Alertmanager data directory, blocks exported by `mimirtool remote-read export`, rules written by `mimirtool rules get` and `mimirtool rules print`, locally cached meta files in the compactor, and index headers in the store-gateway. * [CHANGE] Querier: pass query matchers to queryable `IsApplicable` hook. #10256 * [ENHANCEMENT] Query Frontend: Return server-side `samples_processed` statistics. #10103 * [ENHANCEMENT] Distributor: OTLP receiver now converts also metric metadata. See also https://github.com/prometheus/prometheus/pull/15416. #10168 @@ -41,7 +41,7 @@ ### Mimirtool -* [CHANGE] Blocks exported by `mimirtool remote-read export` are now only read-writable by the Mimir user. #10379 +* [CHANGE] Blocks exported by `mimirtool remote-read export`, rules written by `mimirtool rules get` and `mimirtool rules print` are now only read-writable by the Mimir user. #10379 #10380 * [BUGFIX] Fix issue where `MIMIR_HTTP_PREFIX` environment variable was ignored and the value from `MIMIR_MIMIR_HTTP_PREFIX` was used instead. #10207 * [ENHANCEMENT] Unify mimirtool authentication options and add extra-headers support for commands that depend on MimirClient. #10178