diff --git a/command/compile.go b/command/compile.go index 7993df9..adbafbb 100644 --- a/command/compile.go +++ b/command/compile.go @@ -13,6 +13,7 @@ import ( "github.com/drone-runners/drone-runner-docker/command/internal" "github.com/drone-runners/drone-runner-docker/engine/compiler" + "github.com/drone-runners/drone-runner-docker/engine/linter" "github.com/drone-runners/drone-runner-docker/engine/resource" "github.com/drone/envsubst" "github.com/drone/runner-go/environ" @@ -77,6 +78,15 @@ func (c *compileCommand) run(*kingpin.ParseContext) error { return err } + // lint the pipeline and return an error if any + // linting rules are broken + lint := linter.New() + opts := linter.Opts{Trusted: c.Repo.Trusted} + err = lint.Lint(resource, opts) + if err != nil { + return err + } + // compile the pipeline to an intermediate representation. comp := &compiler.Compiler{ Pipeline: resource, diff --git a/command/daemon/daemon.go b/command/daemon/daemon.go index 2f33279..5e64869 100644 --- a/command/daemon/daemon.go +++ b/command/daemon/daemon.go @@ -9,6 +9,7 @@ import ( "time" "github.com/drone-runners/drone-runner-docker/engine" + "github.com/drone-runners/drone-runner-docker/engine/linter" "github.com/drone-runners/drone-runner-docker/engine/resource" "github.com/drone-runners/drone-runner-docker/internal/match" "github.com/drone-runners/drone-runner-docker/runtime" @@ -82,6 +83,7 @@ func (c *daemonCommand) run(*kingpin.ParseContext) error { Environ: config.Runner.Environ, Machine: config.Runner.Name, Reporter: tracer, + Linter: linter.New(), Match: match.Func( config.Limit.Repos, config.Limit.Events, diff --git a/command/exec.go b/command/exec.go index 724961d..192f818 100644 --- a/command/exec.go +++ b/command/exec.go @@ -16,6 +16,7 @@ import ( "github.com/drone-runners/drone-runner-docker/command/internal" "github.com/drone-runners/drone-runner-docker/engine" "github.com/drone-runners/drone-runner-docker/engine/compiler" + "github.com/drone-runners/drone-runner-docker/engine/linter" "github.com/drone-runners/drone-runner-docker/engine/resource" "github.com/drone-runners/drone-runner-docker/runtime" "github.com/drone/drone-go/drone" @@ -95,6 +96,15 @@ func (c *execCommand) run(*kingpin.ParseContext) error { return err } + // lint the pipeline and return an error if any + // linting rules are broken + lint := linter.New() + opts := linter.Opts{Trusted: c.Repo.Trusted} + err = lint.Lint(resource, opts) + if err != nil { + return err + } + // compile the pipeline to an intermediate representation. comp := &compiler.Compiler{ Pipeline: resource, diff --git a/engine/linter/linter.go b/engine/linter/linter.go index 98e2e37..36884da 100644 --- a/engine/linter/linter.go +++ b/engine/linter/linter.go @@ -34,6 +34,11 @@ type Opts struct { // rules are broken. type Linter struct{} +// New returns a new Linter. +func New() *Linter { + return new(Linter) +} + // Lint executes the linting rules for the pipeline // configuration. func (l *Linter) Lint(pipeline *resource.Pipeline, opts Opts) error { @@ -41,9 +46,9 @@ func (l *Linter) Lint(pipeline *resource.Pipeline, opts Opts) error { } func checkPipeline(pipeline *resource.Pipeline, trusted bool) error { - if err := checkNames(pipeline); err != nil { - return err - } + // if err := checkNames(pipeline); err != nil { + // return err + // } if err := checkSteps(pipeline, trusted); err != nil { return err } @@ -53,21 +58,21 @@ func checkPipeline(pipeline *resource.Pipeline, trusted bool) error { return nil } -func checkNames(pipeline *resource.Pipeline) error { - names := map[string]struct{}{} - if !pipeline.Clone.Disable { - names["clone"] = struct{}{} - } - steps := append(pipeline.Services, pipeline.Steps...) - for _, step := range steps { - _, ok := names[step.Name] - if ok { - return ErrDuplicateStepName - } - names[step.Name] = struct{}{} - } - return nil -} +// func checkNames(pipeline *resource.Pipeline) error { +// names := map[string]struct{}{} +// if !pipeline.Clone.Disable { +// names["clone"] = struct{}{} +// } +// steps := append(pipeline.Services, pipeline.Steps...) +// for _, step := range steps { +// _, ok := names[step.Name] +// if ok { +// return ErrDuplicateStepName +// } +// names[step.Name] = struct{}{} +// } +// return nil +// } func checkSteps(pipeline *resource.Pipeline, trusted bool) error { steps := append(pipeline.Services, pipeline.Steps...) @@ -83,12 +88,12 @@ func checkStep(step *resource.Step, trusted bool) error { if step.Image == "" { return errors.New("linter: invalid or missing image") } - if step.Name == "" { - return errors.New("linter: invalid or missing name") - } - if len(step.Name) > 100 { - return errors.New("linter: name exceeds maximum length") - } + // if step.Name == "" { + // return errors.New("linter: invalid or missing name") + // } + // if len(step.Name) > 100 { + // return errors.New("linter: name exceeds maximum length") + // } if trusted == false && step.Privileged { return errors.New("linter: untrusted repositories cannot enable privileged mode") } diff --git a/engine/linter/linter_test.go b/engine/linter/linter_test.go index 921f13a..f5719f6 100644 --- a/engine/linter/linter_test.go +++ b/engine/linter/linter_test.go @@ -3,3 +3,227 @@ // that can be found in the LICENSE file. package linter + +import ( + "path" + "testing" + + "github.com/drone-runners/drone-runner-docker/engine/resource" + "github.com/drone/runner-go/manifest" +) + +func TestLint(t *testing.T) { + tests := []struct { + path string + trusted bool + invalid bool + message string + }{ + { + path: "testdata/simple.yml", + trusted: false, + invalid: false, + }, + { + path: "testdata/missing_image.yml", + invalid: true, + message: "linter: invalid or missing image", + }, + // user should not use reserved volume names. + { + path: "testdata/volume_invalid_name.yml", + trusted: false, + invalid: true, + message: "linter: invalid volume name: _workspace", + }, + { + path: "testdata/pipeline_volume_invalid_name.yml", + trusted: false, + invalid: true, + message: "linter: invalid volume name: _docker_socket", + }, + // user should not be able to mount host path + // volumes unless the repository is trusted. + { + path: "testdata/volume_host_path.yml", + trusted: false, + invalid: true, + message: "linter: untrusted repositories cannot mount host volumes", + }, + { + path: "testdata/volume_host_path.yml", + trusted: true, + invalid: false, + }, + // user should be able to mount emptyDir volumes + // where no medium is specified. + { + path: "testdata/volume_empty_dir.yml", + trusted: false, + invalid: false, + }, + // user should not be able to mount in-memory + // emptyDir volumes unless the repository is + // trusted. + { + path: "testdata/volume_empty_dir_memory.yml", + trusted: false, + invalid: true, + message: "linter: untrusted repositories cannot mount in-memory volumes", + }, + { + path: "testdata/volume_empty_dir_memory.yml", + trusted: true, + invalid: false, + }, + // user should not be able to mount devices unless + // the repository is trusted. + { + path: "testdata/service_device.yml", + trusted: false, + invalid: true, + message: "linter: untrusted repositories cannot mount devices", + }, + { + path: "testdata/service_device.yml", + trusted: true, + invalid: false, + }, + { + path: "testdata/pipeline_device.yml", + trusted: false, + invalid: true, + message: "linter: untrusted repositories cannot mount devices", + }, + { + path: "testdata/pipeline_device.yml", + trusted: true, + invalid: false, + }, + // user should not be able to set the securityContext + // unless the repository is trusted. + { + path: "testdata/pipeline_privileged.yml", + trusted: false, + invalid: true, + message: "linter: untrusted repositories cannot enable privileged mode", + }, + { + path: "testdata/pipeline_privileged.yml", + trusted: true, + invalid: false, + }, + // user should not be able to set dns, dns_search or + // extra_hosts unless the repository is trusted. + { + path: "testdata/pipeline_dns.yml", + trusted: false, + invalid: true, + message: "linter: untrusted repositories cannot configure dns", + }, + { + path: "testdata/pipeline_dns.yml", + trusted: true, + invalid: false, + }, + { + path: "testdata/pipeline_dns_search.yml", + trusted: false, + invalid: true, + message: "linter: untrusted repositories cannot configure dns_search", + }, + { + path: "testdata/pipeline_dns_search.yml", + trusted: true, + invalid: false, + }, + { + path: "testdata/pipeline_extra_hosts.yml", + trusted: false, + invalid: true, + message: "linter: untrusted repositories cannot configure extra_hosts", + }, + { + path: "testdata/pipeline_extra_hosts.yml", + trusted: true, + invalid: false, + }, + { + path: "testdata/pipeline_network_mode.yml", + trusted: false, + invalid: true, + message: "linter: untrusted repositories cannot configure network_mode", + }, + { + path: "testdata/pipeline_network_mode.yml", + trusted: true, + invalid: false, + }, + + // + // The below checks were moved to the parser, however, we + // should decide where we want this logic to live. + // + + // // user should not be able to use duplicate names + // // for steps or services. + // { + // path: "testdata/duplicate_step.yml", + // invalid: true, + // message: "linter: duplicate step names", + // }, + // { + // path: "testdata/duplicate_step_service.yml", + // invalid: true, + // message: "linter: duplicate step names", + // }, + // { + // path: "testdata/missing_name.yml", + // invalid: true, + // message: "linter: invalid or missing name", + // }, + } + for _, test := range tests { + name := path.Base(test.path) + if test.trusted { + name = name + "/trusted" + } + t.Run(name, func(t *testing.T) { + resources, err := manifest.ParseFile(test.path) + if err != nil { + t.Logf("yaml: %s", test.path) + t.Logf("trusted: %v", test.trusted) + t.Error(err) + return + } + + lint := New() + opts := Opts{Trusted: test.trusted} + err = lint.Lint(resources.Resources[0].(*resource.Pipeline), opts) + if err == nil && test.invalid == true { + t.Logf("yaml: %s", test.path) + t.Logf("trusted: %v", test.trusted) + t.Errorf("Expect lint error") + return + } + + if err != nil && test.invalid == false { + t.Logf("yaml: %s", test.path) + t.Logf("trusted: %v", test.trusted) + t.Errorf("Expect lint error is nil, got %s", err) + return + } + + if err == nil { + return + } + + if got, want := err.Error(), test.message; got != want { + t.Logf("yaml: %s", test.path) + t.Logf("trusted: %v", test.trusted) + t.Errorf("Want message %q, got %q", want, got) + return + } + }) + } +} diff --git a/engine/linter/testdata/duplicate_name.yml b/engine/linter/testdata/duplicate_name.yml new file mode 100644 index 0000000..26f1439 --- /dev/null +++ b/engine/linter/testdata/duplicate_name.yml @@ -0,0 +1,24 @@ +--- +kind: pipeline +type: docker +name: default + +steps: +- name: build + image: golang + commands: + - go build + - go test + +--- +kind: pipeline +name: default + +steps: +- name: build + image: golang + commands: + - go build + - go test + +... \ No newline at end of file diff --git a/engine/linter/testdata/duplicate_step.yml b/engine/linter/testdata/duplicate_step.yml new file mode 100644 index 0000000..1f32261 --- /dev/null +++ b/engine/linter/testdata/duplicate_step.yml @@ -0,0 +1,17 @@ +--- +kind: pipeline +type: docker +name: default + +steps: +- name: build + image: golang + commands: + - go build + - go test + +- name: build + image: golang + commands: + - go build + - go test \ No newline at end of file diff --git a/engine/linter/testdata/duplicate_step_service.yml b/engine/linter/testdata/duplicate_step_service.yml new file mode 100644 index 0000000..12b8557 --- /dev/null +++ b/engine/linter/testdata/duplicate_step_service.yml @@ -0,0 +1,15 @@ +--- +kind: pipeline +type: docker +name: default + +steps: +- name: test + image: golang + commands: + - go build + - go test + +services: +- name: test + image: redis diff --git a/engine/linter/testdata/invalid_arch.yml b/engine/linter/testdata/invalid_arch.yml new file mode 100644 index 0000000..de5c9b9 --- /dev/null +++ b/engine/linter/testdata/invalid_arch.yml @@ -0,0 +1,15 @@ +--- +kind: pipeline +type: docker +name: linux + +platform: + os: linux + arch: s390x + +steps: +- name: build + image: golang + commands: + - go build + - go test diff --git a/engine/linter/testdata/invalid_os.yml b/engine/linter/testdata/invalid_os.yml new file mode 100644 index 0000000..29cd734 --- /dev/null +++ b/engine/linter/testdata/invalid_os.yml @@ -0,0 +1,14 @@ +--- +kind: pipeline +type: docker +name: linux + +platform: + os: openbsd + +steps: +- name: build + image: golang + commands: + - go build + - go test diff --git a/engine/linter/testdata/missing_build_image.yml b/engine/linter/testdata/missing_build_image.yml new file mode 100644 index 0000000..bf7c849 --- /dev/null +++ b/engine/linter/testdata/missing_build_image.yml @@ -0,0 +1,9 @@ +--- +kind: pipeline +type: docker +name: linux + +steps: +- name: test + build: {} + diff --git a/engine/linter/testdata/missing_dep.yml b/engine/linter/testdata/missing_dep.yml new file mode 100644 index 0000000..9088b62 --- /dev/null +++ b/engine/linter/testdata/missing_dep.yml @@ -0,0 +1,35 @@ +--- +kind: pipeline +type: docker +name: amd64 + +steps: +- name: test + image: golang + commands: + - go build + - go test + +services: +- name: database + image: redis + ports: + - 6379 + +--- +kind: pipeline +name: arm + +platform: + arch: arm + +steps: +- name: test + image: golang + commands: + - go build + - go test + +depends_on: +- foo +... \ No newline at end of file diff --git a/engine/linter/testdata/missing_image.yml b/engine/linter/testdata/missing_image.yml new file mode 100644 index 0000000..1d6c582 --- /dev/null +++ b/engine/linter/testdata/missing_image.yml @@ -0,0 +1,10 @@ +--- +kind: pipeline +type: docker +name: linux + +steps: +- name: test + commands: + - go build + - go test diff --git a/engine/linter/testdata/missing_name.yml b/engine/linter/testdata/missing_name.yml new file mode 100644 index 0000000..b3aee41 --- /dev/null +++ b/engine/linter/testdata/missing_name.yml @@ -0,0 +1,10 @@ +--- +kind: pipeline +type: docker +name: linux + +steps: +- image: golang + commands: + - go build + - go test diff --git a/engine/linter/testdata/pipeline_device.yml b/engine/linter/testdata/pipeline_device.yml new file mode 100644 index 0000000..093376f --- /dev/null +++ b/engine/linter/testdata/pipeline_device.yml @@ -0,0 +1,20 @@ +--- +kind: pipeline +type: docker +name: linux + +steps: +- name: test + image: golang + commands: + - go build + - go test + devices: + - name: data + path: /dev/xvda + +services: +- name: database + image: redis + ports: + - 6379 diff --git a/engine/linter/testdata/pipeline_dns.yml b/engine/linter/testdata/pipeline_dns.yml new file mode 100644 index 0000000..bc0a73e --- /dev/null +++ b/engine/linter/testdata/pipeline_dns.yml @@ -0,0 +1,13 @@ +--- +kind: pipeline +type: docker +name: linux + +steps: +- name: test + image: golang + commands: + - go build + - go test + dns: + - 8.8.8.8 diff --git a/engine/linter/testdata/pipeline_dns_search.yml b/engine/linter/testdata/pipeline_dns_search.yml new file mode 100644 index 0000000..3889b64 --- /dev/null +++ b/engine/linter/testdata/pipeline_dns_search.yml @@ -0,0 +1,14 @@ +--- +kind: pipeline +type: docker +name: linux + +steps: +- name: test + image: golang + commands: + - go build + - go test + dns_search: + - dc1.example.com + - dc2.example.com diff --git a/engine/linter/testdata/pipeline_extra_hosts.yml b/engine/linter/testdata/pipeline_extra_hosts.yml new file mode 100644 index 0000000..700b89e --- /dev/null +++ b/engine/linter/testdata/pipeline_extra_hosts.yml @@ -0,0 +1,14 @@ +--- +kind: pipeline +type: docker +name: linux + +steps: +- name: test + image: golang + commands: + - go build + - go test + extra_hosts: + - "somehost:162.242.195.82" + - "otherhost:50.31.209.229" diff --git a/engine/linter/testdata/pipeline_network_mode.yml b/engine/linter/testdata/pipeline_network_mode.yml new file mode 100644 index 0000000..b58d35c --- /dev/null +++ b/engine/linter/testdata/pipeline_network_mode.yml @@ -0,0 +1,12 @@ +--- +kind: pipeline +type: docker +name: linux + +steps: +- name: test + image: golang + commands: + - go build + - go test + network_mode: host diff --git a/engine/linter/testdata/pipeline_port_host.yml b/engine/linter/testdata/pipeline_port_host.yml new file mode 100644 index 0000000..5e01c95 --- /dev/null +++ b/engine/linter/testdata/pipeline_port_host.yml @@ -0,0 +1,18 @@ +--- +kind: pipeline +type: docker +name: linux + +steps: +- name: database + image: redis + detach: true + ports: + - port: 6379 + host: 6379 + +- name: test + image: golang + commands: + - go build + - go test diff --git a/engine/linter/testdata/pipeline_privileged.yml b/engine/linter/testdata/pipeline_privileged.yml new file mode 100644 index 0000000..f1862ba --- /dev/null +++ b/engine/linter/testdata/pipeline_privileged.yml @@ -0,0 +1,18 @@ +--- +kind: pipeline +type: docker +name: linux + +steps: +- name: test + image: golang + commands: + - go build + - go test + privileged: true + +services: +- name: database + image: redis + ports: + - 6379 diff --git a/engine/linter/testdata/pipeline_volume_invalid_name.yml b/engine/linter/testdata/pipeline_volume_invalid_name.yml new file mode 100644 index 0000000..7eaa410 --- /dev/null +++ b/engine/linter/testdata/pipeline_volume_invalid_name.yml @@ -0,0 +1,14 @@ +--- +kind: pipeline +type: docker +name: linux + +steps: +- name: test + image: docker + volumes: + - name: _docker_socket + path: /var/run/docker.sock + commands: + - docker system prune + diff --git a/engine/linter/testdata/service_device.yml b/engine/linter/testdata/service_device.yml new file mode 100644 index 0000000..469857d --- /dev/null +++ b/engine/linter/testdata/service_device.yml @@ -0,0 +1,20 @@ +--- +kind: pipeline +type: docker +name: linux + +steps: +- name: test + image: golang + commands: + - go build + - go test + +services: +- name: database + image: redis + ports: + - 6379 + devices: + - name: data + path: /dev/xvda diff --git a/engine/linter/testdata/service_port_host.yml b/engine/linter/testdata/service_port_host.yml new file mode 100644 index 0000000..51c1dd6 --- /dev/null +++ b/engine/linter/testdata/service_port_host.yml @@ -0,0 +1,18 @@ +--- +kind: pipeline +type: docker +name: linux + +steps: +- name: test + image: golang + commands: + - go build + - go test + +services: +- name: database + image: redis + ports: + - port: 6379 + host: 6379 diff --git a/engine/linter/testdata/simple.yml b/engine/linter/testdata/simple.yml new file mode 100644 index 0000000..f9eae38 --- /dev/null +++ b/engine/linter/testdata/simple.yml @@ -0,0 +1,39 @@ +--- +kind: pipeline +type: docker +name: amd64 + +steps: +- name: build + image: golang + commands: + - go build + +- name: test + image: golang + commands: + - go test + +services: +- name: database + image: redis + ports: + - 6379 + +--- +kind: pipeline +name: arm + +platform: + arch: arm + +steps: +- name: test + image: golang + commands: + - go build + - go test + +depends_on: +- amd64 +... \ No newline at end of file diff --git a/engine/linter/testdata/volume_empty_dir.yml b/engine/linter/testdata/volume_empty_dir.yml new file mode 100644 index 0000000..68a5994 --- /dev/null +++ b/engine/linter/testdata/volume_empty_dir.yml @@ -0,0 +1,21 @@ +--- +kind: pipeline +type: docker +name: linux + +steps: +- name: test + image: golang + commands: + - go build + - go test + +services: +- name: database + image: redis + ports: + - 6379 + +volumes: +- name: vol + temp: {} diff --git a/engine/linter/testdata/volume_empty_dir_memory.yml b/engine/linter/testdata/volume_empty_dir_memory.yml new file mode 100644 index 0000000..d80c266 --- /dev/null +++ b/engine/linter/testdata/volume_empty_dir_memory.yml @@ -0,0 +1,22 @@ +--- +kind: pipeline +type: docker +name: linux + +steps: +- name: test + image: golang + commands: + - go build + - go test + +services: +- name: database + image: redis + ports: + - 6379 + +volumes: +- name: vol + temp: + medium: memory diff --git a/engine/linter/testdata/volume_host_path.yml b/engine/linter/testdata/volume_host_path.yml new file mode 100644 index 0000000..56f0a3f --- /dev/null +++ b/engine/linter/testdata/volume_host_path.yml @@ -0,0 +1,22 @@ +--- +kind: pipeline +type: docker +name: linux + +steps: +- name: test + image: golang + commands: + - go build + - go test + +services: +- name: database + image: redis + ports: + - 6379 + +volumes: +- name: vol + host: + path: /any/path/it/will/be/replaced diff --git a/engine/linter/testdata/volume_invalid_name.yml b/engine/linter/testdata/volume_invalid_name.yml new file mode 100644 index 0000000..d88daad --- /dev/null +++ b/engine/linter/testdata/volume_invalid_name.yml @@ -0,0 +1,21 @@ +--- +kind: pipeline +type: docker +name: linux + +steps: +- name: test + image: golang + commands: + - go build + - go test + +services: +- name: database + image: redis + ports: + - 6379 + +volumes: +- name: _workspace + temp: {} diff --git a/runtime/runner.go b/runtime/runner.go index abab7ab..b434b35 100644 --- a/runtime/runner.go +++ b/runtime/runner.go @@ -15,6 +15,7 @@ import ( "github.com/drone-runners/drone-runner-docker/engine" "github.com/drone-runners/drone-runner-docker/engine/compiler" + "github.com/drone-runners/drone-runner-docker/engine/linter" "github.com/drone-runners/drone-runner-docker/engine/resource" "github.com/drone/drone-go/drone" @@ -37,6 +38,10 @@ type Runner struct { // representation of the pipeline and returns its results. Execer Execer + // Linter is responsible for linting the pipeline + // and failing if any rules are broken. + Linter *linter.Linter + // Reporter reports pipeline status back to the remote // server. Reporter pipeline.Reporter @@ -180,6 +185,15 @@ func (s *Runner) Run(ctx context.Context, stage *drone.Stage) error { return s.Reporter.ReportStage(noContext, state) } + // lint the pipeline configuration and fail the build + // if any linting rules are broken. + err = s.Linter.Lint(resource, linter.Opts{Trusted: data.Repo.Trusted}) + if err != nil { + log.WithError(err).Error("cannot accept configuration") + state.FailAll(err) + return s.Reporter.ReportStage(noContext, state) + } + secrets := secret.Combine( secret.Static(data.Secrets), secret.Encrypted(),