From 0538af44878d6bb47d856c4050eeea264cc83a4d Mon Sep 17 00:00:00 2001 From: sneak Date: Tue, 22 Sep 2026 10:02:34 +0000 Subject: [PATCH] Stop config set echoing secrets; reject credential-bearing storage URLs (closes #166) config set now prints only the key name after a write, never the value: a value may be a secret such as s3.secret_access_key, and echoing it leaks into captured stdout and pasted terminals. The set logic moves into writeConfigSet so this is testable. config set also tightens a pre-existing group- or world-readable config to 0600 after writing. os.WriteFile does not change an existing file's mode, so the previous stat-and-preserve-mode block had no effect; it is removed. ParseStorageURL now rejects s3:// and rclone:// URLs that carry credentials in the userinfo or an unknown query parameter, and names s3.access_key_id and s3.secret_access_key as where credentials belong; rclone:// accepts no parameters, so a misspelt one is caught rather than silently sending the backup to the default endpoint. On a url.Parse failure only the inner cause is wrapped, so the raw URL is not echoed. file:// is unchanged. Model: opus-4-8 --- internal/cli/config.go | 74 +++++++++------ internal/cli/config_test.go | 65 +++++++++++++ internal/storage/url.go | 147 ++++++++++++++++++++--------- internal/storage/url_parse_test.go | 98 +++++++++++++++++++ 4 files changed, 308 insertions(+), 76 deletions(-) diff --git a/internal/cli/config.go b/internal/cli/config.go index 066b32d..87e434f 100644 --- a/internal/cli/config.go +++ b/internal/cli/config.go @@ -4,6 +4,7 @@ import ( "bytes" "errors" "fmt" + "io" "os" "os/exec" "path/filepath" @@ -379,40 +380,53 @@ Examples: return err } - root, err := loadYAMLFile(path) - if err != nil { - return err - } - - err = yamlPathSet(root, strings.Split(args[0], "."), args[1]) - if err != nil { - return err - } - - out, err := marshalConfigYAML(root) - if err != nil { - return fmt.Errorf("marshaling config: %w", err) - } - - mode := os.FileMode(configFileMode) - - info, statErr := os.Stat(path) - if statErr == nil { - mode = info.Mode().Perm() - } - - err = os.WriteFile(path, out, mode) - if err != nil { - return fmt.Errorf("writing config file: %w", err) - } - - _, _ = fmt.Fprintf(os.Stdout, "%s = %s\n", args[0], args[1]) - - return nil + return writeConfigSet(os.Stdout, path, args[0], args[1]) }, } } +// writeConfigSet applies key=value to the config at path, writes it back +// owner-only, and confirms the write by printing just the key name to w. +// The value is never echoed: it may be a secret such as +// s3.secret_access_key, and captured stdout or a pasted terminal would +// then leak it. +func writeConfigSet(w io.Writer, path, key, value string) error { + root, err := loadYAMLFile(path) + if err != nil { + return err + } + + err = yamlPathSet(root, strings.Split(key, "."), value) + if err != nil { + return err + } + + out, err := marshalConfigYAML(root) + if err != nil { + return fmt.Errorf("marshaling config: %w", err) + } + + err = os.WriteFile(path, out, configFileMode) + if err != nil { + return fmt.Errorf("writing config file: %w", err) + } + + // os.WriteFile does not change the mode of a file that already exists, + // so a config that was group- or world-readable stays that way. As it + // may hold S3 credentials, tighten it to owner-only after writing. + info, statErr := os.Stat(path) + if statErr == nil && info.Mode().Perm()&0o044 != 0 { + err = os.Chmod(path, configFileMode) + if err != nil { + return fmt.Errorf("tightening config file permissions: %w", err) + } + } + + _, _ = fmt.Fprintln(w, key) + + 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 diff --git a/internal/cli/config_test.go b/internal/cli/config_test.go index 2bffceb..7ae0c18 100644 --- a/internal/cli/config_test.go +++ b/internal/cli/config_test.go @@ -1,6 +1,9 @@ package cli //nolint:testpackage // exercises unexported yamlPathGet/yamlPathSet import ( + "bytes" + "os" + "path/filepath" "strings" "testing" @@ -229,6 +232,68 @@ func TestConfigSetPreservesFormatting(t *testing.T) { } } +// TestWriteConfigSetHidesSecret checks that setting a secret key prints +// only the key name, never the value, to the confirmation output. +func TestWriteConfigSetHidesSecret(t *testing.T) { + t.Parallel() + + const secret = "SUPERSECRETVALUE" + + path := filepath.Join(t.TempDir(), "config.yaml") + + err := os.WriteFile(path, []byte("version: 1\n"), 0o600) + if err != nil { + t.Fatalf("seed config: %v", err) + } + + var out bytes.Buffer + + err = writeConfigSet(&out, path, "s3.secret_access_key", secret) + if err != nil { + t.Fatalf("writeConfigSet: %v", err) + } + + if strings.Contains(out.String(), secret) { + t.Errorf("output echoed the secret value: %q", out.String()) + } + + if !strings.Contains(out.String(), "s3.secret_access_key") { + t.Errorf("output did not confirm the key name: %q", out.String()) + } +} + +// TestWriteConfigSetTightensMode checks that a pre-existing group- or +// world-readable config is tightened to owner-only after a set, since +// os.WriteFile leaves an existing file's mode untouched. +func TestWriteConfigSetTightensMode(t *testing.T) { + t.Parallel() + + path := filepath.Join(t.TempDir(), "config.yaml") + + // Seed a world-readable config; the loose mode is the condition under + // test, so gosec's G306 is expected here. + err := os.WriteFile(path, []byte("version: 1\n"), 0o644) //nolint:gosec // G306 + if err != nil { + t.Fatalf("seed config: %v", err) + } + + var out bytes.Buffer + + err = writeConfigSet(&out, path, "compression_level", "9") + if err != nil { + t.Fatalf("writeConfigSet: %v", err) + } + + info, err := os.Stat(path) + if err != nil { + t.Fatalf("stat config: %v", err) + } + + if info.Mode().Perm() != 0o600 { + t.Errorf("config mode = %04o, want 0600", info.Mode().Perm()) + } +} + func splitPath(s string) []string { return strings.Split(s, ".") } diff --git a/internal/storage/url.go b/internal/storage/url.go index 1e3ddf2..24ea329 100644 --- a/internal/storage/url.go +++ b/internal/storage/url.go @@ -4,6 +4,7 @@ import ( "errors" "fmt" "net/url" + "slices" "strings" ) @@ -23,6 +24,10 @@ var ( ErrUnsupportedScheme = errors.New( "unsupported URL scheme: must start with s3://, file://, or rclone://") ErrUnsupportedStorage = errors.New("unsupported storage scheme") + ErrURLCredentials = errors.New( + "storage URL must not carry credentials; " + + "set s3.access_key_id and s3.secret_access_key in the config instead") + ErrURLUnknownParam = errors.New("unknown query parameter in storage URL") ) // URL represents a parsed storage URL. @@ -59,61 +64,111 @@ func ParseStorageURL(rawURL string) (*URL, error) { }, nil } - // Handle s3:// URLs if strings.HasPrefix(rawURL, "s3://") { - u, err := url.Parse(rawURL) - if err != nil { - return nil, fmt.Errorf("invalid URL: %w", err) - } - - bucket := u.Host - if bucket == "" { - return nil, ErrMissingBucket - } - - prefix := strings.TrimPrefix(u.Path, "/") - - query := u.Query() - - useSSL := true - if query.Get("ssl") == "false" { - useSSL = false - } - - return &URL{ - Scheme: schemeS3, - Bucket: bucket, - Prefix: prefix, - Endpoint: query.Get("endpoint"), - Region: query.Get("region"), - UseSSL: useSSL, - }, nil + return parseS3URL(rawURL) } - // Handle rclone:// URLs if strings.HasPrefix(rawURL, "rclone://") { - u, err := url.Parse(rawURL) - if err != nil { - return nil, fmt.Errorf("invalid URL: %w", err) - } - - remote := u.Host - if remote == "" { - return nil, ErrMissingRemote - } - - path := strings.TrimPrefix(u.Path, "/") - - return &URL{ - Scheme: schemeRclone, - Prefix: path, - RcloneRemote: remote, - }, nil + return parseRcloneURL(rawURL) } return nil, ErrUnsupportedScheme } +// parseS3URL parses an s3://bucket/prefix URL. It rejects credentials in +// the userinfo and any query parameter other than endpoint, region and +// ssl, so a credential-bearing URL is never stored or echoed. +func parseS3URL(rawURL string) (*URL, error) { + u, err := url.Parse(rawURL) + if err != nil { + return nil, wrapParseError(err) + } + + if u.User != nil { + return nil, ErrURLCredentials + } + + bucket := u.Host + if bucket == "" { + return nil, ErrMissingBucket + } + + query := u.Query() + + err = rejectUnknownParams(query, "endpoint", "region", "ssl") + if err != nil { + return nil, err + } + + return &URL{ + Scheme: schemeS3, + Bucket: bucket, + Prefix: strings.TrimPrefix(u.Path, "/"), + Endpoint: query.Get("endpoint"), + Region: query.Get("region"), + UseSSL: query.Get("ssl") != "false", + }, nil +} + +// parseRcloneURL parses an rclone://remote/path URL. rclone:// takes no +// query parameters, so credentials in the userinfo and any parameter at +// all are rejected rather than silently ignored. +func parseRcloneURL(rawURL string) (*URL, error) { + u, err := url.Parse(rawURL) + if err != nil { + return nil, wrapParseError(err) + } + + if u.User != nil { + return nil, ErrURLCredentials + } + + remote := u.Host + if remote == "" { + return nil, ErrMissingRemote + } + + err = rejectUnknownParams(u.Query()) + if err != nil { + return nil, err + } + + return &URL{ + Scheme: schemeRclone, + Prefix: strings.TrimPrefix(u.Path, "/"), + RcloneRemote: remote, + }, nil +} + +// rejectUnknownParams returns an error naming the first query parameter +// not in allowed. The parameter's name is included (so a misspelt +// endpoint= is caught), but never its value, which could be a secret, +// and never the whole URL. +func rejectUnknownParams(query url.Values, allowed ...string) error { + for name := range query { + if !slices.Contains(allowed, name) { + return fmt.Errorf( + "%w: %q; put credentials in s3.access_key_id and "+ + "s3.secret_access_key, not the URL", + ErrURLUnknownParam, name) + } + } + + return nil +} + +// wrapParseError wraps only the inner cause of a url.Parse failure. The +// *url.Error that url.Parse returns embeds the raw URL in its message, so +// wrapping it directly would echo a credential-bearing URL into logs. +func wrapParseError(err error) error { + var uerr *url.Error + if errors.As(err, &uerr) { + return fmt.Errorf("invalid URL: %w", uerr.Err) + } + + return fmt.Errorf("invalid URL: %w", err) +} + // String returns a human-readable representation of the storage URL. func (u *URL) String() string { switch u.Scheme { diff --git a/internal/storage/url_parse_test.go b/internal/storage/url_parse_test.go index d49bc5c..ad0cce7 100644 --- a/internal/storage/url_parse_test.go +++ b/internal/storage/url_parse_test.go @@ -3,6 +3,7 @@ package storage_test import ( "errors" "reflect" + "strings" "testing" "sneak.berlin/go/vaultik/internal/storage" @@ -108,3 +109,100 @@ func TestParseStorageURLErrors(t *testing.T) { }) } } + +// TestParseStorageURLRejectsCredentials checks that a URL carrying +// credentials in its userinfo or in an unknown query parameter is +// rejected, and that the error never echoes the secret-bearing URL back +// into logs or output. +func TestParseStorageURLRejectsCredentials(t *testing.T) { + t.Parallel() + + // Split so the literals never form a "user:pass@" URL pattern that + // tooling would flag as a real hardcoded credential. + const ( + key = "AKIAKEY" + secret = "topsecret" + ) + + cases := []struct { + name string + raw string + wantErr error + secrets []string // must not appear in the error message + }{ + { + name: "s3 userinfo", + raw: "s3://" + key + ":" + secret + "@mybucket/prefix", + wantErr: storage.ErrURLCredentials, + secrets: []string{key, secret, "mybucket"}, + }, + { + name: "s3 unknown query param", + raw: "s3://mybucket?access_key=" + key + "&secret=" + secret, + wantErr: storage.ErrURLUnknownParam, + secrets: []string{key, secret}, + }, + { + name: "s3 misspelt endpoint", + raw: "s3://mybucket?endpiont=minio.example.com", + wantErr: storage.ErrURLUnknownParam, + secrets: nil, + }, + { + name: "rclone userinfo", + raw: "rclone://user:" + secret + "@gdrive/backups", + wantErr: storage.ErrURLCredentials, + secrets: []string{secret}, + }, + { + name: "rclone query param", + raw: "rclone://gdrive/backups?token=" + secret, + wantErr: storage.ErrURLUnknownParam, + secrets: []string{secret}, + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + _, err := storage.ParseStorageURL(tc.raw) + if !errors.Is(err, tc.wantErr) { + t.Fatalf("ParseStorageURL(%q) error = %v, want %v", + tc.raw, err, tc.wantErr) + } + + // The rejection must name the proper config keys so the + // operator knows where credentials belong. + for _, key := range []string{"s3.access_key_id", "s3.secret_access_key"} { + if !strings.Contains(err.Error(), key) { + t.Errorf("error %q does not name %q", err.Error(), key) + } + } + + for _, secret := range tc.secrets { + if strings.Contains(err.Error(), secret) { + t.Errorf("error message leaked %q: %v", secret, err.Error()) + } + } + }) + } +} + +// TestParseStorageURLParseFailureHidesURL checks that when url.Parse +// itself fails, the wrapped error carries only the inner cause, not the +// *url.Error whose text embeds the raw (possibly credential-bearing) URL. +func TestParseStorageURLParseFailureHidesURL(t *testing.T) { + t.Parallel() + + const raw = "s3://mybucket/%zz" + + _, err := storage.ParseStorageURL(raw) + if err == nil { + t.Fatalf("ParseStorageURL(%q) returned no error", raw) + } + + if strings.Contains(err.Error(), "mybucket") { + t.Errorf("error message echoed the raw URL: %v", err.Error()) + } +} -- 2.54.0