From b130c0ceb2bd8ecd6fe44c0ef16ae8e6e0fe0a09 Mon Sep 17 00:00:00 2001 From: sneak Date: Wed, 7 Oct 2026 11:08:57 +0000 Subject: [PATCH] Fix two misleading messages (closes #240) The warning for a config file that others can read always said the file contained S3 credentials, so a file:// config with none got a false claim. When s3.access_key_id or s3.secret_access_key is set it now says the file may contain them, because Load sees the values only after smartconfig has replaced any ${...} reference, so a set credential need not be in the file. Otherwise it says the file is readable by others. snapshot purge wrapped the listing error, which already starts with "listing remote snapshots:", in that prefix a second time. syncWithRemote now returns it unwrapped, as CleanupLocalSnapshots does. Model: opus-5-5 --- TODO.md | 11 ++ internal/config/config.go | 14 ++- internal/config/config_test.go | 123 +++++++++++++++++++ internal/vaultik/missing_destination_test.go | 21 ++++ internal/vaultik/snapshot.go | 2 +- 5 files changed, 169 insertions(+), 2 deletions(-) diff --git a/TODO.md b/TODO.md index 1a1b960..2f33a00 100644 --- a/TODO.md +++ b/TODO.md @@ -22,6 +22,17 @@ the tag exists and is exercised; what is left is merging `next` to # Completed Steps +- 2026-10-07: Made two messages say only what is true + ([issue #240](https://git.eeqj.de/sneak/vaultik/issues/240)). A config + file that others can read was warned about as containing S3 + credentials even when it set none, as a `file://` config does. The + warning now says the file may contain S3 credentials only when + `s3.access_key_id` or `s3.secret_access_key` is set, since either may + come from a `${...}` reference rather than the file, and otherwise + says the file is readable by others. `snapshot purge` against a + destination store it could not list gave an error with + `listing remote snapshots:` in it twice; the prefix now appears once. + - 2026-10-07: Made `s3.part_size` set the multipart upload part size ([issue #232](https://git.eeqj.de/sneak/vaultik/issues/232)). It was loaded and defaulted but never passed to the S3 client, whose uploader diff --git a/internal/config/config.go b/internal/config/config.go index e5192b0..e8527d7 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -305,7 +305,7 @@ func Load(path string) (*Config, error) { if statErr == nil { mode := info.Mode().Perm() if mode&0044 != 0 { // group or world readable - log.Warn("Config file has insecure permissions (contains S3 credentials)", + log.Warn(cfg.readableByOthersWarning(), "path", path, "mode", fmt.Sprintf("%04o", mode), "recommendation", "chmod 600 "+path) @@ -418,6 +418,18 @@ func (c *Config) setAgeSecretKey() { } } +// readableByOthersWarning is the warning Load logs when others can read +// the config file. It says "may contain" because the S3 credentials are +// seen only after smartconfig has replaced any ${...} reference in the +// file with its value, so a set credential need not be in the file. +func (c *Config) readableByOthersWarning() string { + if c.S3.AccessKeyID != "" || c.S3.SecretAccessKey != "" { + return "Config file is readable by others and may contain S3 credentials" + } + + return "Config file is readable by others" +} + // validateStorage validates storage configuration. // If StorageURL is set, it takes precedence. S3 URLs require credentials. // File URLs don't require any S3 configuration. diff --git a/internal/config/config_test.go b/internal/config/config_test.go index c9bb10d..18d0b20 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -8,6 +8,7 @@ import ( "testing" "sneak.berlin/go/vaultik/internal/chunker" + "sneak.berlin/go/vaultik/internal/log" ) const ( @@ -459,3 +460,125 @@ func TestAgeSecretKeySourceName(t *testing.T) { }) } } + +// loadReadableConfig writes configYAML to a file that others can read, +// loads it, and returns what the logger wrote to stderr meanwhile. The +// logger writes to the os.Stderr it finds when it is initialized, so +// os.Stderr is pointed at a file first. Not parallel-safe: os.Stderr and +// the logger are process-global. +func loadReadableConfig(t *testing.T, configYAML string) string { + t.Helper() + + dir := t.TempDir() + configPath := filepath.Join(dir, "config.yml") + stderrPath := filepath.Join(dir, "stderr") + + err := os.WriteFile(configPath, []byte(configYAML), 0o600) + if err != nil { + t.Fatalf("writing config: %v", err) + } + + //nolint:gosec // G302: the test needs a config file others can read + err = os.Chmod(configPath, 0o644) + if err != nil { + t.Fatalf("chmod config: %v", err) + } + + stderrFile, err := os.Create(stderrPath) //nolint:gosec // G304: test temp path + if err != nil { + t.Fatalf("creating stderr file: %v", err) + } + + previous := os.Stderr + os.Stderr = stderrFile + + log.Initialize(log.Config{}) + + _, loadErr := Load(configPath) + + os.Stderr = previous + + log.Initialize(log.Config{}) + + _ = stderrFile.Close() + + if loadErr != nil { + t.Fatalf("Load() error = %v", loadErr) + } + + captured, err := os.ReadFile(stderrPath) //nolint:gosec // G304: test temp path + if err != nil { + t.Fatalf("reading stderr file: %v", err) + } + + return string(captured) +} + +// TestLoadWarnsReadableConfigWithoutS3Credentials checks that a config +// file others can read, holding no S3 credentials, is warned about +// without a claim that it holds them. +// +//nolint:paralleltest // loadReadableConfig replaces os.Stderr +func TestLoadWarnsReadableConfigWithoutS3Credentials(t *testing.T) { + stderr := loadReadableConfig(t, ` +storage_url: file:///var/backups/vaultik +snapshots: + home: + paths: + - /home +`) + + if !strings.Contains(stderr, "Config file is readable by others") { + t.Errorf("expected a warning that the file is readable by others, got %q", + stderr) + } + + if strings.Contains(stderr, "S3 credentials") { + t.Errorf("warning names S3 credentials the file does not set: %q", stderr) + } +} + +// TestLoadWarnsReadableConfigWithS3Credentials checks that a config file +// others can read and that sets S3 credentials, as values or as ${ENV:...} +// references, is warned about as one that may contain them. +// +//nolint:paralleltest // loadReadableConfig replaces os.Stderr +func TestLoadWarnsReadableConfigWithS3Credentials(t *testing.T) { + t.Setenv("VAULTIK_TEST_ACCESS_KEY_ID", "test-access-key") + t.Setenv("VAULTIK_TEST_SECRET_ACCESS_KEY", "test-secret-key") + + configs := map[string]string{ + "values": ` +storage_url: s3://bucket/prefix?endpoint=s3.example.com +s3: + access_key_id: test-access-key + secret_access_key: test-secret-key +snapshots: + home: + paths: + - /home +`, + "references": ` +storage_url: s3://bucket/prefix?endpoint=s3.example.com +s3: + access_key_id: ${ENV:VAULTIK_TEST_ACCESS_KEY_ID} + secret_access_key: ${ENV:VAULTIK_TEST_SECRET_ACCESS_KEY} +snapshots: + home: + paths: + - /home +`, + } + + for name, configYAML := range configs { + t.Run(name, func(t *testing.T) { + stderr := loadReadableConfig(t, configYAML) + + if !strings.Contains(stderr, + "Config file is readable by others and may contain S3 credentials") { + t.Errorf("expected a warning naming the S3 credentials, got %q", + stderr) + } + }) + } +} diff --git a/internal/vaultik/missing_destination_test.go b/internal/vaultik/missing_destination_test.go index 3869bc7..cc3affa 100644 --- a/internal/vaultik/missing_destination_test.go +++ b/internal/vaultik/missing_destination_test.go @@ -6,6 +6,7 @@ import ( "io/fs" "os" "path/filepath" + "strings" "testing" "github.com/spf13/afero" @@ -153,3 +154,23 @@ func TestPruneKeepsLocalRecordsWhenDestinationMissing(t *testing.T) { require.NoError(t, err) assert.Len(t, snapshots, 1, "prune must delete no local snapshot record") } + +// TestPurgeSaysListingFailedOnceWhenDestinationMissing checks that +// snapshot purge fails on a destination it cannot list, with an error +// that says "listing remote snapshots" once. +// +//nolint:paralleltest // installs the global logger via log.Initialize +func TestPurgeSaysListingFailedOnceWhenDestinationMissing(t *testing.T) { + log.Initialize(log.Config{}) + + ctx := context.Background() + v, _, _ := backUpThenUnplug(ctx, t) + + err := v.PurgeSnapshotsWithOptions(&vaultik.SnapshotPurgeOptions{ + KeepLatest: true, + Force: true, + }) + require.ErrorIs(t, err, fs.ErrNotExist) + assert.Equal(t, 1, strings.Count(err.Error(), "listing remote snapshots"), + err.Error()) +} diff --git a/internal/vaultik/snapshot.go b/internal/vaultik/snapshot.go index 2707516..43ab3e1 100644 --- a/internal/vaultik/snapshot.go +++ b/internal/vaultik/snapshot.go @@ -1052,7 +1052,7 @@ func (v *Vaultik) syncWithRemote() error { // every local snapshot record (issue #160). remoteKeys, err := v.listAllRemoteSnapshotKeys() if err != nil { - return fmt.Errorf("listing remote snapshots: %w", err) + return err } remoteKeySet := make(map[string]bool, len(remoteKeys))