diff --git a/README.md b/README.md index 355322d..c28871a 100644 --- a/README.md +++ b/README.md @@ -271,8 +271,16 @@ existing file. Created with mode `0600` since it will contain credentials. **`config set`**: Set a scalar config value by dotted YAML path (e.g. `vaultik config set compression_level 9`, `vaultik config set storage_url "file:///mnt/backups"`). Comments and -formatting in the file are preserved; intermediate maps are created as -needed. +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 +after which it would not, such as `vaultik config set 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, 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) diff --git a/internal/cli/config.go b/internal/cli/config.go index ea477bd..1d82b73 100644 --- a/internal/cli/config.go +++ b/internal/cli/config.go @@ -44,6 +44,9 @@ var ( errNeedNumericIndex = errors.New("key is a list; use a numeric index") 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, " + + "so the file was not changed") ) const defaultConfigTemplate = `# vaultik configuration @@ -391,8 +394,15 @@ func newConfigSetCommand() *cobra.Command { Use: "set ", Short: "Set a config value by dotted path (e.g. compression_level 5)", Long: `Sets a scalar config value addressed by dotted YAML path and writes -the file back, preserving comments and formatting. Intermediate maps -are created as needed. +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. + +If the config loads, a change after which it would not, such as +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" @@ -422,6 +432,11 @@ 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 @@ -432,6 +447,11 @@ func writeConfigSet(out *ui.Writer, path, key, value string) error { return fmt.Errorf("marshaling config: %w", err) } + err = checkConfigChange(oldData, data) + if err != nil { + return fmt.Errorf("%w: %w", errWouldNotLoad, err) + } + err = os.WriteFile(path, data, configFileMode) if err != nil { return fmt.Errorf("writing config file: %w", err) @@ -453,6 +473,28 @@ 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 @@ -559,8 +601,8 @@ func yamlPathGet(root *yaml.Node, keys []string) (*yaml.Node, error) { return node, nil } -// yamlPathSet navigates a dotted key path, creating intermediate maps as -// needed, and sets the final key to the given scalar value. Numeric path +// yamlPathSet navigates a dotted key path, creating missing keys as needed, +// and sets the final key to the given scalar value. Numeric path // components index into sequences; an index equal to the sequence length // appends a new element (e.g. "age_recipients.1" on a 1-element list). func yamlPathSet(root *yaml.Node, keys []string, value string) error { @@ -573,12 +615,12 @@ func yamlPathSet(root *yaml.Node, keys []string, value string) error { node = node.Content[0] } - for i, key := range keys { + for i := range keys { last := i == len(keys)-1 switch node.Kind { case yaml.MappingNode: - node = yamlSetInMapping(node, key, value, last) + node = yamlSetInMapping(node, keys, i, value, last) case yaml.SequenceNode: next, err := yamlSetInSequence(node, keys, i, value, last) if err != nil { @@ -647,14 +689,16 @@ func yamlField(typ reflect.Type, key string) (reflect.StructField, bool) { return reflect.StructField{}, false } -// yamlSetInMapping resolves (creating if needed) the value node for key -// within a mapping node, setting it to value when it is the final path -// element, and returns the node to descend into. -func yamlSetInMapping(node *yaml.Node, key, value string, last bool) *yaml.Node { +// yamlSetInMapping resolves (creating if needed) the value node for the +// path element keys[i] within a mapping node, setting it to value when it +// is the final path element, and returns the node to descend into. +func yamlSetInMapping( + node *yaml.Node, keys []string, i int, value string, last bool, +) *yaml.Node { var valueNode *yaml.Node for j := 0; j+1 < len(node.Content); j += 2 { - if node.Content[j].Value == key { + if node.Content[j].Value == keys[i] { valueNode = node.Content[j+1] break @@ -662,13 +706,8 @@ func yamlSetInMapping(node *yaml.Node, key, value string, last bool) *yaml.Node } if valueNode == nil { - keyNode := &yaml.Node{Kind: yaml.ScalarNode, Value: key} - - valueNode = &yaml.Node{Kind: yaml.MappingNode} - if last { - valueNode = &yaml.Node{Kind: yaml.ScalarNode, Value: value} - } - + keyNode := &yaml.Node{Kind: yaml.ScalarNode, Value: keys[i]} + valueNode = newPathNode(keys, i, value) node.Content = append(node.Content, keyNode, valueNode) } else if last { setScalar(valueNode, value) @@ -696,12 +735,7 @@ func yamlSetInSequence( } if idx == len(node.Content) { - newNode := &yaml.Node{Kind: yaml.MappingNode} - if last { - newNode = &yaml.Node{Kind: yaml.ScalarNode, Value: value} - } - - node.Content = append(node.Content, newNode) + node.Content = append(node.Content, newPathNode(keys, i, value)) } else if last { setScalar(node.Content[idx], value) } @@ -709,6 +743,22 @@ func yamlSetInSequence( return node.Content[idx], nil } +// newPathNode returns the node to create for the missing path element +// keys[i]: the value when it is the final path element, otherwise an empty +// list when the next path element is a number and an empty map when not. +func newPathNode(keys []string, i int, value string) *yaml.Node { + if i == len(keys)-1 { + return &yaml.Node{Kind: yaml.ScalarNode, Value: value} + } + + _, err := strconv.Atoi(keys[i+1]) + if err == nil { + return &yaml.Node{Kind: yaml.SequenceNode} + } + + return &yaml.Node{Kind: yaml.MappingNode} +} + // setScalar overwrites a node in place with a plain scalar value. func setScalar(n *yaml.Node, value string) { n.Kind = yaml.ScalarNode diff --git a/internal/cli/config_test.go b/internal/cli/config_test.go index f7b9eb2..d9f7982 100644 --- a/internal/cli/config_test.go +++ b/internal/cli/config_test.go @@ -58,14 +58,16 @@ func TestDefaultConfigTemplateParses(t *testing.T) { } } +// testRecipient is a valid age public key. +const testRecipient = "age1278m9q7dp3chsh2dcy82qk27v047zywyvtxwnj4cvt0z65jw6a7q5dqhfj" + // 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() - const recipient = "age1278m9q7dp3chsh2dcy82qk27v047zywyvtxwnj4cvt0z65jw6a7q5dqhfj" - path := filepath.Join(t.TempDir(), "config.yml") err := os.WriteFile(path, []byte(defaultConfigTemplate), configFileMode) @@ -75,7 +77,7 @@ func TestConfigSetRecipientOnFreshConfig(t *testing.T) { out := ui.NewWithColor(&bytes.Buffer{}, false) - err = writeConfigSet(out, path, "age_recipients.0", recipient) + err = writeConfigSet(out, path, "age_recipients.0", testRecipient) if err != nil { t.Fatalf("config set age_recipients.0: %v", err) } @@ -90,8 +92,177 @@ func TestConfigSetRecipientOnFreshConfig(t *testing.T) { t.Fatalf("config.Load: %v", err) } - if len(cfg.AgeRecipients) != 1 || cfg.AgeRecipients[0] != recipient { - t.Errorf("age_recipients = %v, want [%s]", cfg.AgeRecipients, recipient) + if len(cfg.AgeRecipients) != 1 || cfg.AgeRecipients[0] != testRecipient { + t.Errorf("age_recipients = %v, want [%s]", cfg.AgeRecipients, testRecipient) + } +} + +// configWithoutRecipients loads, but has no age_recipients key and no +// snapshot named work. +const configWithoutRecipients = `storage_url: file:///mnt/backups +snapshots: + home: + paths: + - "~" +` + +// TestConfigSetIndexUnderMissingKey checks that a list index under a key +// the file does not have creates a list, and that the result loads. +func TestConfigSetIndexUnderMissingKey(t *testing.T) { + t.Parallel() + + tests := []struct { + key string + value string + field func(cfg *config.Config) []string + }{ + {"age_recipients.0", testRecipient, + func(cfg *config.Config) []string { return cfg.AgeRecipients }}, + {"snapshots.work.paths.0", "/srv/work", + func(cfg *config.Config) []string { return cfg.Snapshots["work"].Paths }}, + } + + for _, tt := range tests { + t.Run(tt.key, func(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) + } + + err = writeConfigSet(ui.NewWithColor(&bytes.Buffer{}, false), + path, tt.key, tt.value) + if err != nil { + t.Fatalf("config set %s: %v", tt.key, err) + } + + cfg, err := config.Load(path) + if err != nil { + t.Fatalf("config.Load: %v", err) + } + + got := tt.field(cfg) + if len(got) != 1 || got[0] != tt.value { + t.Errorf("%s gives %v, want [%s]", tt.key, got, tt.value) + } + }) + } +} + +// TestConfigSetKeepsFileThatWouldNotLoad checks that config set refuses a +// change after which the config would not load, with an error giving the +// reason, and leaves the file as it was. On the file `config init` writes, +// 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 + value string + reason string + }{ + // Index 1 of the empty list the missing key gives. + {configWithoutRecipients, "age_recipients.1", testRecipient, + "index out of range"}, + // A list set to a single value. + {configWithoutRecipients, "snapshots.home.paths", "/srv", + "cannot unmarshal"}, + {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.Parallel() + + path := filepath.Join(t.TempDir(), "config.yml") + + err := os.WriteFile(path, []byte(tt.config), 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) { + t.Errorf("config set %s %s gives error %v, want one saying %q", + tt.key, tt.value, err, tt.reason) + } + + data, err := os.ReadFile(path) //nolint:gosec // G304: test-controlled path + if err != nil { + t.Fatalf("read config: %v", err) + } + + if string(data) != tt.config { + t.Errorf("config set %s changed the file to:\n%s", + tt.key, data) + } + }) + } +} + +// 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) + } + }) } } diff --git a/internal/config/config.go b/internal/config/config.go index eb51263..c9dd2ec 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -4,6 +4,7 @@ package config import ( + "bytes" "errors" "fmt" "os" @@ -233,14 +234,48 @@ func New(path Path) (*Config, error) { return cfg, nil } -// Load reads and parses the configuration file from the specified path. -// It applies default values for optional fields, performs environment variable -// substitution using smartconfig, and validates the configuration. -// The configuration file should be in YAML format. Returns an error if the file -// cannot be read, parsed, or if validation fails. +// Load reads the configuration file at path, parses it with Parse, warns +// when others can read the file, and validates the configuration. +// 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.NewFromConfigPath(path) + sc, err := smartconfig.NewFromReader(bytes.NewReader(data)) if err != nil { return nil, fmt.Errorf("failed to load config file: %w", err) } @@ -302,24 +337,6 @@ func Load(path string) (*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 }