compat: confine dockerfile to build context#28072
compat: confine dockerfile to build context#280721seal wants to merge 4 commits intocontainers:mainfrom
Conversation
The compat build handler accepts a dockerfile query parameter and turns it into a filesystem path. Ensure the resolved path cannot escape the extracted build context directory, rejecting traversal and absolute paths outside the context. Add a linux-only unit test covering confinement checks. Signed-off-by: 1seal <security@1seal.org>
Luap99
left a comment
There was a problem hiding this comment.
I have not made a careful look on what is going but this is breaking many tests depending on the current behavior, seems like there are many tests checking that we can use dockerfiles outside of the context dir so I am not sure we can or should switch that as it could break many users.
|
thanks for the review. i agree this is an API change and will break any callers/tests that relied on the compat handler reading a dockerfile outside the provided build context. the motivation for the change is twofold:
to reduce user impact, i think the right direction is:
if we have tests asserting “dockerfile outside context works”, i suggest updating them to build a context that includes the intended dockerfile (client-side), then pass a context-relative path. this pr already allows absolute paths that still resolve within |
|
I don't see why we need this limitation, there is nothing harmful in a Dockerfile outside of the build context |
|
thanks. i’m not trying to introduce a new security boundary here (agree with the system service threat model). the main reasons for confining
local |
|
pushed an update to address the ci failures caused by the stricter dockerfile confinement. changes in this commit:
hopefully this unblocks the remote/system build tests that were previously relying on absolute host dockerfile paths. |
Signed-off-by: 1seal <security@1seal.org>
3ee8bdb to
701b6aa
Compare
|
pushed a dco-signed amend for the latest commit (no functional changes). |
Signed-off-by: 1seal <security@1seal.org>
|
follow-up: tightened server-side scoping so confinement applies to |
|
thanks. fyi that packit/tmt failure was for an earlier commit ( once those finish, happy to take another look if anything is still failing. |
Signed-off-by: 1seal <security@1seal.org>
|
minor follow-up: fixed the linux unit test to use a path-only request target (previously used an absolute URL, which caused |
|
I checked the code and it looks OK, but I cannot judge if the feature makes a sense in general. That's up to others :-). |
|
/packit retest-failed |
|
A friendly reminder that this PR had no activity for 30 days. |
|
stale bot pinged this, so a brief follow-up. current head narrows the server-side confinement to the remote build handlers ( if maintainers prefer a different shape here, i can split this further, adjust tests again, or close it. otherwise, is there a preferred next step or decision on whether this hardening/compat change should move forward? |
this fixes the compat build handler's
dockerfilequery parameter resolution to ensure the resolved path cannot escape the extracted build context directory.../traversal escapesContextDirectory(still allows absolute paths within the context for libpod/local build semantics)this is being filed as a bug/hardening change (not a security issue) per maintainer guidance on the
podman system servicethreat model.