[simonbrauner]

Checklist

Ensure you have completed the following checklist for your pull request to be reviewed:

• I have read and understood our contributing guidelines and will not have more than two open PRs as a new contributor.
• PR description, commit message, and GitHub comments are human-written, per LLM Policy
• Certify you wrote the patch or otherwise have the right to pass it on as an open-source patch by including your Signed-off-by line in all
  commits. (git commit -s). (If needed, use git commit -s --amend). The author email must match
  the sign-off email address. Read the DCO Sign-off requirements
  and ensure you follow them before signing off your commits.
• Referenced issues using Fixes: #00000 in commit message (if applicable)
• Tests have been added/updated (or no tests are needed)
• Documentation has been updated (or no documentation changes are needed)
• All commits pass make validatepr (format/lint checks)
• Release note entered in the section below (or None if no user-facing changes)

Does this PR introduce a user-facing change?

None

[simonbrauner]

Proof of concept: #29871

[simonbrauner]

The constructor should not allow path to be an empty string, as that would allow two representations of the absence of a root, and an empty string may break the method calls later. Is it sensible to panic on such an attempt?

[mtrmac]

I don’t feel strongly about this.

I think panicking would be good enough, but also… returning an explicit error might be easy enough to be worth doing anyway?

Arguably, raw calls to NewPathRoot should be fairly rare — ideally they would be wrapped in things like newRootInTmpDirForBigFiles, mountContainer, or whatever; such operations do I/O and thus already need an error return.

[simonbrauner]

Sure, I am okay with returning an error as well.

Calls to NewPathRoot should be fairly rare eventually, and the sites do work with errors already. During the migration the density of NewPathRoot will grow temporarily.

Also, even if we do return nil, err, and somebody forgets to check the error, then calling a method on nil will panic afterwards, so the panic will still be there.

[simonbrauner]

PTAL @podman-container-tools/podman-maintainers @podman-container-tools/podman-reviewers @mtrmac @Luap99 @mheon @TomSweeneyRedHat @giuseppe

[packit-as-a-service]

[NON-BLOCKING] Packit jobs failed. @podman-container-tools/packit-jobs please check. Everyone else, feel free to ignore.

[packit-as-a-service]

[NON-BLOCKING] Packit jobs failed. @podman-container-tools/packit-jobs please check. Everyone else, feel free to ignore.

[giuseppe]

thanks for opening the PR, I've left one question and one suggestion

[giuseppe]

portability is nice, but Linux containers are the main focus. If a feature exists for Linux we should use it, openat2 is old enough that we can just assume it is everywhere we care about.

The difficulties I see with fd-based operations is that you need to use /proc/self/fd/$fd for syscalls where a path is required, and preventing leaks; but the net benefits outweigh these two difficulties IMO.

[mtrmac]

I think conceptually it is a bit problematic to have a “secure root context that should be used everywhere, but it is Linux only” — it makes it much harder to develop the habit.

---

On /proc, we’ve run into https://github.com/podman-container-tools/container-libs/blob/88ea1e7783ab0dae0e68ce63c5f777692727a441/storage/internal/xattrs/xattrs_linux.go#L86-L89 vs. https://github.com/podman-container-tools/container-libs/blob/88ea1e7783ab0dae0e68ce63c5f777692727a441/storage/pkg/archive/archive.go#L594-L600 .

Admittedly

• many callers who do I/O don’t need xattrs (but, again, harder to develop the habit)
• if we had reliable and proven handle-based tree walking, we wouldn’t really need the chroot in the first place. But “reliable and proven” happens over time.

[simonbrauner]

portability is nice, but Linux containers are the main focus. If a feature exists for Linux we should use it, openat2 is old enough that we can just assume it is everywhere we care about.

I agree Linux should be the main focus, and Linux-specific code should use openat2, but currently a lot of code is not Linux-specific. In the libpod pilot, most of the files have //go:build !remote && (linux || freebsd) tag, so if we just use openat2, we break FreeBSD. The options are:

a) Refactor the code so that everything that can support openat2 moves into _linux.go.
b) Have a root type which uses openat2 on Linux, but falls back to only static path traversal security guarantees on other platforms.

And in terms of effort and sustainability, b) (which in the doc corresponds to the Racy variants), seems more suitable to me. As the refactoring effort is lower, we do not split code into multiple parts which we would need to maintain separately, and hypothetically, if FreeBSD does support openat2 or something similar eventually, we can just swap the function calls and the implementation travels through the methods across all the usages.

Not to mention that there would probably be other platforms to support in other parts of the codebase, or later in time, so the overhead of a) would grow with that as well.

The difficulties I see with fd-based operations is that you need to use /proc/self/fd/$fd for syscalls where a path is required

In addition to mtrmac's point, the /proc trick is also limited to Go functions that do not touch the string and just pass it to a syscall, while in my argumentation I mostly thought of cases (both internal unmigrated and third-party) that depend on the path being an actual path because they use the string for some parsing or manipulation.

I think conceptually it is a bit problematic to have a “secure root context that should be used everywhere, but it is Linux only” — it makes it much harder to develop the habit.

This is why I think PathRoot is the right default. It can be used everywhere and the habit holds across platforms. The Racy functions would then give us the benefit of being able to use openat2 while staying honest about the overall guarantees and allowing us to upgrade them once we have something better than the fallback on some platform.

if we had reliable and proven handle-based tree walking, we wouldn’t really need the chroot in the first place. But “reliable and proven” happens over time.

Yes, this is something the library could also support.

[giuseppe]

are there cases where we really want the filepath.Join semantic?

Beside a negligible performance benefit, would there be any downside if we replace any filepath.Join with securejoin.SecureJoin and add a CI job that prevents we ever add it back again?

[mtrmac]

Yes, I think we want the filepath.Join semantics for filepath.Join("/etc/containers/$config.d", entryFromReaddir) and probably for things like filepath.Join(userProvidedPath, "some-file-in-that-structure"). User-provided paths are not inherently restricted to physical subdirectories, and using symlinks might be a plausible way to manage duplication.

I feel fairly strongly about this for /etc; in other situations, it’s less clear whether “escaping” symlinks are acceptable (e.g. for the buildah context directory).

---

• There’s the elephant in the room that filepath.Join and Linux file resolution semantics don’t match for symlink/... I suppose we are going to ignore it just like everyone else, I don’t know what else to practically do.
• In some situations, the performance can be pretty visible ( podman build layer finalization slows in 5.8.7 vs 5.8.4 with fuse-overlayfs in Kubernetes #29833 ). I don’t know how rare it is.

[simonbrauner]

are there cases where we really want the filepath.Join semantic?

Beside a negligible performance benefit, would there be any downside if we replace any filepath.Join with securejoin.SecureJoin and add a CI job that prevents we ever add it back again?

filepath.Join has the semantics of join those two paths, regardless of what is on the filesystem, while securejoin.SecureJoin has the left argument is a root we want to confine the right argument to with RESOLVE_IN_ROOT semantics, in a way that the path returned does not contain any symlinks. So the problems of global replacement I can think of are:

• Breaking calls such as filepath.Join("somedir", "..")
• Breaking the code that needs the symlinks to stay
• Breaking cases where the path isn't accessible (yet)
• Possibly breaking the code that has no confinement requirements whatsoever
• Possible inconsistencies when we keep using the resolved path after the filesystem has changed

The error propagation would also take some effort, and I would assume it wouldn't be much easier than supporting the minimal variant of the PathRoot altogether.

Also, filepath.Join is the typical example, but there are other options of insecure manipulation, such as + operator or Sprintf. And I think the situation isn't that "this function is insecure, this function is secure", but rather "this is a root and we should confine against it", so the targeted replacement for the call that matters with compiler guarantees that we don't miss anything seems more robust than banning functions.

On static analysis

I did think of using thorough static analysis, since finding the roots is not a problem, the values derived could be tracked, so we could see if they (or their derived values) would reach something like filepath.Join eventually, and the advantage would be that we wouldn't need to change the code much, but my guess is that setting the analysis up and managing the whole machinery of marking roots, deciding what is and is not secure, handling false positives/false negatives, and such would take comparable effort to the migration. And overall, it would probably not realistically match the maturity and the usability which the compiler and existing developer tooling already has.

[mtrmac]

The constructor itself could perform some verification/preprocessing,

Per the “panic” conversation… that could be a reason to add an error return value now.

[simonbrauner]

Agreed, I will add the error return value and an error on an attempt to construct an empty root. That way the error is there, if the caller wants to handle it somehow, they may, and even if they ignore the error, method usage will panic anyway so it will be noticed.

[mtrmac]

A general question: Do we want this to be a stable API for out-of-project callers?

It would be easier to maintain/improve if it were explicitly unstable and we could just fix things, so I’d prefer to start that way. OTOH if there are known situations where we would want to use it in public API (bindings? c/storage?), that would be good to know.

[simonbrauner]

I think eventually it should be reusable across podman, buildah, and c/storage. And particularly the calls in c/storage which produce roots could return the type already so that we don't need to write code as in https://github.com/simonbrauner/podman/blob/c6d57bb4f968f6d6f38786a51f17daf56f6e32ff/libpod/container_internal.go#L783-L787. If we don't want to break the original function, we can add a new variants alongside the existing functions.

In the early stages of adoption, marking it explicitly as unstable seems right. My idea is to place it in the place of first migration (in the proof of concept this was Podman) so that it can be changed quickly without cross-repository PRs and vendoring in between.

Whether and when to promise stability for out-of-project callers, I don't know, probably not until the library is well established across our codebases.

[mtrmac]

I understand how the Todo/Deferred distinction is valuable when working on a single small project, e.g. on the proof-of-concept… but I’m not sure what it means globally. E.g. if I see a subpackage last changed 1 year ago, and some calls are Todo and some are Deferred, what does that mean?

---

If that “single small project” is a single PR, the “probably-in-scope” / “definitely-out-of-scope” distinction can be managed just by adding temporary private comments (“mitr:FIXME” or whatever), and checking that none remain before the PR is submitted.

[simonbrauner]

My idea was:

• No prefix -> this is an evaluated security-aware decision
• Deferred -> this is the compatibility layer between migrated and unmigrated part of code, considered out of scope for its merging PR. What is the next migration step after that PR can be decided later according to prioritizataion.
• Todo -> this is a temporary construct to help with step by step migration within a single PR, so that it is easy to mark what still needs work while the code compiles, it should not reach main.

Technically they can be comments, and I am open to replacing them with comments, it's just that comments can be misstyped or forgotten or removed in refactoring or outdated, so at least for me personally it feels a little more systematic and convenient to work with method names.

[mtrmac]

I don’t think the Todo variants are truly necessary but I’m fine with having them. The worry about typos in comments is fair.

(Would we want a CI test that ensures PRs don’t merge with Todo?)

[simonbrauner]

Right, if Todo is only for the purpose of individual PRs, then they do not need to be persistently in the library. If I want to work with them, I can implement them as internal helpers, use them for the migration, and remove them from the PR altogether once the PR is ready for review. That way the users of the library will have only the no-prefix/Deferred to choose from, and we won't have to worry about forgotten Todo variants getting merged.

[simonbrauner]

I updated the doc based on discussions there and in the proof of concept. Specifically:

• Constructor returns a value rather than a pointer (pointer can still be used in fields to express optionality)
• Constructor returns an error when called with empty path
• Method calls on a root with an empty path panic
• Library is deliberately unstable
• Removed Todo method suffix markers

[simonbrauner]

Ready for further comments, if any.

I'm also open to suggestions regarding the choice of the root for the initial stage of the migration.

[mtrmac]

LGTM.