From 171f5b92ac838f8fd05a76835428d97689f86991 Mon Sep 17 00:00:00 2001 From: Seena Fallah Date: Sat, 18 May 2024 20:01:53 +0200 Subject: [PATCH 1/5] agent: allow changing file ownership in file sink Allow changing the ownership of the token file in file sink. Signed-off-by: Seena Fallah --- .../agentproxyshared/sink/file/file_sink.go | 30 ++++++- .../sink/file/file_sink_test.go | 89 ++++++++++++++----- .../agent-and-proxy/autoauth/sinks/file.mdx | 4 +- 3 files changed, 101 insertions(+), 22 deletions(-) diff --git a/command/agentproxyshared/sink/file/file_sink.go b/command/agentproxyshared/sink/file/file_sink.go index bf26a86b3c7b..6e1b71aa2e58 100644 --- a/command/agentproxyshared/sink/file/file_sink.go +++ b/command/agentproxyshared/sink/file/file_sink.go @@ -19,6 +19,8 @@ import ( type fileSink struct { path string mode os.FileMode + owner int + group int logger hclog.Logger } @@ -33,6 +35,8 @@ func NewFileSink(conf *sink.SinkConfig) (sink.Sink, error) { f := &fileSink{ logger: conf.Logger, mode: 0o640, + owner: os.Getuid(), + group: os.Getgid(), } pathRaw, ok := conf.Config["path"] @@ -61,11 +65,31 @@ func NewFileSink(conf *sink.SinkConfig) (sink.Sink, error) { f.mode = os.FileMode(mode) } + if modeRaw, ok := conf.Config["owner"]; ok { + owner, typeOK := modeRaw.(int) + if !typeOK { + return nil, errors.New("could not parse 'owner' as integer") + } + + f.logger.Debug("overriding default file sink", "owner", owner) + f.owner = owner + } + + if modeRaw, ok := conf.Config["group"]; ok { + group, typeOK := modeRaw.(int) + if !typeOK { + return nil, errors.New("could not parse 'group' as integer") + } + + f.logger.Debug("overriding default file sink", "group", group) + f.group = group + } + if err := f.WriteToken(""); err != nil { return nil, fmt.Errorf("error during write check: %w", err) } - f.logger.Info("file sink configured", "path", f.path, "mode", f.mode) + f.logger.Info("file sink configured", "path", f.path, "mode", f.mode, "owner", f.owner, "group", f.group) return f, nil } @@ -93,6 +117,10 @@ func (f *fileSink) WriteToken(token string) error { return fmt.Errorf("error opening temp file in dir %s for writing: %w", targetDir, err) } + if err := tmpFile.Chown(f.owner, f.group); err != nil { + return fmt.Errorf("error changing ownership of %s: %w", tmpFile.Name(), err) + } + valToWrite := token if token == "" { valToWrite = u diff --git a/command/agentproxyshared/sink/file/file_sink_test.go b/command/agentproxyshared/sink/file/file_sink_test.go index e603c6a32d3d..6072517bcec8 100644 --- a/command/agentproxyshared/sink/file/file_sink_test.go +++ b/command/agentproxyshared/sink/file/file_sink_test.go @@ -4,10 +4,9 @@ package file import ( - "fmt" - "io/ioutil" "os" "path/filepath" + "syscall" "testing" hclog "github.com/hashicorp/go-hclog" @@ -16,15 +15,8 @@ import ( "github.com/hashicorp/vault/sdk/helper/logging" ) -const ( - fileServerTestDir = "vault-agent-file-test" -) - func testFileSink(t *testing.T, log hclog.Logger) (*sink.SinkConfig, string) { - tmpDir, err := ioutil.TempDir("", fmt.Sprintf("%s.", fileServerTestDir)) - if err != nil { - t.Fatal(err) - } + tmpDir := t.TempDir() path := filepath.Join(tmpDir, "token") @@ -74,7 +66,7 @@ func TestFileSink(t *testing.T) { t.Fatal(err) } - fileBytes, err := ioutil.ReadFile(path) + fileBytes, err := os.ReadFile(path) if err != nil { t.Fatal(err) } @@ -84,19 +76,17 @@ func TestFileSink(t *testing.T) { } } -func testFileSinkMode(t *testing.T, log hclog.Logger) (*sink.SinkConfig, string) { - tmpDir, err := ioutil.TempDir("", fmt.Sprintf("%s.", fileServerTestDir)) - if err != nil { - t.Fatal(err) - } +func testFileSinkMode(t *testing.T, log hclog.Logger, gid int) (*sink.SinkConfig, string) { + tmpDir := t.TempDir() path := filepath.Join(tmpDir, "token") config := &sink.SinkConfig{ Logger: log.Named("sink.file"), Config: map[string]interface{}{ - "path": path, - "mode": 0o644, + "path": path, + "mode": 0o644, + "group": gid, }, } @@ -112,7 +102,62 @@ func testFileSinkMode(t *testing.T, log hclog.Logger) (*sink.SinkConfig, string) func TestFileSinkMode(t *testing.T) { log := logging.NewVaultLogger(hclog.Trace) - fs, tmpDir := testFileSinkMode(t, log) + fs, tmpDir := testFileSinkMode(t, log, os.Getegid()) + defer os.RemoveAll(tmpDir) + + path := filepath.Join(tmpDir, "token") + + uuidStr, _ := uuid.GenerateUUID() + if err := fs.WriteToken(uuidStr); err != nil { + t.Fatal(err) + } + + file, err := os.Open(path) + if err != nil { + t.Fatal(err) + } + defer file.Close() + + fi, err := file.Stat() + if err != nil { + t.Fatal(err) + } + if fi.Mode() != os.FileMode(0o644) { + t.Fatalf("wrong file mode was detected at %s", path) + } + + fileBytes, err := os.ReadFile(path) + if err != nil { + t.Fatal(err) + } + + if string(fileBytes) != uuidStr { + t.Fatalf("expected %s, got %s", uuidStr, string(fileBytes)) + } +} + +func TestFileSinkMode_Ownership(t *testing.T) { + groups, err := os.Getgroups() + if err != nil { + t.Fatal(err) + } + + if len(groups) < 2 { + t.Skip("not enough groups to test file ownership") + } + + // find a group that is not the current group + var gid int + for _, g := range groups { + if g != os.Getegid() { + gid = g + break + } + } + + log := logging.NewVaultLogger(hclog.Trace) + + fs, tmpDir := testFileSinkMode(t, log, gid) defer os.RemoveAll(tmpDir) path := filepath.Join(tmpDir, "token") @@ -135,8 +180,12 @@ func TestFileSinkMode(t *testing.T) { if fi.Mode() != os.FileMode(0o644) { t.Fatalf("wrong file mode was detected at %s", path) } + // check if file is owned by the group + if fi.Sys().(*syscall.Stat_t).Gid != uint32(gid) { + t.Fatalf("file is not owned by the group %d", gid) + } - fileBytes, err := ioutil.ReadFile(path) + fileBytes, err := os.ReadFile(path) if err != nil { t.Fatal(err) } diff --git a/website/content/docs/agent-and-proxy/autoauth/sinks/file.mdx b/website/content/docs/agent-and-proxy/autoauth/sinks/file.mdx index 9e96e8900eae..7a77d8e98379 100644 --- a/website/content/docs/agent-and-proxy/autoauth/sinks/file.mdx +++ b/website/content/docs/agent-and-proxy/autoauth/sinks/file.mdx @@ -21,7 +21,9 @@ written with `0640` permissions as default, but can be overridden with the optio ## Configuration - `path` `(string: required)` - The path to use to write the token file -- `mode` `(int: optional)` - A string containing an octal number representing the bit pattern for the file mode, similar to chmod. Set to `0000` to prevent Vault from modifying the file mode. Note: This configuration option is only available in Vault 1.3.0 and above. +- `mode` `(int: optional)` - Octal number string representing the bit pattern for the file mode, similar to `chmod`. +- `owner` `(int: optional)` - The UID to use for the token file. Defaults to the current user ID. +- `group` `(int: optional)` - The GID to use for token file. Defaults to the current group id. ~> Note: Configuration options for response-wrapping and encryption for the sink file are located within the [options common to all sinks](/vault/docs/agent-and-proxy/autoauth#configuration-sinks) documentation. From 2187d0491d4fd1709fdd662cff52f35e99d416cd Mon Sep 17 00:00:00 2001 From: Violet Hynes Date: Mon, 27 May 2024 13:13:42 -0400 Subject: [PATCH 2/5] Consistency: id -> ID --- website/content/docs/agent-and-proxy/autoauth/sinks/file.mdx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/website/content/docs/agent-and-proxy/autoauth/sinks/file.mdx b/website/content/docs/agent-and-proxy/autoauth/sinks/file.mdx index 7a77d8e98379..2ed451cbb3b2 100644 --- a/website/content/docs/agent-and-proxy/autoauth/sinks/file.mdx +++ b/website/content/docs/agent-and-proxy/autoauth/sinks/file.mdx @@ -23,7 +23,7 @@ written with `0640` permissions as default, but can be overridden with the optio - `path` `(string: required)` - The path to use to write the token file - `mode` `(int: optional)` - Octal number string representing the bit pattern for the file mode, similar to `chmod`. - `owner` `(int: optional)` - The UID to use for the token file. Defaults to the current user ID. -- `group` `(int: optional)` - The GID to use for token file. Defaults to the current group id. +- `group` `(int: optional)` - The GID to use for token file. Defaults to the current group ID. ~> Note: Configuration options for response-wrapping and encryption for the sink file are located within the [options common to all sinks](/vault/docs/agent-and-proxy/autoauth#configuration-sinks) documentation. From dfc9ebbdde9ccaa6d138e1c9bf42ef511ac734ba Mon Sep 17 00:00:00 2001 From: Violet Hynes Date: Mon, 27 May 2024 13:16:43 -0400 Subject: [PATCH 3/5] Add changelog --- changelog/27123.txt | 7 +++++++ 1 file changed, 7 insertions(+) create mode 100644 changelog/27123.txt diff --git a/changelog/27123.txt b/changelog/27123.txt new file mode 100644 index 000000000000..897a00559ff5 --- /dev/null +++ b/changelog/27123.txt @@ -0,0 +1,7 @@ +```release-note:improvement +agent/sink: Allow configuration of the user and group ID of the file sink. +``` +```release-note:improvement +proxy/sink: Allow configuration of the user and group ID of the file sink. +``` + From 7ac566df76e5443e3867339955cb5d512e38288e Mon Sep 17 00:00:00 2001 From: Violet Hynes Date: Mon, 27 May 2024 13:17:54 -0400 Subject: [PATCH 4/5] Remove empty line in changelog --- changelog/27123.txt | 1 - 1 file changed, 1 deletion(-) diff --git a/changelog/27123.txt b/changelog/27123.txt index 897a00559ff5..f23dd835c5ed 100644 --- a/changelog/27123.txt +++ b/changelog/27123.txt @@ -4,4 +4,3 @@ agent/sink: Allow configuration of the user and group ID of the file sink. ```release-note:improvement proxy/sink: Allow configuration of the user and group ID of the file sink. ``` - From bfc5615d1164c3471bc5a9fc33ad6b240829ddf2 Mon Sep 17 00:00:00 2001 From: Seena Fallah Date: Mon, 27 May 2024 21:36:05 +0200 Subject: [PATCH 5/5] agent: add godoc for TestFileSinkMode_Ownership Signed-off-by: Seena Fallah --- command/agentproxyshared/sink/file/file_sink_test.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/command/agentproxyshared/sink/file/file_sink_test.go b/command/agentproxyshared/sink/file/file_sink_test.go index 6072517bcec8..de7840748d14 100644 --- a/command/agentproxyshared/sink/file/file_sink_test.go +++ b/command/agentproxyshared/sink/file/file_sink_test.go @@ -136,6 +136,9 @@ func TestFileSinkMode(t *testing.T) { } } +// TestFileSinkMode_Ownership tests that the file is owned by the group specified +// in the configuration. This test requires the current user to be in at least two +// groups. If the user is not in two groups, the test will be skipped. func TestFileSinkMode_Ownership(t *testing.T) { groups, err := os.Getgroups() if err != nil {