diff --git a/TODO.md b/TODO.md index e8dd4f6..90614f5 100644 --- a/TODO.md +++ b/TODO.md @@ -22,6 +22,16 @@ 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 names the credentials only when `s3.access_key_id` or + `s3.secret_access_key` is set, 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 per-name retention work when the hostname contains `_` ([issue #230](https://git.eeqj.de/sneak/vaultik/issues/230)). A snapshot ID is `hostname_name_timestamp`, and the name was read as diff --git a/internal/config/config.go b/internal/config/config.go index 1448a32..c5b3d7b 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -304,7 +304,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) @@ -411,6 +411,16 @@ func (c *Config) setAgeSecretKey() { } } +// readableByOthersWarning is the warning Load logs when others can read +// the config file. It names the S3 credentials only when they are set. +func (c *Config) readableByOthersWarning() string { + if c.S3.AccessKeyID != "" || c.S3.SecretAccessKey != "" { + return "Config file contains S3 credentials and is readable by others" + } + + 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 52b46ad..bac5b90 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 ( @@ -343,3 +344,102 @@ 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 is warned about as holding the S3 credentials it sets. +// +//nolint:paralleltest // loadReadableConfig replaces os.Stderr +func TestLoadWarnsReadableConfigWithS3Credentials(t *testing.T) { + stderr := loadReadableConfig(t, ` +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 +`) + + if !strings.Contains(stderr, + "Config file contains S3 credentials and is readable by others") { + 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))