Enforce read-only-ness of package directory even when it's inside writable directories

[abertelrud]

Enforce read-only-ness of package directory even when it's inside /tmp and other writable directories to the number of reduce special cases.

Motivation

The package directory should always be read-only unless specifically requested to be writable, even if it happens to be in /tmp or other implicitly writable locations.

Details

The sandbox supports a list of writableDirectories, which are treated as path component prefixes after package resolution. The are applied to the sandbox rules after the default-deny and everything-is-read-only rules, carving out cones of writability in specific areas of the file system.

But if the package directory is under one of these directories that is allowed by default, then it is implicitly writable even in the absence --allow-writing-to-package-directory. It also makes unit tests awkward, since they usually create temporary test fixtures inside the temporary directory.

Modifications:

• extend Sandbox with a list of read-only locations
• add function and parameter documentation to Sandbox
• thread through the list of read only directories
• pass the package directory as a specific read-only directory unless it's covered by an explicit writable-directory parameter
• switch back to using strictness for temporary-directory writability (cleaner)
• remove the flag to make temporary directory read-only that was added mostly for unit tests

Result:

There are fewer edge cases with respect to when the package directory is writable.

[abertelrud]

This flag was just added, so nobody is depending on it yet, and it was mostly for unit tests — it seems much clearer to always make the package directory read-only, even for packages checked out to /tmp or other implicitly writable places.

[abertelrud]

The temporary directories are now again handled by the strictness.

[abertelrud]

This was actually redundant with lines 975-977 above.

[abertelrud]

This one is subject to debate. It avoids marking the package path as readonly if it's covered by any of the directories marked as explicitly writable. Maybe what should be done here in the long term is to have a single list of directory-specific permissions, each of which could be either block or allow and then the caller can craft their sandbox directories as they want to. For example, it might make sense to allow a plugin to modify only a subdirectory of the package. We can extend this in the future — probably better to keep it simple for now.

[tomerd]

starting simple makes sense to me. having the sandbox API accepts a list of allow/deny and having the call site define that make sense.

[abertelrud]

👍

[abertelrud]

This is an incidental fix that will be needed once we mark the 5.6 branch as no longer a development branch (so it stops accepting 999). The PR that added this was still open when all the plugins were changed from 999 to 5.6 after evolution proposal acceptance.

[abertelrud]

@swift-ci please smoke test

[tomerd]

would it better to have the call site pass the temp dirs as writable locations instead of defining a new strictness option for that? alternatively, should this be an argument on "default" strictness instead of it's own case?

[abertelrud]

I had that (passing in the temporary directories from the client) initially actually, but that pushes the platform-specific nature of these paths onto the client. In addition to the paths, there could actually be a varying number of them on different platforms. But I agree that this shouldn't really be part of the strictness, but rather an option on the default strictness as you suggest. I think there are more of those in the future, such as whether or not to allow network, etc.

So yes, in retrospect I think it was a mistake to add the temporary directory handling as a separate strictness level, and it's something to be unwound in the future.

Really what I'd like to do is to change the Sandbox struct to a SandboxProfile or some other name that suggests that it's a set of options, and then that profile could be configured once and passed around so it can be applied to multiple calls later without having to thread through all the individual parameters (and perhaps also cached using operating system calls based on the contents of that profile). Then as we add more parameters, none of the intermediate code has to change. The code that calls the subprocess could just apply the same profile to one command after another. I'm hoping to get to that improvement soonish, in particular since I'm hopeful that it could allow compilation of the profile on macOS just once and the reuse of the compiled profile, saving time when starting sandboxed subprocesses.

At that point it would make sense to get rid of strictness, I think, and just have a set of individually controllable options, and then a convenience initializer to create a SandboxProfile that matches the SwiftPM 5.3 semantics. That special case could then exist outside of the SandboxProfile code itself as an extension, which seems more correct than leaking it into the sandbox API.

All of course for a future PR, but hopefully not too long in the future.