diff --git a/rules/no_bind_mount.go b/rules/no_bind_mount.go index a6864e8..0f08751 100644 --- a/rules/no_bind_mount.go +++ b/rules/no_bind_mount.go @@ -59,7 +59,7 @@ without an error and the container starts missing the data it expects. Volume mo func checkNoBindMount(_ *linter.Context, node *linter.Node) []linter.Finding { mountType, source, ok := parseMount(node.Value) - if !ok || mountType != "bind" || source == dockerSocketPath { + if !ok || mountType != "bind" || isDockerSocketSource(source) { return nil } return []linter.Finding{{ diff --git a/rules/no_bind_mount_test.go b/rules/no_bind_mount_test.go index 47dd0b9..27d14ab 100644 --- a/rules/no_bind_mount_test.go +++ b/rules/no_bind_mount_test.go @@ -26,7 +26,20 @@ func TestNoBindMount(t *testing.T) { {Path: "devcontainer.json", Line: 1, Col: 13, RuleID: "no-bind-mount", Message: `"mounts" entry uses the "bind" type, which GitHub Codespaces silently ignores`}, }}, + {"string bind mount with upper-case keys and type", `{"mounts": ["Type=Bind,Source=/host/d,dst=/x"]}`, []linter.Issue{ + {Path: "devcontainer.json", Line: 1, Col: 13, RuleID: "no-bind-mount", + Message: `"mounts" entry uses the "bind" type, which GitHub Codespaces silently ignores`}, + }}, + {"string bind mount with a quoted field", `{"mounts": ["type=bind,\"src=/host/d\",dst=/x"]}`, []linter.Issue{ + {Path: "devcontainer.json", Line: 1, Col: 13, RuleID: "no-bind-mount", + Message: `"mounts" entry uses the "bind" type, which GitHub Codespaces silently ignores`}, + }}, + {"string mount whose fields are not a CSV record", `{"mounts": ["type=bind,src=\"/host/d\",dst=/x"]}`, nil}, {"object volume mount", `{"mounts": [{"source": "vol", "target": "/data", "type": "volume"}]}`, nil}, + {"object bind mount with upper-case type", `{"mounts": [{"source": "/host/d", "target": "/x", "type": "BIND"}]}`, []linter.Issue{ + {Path: "devcontainer.json", Line: 1, Col: 13, RuleID: "no-bind-mount", + Message: `"mounts" entry uses the "bind" type, which GitHub Codespaces silently ignores`}, + }}, {"object mount with no type", `{"mounts": [{"source": "vol", "target": "/data"}]}`, nil}, {"object bind mount", `{"mounts": [{"source": "/host", "target": "/data", "type": "bind"}]}`, []linter.Issue{ {Path: "devcontainer.json", Line: 1, Col: 13, RuleID: "no-bind-mount", @@ -39,6 +52,11 @@ func TestNoBindMount(t *testing.T) { {"string docker socket bind mount", `{"mounts": ["source=/var/run/docker.sock,target=/var/run/docker.sock,type=bind"]}`, nil}, {"string docker socket bind mount with docker-host.sock target", `{"mounts": ["source=/var/run/docker.sock,target=/var/run/docker-host.sock,type=bind"]}`, nil}, {"object docker socket bind mount", `{"mounts": [{"source": "/var/run/docker.sock", "target": "/var/run/docker.sock", "type": "bind"}]}`, nil}, + // The socket exemption must recognize every spelling no-docker-socket-mount reports, or the two + // rules report the same mount with contradictory findings. + {"string docker socket bind mount via src alias", `{"mounts": ["type=bind,src=/var/run/docker.sock,dst=/x"]}`, nil}, + {"string docker socket bind mount with a doubled leading slash", `{"mounts": ["type=bind,src=//var/run/docker.sock,dst=/x"]}`, nil}, + {"string docker socket bind mount with a trailing slash", `{"mounts": ["type=bind,src=/var/run/docker.sock/,dst=/x"]}`, nil}, {"bind mount with unrelated source named docker.sock", `{"mounts": ["source=/host/docker.sock,target=/var/run/docker.sock,type=bind"]}`, []linter.Issue{ {Path: "devcontainer.json", Line: 1, Col: 13, RuleID: "no-bind-mount", Message: `"mounts" entry uses the "bind" type, which GitHub Codespaces silently ignores`}, diff --git a/rules/no_docker_socket_mount.go b/rules/no_docker_socket_mount.go index 0ffa679..cdfba49 100644 --- a/rules/no_docker_socket_mount.go +++ b/rules/no_docker_socket_mount.go @@ -1,8 +1,6 @@ package rules import ( - "strings" - "github.com/bare-devcontainer/decolint/linter" "github.com/tailscale/hujson" ) @@ -25,7 +23,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"}, Example: linter.Example{ Bad: linter.Snippet{ Files: []linter.ExampleFile{ @@ -58,15 +56,15 @@ rootless daemon keeps that access inside the container.`, } func checkNoDockerSocketMount(_ *linter.Context, node *linter.Node) []linter.Finding { - if strings.HasPrefix(node.Pointer, "/mounts/") { - return checkDockerSocketMount(node) + if node.Pointer == "/runArgs" { + return checkDockerSocketRunArgs(node) } - return checkDockerSocketRunArg(node) + return checkDockerSocketMount(node) } func checkDockerSocketMount(node *linter.Node) []linter.Finding { _, source, ok := parseMount(node.Value) - if !ok || source != dockerSocketPath { + if !ok || !isDockerSocketSource(source) { return nil } return []linter.Finding{{ @@ -75,29 +73,35 @@ func checkDockerSocketMount(node *linter.Node) []linter.Finding { }} } -func checkDockerSocketRunArg(node *linter.Node) []linter.Finding { - lit, ok := node.Value.Value.(hujson.Literal) - if !ok || lit.Kind() != '"' || !runArgMountsDockerSocket(lit.String()) { - return nil - } - return []linter.Finding{{ - Message: `"runArgs" bind-mounts the Docker socket, which grants the container root-equivalent control over the host`, - Offset: node.Value.StartOffset, - }} +// 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, which is why this rule inspects the whole "runArgs" array rather than its +// entries one by one. +var dockerSocketRunArgFlags = []struct { + flag string + source func(string) string +}{ + {"--mount", func(s string) string { _, source := parseMountString(s); return source }}, + {"--volume", volumeSpecSource}, + {"-v", volumeSpecSource}, } -// runArgMountsDockerSocket reports whether s, a single "runArgs" entry, bind-mounts the Docker -// socket. It recognizes a "--mount"-style "key=value,..." entry with a matching "source", and a -// "-v"/"--volume" entry, with or without an "=" before the value, whose host path is the Docker -// socket. -func runArgMountsDockerSocket(s string) bool { - if strings.Contains(s, ",") { - if _, source := parseMountString(strings.TrimPrefix(s, "--mount=")); source == dockerSocketPath { - return true - } +func checkDockerSocketRunArgs(node *linter.Node) []linter.Finding { + arr, ok := node.Value.Value.(*hujson.Array) + if !ok { + return nil } - for _, prefix := range []string{"--volume=", "-v="} { - s = strings.TrimPrefix(s, prefix) + 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, + }) + } } - return s == dockerSocketPath || strings.HasPrefix(s, dockerSocketPath+":") + return findings } diff --git a/rules/no_docker_socket_mount_test.go b/rules/no_docker_socket_mount_test.go index 2c7dcff..0ab54aa 100644 --- a/rules/no_docker_socket_mount_test.go +++ b/rules/no_docker_socket_mount_test.go @@ -31,7 +31,28 @@ func TestNoDockerSocketMount(t *testing.T) { {Path: "devcontainer.json", Line: 1, Col: 13, RuleID: "no-docker-socket-mount", Message: `"mounts" entry bind-mounts the Docker socket, which grants the container root-equivalent control over the host`}, }}, + {"string docker socket mount via src alias", `{"mounts": ["type=bind,src=/var/run/docker.sock,dst=/x"]}`, []linter.Issue{ + {Path: "devcontainer.json", Line: 1, Col: 13, RuleID: "no-docker-socket-mount", + Message: `"mounts" entry bind-mounts the Docker socket, which grants the container root-equivalent control over the host`}, + }}, + {"string docker socket mount with upper-case keys", `{"mounts": ["Type=Bind,SRC=/var/run/docker.sock,DST=/x"]}`, []linter.Issue{ + {Path: "devcontainer.json", Line: 1, Col: 13, RuleID: "no-docker-socket-mount", + Message: `"mounts" entry bind-mounts the Docker socket, which grants the container root-equivalent control over the host`}, + }}, + {"string docker socket mount with a quoted field", `{"mounts": ["type=bind,\"src=/var/run/docker.sock\",dst=/x"]}`, []linter.Issue{ + {Path: "devcontainer.json", Line: 1, Col: 13, RuleID: "no-docker-socket-mount", + Message: `"mounts" entry bind-mounts the Docker socket, which grants the container root-equivalent control over the host`}, + }}, + {"string docker socket mount with leading whitespace", `{"mounts": [" \"src=/var/run/docker.sock\",dst=/x,type=bind"]}`, []linter.Issue{ + {Path: "devcontainer.json", Line: 1, Col: 13, RuleID: "no-docker-socket-mount", + Message: `"mounts" entry bind-mounts the Docker socket, which grants the container root-equivalent control over the host`}, + }}, + {"object docker socket mount with upper-case type", `{"mounts": [{"source": "/var/run/docker.sock", "target": "/x", "type": "BIND"}]}`, []linter.Issue{ + {Path: "devcontainer.json", Line: 1, Col: 13, RuleID: "no-docker-socket-mount", + Message: `"mounts" entry bind-mounts the Docker socket, which grants the container root-equivalent control over the host`}, + }}, {"unrelated source named docker.sock", `{"mounts": ["source=/host/docker.sock,target=/var/run/docker.sock,type=bind"]}`, nil}, + {"string mount whose fields are not a CSV record", `{"mounts": ["type=bind,src=\"/var/run/docker.sock\",dst=/x"]}`, nil}, {"non-string literal mount entry", `{"mounts": [123]}`, nil}, {"non-literal, non-object mount entry", `{"mounts": [["source=/var/run/docker.sock,target=/var/run/docker.sock,type=bind"]]}`, nil}, @@ -61,7 +82,39 @@ func TestNoDockerSocketMount(t *testing.T) { {Path: "devcontainer.json", Line: 1, Col: 14, RuleID: "no-docker-socket-mount", Message: `"runArgs" bind-mounts the Docker socket, which grants the container root-equivalent control over the host`}, }}, + {"runArgs --mount two tokens via src alias", `{"runArgs": ["--mount", "type=bind,src=/var/run/docker.sock,dst=/x"]}`, []linter.Issue{ + {Path: "devcontainer.json", Line: 1, Col: 25, RuleID: "no-docker-socket-mount", + Message: `"runArgs" bind-mounts the Docker socket, which grants the container root-equivalent control over the host`}, + }}, + {"runArgs -v host path with a doubled leading slash", `{"runArgs": ["-v", "//var/run/docker.sock:/x"]}`, []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`}, + }}, + {"runArgs -v host path with a trailing slash", `{"runArgs": ["-v", "/var/run/docker.sock/:/x"]}`, []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`}, + }}, + {"runArgs every -v is reported", `{"runArgs": ["-v", "/var/run/docker.sock:/a", "-v", "/var/run/docker.sock:/b"]}`, []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`}, + {Path: "devcontainer.json", Line: 1, Col: 53, RuleID: "no-docker-socket-mount", + Message: `"runArgs" bind-mounts the Docker socket, which grants the container root-equivalent control over the host`}, + }}, {"runArgs unrelated volume", `{"runArgs": ["-v", "/host/docker.sock:/var/run/docker.sock"]}`, nil}, + // A -v value of a single field is an anonymous volume, and that field is the container path: + // nothing from the host is bound. + {"runArgs -v with only a container path", `{"runArgs": ["-v", "/var/run/docker.sock"]}`, nil}, + // 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}, + {"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`}, + }}, + {"runArgs duplicated, mounted by the last", `{"runArgs": ["--init"], "runArgs": ["-v", "/var/run/docker.sock:/x"]}`, []linter.Issue{ + {Path: "devcontainer.json", Line: 1, Col: 43, RuleID: "no-docker-socket-mount", + Message: `"runArgs" bind-mounts the Docker socket, which grants the container root-equivalent control over the host`}, + }}, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { diff --git a/rules/util.go b/rules/util.go index 0590d27..399c721 100644 --- a/rules/util.go +++ b/rules/util.go @@ -1,7 +1,9 @@ package rules import ( + "encoding/csv" "iter" + "path" "strings" "github.com/bare-devcontainer/decolint/linter" @@ -13,6 +15,16 @@ import ( // container even though other bind mounts are ignored. const dockerSocketPath = "/var/run/docker.sock" +// isDockerSocketSource reports whether source, a mount source, names the host's Docker socket. It is +// the single answer to that question, so that a rule reporting the socket and a rule excusing it +// never disagree about the same mount. +// +// The daemon cleans a bind mount's source before using it, so spellings such as +// "//var/run/docker.sock" and "/var/run/docker.sock/" all reach the same socket. +func isDockerSocketSource(source string) bool { + return path.Clean(source) == dockerSocketPath +} + // hasMember reports whether obj has a member named name. func hasMember(obj *hujson.Object, name string) bool { return memberNamed(obj, name) != nil @@ -64,53 +76,85 @@ func arrayMembers(obj *hujson.Object, name string) iter.Seq[*hujson.Array] { } } -// runArgsFindFlagValue scans arr, a "runArgs" array, for an entry that sets flag to a value accepted -// by match. Docker accepts such a value either as a single combined "flag=value" entry or as two -// adjacent entries, "flag" followed by "value". It returns the hujson.Value holding the matching -// value, or nil if no entry sets flag to a value match accepts. -func runArgsFindFlagValue(arr *hujson.Array, flag string, match func(string) bool) *hujson.Value { - for i := range arr.Elements { - lit, ok := arr.Elements[i].Value.(hujson.Literal) - if !ok || lit.Kind() != '"' { - continue - } +// runArgsFlagValues yields every value that arr, a "runArgs" array, gives to flag, in order. Docker +// accepts such a value either as a single combined "flag=value" entry or as two adjacent entries, +// "flag" followed by "value". Each yielded pair is the hujson.Value 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 i := range arr.Elements { + lit, ok := arr.Elements[i].Value.(hujson.Literal) + if !ok || lit.Kind() != '"' { + continue + } - if v, ok := strings.CutPrefix(lit.String(), flag+"="); ok { - if match(v) { - return &arr.Elements[i] + if v, ok := strings.CutPrefix(lit.String(), flag+"="); ok { + if !yield(&arr.Elements[i], v) { + return + } + continue } - continue - } - if lit.String() != flag || i+1 >= len(arr.Elements) { - continue + if lit.String() != flag || i+1 >= len(arr.Elements) { + continue + } + next, ok := arr.Elements[i+1].Value.(hujson.Literal) + if ok && next.Kind() == '"' && !yield(&arr.Elements[i+1], next.String()) { + return + } } - next, ok := arr.Elements[i+1].Value.(hujson.Literal) - if ok && next.Kind() == '"' && match(next.String()) { - return &arr.Elements[i+1] + } +} + +// 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 } -// parseMountString extracts the "type" and "source" fields from s, a "key=value,..." mount entry. +// 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 +// field may be quoted to protect a comma inside it; +// - field keys are matched case-insensitively, and "src" is an alias for "source"; +// - the type is normalized to lower case. +// +// It returns empty strings for a value that is not a well-formed CSV record, which docker rejects +// outright. +// +// Whitespace around a key or a value is the one deliberate deviation: docker rejects it, but reading +// such a field anyway lets the rules name the mount the author meant rather than fall silent on a +// value that already fails to start the container. func parseMountString(s string) (mountType, source string) { - for part := range strings.SplitSeq(s, ",") { - key, value, ok := strings.Cut(part, "=") + fields, err := csv.NewReader(strings.NewReader(strings.TrimSpace(s))).Read() + if err != nil { + return "", "" + } + for _, field := range fields { + key, value, ok := strings.Cut(field, "=") if !ok { continue } - switch strings.TrimSpace(key) { + value = strings.TrimSpace(value) + switch strings.ToLower(strings.TrimSpace(key)) { case "type": - mountType = strings.TrimSpace(value) - case "source": - source = strings.TrimSpace(value) + mountType = strings.ToLower(value) + case "source", "src": + source = value } } return mountType, source } -// parseMountObject extracts the "type" and "source" members from obj, a "mounts" entry. +// parseMountObject extracts the "type" and "source" members from obj, a "mounts" entry. The member +// names are the ones devcontainers/cli reads off the object and so are case-sensitive, but it hands +// their values to docker as a "--mount" value, which lower-cases the type. func parseMountObject(obj *hujson.Object) (mountType, source string) { for _, m := range obj.Members { name, ok := m.Name.Value.(hujson.Literal) @@ -123,7 +167,7 @@ func parseMountObject(obj *hujson.Object) (mountType, source string) { } switch name.String() { case "type": - mountType = value.String() + mountType = strings.ToLower(value.String()) case "source": source = value.String() } @@ -131,9 +175,21 @@ func parseMountObject(obj *hujson.Object) (mountType, source string) { return mountType, source } +// volumeSpecSource returns the host path or volume name that s, a "-v"/"--volume" value, mounts, or +// "" if s names no source. A volume spec shares no syntax with a "--mount" value: its fields are +// separated by colons and a comma is an ordinary character. A spec of a single field is an anonymous +// volume, and that field is the container path, not a host path. +func volumeSpecSource(s string) string { + source, _, ok := strings.Cut(s, ":") + if !ok { + return "" + } + return source +} + // parseMount extracts the "type" and "source" fields from v, a "mounts" entry, which may be either -// the "key=value,..." string shorthand or an object with corresponding members. ok is false if v is -// neither. +// the "--mount" string shorthand (see [parseMountString]) or an object with corresponding members. +// ok is false if v is neither. func parseMount(v *hujson.Value) (mountType, source string, ok bool) { switch val := v.Value.(type) { case hujson.Literal: