From 79b0c96334932c9335b6a15cdbd59c2f3a7ce47d Mon Sep 17 00:00:00 2001 From: "Jonathan A. Sternberg" Date: Fri, 26 Apr 2024 15:19:24 -0500 Subject: [PATCH] linter: add lint rule for required json arguments This lint requires json arguments to be used when the `SHELL` command isn't used previously for `ENTRYPOINT` and `CMD`. This is because using non-json arguments can block signals from properly being handled when used in shell mode. This uses the `Shell` attribute on the image configuration and therefore requires the stage to be included in the build for it to properly run. This is done because we don't want to force image config resolution on every stage even if the stage isn't part of the build, but the only way to definitively know if a custom shell is set is to look at the image configuration. Signed-off-by: Jonathan A. Sternberg --- frontend/dockerfile/dockerfile2llb/convert.go | 20 ++++-- frontend/dockerfile/dockerfile_lint_test.go | 69 +++++++++++++++++++ frontend/dockerfile/linter/ruleset.go | 7 ++ 3 files changed, 92 insertions(+), 4 deletions(-) diff --git a/frontend/dockerfile/dockerfile2llb/convert.go b/frontend/dockerfile/dockerfile2llb/convert.go index b40576c67e36..391db439c644 100644 --- a/frontend/dockerfile/dockerfile2llb/convert.go +++ b/frontend/dockerfile/dockerfile2llb/convert.go @@ -611,6 +611,7 @@ func toDispatchState(ctx context.Context, dt []byte, opt ConvertOpt) (*dispatchS cgroupParent: opt.CgroupParent, llbCaps: opt.LLBCaps, sourceMap: opt.SourceMap, + lintWarn: opt.Warn, } if err = dispatchOnBuildTriggers(d, d.image.Config.OnBuild, opt); err != nil { @@ -746,6 +747,7 @@ type dispatchOpt struct { cgroupParent string llbCaps *apicaps.CapSet sourceMap *llb.SourceMap + lintWarn linter.LintWarnFunc } func dispatch(d *dispatchState, cmd command, opt dispatchOpt) error { @@ -820,9 +822,9 @@ func dispatch(d *dispatchState, cmd command, opt dispatchOpt) error { case *instructions.OnbuildCommand: err = dispatchOnbuild(d, c) case *instructions.CmdCommand: - err = dispatchCmd(d, c) + err = dispatchCmd(d, c, opt.lintWarn) case *instructions.EntrypointCommand: - err = dispatchEntrypoint(d, c) + err = dispatchEntrypoint(d, c, opt.lintWarn) case *instructions.HealthCheckCommand: err = dispatchHealthcheck(d, c) case *instructions.ExposeCommand: @@ -1051,6 +1053,8 @@ func dispatchRun(d *dispatchState, c *instructions.RunCommand, proxy *llb.ProxyE } } if c.PrependShell { + // Don't pass the linter function because we do not report a warning for + // shell usage on run commands. args = withShell(d.image, args) } @@ -1435,9 +1439,13 @@ func dispatchOnbuild(d *dispatchState, c *instructions.OnbuildCommand) error { return nil } -func dispatchCmd(d *dispatchState, c *instructions.CmdCommand) error { +func dispatchCmd(d *dispatchState, c *instructions.CmdCommand, warn linter.LintWarnFunc) error { var args []string = c.CmdLine if c.PrependShell { + if len(d.image.Config.Shell) == 0 { + msg := linter.RuleJSONArgsRecommended.Format(c.Name()) + linter.RuleJSONArgsRecommended.Run(warn, c.Location(), msg) + } args = withShell(d.image, args) } d.image.Config.Cmd = args @@ -1446,9 +1454,13 @@ func dispatchCmd(d *dispatchState, c *instructions.CmdCommand) error { return commitToHistory(&d.image, fmt.Sprintf("CMD %q", args), false, nil, d.epoch) } -func dispatchEntrypoint(d *dispatchState, c *instructions.EntrypointCommand) error { +func dispatchEntrypoint(d *dispatchState, c *instructions.EntrypointCommand, warn linter.LintWarnFunc) error { var args []string = c.CmdLine if c.PrependShell { + if len(d.image.Config.Shell) == 0 { + msg := linter.RuleJSONArgsRecommended.Format(c.Name()) + linter.RuleJSONArgsRecommended.Run(warn, c.Location(), msg) + } args = withShell(d.image, args) } d.image.Config.Entrypoint = args diff --git a/frontend/dockerfile/dockerfile_lint_test.go b/frontend/dockerfile/dockerfile_lint_test.go index 34e2637436ec..92b756c157af 100644 --- a/frontend/dockerfile/dockerfile_lint_test.go +++ b/frontend/dockerfile/dockerfile_lint_test.go @@ -28,6 +28,7 @@ var lintTests = integration.TestFuncs( testFileConsistentCommandCasing, testDuplicateStageName, testReservedStageName, + testJSONArgsRecommended, testMaintainerDeprecated, testWarningsBeforeError, testUndeclaredArg, @@ -302,6 +303,74 @@ FROM scratch AS a checkLinterWarnings(t, sb, &lintTestParams{Dockerfile: dockerfile}) } +func testJSONArgsRecommended(t *testing.T, sb integration.Sandbox) { + dockerfile := []byte(` +FROM scratch +CMD mycommand +`) + checkLinterWarnings(t, sb, &lintTestParams{ + Dockerfile: dockerfile, + Warnings: []expectedLintWarning{ + { + RuleName: "JSONArgsRecommended", + Description: "JSON arguments recommended for ENTRYPOINT/CMD to prevent unintended behavior related to OS signals", + Detail: "JSON arguments recommended for CMD to prevent unintended behavior related to OS signals", + Level: 1, + Line: 3, + }, + }, + }) + + dockerfile = []byte(` +FROM scratch +ENTRYPOINT mycommand +`) + checkLinterWarnings(t, sb, &lintTestParams{ + Dockerfile: dockerfile, + Warnings: []expectedLintWarning{ + { + RuleName: "JSONArgsRecommended", + Description: "JSON arguments recommended for ENTRYPOINT/CMD to prevent unintended behavior related to OS signals", + Detail: "JSON arguments recommended for ENTRYPOINT to prevent unintended behavior related to OS signals", + Level: 1, + Line: 3, + }, + }, + }) + + dockerfile = []byte(` +FROM scratch +SHELL ["/usr/bin/customshell"] +CMD mycommand +`) + checkLinterWarnings(t, sb, &lintTestParams{Dockerfile: dockerfile}) + + dockerfile = []byte(` +FROM scratch +SHELL ["/usr/bin/customshell"] +ENTRYPOINT mycommand +`) + checkLinterWarnings(t, sb, &lintTestParams{Dockerfile: dockerfile}) + + dockerfile = []byte(` +FROM scratch AS base +SHELL ["/usr/bin/customshell"] + +FROM base +CMD mycommand +`) + checkLinterWarnings(t, sb, &lintTestParams{Dockerfile: dockerfile}) + + dockerfile = []byte(` +FROM scratch AS base +SHELL ["/usr/bin/customshell"] + +FROM base +ENTRYPOINT mycommand +`) + checkLinterWarnings(t, sb, &lintTestParams{Dockerfile: dockerfile}) +} + func testMaintainerDeprecated(t *testing.T, sb integration.Sandbox) { dockerfile := []byte(` FROM scratch diff --git a/frontend/dockerfile/linter/ruleset.go b/frontend/dockerfile/linter/ruleset.go index f21e62a85520..d1f4b8af0cf1 100644 --- a/frontend/dockerfile/linter/ruleset.go +++ b/frontend/dockerfile/linter/ruleset.go @@ -55,6 +55,13 @@ var ( return fmt.Sprintf("Stage name should not use the same name as reserved stage %q", reservedStageName) }, } + RuleJSONArgsRecommended = LinterRule[func(instructionName string) string]{ + Name: "JSONArgsRecommended", + Description: "JSON arguments recommended for ENTRYPOINT/CMD to prevent unintended behavior related to OS signals", + Format: func(instructionName string) string { + return fmt.Sprintf("JSON arguments recommended for %s to prevent unintended behavior related to OS signals", instructionName) + }, + } RuleMaintainerDeprecated = LinterRule[func() string]{ Name: "MaintainerDeprecated", Description: "The maintainer instruction is deprecated, use a label instead to define an image author",