Things calling exec directly not handling bad commands. (#157)

If we exec something that returns non-zero it can happen for 2 reasons:

1. Command simply ran and exited non-zero. That works as expected mostly except the client code didn't error out for any target exiting this way.

2. Command couldn't run (i.e. exec error). That's returned from exec.RunCommand as an error. But we may have nothing in stderr. In this case we should copy the error to stderr since errors here are for commands not being setup correctly. Exit code was already right but otherwise we ate the error string.

Adjust all the tests so they account for this too and add an integration test.

Ansible had same issues as exec due to this.

.github/workflows/pr.yaml

@@ -8,9 +8,9 @@ jobs:
     runs-on: ubuntu-latest
     steps:
     - uses: actions/checkout@v3
-    - uses: actions/setup-go@331ce1d993939866bb63c32c6cbbfd48fa76fc57
+    - uses: actions/setup-go@v3
       with:
-        go-version: '^1.18'
+        go-version-file: 'go.mod'
     - name: Install tools
       run: |
         curl -sSfL https://raw.githubusercontent.com/golangci/golangci-lint/master/install.sh | sh -s -- -b $(go env GOPATH)/bin v1.46.2
@@ -20,10 +20,10 @@ jobs:
     name: Unit tests
     runs-on: ubuntu-20.04
     steps:
-      - uses: actions/checkout@5a4ac9002d0be2fb38bd78e4b4dbde5606d7042f
-      - uses: actions/setup-go@331ce1d993939866bb63c32c6cbbfd48fa76fc57
+      - uses: actions/checkout@v3
+      - uses: actions/setup-go@v3
         with:
-          go-version: '^1.18'
+          go-version-file: 'go.mod'
       - name: Install tools
         run: |
           sudo apt-get update

services/ansible/client/client.go

@@ -129,6 +129,9 @@ func (a *playbookCmd) Execute(ctx context.Context, f *flag.FlagSet, args ...inte
 			continue
 		}
 		fmt.Fprintf(state.Out[r.Index], "Return code: %d\nStdout:%s\nStderr:%s\n", r.Resp.ReturnCode, r.Resp.Stdout, r.Resp.Stderr)
+		if r.Resp.ReturnCode != 0 {
+			retCode = subcommands.ExitFailure
+		}
 	}
 	return retCode
 }

services/ansible/server/ansible.go

@@ -108,6 +108,9 @@ func (s *server) Run(ctx context.Context, req *pb.RunRequest) (*pb.RunReply, err
 	if err != nil {
 		return nil, err
 	}
+	if run.Error != nil {
+		return nil, err
+	}
 
 	return &pb.RunReply{
 		Stdout:     run.Stdout.String(),

services/ansible/server/ansible_test.go

@@ -115,10 +115,10 @@ func TestRun(t *testing.T) {
 			wantErr: true,
 		},
 		{
-			name:              "A bad command that doesn't exec",
-			bin:               "/non-existant-command",
-			path:              path,
-			returnCodeNonZero: true,
+			name:    "A bad command that doesn't exec",
+			bin:     "/non-existant-command",
+			path:    path,
+			wantErr: true,
 		},
 		{
 			name:              "A command that exits non-zero",

services/exec/client/client.go

@@ -105,6 +105,9 @@ func (p *runCmd) Execute(ctx context.Context, f *flag.FlagSet, args ...interface
 			fmt.Fprintf(state.Err[r.Index], "%s", r.Resp.Stderr)
 		}
 		fmt.Fprintf(state.Out[r.Index], "%s", r.Resp.Stdout)
+		if r.Resp.RetCode != 0 {
+			returnCode = subcommands.ExitFailure
+		}
 	}
 	return returnCode
 }

services/exec/server/exec.go

@@ -36,15 +36,10 @@ func (s *server) Run(ctx context.Context, req *pb.ExecRequest) (res *pb.ExecResp
 		return nil, err
 	}
 
-	if err := run.Error; err != nil {
-		return &pb.ExecResponse{
-			Stdout:  run.Stdout.Bytes(),
-			Stderr:  run.Stderr.Bytes(),
-			RetCode: int32(run.ExitCode),
-		}, nil
+	if run.Error != nil {
+		return nil, run.Error
 	}
-
-	return &pb.ExecResponse{Stderr: run.Stderr.Bytes(), Stdout: run.Stdout.Bytes(), RetCode: 0}, nil
+	return &pb.ExecResponse{Stderr: run.Stderr.Bytes(), Stdout: run.Stdout.Bytes(), RetCode: int32(run.ExitCode)}, nil
 }
 
 // Register is called to expose this handler to the gRPC server

services/exec/server/exec_test.go

@@ -84,9 +84,9 @@ func TestExec(t *testing.T) {
 			returnCodeNonZero: true,
 		},
 		{
-			name:              "Non-existant program",
-			bin:               "/something/non-existant",
-			returnCodeNonZero: true,
+			name:    "Non-existant program",
+			bin:     "/something/non-existant",
+			wantErr: true,
 		},
 		{
 			name:    "non-absolute path",

services/fdb/server/fdbcli.go

@@ -236,6 +236,9 @@ func (s *server) FDBCLI(req *pb.FDBCLIRequest, stream pb.CLI_FDBCLIServer) error
 	if run.Error != nil {
 		return status.Errorf(codes.Internal, "can't exec fdbcli: %v - %s", run.Error, run.Stderr)
 	}
+	if run.ExitCode != 0 {
+		return status.Errorf(codes.Internal, "fdbcli returned non-zero: %d - %v - %s", run.ExitCode, run.Error, run.Stderr)
+	}
 
 	// Make sure we always remove logs even if errors happen below.
 	defer func() {

services/packages/server/packages.go

@@ -181,7 +181,7 @@ func (s *server) Install(ctx context.Context, req *pb.InstallRequest) (*pb.Insta
 		return nil, err
 	}
 
-	if err := run.Error; err != nil {
+	if err := run.Error; run.ExitCode != 0 || err != nil {
 		return nil, status.Errorf(codes.Internal, "error from running - %v\nstdout:\n%s\nstderr:\n%s", err, util.TrimString(run.Stdout.String()), util.TrimString(run.Stderr.String()))
 	}
 
@@ -230,7 +230,7 @@ func (s *server) Update(ctx context.Context, req *pb.UpdateRequest) (*pb.UpdateR
 	if err != nil {
 		return nil, err
 	}
-	if err := run.Error; err != nil {
+	if err := run.Error; run.ExitCode != 0 || err != nil {
 		return nil, status.Errorf(codes.Internal, "package %s at version %s doesn't appear to be installed.\nStderr:\n%s", req.Name, req.OldVersion, util.TrimString(run.Stderr.String()))
 	}
 
@@ -239,7 +239,7 @@ func (s *server) Update(ctx context.Context, req *pb.UpdateRequest) (*pb.UpdateR
 	if err != nil {
 		return nil, err
 	}
-	if err := run.Error; err != nil {
+	if err := run.Error; run.ExitCode != 0 || err != nil {
 		return nil, status.Errorf(codes.Internal, "error from running %q: %v", updateCommand, err)
 	}
 
@@ -335,7 +335,7 @@ func (s *server) ListInstalled(ctx context.Context, req *pb.ListInstalledRequest
 	if err != nil {
 		return nil, err
 	}
-	if err := run.Error; err != nil {
+	if err := run.Error; run.ExitCode != 0 || err != nil {
 		return nil, status.Errorf(codes.Internal, "error from running %q: %v", command, err)
 	}
 
@@ -439,7 +439,7 @@ func (s *server) RepoList(ctx context.Context, req *pb.RepoListRequest) (*pb.Rep
 	if err != nil {
 		return nil, err
 	}
-	if err := run.Error; err != nil {
+	if err := run.Error; run.ExitCode != 0 || err != nil {
 		return nil, status.Errorf(codes.Internal, "error from running %q: %v\nstdout:\n%s\nstderr:\n%s", command, err, util.TrimString(run.Stdout.String()), util.TrimString(run.Stderr.String()))
 	}
 

services/process/server/process.go

@@ -119,8 +119,8 @@ func (s *server) List(ctx context.Context, req *pb.ListRequest) (*pb.ListReply,
 		return nil, err
 	}
 
-	if err := run.Error; err != nil {
-		return nil, status.Errorf(codes.Internal, "command exited with error: %v\n%s", err, util.TrimString(run.Stderr.String()))
+	if err := run.Error; run.ExitCode != 0 || err != nil {
+		return nil, status.Errorf(codes.Internal, "command exited with error/non-zero exit: %v (%d)\n%s", err, run.ExitCode, util.TrimString(run.Stderr.String()))
 	}
 
 	entries, err := parser(run.Stdout)
@@ -177,8 +177,8 @@ func (s *server) GetStacks(ctx context.Context, req *pb.GetStacksRequest) (*pb.G
 		return nil, err
 	}
 
-	if err := run.Error; err != nil {
-		return nil, status.Errorf(codes.Internal, "command exited with error: %v\n%s", err, util.TrimString(run.Stderr.String()))
+	if err := run.Error; run.ExitCode != 0 || err != nil {
+		return nil, status.Errorf(codes.Internal, "command exited with error/non-zero exit: %v (%d)\n%s", err, run.ExitCode, util.TrimString(run.Stderr.String()))
 	}
 
 	scanner := bufio.NewScanner(run.Stdout)
@@ -248,8 +248,8 @@ func (s *server) GetJavaStacks(ctx context.Context, req *pb.GetJavaStacksRequest
 		return nil, err
 	}
 
-	if err := run.Error; err != nil {
-		return nil, status.Errorf(codes.Internal, "command exited with error: %v\n%s", err, util.TrimString(run.Stderr.String()))
+	if err := run.Error; run.ExitCode != 0 || err != nil {
+		return nil, status.Errorf(codes.Internal, "command exited with error/non-zero exit: %v (%d)\n%s", err, run.ExitCode, util.TrimString(run.Stderr.String()))
 	}
 
 	scanner := bufio.NewScanner(run.Stdout)
@@ -429,8 +429,8 @@ func (s *server) GetMemoryDump(req *pb.GetMemoryDumpRequest, stream pb.Process_G
 		return err
 	}
 
-	if err := run.Error; err != nil {
-		return status.Errorf(codes.Internal, "command exited with error: %v\n%s", err, util.TrimString(run.Stderr.String()))
+	if err := run.Error; run.ExitCode != 0 || err != nil {
+		return status.Errorf(codes.Internal, "command exited with error/non-zero exit: %v (%d)\n%s", err, run.ExitCode, util.TrimString(run.Stderr.String()))
 	}
 
 	f, err := os.Open(file)

services/util/util.go

@@ -250,7 +250,13 @@ func RunCommand(ctx context.Context, bin string, args []string, opts ...Option)
 	logger.Info("executing local command", "cmd", cmd.String())
 	run.Error = cmd.Run()
 	run.ExitCode = cmd.ProcessState.ExitCode()
-
+	// If this was an error it could be two different things. Just exiting non-zero results in an exec.ExitError
+	// and we can suppress that as exit code is enough. Otherwise we leave run.Error for callers to use.
+	if run.Error != nil {
+		if _, ok := run.Error.(*exec.ExitError); ok {
+			run.Error = nil
+		}
+	}
 	if options.failOnStderr && len(run.Stderr.String()) != 0 {
 		return nil, status.Errorf(codes.Internal, "unexpected error output:\n%s", TrimString(run.Stderr.String()))
 	}

services/util/util_test.go

@@ -30,6 +30,7 @@ func TestRunCommand(t *testing.T) {
 		bin               string
 		args              []string
 		wantErr           bool
+		wantRunErr        bool
 		returnCodeNonZero bool
 		uid               uint32
 		gid               uint32
@@ -52,6 +53,7 @@ func TestRunCommand(t *testing.T) {
 			name:              "non-existant binary",
 			bin:               "/non-existant-path",
 			returnCodeNonZero: true,
+			wantRunErr:        true,
 		},
 		{
 			name:   "Command with stdout and stderr",
@@ -90,12 +92,14 @@ func TestRunCommand(t *testing.T) {
 			bin:               testutil.ResolvePath(t, "env"),
 			uid:               99,
 			returnCodeNonZero: true,
+			wantRunErr:        true,
 		},
 		{
 			name:              "set gid (will fail)",
 			bin:               testutil.ResolvePath(t, "env"),
 			gid:               99,
 			returnCodeNonZero: true,
+			wantRunErr:        true,
 		},
 	} {
 		tc := tc
@@ -125,6 +129,7 @@ func TestRunCommand(t *testing.T) {
 				}
 				return
 			}
+			testutil.WantErr(tc.name, run.Error, tc.wantRunErr, t)
 			if got, want := run.Stdout.String(), tc.stdout; got != want {
 				t.Fatalf("%s: Stdout differs. Want %q Got %q", tc.name, want, got)
 			}

testing/integrate.sh

@@ -544,6 +544,7 @@ if [ $? != 1 ]; then
   check_status 1 /dev/null missing justification failed
 fi
 
+echo "Testing client policy"
 cat >${LOGS}/client-policy.rego <<EOF
 package sansshell.authz
 
@@ -554,7 +555,7 @@ allow {
   input.peer.cert.subject.organization[i] = "foo"
 }
 EOF
-${SANSSH_PROXY} ${SINGLE_TARGET} --v=1 --client-policy-file=${LOGS}/client-policy.rego healthcheck validate
+${SANSSH_PROXY} ${SINGLE_TARGET} --v=1 --client-policy-file=${LOGS}/client-policy.rego --timeout=10s healthcheck validate
 if [ $? != 1 ]; then
   check_status 1 /dev/null policy check did not fail
 fi
@@ -612,6 +613,12 @@ run_a_test false 50 ansible playbook --playbook=$PWD/services/ansible/server/tes
 
 run_a_test false 1 exec run /usr/bin/echo Hello World
 
+# Test exec with a bad path doesn't work and emits errors
+echo "Checking exec fails for bad path"
+if ${SANSSH_PROXY} ${MULTI_TARGETS} exec run /bin/nosuchfile; then
+  check_status 1 /dev/null exec not erroring as expected
+fi
+
 # Test that outputs/errors go where expected and --output-dir works.
 mkdir -p ${LOGS}/exec-testdir
 ${SANSSH_PROXY} ${MULTI_TARGETS} --output-dir=${LOGS}/exec-testdir exec run /bin/sh -c "echo foo >&2"