diff --git a/internal/target/remote_target.go b/internal/target/remote_target.go index 2355a042..5a9d46a0 100644 --- a/internal/target/remote_target.go +++ b/internal/target/remote_target.go @@ -397,12 +397,16 @@ func (t *RemoteTarget) prepareSSHCommand(command []string, useControlMaster bool var cmd []string cmd = append(cmd, "ssh") cmd = append(cmd, t.prepareSSHFlags(false, useControlMaster, prompt)...) + // end ssh's option parsing before the destination so that a destination beginning with a dash + // cannot be interpreted as an ssh option, e.g., -oProxyCommand= + cmd = append(cmd, "--") if t.user != "" { cmd = append(cmd, t.user+"@"+t.host) } else { cmd = append(cmd, t.host) } - cmd = append(cmd, "--") + // note: no "--" separator before the remote command; option parsing already ended above and ssh + // would pass a second separator to the remote shell as part of the command cmd = append(cmd, command...) return cmd } @@ -420,6 +424,8 @@ func (t *RemoteTarget) prepareSCPCommand(src string, dstDir string, push bool) [ if fileInfo.IsDir() { cmd = append(cmd, "-r") } + // end scp's option parsing before the paths, see prepareSSHCommand + cmd = append(cmd, "--") cmd = append(cmd, src) dst := t.host + ":" + dstDir if t.user != "" { @@ -431,6 +437,8 @@ func (t *RemoteTarget) prepareSCPCommand(src string, dstDir string, push bool) [ if t.user != "" { s = t.user + "@" + s } + // end scp's option parsing before the paths, see prepareSSHCommand + cmd = append(cmd, "--") cmd = append(cmd, s) cmd = append(cmd, dstDir) } diff --git a/internal/workflow/targets.go b/internal/workflow/targets.go index 64a60375..a1be1915 100644 --- a/internal/workflow/targets.go +++ b/internal/workflow/targets.go @@ -92,38 +92,102 @@ func ValidateTargetFlags(cmd *cobra.Command) error { } } // confirm that port is a positive integer - if flagTargetPort != "" { - var port int - var err error - if port, err = strconv.Atoi(flagTargetPort); err != nil || port <= 0 { - return fmt.Errorf("port %s is not a positive integer", flagTargetPort) - } + if err := validateTargetPort(flagTargetPort); err != nil { + return err } // confirm that the key file exists - if flagTargetKeyFile != "" { - if _, err := os.Stat(flagTargetKeyFile); os.IsNotExist(err) { - return fmt.Errorf("key file %s does not exist", flagTargetKeyFile) - } + if err := validateTargetKeyFile(flagTargetKeyFile); err != nil { + return err } // confirm that user is a valid user name - if flagTargetUser != "" { - userNameRegex := `^[a-z_][a-z0-9_.-]{0,63}$` - re := regexp.MustCompile(userNameRegex) - if !re.MatchString(flagTargetUser) { - return fmt.Errorf("user name %s does not match the user name regex '%s'", flagTargetUser, userNameRegex) - } + if err := validateTargetUser(flagTargetUser); err != nil { + return err } // confirm that host is a valid host name or IP address - if flagTargetHost != "" { - hostNameRegex := `^([a-zA-Z0-9.-]+)$` - re := regexp.MustCompile(hostNameRegex) - if !re.MatchString(flagTargetHost) { - return fmt.Errorf("host name %s does not match the host name regex '%s'", flagTargetHost, hostNameRegex) - } + if err := validateTargetHost(flagTargetHost); err != nil { + return err } return nil } +// Connection details arrive from the command line and from the targets file. They are placed +// directly into the argument vector of the local ssh/scp process, so a value that begins with a +// dash would be interpreted by ssh as an option, e.g., -oProxyCommand=, which +// ssh runs on the local host. The host name additionally becomes a path component of the target's +// output directory when no target name is given. The regular expressions below therefore exclude +// both leading dashes and path separators. They are deliberately strict; note that IPv6 literals +// are not accepted (a limitation that predates this validation). +const ( + hostNameRegex = `^[a-zA-Z0-9]([a-zA-Z0-9.-]*[a-zA-Z0-9])?$` + userNameRegex = `^[a-z_][a-z0-9_.-]{0,63}$` +) + +// validateTargetHost confirms that host is a valid host name or IP address. An empty host is +// valid; callers that require a host check for it separately. +func validateTargetHost(host string) error { + if host == "" { + return nil + } + if !regexp.MustCompile(hostNameRegex).MatchString(host) { + return fmt.Errorf("host name %s does not match the host name regex '%s'", host, hostNameRegex) + } + return nil +} + +// validateTargetUser confirms that user is a valid user name. An empty user is valid; ssh falls +// back to the local user name. +func validateTargetUser(user string) error { + if user == "" { + return nil + } + if !regexp.MustCompile(userNameRegex).MatchString(user) { + return fmt.Errorf("user name %s does not match the user name regex '%s'", user, userNameRegex) + } + return nil +} + +// validateTargetPort confirms that port is a positive integer. An empty port is valid; ssh uses +// its default port. +func validateTargetPort(port string) error { + if port == "" { + return nil + } + portNum, err := strconv.Atoi(port) + if err != nil || portNum <= 0 || portNum > 65535 { + return fmt.Errorf("port %s is not a valid port number", port) + } + return nil +} + +// validateTargetKeyFile confirms that the key file exists. An empty key file is valid; ssh uses +// its default keys or a password. +func validateTargetKeyFile(keyFile string) error { + if keyFile == "" { + return nil + } + if _, err := os.Stat(keyFile); os.IsNotExist(err) { + return fmt.Errorf("key file %s does not exist", keyFile) + } + return nil +} + +// validateTargetFromFile validates the connection details of one target read from a targets file. +func validateTargetFromFile(t targetFromYAML) error { + if t.Host == "" { + return fmt.Errorf("target in targets file has no host") + } + if err := validateTargetHost(t.Host); err != nil { + return err + } + if err := validateTargetUser(t.User); err != nil { + return err + } + if err := validateTargetPort(t.Port); err != nil { + return err + } + return validateTargetKeyFile(t.Key) +} + // GetTargets retrieves the list of targets based on the provided command and parameters. It creates // a temporary directory for each target and returns a slice of target.Target objects. func GetTargets(cmd *cobra.Command, needsElevatedPrivileges bool, failIfCantElevate bool, localTempDir string) (targets []target.Target, targetErrs []error, err error) { @@ -313,6 +377,8 @@ type targetsFile struct { } // sanitizeTargetName sanitizes the target name by removing any invalid characters. +// The result is used as a file and directory name, so names that would refer to another directory +// are replaced entirely. func sanitizeTargetName(targetName string) string { // remove any invalid characters from the target name // this is needed for the report file names @@ -333,6 +399,10 @@ func sanitizeTargetName(targetName string) string { } return '_' }, targetName) + // "." and ".." survive the mapping above but refer to a directory rather than naming one + if sanitizedTargetName == "." || sanitizedTargetName == ".." { + return strings.Repeat("_", len(sanitizedTargetName)) + } return sanitizedTargetName } @@ -356,7 +426,7 @@ func getTargetsFromFile(targetsFilePath string, localTempDir string) (targets [] return } targetNameUsed := make(map[string]bool) - for _, t := range targetsFile.Targets { + for targetIdx, t := range targetsFile.Targets { // create a target object // target name is not required, but if it is provided there must not be duplicate names var targetName string @@ -367,6 +437,22 @@ func getTargetsFromFile(targetsFilePath string, localTempDir string) (targets [] return } targetNameUsed[targetName] = true + } else { + // without a name, the target is identified by its host name, which is also used to + // name the target's output directory and files, so it must be sanitized + targetName = sanitizeTargetName(t.Host) + } + if targetName == "" { // neither a name nor a host was provided + targetName = fmt.Sprintf("target %d", targetIdx+1) + } + // the connection details are used to build the local ssh/scp command line, so they must be + // validated before they are used, see validateTargetHost for details + if targetErr := validateTargetFromFile(t); targetErr != nil { + // retain a placeholder target, without connection details, so that the caller's + // targets and errors remain parallel and the invalid details are never used + targets = append(targets, target.NewRemoteTarget(targetName, "", "", "", "")) + targetErrs = append(targetErrs, targetErr) + continue } newTarget := target.NewRemoteTarget(targetName, t.Host, t.Port, t.User, t.Key) newTarget.SetSshPass(t.Pwd) diff --git a/internal/workflow/targets_test.go b/internal/workflow/targets_test.go index 4dd53651..d6173687 100644 --- a/internal/workflow/targets_test.go +++ b/internal/workflow/targets_test.go @@ -4,6 +4,8 @@ package workflow import ( + "os" + "path/filepath" "testing" "github.com/stretchr/testify/assert" @@ -178,6 +180,21 @@ func TestSanitizeTargetName(t *testing.T) { input: "valid@name#123!.txt", expected: "valid_name_123_.txt", }, + { + name: "Name referring to the current directory", + input: ".", + expected: "_", + }, + { + name: "Name referring to the parent directory", + input: "..", + expected: "__", + }, + { + name: "Name attempting path traversal", + input: "../../etc", + expected: ".._.._etc", + }, } for _, tt := range tests { @@ -187,3 +204,119 @@ func TestSanitizeTargetName(t *testing.T) { }) } } + +// TestValidateTargetHost confirms the host name rules. A host name becomes an argument of the local +// ssh process and, when a target is not named, a component of the target's output path, so a leading +// dash and a path separator must both be rejected. See validateTargetHost for the reasoning. +func TestValidateTargetHost(t *testing.T) { + tests := []struct { + name string + input string + wantErr bool + }{ + {name: "Empty host", input: "", wantErr: false}, + {name: "Host name", input: "my-host.example.com", wantErr: false}, + {name: "IPv4 address", input: "192.168.1.1", wantErr: false}, + {name: "Single character host", input: "h", wantErr: false}, + {name: "Host beginning with a dash", input: "-host", wantErr: true}, + {name: "Host ending with a dash", input: "host-", wantErr: true}, + {name: "Host with a path separator", input: "../../etc", wantErr: true}, + {name: "Host with a space", input: "host name", wantErr: true}, + {name: "Host with a semicolon", input: "host;name", wantErr: true}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := validateTargetHost(tt.input) + if tt.wantErr { + assert.Error(t, err) + } else { + assert.NoError(t, err) + } + }) + } +} + +func TestValidateTargetFromFile(t *testing.T) { + tests := []struct { + name string + input targetFromYAML + wantErr bool + }{ + { + name: "Valid target", + input: targetFromYAML{Name: "target1", Host: "host1", Port: "22", User: "user1"}, + wantErr: false, + }, + { + name: "Valid target with only a host", + input: targetFromYAML{Host: "host1"}, + wantErr: false, + }, + { + name: "Missing host", + input: targetFromYAML{Name: "target1", User: "user1"}, + wantErr: true, + }, + { + name: "Port is not a number", + input: targetFromYAML{Host: "host1", Port: "notanumber"}, + wantErr: true, + }, + { + name: "Port is out of range", + input: targetFromYAML{Host: "host1", Port: "65536"}, + wantErr: true, + }, + { + name: "Key file does not exist", + input: targetFromYAML{Host: "host1", Key: "/no/such/key/file"}, + wantErr: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := validateTargetFromFile(tt.input) + if tt.wantErr { + assert.Error(t, err) + } else { + assert.NoError(t, err) + } + }) + } +} + +// TestGetTargetsFromFileRejectsInvalidTargets confirms that an entry with invalid connection details +// yields a target error rather than a usable target, and that the returned targets and errors stay +// parallel, which the callers of GetTargets require. +func TestGetTargetsFromFileRejectsInvalidTargets(t *testing.T) { + tempDir := t.TempDir() + // every entry is invalid, so that no connection is attempted and the test cannot hang on name + // resolution; the second entry is unnamed, so its name comes from its host + yaml := `targets: + - name: target1 + host: "host name with a space" + port: + user: + key: + pwd: + - host: host2 + port: 65536 +` + targetsFilePath := filepath.Join(tempDir, "targets.yaml") + if err := os.WriteFile(targetsFilePath, []byte(yaml), 0600); err != nil { + t.Fatalf("failed to write targets file: %v", err) + } + + targets, targetErrs, err := getTargetsFromFile(targetsFilePath, tempDir) + assert.NoError(t, err) + // the targets and their errors remain parallel, as the caller requires + assert.Len(t, targets, 2) + assert.Len(t, targetErrs, 2) + // both targets are rejected, and both keep a usable name for display + assert.Error(t, targetErrs[0]) + assert.Equal(t, "target1", targets[0].GetName()) + assert.Error(t, targetErrs[1]) + assert.Equal(t, "host2", targets[1].GetName()) +}