From: Simon Ruderich Date: Sun, 19 Oct 2025 07:19:01 +0000 (+0200) Subject: Remove permission checks on local files and remove `safcm fixperms` X-Git-Url: https://ruderich.org/simon/gitweb/?a=commitdiff_plain;h=17667dca712f61e07c0ec31ad88be2d6b4806b32;p=safcm%2Fsafcm.git Remove permission checks on local files and remove `safcm fixperms` The intended use case was to "protect" users against unexpected behavior when migrating from a legacy system configuration management. This no longer applies. In addition, needing to run fixperms is generally annoying and makes the tests more complex and fragile. `safcm fixperms` is kept as command not to break existing workflows. It has no effect now. --- diff --git a/Makefile b/Makefile index b21ed3d..0cf37fb 100644 --- a/Makefile +++ b/Makefile @@ -15,9 +15,6 @@ safcm: go build $(GOFLAGS) -ldflags $(LDFLAGS) ruderich.org/simon/safcm/cmd/safcm test: - @# For proper permissions after initial clone with a strict umask - cd cmd/safcm/testdata/project && ../../../../safcm fixperms 2> /dev/null - @# ./cmd/safcm/testdata/ssh/prepare.sh go vet ./... go test $(GOFLAGS) ./... diff --git a/cmd/safcm/config/files.go b/cmd/safcm/config/files.go index 66b9c60..c049ad1 100644 --- a/cmd/safcm/config/files.go +++ b/cmd/safcm/config/files.go @@ -19,16 +19,7 @@ import ( func LoadFiles(group string) (map[string]*safcm.File, error) { basePath := filepath.Join(group, "files") - const errMsg = ` - -The actual permissions and user/group of files and directories are not used -(except for +x on files). 0644/0755 and current remote user/group is used per -default. Apply different file permissions via permissions.yaml. To prevent -confusion files must be manually chmodded 0644/0755 and directories 0755 or -via "safcm fixperms". -` - - // No permission checks on windows which doesn't track them. + // Windows doesn't track permissions. windows := runtime.GOOS == "windows" files := make(map[string]*safcm.File) @@ -44,40 +35,35 @@ via "safcm fixperms". return err } typ := info.Mode().Type() - perm := FileModeToFullPerm(info.Mode()) + var perm int var data []byte - // See errMsg above. If a user stores a file with stricter permissions - // they could assume that these permissions are respected. This is not - // the case. + // Existing permissions of the files are not respected (except for +x + // on regular files). if typ == 0 /* regular file */ { if windows { perm = 0644 // 0755 must be set via permissions.yaml if windows is used - } - if perm != 0644 && perm != 0755 { - return fmt.Errorf("%q: invalid permissions %#o%s", - path, perm, errMsg) + } else { + perm = 0644 + // Respect +x for files + if info.Mode().Perm()&0100 != 0 { + perm = 0755 + } } data, err = os.ReadFile(path) if err != nil { return err } } else if typ == fs.ModeDir { - if windows { - perm = 0755 - } - if perm != 0755 { - return fmt.Errorf("%q: invalid permissions %#o%s", - path, perm, errMsg) - } + perm = 0755 } else if typ == fs.ModeSymlink { x, err := os.Readlink(path) if err != nil { return err } data = []byte(x) - perm |= 0777 // see cmd/safcm-remote/sync/files.go + perm = 0777 // see remote/sync/files.go } else { return fmt.Errorf("%q: file type not supported", path) } diff --git a/cmd/safcm/config/files_test.go b/cmd/safcm/config/files_test.go index 1942380..2c2fdad 100644 --- a/cmd/safcm/config/files_test.go +++ b/cmd/safcm/config/files_test.go @@ -7,7 +7,6 @@ import ( "fmt" "io/fs" "os" - "runtime" "testing" "ruderich.org/simon/safcm" @@ -15,52 +14,12 @@ import ( "ruderich.org/simon/safcm/testutil" ) -func chmod(name string, mode fs.FileMode) { - err := os.Chmod(name, mode) - if err != nil { - panic(err) - } -} - func TestLoadFiles(t *testing.T) { t.Chdir("../testdata/project") - // Regular users cannot create sticky files - skipInvalidSticky := os.Getuid() != 0 && - (runtime.GOOS == "freebsd" || runtime.GOOS == "openbsd") - - chmod("files-invalid-perm-dir/files", 0500) - defer chmod("files-invalid-perm-dir/files", 0700) - chmod("files-invalid-perm-dir/files/etc/", 0755) - chmod("files-invalid-perm-dir/files/etc/resolv.conf", 0644) - chmod("files-invalid-perm-dir-setgid/files", 0755) - chmod("files-invalid-perm-dir-setgid/files/etc/", 0755|fs.ModeSetgid) - chmod("files-invalid-perm-dir-setgid/files/etc/resolv.conf", 0644) - chmod("files-invalid-perm-file/files", 0755) - chmod("files-invalid-perm-file/files/etc/", 0755) - chmod("files-invalid-perm-file/files/etc/resolv.conf", 0600) - chmod("files-invalid-perm-file-executable/files", 0755) - chmod("files-invalid-perm-file-executable/files/etc", 0755) - chmod("files-invalid-perm-file-executable/files/etc/rc.local", 0750) - if !skipInvalidSticky { - chmod("files-invalid-perm-file-sticky/files", 0755) - chmod("files-invalid-perm-file-sticky/files/etc", 0755) - chmod("files-invalid-perm-file-sticky/files/etc/resolv.conf", - 0644|fs.ModeSticky) - } - ft.CreateFifo("files-invalid-type/files/invalid", 0644) defer os.Remove("files-invalid-type/files/invalid") //nolint:errcheck - const errMsg = ` - -The actual permissions and user/group of files and directories are not used -(except for +x on files). 0644/0755 and current remote user/group is used per -default. Apply different file permissions via permissions.yaml. To prevent -confusion files must be manually chmodded 0644/0755 and directories 0755 or -via "safcm fixperms". -` - tests := []struct { group string skip bool @@ -160,36 +119,6 @@ host3.example.net nil, fmt.Errorf("files-invalid-type: \"files-invalid-type/files/invalid\": file type not supported"), }, - { - "files-invalid-perm-dir", - false, - nil, - fmt.Errorf("files-invalid-perm-dir: \"files-invalid-perm-dir/files\": invalid permissions 0500" + errMsg), - }, - { - "files-invalid-perm-dir-setgid", - false, - nil, - fmt.Errorf("files-invalid-perm-dir-setgid: \"files-invalid-perm-dir-setgid/files/etc\": invalid permissions 02755" + errMsg), - }, - { - "files-invalid-perm-file", - false, - nil, - fmt.Errorf("files-invalid-perm-file: \"files-invalid-perm-file/files/etc/resolv.conf\": invalid permissions 0600" + errMsg), - }, - { - "files-invalid-perm-file-executable", - false, - nil, - fmt.Errorf("files-invalid-perm-file-executable: \"files-invalid-perm-file-executable/files/etc/rc.local\": invalid permissions 0750" + errMsg), - }, - { - "files-invalid-perm-file-sticky", - skipInvalidSticky, - nil, - fmt.Errorf("files-invalid-perm-file-sticky: \"files-invalid-perm-file-sticky/files/etc/resolv.conf\": invalid permissions 01644" + errMsg), - }, } for _, tc := range tests { diff --git a/cmd/safcm/fixperms.go b/cmd/safcm/fixperms.go deleted file mode 100644 index cebe6f6..0000000 --- a/cmd/safcm/fixperms.go +++ /dev/null @@ -1,105 +0,0 @@ -// "fixperms" sub-command: apply proper permissions in files/ directories - -// SPDX-License-Identifier: GPL-3.0-or-later -// Copyright (C) 2021-2025 Simon Ruderich - -package main - -import ( - "fmt" - "io/fs" - "log" - "os" - "path/filepath" - - "ruderich.org/simon/safcm/cmd/safcm/config" - "ruderich.org/simon/safcm/remote/sync" -) - -func MainFixperms() error { - _, _, _, err := LoadBaseFiles() - if err != nil { - return fmt.Errorf("not in a safcm directory: %v", err) - } - - xs, err := os.ReadDir(".") - if err != nil { - return err - } - for _, x := range xs { - if !x.IsDir() { - continue - } - path := filepath.Join(x.Name(), "files") - - err := filepath.WalkDir(path, fixpermsWalkDirFunc) - if err != nil { - if os.IsNotExist(err) { - continue - } - return fmt.Errorf("%s: %v", path, err) - } - } - - return nil -} - -func fixpermsWalkDirFunc(path string, d fs.DirEntry, err error) error { - if err != nil { - return err - } - - info, err := d.Info() - if err != nil { - return err - } - typ := info.Mode().Type() - perm := config.FileModeToFullPerm(info.Mode()) - - if typ == 0 /* regular file */ { - if perm != 0644 && perm != 0755 { - if perm&0111 != 0 /* executable */ { - perm = 0755 - } else { - perm = 0644 - } - log.Printf("chmodding %q to %#o", path, perm) - // This is safe because perm does not include setuid/setgid/sticky - // which use different values in FileMode. - err := chmodNoFollow(path, fs.FileMode(perm)) - if err != nil { - return err - } - } - } else if typ == fs.ModeDir { - if perm != 0755 { - perm = 0755 - log.Printf("chmodding %q to %#o", path, perm) - err := chmodNoFollow(path, fs.FileMode(perm)) - if err != nil { - return err - } - } - } - // Other file types are caught by regular "sync" - - return nil -} - -// chmodNoFollow works like os.Chmod but doesn't follow symlinks. -func chmodNoFollow(path string, mode fs.FileMode) error { - x, err := sync.OpenFileNoSymlinks(path) - if err != nil { - return err - } - defer x.Close() //nolint:errcheck - - err = x.Chmod(mode) - if err != nil { - return err - } - - return nil -} - -// vi: set noet ts=4 sw=4 sts=4: diff --git a/cmd/safcm/main.go b/cmd/safcm/main.go index f1d5f9e..a1a8276 100644 --- a/cmd/safcm/main.go +++ b/cmd/safcm/main.go @@ -15,7 +15,6 @@ import ( func usage() { log.SetFlags(0) log.Fatalf("usage: %[1]s sync [] \n"+ - " %[1]s fixperms\n"+ " %[1]s version\n"+ "", os.Args[0]) } @@ -30,10 +29,8 @@ func main() { case "sync": err = MainSync(os.Args) case "fixperms": - if len(os.Args) != 2 { - usage() - } - err = MainFixperms() + log.Printf("safcm fixperms is deprecated and has no effect") + err = nil case "version": if len(os.Args) != 2 { usage() diff --git a/cmd/safcm/sync_sync_test.go b/cmd/safcm/sync_sync_test.go index 8f377ac..e442fc8 100644 --- a/cmd/safcm/sync_sync_test.go +++ b/cmd/safcm/sync_sync_test.go @@ -5,9 +5,7 @@ package main import ( "fmt" - "io" "io/fs" - "log" "os" "path/filepath" "testing" @@ -339,14 +337,6 @@ func TestHostSyncReq(t *testing.T) { t.Run(tc.name, func(t *testing.T) { t.Chdir(filepath.Join("testdata", tc.project)) - // `safcm fixperms` in case user has strict umask - log.SetOutput(io.Discard) - err := MainFixperms() - if err != nil { - t.Fatal(err) - } - log.SetOutput(os.Stderr) - cfg, allHosts, allGroups, err := LoadBaseFiles() if err != nil { t.Fatal(err)