diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 7117d80..6da3c05 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -44,6 +44,31 @@ and calls `Check` for every value matching one of the paths; a `*` segment matches any object member name or array index, and the empty string matches the document root. +A devcontainer.json's `runArgs` is the exception: it is traversed as +the `docker run` argv it becomes (see [The `docker run` flag +table](#the-docker-run-flag-table) below), so a rule addresses its +entries by flag rather than by index. `/runArgs/--volume` matches once +per occurrence of that flag, whichever spelling the argv uses; +`node.Arg` carries the flag's value, and `node.Value` is the entry that +value is written in, which is not necessarily the one naming the flag. +Nothing else is addressed under `/runArgs` there — `/runArgs/*` +included, and a `runArgs` that is not an array is reached only as a +whole, at `/runArgs` — so a path under it always arrives with +`node.Arg` set. + +Only a devcontainer.json has a `runArgs` at all. A Feature or a +Template that carries one is walked as the ordinary data it is, so +`/runArgs/--volume` matches a member merely spelled like the flag +there, with `node.Arg` nil — exactly as it is on the property the +rule's other path names. A rule reporting both a property and a flag +has to ignore those matches, with `underRunArgs` (see +[`rules/util.go`](rules/util.go)). + +A rule that reports a flag's *absence* cannot be driven by any of that: +a flag that is not there is never matched. It inspects the document +root instead, asking `runArgsHasFlagValue` (also in +[`rules/util.go`](rules/util.go)) for the flag's values. + A rule's default severity is not set individually; it comes entirely from its category (see `categoryDefaultSeverities` in [`rules.go`](rules/rules.go)) — only `CategoryCorrectness` runs by @@ -125,8 +150,9 @@ line, so where a rule finds a value depends on which flags take one: `["--label", "--cap-drop=ALL"]` drops no capability, because `--label` consumes the entry after it. [`dockerargs`](dockerargs/) reads a `runArgs` array the way pflag — the parser docker/cli uses — reads an -argv, and rules ask it for a flag's values instead of matching entries -themselves. +argv. Both ways a rule reaches a flag (see [Adding a +rule](#adding-a-rule) above) are built on that reading, so no rule +matches entries itself. That needs to know every flag `docker run` registers and whether it takes a value. A table written by hand would go stale the first time diff --git a/linter/linter_test.go b/linter/linter_test.go index a1ec1fd..c827762 100644 --- a/linter/linter_test.go +++ b/linter/linter_test.go @@ -430,44 +430,95 @@ func TestLintDocument_RulePanicIsRecovered(t *testing.T) { } } -// 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 +// runArgsSpy returns a stub Rule subscribing to path that reports every value 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}} - }, +func runArgsSpy(id, path string) *Rule { + return &Rule{ + ID: id, + Description: "reports how each value under runArgs was reached", + FileTypes: []FileType{Devcontainer, Feature, Template}, + Paths: []string{path}, + 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. +// ordinary one, walked by index — and so is a "runArgs" that is not an array at all, which a +// devcontainer.json has nothing to read in. func TestLintDocument_RunArgsFileTypes(t *testing.T) { t.Parallel() + const ( + argv = `{"runArgs": ["--cap-add=ALL"]}` + object = `{"runArgs": {"--cap-add": "ALL"}}` + ) tests := []struct { + name string fileType FileType + src string want []string }{ - {Devcontainer, []string{"flag --cap-add"}}, - {Feature, []string{"element /runArgs/0"}}, - {Template, []string{"element /runArgs/0"}}, + {"devcontainer", Devcontainer, argv, []string{"flag --cap-add"}}, + {"feature", Feature, argv, []string{"element /runArgs/0"}}, + {"template", Template, argv, []string{"element /runArgs/0"}}, + {"devcontainer object", Devcontainer, object, nil}, + {"feature object", Feature, object, []string{"element /runArgs/--cap-add"}}, + {"template object", Template, object, []string{"element /runArgs/--cap-add"}}, } for _, tt := range tests { - t.Run(string(tt.fileType), func(t *testing.T) { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + l := New() + l.RegisterRule(runArgsSpy("run-args-spy", "/runArgs/*"), SeverityWarn) + var got []string + for _, issue := range lintSource(t, l, "config.json", tt.fileType, tt.src) { + got = append(got, issue.Message) + } + if !slices.Equal(got, tt.want) { + t.Errorf("messages = %v, want %v", got, tt.want) + } + }) + } +} + +// TestLintDocument_RunArgsFlagPath checks what a "/runArgs/--flag" path matches. In a devcontainer.json +// it is the argv's occurrences of that flag and nothing else; in a Feature or a Template, where +// "runArgs" is no property of the file at all, it is an ordinary member that happens to be spelled +// like the flag. A rule that reports both a property and a flag has to tell the two apart itself, +// since [Node.Arg] is nil for such a member just as it is for the property. +func TestLintDocument_RunArgsFlagPath(t *testing.T) { + t.Parallel() + + const ( + argv = `{"runArgs": ["--cap-add=ALL"]}` + object = `{"runArgs": {"--cap-add": "ALL"}}` + ) + tests := []struct { + name string + fileType FileType + src string + want []string + }{ + {"devcontainer", Devcontainer, argv, []string{"flag --cap-add"}}, + {"devcontainer object", Devcontainer, object, nil}, + {"feature", Feature, argv, nil}, + {"feature object", Feature, object, []string{"element /runArgs/--cap-add"}}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { t.Parallel() l := New() - l.RegisterRule(runArgsSpyRule, SeverityWarn) + l.RegisterRule(runArgsSpy("run-args-flag-spy", "/runArgs/--cap-add"), SeverityWarn) var got []string - for _, issue := range lintSource(t, l, "config.json", tt.fileType, `{"runArgs": ["--cap-add=ALL"]}`) { + for _, issue := range lintSource(t, l, "config.json", tt.fileType, tt.src) { got = append(got, issue.Message) } if !slices.Equal(got, tt.want) { diff --git a/linter/rule.go b/linter/rule.go index 503132b..c91fadb 100644 --- a/linter/rule.go +++ b/linter/rule.go @@ -289,8 +289,13 @@ type Rule struct { // 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. + // occurrence gives it. Nothing else is addressed under it, so a pattern matching there always + // arrives with [Node.Arg] set. Only a devcontainer.json has a "runArgs" at all: in a Feature or a + // Template the same pattern matches whatever that name holds, an ordinary member merely spelled + // like the flag included, with [Node.Arg] nil. + // + // A rule reporting a flag's absence cannot be driven by any of 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. diff --git a/linter/walk.go b/linter/walk.go index 1461704..f4925c9 100644 --- a/linter/walk.go +++ b/linter/walk.go @@ -80,6 +80,16 @@ type walker struct { func (w *walker) value(v *hujson.Value, pointer string, segs []string) { w.dispatch(&Node{Pointer: pointer, Value: v}, segs) + // The segments under a devcontainer.json's "runArgs" name the argv's flags and nothing else, so a + // "runArgs" that is not an array is not descended: it has no flag to visit, and its members would + // otherwise be addressed in the space the flags occupy. + if w.runArgs && len(segs) == 1 && segs[0] == "runArgs" { + if arr, ok := v.Value.(*hujson.Array); ok { + w.runArgsFlags(arr, pointer, segs) + } + return + } + // 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 @@ -97,10 +107,6 @@ func (w *walker) value(v *hujson.Value, pointer string, segs []string) { 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) w.value(&t.Elements[i], pointer+"/"+seg, append(segs, seg)) diff --git a/linter/walk_test.go b/linter/walk_test.go index 15dd966..0c6beb0 100644 --- a/linter/walk_test.go +++ b/linter/walk_test.go @@ -192,6 +192,13 @@ func TestWalk_RunArgs(t *testing.T) { {"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 an object is no argv, so it holds no flag occurrence — by whatever path + // one is asked for, including a member of it named like a flag. + {"a member named like a flag", []string{"/runArgs/--cap-add"}, `{"runArgs": {"--cap-add": "ALL"}}`, nil}, + {"wildcard over an object runArgs", []string{"/runArgs/*"}, `{"runArgs": {"--cap-add": "ALL"}}`, nil}, + {"the object itself", []string{"/runArgs"}, `{"runArgs": {"--cap-add": "ALL"}}`, + []visit{{"/runArgs", `{"--cap-add": "ALL"}`, "", ""}}}, {"a runArgs that is not the document's", []string{"/build/runArgs/*"}, `{"build": {"runArgs": ["--cap-add=ALL"]}}`, []visit{{"/build/runArgs/0", `"--cap-add=ALL"`, "", ""}}}, } diff --git a/rules/no_cap_add_all.go b/rules/no_cap_add_all.go index b184fa8..6a51eff 100644 --- a/rules/no_cap_add_all.go +++ b/rules/no_cap_add_all.go @@ -57,6 +57,9 @@ func checkNoCapAddAll(_ *linter.Context, node *linter.Node) []linter.Finding { }} } + if underRunArgs(node) { + return nil + } lit, ok := node.Value.Value.(hujson.Literal) if !ok || lit.Kind() != '"' || !isAllCapability(lit.String()) { return nil diff --git a/rules/no_cap_add_all_test.go b/rules/no_cap_add_all_test.go index 0ddef11..79856a2 100644 --- a/rules/no_cap_add_all_test.go +++ b/rules/no_cap_add_all_test.go @@ -50,6 +50,8 @@ func TestNoCapAddAll(t *testing.T) { {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`}, }}, + // An object "runArgs" is no command line, so a member named like a flag is not that flag. + {"runArgs object with a cap-add member", `{"runArgs": {"--cap-add": "ALL"}}`, nil}, {"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`}, @@ -87,8 +89,9 @@ func TestNoCapAddAll_Feature(t *testing.T) { {Path: "devcontainer-feature.json", Line: 1, Col: 27, RuleID: "no-cap-add-all", Message: `"capAdd" contains "ALL", granting every Linux capability to the container`}, }}, - // "runArgs" has no meaning in a Feature, so it's not flagged there. + // "runArgs" has no meaning in a Feature, so it's not flagged there, whatever it holds. {"runArgs with cap-add=ALL is ignored", `{"id": "test", "runArgs": ["--cap-add=ALL"]}`, nil}, + {"runArgs object with a cap-add member is ignored", `{"id": "test", "runArgs": {"--cap-add": "ALL"}}`, nil}, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { diff --git a/rules/no_docker_socket_mount_test.go b/rules/no_docker_socket_mount_test.go index 60c1ae7..a77447f 100644 --- a/rules/no_docker_socket_mount_test.go +++ b/rules/no_docker_socket_mount_test.go @@ -116,6 +116,9 @@ func TestNoDockerSocketMount(t *testing.T) { // A -v value is colon-separated, so a comma in it is part of a field rather than a separator. {"runArgs --volume with a comma in the container path", `{"runArgs": ["--volume", "myvol:/data,source=/var/run/docker.sock"]}`, nil}, {"runArgs not an array", `{"runArgs": "-v /var/run/docker.sock:/x"}`, nil}, + // An object "runArgs" is no command line, so a member named like a flag is not that flag. + {"runArgs object with a volume member", + `{"runArgs": {"--volume": {"type": "bind", "source": "/var/run/docker.sock", "target": "/x"}}}`, nil}, {"runArgs duplicated, mounted by the first", `{"runArgs": ["-v", "/var/run/docker.sock:/x"], "runArgs": ["--init"]}`, []linter.Issue{ {Path: "devcontainer.json", Line: 1, Col: 20, RuleID: "no-docker-socket-mount", Message: `"runArgs" bind-mounts the Docker socket, which grants the container root-equivalent control over the host`}, diff --git a/rules/no_privileged_container.go b/rules/no_privileged_container.go index 6f63a7f..2dfc924 100644 --- a/rules/no_privileged_container.go +++ b/rules/no_privileged_container.go @@ -63,6 +63,9 @@ func checkNoPrivilegedContainer(_ *linter.Context, node *linter.Node) []linter.F }} } + if underRunArgs(node) { + return nil + } lit, ok := node.Value.Value.(hujson.Literal) if !ok || lit.Kind() != 't' { return nil diff --git a/rules/no_privileged_container_test.go b/rules/no_privileged_container_test.go index 8bdf519..72d884e 100644 --- a/rules/no_privileged_container_test.go +++ b/rules/no_privileged_container_test.go @@ -35,6 +35,8 @@ func TestNoPrivilegedContainer(t *testing.T) { }}, {"runArgs with privileged set to false", `{"runArgs": ["--privileged=false"]}`, nil}, {"runArgs with privileged consumed as another flag's value", `{"runArgs": ["--label", "--privileged"]}`, nil}, + // An object "runArgs" is no command line, so a member named like a flag is not that flag. + {"runArgs object with a privileged member", `{"runArgs": {"--privileged": true}}`, nil}, {"both privileged and runArgs flag", `{"privileged": true, "runArgs": ["--privileged"]}`, []linter.Issue{ {Path: "devcontainer.json", Line: 1, Col: 16, RuleID: "no-privileged-container", Message: `"privileged" is set to true, disabling the container's isolation from the host`}, @@ -64,8 +66,9 @@ func TestNoPrivilegedContainer_Feature(t *testing.T) { {Path: "devcontainer-feature.json", Line: 1, Col: 30, RuleID: "no-privileged-container", Message: `"privileged" is set to true, disabling the container's isolation from the host`}, }}, - // "runArgs" has no meaning in a Feature, so it's not flagged there. + // "runArgs" has no meaning in a Feature, so it's not flagged there, whatever it holds. {"runArgs with privileged is ignored", `{"id": "test", "runArgs": ["--privileged"]}`, nil}, + {"runArgs object with a privileged member is ignored", `{"id": "test", "runArgs": {"--privileged": true}}`, nil}, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { diff --git a/rules/no_seccomp_override.go b/rules/no_seccomp_override.go index 59636ef..4ba1d38 100644 --- a/rules/no_seccomp_override.go +++ b/rules/no_seccomp_override.go @@ -63,6 +63,9 @@ func checkNoSeccompOverride(_ *linter.Context, node *linter.Node) []linter.Findi }} } + if underRunArgs(node) { + return nil + } lit, ok := node.Value.Value.(hujson.Literal) if !ok || lit.Kind() != '"' || !securityOptOverridesSeccomp(lit.String()) { return nil diff --git a/rules/no_seccomp_override_test.go b/rules/no_seccomp_override_test.go index ab1f9d2..1d4a63f 100644 --- a/rules/no_seccomp_override_test.go +++ b/rules/no_seccomp_override_test.go @@ -56,6 +56,8 @@ func TestNoSeccompOverride(t *testing.T) { }}, {"runArgs security-opt consumed as another flag's value", `{"runArgs": ["--label", "--security-opt=seccomp=unconfined"]}`, nil}, {"runArgs bare seccomp entry names no flag", `{"runArgs": ["seccomp=unconfined"]}`, nil}, + // An object "runArgs" is no command line, so a member named like a flag is not that flag. + {"runArgs object with a security-opt member", `{"runArgs": {"--security-opt": "seccomp=unconfined"}}`, nil}, {"runArgs with custom seccomp profile", `{"runArgs": ["--security-opt", "seccomp=/path/to/profile.json"]}`, []linter.Issue{ {Path: "devcontainer.json", Line: 1, Col: 32, RuleID: "no-seccomp-override", Message: `"runArgs" overrides the default seccomp profile via "--security-opt"`}, @@ -83,8 +85,9 @@ func TestNoSeccompOverride_Feature(t *testing.T) { {Path: "devcontainer-feature.json", Line: 1, Col: 32, RuleID: "no-seccomp-override", Message: `"securityOpt" overrides the default seccomp profile`}, }}, - // "runArgs" has no meaning in a Feature, so it's not flagged there. + // "runArgs" has no meaning in a Feature, so it's not flagged there, whatever it holds. {"runArgs with security-opt is ignored", `{"id": "test", "runArgs": ["--security-opt=seccomp=unconfined"]}`, nil}, + {"runArgs object with a security-opt member is ignored", `{"id": "test", "runArgs": {"--security-opt": "seccomp=unconfined"}}`, nil}, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { diff --git a/rules/no_seccomp_unconfined.go b/rules/no_seccomp_unconfined.go index 52dbd27..85b9c1a 100644 --- a/rules/no_seccomp_unconfined.go +++ b/rules/no_seccomp_unconfined.go @@ -58,6 +58,9 @@ func checkNoSeccompUnconfined(_ *linter.Context, node *linter.Node) []linter.Fin }} } + if underRunArgs(node) { + return nil + } lit, ok := node.Value.Value.(hujson.Literal) if !ok || lit.Kind() != '"' || !securityOptDisablesSeccomp(lit.String()) { return nil diff --git a/rules/no_seccomp_unconfined_test.go b/rules/no_seccomp_unconfined_test.go index 1e9cbb9..a51ec57 100644 --- a/rules/no_seccomp_unconfined_test.go +++ b/rules/no_seccomp_unconfined_test.go @@ -48,6 +48,8 @@ func TestNoSeccompUnconfined(t *testing.T) { {"runArgs seccomp builtin", `{"runArgs": ["--security-opt", "seccomp=builtin"]}`, nil}, {"runArgs security-opt consumed as another flag's value", `{"runArgs": ["--label", "--security-opt=seccomp=unconfined"]}`, nil}, {"runArgs bare seccomp entry names no flag", `{"runArgs": ["seccomp=unconfined"]}`, nil}, + // An object "runArgs" is no command line, so a member named like a flag is not that flag. + {"runArgs object with a security-opt member", `{"runArgs": {"--security-opt": "seccomp=unconfined"}}`, nil}, {"runArgs seccomp unconfined combined", `{"runArgs": ["--security-opt=seccomp=unconfined"]}`, []linter.Issue{ {Path: "devcontainer.json", Line: 1, Col: 14, RuleID: "no-seccomp-unconfined", Message: `"runArgs" contains "--security-opt seccomp=unconfined", disabling the container's syscall filtering`}, @@ -75,8 +77,9 @@ func TestNoSeccompUnconfined_Feature(t *testing.T) { {Path: "devcontainer-feature.json", Line: 1, Col: 32, RuleID: "no-seccomp-unconfined", Message: `"securityOpt" contains "seccomp=unconfined", disabling the container's syscall filtering`}, }}, - // "runArgs" has no meaning in a Feature, so it's not flagged there. + // "runArgs" has no meaning in a Feature, so it's not flagged there, whatever it holds. {"runArgs with security-opt is ignored", `{"id": "test", "runArgs": ["--security-opt=seccomp=unconfined"]}`, nil}, + {"runArgs object with a security-opt member is ignored", `{"id": "test", "runArgs": {"--security-opt": "seccomp=unconfined"}}`, nil}, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { diff --git a/rules/util.go b/rules/util.go index c432ea9..835497f 100644 --- a/rules/util.go +++ b/rules/util.go @@ -112,6 +112,15 @@ func runArgsHasFlagValue(obj *hujson.Object, flag string, match func(string) boo return false } +// underRunArgs reports whether node is a value inside a "runArgs" rather than the property a rule's +// other paths name. Only a devcontainer.json's "runArgs" is a "docker run" argv: in a Feature or a +// Template it is ordinary data, so a "/runArgs/--flag" path matches a member merely spelled like a +// flag there, with [linter.Node.Arg] nil just as on the property. A rule reporting both a property +// and a flag must ignore such a node. +func underRunArgs(node *linter.Node) bool { + return strings.HasPrefix(node.Pointer, "/runArgs/") +} + // parseMountString extracts the "type" and "source" fields from s, a "--mount" value, as docker/cli // reads it: // - the value is trimmed of surrounding whitespace and then read as a single CSV record, so a