Improve and add comments

diff --git a/cmd/safcm-remote/main.go b/cmd/safcm-remote/main.go
index e9ce1cd0ccac627e6573bc5b07c040166cb4be65..ab0bfae917286283e59bcb16af1c2ffabe15432a 100644 (file)
--- a/cmd/safcm-remote/main.go
+++ b/cmd/safcm-remote/main.go
@@ -1,4 +1,4 @@
-// Helper copied to the remote hosts to run commands and deploy configuration
+// Helper copied to the remote host to run commands and deploy configuration

 
 // Copyright (C) 2021  Simon Ruderich
 //
 
 // Copyright (C) 2021  Simon Ruderich
 //

diff --git a/cmd/safcm-remote/sync/files.go b/cmd/safcm-remote/sync/files.go
index f3886065803781d8b0c0613766fbe981e8d4f61e..119a3534239f68e2e5d10aee08ec2559590ada87 100644 (file)
--- a/cmd/safcm-remote/sync/files.go
+++ b/cmd/safcm-remote/sync/files.go
@@ -152,8 +152,8 @@ reopen:
                if change.Old.Mode.Type() == fs.ModeSymlink {
                        // Some BSD systems permit changing permissions of
                        // symlinks but ignore them on traversal. To keep it
                if change.Old.Mode.Type() == fs.ModeSymlink {
                        // Some BSD systems permit changing permissions of
                        // symlinks but ignore them on traversal. To keep it

-                       // simple we don't support that and always use 0777 for
-                       // symlink permissions (the value on GNU/Linux).
+                       // simple we don't support that and always use 0777
+                       // for symlink permissions (the value on GNU/Linux).

                        //
                        // TODO: Add proper support for symlinks on BSD
                        change.Old.Mode |= 0777
                        //
                        // TODO: Add proper support for symlinks on BSD
                        change.Old.Mode |= 0777


@@ -267,7 +267,7 @@ reopen:
                }
        }
 
                }
        }
 

-       // Directory: create new directory (also type change to directory)
+       // Directory: create new directory, also type change to directory

        if file.Mode.IsDir() && (change.Created || changeType) {
                debugf("creating directory")
                err := os.Mkdir(file.Path, 0700)
        if file.Mode.IsDir() && (change.Created || changeType) {
                debugf("creating directory")
                err := os.Mkdir(file.Path, 0700)


@@ -373,7 +373,7 @@ reopen:
                        os.Remove(tmpPath)
                        return err
                }
                        os.Remove(tmpPath)
                        return err
                }

-               // Permissions are irrelevant for symlinks
+               // Permissions are irrelevant for symlinks (on most systems)

 
        default:
                panic(fmt.Sprintf("invalid file type %s", file.Mode))
 
        default:
                panic(fmt.Sprintf("invalid file type %s", file.Mode))

diff --git a/cmd/safcm-remote/sync/sync_test.go b/cmd/safcm-remote/sync/sync_test.go
index 4c0f08d5abdf6515f4da8df503b2088fd71d355d..6f8025a2c7cb8d106be7efe23c4efb1b1644e321 100644 (file)
--- a/cmd/safcm-remote/sync/sync_test.go
+++ b/cmd/safcm-remote/sync/sync_test.go
@@ -31,6 +31,8 @@ import (
        "ruderich.org/simon/safcm/cmd/safcm-remote/run"
 )
 
        "ruderich.org/simon/safcm/cmd/safcm-remote/run"
 )
 

+// testRunner implements run.Runner to test commands without actually running
+// them.

 type testRunner struct {
        t         *testing.T
        expCmds   []*exec.Cmd
 type testRunner struct {
        t         *testing.T
        expCmds   []*exec.Cmd

diff --git a/cmd/safcm/config/groups.go b/cmd/safcm/config/groups.go
index 5f3f679f5a9bde9ec1368d3f705f08d313647094..a1dd83a0164267a0f24a351a74966fd108300323 100644 (file)
--- a/cmd/safcm/config/groups.go
+++ b/cmd/safcm/config/groups.go
@@ -34,7 +34,7 @@ const (
        GroupRemoveSuffix     = GroupSpecialSeparator + "remove"
 )
 
        GroupRemoveSuffix     = GroupSpecialSeparator + "remove"
 )
 

-// Keep in sync with sync_info.go:infoGroupDetectedRegexp
+// Keep in sync with cmd/safcm/sync_info.go:infoGroupDetectedRegexp

 var groupNameRegexp = regexp.MustCompile(`^[a-z0-9_-]+$`)
 
 func LoadGroups(cfg *Config, hosts *Hosts) (map[string][]string, error) {
 var groupNameRegexp = regexp.MustCompile(`^[a-z0-9_-]+$`)
 
 func LoadGroups(cfg *Config, hosts *Hosts) (map[string][]string, error) {


@@ -86,6 +86,8 @@ func LoadGroups(cfg *Config, hosts *Hosts) (map[string][]string, error) {
                        if x == GroupAll {
                                continue
                        }
                        if x == GroupAll {
                                continue
                        }

+                       // Don't validate against groupNameRegexp because
+                       // hosts have less strict restrictions.

                        if strings.Contains(x, GroupSpecialSeparator) {
                                return nil, fmt.Errorf(
                                        "%s member %q must not contain %q",
                        if strings.Contains(x, GroupSpecialSeparator) {
                                return nil, fmt.Errorf(
                                        "%s member %q must not contain %q",

diff --git a/cmd/safcm/sync.go b/cmd/safcm/sync.go
index 98b118ed821d2224bf4f7e77e0ed002b368d9df3..77e83c4187592ca1bd5cc31e44da3f4acfb4830d 100644 (file)
--- a/cmd/safcm/sync.go
+++ b/cmd/safcm/sync.go
@@ -247,8 +247,8 @@ are only available after the hosts were contacted.
                nameMap[x] = true
        }
        nameMatched := make(map[string]bool)
                nameMap[x] = true
        }
        nameMatched := make(map[string]bool)

-       // To detect typos we must check all given names but only want to add
-       // each match once
+       // To detect typos we must check all given names but one host can be
+       // matched by multiple names (e.g. two groups with overlapping hosts)

        hostMatched := make(map[string]bool)
 
        var res []*config.Host
        hostMatched := make(map[string]bool)
 
        var res []*config.Host

diff --git a/cmd/safcm/sync_info.go b/cmd/safcm/sync_info.go
index b0a85ee507f4603b2966eb0b7b6e8b6f74dba12a..0df33e3eeaa302ef52c8a7d149e5f107d82812b1 100644 (file)
--- a/cmd/safcm/sync_info.go
+++ b/cmd/safcm/sync_info.go
@@ -45,7 +45,7 @@ func (s *Sync) hostInfo(conn *rpc.Conn) ([]string, error) {
        return hostInfoRespToGroups(resp), nil
 }
 
        return hostInfoRespToGroups(resp), nil
 }
 

-// Keep in sync with config/groups.go:groupNameRegexp
+// Keep in sync with cmd/safcm/config/groups.go:groupNameRegexp

 var infoGroupDetectedRegexp = regexp.MustCompile(`[^a-z0-9_-]+`)
 
 func hostInfoRespToGroups(resp safcm.MsgInfoResp) []string {
 var infoGroupDetectedRegexp = regexp.MustCompile(`[^a-z0-9_-]+`)
 
 func hostInfoRespToGroups(resp safcm.MsgInfoResp) []string {

diff --git a/cmd/safcm/sync_sync.go b/cmd/safcm/sync_sync.go
index 6977edd584406247c9df8ebbd24d75ecfba5a31c..88c93beb85c5c39fbdb1a73c3c80f0dab1842d0f 100644 (file)
--- a/cmd/safcm/sync_sync.go
+++ b/cmd/safcm/sync_sync.go
@@ -186,7 +186,7 @@ func (s *Sync) resolveHostGroups(detectedGroups []string) (
                return nil, nil, err
        }
 
                return nil, nil, err
        }
 

-       // Early entries have higher priorities
+       // Early entries in "group_priority" have higher priorities

        groupPriority := make(map[string]int)
        for i, x := range s.config.GroupPriority {
                groupPriority[x] = len(s.config.GroupPriority) - i
        groupPriority := make(map[string]int)
        for i, x := range s.config.GroupPriority {
                groupPriority[x] = len(s.config.GroupPriority) - i

diff --git a/cmd/safcm/term.go b/cmd/safcm/term.go
index d7568e47c4d6dd7b80576011d1aa19e446bb14ba..47f7d62cc1b14fe0ddbe3239d597e1d9f4b68ecc 100644 (file)
--- a/cmd/safcm/term.go
+++ b/cmd/safcm/term.go
@@ -57,7 +57,8 @@ func ColorString(isTTY bool, color Color, x string) string {
 var escapeRegexp = regexp.MustCompile(`[\x00-\x08\x0B-\x1F\x7F]`)
 
 // EscapeControlCharacters escapes all ASCII control characters (except
 var escapeRegexp = regexp.MustCompile(`[\x00-\x08\x0B-\x1F\x7F]`)
 
 // EscapeControlCharacters escapes all ASCII control characters (except

-// newline and tab) by replacing them with their hex value.
+// newline and tab) by replacing them with their hex value. If the output is
+// to a TTY then the escaped characters are colored.

 //
 // This function must be used when displaying any input from remote hosts to
 // prevent terminal escape code injections.
 //
 // This function must be used when displaying any input from remote hosts to
 // prevent terminal escape code injections.

diff --git a/rpc/conn.go b/rpc/conn.go
index 626a507f7e034ce57a0e3766f1fc93412499727c..9786941c7e808966a9a76841bfd6e03981f81ba6 100644 (file)
--- a/rpc/conn.go
+++ b/rpc/conn.go
@@ -55,7 +55,7 @@ type ConnEvent struct {
 }
 
 // NewConn creates a new connection. Events in the returned struct must be
 }
 
 // NewConn creates a new connection. Events in the returned struct must be

-// regularly read or the connection will stall. This must be done before
+// regularly read or the connection will hang. This must be done before

 // DialSSH is called to open a connection.
 func NewConn(debug bool) *Conn {
        ch := make(chan ConnEvent)
 // DialSSH is called to open a connection.
 func NewConn(debug bool) *Conn {
        ch := make(chan ConnEvent)

diff --git a/rpc/dial.go b/rpc/dial.go
index 945a75ff86c365fd1e251e205a0b68efbd2b3aef..0782ab08e617933e4e2c369651253d11324e355f 100644 (file)
--- a/rpc/dial.go
+++ b/rpc/dial.go
@@ -141,13 +141,13 @@ compat_sha512sum() {
        //
        // The target directory must no permit other users to delete our files
        // or symlink attacks and arbitrary code execution is possible. For
        //
        // The target directory must no permit other users to delete our files
        // or symlink attacks and arbitrary code execution is possible. For

-       // /tmp this is guaranteed by the sticky bit. Make sure it has the
-       // proper permissions.
+       // /tmp this is guaranteed by the sticky bit. The code verifies the
+       // directory has the proper permissions.

        //
        // We cannot use `test -f && test -O` because this is open to TOCTOU
        // attacks. `stat` gives use the full file state. If the file is owned
        //
        // We cannot use `test -f && test -O` because this is open to TOCTOU
        // attacks. `stat` gives use the full file state. If the file is owned

-       // by us and not a symlink then it's safe to use (assuming sticky or
-       // directory not writable by others).
+       // by us and not a symlink then it's safe to use (assuming sticky
+       // directory or directory not writable by others).

        //
        // `test -e` is only used to prevent error messages if the file
        // doesn't exist. It does not guard against any races.
        //
        // `test -e` is only used to prevent error messages if the file
        // doesn't exist. It does not guard against any races.


@@ -264,7 +264,7 @@ f
                                c.sshRemote,
                                fmt.Sprintf("cat > %q", path))...)
                cmd.Stdin = bytes.NewReader(helper)
                                c.sshRemote,
                                fmt.Sprintf("cat > %q", path))...)
                cmd.Stdin = bytes.NewReader(helper)

-               err = c.handleStderrAsEvents(cmd)
+               err = c.handleStderrAsEvents(cmd) // cmd.Stderr

                if err != nil {
                        return err
                }
                if err != nil {
                        return err
                }