From 5d6cc7f14a4bacc36bf3a23cd735a75ad4a90f1d Mon Sep 17 00:00:00 2001 From: Simon Ruderich Date: Sun, 9 May 2021 10:10:56 +0200 Subject: [PATCH] Improve and add comments --- cmd/safcm-remote/main.go | 2 +- cmd/safcm-remote/sync/files.go | 8 ++++---- cmd/safcm-remote/sync/sync_test.go | 2 ++ cmd/safcm/config/groups.go | 4 +++- cmd/safcm/sync.go | 4 ++-- cmd/safcm/sync_info.go | 2 +- cmd/safcm/sync_sync.go | 2 +- cmd/safcm/term.go | 3 ++- rpc/conn.go | 2 +- rpc/dial.go | 10 +++++----- 10 files changed, 22 insertions(+), 17 deletions(-) diff --git a/cmd/safcm-remote/main.go b/cmd/safcm-remote/main.go index e9ce1cd..ab0bfae 100644 --- 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 // diff --git a/cmd/safcm-remote/sync/files.go b/cmd/safcm-remote/sync/files.go index f388606..119a353 100644 --- 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 - // 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 @@ -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) @@ -373,7 +373,7 @@ reopen: 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)) diff --git a/cmd/safcm-remote/sync/sync_test.go b/cmd/safcm-remote/sync/sync_test.go index 4c0f08d..6f8025a 100644 --- 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" ) +// testRunner implements run.Runner to test commands without actually running +// them. type testRunner struct { t *testing.T expCmds []*exec.Cmd diff --git a/cmd/safcm/config/groups.go b/cmd/safcm/config/groups.go index 5f3f679..a1dd83a 100644 --- a/cmd/safcm/config/groups.go +++ b/cmd/safcm/config/groups.go @@ -34,7 +34,7 @@ const ( 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) { @@ -86,6 +86,8 @@ func LoadGroups(cfg *Config, hosts *Hosts) (map[string][]string, error) { 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", diff --git a/cmd/safcm/sync.go b/cmd/safcm/sync.go index 98b118e..77e83c4 100644 --- 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) - // 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 diff --git a/cmd/safcm/sync_info.go b/cmd/safcm/sync_info.go index b0a85ee..0df33e3 100644 --- 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 } -// 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 { diff --git a/cmd/safcm/sync_sync.go b/cmd/safcm/sync_sync.go index 6977edd..88c93be 100644 --- 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 } - // 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 diff --git a/cmd/safcm/term.go b/cmd/safcm/term.go index d7568e4..47f7d62 100644 --- 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 -// 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. diff --git a/rpc/conn.go b/rpc/conn.go index 626a507..9786941 100644 --- 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 -// 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) diff --git a/rpc/dial.go b/rpc/dial.go index 945a75f..0782ab0 100644 --- 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 - // /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 - // 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. @@ -264,7 +264,7 @@ f 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 } -- 2.45.2