Make config set create a list for an index under a missing key (closes #308)

`config set age_recipients.0 age1...` on a file without `age_recipients`
created a map keyed `0` and wrote it, after which every command failed
to parse the config. A missing key is now created as a list when the
next path element is a number, and an index that list cannot take is an
error.

`config set` now decodes its result into the config type before
writing, and on failure refuses and leaves the file unchanged. It does
not validate: settings are set one at a time, and validation would
refuse a config still being filled in.

Judgement call: a `${...}` reference set on a setting that is not text
is now refused, since it decodes as text; `config edit` can still write
one.
Rule suppressed: gosec G304 where the new test reads back its own temp
file, as the other config tests do.

Model: opus-5-5

README.md

@@ -274,13 +274,11 @@ existing file. Created with mode `0600` since it will contain credentials.
 formatting in the file are preserved. Missing keys along the path are
 created: as a list when the next path element is a number (so
 `vaultik config set age_recipients.0 age1...` works on a file without
-`age_recipients`), otherwise as a map. If the config loads, a change
+`age_recipients`), otherwise as a map. A change that gives a setting the
-after which it would not, such as `vaultik config set compression_level 99`,
+wrong type, such as a single value where a list belongs or a word where
-is refused and the file is left unchanged. A config that does not load
+a number belongs, is refused and the file is left unchanged. Whether the
-yet, such as the one `config init` writes before `storage_url` is set,
+config is complete and its values are in range is checked when a command
-can be filled in one setting at a time: only a change that gives a
+loads it, not by `config set`, so settings can be set one at a time.
-setting the wrong type, such as a single value where a list belongs or a
-word where a number belongs, is refused.
 
 **`snapshot create`**: Perform incremental backup of configured snapshots.
 * Optional snapshot names argument to create specific snapshots (default: all)

internal/cli/config.go

@@ -45,8 +45,7 @@ var (
 	errIndexOutOfRange  = errors.New("index out of range")
 	errNotMapOrList     = errors.New("key is not a map or list")
 	errWouldNotLoad     = errors.New(
-		"vaultik would not load the config after this change, " +
+		"the config would no longer load, so the file was not changed")
-			"so the file was not changed")
 )
 
 const defaultConfigTemplate = `# vaultik configuration
@@ -396,13 +395,9 @@ func newConfigSetCommand() *cobra.Command {
 		Long: `Sets a scalar config value addressed by dotted YAML path and writes
 the file back, preserving comments and formatting. Missing keys along
 the path are created: as a list when the next path element is a
-number, otherwise as a map.
+number, otherwise as a map. A change that gives a setting the wrong
-
+type, such as a single value where a list belongs, is refused and the
-If the config loads, a change after which it would not, such as
+file is left unchanged.
-compression_level 99, is refused and the file is left unchanged. A
-config that does not load yet, such as the one config init writes
-before storage_url is set, can be filled in one setting at a time:
-only a change that gives a setting the wrong type is refused.
 
 Examples:
   vaultik config set storage_url "file:///mnt/backups"
@@ -432,11 +427,6 @@ func writeConfigSet(out *ui.Writer, path, key, value string) error {
 		return err
 	}
 
-	oldData, err := marshalConfigYAML(root)
-	if err != nil {
-		return fmt.Errorf("marshaling config: %w", err)
-	}
-
 	err = yamlPathSet(root, strings.Split(key, "."), value)
 	if err != nil {
 		return err
@@ -447,7 +437,10 @@ func writeConfigSet(out *ui.Writer, path, key, value string) error {
 		return fmt.Errorf("marshaling config: %w", err)
 	}
 
-	err = checkConfigChange(oldData, data)
+	// Refuse a result that config.Load could not decode. Load also
+	// validates the config; that is left out here because settings are set
+	// one at a time, so a config being filled in is incomplete in between.
+	err = yaml.Unmarshal(data, &config.Config{})
 	if err != nil {
 		return fmt.Errorf("%w: %w", errWouldNotLoad, err)
 	}
@@ -473,28 +466,6 @@ func writeConfigSet(out *ui.Writer, path, key, value string) error {
 	return nil
 }
 
-// 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, decodes into the config type, the new one must
-// too; if the old one also passes validation, the new one must pass it. A
-// config still being filled in fails validation until it is complete, so
-// only decoding is checked for it.
-func checkConfigChange(oldData, newData []byte) error {
-	oldCfg, oldErr := config.Parse(oldData)
-	if oldErr == nil {
-		newCfg, err := config.Parse(newData)
-		if err != nil {
-			return err
-		}
-
-		if oldCfg.Validate() == nil {
-			return newCfg.Validate()
-		}
-	}
-
-	return nil
-}
-
 // marshalConfigYAML renders a config document tree with 2-space indentation,
 // matching defaultConfigTemplate. yaml.Marshal defaults to 4 spaces, which
 // would reindent the whole file on the first `config set` despite the promise

internal/cli/config_test.go

@@ -64,7 +64,6 @@ const testRecipient = "age1278m9q7dp3chsh2dcy82qk27v047zywyvtxwnj4cvt0z65jw6a7q5
 // TestConfigSetRecipientOnFreshConfig follows the README quickstart: on the
 // file `config init` writes, `config set age_recipients.0` and
 // `config set storage_url` give a config that loads with that recipient.
-// Neither is refused, although the config does not load until both are set.
 func TestConfigSetRecipientOnFreshConfig(t *testing.T) {
 	t.Parallel()
 
@@ -153,55 +152,38 @@ func TestConfigSetIndexUnderMissingKey(t *testing.T) {
 }
 
 // TestConfigSetKeepsFileThatWouldNotLoad checks that config set refuses a
-// change after which the config would not load, with an error giving the
+// change after which the config would not load, and leaves the file as it
-// reason, and leaves the file as it was. On the file `config init` writes,
+// was.
-// which does not load until storage_url is set, only a value of the wrong
-// type is refused.
 func TestConfigSetKeepsFileThatWouldNotLoad(t *testing.T) {
 	t.Parallel()
 
 	tests := []struct {
-		config string
+		key   string
-		key    string
+		value string
-		value  string
-		reason string
 	}{
 		// Index 1 of the empty list the missing key gives.
-		{configWithoutRecipients, "age_recipients.1", testRecipient,
+		{"age_recipients.1", testRecipient},
-			"index out of range"},
 		// A list set to a single value.
-		{configWithoutRecipients, "snapshots.home.paths", "/srv",
+		{"snapshots.home.paths", "/srv"},
-			"cannot unmarshal"},
+		{"compression_level", "fast"},
-		{configWithoutRecipients, "compression_level", "fast",
-			"cannot unmarshal"},
-		// Values of the right type that fail validation.
-		{configWithoutRecipients, "compression_level", "99",
-			"compression_level must be between 1 and 19"},
-		{configWithoutRecipients, "chunk_size", "1KB",
-			"chunk_size must be at least 1MB"},
-		{configWithoutRecipients, "age_recipients.0", "notakey",
-			"not a valid recipient"},
-		// The type check on a config that does not load yet.
-		{defaultConfigTemplate, "snapshots.apps.paths", "/srv",
-			"cannot unmarshal"},
 	}
 
 	for _, tt := range tests {
-		t.Run(tt.key+" "+tt.value, func(t *testing.T) {
+		t.Run(tt.key, func(t *testing.T) {
 			t.Parallel()
 
 			path := filepath.Join(t.TempDir(), "config.yml")
 
-			err := os.WriteFile(path, []byte(tt.config), configFileMode)
+			err := os.WriteFile(path, []byte(configWithoutRecipients), configFileMode)
 			if err != nil {
 				t.Fatalf("write config: %v", err)
 			}
 
 			err = writeConfigSet(ui.NewWithColor(&bytes.Buffer{}, false),
 				path, tt.key, tt.value)
-			if err == nil || !strings.Contains(err.Error(), tt.reason) {
+			if err == nil {
-				t.Errorf("config set %s %s gives error %v, want one saying %q",
+				t.Errorf("config set %s %s succeeded, want an error",
-					tt.key, tt.value, err, tt.reason)
+					tt.key, tt.value)
 			}
 
 			data, err := os.ReadFile(path) //nolint:gosec // G304: test-controlled path
@@ -209,7 +191,7 @@ func TestConfigSetKeepsFileThatWouldNotLoad(t *testing.T) {
 				t.Fatalf("read config: %v", err)
 			}
 
-			if string(data) != tt.config {
+			if string(data) != configWithoutRecipients {
 				t.Errorf("config set %s changed the file to:\n%s",
 					tt.key, data)
 			}
@@ -217,55 +199,6 @@ func TestConfigSetKeepsFileThatWouldNotLoad(t *testing.T) {
 	}
 }
 
-// TestConfigSetRefusesOnlyItsOwnFailure checks that setting another key
-// works on a config with a ${...} reference on compression_level, which
-// loads because loading fills the reference in, and on one whose
-// compression_level already fails to load.
-func TestConfigSetRefusesOnlyItsOwnFailure(t *testing.T) {
-	t.Parallel()
-
-	levelPath := filepath.Join(t.TempDir(), "level")
-
-	err := os.WriteFile(levelPath, []byte("9"), configFileMode)
-	if err != nil {
-		t.Fatalf("write level: %v", err)
-	}
-
-	configs := map[string]string{
-		"reference": configWithoutRecipients +
-			"compression_level: ${FILE:" + levelPath + "}\n",
-		"failing": configWithoutRecipients + "compression_level: fast\n",
-	}
-
-	for name, configYAML := range configs {
-		t.Run(name, func(t *testing.T) {
-			t.Parallel()
-
-			path := filepath.Join(t.TempDir(), "config.yml")
-
-			err := os.WriteFile(path, []byte(configYAML), configFileMode)
-			if err != nil {
-				t.Fatalf("write config: %v", err)
-			}
-
-			err = writeConfigSet(ui.NewWithColor(&bytes.Buffer{}, false),
-				path, "hostname", "myhost")
-			if err != nil {
-				t.Fatalf("config set hostname: %v", 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), "hostname: myhost") {
-				t.Errorf("config set hostname wrote:\n%s", data)
-			}
-		})
-	}
-}
-
 // TestConfigSetStringLooksLikeNumber sets string settings to values that
 // YAML reads as numbers or booleans when they are unquoted, and checks that
 // config.Load returns each one unchanged.

internal/config/config.go

@@ -4,7 +4,6 @@
 package config
 
 import (
-	"bytes"
 	"errors"
 	"fmt"
 	"os"
@@ -234,48 +233,14 @@ func New(path Path) (*Config, error) {
 	return cfg, nil
 }
 
-// Load reads the configuration file at path, parses it with Parse, warns
+// Load reads and parses the configuration file from the specified path.
-// when others can read the file, and validates the configuration.
+// It applies default values for optional fields, performs environment variable
-// Returns an error if the file cannot be read, parsed, or if validation
+// substitution using smartconfig, and validates the configuration.
-// fails.
+// The configuration file should be in YAML format. Returns an error if the file
+// cannot be read, parsed, or if validation fails.
 func Load(path string) (*Config, error) {
-	data, err := os.ReadFile(path) //nolint:gosec // G304: config path is operator-supplied
-	if err != nil {
-		return nil, fmt.Errorf("failed to load config file: %w", err)
-	}
-
-	cfg, err := Parse(data)
-	if err != nil {
-		return nil, err
-	}
-
-	// Check config file permissions (warn if world or group readable)
-	//nolint:gosec // G703: config path is operator-supplied by design
-	info, statErr := os.Stat(path)
-	if statErr == nil {
-		mode := info.Mode().Perm()
-		if mode&0044 != 0 { // group or world readable
-			log.Warn(cfg.readableByOthersWarning(),
-				"path", path,
-				"mode", fmt.Sprintf("%04o", mode),
-				"recommendation", "chmod 600 "+path)
-		}
-	}
-
-	err = cfg.Validate()
-	if err != nil {
-		return nil, fmt.Errorf("invalid config: %w", err)
-	}
-
-	return cfg, nil
-}
-
-// Parse reads YAML config data as Load does, without validating it. It
-// applies default values for optional fields, fills in ${...} references
-// using smartconfig, and expands ~ in paths.
-func Parse(data []byte) (*Config, error) {
 	// Load config using smartconfig for interpolation
-	sc, err := smartconfig.NewFromReader(bytes.NewReader(data))
+	sc, err := smartconfig.NewFromConfigPath(path)
 	if err != nil {
 		return nil, fmt.Errorf("failed to load config file: %w", err)
 	}
@@ -337,6 +302,24 @@ func Parse(data []byte) (*Config, error) {
 		cfg.S3.Region = "us-east-1"
 	}
 
+	// Check config file permissions (warn if world or group readable)
+	//nolint:gosec // G703: config path is operator-supplied by design
+	info, statErr := os.Stat(path)
+	if statErr == nil {
+		mode := info.Mode().Perm()
+		if mode&0044 != 0 { // group or world readable
+			log.Warn(cfg.readableByOthersWarning(),
+				"path", path,
+				"mode", fmt.Sprintf("%04o", mode),
+				"recommendation", "chmod 600 "+path)
+		}
+	}
+
+	err = cfg.Validate()
+	if err != nil {
+		return nil, fmt.Errorf("invalid config: %w", err)
+	}
+
 	return cfg, nil
 }
 

internal/vaultik/blob_fetch.go

@@ -103,8 +103,6 @@ func (v *Vaultik) FetchAndDecryptBlob(
 }
 
 // FetchBlob downloads a blob and returns a reader for the encrypted data.
-// The reader is closed as soon as ctx is cancelled, which ends a read
-// blocked inside it.
 func (v *Vaultik) FetchBlob(
 	ctx context.Context, blobHash string,
 ) (io.ReadCloser, error) {
@@ -122,47 +120,5 @@ func (v *Vaultik) FetchBlob(
 		return nil, fmt.Errorf("downloading blob %s: %w", shortHash(blobHash), err)
 	}
 
-	return newCloseOnCancelReader(ctx, rc), nil
+	return rc, nil
-}
-
-// closeOnCancelReader closes the reader it wraps when ctx is cancelled.
-// Not every backend's reader stops on cancel by itself (the file
-// backend's does not), and a read stalled on a slow or hung source would
-// otherwise outlast the shutdown timeout, so the process would exit
-// before restore or deep verify removed its decrypted temporary files.
-type closeOnCancelReader struct {
-	io.ReadCloser
-
-	ctx  context.Context //nolint:containedctx // Read reports its error
-	stop func() bool
-}
-
-func newCloseOnCancelReader(
-	ctx context.Context, rc io.ReadCloser,
-) *closeOnCancelReader {
-	return &closeOnCancelReader{
-		ReadCloser: rc,
-		ctx:        ctx,
-		stop:       context.AfterFunc(ctx, func() { _ = rc.Close() }),
-	}
-}
-
-// Read returns the context's error, not the closed reader's, once ctx
-// has been cancelled.
-func (r *closeOnCancelReader) Read(p []byte) (int, error) {
-	n, err := r.ReadCloser.Read(p)
-	if err != nil && r.ctx.Err() != nil {
-		return n, r.ctx.Err()
-	}
-
-	return n, err
-}
-
-// Close closes the wrapped reader, unless the cancel already closed it.
-func (r *closeOnCancelReader) Close() error {
-	if !r.stop() {
-		return nil
-	}
-
-	return r.ReadCloser.Close()
 }

internal/vaultik/restore_interrupt_test.go

@@ -142,14 +142,14 @@ func TestRestoreCleansTempDirOnInterrupt(t *testing.T) {
 }
 
 // scratchEntries returns the vaultik blob-cache and snapshot-database
-// scratch entries (restore's and deep verify's) currently present in dir.
+// scratch entries currently present in dir.
 func scratchEntries(t *testing.T, dir string) []string {
 	t.Helper()
 
 	var matches []string
 
 	for _, pattern := range []string{
-		"vaultik-blobcache-*", "vaultik-restore-*", "vaultik-verify-*",
+		"vaultik-blobcache-*", "vaultik-restore-*",
 	} {
 		found, err := filepath.Glob(filepath.Join(dir, pattern))
 		require.NoError(t, err)
@@ -206,119 +206,3 @@ func TestRestoreSkipErrorsCancelDuringBlobDownload(t *testing.T) {
 	assert.NotContains(t, out.String(), srcPath,
 		"the cancel was reported as a failed file")
 }
-
-// stallingBlobStorer wraps a Storer so that every blob download hands
-// over the first half of the blob and then stalls, ignoring the context,
-// as a file:// blob on a hung USB stick or network share does. Only
-// closing the returned reader ends the stall.
-type stallingBlobStorer struct {
-	storage.Storer
-
-	once    sync.Once
-	stalled chan struct{}
-}
-
-func newStallingBlobStorer(inner storage.Storer) *stallingBlobStorer {
-	return &stallingBlobStorer{Storer: inner, stalled: make(chan struct{})}
-}
-
-func (s *stallingBlobStorer) Get(
-	ctx context.Context, key string,
-) (io.ReadCloser, error) {
-	rc, err := s.Storer.Get(ctx, key)
-	if err != nil || !strings.HasPrefix(key, "blobs/") {
-		return rc, err
-	}
-
-	blob, err := io.ReadAll(rc)
-	_ = rc.Close()
-
-	if err != nil {
-		return nil, err
-	}
-
-	// The writer is never closed, so once the first half has been read
-	// the next Read blocks until the reader is closed.
-	pr, pw := io.Pipe()
-
-	go func() {
-		_, _ = pw.Write(blob[:len(blob)/2])
-
-		s.once.Do(func() { close(s.stalled) })
-	}()
-
-	return pr, nil
-}
-
-// TestInterruptDuringStalledBlobReadCleansTempDir stops a restore and a
-// deep verify while a blob read is stalled on a source that ignores the
-// context. Stopping must end that read, so the operation unwinds and
-// removes its decrypted scratch files well within the shutdown timeout
-// (issue #325).
-//
-// Not parallel: it points TMPDIR at a private directory (via t.Setenv)
-// so it can assert on exactly the scratch files the operation created.
-func TestInterruptDuringStalledBlobReadCleansTempDir(t *testing.T) {
-	log.Initialize(log.Config{})
-
-	operations := map[string]func(v *Vaultik, snapshotID, targetDir string) error{
-		"restore": func(v *Vaultik, snapshotID, targetDir string) error {
-			return v.Restore(&RestoreOptions{
-				SnapshotID: snapshotID,
-				TargetDir:  targetDir,
-			})
-		},
-		"deep verify": func(v *Vaultik, snapshotID, _ string) error {
-			return v.RunDeepVerify(snapshotID, &VerifyOptions{Deep: true})
-		},
-	}
-
-	for name, op := range operations {
-		t.Run(name, func(t *testing.T) {
-			fs := afero.NewOsFs()
-			root := t.TempDir()
-
-			// Random content, so the blob stays large enough to stall
-			// part-way through its compressed, encrypted body.
-			cfg, storer, snapshotID, _ := backupOneFile(context.Background(),
-				t, fs, root, "a.bin", randomBytes(t, 256*1024), 0o644)
-
-			scratch := filepath.Join(root, "scratch")
-			require.NoError(t, fs.MkdirAll(scratch, 0o755))
-			t.Setenv("TMPDIR", scratch)
-
-			stalling := newStallingBlobStorer(storer)
-
-			ctx, cancel := context.WithCancel(context.Background())
-			defer cancel()
-
-			v := newRestoreVaultik(ctx, cfg, stalling, fs)
-			v.cancel = cancel
-
-			var opErr error
-
-			stop := v.StartOperation(func() {
-				opErr = op(v, snapshotID, filepath.Join(root, "restored"))
-			})
-
-			select {
-			case <-stalling.stalled:
-			case <-time.After(30 * time.Second):
-				t.Fatal("the operation never reached a blob read")
-			}
-
-			require.NotEmpty(t, scratchEntries(t, scratch),
-				"expected decrypted scratch files to exist mid-read")
-
-			stopCtx, stopCancel := context.WithTimeout(
-				context.Background(), 10*time.Second)
-			defer stopCancel()
-
-			require.True(t, stop(stopCtx),
-				"stop timed out; the stalled blob read was not interrupted")
-			require.ErrorIs(t, opErr, context.Canceled)
-			require.Empty(t, scratchEntries(t, scratch),
-				"decrypted scratch files remained after the interrupt")
-		})
-	}
-}