From f2138dafc8322b3e9888052611dfc75c6e6cbb9d Mon Sep 17 00:00:00 2001 From: sneak Date: Wed, 11 Nov 2026 04:24:28 +0000 Subject: [PATCH] Name the S3 keys when storage_url moves to s3:// first (closes #428) On a config that loads, `config set storage_url s3://...` is refused until `s3.access_key_id` and `s3.secret_access_key` are set. The refusal now says to set those two keys first, then `storage_url`. Model: opus-5-5 --- internal/cli/config.go | 23 ++++++++- internal/cli/config_test.go | 94 +++++++++++++++++++++++++++++++++++++ internal/config/config.go | 17 ++++--- 3 files changed, 127 insertions(+), 7 deletions(-) diff --git a/internal/cli/config.go b/internal/cli/config.go index ccfd944..fe0face 100644 --- a/internal/cli/config.go +++ b/internal/cli/config.go @@ -49,8 +49,14 @@ var ( errWouldNotLoad = errors.New( "vaultik would not load the config after this change, " + "so the file was not changed") + errSetS3KeysFirst = errors.New( + "set s3.access_key_id and s3.secret_access_key first, " + + "then storage_url") ) +// storageURLKey is the config key that selects the storage backend. +const storageURLKey = "storage_url" + // macOS is the value of runtime.GOOS on macOS. const macOS = "darwin" @@ -513,7 +519,7 @@ func writeConfigSet(out *ui.Writer, path, key, value string) error { err = checkConfigChange(oldData, data, settingData) if err != nil { - return fmt.Errorf("%w: %w", errWouldNotLoad, err) + return wouldNotLoadError(key, err) } err = os.WriteFile(path, data, configFileMode) @@ -537,6 +543,21 @@ func writeConfigSet(out *ui.Writer, path, key, value string) error { return nil } +// wouldNotLoadError returns the refusal of a change to key that vaultik +// would not load for reason. When storage_url moves to s3:// before the S3 +// credentials are set, it says to set them first: they can be set while +// the old storage_url is still in place. +func wouldNotLoadError(key string, reason error) error { + if key == storageURLKey && + (errors.Is(reason, config.ErrS3KeyIDRequired) || + errors.Is(reason, config.ErrS3SecretRequired)) { + return fmt.Errorf("%w: %w; %w", errWouldNotLoad, reason, + errSetS3KeysFirst) + } + + return fmt.Errorf("%w: %w", errWouldNotLoad, reason) +} + // checkConfigChange returns the reason vaultik would not load newData, the // config after a change, if the change brings that failure in. If oldData, // the config before it, loads, the new one must load too. A config that diff --git a/internal/cli/config_test.go b/internal/cli/config_test.go index 6eb45cb..37bae2e 100644 --- a/internal/cli/config_test.go +++ b/internal/cli/config_test.go @@ -2,6 +2,7 @@ package cli //nolint:testpackage // exercises unexported yamlPathGet/yamlPathSet import ( "bytes" + "errors" "os" "path/filepath" "strconv" @@ -152,6 +153,21 @@ snapshots: - "~" ` +// testS3URL is an S3 storage_url, which needs credentials to load. +const testS3URL = "s3://bucket/prefix?endpoint=https://s3.example.com" + +// configWithS3Keys loads: its storage_url is testS3URL and both S3 keys are +// set. +const configWithS3Keys = storageURLKey + ": " + testS3URL + ` +s3: + access_key_id: AKIAEXAMPLE + secret_access_key: example-secret +snapshots: + home: + paths: + - "~" +` + // configWithUnsetReference loads only where VAULTIK_TEST_UNSET_SECRET is // set, as a backup job's environment might set it. No test sets it. const configWithUnsetReference = configWithoutRecipients + `s3: @@ -258,6 +274,14 @@ func TestConfigSetKeepsFileThatWouldNotLoad(t *testing.T) { "compression_level must be between 1 and 19"}, {configWithoutRecipients, "chunk_size", "1KB", "chunk_size must be at least 1MB"}, + // An s3:// URL before its credentials names them. + {configWithoutRecipients, storageURLKey, testS3URL, + "s3.access_key_id is required for s3:// URLs; " + + "set s3.access_key_id and s3.secret_access_key first"}, + {configWithoutRecipients + "s3:\n access_key_id: AKIAEXAMPLE\n", + storageURLKey, testS3URL, + "s3.secret_access_key is required for s3:// URLs; " + + "set s3.access_key_id and s3.secret_access_key first"}, // With its reference filled in, the config loads. {levelReference, "blob_size_limit", "1MB", "blob_size_limit must be at least"}, @@ -343,6 +367,76 @@ func TestConfigSetRefusesOnlyItsOwnFailure(t *testing.T) { } } +// TestConfigSetS3KeysBeforeStorageURL checks that the order the refusal +// gives works on a config that loads with a file:// storage_url. +func TestConfigSetS3KeysBeforeStorageURL(t *testing.T) { + t.Parallel() + + path := filepath.Join(t.TempDir(), "config.yml") + + err := os.WriteFile(path, []byte(configWithoutRecipients), configFileMode) + if err != nil { + t.Fatalf("write config: %v", err) + } + + for _, set := range [][2]string{ + {"s3.access_key_id", "AKIAEXAMPLE"}, + {"s3.secret_access_key", "example-secret"}, + {storageURLKey, testS3URL}, + } { + err = writeConfigSet(ui.NewWithColor(&bytes.Buffer{}, false), + path, set[0], set[1]) + if err != nil { + t.Fatalf("config set %s: %v", set[0], err) + } + } + + data, err := os.ReadFile(path) //nolint:gosec // G304: test-controlled path + if err != nil { + t.Fatalf("read config: %v", err) + } + + if !strings.Contains(string(data), storageURLKey+": "+testS3URL) { + t.Errorf("config set storage_url wrote:\n%s", data) + } +} + +// TestConfigSetRefusesClearingS3Key checks that clearing s3.access_key_id on +// a config with an s3:// storage_url is refused without saying to set the +// S3 keys first. That advice is only for a change to storage_url. +func TestConfigSetRefusesClearingS3Key(t *testing.T) { + t.Parallel() + + const key = "s3.access_key_id" + + path := filepath.Join(t.TempDir(), "config.yml") + + err := os.WriteFile(path, []byte(configWithS3Keys), configFileMode) + if err != nil { + t.Fatalf("write config: %v", err) + } + + err = writeConfigSet(ui.NewWithColor(&bytes.Buffer{}, false), path, key, "") + if !errors.Is(err, errWouldNotLoad) { + t.Errorf("clearing %s gives error %v, want one saying %q", + key, err, errWouldNotLoad) + } + + if errors.Is(err, errSetS3KeysFirst) { + t.Errorf("clearing %s gives error %v, want one not saying %q", + key, err, errSetS3KeysFirst) + } + + data, err := os.ReadFile(path) //nolint:gosec // G304: test-controlled path + if err != nil { + t.Fatalf("read config: %v", err) + } + + if string(data) != configWithS3Keys { + t.Errorf("clearing %s changed the file to:\n%s", key, data) + } +} + // TestConfigSetWritesEnvSection checks that config set writes a key under // the top-level env section, whose keys are not settings, on a config that // loads. diff --git a/internal/config/config.go b/internal/config/config.go index a615400..b842abc 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -79,8 +79,13 @@ var ( "storage not configured; set storage_url or provide s3.endpoint + " + "s3.bucket + credentials") errS3BucketRequired = errors.New("s3.bucket is required (or set storage_url)") - errS3KeyIDRequired = errors.New("s3.access_key_id is required") - errS3SecretRequired = errors.New("s3.secret_access_key is required") +) + +// Validate returns these when S3 storage is configured without its +// credentials. +var ( + ErrS3KeyIDRequired = errors.New("s3.access_key_id is required") + ErrS3SecretRequired = errors.New("s3.secret_access_key is required") ) // expandTilde expands ~ at the start of a path to the user's home directory. @@ -522,11 +527,11 @@ func (c *Config) validateStorage() error { } if c.S3.AccessKeyID == "" { - return errS3KeyIDRequired + return ErrS3KeyIDRequired } if c.S3.SecretAccessKey == "" { - return errS3SecretRequired + return ErrS3SecretRequired } return nil @@ -553,11 +558,11 @@ func (c *Config) validateStorageURL() error { case strings.HasPrefix(c.StorageURL, "s3://"): // S3 storage needs credentials if c.S3.AccessKeyID == "" { - return fmt.Errorf("%w for s3:// URLs", errS3KeyIDRequired) + return fmt.Errorf("%w for s3:// URLs", ErrS3KeyIDRequired) } if c.S3.SecretAccessKey == "" { - return fmt.Errorf("%w for s3:// URLs", errS3SecretRequired) + return fmt.Errorf("%w for s3:// URLs", ErrS3SecretRequired) } return nil -- 2.54.0