From 45d758e08e541f95ab5344bd439e06c7f9ce022f 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 | 45 +++++++++++++++++++++++++++++++++++++ internal/config/config.go | 17 +++++++++----- 3 files changed, 78 insertions(+), 7 deletions(-) diff --git a/internal/cli/config.go b/internal/cli/config.go index 806a78a..ef78f18 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" @@ -512,7 +518,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) @@ -536,6 +542,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..b2c5b57 100644 --- a/internal/cli/config_test.go +++ b/internal/cli/config_test.go @@ -152,6 +152,9 @@ snapshots: - "~" ` +// testS3URL is an S3 storage_url, which needs credentials to load. +const testS3URL = "s3://bucket/prefix?endpoint=https://s3.example.com" + // 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 +261,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 +354,40 @@ 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) + } +} + // 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 8b79f88..5e05642 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -76,8 +76,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. @@ -513,11 +518,11 @@ func (c *Config) validateStorage() error { } if c.S3.AccessKeyID == "" { - return errS3KeyIDRequired + return ErrS3KeyIDRequired } if c.S3.SecretAccessKey == "" { - return errS3SecretRequired + return ErrS3SecretRequired } return nil @@ -544,11 +549,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