diff --git a/agent-config.yaml b/agent-config.yaml index 210c2141..300ff114 100644 --- a/agent-config.yaml +++ b/agent-config.yaml @@ -10,6 +10,8 @@ sinks: - type: "file" config: path: "access-token" + # Optional octal mode for the written token. Defaults to 0644. + permission: "0600" templates: - template-content: | {{- with secret "202f04d7-e4cb-43d4-a292-e893712d61fc" "dev" "/" }} @@ -20,6 +22,9 @@ templates: destination-path: my-dot-env-0.env config: polling-interval: 60s + # Optional octal mode for the rendered file. When omitted the mode is + # left to the process umask, which usually means 0644. + permission: "0600" execute: command: docker-compose -f docker-compose.prod.yml down && docker-compose -f docker-compose.prod.yml up -d diff --git a/packages/cmd/agent.go b/packages/cmd/agent.go index 93ce80e5..f93528a7 100644 --- a/packages/cmd/agent.go +++ b/packages/cmd/agent.go @@ -179,7 +179,8 @@ type Sink struct { } type SinkDetails struct { - Path string `yaml:"path"` + Path string `yaml:"path"` + Permission string `yaml:"permission"` // Octal file mode for the written token, e.g. "0600" } type Template struct { @@ -190,6 +191,7 @@ type Template struct { Config struct { // Configurations for the template PollingInterval string `yaml:"polling-interval"` // How often to poll for changes in the secret + Permission string `yaml:"permission"` // Octal file mode for the rendered file, e.g. "0600" Execute struct { Command string `yaml:"command"` // Command to execute once the template has been rendered Timeout int64 `yaml:"timeout"` // Timeout for the command @@ -785,14 +787,64 @@ func FileExists(filepath string) bool { return !info.IsDir() } -// WriteToFile writes data to the specified file path. -func WriteBytesToFile(data *bytes.Buffer, outputPath string) error { - outputFile, err := os.Create(outputPath) +// parseFileMode parses an octal file-mode string from the agent config, such +// as "0600". An empty string yields a nil mode, which callers treat as "keep +// the existing behaviour" rather than forcing a permission on the file. +// Modes above 0777 are rejected so that setuid, setgid and the sticky bit +// cannot be set on a file holding secret material. +func parseFileMode(permission string) (*os.FileMode, error) { + if permission == "" { + return nil, nil + } + + parsed, err := strconv.ParseUint(permission, 8, 32) + if err != nil { + return nil, fmt.Errorf("invalid file permission %q: expected an octal mode such as \"0600\"", permission) + } + + if parsed > 0777 { + return nil, fmt.Errorf("invalid file permission %q: must not exceed \"0777\"", permission) + } + + mode := os.FileMode(parsed) + return &mode, nil +} + +// chmodFile is indirected so the chmod failure path can be covered by a test. +// fchmod cannot be made to fail portably from an unprivileged process, and the +// behaviour on failure matters enough to be worth pinning down. +var chmodFile = (*os.File).Chmod + +// WriteBytesToFile writes data to the specified file path, creating it with the +// configured mode rather than os.Create's 0666. +// +// A nil mode preserves the historical behaviour of leaving the mode to the +// umask. +func WriteBytesToFile(data *bytes.Buffer, outputPath string, mode *os.FileMode) error { + createMode := os.FileMode(0666) + if mode != nil { + createMode = *mode + } + + outputFile, err := os.OpenFile(outputPath, os.O_WRONLY|os.O_CREATE, createMode) if err != nil { return err } defer outputFile.Close() + if mode != nil { + if err := chmodFile(outputFile, *mode); err != nil { + return fmt.Errorf("unable to set permission %#o on %s: %w", *mode, outputPath, err) + } + } + + // Without O_TRUNC this is what stops a shorter render from leaving + // trailing bytes of the previous one behind. Truncating here avoids a situation + // where we cannot write to file due to ownership, yet we truncate it + if err := outputFile.Truncate(0); err != nil { + return fmt.Errorf("unable to truncate %s: %w", outputPath, err) + } + _, err = outputFile.Write(data.Bytes()) return err } @@ -878,9 +930,33 @@ func parseAgentConfigWithMode(configFile []byte, isCertManagerMode bool) (*Confi return nil, err } + if err := validateFilePermissions(&rawConfig); err != nil { + return nil, err + } + return &rawConfig, nil } +// validateFilePermissions rejects malformed permission values while the config +// is being parsed. Catching a typo here fails the agent at startup instead of +// silently falling back to a looser mode at the first render, which would be +// invisible until someone stats the file. +func validateFilePermissions(config *Config) error { + for i, sink := range config.Sinks { + if _, err := parseFileMode(sink.Config.Permission); err != nil { + return fmt.Errorf("sinks[%d].config.permission: %w", i, err) + } + } + + for i, template := range config.Templates { + if _, err := parseFileMode(template.Config.Permission); err != nil { + return fmt.Errorf("templates[%d].config.permission: %w", i, err) + } + } + + return nil +} + type secretArguments struct { IsRecursive bool `json:"recursive"` ShouldExpandSecretReferences *bool `json:"expandSecretReferences,omitempty"` @@ -1804,11 +1880,29 @@ func (tm *AgentManager) ManageTokenLifecycle() { func (tm *AgentManager) WriteTokenToFiles() { token := tm.GetToken() + writeSink := func(path string, mode *os.FileMode) error { + if mode == nil { + // Historical behaviour: 0644 is applied only when the file is + // created, so an existing sink keeps whatever mode it already has. + return os.WriteFile(path, []byte(token), 0644) + } + return WriteBytesToFile(bytes.NewBufferString(token), path, mode) + } + for _, sinkFile := range tm.filePaths { if sinkFile.Type == "file" { - err := ioutil.WriteFile(sinkFile.Config.Path, []byte(token), 0644) + // Already validated when the agent config was parsed, so an error + // here is not reachable in practice; fall back to the historical + // mode rather than dropping the token. + mode, err := parseFileMode(sinkFile.Config.Permission) if err != nil { + log.Warn().Msgf("sink '%s': %v. Falling back to the default file mode", sinkFile.Config.Path, err) + mode = nil + } + + if err := writeSink(sinkFile.Config.Path, mode); err != nil { log.Error().Msgf("unable to write file sink to path '%s' because %v", sinkFile.Config.Path, err) + continue } log.Info().Msgf("new access token saved to file at path '%s'", sinkFile.Config.Path) @@ -1838,7 +1932,16 @@ func (tm *AgentManager) FetchTokenFromFiles() string { } func (tm *AgentManager) WriteTemplateToFile(bytes *bytes.Buffer, template *Template, templateId int) { - if err := WriteBytesToFile(bytes, template.DestinationPath); err != nil { + // Already validated when the agent config was parsed, so an error here is + // not reachable in practice; fall back to the umask default rather than + // dropping the render. + mode, err := parseFileMode(template.Config.Permission) + if err != nil { + log.Warn().Msgf("template engine: %v. Falling back to the default file mode", err) + mode = nil + } + + if err := WriteBytesToFile(bytes, template.DestinationPath, mode); err != nil { log.Error().Msgf("template engine: unable to write secrets to path because %s. Will try again on next cycle", err) return } @@ -2673,13 +2776,19 @@ func (tm *AgentManager) handleFailedCertificateRequest(certificateId int, errorM } func (tm *AgentManager) WriteCertificateFiles(certificate *AgentCertificateConfig, response *api.CertificateResponse) error { + // Certificates keep their long-standing 0600 default, including when the + // configured value is malformed. The warning is new: previously a typo was + // silently swallowed. getFilePermission := func(permission string) os.FileMode { - if permission != "" { - if perms, err := strconv.ParseInt(permission, 8, 32); err == nil { - return os.FileMode(perms) - } + mode, err := parseFileMode(permission) + if err != nil { + log.Warn().Msgf("certificate file permission: %v. Falling back to 0600", err) + return os.FileMode(0600) + } + if mode == nil { + return os.FileMode(0600) } - return os.FileMode(0600) + return *mode } privateKeyPath := certificate.FileConfig.PrivateKey.Path diff --git a/packages/cmd/agent_file_permission_test.go b/packages/cmd/agent_file_permission_test.go new file mode 100644 index 00000000..90f1aaed --- /dev/null +++ b/packages/cmd/agent_file_permission_test.go @@ -0,0 +1,299 @@ +//go:build unix + +// File modes are a POSIX concept. On Windows os.Chmod only toggles the +// read-only bit, so these tests would assert something meaningless there. +package cmd + +import ( + "bytes" + "errors" + "os" + "path/filepath" + "testing" + + "github.com/Infisical/infisical-merge/packages/config" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestParseFileMode(t *testing.T) { + cases := []struct { + name string + permission string + want *os.FileMode + wantErr bool + }{ + {name: "empty means unset", permission: "", want: nil}, + {name: "leading zero", permission: "0600", want: fileMode(0600)}, + {name: "without leading zero", permission: "600", want: fileMode(0600)}, + {name: "world readable", permission: "0644", want: fileMode(0644)}, + {name: "maximum", permission: "0777", want: fileMode(0777)}, + {name: "not a number", permission: "invalid", wantErr: true}, + {name: "not octal", permission: "0800", wantErr: true}, + {name: "setuid rejected", permission: "4600", wantErr: true}, + {name: "sticky bit rejected", permission: "1777", wantErr: true}, + {name: "negative", permission: "-1", wantErr: true}, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + got, err := parseFileMode(tc.permission) + + if tc.wantErr { + require.Error(t, err) + assert.Contains(t, err.Error(), tc.permission) + return + } + + require.NoError(t, err) + if tc.want == nil { + assert.Nil(t, got) + return + } + require.NotNil(t, got) + assert.Equal(t, *tc.want, *got) + }) + } +} + +func TestWriteBytesToFileAppliesConfiguredMode(t *testing.T) { + destination := filepath.Join(t.TempDir(), "app.env") + + require.NoError(t, WriteBytesToFile(bytes.NewBufferString("SECRET=1"), destination, fileMode(0600))) + + assert.Equal(t, os.FileMode(0600), statMode(t, destination)) + assert.Equal(t, "SECRET=1", readFile(t, destination)) +} + +// The case a bare os.Create gets wrong: a mode passed to open is only honoured +// when the file is created, so an already-present file keeps its old mode +// unless it is chmod'ed explicitly. This is also umask-independent, which +// makes it the strongest evidence that the chmod is doing the work. +func TestWriteBytesToFileTightensExistingFile(t *testing.T) { + destination := filepath.Join(t.TempDir(), "app.env") + require.NoError(t, os.WriteFile(destination, []byte("stale"), 0666)) + require.NoError(t, os.Chmod(destination, 0666)) + + require.NoError(t, WriteBytesToFile(bytes.NewBufferString("SECRET=1"), destination, fileMode(0600))) + + assert.Equal(t, os.FileMode(0600), statMode(t, destination)) + assert.Equal(t, "SECRET=1", readFile(t, destination)) +} + +// A nil mode must behave exactly like the pre-existing implementation, which +// left the mode to the process umask. Comparing against a reference file keeps +// the assertion correct under any umask the test runner happens to have. +func TestWriteBytesToFileNilModeMatchesOsCreate(t *testing.T) { + dir := t.TempDir() + destination := filepath.Join(dir, "app.env") + reference := filepath.Join(dir, "reference.env") + + referenceFile, err := os.Create(reference) + require.NoError(t, err) + require.NoError(t, referenceFile.Close()) + + require.NoError(t, WriteBytesToFile(bytes.NewBufferString("SECRET=1"), destination, nil)) + + assert.Equal(t, statMode(t, reference), statMode(t, destination)) +} + +// A chmod failure must not cost the caller the content that was already on +// disk. This is reachable whenever the destination is writable but owned by +// another user, where the open succeeds and the fchmod returns EPERM. The +// failure is injected because fchmod cannot be made to fail portably from an +// unprivileged test process. +func TestWriteBytesToFilePreservesContentWhenChmodFails(t *testing.T) { + original := chmodFile + t.Cleanup(func() { chmodFile = original }) + chmodFile = func(*os.File, os.FileMode) error { + return errors.New("operation not permitted") + } + + destination := filepath.Join(t.TempDir(), "app.env") + require.NoError(t, os.WriteFile(destination, []byte("PREVIOUS=1"), 0644)) + + err := WriteBytesToFile(bytes.NewBufferString("SECRET=1"), destination, fileMode(0600)) + + require.Error(t, err) + assert.Equal(t, "PREVIOUS=1", readFile(t, destination)) +} + +// The open no longer passes O_TRUNC, so guard the explicit truncate that +// replaced it. Without it a shorter render would leave a tail of the previous +// one, which for a dotenv file means stale secrets the consumer still parses. +func TestWriteBytesToFileReplacesLongerContent(t *testing.T) { + for _, tc := range []struct { + name string + mode *os.FileMode + }{ + {name: "with configured mode", mode: fileMode(0600)}, + {name: "without configured mode", mode: nil}, + } { + t.Run(tc.name, func(t *testing.T) { + destination := filepath.Join(t.TempDir(), "app.env") + require.NoError(t, os.WriteFile(destination, []byte("PREVIOUS=aaaaaaaaaaaaaaaaaaaaaaaa"), 0644)) + + require.NoError(t, WriteBytesToFile(bytes.NewBufferString("NEW=1"), destination, tc.mode)) + + assert.Equal(t, "NEW=1", readFile(t, destination)) + }) + } +} + +// WriteTemplateToFile is the function the reported issue is about, so cover +// the whole path from a parsed template config through to the mode on disk. +// It never touches its receiver, hence the zero-value AgentManager. +func TestWriteTemplateToFileAppliesConfiguredPermission(t *testing.T) { + destination := filepath.Join(t.TempDir(), "app.env") + + template := &Template{DestinationPath: destination} + template.Config.Permission = "0600" + + (&AgentManager{}).WriteTemplateToFile(bytes.NewBufferString("SECRET=1"), template, 1) + + assert.Equal(t, os.FileMode(0600), statMode(t, destination)) + assert.Equal(t, "SECRET=1", readFile(t, destination)) +} + +func TestWriteTemplateToFileTightensExistingFile(t *testing.T) { + destination := filepath.Join(t.TempDir(), "app.env") + require.NoError(t, os.WriteFile(destination, []byte("stale"), 0666)) + require.NoError(t, os.Chmod(destination, 0666)) + + template := &Template{DestinationPath: destination} + template.Config.Permission = "0600" + + (&AgentManager{}).WriteTemplateToFile(bytes.NewBufferString("SECRET=1"), template, 1) + + assert.Equal(t, os.FileMode(0600), statMode(t, destination)) + assert.Equal(t, "SECRET=1", readFile(t, destination)) +} + +func TestWriteTemplateToFileWithoutPermissionMatchesOsCreate(t *testing.T) { + dir := t.TempDir() + destination := filepath.Join(dir, "app.env") + reference := filepath.Join(dir, "reference.env") + + referenceFile, err := os.Create(reference) + require.NoError(t, err) + require.NoError(t, referenceFile.Close()) + + (&AgentManager{}).WriteTemplateToFile( + bytes.NewBufferString("SECRET=1"), + &Template{DestinationPath: destination}, + 1, + ) + + assert.Equal(t, statMode(t, reference), statMode(t, destination)) + assert.Equal(t, "SECRET=1", readFile(t, destination)) +} + +func TestParseAgentConfigReadsPermissions(t *testing.T) { + snapshotInfisicalURL(t) + + parsed, err := ParseAgentConfig([]byte(` +sinks: + - type: file + config: + path: access-token + permission: "0600" +templates: + - source-path: dotenv.tmpl + destination-path: app.env + config: + polling-interval: 60s + permission: "0640" +`)) + require.NoError(t, err) + + assert.Equal(t, "0600", parsed.Sinks[0].Config.Permission) + assert.Equal(t, "0640", parsed.Templates[0].Config.Permission) +} + +// A typo must stop the agent at startup. Falling through to a looser mode at +// the first render would be invisible until somebody stats the file. +func TestParseAgentConfigRejectsInvalidPermissions(t *testing.T) { + cases := []struct { + name string + configFile string + wantField string + }{ + { + name: "template", + wantField: "templates[0].config.permission", + configFile: ` +templates: + - source-path: dotenv.tmpl + destination-path: app.env + config: + permission: "600x" +`, + }, + { + name: "sink", + wantField: "sinks[0].config.permission", + configFile: ` +sinks: + - type: file + config: + path: access-token + permission: "rw-------" +`, + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + snapshotInfisicalURL(t) + + _, err := ParseAgentConfig([]byte(tc.configFile)) + + require.Error(t, err) + assert.Contains(t, err.Error(), tc.wantField) + }) + } +} + +func TestParseAgentConfigAllowsOmittedPermissions(t *testing.T) { + snapshotInfisicalURL(t) + + parsed, err := ParseAgentConfig([]byte(` +sinks: + - type: file + config: + path: access-token +templates: + - source-path: dotenv.tmpl + destination-path: app.env +`)) + require.NoError(t, err) + + assert.Empty(t, parsed.Sinks[0].Config.Permission) + assert.Empty(t, parsed.Templates[0].Config.Permission) +} + +func fileMode(mode os.FileMode) *os.FileMode { + return &mode +} + +func statMode(t *testing.T, path string) os.FileMode { + t.Helper() + info, err := os.Stat(path) + require.NoError(t, err) + return info.Mode().Perm() +} + +func readFile(t *testing.T, path string) string { + t.Helper() + contents, err := os.ReadFile(path) + require.NoError(t, err) + return string(contents) +} + +// ParseAgentConfig writes the resolved address into package-level config, so +// snapshot it and let withMockInfisicalURL restore it during cleanup. Passing +// the current value keeps this a pure save and restore. +func snapshotInfisicalURL(t *testing.T) { + t.Helper() + withMockInfisicalURL(t, config.INFISICAL_URL) +}