diff --git a/act/runner/action.go b/act/runner/action.go index 1e144462..61793d9e 100644 --- a/act/runner/action.go +++ b/act/runner/action.go @@ -436,13 +436,11 @@ func newStepContainer(ctx context.Context, step step, image string, cmd, entrypo if rc.IsHostEnv(ctx) { networkMode = "default" } - stepContainer := container.NewContainer(&container.NewContainerInput{ + stepContainer := ContainerNewContainer(&container.NewContainerInput{ Cmd: cmd, Entrypoint: entrypoint, WorkingDir: rc.JobContainer.ToContainerPath(rc.Config.Workdir), Image: image, - Username: rc.Config.Secrets["DOCKER_USERNAME"], - Password: rc.Config.Secrets["DOCKER_PASSWORD"], Name: createContainerName(rc.jobContainerName(), "STEP-"+stepModel.ID), Env: envList, Mounts: mounts, diff --git a/act/runner/action_test.go b/act/runner/action_test.go index 3814c6eb..c5dc2360 100644 --- a/act/runner/action_test.go +++ b/act/runner/action_test.go @@ -258,6 +258,54 @@ func TestActionRunner(t *testing.T) { } } +func TestNewStepContainerDoesNotUseDockerSecrets(t *testing.T) { + cm := &containerMock{} + + var captured *container.NewContainerInput + origContainerNewContainer := ContainerNewContainer + ContainerNewContainer = func(input *container.NewContainerInput) container.ExecutionsEnvironment { + captured = input + return cm + } + defer func() { + ContainerNewContainer = origContainerNewContainer + }() + + ctx := context.Background() + rc := &RunContext{ + Name: "job", + Config: &Config{ + Secrets: map[string]string{ + "DOCKER_USERNAME": "docker-user", + "DOCKER_PASSWORD": "docker-password", + }, + }, + Run: &model.Run{ + JobID: "job", + Workflow: &model.Workflow{ + Name: "test", + Jobs: map[string]*model.Job{ + "job": {}, + }, + }, + }, + JobContainer: cm, + StepResults: map[string]*model.StepResult{}, + } + env := map[string]string{} + step := &stepMock{} + step.On("getRunContext").Return(rc) + step.On("getStepModel").Return(&model.Step{ID: "action"}) + step.On("getEnv").Return(&env) + + _ = newStepContainer(ctx, step, "registry.example.com/action:tag", nil, nil) + + // DOCKER_USERNAME/DOCKER_PASSWORD should not be injected as pull credentials for docker action containers. + assert.Empty(t, captured.Username) + assert.Empty(t, captured.Password) + step.AssertExpectations(t) +} + func TestMaybeCopyToActionDirHoldsCloneLock(t *testing.T) { ctx := context.Background() diff --git a/act/runner/run_context.go b/act/runner/run_context.go index 0ff660bd..de85ebce 100644 --- a/act/runner/run_context.go +++ b/act/runner/run_context.go @@ -1175,21 +1175,18 @@ func setActionRuntimeVars(rc *RunContext, env map[string]string) { } func (rc *RunContext) handleCredentials(ctx context.Context) (string, string, error) { - // TODO: remove below 2 lines when we can release act with breaking changes - username := rc.Config.Secrets["DOCKER_USERNAME"] - password := rc.Config.Secrets["DOCKER_PASSWORD"] - container := rc.Run.Job().Container() if container == nil || container.Credentials == nil { - return username, password, nil + return "", "", nil } - if container.Credentials != nil && len(container.Credentials) != 2 { + if len(container.Credentials) != 2 { err := errors.New("invalid property count for key 'credentials:'") return "", "", err } ee := rc.NewExpressionEvaluator(ctx) + var username, password string if username = ee.Interpolate(ctx, container.Credentials["username"]); username == "" { err := errors.New("failed to interpolate container.credentials.username") return "", "", err diff --git a/act/runner/run_context_test.go b/act/runner/run_context_test.go index 56c14bd3..40f4d73c 100644 --- a/act/runner/run_context_test.go +++ b/act/runner/run_context_test.go @@ -170,6 +170,38 @@ func TestRunContext_EvalBool(t *testing.T) { } } +func TestRunContextHandleCredentialsDoesNotUseDockerSecrets(t *testing.T) { + workflow, err := model.ReadWorkflow(strings.NewReader(` +name: test +on: push +jobs: + job: + runs-on: ubuntu-latest + steps: [] +`)) + require.NoError(t, err) + + rc := &RunContext{ + Config: &Config{ + Secrets: map[string]string{ + "DOCKER_USERNAME": "docker-user", + "DOCKER_PASSWORD": "docker-password", + }, + Env: map[string]string{}, + }, + Run: &model.Run{ + JobID: "job", + Workflow: workflow, + }, + } + + // DOCKER_USERNAME/DOCKER_PASSWORD secrets should not be used as implicit job container pull credentials. + username, password, err := rc.handleCredentials(t.Context()) + require.NoError(t, err) + assert.Empty(t, username) + assert.Empty(t, password) +} + func TestRunContext_GetBindsAndMounts(t *testing.T) { rctemplate := &RunContext{ Name: "TestRCName", diff --git a/act/runner/step_docker.go b/act/runner/step_docker.go index 28c3bc10..9d2a85a1 100644 --- a/act/runner/step_docker.go +++ b/act/runner/step_docker.go @@ -125,8 +125,6 @@ func (sd *stepDocker) newStepContainer(ctx context.Context, image string, cmd, e Entrypoint: entrypoint, WorkingDir: rc.JobContainer.ToContainerPath(rc.Config.Workdir), Image: image, - Username: rc.Config.Secrets["DOCKER_USERNAME"], - Password: rc.Config.Secrets["DOCKER_PASSWORD"], Name: createContainerName(rc.jobContainerName(), "STEP-"+step.ID), Env: envList, Mounts: mounts, diff --git a/act/runner/step_docker_test.go b/act/runner/step_docker_test.go index 230fa6b4..da508d1f 100644 --- a/act/runner/step_docker_test.go +++ b/act/runner/step_docker_test.go @@ -38,7 +38,12 @@ func TestStepDockerMain(t *testing.T) { sd := &stepDocker{ RunContext: &RunContext{ StepResults: map[string]*model.StepResult{}, - Config: &Config{}, + Config: &Config{ + Secrets: map[string]string{ + "DOCKER_USERNAME": "docker-user", + "DOCKER_PASSWORD": "docker-password", + }, + }, Run: &model.Run{ JobID: "1", Workflow: &model.Workflow{ @@ -106,6 +111,10 @@ func TestStepDockerMain(t *testing.T) { assert.Equal(t, "node:14", input.Image) + // DOCKER_USERNAME/DOCKER_PASSWORD secrets should not be used as implicit pull credentials for docker:// action containers. + assert.Empty(t, input.Username) + assert.Empty(t, input.Password) + cm.AssertExpectations(t) }