`713648d`

runner/engine: harden Docker step containers

Authored by mfwolffe <wolffemf@dukes.jmu.edu> 3 days ago

SHA: 713648dffea0a0077d06aabb3f2909a19e8b6325
Parents: a19cb1d
Tree: 10c1321

2 changed files

Status	File	+	-
M	`internal/runner/engine/docker.go`	115	9
M	`internal/runner/engine/docker_test.go`	58	2

internal/runner/engine/docker.gomodified

  	"errors"
  	"fmt"
  	"io"
++	"log/slog"
  	"os"
  	"os/exec"
  	"path"
  	"regexp"
  	"sort"
++	"strconv"
  	"strings"
  	"sync"
  	"time"
  	ErrUnsupported     = errors.New("runner engine: unsupported operation")
+ )
++const (
++	defaultSeccompProfile = "/etc/shithubd-runner/seccomp.json"
++	defaultContainerUser  = "65534:65534"
++	defaultPidsLimit      = 512
++	defaultNofileLimit    = "4096:4096"
++	defaultNprocLimit     = "512:512"
++
++	// rootPermissionKey is an intentionally shithub-specific escape hatch.
++	// It requires an explicit per-job permissions entry rather than treating
++	// broad write-all permissions as permission to run the container as root.
++	rootPermissionKey = "shithub-runner-root"
++)
++
  type CommandRunner interface {
  	Run(ctx context.Context, name string, args []string, stdout, stderr io.Writer) error
+ }
  	Network          string
  	Memory           string
  	CPUs             string
++	SeccompProfile   string
++	User             string
++	PidsLimit        int
  	LogChunkBytes    int
  	LogFlushInterval time.Duration
  	StepLogLimit     int64
  	Stderr           io.Writer
  	Runner           CommandRunner
  	MaskValues       []string
++	Logger           *slog.Logger
+ }
  type Docker struct {
  	if cfg.Runner == nil {
  		cfg.Runner = ExecRunner{}
+ 	}
++	if cfg.SeccompProfile == "" {
++		cfg.SeccompProfile = defaultSeccompProfile
++	}
++	if cfg.User == "" {
++		cfg.User = defaultContainerUser
++	}
++	if cfg.PidsLimit <= 0 {
++		cfg.PidsLimit = defaultPidsLimit
++	}
++	if cfg.Logger == nil {
++		cfg.Logger = slog.New(slog.NewTextHandler(io.Discard, nil))
++	}
  	return &Docker{cfg: cfg, streams: make(map[int64]chan LogChunk), eventSubs: make(map[int64]chan Event)}
+ }
  	if strings.TrimSpace(step.Run) == "" {
  		return nil
+ 	}
--	args, err := d.dockerArgs(job, step)
++	invocation, err := d.dockerInvocation(job, step)
  	if err != nil {
  		return err
+ 	}
++	d.logStep(ctx, "runner step starting", job, step, invocation, "")
  	writer := d.newStepLogWriter(ctx, job.ID, step.ID, job.MaskValues)
  	out := io.MultiWriter(d.cfg.Stdout, writer)
  	errOut := io.MultiWriter(d.cfg.Stderr, writer)
--	if err := d.cfg.Runner.Run(ctx, d.cfg.Binary, args, out, errOut); err != nil {
++	if err := d.cfg.Runner.Run(ctx, d.cfg.Binary, invocation.args, out, errOut); err != nil {
++		d.logStep(ctx, "runner step completed", job, step, invocation, conclusionForError(err))
  		if closeErr := writer.Close(); closeErr != nil {
  			return fmt.Errorf("runner engine: step %q failed: %w", stepLabel(step), errors.Join(err, closeErr))
+ 		}
  		return fmt.Errorf("runner engine: step %q failed: %w", stepLabel(step), err)
+ 	}
++	d.logStep(ctx, "runner step completed", job, step, invocation, ConclusionSuccess)
  	if err := writer.Close(); err != nil {
  		return fmt.Errorf("runner engine: flush step %q logs: %w", stepLabel(step), err)
+ 	}
  	return nil
+ }
--func (d *Docker) dockerArgs(job Job, step Step) ([]string, error) {
++type dockerInvocation struct {
++	args           []string
++	image          string
++	network        string
++	memory         string
++	cpus           string
++	user           string
++	seccompProfile string
++	pidsLimit      int
++}
++
++func (d *Docker) dockerInvocation(job Job, step Step) (dockerInvocation, error) {
  	workdir, err := containerWorkdir(step.WorkingDirectory)
  	if err != nil {
--		return nil, err
++		return dockerInvocation{}, err
+ 	}
  	image := strings.TrimSpace(job.Image)
  	if image == "" {
  		image = d.cfg.DefaultImage
+ 	}
  	if image == "" {
--		return nil, errors.New("runner engine: image is required")
++		return dockerInvocation{}, errors.New("runner engine: image is required")
+ 	}
  	rendered, err := runnerexec.RenderStep(runnerexec.StepInput{
  		Run:     step.Run,
  		Context: expressionContext(job),
  	})
  	if err != nil {
--		return nil, fmt.Errorf("runner engine: render step %q: %w", stepLabel(step), err)
++		return dockerInvocation{}, fmt.Errorf("runner engine: render step %q: %w", stepLabel(step), err)
++	}
++	user := d.cfg.User
++	if permissionsRequestRoot(job.Permissions) {
++		user = "0:0"
+ 	}
  	args := []string{
  		"run",
  		"--network=" + d.cfg.Network,
  		"--memory=" + d.cfg.Memory,
  		"--cpus=" + d.cfg.CPUs,
++		"--pids-limit=" + strconv.Itoa(d.cfg.PidsLimit),
++		"--read-only",
++		"--tmpfs", "/tmp:rw,exec,nosuid,nodev,size=1g",
++		"--cap-drop=ALL",
++		"--cap-add=DAC_OVERRIDE",
++		"--cap-add=SETGID",
++		"--cap-add=SETUID",
++		"--security-opt=no-new-privileges",
++		"--security-opt=seccomp=" + d.cfg.SeccompProfile,
++		"--ulimit", "nofile=" + defaultNofileLimit,
++		"--ulimit", "nproc=" + defaultNprocLimit,
++		"--user", user,
  		"--workdir=" + workdir,
--		"-v", job.WorkspaceDir + ":/workspace",
++		"--mount", "type=bind,src=" + job.WorkspaceDir + ",dst=/workspace,rw",
+ 	}
  	env, err := validateEnv(rendered.Env)
  	if err != nil {
--		return nil, err
++		return dockerInvocation{}, err
+ 	}
  	for _, key := range sortedKeys(env) {
  		args = append(args, "-e", key+"="+env[key])
+ 	}
  	args = append(args, image, "bash", "-c", rendered.Run)
--	return args, nil
++	return dockerInvocation{
++		args:           args,
++		image:          image,
++		network:        d.cfg.Network,
++		memory:         d.cfg.Memory,
++		cpus:           d.cfg.CPUs,
++		user:           user,
++		seccompProfile: d.cfg.SeccompProfile,
++		pidsLimit:      d.cfg.PidsLimit,
++	}, nil
++}
++
++func (d *Docker) logStep(ctx context.Context, msg string, job Job, step Step, invocation dockerInvocation, conclusion string) {
++	attrs := []any{
++		"run_id", job.RunID,
++		"job_id", job.ID,
++		"step_id", step.ID,
++		"image", invocation.image,
++		"network", invocation.network,
++		"cpu_limit", invocation.cpus,
++		"memory_limit", invocation.memory,
++		"pids_limit", invocation.pidsLimit,
++		"container_user", invocation.user,
++		"seccomp_profile", invocation.seccompProfile,
++	}
++	if conclusion != "" {
++		attrs = append(attrs, "conclusion", conclusion)
++	}
++	d.cfg.Logger.InfoContext(ctx, msg, attrs...)
+ }
  func expressionContext(job Job) expr.Context {
+ 	}
+ }
++func permissionsRequestRoot(raw json.RawMessage) bool {
++	if len(raw) == 0 || !json.Valid(raw) {
++		return false
++	}
++	var shaped struct {
++		Per map[string]string `json:"per"`
++	}
++	if err := json.Unmarshal(raw, &shaped); err == nil && strings.EqualFold(shaped.Per[rootPermissionKey], "write") {
++		return true
++	}
++	var flat map[string]string
++	if err := json.Unmarshal(raw, &flat); err != nil {
++		return false
++	}
++	return strings.EqualFold(flat[rootPermissionKey], "write")
++}
++
  func (d *Docker) StreamLogs(_ context.Context, jobID int64) (<-chan LogChunk, error) {
  	return d.ensureStream(jobID), nil
+ }

internal/runner/engine/docker_test.gomodified

  	"errors"
  	"io"
  	"reflect"
++	"strings"
  	"testing"
  	"time"
+ )
  		t.Fatalf("Conclusion: %q", out.Conclusion)
+ 	}
  	want := []string{
--		"run", "--rm", "--network=none", "--memory=2g", "--cpus=2", "--workdir=/workspace/subdir",
++		"run", "--rm", "--network=none", "--memory=2g", "--cpus=2",
--		"-v", rec.args[7],
++		"--pids-limit=512", "--read-only",
++		"--tmpfs", "/tmp:rw,exec,nosuid,nodev,size=1g",
++		"--cap-drop=ALL", "--cap-add=DAC_OVERRIDE", "--cap-add=SETGID", "--cap-add=SETUID",
++		"--security-opt=no-new-privileges", "--security-opt=seccomp=/etc/shithubd-runner/seccomp.json",
++		"--ulimit", "nofile=4096:4096", "--ulimit", "nproc=512:512",
++		"--user", "65534:65534",
++		"--workdir=/workspace/subdir",
++		"--mount", rec.args[23],
  		"-e", "A=job", "-e", "B=step",
  		"runner-image", "bash", "-c", "echo hi",
+ 	}
  	if !reflect.DeepEqual(rec.args, want) {
  		t.Fatalf("args:\ngot  %#v\nwant %#v", rec.args, want)
+ 	}
++	if !strings.HasPrefix(rec.args[23], "type=bind,src=") || !strings.HasSuffix(rec.args[23], ",dst=/workspace,rw") {
++		t.Fatalf("workspace mount arg: %q", rec.args[23])
++	}
+ }
  func TestDockerExecute_RendersTaintedExpressionsThroughInputEnv(t *testing.T) {
+ 	}
+ }
++func TestDockerExecute_RootRequiresExplicitPermission(t *testing.T) {
++	t.Parallel()
++	for _, tc := range []struct {
++		name        string
++		permissions string
++		wantUser    string
++	}{
++		{name: "default", permissions: `{}`, wantUser: "65534:65534"},
++		{name: "write-all-does-not-root", permissions: `{"mode":"write-all"}`, wantUser: "65534:65534"},
++		{name: "explicit-root", permissions: `{"per":{"shithub-runner-root":"write"}}`, wantUser: "0:0"},
++	} {
++		t.Run(tc.name, func(t *testing.T) {
++			t.Parallel()
++			rec := &recordingRunner{}
++			d := NewDocker(DockerConfig{
++				DefaultImage: "runner-image",
++				Network:      "bridge",
++				Memory:       "2g",
++				CPUs:         "2",
++				Runner:       rec,
++			})
++			if _, err := d.Execute(t.Context(), Job{
++				ID:           1,
++				Permissions:  []byte(tc.permissions),
++				WorkspaceDir: t.TempDir(),
++				Steps:        []Step{{Run: "id -u"}},
++			}); err != nil {
++				t.Fatalf("Execute: %v", err)
++			}
++			if got := argAfter(rec.args, "--user"); got != tc.wantUser {
++				t.Fatalf("--user: got %q want %q in %#v", got, tc.wantUser, rec.args)
++			}
++		})
++	}
++}
++
  func TestDockerExecute_StreamsStepLogs(t *testing.T) {
  	t.Parallel()
  	d := NewDocker(DockerConfig{
+ 	}
  	return false
+ }
++
++func argAfter(args []string, flag string) string {
++	for i, arg := range args {
++		if arg == flag && i+1 < len(args) {
++			return args[i+1]
++		}
++	}
++	return ""
++}