From a5a5d48437f83ad2044afe9656adfd341ae4bca1 Mon Sep 17 00:00:00 2001 From: John Viega Date: Thu, 5 Oct 2023 21:40:10 -0400 Subject: [PATCH] [fix] Enable (most) wrapping when seeing USER directive (#34) * Use COPY --chmod=0755 whenever possible when doing entrypoint wrapping. * Capture any USER command in a Dockerfile. If we're wrapping, and COPY --chmod is not available to us, and the last section has a USER section, then we wrap a manual chmod with USER root / USER * adding new section for 0.1.3 release notes --------- Co-authored-by: indecisivedragon Co-authored-by: Miroslav Shubernetskiy --- src/chalk_common.nim | 4 ++ src/commands/cmd_docker.nim | 2 + src/docker_base.nim | 45 +++++++++++++--- src/dockerfile.nim | 4 ++ src/docs/core-release-notes.md | 23 ++++++++ tests/chalk/runner.py | 13 ++++- tests/data/configs/docker_wrap.conf | 1 + .../dockerfiles/valid/sample_3/Dockerfile | 6 +++ .../valid/split_dockerfiles/base.Dockerfile | 6 +++ .../valid/split_dockerfiles/test.Dockerfile | 4 ++ tests/test_docker.py | 53 ++++++++++++++++++- tests/utils/docker.py | 8 ++- 12 files changed, 159 insertions(+), 10 deletions(-) create mode 100644 tests/data/configs/docker_wrap.conf create mode 100644 tests/data/dockerfiles/valid/sample_3/Dockerfile create mode 100644 tests/data/dockerfiles/valid/split_dockerfiles/base.Dockerfile create mode 100644 tests/data/dockerfiles/valid/split_dockerfiles/test.Dockerfile diff --git a/src/chalk_common.nim b/src/chalk_common.nim index a1d388a7..af795c58 100644 --- a/src/chalk_common.nim +++ b/src/chalk_common.nim @@ -229,6 +229,9 @@ type rawSrc*: seq[string] rawDst*: string + DfUserInfo* = ref object of InfoBase + str*: string + LabelInfo* = ref object of InfoBase labels*: OrderedTable[string, string] @@ -238,6 +241,7 @@ type entryPoint*: EntryPointInfo cmd*: CmdInfo shell*: ShellInfo + lastUser*: DfUserInfo DockerInvocation* = ref object dockerExe*: string diff --git a/src/commands/cmd_docker.nim b/src/commands/cmd_docker.nim index 75179c95..0608b18b 100644 --- a/src/commands/cmd_docker.nim +++ b/src/commands/cmd_docker.nim @@ -37,6 +37,8 @@ proc runMungedDockerInvocation*(ctx: DockerInvocation): int = trace("Running docker: " & dockerExeLocation & " " & args.join(" ")) if ctx.dfPassOnStdin: + if not ctx.inDockerFile.endswith("\n"): + ctx.inDockerFile &= "\n" newStdin = ctx.inDockerFile & ctx.addedInstructions.join("\n") trace("Passing on stdin: \n" & newStdin) diff --git a/src/docker_base.nim b/src/docker_base.nim index d1ec941e..1fd87443 100644 --- a/src/docker_base.nim +++ b/src/docker_base.nim @@ -12,6 +12,7 @@ import osproc, config, util, reporting var buildXVersion: float = 0 # Major and minor only + dockerVersion: string = "" const hashHeader* = "sha256:" @@ -62,6 +63,19 @@ proc getBuildXVersion*(): float = return buildXVersion +proc getDockerVersion*(): string = + once: + let (output, exitcode) = execCmdEx(dockerExeLocation & " version") + if exitcode == 0: + let words = output.split(" ") + + for item in words: + if '.' in item: + dockerVersion = item + break + + return dockerVersion + template haveBuildContextFlag*(): bool = buildXVersion >= 0.8 @@ -105,12 +119,15 @@ proc makeFileAvailableToDocker*(ctx: DockerInvocation, once: trace("Docker injection method: --build-context") + var chmodstr = "" + + if chmod: + chmodstr = "--chmod=0755 " + ctx.newCmdLine.add("--build-context") ctx.newCmdLine.add("chalkexedir" & $(contextCounter) & "=\"" & dir & "\"") - ctx.addedInstructions.add("COPY --from=chalkexedir" & $(contextCounter) & - " " & file & " /" & newname) - if chmod: - ctx.addedInstructions.add("RUN chmod 0755 /" & newname) + ctx.addedInstructions.add("COPY " & chmodstr & "--from=chalkexedir" & + $(contextCounter) & " " & file & " /" & newname) contextCounter += 1 if move: registerTempFile(loc) @@ -138,10 +155,24 @@ proc makeFileAvailableToDocker*(ctx: DockerInvocation, copyFile(loc, dstLoc) trace("Copied " & loc & " to " & dstLoc) - ctx.addedInstructions.add("COPY " & file & " " & " /" & newname) - if chmod: + if chmod and getDockerVersion().startswith("2") and + getBuildXVersion() > 0: + ctx.addedInstructions.add("COPY --chmod=0755 " & file & " " & " /" & + newname) + elif chmod: + let useDirective = ctx.dfSections[^1].lastUser + + # TODO detect user from base image if possible but thats not + # trivial as what is a base image is not a trivial question + # due to multi-stage build possibilities... + if useDirective != nil: + ctx.addedInstructions.add("USER root") + ctx.addedInstructions.add("COPY " & file & " " & " /" & newname) ctx.addedInstructions.add("RUN chmod 0755 /" & newname) - + if useDirective != nil: + ctx.addedInstructions.add("USER " & useDirective.str) + else: + ctx.addedInstructions.add("COPY " & file & " " & " /" & newname) registerTempFile(dstLoc) except: diff --git a/src/dockerfile.nim b/src/dockerfile.nim index e3cebfeb..29f53f2b 100644 --- a/src/dockerfile.nim +++ b/src/dockerfile.nim @@ -788,6 +788,8 @@ proc parseAndEval*(s: Stream, of "CMD": firstFromCheck() res.add(parse.parseCmd(cmd)) + of "USER": + res.add(DfUserInfo(str: unicode.strip(cmd.rawArg))) of "ONBUILD": firstFromCheck() res.add(parse.parseOnBuild(cmd)) @@ -855,6 +857,8 @@ proc evalAndExtractDockerfile*(ctx: DockerInvocation) = section.cmd = CmdInfo(obj) elif obj of ShellInfo: section.shell = ShellInfo(obj) + elif obj of DfUserInfo: + section.lastUser = DfUserInfo(obj) elif obj of LabelInfo: for k, v in LabelInfo(obj).labels: labels[k] = v diff --git a/src/docs/core-release-notes.md b/src/docs/core-release-notes.md index 44544e0e..2da0b4ab 100644 --- a/src/docs/core-release-notes.md +++ b/src/docs/core-release-notes.md @@ -1,3 +1,26 @@ +# Release Notes for Chalk version 0.1.3 + +## Fixes + +- Docker support when installed via `Snap`. + [9](https://github.com/crashappsec/chalk/pull/9) +- Removes error log when using chalk on ARM Linux as chalk fully + runs on ARM Linux now. + [7](https://github.com/crashappsec/chalk/pull/7) +- When a `Dockerfile` uses `USER` directive, chalk can now wrap + entrypoint in that image + (`docker.wrap_entrypoint = true` in chalk config). + [34](https://github.com/crashappsec/chalk/pull/34) + +## Known Issues + +### Containers + +- When a `Dockerfile` does not use `USER` directive but base image + uses it to change default image user, chalk cannot wrap the + image as it on legacy Docker builder (not buildx) as it will + fail to `chmod` permissions of chalk during the build. + # Release Notes for Chalk version 0.1.2 This is the first open source release of Chalk. For those who diff --git a/tests/chalk/runner.py b/tests/chalk/runner.py index 581e0762..8329bb62 100644 --- a/tests/chalk/runner.py +++ b/tests/chalk/runner.py @@ -345,8 +345,10 @@ def docker_build( tag: Optional[str] = None, context: Optional[Path] = None, expected_success: bool = True, + expecting_report: bool = True, virtual: bool = False, cwd: Optional[Path] = None, + args: Optional[dict[str, str]] = None, config: Optional[Path] = None, buildkit: bool = True, ) -> tuple[str, ChalkProgram]: @@ -358,6 +360,7 @@ def docker_build( tag=tag, context=context, dockerfile=dockerfile, + args=args, cwd=cwd, expected_success=expected_success, buildkit=buildkit, @@ -375,6 +378,7 @@ def docker_build( tag=tag, context=context, dockerfile=dockerfile, + args=args, ), expected_success=expected_success, cwd=cwd, @@ -384,11 +388,18 @@ def docker_build( dockerfile = dockerfile or ( (cwd or context or Path(os.getcwd())) / "Dockerfile" ) - if expected_success: + if expecting_report and expected_success: # sanity check that chalk mark includes basic chalk keys assert image_hash == result.mark["_CURRENT_HASH"] assert image_hash == result.mark["_IMAGE_ID"] assert str(dockerfile) == result.mark["DOCKERFILE_PATH"] + elif not expecting_report: + try: + assert not result.reports + except json.JSONDecodeError: + # we are not expecting any report json to be present in output + # so this exception is expected here + pass return image_hash, result def docker_push(self, image: str): diff --git a/tests/data/configs/docker_wrap.conf b/tests/data/configs/docker_wrap.conf new file mode 100644 index 00000000..d9dd159a --- /dev/null +++ b/tests/data/configs/docker_wrap.conf @@ -0,0 +1 @@ +docker.wrap_entrypoint = true diff --git a/tests/data/dockerfiles/valid/sample_3/Dockerfile b/tests/data/dockerfiles/valid/sample_3/Dockerfile new file mode 100644 index 00000000..3392f5c0 --- /dev/null +++ b/tests/data/dockerfiles/valid/sample_3/Dockerfile @@ -0,0 +1,6 @@ +FROM alpine as base + +RUN adduser -D testuser +RUN id +USER testuser +RUN id diff --git a/tests/data/dockerfiles/valid/split_dockerfiles/base.Dockerfile b/tests/data/dockerfiles/valid/split_dockerfiles/base.Dockerfile new file mode 100644 index 00000000..3392f5c0 --- /dev/null +++ b/tests/data/dockerfiles/valid/split_dockerfiles/base.Dockerfile @@ -0,0 +1,6 @@ +FROM alpine as base + +RUN adduser -D testuser +RUN id +USER testuser +RUN id diff --git a/tests/data/dockerfiles/valid/split_dockerfiles/test.Dockerfile b/tests/data/dockerfiles/valid/split_dockerfiles/test.Dockerfile new file mode 100644 index 00000000..18bab021 --- /dev/null +++ b/tests/data/dockerfiles/valid/split_dockerfiles/test.Dockerfile @@ -0,0 +1,4 @@ +ARG BASE +FROM $BASE + +ENTRYPOINT ["/bin/bash", "-c"] diff --git a/tests/test_docker.py b/tests/test_docker.py index dc8fa704..0adaaadb 100644 --- a/tests/test_docker.py +++ b/tests/test_docker.py @@ -22,6 +22,7 @@ from .conf import CONFIGS, DOCKERFILES, REGISTRY from .utils.docker import Docker from .utils.log import get_logger +from .utils.os import run logger = get_logger() @@ -69,6 +70,10 @@ def run_container(*args, **kwargs): (DOCKERFILES / "valid" / "sample_1", None, False), # PWD=foo && docker build -t test . (DOCKERFILES / "valid" / "sample_1", None, True), + # PWD=foo && docker build . + (DOCKERFILES / "valid" / "sample_3", None, False), + # PWD=foo && docker build -t test . + (DOCKERFILES / "valid" / "sample_3", None, True), # docker build -f foo/Dockerfile foo (None, DOCKERFILES / "valid" / "sample_1" / "Dockerfile", False), # docker build -f foo/Dockerfile -t test foo @@ -91,16 +96,59 @@ def test_build( cwd=cwd, tag=random_hex if tag else None, buildkit=buildkit, + config=CONFIGS / "docker_wrap.conf", ) assert image_id +@pytest.mark.parametrize("buildkit", [True, False]) +@pytest.mark.parametrize( + "base, test", + [ + ( + DOCKERFILES / "valid" / "split_dockerfiles" / "base.Dockerfile", + DOCKERFILES / "valid" / "split_dockerfiles" / "test.Dockerfile", + ), + ], +) +def test_composite_build( + chalk: Chalk, + base: Path, + test: Path, + buildkit: bool, + random_hex: str, +): + image_id, _ = Docker.build( + dockerfile=base, + buildkit=buildkit, + tag=random_hex, + ) + assert image_id + + # TODO this is a known limitation for the moment + # we EXPECT this case to fail without buildkit enabled, + # as base image adjusts USER and child Dockerfile + # does not have any indication that USER was adjusted + # and so we cannot detect USER from base image. + # In this case chalk falls back to standard docker build + # which means there is no chalk report in the output + second_image_id, result = chalk.docker_build( + dockerfile=test, + buildkit=buildkit, + args={"BASE": random_hex}, + config=CONFIGS / "docker_wrap.conf", + expecting_report=buildkit, + ) + assert second_image_id + + @mock.patch.dict(os.environ, {"SINK_TEST_OUTPUT_FILE": "/tmp/sink_file.json"}) @pytest.mark.parametrize( "test_file", [ "valid/sample_1", "valid/sample_2", + "valid/sample_3", ], ) def test_virtual_valid( @@ -173,12 +221,15 @@ def test_virtual_invalid( ).is_file(), "virtual-chalk.json should not have been created!" -@pytest.mark.parametrize("test_file", ["valid/sample_1", "valid/sample_2"]) +@pytest.mark.parametrize( + "test_file", ["valid/sample_1", "valid/sample_2", "valid/sample_3"] +) def test_nonvirtual_valid(chalk: Chalk, test_file: str, random_hex: str): tag = f"{test_file}_{random_hex}" image_hash, build = chalk.docker_build( dockerfile=DOCKERFILES / test_file / "Dockerfile", tag=tag, + config=CONFIGS / "docker_wrap.conf", ) # artifact is the docker image diff --git a/tests/utils/docker.py b/tests/utils/docker.py index 1aaf4bfb..2c62e68a 100644 --- a/tests/utils/docker.py +++ b/tests/utils/docker.py @@ -21,12 +21,15 @@ def build_cmd( tag: Optional[str], context: Optional[Path] = None, dockerfile: Optional[Path] = None, + args: Optional[dict[str, str]] = None, ): cmd = ["docker", "build"] if tag: cmd += ["-t", tag] if dockerfile: cmd += ["-f", str(dockerfile)] + for name, value in (args or {}).items(): + cmd += [f"--build-arg={name}={value}"] cmd += [str(context or ".")] return cmd @@ -36,6 +39,7 @@ def build( tag: Optional[str] = None, context: Optional[Path] = None, dockerfile: Optional[Path] = None, + args: Optional[dict[str, str]] = None, cwd: Optional[Path] = None, expected_success: bool = True, buildkit: bool = True, @@ -45,7 +49,9 @@ def build( """ return Docker.with_image_id( run( - Docker.build_cmd(tag=tag, context=context, dockerfile=dockerfile), + Docker.build_cmd( + tag=tag, context=context, dockerfile=dockerfile, args=args + ), expected_exit_code=int(not expected_success), env=Docker.build_env(buildkit=buildkit), cwd=cwd,