Merge pull request 'fix: filter job container options with an allow list' (#83) from earl-warren/act:wip-pid into main

Reviewed-on: https://code.forgejo.org/forgejo/act/pulls/83
Reviewed-by: Kwonunn <kwonunn@noreply.code.forgejo.org>

5 files changed, 135 insertions, 41 deletions

diff --git a/pkg/container/container_types.go b/pkg/container/container_types.go
index 511db9e..1d0acac 100644
--- a/pkg/container/container_types.go
+++ b/pkg/container/container_types.go
@@ -26,11 +26,13 @@ type NewContainerInput struct {
 	Privileged     bool
 	UsernsMode     string
 	Platform       string
-	Options        string
 	NetworkAliases []string
 	ExposedPorts   nat.PortSet
 	PortBindings   nat.PortMap
 
+	ConfigOptions string
+	JobOptions    string
+
 	// Gitea specific
 	AutoRemove bool
 
diff --git a/pkg/container/docker_run.go b/pkg/container/docker_run.go
index 08bf53d..4f606a2 100644
--- a/pkg/container/docker_run.go
+++ b/pkg/container/docker_run.go
@@ -346,54 +346,42 @@ func (cr *containerReference) remove() common.Executor {
 	}
 }
 
-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
-
-	if input.Options == "" {
+func (cr *containerReference) mergeOptions(ctx context.Context, config *container.Config, hostConfig *container.HostConfig) (*container.Config, *container.HostConfig, error) {
+	if cr.input.ConfigOptions == "" && cr.input.JobOptions == "" {
 		return config, hostConfig, nil
 	}
 
-	// parse configuration from CLI container.options
-	flags := pflag.NewFlagSet("container_flags", pflag.ContinueOnError)
-	copts := addFlags(flags)
+	var err error
 
-	optionsArgs, err := shellquote.Split(input.Options)
-	if err != nil {
-		return nil, nil, fmt.Errorf("Cannot split container options: '%s': '%w'", input.Options, err)
+	if config, hostConfig, err = cr.mergeConfigOptions(ctx, config, hostConfig); err != nil {
+		return nil, nil, err
 	}
 
-	err = flags.Parse(optionsArgs)
-	if err != nil {
-		return nil, nil, fmt.Errorf("Cannot parse container options: '%s': '%w'", input.Options, err)
+	if config, hostConfig, err = cr.mergeJobOptions(ctx, config, hostConfig); err != nil {
+		return nil, nil, err
 	}
 
-	// If a service container's network is set to `host`, the container will not be able to
-	// connect to the specified network created for the job container and the service containers.
-	// So comment out the following code.
+	return config, hostConfig, nil
+}
 
-	// if len(copts.netMode.Value()) == 0 {
-	// 	if err = copts.netMode.Set(cr.input.NetworkMode); err != nil {
-	// 		return nil, nil, fmt.Errorf("Cannot parse networkmode=%s. This is an internal error and should not happen: '%w'", cr.input.NetworkMode, err)
-	// 	}
-	// }
+func (cr *containerReference) mergeConfigOptions(ctx context.Context, config *container.Config, hostConfig *container.HostConfig) (*container.Config, *container.HostConfig, error) {
+	logger := common.Logger(ctx)
+	input := cr.input
 
-	// If the `privileged` config has been disabled, `copts.privileged` need to be forced to false,
-	// even if the user specifies `--privileged` in the options string.
-	if !hostConfig.Privileged {
-		copts.privileged = false
+	containerConfig, err := parseOptions(ctx, input.ConfigOptions)
+	if err != nil {
+		return nil, nil, err
 	}
 
-	containerConfig, err := parse(flags, copts, runtime.GOOS)
-	if err != nil {
-		return nil, nil, fmt.Errorf("Cannot process container options: '%s': '%w'", input.Options, err)
+	if !hostConfig.Privileged {
+		containerConfig.HostConfig.Privileged = false
 	}
 
 	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'", input.ConfigOptions, err)
 	}
 	logger.Debugf("Merged container.Config ==> %+v", config)
 
@@ -406,19 +394,78 @@ 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'", input.ConfigOptions, err)
 	}
 	hostConfig.Binds = binds
 	hostConfig.Mounts = mounts
-	if len(copts.netMode.Value()) > 0 {
-		logger.Warn("--network and --net in the options will be ignored.")
-	}
 	hostConfig.NetworkMode = networkMode
 	logger.Debugf("Merged container.HostConfig ==> %+v", hostConfig)
+	return config, hostConfig, nil
+}
+
+func (cr *containerReference) mergeJobOptions(ctx context.Context, config *container.Config, hostConfig *container.HostConfig) (*container.Config, *container.HostConfig, error) {
+	jobConfig, err := parseOptions(ctx, cr.input.JobOptions)
+	if err != nil {
+		return nil, nil, err
+	}
+
+	logger := common.Logger(ctx)
+
+	if len(jobConfig.Config.Volumes) > 0 {
+		logger.Debugf("--volume options (except bind) %v", jobConfig.Config.Volumes)
+		err = mergo.Merge(&config.Volumes, jobConfig.Config.Volumes, mergo.WithOverride, mergo.WithAppendSlice)
+		if err != nil {
+			return nil, nil, fmt.Errorf("Cannot merge container.Config.Volumes options: '%s': '%w'", cr.input.JobOptions, err)
+		}
+	}
+
+	if len(jobConfig.HostConfig.Binds) > 0 {
+		logger.Debugf("--volume options (only bind) %v", jobConfig.HostConfig.Binds)
+		err = mergo.Merge(&hostConfig.Binds, jobConfig.HostConfig.Binds, mergo.WithOverride, mergo.WithAppendSlice)
+		if err != nil {
+			return nil, nil, fmt.Errorf("Cannot merge hostConfig.Bind options: '%s': '%w'", cr.input.JobOptions, err)
+		}
+	}
+
+	if len(jobConfig.HostConfig.Tmpfs) > 0 {
+		logger.Debugf("--tmpfs options %v", jobConfig.HostConfig.Tmpfs)
+		err = mergo.Merge(&hostConfig.Tmpfs, jobConfig.HostConfig.Tmpfs, mergo.WithOverride, mergo.WithAppendSlice)
+		if err != nil {
+			return nil, nil, fmt.Errorf("Cannot merge Config.Tmpfs options: '%s': '%w'", cr.input.JobOptions, err)
+		}
+	}
 
 	return config, hostConfig, nil
 }
 
+func parseOptions(ctx context.Context, options string) (*containerConfig, error) {
+	logger := common.Logger(ctx)
+
+	flags := pflag.NewFlagSet("container_flags", pflag.ContinueOnError)
+	copts := addFlags(flags)
+
+	if len(copts.netMode.Value()) > 0 {
+		logger.Warn("--network and --net in the options will be ignored.")
+	}
+
+	optionsArgs, err := shellquote.Split(options)
+	if err != nil {
+		return nil, fmt.Errorf("Cannot split container options: '%s': '%w'", options, err)
+	}
+
+	err = flags.Parse(optionsArgs)
+	if err != nil {
+		return nil, fmt.Errorf("Cannot parse container options: '%s': '%w'", options, err)
+	}
+
+	containerConfig, err := parse(flags, copts, runtime.GOOS)
+	if err != nil {
+		return nil, fmt.Errorf("Cannot process container options: '%s': '%w'", options, err)
+	}
+
+	return containerConfig, nil
+}
+
 func (cr *containerReference) create(capAdd []string, capDrop []string) common.Executor {
 	return func(ctx context.Context) error {
 		if cr.id != "" {
@@ -481,7 +528,7 @@ func (cr *containerReference) create(capAdd []string, capDrop []string) common.E
 		}
 		logger.Debugf("Common container.HostConfig ==> %+v", hostConfig)
 
-		config, hostConfig, err := cr.mergeContainerConfigs(ctx, config, hostConfig)
+		config, hostConfig, err := cr.mergeOptions(ctx, config, hostConfig)
 		if err != nil {
 			return err
 		}
diff --git a/pkg/container/docker_run_test.go b/pkg/container/docker_run_test.go
index 6a2c05b..368a562 100644
--- a/pkg/container/docker_run_test.go
+++ b/pkg/container/docker_run_test.go
@@ -17,6 +17,7 @@ import (
 	"github.com/sirupsen/logrus/hooks/test"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/mock"
+	"github.com/stretchr/testify/require"
 )
 
 func TestDocker(t *testing.T) {
@@ -247,3 +248,42 @@ func TestCheckVolumes(t *testing.T) {
 		})
 	}
 }
+
+func TestMergeJobOptions(t *testing.T) {
+	for _, testCase := range []struct {
+		name       string
+		options    string
+		config     *container.Config
+		hostConfig *container.HostConfig
+	}{
+		{
+			name:    "ok",
+			options: "--volume /foo:/bar --volume /frob:/nitz --volume somevolume --tmpfs /tmp:exec,noatime",
+			config: &container.Config{
+				Volumes: map[string]struct{}{"somevolume": {}},
+			},
+			hostConfig: &container.HostConfig{
+				Binds: []string{"/foo:/bar", "/frob:/nitz"},
+				Tmpfs: map[string]string{"/tmp": "exec,noatime"},
+			},
+		},
+		{
+			name:       "ignore",
+			options:    "--pid=host --device=/dev/sda",
+			config:     &container.Config{},
+			hostConfig: &container.HostConfig{},
+		},
+	} {
+		t.Run(testCase.name, func(t *testing.T) {
+			cr := &containerReference{
+				input: &NewContainerInput{
+					JobOptions: testCase.options,
+				},
+			}
+			config, hostConfig, err := cr.mergeJobOptions(context.Background(), &container.Config{}, &container.HostConfig{})
+			require.NoError(t, err)
+			assert.EqualValues(t, testCase.config, config)
+			assert.EqualValues(t, testCase.hostConfig, hostConfig)
+		})
+	}
+}
diff --git a/pkg/runner/action.go b/pkg/runner/action.go
index b26e52a..c417467 100644
--- a/pkg/runner/action.go
+++ b/pkg/runner/action.go
@@ -429,9 +429,10 @@ func newStepContainer(ctx context.Context, step step, image string, cmd []string
 		Privileged:   rc.Config.Privileged,
 		UsernsMode:   rc.Config.UsernsMode,
 		Platform:     rc.Config.ContainerArchitecture,
-		Options:      rc.Config.ContainerOptions,
 		AutoRemove:   rc.Config.AutoRemove,
 		ValidVolumes: rc.Config.ValidVolumes,
+
+		ConfigOptions: rc.Config.ContainerOptions,
 	})
 	return stepContainer
 }
diff --git a/pkg/runner/run_context.go b/pkg/runner/run_context.go
index a833324..82443f8 100644
--- a/pkg/runner/run_context.go
+++ b/pkg/runner/run_context.go
@@ -489,12 +489,14 @@ func (rc *RunContext) startJobContainer() common.Executor {
 				UsernsMode:     rc.Config.UsernsMode,
 				Platform:       rc.Config.ContainerArchitecture,
 				AutoRemove:     rc.Config.AutoRemove,
-				Options:        rc.ExprEval.Interpolate(ctx, spec.Options),
 				NetworkMode:    networkName,
 				NetworkAliases: []string{serviceID},
 				ExposedPorts:   exposedPorts,
 				PortBindings:   portBindings,
 				ValidVolumes:   rc.Config.ValidVolumes,
+
+				JobOptions:    rc.ExprEval.Interpolate(ctx, spec.Options),
+				ConfigOptions: rc.Config.ContainerOptions,
 			})
 			rc.ServiceContainers = append(rc.ServiceContainers, c)
 		}
@@ -549,9 +551,11 @@ func (rc *RunContext) startJobContainer() common.Executor {
 			Privileged:     rc.Config.Privileged,
 			UsernsMode:     rc.Config.UsernsMode,
 			Platform:       rc.Config.ContainerArchitecture,
-			Options:        rc.options(ctx),
 			AutoRemove:     rc.Config.AutoRemove,
 			ValidVolumes:   rc.Config.ValidVolumes,
+
+			JobOptions:    rc.options(ctx),
+			ConfigOptions: "",
 		})
 		if rc.JobContainer == nil {
 			return errors.New("Failed to create job container")
@@ -889,10 +893,10 @@ 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)
+		return rc.ExprEval.Interpolate(ctx, c.Options)
 	}
 
-	return rc.Config.ContainerOptions
+	return ""
 }
 
 func (rc *RunContext) isEnabled(ctx context.Context) (bool, error) {