Skip to content

Commit

Permalink
fix: set PATH on bare-based rocks
Browse files Browse the repository at this point in the history
If the PATH is empty, Pebble will be default use the standard Ubuntu value
of "/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin" (see [0]).
Follow this lead and set this value on the image's PATH.

This PATH setting on the image itself has no bearing on most cases, as the
PATH that prevails is the one defined by Pebble and its services, but an
empty (or unset) PATH is a potential security issue in cases where the pebble
entrypoint is bypassed.

0: https://github.com/canonical/pebble/blob/master/internals/overlord/cmdstate/request.go#L91

Fixes #711
  • Loading branch information
tigarmo committed Oct 8, 2024
1 parent 1d87e33 commit b7608e0
Show file tree
Hide file tree
Showing 5 changed files with 79 additions and 2 deletions.
16 changes: 16 additions & 0 deletions rockcraft/oci.py
Original file line number Diff line number Diff line change
Expand Up @@ -389,6 +389,22 @@ def set_cmd(self, command: str | None = None) -> None:
_config_image(image_path, cmd_params)
emit.progress(f"CMD set to {opt_args}")

def set_path(self, base: str, build_base: str | None) -> None:
"""Set the default PATH on the image (only for bare rocks)."""
if base != "bare":
emit.debug(f"Not setting a PATH on the image as base is {base!r}")
return

# Follow Pebble's lead here: if PATH is empty, use the standard one.
# This means that containers that bypass the pebble entrypoint will
# have the same behavior as PATH-less pebble services.
# (https://github.com/canonical/pebble/blob/master/internals/overlord/cmdstate/request.go#L91)
pebble_path = "/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin"
image_path = self.path / self.image_name

emit.debug(f"Setting bare-based rock PATH to {pebble_path!r}")
_config_image(image_path, ["--config.env", f"PATH={pebble_path}"])

def set_pebble_layer(
self,
services: dict[str, Any],
Expand Down
2 changes: 2 additions & 0 deletions rockcraft/services/package.py
Original file line number Diff line number Diff line change
Expand Up @@ -157,6 +157,8 @@ def _pack(
if project.services and project.entrypoint_service in project.services:
new_image.set_cmd(project.services[project.entrypoint_service].command)

new_image.set_path(project.base, project.build_base)

dumped = project.marshal()
services = cast(dict[str, typing.Any], dumped.get("services", {}))
checks = cast(dict[str, typing.Any], dumped.get("checks", {}))
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ version: latest
summary: A tiny rock
description: Building a tiny rock from a bare base, with just one package
license: Apache-2.0
build-base: [email protected]
build-base: placeholder-base
base: bare
services:
hello:
Expand Down
15 changes: 14 additions & 1 deletion tests/spread/rockcraft/bare-base/task.yaml
Original file line number Diff line number Diff line change
@@ -1,6 +1,14 @@
summary: bare base build test
environment:
BASE/24_04: "[email protected]"
PEBBLE_DIR/24_04: "/usr/bin"

BASE/22_04: "[email protected]"
PEBBLE_DIR/22_04: "/bin"

execute: |
sed "s/placeholder-base/$BASE/" rockcraft.orig.yaml > rockcraft.yaml
run_rockcraft pack
test -f bare-base-test_latest_amd64.rock
Expand All @@ -15,6 +23,11 @@ execute: |
grep_docker_log "$id" "ship it!"
docker exec "$id" pebble services | grep hello
docker exec "$id" pebble ls /usr/bin/hello
test "$(docker inspect "$id" -f '{{json .Config.Entrypoint}}')" = '["/bin/pebble","enter"]'
test "$(docker inspect "$id" -f '{{json .Config.Entrypoint}}')" = "[\"${PEBBLE_DIR}/pebble\",\"enter\"]"
# Bare-based rocks should have the default Ubuntu path, as an empty PATH is a
# security issue when containers bypass the pebble entrypoint.
EXPECTED_PATH="/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin"
test "$(docker inspect "$id" -f '{{json .Config.Env}}')" = "[\"PATH=${EXPECTED_PATH}\"]"
docker rm -f "$id"
46 changes: 46 additions & 0 deletions tests/unit/test_oci.py
Original file line number Diff line number Diff line change
Expand Up @@ -951,3 +951,49 @@ def test_stat(self, new_dir, mock_run, mocker):
)
]
assert mock_loads.called

@pytest.mark.parametrize(
"build_base",
[
"[email protected]",
"ubuntu@devel",
"[email protected]",
"[email protected]",
],
)
def test_set_path_bare(self, mock_run, build_base):
image = oci.Image("a:b", Path("/c"))

image.set_path("bare", build_base)

expected = "/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin"
assert mock_run.mock_calls == [
call(
[
"umoci",
"config",
"--image",
"/c/a:b",
"--config.env",
f"PATH={expected}",
]
)
]

@pytest.mark.parametrize(
["base", "build_base"],
[
("[email protected]", "[email protected]"),
("[email protected]", None),
("[email protected]", "[email protected]"),
("[email protected]", None),
("[email protected]", "[email protected]"),
("[email protected]", None),
],
)
def test_set_path_non_bare(self, mock_run, base, build_base):
image = oci.Image("a:b", Path("/c"))

image.set_path(base, build_base)

assert not mock_run.called

0 comments on commit b7608e0

Please sign in to comment.