diff --git a/dockerargs/dockerargs.go b/dockerargs/dockerargs.go index 33e0687..974891f 100644 --- a/dockerargs/dockerargs.go +++ b/dockerargs/dockerargs.go @@ -2,8 +2,8 @@ // devcontainer tooling, gives meaning to: // // - "runArgs", which becomes the argv of the "docker run" command the tooling builds. [Parse] is -// the single place that knows where a flag's value can be written, so a rule only has to know -// the values it cares about and never which entry of the array holds one. +// the single place that knows where a flag's value can be written, so its callers only have to +// know the values they care about and never which entry of the array holds one. // - the values themselves, whose syntax is Docker's wherever they are written: a "securityOpt" // entry ([ParseSecurityOpt]), a capability name ([Capability]), a boolean ([IsTrue]). package dockerargs diff --git a/linter/linter.go b/linter/linter.go index a30aa63..4bc8582 100644 --- a/linter/linter.go +++ b/linter/linter.go @@ -69,7 +69,7 @@ func (l *Linter) LintDocument(path string, fileType FileType, doc *Document, dir rctx := &Context{Path: path, Type: fileType, Root: doc.tree, Dir: dir} var issues []Issue seen := map[Issue]struct{}{} - walk(doc.tree, "", nil, patterns, func(r *Rule, node *Node) { + walk(doc.tree, fileType, patterns, func(r *Rule, node *Node) { id := r.ID severity := l.severities[id] for _, f := range safeCheck(r, rctx, node) { diff --git a/linter/linter_test.go b/linter/linter_test.go index 3fb9d38..a1ec1fd 100644 --- a/linter/linter_test.go +++ b/linter/linter_test.go @@ -429,3 +429,50 @@ func TestLintDocument_RulePanicIsRecovered(t *testing.T) { t.Errorf("Severity = %v, want %v", issues[0].Severity, SeverityError) } } + +// runArgsSpyRule is a stub Rule that reports every element of "runArgs" it is handed, naming how it +// was reached. It declares every file type deliberately: a rule that declares only some leaves +// LintDocument with no patterns for the rest, which it short-circuits before traversing anything, so +// a test written on such a rule would pass whatever the traversal does with the file types it skips. +var runArgsSpyRule = &Rule{ + ID: "run-args-spy", + Description: "reports how each element of runArgs was reached", + FileTypes: []FileType{Devcontainer, Feature, Template}, + Paths: []string{"/runArgs/*"}, + Check: func(_ *Context, node *Node) []Finding { + if node.Arg == nil { + return []Finding{{Message: "element " + node.Pointer, Offset: node.Value.StartOffset}} + } + return []Finding{{Message: "flag --" + node.Arg.Flag, Offset: node.Value.StartOffset}} + }, +} + +// TestLintDocument_RunArgsFileTypes checks that "runArgs" is read as a "docker run" argv only in a +// devcontainer.json. It is not a property of a Feature or a Template, so there the array is an +// ordinary one, walked by index. +func TestLintDocument_RunArgsFileTypes(t *testing.T) { + t.Parallel() + + tests := []struct { + fileType FileType + want []string + }{ + {Devcontainer, []string{"flag --cap-add"}}, + {Feature, []string{"element /runArgs/0"}}, + {Template, []string{"element /runArgs/0"}}, + } + for _, tt := range tests { + t.Run(string(tt.fileType), func(t *testing.T) { + t.Parallel() + l := New() + l.RegisterRule(runArgsSpyRule, SeverityWarn) + var got []string + for _, issue := range lintSource(t, l, "config.json", tt.fileType, `{"runArgs": ["--cap-add=ALL"]}`) { + got = append(got, issue.Message) + } + if !slices.Equal(got, tt.want) { + t.Errorf("messages = %v, want %v", got, tt.want) + } + }) + } +} diff --git a/linter/rule.go b/linter/rule.go index c76cbf4..503132b 100644 --- a/linter/rule.go +++ b/linter/rule.go @@ -6,6 +6,7 @@ import ( "io/fs" "strings" + "github.com/bare-devcontainer/decolint/dockerargs" "github.com/tailscale/hujson" ) @@ -238,6 +239,10 @@ type Node struct { Pointer string // Value is the HuJSON value at Pointer. Value *hujson.Value + // Arg is the "docker run" flag occurrence the value was reached as, set only on a node the + // "runArgs" traversal produced (see [Rule.Paths]) and nil on every other node. Value is then the + // element the flag's value is written in, which is the flag's own element or the one after it. + Arg *dockerargs.Arg } // Finding is a single problem reported by a rule. @@ -280,13 +285,20 @@ type Rule struct { // Paths are the JSON Pointer patterns of the values this rule wants to inspect. A "*" segment // matches any object member name or array index (e.g. "/mounts/*"); the empty string matches the // document root. + // + // A devcontainer.json's "runArgs" is traversed as the "docker run" argv it becomes, so its + // elements are addressed by flag rather than by index: "/runArgs/--volume" matches once per + // occurrence of that flag, whichever spelling the argv uses, and [Node.Arg] carries the value the + // occurrence gives it. A rule reporting a flag's absence cannot be driven by that, since a flag + // that is not there is never matched; it inspects the document root instead. Paths []string // Example shows the rule firing and not firing on realistic configuration. Tests lint both: Bad // must report the rule, Good must not. Example Example - // Check inspects one value matching Paths and returns any findings. It is called at most once per - // rule for a given value, even if several patterns match it. Check must be safe for concurrent - // use, since it may be called for multiple files. + // Check inspects one value matching Paths and returns any findings. It is called once for a given + // value, even if several patterns match it — except for a "runArgs" element naming several flags, + // which it is called for once per flag, with [Node.Arg] telling the occurrences apart. Check must + // be safe for concurrent use, since it may be called for multiple files. Check func(ctx *Context, node *Node) []Finding } diff --git a/linter/walk.go b/linter/walk.go index db7d2a2..1461704 100644 --- a/linter/walk.go +++ b/linter/walk.go @@ -5,6 +5,7 @@ import ( "strconv" "strings" + "github.com/bare-devcontainer/decolint/dockerargs" "github.com/tailscale/hujson" ) @@ -56,29 +57,34 @@ func matches(pat, segs []string) bool { return true } -// walk traverses the syntax tree depth-first exactly once and calls visit for every (rule, value) -// pair where one of the rule's patterns matches the value's path. A rule is visited at most once -// per value. pointer and segs describe the location of v; they must be "" and nil for the document -// root. -func walk(v *hujson.Value, pointer string, segs []string, patterns []pattern, visit func(*Rule, *Node)) { - node := &Node{Pointer: pointer, Value: v} - var called []*Rule - for _, p := range patterns { - if !matches(p.segments, segs) { - continue - } - if slices.Contains(called, p.rule) { - continue - } - called = append(called, p.rule) - visit(p.rule, node) - } +// walk traverses the syntax tree of a file of the given type depth-first exactly once and calls +// visit for every (rule, value) pair where one of the rule's patterns matches the value's path. A +// rule is visited once for a value however many of its patterns match, except for a "runArgs" +// element naming several flags, which is visited once per flag (see runArgsFlags). +func walk(root *hujson.Value, fileType FileType, patterns []pattern, visit func(*Rule, *Node)) { + w := walker{patterns: patterns, runArgs: fileType == Devcontainer, visit: visit} + w.value(root, "", nil) +} + +// walker carries the state of one traversal. +type walker struct { + patterns []pattern + // runArgs reports whether the document's "runArgs" is traversed as an argv (see runArgsFlags). + // Only a devcontainer.json has one: it is not a property of a Feature or a Template. + runArgs bool + visit func(*Rule, *Node) +} + +// value visits v and descends into it. pointer and segs describe the location of v; they must be "" +// and nil for the document root. +func (w *walker) value(v *hujson.Value, pointer string, segs []string) { + w.dispatch(&Node{Pointer: pointer, Value: v}, segs) - // append(segs, seg) below may share segs's backing array across sibling calls, so a later - // sibling can overwrite an element a previous sibling appended. This is safe only because - // traversal is sequential and no walk call retains segs past its own return (matches reads it - // synchronously via the visit callback). Parallelizing this traversal or having Node retain segs - // would require copying it first. + // append(segs, seg) here and in runArgsFlags may share segs's backing array across sibling calls, + // so a later sibling can overwrite an element a previous sibling appended. This is safe only + // because traversal is sequential and no walk call retains segs past its own return (matches + // reads it synchronously via the visit callback). Parallelizing this traversal or having Node + // retain segs would require copying it first. switch t := v.Value.(type) { case *hujson.Object: for i := range t.Members { @@ -88,12 +94,71 @@ func walk(v *hujson.Value, pointer string, segs []string, patterns []pattern, vi continue } seg := name.String() - walk(&m.Value, pointer+"/"+escapeSegment(seg), append(segs, seg), patterns, visit) + w.value(&m.Value, pointer+"/"+escapeSegment(seg), append(segs, seg)) } case *hujson.Array: + if w.runArgs && len(segs) == 1 && segs[0] == "runArgs" { + w.runArgsFlags(t, pointer, segs) + return + } for i := range t.Elements { seg := strconv.Itoa(i) - walk(&t.Elements[i], pointer+"/"+seg, append(segs, seg), patterns, visit) + w.value(&t.Elements[i], pointer+"/"+seg, append(segs, seg)) } } } + +// RunArgs returns every "docker run" flag occurrence in arr, a devcontainer.json's "runArgs", as +// [dockerargs.Parse] reads the argv the array becomes; [dockerargs.Arg.Index] indexes arr.Elements. +// An element that is not a string, which the devcontainer tooling could not hand to docker at all, +// stands in as an empty entry so that the elements around it keep the positions docker would read +// them at. +// +// It is the reading the traversal hands "/runArgs/--flag" patterns (see [Rule.Paths]), for the rules +// that cannot be driven by it because they report a flag's absence. +func RunArgs(arr *hujson.Array) []dockerargs.Arg { + argv := make([]string, len(arr.Elements)) + for i, elem := range arr.Elements { + if lit, ok := elem.Value.(hujson.Literal); ok && lit.Kind() == '"' { + argv[i] = lit.String() + } + } + return dockerargs.Parse(argv) +} + +// runArgsFlags visits the elements of arr, a devcontainer.json's "runArgs", as the "docker run" argv +// the array becomes: each flag occurrence is reached at the flag's long spelling, so "-v" and +// "--volume" alike are reached at "/runArgs/--volume", on the element the flag's value is written +// in. +// +// One element can hold several occurrences — a run of shorthands, "-it", names two flags — and is +// visited once for each, so that a rule subscribing to more than one of them, or to "/runArgs/*", +// sees every flag the argv gives rather than whichever comes first. Collapsing them would drop a +// flag silently, which is the failure this addressing exists to prevent. The elements are +// deliberately not visited by index as well: that reaches the same occurrence by a second path and +// buys nothing, where two occurrences in one element are two distinct things to report on. +func (w *walker) runArgsFlags(arr *hujson.Array, pointer string, segs []string) { + for _, arg := range RunArgs(arr) { + node := &Node{ + Pointer: pointer + "/" + strconv.Itoa(arg.Index), + Value: &arr.Elements[arg.Index], + Arg: &arg, + } + w.dispatch(node, append(segs, "--"+arg.Flag)) + } +} + +// dispatch calls visit for every rule with a pattern matching segs, at most once per rule. +func (w *walker) dispatch(node *Node, segs []string) { + var called []*Rule + for _, p := range w.patterns { + if !matches(p.segments, segs) { + continue + } + if slices.Contains(called, p.rule) { + continue + } + called = append(called, p.rule) + w.visit(p.rule, node) + } +} diff --git a/linter/walk_test.go b/linter/walk_test.go index 2c30bc4..15dd966 100644 --- a/linter/walk_test.go +++ b/linter/walk_test.go @@ -4,6 +4,8 @@ import ( "slices" "testing" + "github.com/bare-devcontainer/decolint/dockerargs" + "github.com/google/go-cmp/cmp" "github.com/tailscale/hujson" ) @@ -68,7 +70,7 @@ func TestWalk_Dispatch(t *testing.T) { var calls []string root := parseValue(t, src) patterns := compilePatterns(pathSpy("spy", tt.paths)) - walk(&root, "", nil, patterns, func(_ *Rule, node *Node) { + walk(&root, Devcontainer, patterns, func(_ *Rule, node *Node) { calls = append(calls, node.Pointer) }) if !slices.Equal(calls, tt.want) { @@ -90,7 +92,7 @@ func TestWalk_SingleTraversal(t *testing.T) { compilePatterns(pathSpy("b", []string{"/image"}))..., ) var callsA, callsB []string - walk(&root, "", nil, patterns, func(r *Rule, node *Node) { + walk(&root, Devcontainer, patterns, func(r *Rule, node *Node) { switch r.ID { case "a": callsA = append(callsA, node.Pointer) @@ -103,6 +105,116 @@ func TestWalk_SingleTraversal(t *testing.T) { } } +func TestRunArgs(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + src string + want []dockerargs.Arg + }{ + {"empty array", `[]`, nil}, + {"flag and value in one element", `["--cap-drop=ALL"]`, []dockerargs.Arg{ + {Flag: "cap-drop", Value: "ALL", Index: 0}, + }}, + {"value in the following element", `["--cap-drop", "ALL"]`, []dockerargs.Arg{ + {Flag: "cap-drop", Value: "ALL", Index: 1}, + }}, + // A non-string element keeps its position so that the ones after it keep theirs. + {"non-string element", `[123, "--privileged"]`, []dockerargs.Arg{ + {Flag: "privileged", Value: "true", Index: 1}, + }}, + {"non-string element consumed as a value", `["--label", 123, "--privileged"]`, []dockerargs.Arg{ + {Flag: "label", Value: "", Index: 1}, + {Flag: "privileged", Value: "true", Index: 2}, + }}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + v := parseValue(t, tt.src) + arr, ok := v.Value.(*hujson.Array) + if !ok { + t.Fatalf("%s is not an array", tt.src) + } + if diff := cmp.Diff(tt.want, RunArgs(arr)); diff != "" { + t.Errorf("RunArgs(%s) mismatch (-want +got):\n%s", tt.src, diff) + } + }) + } +} + +// TestWalk_RunArgs checks the traversal of a devcontainer.json's "runArgs" as the "docker run" argv +// it becomes: its elements are reached by flag rather than by index, and each of them at most once. +func TestWalk_RunArgs(t *testing.T) { + t.Parallel() + + // visit is one (rule, value) pair walk produced: where the value is, its source text, and the + // flag occurrence it was reached as, if any. + type visit struct { + pointer string + element string // the visited value, as written in the source + flag string + value string + } + tests := []struct { + name string + paths []string + src string + want []visit + }{ + {"long flag holding its value", []string{"/runArgs/--cap-add"}, `{"runArgs": ["--cap-add=ALL"]}`, + []visit{{"/runArgs/0", `"--cap-add=ALL"`, "cap-add", "ALL"}}}, + {"long flag consuming the next element", []string{"/runArgs/--cap-add"}, `{"runArgs": ["--cap-add", "ALL"]}`, + []visit{{"/runArgs/1", `"ALL"`, "cap-add", "ALL"}}}, + {"shorthand reaches the long spelling", []string{"/runArgs/--volume"}, `{"runArgs": ["-v", "/a:/b"]}`, + []visit{{"/runArgs/1", `"/a:/b"`, "volume", "/a:/b"}}}, + {"one element naming several flags", []string{"/runArgs/--interactive", "/runArgs/--tty"}, `{"runArgs": ["-it"]}`, + []visit{{"/runArgs/0", `"-it"`, "interactive", "true"}, {"/runArgs/0", `"-it"`, "tty", "true"}}}, + {"every occurrence of a flag", []string{"/runArgs/--cap-add"}, `{"runArgs": ["--cap-add=ALL", "--cap-add=NET_ADMIN"]}`, + []visit{{"/runArgs/0", `"--cap-add=ALL"`, "cap-add", "ALL"}, {"/runArgs/1", `"--cap-add=NET_ADMIN"`, "cap-add", "NET_ADMIN"}}}, + {"another flag's value names no flag", []string{"/runArgs/--cap-add"}, `{"runArgs": ["--label", "--cap-add=ALL"]}`, nil}, + {"non-string element", []string{"/runArgs/--cap-add"}, `{"runArgs": [123, "--cap-add=ALL"]}`, + []visit{{"/runArgs/1", `"--cap-add=ALL"`, "cap-add", "ALL"}}}, + {"every copy of a duplicated member", []string{"/runArgs/--cap-add"}, + `{"runArgs": ["--cap-add=ALL"], "runArgs": ["--cap-add=NET_ADMIN"]}`, + []visit{{"/runArgs/0", `"--cap-add=ALL"`, "cap-add", "ALL"}, {"/runArgs/0", `"--cap-add=NET_ADMIN"`, "cap-add", "NET_ADMIN"}}}, + + // The elements are addressed by flag only, so a wildcard reaches an element once per flag it + // names — and only the elements a flag's value is written in. + {"wildcard over flags holding their values", []string{"/runArgs/*"}, `{"runArgs": ["--privileged", "--init"]}`, + []visit{{"/runArgs/0", `"--privileged"`, "privileged", "true"}, {"/runArgs/1", `"--init"`, "init", "true"}}}, + {"wildcard over a flag consuming the next element", []string{"/runArgs/*"}, `{"runArgs": ["--cap-add", "ALL"]}`, + []visit{{"/runArgs/1", `"ALL"`, "cap-add", "ALL"}}}, + {"wildcard over one element naming several flags", []string{"/runArgs/*"}, `{"runArgs": ["-it"]}`, + []visit{{"/runArgs/0", `"-it"`, "interactive", "true"}, {"/runArgs/0", `"-it"`, "tty", "true"}}}, + + {"the array itself", []string{"/runArgs"}, `{"runArgs": ["--cap-add=ALL"]}`, + []visit{{"/runArgs", `["--cap-add=ALL"]`, "", ""}}}, + {"a runArgs that is not an array", []string{"/runArgs/--cap-add"}, `{"runArgs": "--cap-add=ALL"}`, nil}, + {"a runArgs that is not the document's", []string{"/build/runArgs/*"}, `{"build": {"runArgs": ["--cap-add=ALL"]}}`, + []visit{{"/build/runArgs/0", `"--cap-add=ALL"`, "", ""}}}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + var got []visit + root := parseValue(t, tt.src) + patterns := compilePatterns(pathSpy("spy", tt.paths)) + walk(&root, Devcontainer, patterns, func(_ *Rule, node *Node) { + v := visit{pointer: node.Pointer, element: tt.src[node.Value.StartOffset:node.Value.EndOffset]} + if node.Arg != nil { + v.flag, v.value = node.Arg.Flag, node.Arg.Value + } + got = append(got, v) + }) + if !slices.Equal(got, tt.want) { + t.Errorf("visited %v, want %v", got, tt.want) + } + }) + } +} + func TestSplitPointer(t *testing.T) { t.Parallel() diff --git a/rules/no_cap_add_all.go b/rules/no_cap_add_all.go index 215f82e..b184fa8 100644 --- a/rules/no_cap_add_all.go +++ b/rules/no_cap_add_all.go @@ -22,7 +22,7 @@ withholds the dangerous ones by default. "ALL" hands them all over, including ca }, Category: linter.CategorySecurity, FileTypes: []linter.FileType{linter.Devcontainer, linter.Feature}, - Paths: []string{"/capAdd/*", "/runArgs"}, + Paths: []string{"/capAdd/*", "/runArgs/--cap-add"}, Example: linter.Example{ Bad: linter.Snippet{ Files: []linter.ExampleFile{ @@ -46,19 +46,14 @@ withholds the dangerous ones by default. "ALL" hands them all over, including ca Check: checkNoCapAddAll, } -func checkNoCapAddAll(ctx *linter.Context, node *linter.Node) []linter.Finding { - if node.Pointer == "/runArgs" { - arr, ok := node.Value.Value.(*hujson.Array) - if !ok || !runArgsApplicable(ctx) { - return nil - } - v := runArgsFindFlagValue(arr, "cap-add", isAllCapability) - if v == nil { +func checkNoCapAddAll(_ *linter.Context, node *linter.Node) []linter.Finding { + if node.Arg != nil { + if !isAllCapability(node.Arg.Value) { return nil } return []linter.Finding{{ Message: `"runArgs" contains "--cap-add=ALL", granting every Linux capability to the container`, - Offset: v.StartOffset, + Offset: node.Value.StartOffset, }} } diff --git a/rules/no_cap_add_all_test.go b/rules/no_cap_add_all_test.go index 730cc34..0ddef11 100644 --- a/rules/no_cap_add_all_test.go +++ b/rules/no_cap_add_all_test.go @@ -43,6 +43,13 @@ func TestNoCapAddAll(t *testing.T) { Message: `"runArgs" contains "--cap-add=ALL", granting every Linux capability to the container`}, }}, {"runArgs with cap-add consumed as another flag's value", `{"runArgs": ["--label", "--cap-add=ALL"]}`, nil}, + // Every entry granting "ALL" is reported, so suppressing one does not hide the rest. + {"runArgs with two cap-add=ALL entries", `{"runArgs": ["--cap-add=ALL", "--cap-add=all"]}`, []linter.Issue{ + {Path: "devcontainer.json", Line: 1, Col: 14, RuleID: "no-cap-add-all", + Message: `"runArgs" contains "--cap-add=ALL", granting every Linux capability to the container`}, + {Path: "devcontainer.json", Line: 1, Col: 31, RuleID: "no-cap-add-all", + Message: `"runArgs" contains "--cap-add=ALL", granting every Linux capability to the container`}, + }}, {"runArgs with non-string entry before cap-add=ALL", `{"runArgs": [123, "--cap-add=ALL"]}`, []linter.Issue{ {Path: "devcontainer.json", Line: 1, Col: 19, RuleID: "no-cap-add-all", Message: `"runArgs" contains "--cap-add=ALL", granting every Linux capability to the container`}, diff --git a/rules/no_docker_socket_mount.go b/rules/no_docker_socket_mount.go index 7771f30..9c745e7 100644 --- a/rules/no_docker_socket_mount.go +++ b/rules/no_docker_socket_mount.go @@ -2,7 +2,6 @@ package rules import ( "github.com/bare-devcontainer/decolint/linter" - "github.com/tailscale/hujson" ) // NoDockerSocketMount reports a devcontainer.json that bind-mounts the host's Docker daemon socket @@ -23,7 +22,7 @@ rootless daemon keeps that access inside the container.`, }, Category: linter.CategorySecurity, FileTypes: []linter.FileType{linter.Devcontainer}, - Paths: []string{"/mounts/*", "/runArgs"}, + Paths: []string{"/mounts/*", "/runArgs/--mount", "/runArgs/--volume"}, Example: linter.Example{ Bad: linter.Snippet{ Files: []linter.ExampleFile{ @@ -56,8 +55,8 @@ rootless daemon keeps that access inside the container.`, } func checkNoDockerSocketMount(_ *linter.Context, node *linter.Node) []linter.Finding { - if node.Pointer == "/runArgs" { - return checkDockerSocketRunArgs(node) + if node.Arg != nil { + return checkDockerSocketRunArg(node) } return checkDockerSocketMount(node) } @@ -73,33 +72,22 @@ func checkDockerSocketMount(node *linter.Node) []linter.Finding { }} } -// dockerSocketRunArgFlags are the "runArgs" flags that can mount a host path, each paired with the -// reader for its own value syntax. The two syntaxes are unrelated, so a value must be read only as -// the flag introducing it. -var dockerSocketRunArgFlags = []struct { - flag string - source func(string) string -}{ - {"mount", func(s string) string { _, source := parseMountString(s); return source }}, - {"volume", volumeSpecSource}, -} - -func checkDockerSocketRunArgs(node *linter.Node) []linter.Finding { - arr, ok := node.Value.Value.(*hujson.Array) - if !ok { - return nil +// checkDockerSocketRunArg reports the host path node's "runArgs" flag mounts, if it is the Docker +// socket. The value syntaxes of the two flags that can mount one are unrelated, so a value is read +// only as the flag introducing it. +func checkDockerSocketRunArg(node *linter.Node) []linter.Finding { + var source string + switch node.Arg.Flag { + case "mount": + _, source = parseMountString(node.Arg.Value) + case "volume": + source = volumeSpecSource(node.Arg.Value) } - var findings []linter.Finding - for _, f := range dockerSocketRunArgFlags { - for value, s := range runArgsFlagValues(arr, f.flag) { - if !isDockerSocketSource(f.source(s)) { - continue - } - findings = append(findings, linter.Finding{ - Message: `"runArgs" bind-mounts the Docker socket, which grants the container root-equivalent control over the host`, - Offset: value.StartOffset, - }) - } + if !isDockerSocketSource(source) { + return nil } - return findings + return []linter.Finding{{ + Message: `"runArgs" bind-mounts the Docker socket, which grants the container root-equivalent control over the host`, + Offset: node.Value.StartOffset, + }} } diff --git a/rules/no_privileged_container.go b/rules/no_privileged_container.go index 89fb647..6f63a7f 100644 --- a/rules/no_privileged_container.go +++ b/rules/no_privileged_container.go @@ -24,7 +24,7 @@ capabilities and devices the workload needs, is a far narrower grant.`, }, Category: linter.CategorySecurity, FileTypes: []linter.FileType{linter.Devcontainer, linter.Feature}, - Paths: []string{"/privileged", "/runArgs"}, + Paths: []string{"/privileged", "/runArgs/--privileged"}, Example: linter.Example{ Bad: linter.Snippet{ Files: []linter.ExampleFile{ @@ -52,19 +52,14 @@ nested containers.`, Check: checkNoPrivilegedContainer, } -func checkNoPrivilegedContainer(ctx *linter.Context, node *linter.Node) []linter.Finding { - if node.Pointer == "/runArgs" { - arr, ok := node.Value.Value.(*hujson.Array) - if !ok || !runArgsApplicable(ctx) { - return nil - } - v := runArgsFindFlagValue(arr, "privileged", dockerargs.IsTrue) - if v == nil { +func checkNoPrivilegedContainer(_ *linter.Context, node *linter.Node) []linter.Finding { + if node.Arg != nil { + if !dockerargs.IsTrue(node.Arg.Value) { return nil } return []linter.Finding{{ Message: `"runArgs" contains "--privileged", disabling the container's isolation from the host`, - Offset: v.StartOffset, + Offset: node.Value.StartOffset, }} } diff --git a/rules/no_seccomp_override.go b/rules/no_seccomp_override.go index 9e5332c..59636ef 100644 --- a/rules/no_seccomp_override.go +++ b/rules/no_seccomp_override.go @@ -26,7 +26,7 @@ does.`, }, Category: linter.CategorySecurity, FileTypes: []linter.FileType{linter.Devcontainer, linter.Feature}, - Paths: []string{"/securityOpt/*", "/runArgs"}, + Paths: []string{"/securityOpt/*", "/runArgs/--security-opt"}, Example: linter.Example{ Bad: linter.Snippet{ Files: []linter.ExampleFile{ @@ -52,19 +52,14 @@ which already allows what a development container normally does.`, Check: checkNoSeccompOverride, } -func checkNoSeccompOverride(ctx *linter.Context, node *linter.Node) []linter.Finding { - if node.Pointer == "/runArgs" { - arr, ok := node.Value.Value.(*hujson.Array) - if !ok || !runArgsApplicable(ctx) { - return nil - } - v := runArgsFindFlagValue(arr, "security-opt", securityOptOverridesSeccomp) - if v == nil { +func checkNoSeccompOverride(_ *linter.Context, node *linter.Node) []linter.Finding { + if node.Arg != nil { + if !securityOptOverridesSeccomp(node.Arg.Value) { return nil } return []linter.Finding{{ Message: `"runArgs" overrides the default seccomp profile via "--security-opt"`, - Offset: v.StartOffset, + Offset: node.Value.StartOffset, }} } diff --git a/rules/no_seccomp_unconfined.go b/rules/no_seccomp_unconfined.go index b7b3791..52dbd27 100644 --- a/rules/no_seccomp_unconfined.go +++ b/rules/no_seccomp_unconfined.go @@ -23,7 +23,7 @@ enough on current runtimes.`, }, Category: linter.CategorySecurity, FileTypes: []linter.FileType{linter.Devcontainer, linter.Feature}, - Paths: []string{"/securityOpt/*", "/runArgs"}, + Paths: []string{"/securityOpt/*", "/runArgs/--security-opt"}, Example: linter.Example{ Bad: linter.Snippet{ Files: []linter.ExampleFile{ @@ -47,19 +47,14 @@ enough on current runtimes.`, Check: checkNoSeccompUnconfined, } -func checkNoSeccompUnconfined(ctx *linter.Context, node *linter.Node) []linter.Finding { - if node.Pointer == "/runArgs" { - arr, ok := node.Value.Value.(*hujson.Array) - if !ok || !runArgsApplicable(ctx) { - return nil - } - v := runArgsFindFlagValue(arr, "security-opt", securityOptDisablesSeccomp) - if v == nil { +func checkNoSeccompUnconfined(_ *linter.Context, node *linter.Node) []linter.Finding { + if node.Arg != nil { + if !securityOptDisablesSeccomp(node.Arg.Value) { return nil } return []linter.Finding{{ Message: `"runArgs" contains "--security-opt seccomp=unconfined", disabling the container's syscall filtering`, - Offset: v.StartOffset, + Offset: node.Value.StartOffset, }} } diff --git a/rules/require_cap_drop_all.go b/rules/require_cap_drop_all.go index 9fa698e..b20384e 100644 --- a/rules/require_cap_drop_all.go +++ b/rules/require_cap_drop_all.go @@ -56,10 +56,8 @@ func checkRequireCapDropAll(_ *linter.Context, node *linter.Node) []linter.Findi return nil } - for arr := range arrayMembers(obj, "runArgs") { - if runArgsFindFlagValue(arr, "cap-drop", isAllCapability) != nil { - return nil - } + if runArgsHasFlagValue(obj, "cap-drop", isAllCapability) { + return nil } return []linter.Finding{{ diff --git a/rules/require_no_new_privileges.go b/rules/require_no_new_privileges.go index 29f58a8..eb22fb0 100644 --- a/rules/require_no_new_privileges.go +++ b/rules/require_no_new_privileges.go @@ -56,10 +56,8 @@ func checkRequireNoNewPrivileges(_ *linter.Context, node *linter.Node) []linter. if stringArrayContains(obj, "securityOpt", securityOptIsNoNewPrivileges) { return nil } - for arr := range arrayMembers(obj, "runArgs") { - if runArgsFindFlagValue(arr, "security-opt", securityOptIsNoNewPrivileges) != nil { - return nil - } + if runArgsHasFlagValue(obj, "security-opt", securityOptIsNoNewPrivileges) { + return nil } return []linter.Finding{{ diff --git a/rules/util.go b/rules/util.go index 86a06d4..c432ea9 100644 --- a/rules/util.go +++ b/rules/util.go @@ -94,43 +94,22 @@ func arrayMembers(obj *hujson.Object, name string) iter.Seq[*hujson.Array] { } } -// runArgsFlagValues yields every value that arr, a "runArgs" array, gives to the "docker run" flag -// named flag, in order. flag is the flag's name rather than a spelling of it, so "volume" covers -// both "-v" and "--volume"; see [dockerargs.Parse] for the entry forms a value can be written in. -// Each yielded pair is the array element holding the value and the value itself. -func runArgsFlagValues(arr *hujson.Array, flag string) iter.Seq2[*hujson.Value, string] { - return func(yield func(*hujson.Value, string) bool) { - for _, arg := range dockerargs.Parse(runArgsArgv(arr)) { - if arg.Flag == flag && !yield(&arr.Elements[arg.Index], arg.Value) { - return +// runArgsHasFlagValue reports whether any "runArgs" member of obj (see [arrayMembers]) gives the +// "docker run" flag named flag a value match accepts. flag is the flag's long name without the +// leading "--", so "volume" covers both "-v" and "--volume". +// +// It is for the rules that report a flag's absence, which the engine cannot hand an occurrence of. +// A rule reporting a flag's presence declares a "/runArgs/--flag" path instead and never reads the +// array itself. +func runArgsHasFlagValue(obj *hujson.Object, flag string, match func(string) bool) bool { + for arr := range arrayMembers(obj, "runArgs") { + for _, arg := range linter.RunArgs(arr) { + if arg.Flag == flag && match(arg.Value) { + return true } } } -} - -// runArgsArgv returns arr, a "runArgs" array, as the argv it becomes. An element that is not a -// string, which the devcontainer tooling could not hand to docker at all, stands in as an empty -// entry so that the elements around it keep the positions docker would read them at. -func runArgsArgv(arr *hujson.Array) []string { - argv := make([]string, len(arr.Elements)) - for i, elem := range arr.Elements { - if lit, ok := elem.Value.(hujson.Literal); ok && lit.Kind() == '"' { - argv[i] = lit.String() - } - } - return argv -} - -// runArgsFindFlagValue returns the hujson.Value holding the first value arr gives to flag that match -// accepts, or nil if it gives flag no such value. See [runArgsFlagValues] for the entry forms it -// recognizes. -func runArgsFindFlagValue(arr *hujson.Array, flag string, match func(string) bool) *hujson.Value { - for v, s := range runArgsFlagValues(arr, flag) { - if match(s) { - return v - } - } - return nil + return false } // parseMountString extracts the "type" and "source" fields from s, a "--mount" value, as docker/cli @@ -238,12 +217,6 @@ func stringMember(obj *hujson.Object, name string) (string, bool) { return "", false } -// runArgsApplicable reports whether ctx is for a devcontainer.json, the only file type where -// "runArgs" is meaningful; a Feature has no use for it, so rules should not flag one there. -func runArgsApplicable(ctx *linter.Context) bool { - return ctx.Type == linter.Devcontainer -} - // refTag extracts the tag from an OCI-style reference, e.g. a container image or Feature reference. // A reference pinned by digest (e.g. "ref@sha256:...") is treated as tagged. The colon in a // registry host with a port (e.g. "localhost:5000/img") is not a tag separator.