diff --git a/cli/config/config.go b/cli/config/config.go index b2e36ce..9991505 100644 --- a/cli/config/config.go +++ b/cli/config/config.go @@ -359,6 +359,13 @@ func WriteProfileWithExtras(path, name string, p Profile, extra map[string]any) if err != nil { return fmt.Errorf("config: write %s: %w", path, err) } + // The mode above applies only to a new file. The file names private + // keys and data bag secrets, so tighten an existing one too, before + // anything is written into it; knife setups often leave it 0644. + if err := f.Chmod(0o600); err != nil { + _ = f.Close() + return fmt.Errorf("config: restrict %s to its owner: %w", path, err) + } if err := toml.NewEncoder(f).Encode(raw); err != nil { _ = f.Close() return fmt.Errorf("config: encode %s: %w", path, err) diff --git a/cli/config/config_test.go b/cli/config/config_test.go index b190ae8..135ed60 100644 --- a/cli/config/config_test.go +++ b/cli/config/config_test.go @@ -416,6 +416,32 @@ func TestWriteProfileCreatesCredentialsFile(t *testing.T) { } } +// TestWriteProfileTightensAnExistingFile rewrites a credentials file that +// others can read, as knife setups often leave one. The file names private +// keys and data bag secrets, so any write leaves it readable by its owner +// alone, whatever mode it had. +func TestWriteProfileTightensAnExistingFile(t *testing.T) { + path := writeConfig(t, sampleConfig) + if err := os.Chmod(path, 0o644); err != nil { + t.Fatal(err) + } + + if err := UpdateProfile(path, "default", func(p *Profile) error { + p.SSLVerifyMode = ":verify_peer" + return nil + }); err != nil { + t.Fatalf("UpdateProfile: %v", err) + } + + info, err := os.Stat(path) + if err != nil { + t.Fatal(err) + } + if perm := info.Mode().Perm(); perm != 0o600 { + t.Errorf("credentials mode after a rewrite = %o, want 0600", perm) + } +} + func TestWriteProfileRoundTripsAllKeys(t *testing.T) { path := filepath.Join(t.TempDir(), ".cinc", "credentials") diff --git a/docs/configuration.md b/docs/configuration.md index e193b2a..f027975 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -20,6 +20,12 @@ A leading `~` in `--config` is expanded to your home directory, so `--config=~/.chef/credentials` works even though no shell expands a `~` after `=`. +The file names your signing keys and data bag secrets, so `cinc` keeps +it readable by you alone. A new file is created with mode `0600` in a +`0700` directory, and whenever `cinc` rewrites an existing file (with +`cinc config create` or first-run setup) it tightens that file to +`0600` too, even if it started out readable by others. + The file holds one or more **profiles**. A profile is a named bundle of everything needed to talk to one server (or one Supermarket): a server URL, a client identity, a signing key, and a handful of optional @@ -363,6 +369,7 @@ that: ```sh cinc config validate # checks ~/.cinc/credentials cinc config validate ./creds # checks a specific file +cinc config validate --profile staging # checks just one profile cinc config validate --format json ``` diff --git a/integration/suite/behaviour.go b/integration/suite/behaviour.go index 1fe298b..136690e 100644 --- a/integration/suite/behaviour.go +++ b/integration/suite/behaviour.go @@ -48,6 +48,7 @@ var cliFamily = family{cases: []testCase{ {"cli/config-create-interactive-update", []string{"config create", "node list"}, testConfigCreateInteractiveUpdate}, {"cli/config-create-replace", []string{"config create", "node list"}, testConfigCreateReplace}, {"cli/config-create-name-collision", []string{"config create", "node list"}, testConfigCreateNameCollision}, + {"cli/config-create-tightens-permissions", []string{"config create", "node list"}, testConfigCreateTightensPermissions}, {"cli/config-create-profile-name", []string{"config create"}, testConfigCreateProfileName}, {"cli/first-run-migrates-chef-credentials", []string{"node list", "node create", "node show"}, testFirstRunMigrates}, {"cli/first-run-migration-keeps-knife-keys", []string{"node list"}, testFirstRunMigrationKeepsKnifeKeys}, diff --git a/integration/suite/configcreate.go b/integration/suite/configcreate.go index cd84acc..f7dabad 100644 --- a/integration/suite/configcreate.go +++ b/integration/suite/configcreate.go @@ -3,6 +3,7 @@ package suite import ( "os" "path/filepath" + "runtime" "slices" "strings" "testing" @@ -91,6 +92,27 @@ func testConfigCreateSymlinkedFile(t *testing.T, tgt Target, c *cli) { c.run("node", "list", "--profile", "fresh") } +// testConfigCreateTightensPermissions updates a profile in a credentials +// file others can read, as knife setups often leave it. The file names +// private keys and secrets, so config create leaves it readable by its +// owner alone. +func testConfigCreateTightensPermissions(t *testing.T, tgt Target, c *cli) { + if runtime.GOOS == "windows" { + t.Skip("Windows has no owner-only file mode to check") + } + if err := os.Chmod(c.credentialsPath(), 0o644); err != nil { + t.Fatal(err) + } + c.run("config", "create", "--profile", "extra", "--server-url", c.orgURL(tgt.Org), + "--client-name", tgt.Admin, "--client-key", tgt.KeyPath) + info, err := os.Stat(c.credentialsPath()) + if err != nil { + t.Fatal(err) + } + wantEqual(t, "credentials file mode after config create", info.Mode().Perm(), os.FileMode(0o600)) + c.run("node", "list", "--profile", "extra") +} + // testConfigCreateNameCollision adds a profile under a name the file // already has. Yes to "Update it instead?" updates that profile in place, // offering its current values; no asks for another name.