diff --git a/act/container/container_types.go b/act/container/container_types.go index 5e0829d3..6fa53a68 100644 --- a/act/container/container_types.go +++ b/act/container/container_types.go @@ -25,26 +25,27 @@ func (e ExitCodeError) Error() string { // NewContainerInput the input for the New function type NewContainerInput struct { - Image string - Username string - Password string - Entrypoint []string - Cmd []string - WorkingDir string - Env []string - Binds []string - Mounts map[string]string - Name string - Stdout io.Writer - Stderr io.Writer - NetworkMode string - Privileged bool - UsernsMode string - Platform string - Options string - NetworkAliases []string - ExposedPorts nat.PortSet - PortBindings nat.PortMap + Image string + Username string + Password string + Entrypoint []string + Cmd []string + WorkingDir string + Env []string + Binds []string + Mounts map[string]string + Name string + Stdout io.Writer + Stderr io.Writer + NetworkMode string + Privileged bool + UsernsMode string + Platform string + RunnerOptions string // container options the runner was configured with, trusted + WorkflowOptions string // container options the workflow asked for, untrusted + NetworkAliases []string + ExposedPorts nat.PortSet + PortBindings nat.PortMap // Gitea specific AutoRemove bool diff --git a/act/container/docker_cli.go b/act/container/docker_cli.go index 26e0b11e..e9135d36 100644 --- a/act/container/docker_cli.go +++ b/act/container/docker_cli.go @@ -350,7 +350,7 @@ type containerConfig struct { // parse parses the args for the specified command and generates a Config, // a HostConfig and returns them with the specified command. // If the specified args are not valid, it will return an error. -func parse(flags *pflag.FlagSet, copts *containerOptions, serverOS string) (*containerConfig, error) { //nolint:gocyclo // verbatim copy from docker/cli +func parse(flags *pflag.FlagSet, copts *containerOptions, serverOS string) (*containerConfig, error) { //nolint:gocyclo,unparam // verbatim copy from docker/cli var ( attachStdin = copts.attach.Get("stdin") attachStdout = copts.attach.Get("stdout") diff --git a/act/container/docker_create_flags.go b/act/container/docker_create_flags.go index 82796cc2..9638137a 100644 --- a/act/container/docker_create_flags.go +++ b/act/container/docker_create_flags.go @@ -10,7 +10,9 @@ import ( "fmt" "io" "slices" + "strings" + "github.com/docker/cli/opts" "github.com/kballard/go-shellquote" "github.com/spf13/pflag" ) @@ -51,6 +53,7 @@ func parseContainerOptions(options string) (*pflag.FlagSet, *containerOptions, * flags := pflag.NewFlagSet("container_flags", pflag.ContinueOnError) flags.SetOutput(io.Discard) copts := addFlags(flags) + copts.env = opts.NewListOpts(validateEnv) // addFlags registered this field's address, so the swap takes effect cf := registerCreateFlags(flags) args, err := shellquote.Split(options) @@ -73,6 +76,30 @@ func createFlagsFromOptions(options string) *createFlags { return cf } +// validateEnv is opts.ValidateEnv without its lookup of a bare name in the runner's environment. +func validateEnv(val string) (string, error) { + if name, _, _ := strings.Cut(val, "="); name == "" { + return "", errors.New("invalid environment variable: " + val) + } + return val, nil +} + +// rejectHostReadingOptions refuses the flags naming files that are read here, on the +// runner, rather than in the container. +func rejectHostReadingOptions(options string) error { + flags, _, _, err := parseContainerOptions(options) + if err != nil { + return err + } + + for _, name := range []string{"env-file", "label-file"} { + if flags.Changed(name) { + return fmt.Errorf("container option --%s reads files from the runner and is not allowed in a workflow", name) + } + } + return nil +} + func (cf *createFlags) validate() error { if !slices.Contains(pullPolicies, cf.pull) { return fmt.Errorf("invalid --pull option %q: must be one of %q", cf.pull, pullPolicies) diff --git a/act/container/docker_create_flags_test.go b/act/container/docker_create_flags_test.go index 705c3b14..ca7bc180 100644 --- a/act/container/docker_create_flags_test.go +++ b/act/container/docker_create_flags_test.go @@ -50,13 +50,13 @@ func TestCreateFlagsValidate(t *testing.T) { } func TestNewContainerAppliesCreateFlags(t *testing.T) { - input := &NewContainerInput{Platform: "linux/amd64", Options: "--platform linux/arm64 --pull never"} + input := &NewContainerInput{Platform: "linux/amd64", RunnerOptions: "--pull never", WorkflowOptions: "--platform linux/arm64"} cr, ok := NewContainer(input).(*containerReference) require.True(t, ok) assert.Equal(t, "linux/arm64", input.Platform) assert.Equal(t, pullPolicyNever, cr.pullPolicy) - kept := &NewContainerInput{Platform: "linux/amd64", Options: "--privileged"} + kept := &NewContainerInput{Platform: "linux/amd64", RunnerOptions: "--privileged"} NewContainer(kept) assert.Equal(t, "linux/amd64", kept.Platform) } diff --git a/act/container/docker_run.go b/act/container/docker_run.go index c7dfca5f..d3e86e40 100644 --- a/act/container/docker_run.go +++ b/act/container/docker_run.go @@ -15,6 +15,7 @@ import ( "io" "os" "path/filepath" + "reflect" "regexp" "runtime" "slices" @@ -57,7 +58,7 @@ func NewContainer(input *NewContainerInput) ExecutionsEnvironment { cr := new(containerReference) cr.input = input // Resolved up front because the image pull runs before the container is created. - cf := createFlagsFromOptions(input.Options) + cf := createFlagsFromOptions(input.allOptions()) if cf.platform != "" { cr.input.Platform = cf.platform } @@ -524,22 +525,32 @@ func (cr *containerReference) waitForRemoval(ctx context.Context, idOrName strin } } +// allOptions puts the runner's options first, so a flag both sources set ends up the workflow's. +func (input *NewContainerInput) allOptions() string { + return strings.TrimSpace(input.RunnerOptions + " " + input.WorkflowOptions) +} + func (cr *containerReference) mergeContainerConfigs(ctx context.Context, config *container.Config, hostConfig *container.HostConfig) (*container.Config, *container.HostConfig, error) { logger := common.Logger(ctx) - input := cr.input + options := cr.input.allOptions() - if input.Options == "" { + if options == "" { return config, hostConfig, nil } + // For Gitea, checked here because the parse below is what would read those files + if err := rejectHostReadingOptions(cr.input.WorkflowOptions); err != nil { + return nil, nil, err + } + // parse configuration from CLI container.options - flags, copts, cf, err := parseContainerOptions(input.Options) + flags, copts, cf, err := parseContainerOptions(options) if err != nil { return nil, nil, err } if err := cf.validate(); err != nil { - return nil, nil, fmt.Errorf("cannot process container options: '%s': '%w'", input.Options, err) + return nil, nil, fmt.Errorf("cannot process container options: '%s': '%w'", options, err) } // FIXME: If everything is fine after gitea/act v0.260.0, remove the following comment. @@ -570,24 +581,23 @@ func (cr *containerReference) mergeContainerConfigs(ctx context.Context, config containerConfig, err := parse(flags, copts, runtime.GOOS) if err != nil { - return nil, nil, fmt.Errorf("cannot process container options: '%s': '%w'", input.Options, err) + return nil, nil, fmt.Errorf("cannot process container options: '%s': '%w'", options, err) } - // For Gitea - // When privileged mode is disabled, container.options is workflow-controlled - // untrusted input. Strip the HostConfig fields that would let a workflow break - // out of the container (host namespaces, capability expansion, security profile - // overrides, device and runtime access). Otherwise these survive into the final - // HostConfig even though --privileged is forced off. + // For Gitea, forcing --privileged off is not enough, other options reach the host too if !hostConfig.Privileged { - sanitizeOptionsHostConfig(logger, containerConfig.HostConfig) + trusted, err := parseOptionsHostConfig(cr.input.RunnerOptions) + if err != nil { + return nil, nil, err + } + sanitizeOptionsHostConfig(logger, containerConfig.HostConfig, trusted) } logger.Debugf("Custom container.Config from options ==> %+v", containerConfig.Config) err = mergo.Merge(config, containerConfig.Config, mergo.WithOverride, mergo.WithAppendSlice) if err != nil { - return nil, nil, fmt.Errorf("cannot merge container.Config options: '%s': '%w'", input.Options, err) + return nil, nil, fmt.Errorf("cannot merge container.Config options: '%s': '%w'", options, err) } logger.Debugf("Merged container.Config ==> %+v", config) @@ -599,14 +609,15 @@ func (cr *containerReference) mergeContainerConfigs(ctx context.Context, config networkMode := hostConfig.NetworkMode err = mergo.Merge(hostConfig, containerConfig.HostConfig, mergo.WithOverride) if err != nil { - return nil, nil, fmt.Errorf("cannot merge container.HostConfig options: '%s': '%w'", input.Options, err) + return nil, nil, fmt.Errorf("cannot merge container.HostConfig options: '%s': '%w'", options, err) } hostConfig.Binds = binds hostConfig.Mounts = mounts if cf.name != "" { logger.Warn("--name in the options will be ignored.") } - if len(copts.netMode.Value()) > 0 { + // the runner's own network mode was put into copts above, so ask the flags instead + if flags.Changed("network") || flags.Changed("net") { logger.Warn("--network and --net in the options will be ignored.") } hostConfig.NetworkMode = networkMode @@ -1112,74 +1123,64 @@ func (cr *containerReference) wait() common.Executor { } // For Gitea -// sanitizeOptionsHostConfig clears the HostConfig fields parsed from a -// workflow-controlled container.options string that could be used to escape the -// container when privileged mode is disabled. It must only be called when the -// runner has privileged mode turned off; with privileged mode enabled the -// administrator has already opted into host access. -func sanitizeOptionsHostConfig(logger logrus.FieldLogger, hostConfig *container.HostConfig) { - warn := func(option string) { - logger.Warnf("container option %q is not allowed when privileged mode is disabled and will be ignored", option) - } +// sanitizeOptionsHostConfig takes back everything a workflow could escape the container with, +// setting each field to trusted, which is what the runner's own options parse to on their own. +// Only for unprivileged mode, since privileged mode grants host access anyway. +func sanitizeOptionsHostConfig(logger logrus.FieldLogger, hostConfig, trusted *container.HostConfig) { + resetOption(logger, "--pid", &hostConfig.PidMode, trusted.PidMode) + resetOption(logger, "--ipc", &hostConfig.IpcMode, trusted.IpcMode) + resetOption(logger, "--uts", &hostConfig.UTSMode, trusted.UTSMode) + resetOption(logger, "--cgroupns", &hostConfig.CgroupnsMode, trusted.CgroupnsMode) + resetOption(logger, "--userns", &hostConfig.UsernsMode, trusted.UsernsMode) // --userns=host would undo the remapping the runner asked for + resetOption(logger, "--cap-add", &hostConfig.CapAdd, trusted.CapAdd) + resetOption(logger, "--security-opt", &hostConfig.SecurityOpt, trusted.SecurityOpt) + resetOption(logger, "--device", &hostConfig.Devices, trusted.Devices) + resetOption(logger, "--device-cgroup-rule", &hostConfig.DeviceCgroupRules, trusted.DeviceCgroupRules) + resetOption(logger, "--gpus", &hostConfig.DeviceRequests, trusted.DeviceRequests) + resetOption(logger, "--volumes-from", &hostConfig.VolumesFrom, trusted.VolumesFrom) + resetOption(logger, "--runtime", &hostConfig.Runtime, trusted.Runtime) + resetOption(logger, "--cgroup-parent", &hostConfig.CgroupParent, trusted.CgroupParent) + resetOption(logger, "--sysctl", &hostConfig.Sysctls, trusted.Sysctls) + resetOption(logger, "--isolation", &hostConfig.Isolation, trusted.Isolation) // windows: process isolation drops the hyper-v boundary + resetOption(logger, "--volume-driver", &hostConfig.VolumeDriver, trusted.VolumeDriver) + // systempaths=unconfined lands in these two rather than in SecurityOpt + resetOption(logger, "--security-opt", &hostConfig.MaskedPaths, trusted.MaskedPaths) + resetOption(logger, "--security-opt", &hostConfig.ReadonlyPaths, trusted.ReadonlyPaths) - if hostConfig.PidMode != "" { - warn("--pid") - hostConfig.PidMode = "" + // a driver mounts what it likes, e.g. local with device= binds any host path, which + // valid_volumes never gets to see + hostConfig.Mounts = slices.DeleteFunc(hostConfig.Mounts, func(mt mount.Mount) bool { + if mt.VolumeOptions == nil || mt.VolumeOptions.DriverConfig == nil || + slices.ContainsFunc(trusted.Mounts, func(t mount.Mount) bool { return reflect.DeepEqual(t, mt) }) { + return false + } + logger.Warnf("volume driver of %q in the workflow is not allowed when privileged mode is disabled and will be ignored", mt.Source) + return true + }) +} + +// resetOption puts a field back to the runner's own value. It compares the values rather than +// the flags, so a field that more than one option feeds cannot slip through. +func resetOption[T any](logger logrus.FieldLogger, option string, field *T, trusted T) { + if reflect.DeepEqual(*field, trusted) { + return } - if hostConfig.IpcMode != "" { - warn("--ipc") - hostConfig.IpcMode = "" + logger.Warnf("container option %q in the workflow is not allowed when privileged mode is disabled and will be ignored", option) + *field = trusted +} + +// parseOptionsHostConfig parses one options string on its own, to see what it alone asks for. +// Even "" goes through the parser, or its empty slices and maps would differ from a real parse. +func parseOptionsHostConfig(options string) (*container.HostConfig, error) { + flags, copts, _, err := parseContainerOptions(options) + if err != nil { + return nil, err } - if hostConfig.UTSMode != "" { - warn("--uts") - hostConfig.UTSMode = "" - } - if hostConfig.CgroupnsMode != "" { - warn("--cgroupns") - hostConfig.CgroupnsMode = "" - } - // UsernsMode is set from the runner-controlled input; never let options - // override it (e.g. --userns=host disables user namespace remapping). - if hostConfig.UsernsMode != "" { - warn("--userns") - hostConfig.UsernsMode = "" - } - if len(hostConfig.CapAdd) > 0 { - warn("--cap-add") - hostConfig.CapAdd = nil - } - if len(hostConfig.SecurityOpt) > 0 { - warn("--security-opt") - hostConfig.SecurityOpt = nil - } - if len(hostConfig.Devices) > 0 { - warn("--device") - hostConfig.Devices = nil - } - if len(hostConfig.DeviceCgroupRules) > 0 { - warn("--device-cgroup-rule") - hostConfig.DeviceCgroupRules = nil - } - if len(hostConfig.DeviceRequests) > 0 { - warn("--gpus") - hostConfig.DeviceRequests = nil - } - if len(hostConfig.VolumesFrom) > 0 { - warn("--volumes-from") - hostConfig.VolumesFrom = nil - } - if hostConfig.Runtime != "" { - warn("--runtime") - hostConfig.Runtime = "" - } - if hostConfig.CgroupParent != "" { - warn("--cgroup-parent") - hostConfig.CgroupParent = "" - } - if len(hostConfig.Sysctls) > 0 { - warn("--sysctl") - hostConfig.Sysctls = nil + containerConfig, err := parse(flags, copts, runtime.GOOS) + if err != nil { + return nil, fmt.Errorf("cannot process container options: '%s': '%w'", options, err) } + return containerConfig.HostConfig, nil } // For Gitea diff --git a/act/container/docker_run_test.go b/act/container/docker_run_test.go index 641d9be6..3998c8a4 100644 --- a/act/container/docker_run_test.go +++ b/act/container/docker_run_test.go @@ -583,106 +583,133 @@ func TestCheckVolumes(t *testing.T) { } } +// A volume driver decides for itself what it mounts, e.g. the local driver with device= binds +// any host path, which valid_volumes never gets to see. +func TestMergeContainerConfigsDropsVolumeDriversFromWorkflows(t *testing.T) { + const escape = "--mount type=volume,src=job-escape,dst=/host,volume-driver=local,volume-opt=type=none,volume-opt=o=bind,volume-opt=device=/" + + hostConfig, _ := mergeOptions(t, "", escape+" --mount type=volume,src=job-plain,dst=/cache", false) + require.Len(t, hostConfig.Mounts, 1) + assert.Equal(t, "job-plain", hostConfig.Mounts[0].Source) + + // the same mount from the runner's own options is the administrator's to make + hostConfig, _ = mergeOptions(t, escape, "", false) + require.Len(t, hostConfig.Mounts, 1) + assert.Equal(t, "job-escape", hostConfig.Mounts[0].Source) +} + +// Both of these are read here, on the runner, so a workflow could read the runner's files +// and environment with them. +func TestMergeContainerConfigsKeepsTheRunnersFilesAndEnvToItself(t *testing.T) { + hostFile := filepath.Join(t.TempDir(), "host.env") + require.NoError(t, os.WriteFile(hostFile, []byte("STOLEN=from-the-host\n"), 0o600)) + t.Setenv("RUNNER_SECRET", "s3cr3t") + + for _, option := range []string{"--env-file " + hostFile, "--label-file " + hostFile} { + logger, _ := test.NewNullLogger() + cr := &containerReference{input: &NewContainerInput{NetworkMode: "bridge", WorkflowOptions: option}} + + _, _, err := cr.mergeContainerConfigs(common.WithLogger(context.Background(), logger), &container.Config{}, &container.HostConfig{}) + require.ErrorContains(t, err, "not allowed in a workflow") + + // the runner reading its own files is what those options are for + cr = &containerReference{input: &NewContainerInput{NetworkMode: "bridge", RunnerOptions: option}} + _, _, err = cr.mergeContainerConfigs(common.WithLogger(context.Background(), logger), &container.Config{}, &container.HostConfig{}) + require.NoError(t, err) + } + + // a bare name is no longer resolved from the runner's environment, for either source + logger, _ := test.NewNullLogger() + cr := &containerReference{input: &NewContainerInput{ + NetworkMode: "bridge", + RunnerOptions: "--env RUNNER_SECRET", + WorkflowOptions: "--env RUNNER_SECRET --env GIVEN=value", + }} + + config, _, err := cr.mergeContainerConfigs(common.WithLogger(context.Background(), logger), &container.Config{}, &container.HostConfig{}) + require.NoError(t, err) + assert.Equal(t, []string{"RUNNER_SECRET", "RUNNER_SECRET", "GIVEN=value"}, config.Env) +} + func TestSanitizeOptionsHostConfig(t *testing.T) { logger, _ := test.NewNullLogger() - dangerous := func() *container.HostConfig { - return &container.HostConfig{ - PidMode: "host", - IpcMode: "host", - UTSMode: "host", - CgroupnsMode: "host", - UsernsMode: "host", - CapAdd: []string{"ALL"}, - SecurityOpt: []string{"seccomp=unconfined", "apparmor=unconfined"}, - VolumesFrom: []string{"other"}, - Runtime: "runc", - CgroupParent: "/custom", - Devices: []container.DeviceMapping{{PathOnHost: "/dev/sda", PathInContainer: "/dev/sda", CgroupPermissions: "rwm"}}, - DeviceCgroupRules: []string{"a *:* rwm"}, - Sysctls: map[string]string{"net.ipv4.ip_forward": "1"}, - } + // every field the sanitizer resets, so a reset dropped in a refactor fails here + hostConfig := &container.HostConfig{ + PidMode: "host", + IpcMode: "host", + UTSMode: "host", + CgroupnsMode: "host", + UsernsMode: "host", + CapAdd: []string{"ALL"}, + SecurityOpt: []string{"seccomp=unconfined", "apparmor=unconfined"}, + VolumesFrom: []string{"other"}, + Runtime: "runc", + Isolation: "process", + VolumeDriver: "rogue", + MaskedPaths: []string{}, + ReadonlyPaths: []string{}, + CgroupParent: "/custom", + Devices: []container.DeviceMapping{{PathOnHost: "/dev/sda", PathInContainer: "/dev/sda", CgroupPermissions: "rwm"}}, + DeviceCgroupRules: []string{"a *:* rwm"}, + DeviceRequests: []container.DeviceRequest{{Count: -1, Capabilities: [][]string{{"gpu"}}}}, + Sysctls: map[string]string{"net.ipv4.ip_forward": "1"}, } - hostConfig := dangerous() - sanitizeOptionsHostConfig(logger, hostConfig) + sanitizeOptionsHostConfig(logger, hostConfig, &container.HostConfig{}) - assert.Empty(t, string(hostConfig.PidMode)) - assert.Empty(t, string(hostConfig.IpcMode)) - assert.Empty(t, string(hostConfig.UTSMode)) - assert.Empty(t, string(hostConfig.CgroupnsMode)) - assert.Empty(t, string(hostConfig.UsernsMode)) - assert.Empty(t, hostConfig.CapAdd) - assert.Empty(t, hostConfig.SecurityOpt) - assert.Empty(t, hostConfig.Devices) - assert.Empty(t, hostConfig.DeviceCgroupRules) - assert.Empty(t, hostConfig.VolumesFrom) - assert.Empty(t, hostConfig.Runtime) - assert.Empty(t, hostConfig.CgroupParent) - assert.Empty(t, hostConfig.Sysctls) + assert.Equal(t, &container.HostConfig{}, hostConfig) +} + +// mergeOptions merges both option sources into a bare container, returning the result and its log. +func mergeOptions(t *testing.T, runnerOptions, workflowOptions string, privileged bool) (*container.HostConfig, *test.Hook) { + t.Helper() + logger, hook := test.NewNullLogger() + cr := &containerReference{input: &NewContainerInput{ + RunnerOptions: runnerOptions, + WorkflowOptions: workflowOptions, + NetworkMode: "bridge", + UsernsMode: "private", + }} + + _, hostConfig, err := cr.mergeContainerConfigs(common.WithLogger(context.Background(), logger), &container.Config{}, &container.HostConfig{ + Privileged: privileged, + UsernsMode: container.UsernsMode("private"), + NetworkMode: container.NetworkMode("bridge"), + }) + require.NoError(t, err) + return hostConfig, hook } func TestMergeContainerConfigsStripsDangerousOptionsWhenUnprivileged(t *testing.T) { - // OS-independent options only: --device parsing requires a linux/windows - // server OS, which is not guaranteed for the test host. + // OS-independent options only, --device and --gpus need a linux/windows server OS const dangerousOptions = "--pid=host --ipc=host --uts=host --cgroupns=host " + "--userns=host --cap-add=ALL --security-opt seccomp=unconfined " + - "--security-opt apparmor=unconfined --volumes-from other " + + "--security-opt apparmor=unconfined --volumes-from other --isolation process " + "--runtime runc --cgroup-parent /custom --sysctl net.ipv4.ip_forward=1" - t.Run("unprivileged strips host-escape options", func(t *testing.T) { - logger, _ := test.NewNullLogger() - ctx := common.WithLogger(context.Background(), logger) - cr := &containerReference{ - input: &NewContainerInput{ - Options: dangerousOptions, - NetworkMode: "bridge", - UsernsMode: "private", - }, - } + // whatever the workflow adds, an unprivileged container comes out exactly as the runner's + // own options alone describe it, field for field + for _, runnerOptions := range []string{"--shm-size 1g", dangerousOptions, "--cap-add SYS_ADMIN --security-opt seccomp=unconfined"} { + runnerOnly, _ := mergeOptions(t, runnerOptions, "", false) + withWorkflow, _ := mergeOptions(t, runnerOptions, dangerousOptions, false) - _, hostConfig, err := cr.mergeContainerConfigs(ctx, &container.Config{}, &container.HostConfig{ - Privileged: false, - UsernsMode: container.UsernsMode("private"), - NetworkMode: container.NetworkMode("bridge"), - }) - require.NoError(t, err) + assert.Equal(t, runnerOnly, withWorkflow, "runner options: %q", runnerOptions) + } - assert.False(t, hostConfig.Privileged) - assert.Empty(t, string(hostConfig.PidMode)) - assert.Empty(t, string(hostConfig.IpcMode)) - assert.Empty(t, string(hostConfig.UTSMode)) - assert.Empty(t, string(hostConfig.CgroupnsMode)) - // UsernsMode must keep the runner-controlled value, not the one from options. - assert.Equal(t, "private", string(hostConfig.UsernsMode)) - assert.Empty(t, hostConfig.CapAdd) - assert.Empty(t, hostConfig.SecurityOpt) - assert.Empty(t, hostConfig.VolumesFrom) - assert.Empty(t, hostConfig.Runtime) - assert.Empty(t, hostConfig.CgroupParent) - assert.Empty(t, hostConfig.Sysctls) - }) + // the same options from the runner reach the daemon, even --userns, which no workflow may set + kept, _ := mergeOptions(t, dangerousOptions, "", false) + assert.Equal(t, "host", string(kept.PidMode)) + assert.Equal(t, []string{"ALL"}, kept.CapAdd) + assert.Equal(t, "runc", kept.Runtime) + assert.Equal(t, "host", string(kept.UsernsMode)) + assert.False(t, kept.Privileged) - t.Run("privileged preserves options", func(t *testing.T) { - logger, _ := test.NewNullLogger() - ctx := common.WithLogger(context.Background(), logger) - cr := &containerReference{ - input: &NewContainerInput{ - Options: "--pid=host --cap-add=ALL --security-opt seccomp=unconfined", - NetworkMode: "bridge", - }, - } - - _, hostConfig, err := cr.mergeContainerConfigs(ctx, &container.Config{}, &container.HostConfig{ - Privileged: true, - NetworkMode: container.NetworkMode("bridge"), - }) - require.NoError(t, err) - - assert.Equal(t, "host", string(hostConfig.PidMode)) - assert.Equal(t, []string{"ALL"}, hostConfig.CapAdd) - assert.Equal(t, []string{"seccomp=unconfined"}, hostConfig.SecurityOpt) - }) + // privileged is the administrator opting in, so the workflow's options are honored + privileged, _ := mergeOptions(t, "", dangerousOptions, true) + assert.Equal(t, "host", string(privileged.PidMode)) + assert.Equal(t, []string{"ALL"}, privileged.CapAdd) + assert.Equal(t, []string{"seccomp=unconfined", "apparmor=unconfined"}, privileged.SecurityOpt) } func TestCheckVolumesRejectsEscapingHostPaths(t *testing.T) { @@ -780,8 +807,8 @@ func TestMergeContainerConfigsVolumesReplaceRunnerMounts(t *testing.T) { ctx := common.WithLogger(context.Background(), logger) cr := &containerReference{ input: &NewContainerInput{ - NetworkMode: "bridge", - Options: "--volume /host/tools:/opt/hostedtoolcache", + NetworkMode: "bridge", + RunnerOptions: "--volume /host/tools:/opt/hostedtoolcache", }, } @@ -794,6 +821,17 @@ func TestMergeContainerConfigsVolumesReplaceRunnerMounts(t *testing.T) { assert.Empty(t, hostConf.Mounts) } +func TestMergeContainerConfigsWarnsOnlyAboutOptionsThatWereGiven(t *testing.T) { + warnings := func(runnerOptions, workflowOptions string) int { + _, hook := mergeOptions(t, runnerOptions, workflowOptions, false) + return len(hook.AllEntries()) + } + + assert.Zero(t, warnings("--volume /host/tools:/opt/hostedtoolcache", "")) + assert.Zero(t, warnings("", "--shm-size 1g")) + assert.Equal(t, 1, warnings("--network host", "")) +} + // A dead daemon must fail the job, not panic through logrus and not silently // drop the requested platform. func TestSupportsContainerImagePlatformDaemonError(t *testing.T) { diff --git a/act/runner/action.go b/act/runner/action.go index 9ea82cd1..10d36049 100644 --- a/act/runner/action.go +++ b/act/runner/action.go @@ -422,7 +422,7 @@ func evalDockerArgs(ctx context.Context, step step, action *model.Action, cmd *[ } } -func newStepContainer(ctx context.Context, step step, image string, cmd, entrypoint []string, options string) container.Container { +func newStepContainer(ctx context.Context, step step, image string, cmd, entrypoint []string, runnerOptions string) container.Container { rc := step.getRunContext() logWriter := rc.commandLogWriter(ctx) envList := make([]string, 0) @@ -438,24 +438,24 @@ func newStepContainer(ctx context.Context, step step, image string, cmd, entrypo networkMode = "default" } return ContainerNewContainer(&container.NewContainerInput{ - Cmd: cmd, - Entrypoint: entrypoint, - WorkingDir: rc.JobContainer.ToContainerPath(rc.Config.Workdir), - Image: image, - Name: createContainerName(rc.jobContainerName(), "STEP-"+step.getStepModel().ID), - Env: envList, - Mounts: mounts, - NetworkMode: networkMode, - Binds: binds, - Stdout: logWriter, - Stderr: logWriter, - Privileged: rc.Config.Privileged, - UsernsMode: rc.Config.UsernsMode, - Platform: rc.Config.ContainerArchitecture, - Options: options, - AutoRemove: true, - ValidVolumes: rc.validVolumes(), - AllocatePTY: rc.Config.AllocatePTY, + Cmd: cmd, + Entrypoint: entrypoint, + WorkingDir: rc.JobContainer.ToContainerPath(rc.Config.Workdir), + Image: image, + Name: createContainerName(rc.jobContainerName(), "STEP-"+step.getStepModel().ID), + Env: envList, + Mounts: mounts, + NetworkMode: networkMode, + Binds: binds, + Stdout: logWriter, + Stderr: logWriter, + Privileged: rc.Config.Privileged, + UsernsMode: rc.Config.UsernsMode, + Platform: rc.Config.ContainerArchitecture, + RunnerOptions: runnerOptions, + AutoRemove: true, + ValidVolumes: rc.validVolumes(), + AllocatePTY: rc.Config.AllocatePTY, }) } diff --git a/act/runner/run_context.go b/act/runner/run_context.go index d4ed7bbc..255d5d8f 100644 --- a/act/runner/run_context.go +++ b/act/runner/run_context.go @@ -497,27 +497,27 @@ func (rc *RunContext) startJobContainer() common.Executor { serviceContainerName := createContainerName(rc.jobContainerName(), serviceID) c := newContainer(&container.NewContainerInput{ - Name: serviceContainerName, - WorkingDir: ext.ToContainerPath(rc.Config.Workdir), - Image: serviceImage, - Username: serviceUsername, - Password: servicePassword, - Cmd: interpolatedCmd, - Env: envs, - Mounts: serviceMounts, - Binds: serviceBinds, - Stdout: logWriter, - Stderr: logWriter, - Privileged: rc.Config.Privileged, - UsernsMode: rc.Config.UsernsMode, - Platform: rc.Config.ContainerArchitecture, - AutoRemove: false, // so a dead service's log survives, cleanupJobResources removes it - Options: rc.ExprEval.Interpolate(ctx, spec.Options), - NetworkMode: networkName, - NetworkAliases: []string{serviceID}, - ExposedPorts: exposedPorts, - PortBindings: portBindings, - AllocatePTY: rc.Config.AllocatePTY, + Name: serviceContainerName, + WorkingDir: ext.ToContainerPath(rc.Config.Workdir), + Image: serviceImage, + Username: serviceUsername, + Password: servicePassword, + Cmd: interpolatedCmd, + Env: envs, + Mounts: serviceMounts, + Binds: serviceBinds, + Stdout: logWriter, + Stderr: logWriter, + Privileged: rc.Config.Privileged, + UsernsMode: rc.Config.UsernsMode, + Platform: rc.Config.ContainerArchitecture, + AutoRemove: false, // so a dead service's log survives, cleanupJobResources removes it + WorkflowOptions: rc.ExprEval.Interpolate(ctx, spec.Options), + NetworkMode: networkName, + NetworkAliases: []string{serviceID}, + ExposedPorts: exposedPorts, + PortBindings: portBindings, + AllocatePTY: rc.Config.AllocatePTY, }) rc.serviceContainers = append(rc.serviceContainers, &serviceContainer{name: serviceID, image: serviceImage, container: c}) } @@ -528,27 +528,28 @@ func (rc *RunContext) startJobContainer() common.Executor { jobContainerNetwork := networkName rc.JobContainer = newContainer(&container.NewContainerInput{ - Cmd: nil, - Entrypoint: []string{"/bin/sleep", fmt.Sprint(rc.Config.ContainerMaxLifetime.Round(time.Second).Seconds())}, - WorkingDir: ext.ToContainerPath(rc.Config.Workdir), - Image: image, - Username: username, - Password: password, - Name: name, - Env: envList, - Mounts: mounts, - NetworkMode: jobContainerNetwork, - NetworkAliases: []string{rc.Name}, - Binds: binds, - Stdout: logWriter, - Stderr: logWriter, - Privileged: rc.Config.Privileged, - UsernsMode: rc.Config.UsernsMode, - Platform: rc.Config.ContainerArchitecture, - Options: rc.options(ctx), - AutoRemove: true, - ValidVolumes: rc.validVolumes(), - AllocatePTY: rc.Config.AllocatePTY, + Cmd: nil, + Entrypoint: []string{"/bin/sleep", fmt.Sprint(rc.Config.ContainerMaxLifetime.Round(time.Second).Seconds())}, + WorkingDir: ext.ToContainerPath(rc.Config.Workdir), + Image: image, + Username: username, + Password: password, + Name: name, + Env: envList, + Mounts: mounts, + NetworkMode: jobContainerNetwork, + NetworkAliases: []string{rc.Name}, + Binds: binds, + Stdout: logWriter, + Stderr: logWriter, + Privileged: rc.Config.Privileged, + UsernsMode: rc.Config.UsernsMode, + Platform: rc.Config.ContainerArchitecture, + RunnerOptions: rc.Config.ContainerOptions, + WorkflowOptions: rc.workflowOptions(ctx), + AutoRemove: true, + ValidVolumes: rc.validVolumes(), + AllocatePTY: rc.Config.AllocatePTY, }) if rc.JobContainer == nil { return errors.New("failed to create job container") @@ -1090,14 +1091,13 @@ func (rc *RunContext) platformImage(ctx context.Context) string { return rc.runsOnImage(ctx) } -func (rc *RunContext) options(ctx context.Context) string { - job := rc.Run.Job() - c := job.Container() - if c != nil { - return rc.Config.ContainerOptions + " " + rc.ExprEval.Interpolate(ctx, c.Options) +func (rc *RunContext) workflowOptions(ctx context.Context) string { + c := rc.Run.Job().Container() + if c == nil { + return "" } - return rc.Config.ContainerOptions + return rc.ExprEval.Interpolate(ctx, c.Options) } func (rc *RunContext) isEnabled(ctx context.Context) (bool, error) { diff --git a/act/runner/run_context_test.go b/act/runner/run_context_test.go index ea32423c..e52b86bb 100644 --- a/act/runner/run_context_test.go +++ b/act/runner/run_context_test.go @@ -236,10 +236,48 @@ func (fakeContainer) Inspect(context.Context) (*container.Info, error) { func (fakeContainer) DumpLogs(context.Context) error { return nil } +// startJobContainerInputs runs startJobContainer against fakeContainer and returns the +// inputs it built, one per container. +func startJobContainerInputs(t *testing.T, workflowYAML string, cfg *Config) []*container.NewContainerInput { + t.Helper() + workflow, err := model.ReadWorkflow(strings.NewReader(workflowYAML)) + require.NoError(t, err) + + var inputs []*container.NewContainerInput + origNewContainer := newContainer + newContainer = func(input *container.NewContainerInput) container.ExecutionsEnvironment { + inputs = append(inputs, input) + return fakeContainer{} + } + t.Cleanup(func() { newContainer = origNewContainer }) + + cfg.Workdir = "/tmp" + cfg.ContainerNetworkMode = "host" // an explicit network mode creates no network + cfg.Env = map[string]string{} + cfg.Secrets = map[string]string{} + + rc := &RunContext{ + Name: "test", + Config: cfg, + Env: map[string]string{}, + Run: &model.Run{ + JobID: "job", + Workflow: workflow, + }, + } + rc.ExprEval = rc.NewExpressionEvaluator(t.Context()) + + // the inputs are built before the missing daemon fails the first call + t.Setenv("DOCKER_HOST", "unix:///nonexistent.sock") + require.Error(t, rc.startJobContainer()(t.Context())) + + return inputs +} + // Regression test: a service without a `credentials:` block resolves to empty // credentials, which used to overwrite the job container's own credentials. func TestStartJobContainerKeepsJobCredentialsWithServices(t *testing.T) { - workflow, err := model.ReadWorkflow(strings.NewReader(` + inputs := startJobContainerInputs(t, ` name: test on: push jobs: @@ -259,35 +297,7 @@ jobs: username: db-user password: db-password steps: [] -`)) - require.NoError(t, err) - - var inputs []*container.NewContainerInput - origNewContainer := newContainer - newContainer = func(input *container.NewContainerInput) container.ExecutionsEnvironment { - inputs = append(inputs, input) - return fakeContainer{} - } - t.Cleanup(func() { newContainer = origNewContainer }) - - rc := &RunContext{ - Name: "test", - Config: &Config{ - Workdir: "/tmp", - ContainerNetworkMode: "host", - Env: map[string]string{}, - Secrets: map[string]string{}, - }, - Env: map[string]string{}, - Run: &model.Run{ - JobID: "job", - Workflow: workflow, - }, - } - rc.ExprEval = rc.NewExpressionEvaluator(t.Context()) - - t.Setenv("DOCKER_HOST", "unix:///nonexistent.sock") - require.Error(t, rc.startJobContainer()(t.Context())) +`, &Config{}) credentials := map[string][2]string{} for _, in := range inputs { @@ -301,10 +311,39 @@ jobs: require.Equal(t, [2]string{"", ""}, credentials["redis:latest"]) } +// Only the workflow's options may be stripped later, so the two sources have to reach the +// container apart from each other. +func TestStartJobContainerKeepsRunnerOptionsApartFromWorkflowOptions(t *testing.T) { + inputs := startJobContainerInputs(t, ` +name: test +on: push +jobs: + job: + runs-on: ubuntu-latest + container: + image: registry.example/job:latest + options: --cap-add SYS_PTRACE + services: + redis: + image: redis:latest + options: --shm-size 1g + steps: [] +`, &Config{ContainerOptions: "--device /dev/fuse"}) + + options := map[string][2]string{} + for _, in := range inputs { + options[in.Image] = [2]string{in.RunnerOptions, in.WorkflowOptions} + } + + require.Equal(t, [2]string{"--device /dev/fuse", "--cap-add SYS_PTRACE"}, options["registry.example/job:latest"]) + // a service container gets no options from the runner's config today + require.Equal(t, [2]string{"", "--shm-size 1g"}, options["redis:latest"]) +} + // A service container reaches the internet the same way the job does, so it inherits the // job's proxy; a service that sets the variable itself keeps its own value. func TestStartJobContainerGivesServicesTheJobProxy(t *testing.T) { - workflow, err := model.ReadWorkflow(strings.NewReader(` + inputs := startJobContainerInputs(t, ` name: test on: push jobs: @@ -320,36 +359,7 @@ jobs: env: no_proxy: db-only.example steps: [] -`)) - require.NoError(t, err) - - var inputs []*container.NewContainerInput - origNewContainer := newContainer - newContainer = func(input *container.NewContainerInput) container.ExecutionsEnvironment { - inputs = append(inputs, input) - return fakeContainer{} - } - t.Cleanup(func() { newContainer = origNewContainer }) - - rc := &RunContext{ - Name: "test", - Config: &Config{ - Workdir: "/tmp", - ContainerNetworkMode: "host", - Env: map[string]string{}, - ProxyEnv: map[string]string{"http_proxy": "http://proxy:3128", "no_proxy": "internal.example"}, - Secrets: map[string]string{}, - }, - Env: map[string]string{}, - Run: &model.Run{ - JobID: "job", - Workflow: workflow, - }, - } - rc.ExprEval = rc.NewExpressionEvaluator(t.Context()) - - t.Setenv("DOCKER_HOST", "unix:///nonexistent.sock") - require.Error(t, rc.startJobContainer()(t.Context())) +`, &Config{ProxyEnv: map[string]string{"http_proxy": "http://proxy:3128", "no_proxy": "internal.example"}}) env := map[string][]string{} for _, in := range inputs { diff --git a/internal/pkg/config/config.example.yaml b/internal/pkg/config/config.example.yaml index d63ff81c..8d1df857 100644 --- a/internal/pkg/config/config.example.yaml +++ b/internal/pkg/config/config.example.yaml @@ -219,6 +219,8 @@ container: # A volume declared here replaces the one the runner would mount on the same container path, so the # tool cache can be kept on the host. Its source must also be allowed by valid_volumes below: # options: --volume /host/toolcache:/opt/hostedtoolcache + # With privileged disabled, options that could escape the container (--security-opt, --device, + # --cap-add, --pid, ...) are ignored in a workflow's container.options, but keep working here. #options: # The parent directory of a job's working directory. # NOTE: There is no need to add the first '/' of the path as runner will add it automatically.