proposal: os: convenience function for one-off Root operations

[neild]

I propose adding the following function:

package os

// InRoot returns a Root that operates on the named directory.
//
// The directory is opened anew at the start of every operation on the Root.
// If the directory cannot be opened, the operation returns an error.
// The directory is closed after every operation. Calling Close on the Root does nothing.
//
// InRoot may be used for one-off operations rooted in a directory.
// For example:
//
//    // Remove name in dir.
//    err := os.InRoot(dir).Remove(name)
func InRoot(name string) *Root

os.Root (#67002) supports performing filesystem operations within the context of some directory.

Performing a single operation in a Root is substantially more verbose than doing so outside of one. Compare:

f, err := os.Create(filepath.Join(base, name))

root, err := os.OpenRoot(base)
if err != nil {
  return err
}
defer root.Close()
f, err := root.Create(name)

If the Root will be reused for many operations, the additional overhead of opening and closing it is not so significant. For a single operation, however, it's enough to be annoying.

We have an os.OpenInRoot convenience function for opening files, but this covers only one operation. We could add similar functions for other operations (os.CreateInRoot, os.RemoveInRoot, etc.), but that would be a lot of new functions.

This has come up in the context of #73126, which proposes adding Root.ReadFile and Root.WriteFile methods: #73126 (comment) asks if we can have top-level convenience functions as well, since opening and closing the root for short operations is inconvenient.

I've also had this raised in a Google-internal discussion: We have a linter which recommends using the github.com/google/safeopen package for certain operations (recommending, for example, that os.Create(filepath.Join(a, b)) could be safeopen.CreateBeneath(a, b)). We're reluctant to change the linter to recommend os.Root in cases where a one-liner becomes several lines opening and closing a Root.

The InRoot function allows performing operations in a root with slightly less verbosity than the non-rooted equivalent using filepath.Join. To continue the above example:

f, err := os.InRoot(base).Create(name)

[gabyhelp]

Related Documentation

• Traversal-resistant file APIs > Introducing os.Root

_{(Emoji vote if this was helpful or unhelpful; more detailed feedback welcome in this discussion.)}

[earthboundkid]

Does it make sense to reuse the *os.Root type or should this be a new type like "OneOffRoot" "FallibleRoot" "MaybeRoot" "RootMonad" etc.? It does seem odd that there's no programmatic way to tell if a Root is a one-off Root or a normal one. Another option would be to have a public AlwaysReopen bool field on the struct.

[magical]

@earthboundkid It sounds like a goal of this proposal is to minimize the amount of added API, in which case adding a new single-purpose type which duplicates all the methods of Root seems like it would run counter to that goal. If we're ok with that then we may as well just add top-level FooInRoot functions instead.

[earthboundkid]

The more I think about it, the more I like the AlwayReopen bool idea. You might want to always reopen even if you had a normal root. And it solves the problem with platforms that don't have native roots.

[apparentlymart]

As I read just the original proposal text, before reading the subsequent discussion about how it might actually work, the mental model I developed was:

• An os.Root object can be in one of three states: pre-open, opened, closed.
• The result of os.OpenRoot is an opened root. The result of os.InRoot is a pre-open root.
• An os.Root that is in the pre-open state supports all of the same methods as an opened root, but implicitly transitions to the "opened" state on the first call -- as if calling os.OpenRoot "in-place" -- returning an error if it fails to do so. If there is any subsequent method call then it behaves the same as if the Root had been created with os.OpenRoot.
• An opened root cannot return to being pre-open.
• Calling Close on either a pre-open or opened root moves it to the "closed" state. A closed root cannot return to either of the prior states.

Under that mental model, there could potentially be a new method that somehow reports which of the three states the root is currently in¹ but I'm not sure I understand the use-case of either moving back from "opened" to "pre-open" or, alternatively, forcing the root to stay in the "pre-open" state after a single successful method call has complete made on it (which is how I understood this "AlwaysReopen" idea).

EDIT: Of course immediately after posting this comment I realized what I was missing: under what I described, you'd presumably still need to call Close at some point, which means the root would need to be stored in a variable. So remaining in the "opened" state after the first call would not actually solve the problem.

Footnotes

1. (though I'm not sure it's actually needed if transitioning from pre-open to open is automatic as I described above) ↩

[earthboundkid]

In the scenario of having AlwaysReopen bool, when AlwaysReopen == true then Root.Close would be a noop because the Root would always close after any other method call.

AlwaysReopen doesn't necessarily have to be a public field, but this proposal requires some equivalent private fields, so there should be either a method or a field that lets you get at that state IMO.

[earthboundkid]

And it solves the problem with platforms that don't have native roots.

To elaborate on this, JS/Plan 9 could return a Root with AlwaysReopen set even from OpenRoot, not just InRoot, so you would know the root is resetting on every use (which lessens its security guarantees).

[neild]

The goal of this proposal is to make it simple to replace an unsafe one-line file operation using filepath.Join with a safe one-line operation using os.Root. For example:

// unsafe
f, err := os.Create(filepath.Join(baseDir, untrustedPath))

// safer
f, err := os.InRoot(baseDir).Create(untrustedPath)

The idea behind InRoot is that it lets you create a one-off, disposable *os.Root that doesn't need to be closed, providing the equivalent of os.OpenInRoot for every operation on a Root without the need to define new top-level functions for each. While we need to specify what happens if you hang on to the result of os.InRoot and reuse it across multiple operations, that's not really a case I'm aiming to support and I'd much prefer to not add additional complexity to support it.

Another option would be to have a public AlwaysReopen bool field on the struct.

I don't understand how AlwaysReopen is supposed to work. Is it a field? A method? How do you set it? Why do you want to examine it?

To elaborate on this, JS/Plan 9 could return a Root with AlwaysReopen set even from OpenRoot, not just InRoot, so you would know the root is resetting on every use (which lessens its security guarantees).

The main lessening in security guarantees is on GOOS=js, where the lack of openat or any equivalent means we can't defend against TOCTOU races in symlink resolution. That's related to but not the same as the fact that we re-resolve the root path itself on each operation.

[earthboundkid]

Why do you want to examine it?

If a function took a Root, it might want to how it behaves, since there's a difference between all operations being guaranteed to be in the same directory and it not being guaranteed if the directory is moved/renamed.

[apparentlymart]

Although in my earlier comment I missed the detail about how the thing gets closed, in retrospect I think that mental model was still mostly okay, with some adjustments:

• An os.Root object can be in one of three states: open-when-needed, opened, or closed.
• The result of os.OpenRoot is an opened root. The result of os.InRoot is an open-when-needed root.
• An os.Root that is in the open-when-needed state supports all of the same methods as an opened root, but implicitly opens the underlying directory before taking the action and closes it after the operation is complete, returning any errors from those additional steps. The implicit open/close affects only the underlying directory handle, and not the state of the os.Root object itself.
• Calling Close on either an open-when-needed or opened root moves it to the "closed" state. If it was opened, then the underlying filehandle is also closed. A closed root cannot return to either of the prior states, and all methods on a closed root fail immediately with an error.

With this in mind, I do now agree with @earthboundkid that I'd like some way either to ask an os.Root I've been given which state it is in, or alternatively to force it into the "opened" state (no-op if it already is), so that I can still rely on the guarantees that only an "opened" root can provide.

I think I personally prefer the "force into opened" model, since that avoids directly exposing the state and encourages writing functions that will do the right thing regardless of which flavor of Root they are passed:

// ForceOpened upgrades an [Root] created by [InRoot] to provide the same
// guarantees as a root created by [OpenRoot].
//
// If the root was created by [OpenRoot] or has already been upgraded
// using this function, the call immediately succeeds having made no change.
//
// If this function succeeds then the Root must be explicitly closed using
// [Root.Close].
func (r *Root) ForceOpened() error

---

These details aside though, it does seem a little concerning that there will now be Root objects in the world that don't provide the very guarantees that Root was designed to provide.

Existing code that is relying on these guarantees would presumably now need to be updated to either check whether the root is "opened", use something like the ForceOpened method I suggested above, or at least to document that it requires an opened root in order to achieve correct behavior.

Root is relatively new so hopefully hasn't been used too widely in third-party code yet, but nonetheless this seems like a hazard for anyone intending to use a library with a function that takes a Root as an argument but who doesn't understand the open-when-needed vs. opened distinction enough to understand that they might be passing the wrong dynamic "flavor" of os.Root, and the compiler doesn't have enough information to help them realize that. 🤔

[neild]

Root.OpenRoot(".") will open the root of the Root.

I think it would be useful to understand an realistic concrete scenario in which code would want to distinguish between an opened-on-demand Root from os.InRoot and a regular Root, or in which it's plausible that a user would use the wrong one.

If we're really concerned about misuse, I suppose there could be a vet check to verify that the result of InRoot is never stored. That seems like overkill to me, though.

Edit: A less invasive alternative to a vet check might be to say that the Root returned by InRoot may be used exactly once. Again, this seems like a bit of overkill to me.

[apparentlymart]

It is true that the concerns I raised at the end of my last comment are theoretical rather than practical. In large part that is because Root is very new and I've not had much opportunity to use it in practice yet, and so I've not yet got experience with its typical usage patterns.

This new flavor of os.Root can still provide a subset of the guarantees that os.Root provides: it prevents upward traversal out of the given directory even though it can no longer guarantee that the path still refers to the same directory that was present when the os.Root was created.

A function that takes an *os.Root as an argument could presumably be doing that for a variety of reasons:

1. It's just a convenient way to pass a reference to a filesystem subtree around as an encapsulated object, instead of taking a path string and doing filepath.Join-type stuff to it.

   In this case, an open-when-needed root is just fine.

2. It's going to do some operations using paths provided from outside the program, such as extracting an archive, and it's expecting os.Root to prevent upward traversal out of the root, but isn't concerned with the target directory itself being moved or otherwise interefered with.

   I believe this case is also fine with an open-when-needed root.

3. It needs to operate consistently on a particular directory regardless of where that directory has ended up in the filesystem or even whether it's still accessible via a filesystem path at all.

   This case does seem to require a real "opened" root rather than an open-when-needed root, although depending on the division of responsibilities in the system it could potentially use root.OpenRoot(".") to provide itself that guarantee.

   In that case the callee can't be sure it's using exactly the same directory that its caller had intended. But at that point we're talking about caller intentions, and the caller can be responsible for expressing that intention by using os.OpenRoot instead of os.InRoot, as long there's a clear enough signal about which is the correct one to use.

   I think the vet checks discussed just above would be sufficient to catch and discourage the mistake of using an open-when-needed root for something that ought to have been an opened root; the main hazard I was worried about was someone relying on gopls code completion which would presumably consider OpenRoot and InRoot to both be valid completions in a context where an *os.Root value is required.

So indeed, there seem to be fewer cases where this matters than I initially thought, and so perhaps the tradeoff is worth it. I'm still on the fence about it -- weakening the guarantees of a security-oriented feature in a way the compiler can't recognize purely for convenience does still give me pause -- but I'm not as concerned as I was before! I think I've ended up pretty neutral on this feature, so I'll bow out of the discussion here.