From 9d223cb95fb75a48f1d8f0db2822259fc7fd55cb Mon Sep 17 00:00:00 2001 From: Lukas W Date: Wed, 2 Feb 2022 11:10:24 +0100 Subject: [PATCH 1/3] allow setting file ownership by name --- config/config_test.go | 12 +++---- config/template.go | 8 ++--- manager/runner.go | 4 +-- renderer/file_ownership.go | 50 +++++++++++++++++++++--------- renderer/file_ownership_windows.go | 21 ++++++++++--- renderer/renderer.go | 15 +++++++-- renderer/renderer_test.go | 29 +++++++---------- 7 files changed, 87 insertions(+), 52 deletions(-) diff --git a/config/config_test.go b/config/config_test.go index 26524ac8a..2656937e8 100644 --- a/config/config_test.go +++ b/config/config_test.go @@ -1096,12 +1096,12 @@ func TestParse(t *testing.T) { { "template_uid", `template { - uid = 1000 + user = "1000" }`, &Config{ Templates: &TemplateConfigs{ &TemplateConfig{ - Uid: Int(1000), + User: String("1000"), }, }, }, @@ -1110,12 +1110,12 @@ func TestParse(t *testing.T) { { "template_gid", `template { - gid = 1000 + group = "1000" }`, &Config{ Templates: &TemplateConfigs{ &TemplateConfig{ - Gid: Int(1000), + Group: String("1000"), }, }, }, @@ -1128,8 +1128,8 @@ func TestParse(t *testing.T) { &Config{ Templates: &TemplateConfigs{ &TemplateConfig{ - Uid: nil, - Gid: nil, + User: nil, + Group: nil, }, }, }, diff --git a/config/template.go b/config/template.go index 3dd34776d..1696dec98 100644 --- a/config/template.go +++ b/config/template.go @@ -64,19 +64,19 @@ type TemplateConfig struct { // secrets from Vault. Perms *os.FileMode `mapstructure:"perms"` - // Uid is the numeric uid that will be set when creating the file on disk. + // User is the username or uid that will be set when creating the file on disk. // Useful when simply setting Perms is not enough. // // Platform dependent: this doesn't work on Windows but it fails gracefully // with a warning - Uid *int `mapstructure:"uid"` + User *string `mapstructure:"user"` - // Gid is the numeric gid that will be set when creating the file on disk. + // Group is the group name or gid that will be set when creating the file on disk. // Useful when simply setting Perms is not enough. // // Platform dependent: this doesn't work on Windows but it fails gracefully // with a warning - Gid *int `mapstructure:"gid"` + Group *string `mapstructure:"group"` // Source is the path on disk to the template contents to evaluate. Either // this or Contents should be specified, but not both. diff --git a/manager/runner.go b/manager/runner.go index 951f43da1..a41857090 100644 --- a/manager/runner.go +++ b/manager/runner.go @@ -780,8 +780,8 @@ func (r *Runner) runTemplate(tmpl *template.Template, runCtx *templateRunCtx) (* DryStream: r.outStream, Path: config.StringVal(templateConfig.Destination), Perms: config.FileModeVal(templateConfig.Perms), - Uid: templateConfig.Uid, - Gid: templateConfig.Gid, + User: config.StringVal(templateConfig.User), + Group: config.StringVal(templateConfig.Group), }) if err != nil { return nil, errors.Wrap(err, "error rendering "+templateConfig.Display()) diff --git a/renderer/file_ownership.go b/renderer/file_ownership.go index f4879573c..0f86ed592 100644 --- a/renderer/file_ownership.go +++ b/renderer/file_ownership.go @@ -5,6 +5,8 @@ package renderer import ( "os" + "os/user" + "strconv" "syscall" ) @@ -20,30 +22,48 @@ func getFileOwnership(path string) (*int, *int) { return intPtr(int(st.Uid)), intPtr(int(st.Gid)) } -func setFileOwnership(path string, uid, gid *int) error { - wantedUid := sanitizeUidGid(uid) - wantedGid := sanitizeUidGid(gid) - if wantedUid == -1 && wantedGid == -1 { +func setFileOwnership(path string, uid, gid int) error { + if uid == -1 && gid == -1 { return nil //noop } - return os.Chown(path, wantedUid, wantedGid) + return os.Chown(path, uid, gid) } -func isChownNeeded(path string, uid, gid *int) bool { - wantedUid := sanitizeUidGid(uid) - wantedGid := sanitizeUidGid(gid) - if wantedUid == -1 && wantedGid == -1 { +func isChownNeeded(path string, uid, gid int) bool { + if uid == -1 && gid == -1 { return false } currUid, currGid := getFileOwnership(path) - return wantedUid != *currUid || wantedGid != *currGid + return uid != *currUid || gid != *currGid } -// sanitizeUidGid sanitizes the uid/gid so that can be an input for os.Chown -func sanitizeUidGid(id *int) int { - if id == nil { - return -1 +// parseUidGid parses the uid/gid so that it can be input to os.Chown +func parseUidGid(s string) (int, error) { + if s == "" { + return -1, nil } - return *id + return strconv.Atoi(s) +} + +func lookupUser(s string) (int, error) { + if id, err := parseUidGid(s); err == nil { + return id, nil + } + u, err := user.Lookup(s) + if err != nil { + return 0, err + } + return strconv.Atoi(u.Uid) +} + +func lookupGroup(s string) (int, error) { + if id, err := parseUidGid(s); err == nil { + return id, nil + } + u, err := user.LookupGroup(s) + if err != nil { + return 0, err + } + return strconv.Atoi(u.Gid) } diff --git a/renderer/file_ownership_windows.go b/renderer/file_ownership_windows.go index 79397e6fc..eaa491d4e 100644 --- a/renderer/file_ownership_windows.go +++ b/renderer/file_ownership_windows.go @@ -7,13 +7,24 @@ import ( "log" ) -func setFileOwnership(path string, uid, gid *int) error { - if uid != nil || gid != nil { - log.Printf("[WARN] (runner) cannot set uid/gid for rendered files on Windows") - } +func setFileOwnership(path string, uid, gid int) error { return nil } -func isChownNeeded(path string, wantedUid, wantedGid *int) bool { +func isChownNeeded(path string, wantedUid, wantedGid int) bool { return false } + +func lookupUser(user string) (int, error) { + if user != "" { + log.Printf("[WARN] (runner) cannot set user for rendered files on Windows") + } + return -1, nil +} + +func lookupGroup(group string) (int, error) { + if group != "" { + log.Printf("[WARN] (runner) cannot set group for rendered files on Windows") + } + return -1, nil +} diff --git a/renderer/renderer.go b/renderer/renderer.go index 6190381a1..6a04e548c 100644 --- a/renderer/renderer.go +++ b/renderer/renderer.go @@ -36,7 +36,7 @@ type RenderInput struct { DryStream io.Writer Path string Perms os.FileMode - Uid, Gid *int + User, Group string } // RenderResult is returned and stored. It contains the status of the render @@ -66,10 +66,19 @@ func Render(i *RenderInput) (*RenderResult, error) { return nil, errors.Wrap(err, "failed reading file") } + uid, err := lookupUser(i.User) + if err != nil { + return nil, errors.Wrap(err, "failed looking up user") + } + gid, err := lookupGroup(i.Group) + if err != nil { + return nil, errors.Wrap(err, "failed looking up group") + } + var chownNeeded bool if fileExists { - chownNeeded = isChownNeeded(i.Path, i.Uid, i.Gid) + chownNeeded = isChownNeeded(i.Path, uid, gid) } if bytes.Equal(existing, i.Contents) && fileExists && !chownNeeded { @@ -87,7 +96,7 @@ func Render(i *RenderInput) (*RenderResult, error) { return nil, errors.Wrap(err, "failed writing file") } - if err = setFileOwnership(i.Path, i.Uid, i.Gid); err != nil { + if err = setFileOwnership(i.Path, uid, gid); err != nil { return nil, errors.Wrap(err, "failed setting file ownership") } } diff --git a/renderer/renderer_test.go b/renderer/renderer_test.go index f3540f65c..ede6be644 100644 --- a/renderer/renderer_test.go +++ b/renderer/renderer_test.go @@ -6,6 +6,7 @@ import ( "os" "path" "path/filepath" + "strconv" "testing" ) @@ -320,8 +321,6 @@ func TestRender_Chown(t *testing.T) { // Can't change uid unless root, but can try // changing the group id. - // setting Uid to -1 means no change - wantedUid := -1 // we enumerate the groups the current user (running the tests) belongs to callerGroups, err := os.Getgroups() @@ -365,8 +364,7 @@ func TestRender_Chown(t *testing.T) { rr, err := Render(&RenderInput{ Path: path, Contents: contents, - Uid: intPtr(wantedUid), - Gid: intPtr(wantedGid), + Group: strconv.Itoa(wantedGid), }) if err != nil { t.Fatal(err) @@ -410,8 +408,7 @@ func TestRender_Chown(t *testing.T) { rr, err := Render(&RenderInput{ Path: path, Contents: diff_contents, - Uid: intPtr(wantedUid), - Gid: intPtr(wantedGid), + Group: strconv.Itoa(wantedGid), }) if err != nil { t.Fatal(err) @@ -443,8 +440,7 @@ func TestRender_Chown(t *testing.T) { rr, err := Render(&RenderInput{ Path: path, Contents: contents, - Uid: intPtr(wantedUid), - Gid: intPtr(wantedGid), + Group: strconv.Itoa(wantedGid), }) if err != nil { t.Fatal(err) @@ -476,8 +472,7 @@ func TestRender_Chown(t *testing.T) { rr, err := Render(&RenderInput{ Path: path, Contents: contents, - Uid: intPtr(wantedUid), - Gid: intPtr(wantedGid), + Group: strconv.Itoa(wantedGid), }) if err != nil { t.Fatal(err) @@ -496,7 +491,7 @@ func TestRender_Chown(t *testing.T) { } }) - t.Run("should-be-noop-when-missing-uid", func(t *testing.T) { + t.Run("should-be-noop-when-missing-user", func(t *testing.T) { outDir, err := ioutil.TempDir("", "") if err != nil { @@ -523,7 +518,7 @@ func TestRender_Chown(t *testing.T) { rr, err := Render(&RenderInput{ Path: path, Contents: diff_contents, - Gid: intPtr(-1), + Group: "", }) if err != nil { t.Fatal(err) @@ -542,7 +537,7 @@ func TestRender_Chown(t *testing.T) { } }) - t.Run("should-be-noop-when-missing-gid", func(t *testing.T) { + t.Run("should-be-noop-when-missing-group", func(t *testing.T) { outDir, err := ioutil.TempDir("", "") if err != nil { @@ -569,7 +564,7 @@ func TestRender_Chown(t *testing.T) { rr, err := Render(&RenderInput{ Path: path, Contents: diff_contents, - Uid: intPtr(-1), + User: "", }) if err != nil { t.Fatal(err) @@ -588,7 +583,7 @@ func TestRender_Chown(t *testing.T) { } }) - t.Run("should-be-noop-when-uid-and-gid-are-both-set-to-minus1", func(t *testing.T) { + t.Run("should-be-noop-when-user-and-group-are-both-empty", func(t *testing.T) { outDir, err := ioutil.TempDir("", "") if err != nil { @@ -615,8 +610,8 @@ func TestRender_Chown(t *testing.T) { rr, err := Render(&RenderInput{ Path: path, Contents: diff_contents, - Uid: intPtr(-1), - Gid: intPtr(-1), + User: "", + Group: "", }) if err != nil { t.Fatal(err) From af770eccf6ec1fd2b6722621cea4af50042adb1e Mon Sep 17 00:00:00 2001 From: Lukas W Date: Fri, 4 Feb 2022 09:21:16 +0100 Subject: [PATCH 2/3] handle os.Stat error in getFilePermissions Introduced in 24cb14239c10d7f8dc4536d12c4d8c878ec93364: An error in os.Stat would lead to a nil pointer dereference in the calling function --- renderer/file_ownership.go | 23 +++++---- renderer/file_ownership_windows.go | 4 +- renderer/renderer.go | 6 ++- renderer/renderer_test.go | 79 ++++++++++++++++++++---------- 4 files changed, 74 insertions(+), 38 deletions(-) diff --git a/renderer/file_ownership.go b/renderer/file_ownership.go index 0f86ed592..ff82d79f3 100644 --- a/renderer/file_ownership.go +++ b/renderer/file_ownership.go @@ -10,16 +10,16 @@ import ( "syscall" ) -func getFileOwnership(path string) (*int, *int) { - file_info, err := os.Stat(path) +func getFileOwnership(path string) (int, int, error) { + fileInfo, err := os.Stat(path) if err != nil { - return nil, nil + return 0, 0, err } - file_sys := file_info.Sys() - st := file_sys.(*syscall.Stat_t) - return intPtr(int(st.Uid)), intPtr(int(st.Gid)) + fileSys := fileInfo.Sys() + st := fileSys.(*syscall.Stat_t) + return int(st.Uid), int(st.Gid), nil } func setFileOwnership(path string, uid, gid int) error { @@ -29,13 +29,16 @@ func setFileOwnership(path string, uid, gid int) error { return os.Chown(path, uid, gid) } -func isChownNeeded(path string, uid, gid int) bool { +func isChownNeeded(path string, uid, gid int) (bool, error) { if uid == -1 && gid == -1 { - return false + return false, nil } - currUid, currGid := getFileOwnership(path) - return uid != *currUid || gid != *currGid + currUid, currGid, err := getFileOwnership(path) + if err != nil { + return false, err + } + return uid != currUid || gid != currGid, nil } // parseUidGid parses the uid/gid so that it can be input to os.Chown diff --git a/renderer/file_ownership_windows.go b/renderer/file_ownership_windows.go index eaa491d4e..84e23fd27 100644 --- a/renderer/file_ownership_windows.go +++ b/renderer/file_ownership_windows.go @@ -11,8 +11,8 @@ func setFileOwnership(path string, uid, gid int) error { return nil } -func isChownNeeded(path string, wantedUid, wantedGid int) bool { - return false +func isChownNeeded(path string, wantedUid, wantedGid int) (bool, error) { + return false, nil } func lookupUser(user string) (int, error) { diff --git a/renderer/renderer.go b/renderer/renderer.go index 6a04e548c..7808ed24e 100644 --- a/renderer/renderer.go +++ b/renderer/renderer.go @@ -78,7 +78,11 @@ func Render(i *RenderInput) (*RenderResult, error) { var chownNeeded bool if fileExists { - chownNeeded = isChownNeeded(i.Path, uid, gid) + chownNeeded, err = isChownNeeded(i.Path, uid, gid) + if err != nil { + log.Printf("[WARN] (runner) could not determine existing output file's permissions") + chownNeeded = true + } } if bytes.Equal(existing, i.Contents) && fileExists && !chownNeeded { diff --git a/renderer/renderer_test.go b/renderer/renderer_test.go index ede6be644..f674fa1a8 100644 --- a/renderer/renderer_test.go +++ b/renderer/renderer_test.go @@ -376,10 +376,13 @@ func TestRender_Chown(t *testing.T) { rr.WouldRender, rr.DidRender) } - gotUid, gotGid := getFileOwnership(path) - if *gotGid != wantedGid { + gotUid, gotGid, err := getFileOwnership(path) + if err != nil { + t.Fatalf("getFileOwnership: %s", err) + } + if gotGid != wantedGid { t.Fatalf("Bad render results; gotUid: %v, wantedGid: %v, gotGid: %v", - *gotUid, wantedGid, *gotGid) + gotUid, wantedGid, gotGid) } }) @@ -420,10 +423,13 @@ func TestRender_Chown(t *testing.T) { rr.WouldRender, rr.DidRender) } - gotUid, gotGid := getFileOwnership(path) - if *gotGid != wantedGid { + gotUid, gotGid, err := getFileOwnership(path) + if err != nil { + t.Fatalf("getFileOwnership: %s", err) + } + if gotGid != wantedGid { t.Fatalf("Bad render results; gotUid: %v, wantedGid: %v, gotGid: %v", - *gotUid, wantedGid, *gotGid) + gotUid, wantedGid, gotGid) } }) @@ -452,10 +458,13 @@ func TestRender_Chown(t *testing.T) { rr.WouldRender, rr.DidRender) } - gotUid, gotGid := getFileOwnership(path) - if *gotGid != wantedGid { + gotUid, gotGid, err := getFileOwnership(path) + if err != nil { + t.Fatalf("getFileOwnership: %s", err) + } + if gotGid != wantedGid { t.Fatalf("Bad render results; gotUid: %v, wantedGid: %v, gotGid: %v", - *gotUid, wantedGid, *gotGid) + gotUid, wantedGid, gotGid) } }) @@ -484,10 +493,13 @@ func TestRender_Chown(t *testing.T) { rr.WouldRender, rr.DidRender) } - gotUid, gotGid := getFileOwnership(path) - if *gotGid != wantedGid { + gotUid, gotGid, err := getFileOwnership(path) + if err != nil { + t.Fatalf("getFileOwnership: %s", err) + } + if gotGid != wantedGid { t.Fatalf("Bad render results; gotUid: %v, wantedGid: %v, gotGid: %v", - *gotUid, wantedGid, *gotGid) + gotUid, wantedGid, gotGid) } }) @@ -512,8 +524,10 @@ func TestRender_Chown(t *testing.T) { } // getting file uid:gid for the default behaviour - wantUid, wantGid := getFileOwnership(path) - + wantUid, wantGid, err := getFileOwnership(path) + if err != nil { + t.Fatalf("getFileOwnership: %s", err) + } diff_contents := []byte("not-first") rr, err := Render(&RenderInput{ Path: path, @@ -530,10 +544,13 @@ func TestRender_Chown(t *testing.T) { rr.WouldRender, rr.DidRender) } - gotUid, gotGid := getFileOwnership(path) - if *gotUid != *wantUid || *gotGid != *wantGid { + gotUid, gotGid, err := getFileOwnership(path) + if err != nil { + t.Fatalf("getFileOwnership: %s", err) + } + if gotUid != wantUid || gotGid != wantGid { t.Fatalf("Bad render results; we shouldn't have altered uid/gid. wantUid: %v, wantGid: %v, gotUid: %v, gotGid: %v ", - *wantUid, *wantGid, *gotUid, *gotGid) + wantUid, wantGid, gotUid, gotGid) } }) @@ -558,7 +575,10 @@ func TestRender_Chown(t *testing.T) { } // getting file uid:gid for the default behaviour - wantUid, wantGid := getFileOwnership(path) + wantUid, wantGid, err := getFileOwnership(path) + if err != nil { + t.Fatalf("getFileOwnership: %s", err) + } diff_contents := []byte("not-first") rr, err := Render(&RenderInput{ @@ -576,10 +596,13 @@ func TestRender_Chown(t *testing.T) { rr.WouldRender, rr.DidRender) } - gotUid, gotGid := getFileOwnership(path) - if *gotUid != *wantUid || *gotGid != *wantGid { + gotUid, gotGid, err := getFileOwnership(path) + if err != nil { + t.Fatalf("getFileOwnership: %s", err) + } + if gotUid != wantUid || gotGid != wantGid { t.Fatalf("Bad render results; we shouldn't have altered uid/gid. wantUid: %v, wantGid: %v, gotUid: %v, gotGid: %v ", - *wantUid, *wantGid, *gotUid, *gotGid) + wantUid, wantGid, gotUid, gotGid) } }) @@ -604,7 +627,10 @@ func TestRender_Chown(t *testing.T) { } // getting file uid:gid for the default behaviour - wantUid, wantGid := getFileOwnership(path) + wantUid, wantGid, err := getFileOwnership(path) + if err != nil { + t.Fatalf("getFileOwnership: %s", err) + } diff_contents := []byte("not-first") rr, err := Render(&RenderInput{ @@ -623,10 +649,13 @@ func TestRender_Chown(t *testing.T) { rr.WouldRender, rr.DidRender) } - gotUid, gotGid := getFileOwnership(path) - if *gotUid != *wantUid || *gotGid != *wantGid { + gotUid, gotGid, err := getFileOwnership(path) + if err != nil { + t.Fatalf("getFileOwnership: %s", err) + } + if gotUid != wantUid || gotGid != wantGid { t.Fatalf("Bad render results; we shouldn't have altered uid/gid. wantUid: %v, wantGid: %v, gotUid: %v, gotGid: %v ", - *wantUid, *wantGid, *gotUid, *gotGid) + wantUid, wantGid, gotUid, gotGid) } }) } From 5be1f708cd67dc7136e8534b924cb67cd244cd6f Mon Sep 17 00:00:00 2001 From: Lukas W Date: Fri, 4 Feb 2022 09:33:22 +0100 Subject: [PATCH 3/3] fail when trying to chown on Windows --- renderer/file_ownership_windows.go | 26 +++++++++++++++++--------- 1 file changed, 17 insertions(+), 9 deletions(-) diff --git a/renderer/file_ownership_windows.go b/renderer/file_ownership_windows.go index 84e23fd27..a88db2559 100644 --- a/renderer/file_ownership_windows.go +++ b/renderer/file_ownership_windows.go @@ -4,27 +4,35 @@ package renderer import ( - "log" + "fmt" ) +var notSupportedError error = fmt.Errorf("managing file ownership is not supported on Windows") + func setFileOwnership(path string, uid, gid int) error { - return nil + if uid == -1 && gid == -1 { + return nil + } + return notSupportedError } func isChownNeeded(path string, wantedUid, wantedGid int) (bool, error) { - return false, nil + if wantedUid == -1 && wantedGid == -1 { + return false, nil + } + return false, notSupportedError } func lookupUser(user string) (int, error) { - if user != "" { - log.Printf("[WARN] (runner) cannot set user for rendered files on Windows") + if user == "" { + return -1, nil } - return -1, nil + return 0, notSupportedError } func lookupGroup(group string) (int, error) { - if group != "" { - log.Printf("[WARN] (runner) cannot set group for rendered files on Windows") + if group == "" { + return -1, nil } - return -1, nil + return 0, notSupportedError }