Rework the `Sandbox` enum into a `SandboxProfile` struct that can separate out point of configuration from point of application #4005

abertelrud · 2022-01-12T03:16:47Z

This change turns the stateless Sandbox enum into a SandboxProfile struct that can be configured and passed around well before being applied, and it makes the configuration more flexible in several regards:

there is a list of path rules allowing a mixed order of allow and deny rules
the choice of allowing writing to temporary directories is independent of anything other choice
the defaults are built into the initializer rather than a separate strictness parameter — this means that they can be queried once the SandboxProfile has been created and are expressed in terms of the choices available to all sandbox profiles.

Motivation

Separating out the point of configuration from the point of application of the sandbox makes it easier to configure.

The ergonomic changes described above also make it easier to use.

Having the sandbox profile be a struct that generates the platform specifics as needed also provides a place with which to associate any cached/compiled representation for profiles that are largely static.

Details

As cleanup, this commit also removes the pre-SwiftPM 5.3 specialities which were specific to running the package manifest in an interpreter rather than compiling and executing it. This functionality is no longer relevant since it isn't possible to run any manifest in the interpreter.

In this commit, the call sites have been adjusted so that they use the modified SandboxProfile API, but still construct the profiles on-the-fly as before. A future change could make them instead be configured at an early point and then applied later when the sandboxed process is actually launched. Another future change could add the sandbox profile to Process as a property so that there is no API assumption that applying a sandbox necessarily involves just modifying the command line.

Sources/Basics/Sandbox.swift

abertelrud · 2022-01-12T03:19:46Z

Tests/CommandsTests/PackageToolTests.swift

@@ -1361,6 +1370,7 @@ final class PackageToolTests: CommandsTestCase {
                let output = try result.utf8Output() + result.utf8stderrOutput()
                XCTAssertEqual(result.exitStatus, .terminated(code: 0), "output: \(output)")
                XCTAssertMatch(output, .contains("This is MyCommandPlugin."))
+                XCTAssertMatch(output, .contains("successfully created file at path \(resolveSymlinks(packageDir).appending(components: ".build", "plugins", "outputs", "Foo"))"))


This is the check that was incomplete in the previous test, and would have caught the problem.

abertelrud · 2022-01-12T03:20:01Z

@swift-ci smoke test

abertelrud · 2022-01-12T08:46:13Z

@swift-ci smoke test

abertelrud · 2022-01-12T08:50:48Z

Tests/BasicsTests/SandboxTests.swift

+        }
+    }
+
+    func testWritingToWritableInsideReadOnlyAllowed() throws {


This adds a specific test case for a writable directory inside of a read-only directory.

abertelrud · 2022-01-12T08:53:25Z

Sources/PackageLoading/ManifestLoader.swift

-                        let strictness: Sandbox.Strictness = toolsVersion < .v5_3 ? .manifest_pre_53 : .default
-                        cmd = Sandbox.apply(command: cmd, strictness: strictness, writableDirectories: cacheDirectories)
+                        let sandboxProfile = SandboxProfile(pathRules: cacheDirectories.map{ .writable($0) })
+                        cmd = sandboxProfile.apply(to: cmd)


The notes in the Sandbox profile generator indicate that the pre-5.3 rules were needed for interpreting the manifest rather than compiling and running it. This isn't supported anymore, and the operations affected were being able to write to some Clang locations and to set sysctls. Those don't seem like operations that we want to keep supporting. Do we know of any packages that required the < 5.3 strictness?

abertelrud · 2022-01-12T08:59:30Z

@swift-ci smoke test

tomerd

<3 this is great

abertelrud · 2022-01-12T18:48:10Z

@swift-ci please test package compatibility

abertelrud · 2022-01-12T19:39:35Z

Hmm...

11:01:23 
/Users/buildnode/jenkins/workspace/swift-package-manager-source-compat-suite-PR-macos/swift-driver/Sources/SwiftDriver/Utilities/System.swift:60:32: error: multi-line string literal content must begin on a new line
11:01:23       var quoted: String = #"""#

abertelrud · 2022-01-13T00:03:37Z

Moving this one aside temporarily while preparing a much smaller fix for the problem at hand. That fix can then be nominated for inclusion in 5.6. Then I'll bring this back since it's what we want to do moving forward.

abertelrud · 2022-01-13T05:31:51Z

@swift-ci please test package compatibility

abertelrud · 2022-04-19T08:29:34Z

@swift-ci please smoke test

…vide a unique path specified by `TMPDIR` The `/tmp` temporary directory is particularly unsafe because it is noisy — various processes with various ownerships write and read files there. So it's better to use the per-user directory returned by `NSTemporaryDirectory()`. We can improve several things here by passing through the result of calling `NSTemporaryDirectory()` in the sandbox instead of `/tmp` and also setting `TMPDIR` in the environment (regardless of whether or not it was set in the inherited one). This will cause the inferior's use of Foundation to respect this directory (the implementation of `NSTemporaryDirectory()` looks at `TMPDIR` on all platforms), and that should also be true for any command line tools it calls, as long as it passes through the environment. If `TMPDIR` is set in SwiftPM's own environment, it will control SwiftPM's own `NSTemporaryDirectory()` will be respected all the way through. As part of this we also take the opportunity to clean up the interface a little. In particular, this change turns the stateless `Sandbox` enum into a `SandboxProfile` struct that's a bit more flexible: - there is a list of path rules allowing a mixed order of `writable`, `readonly`, and `noaccess` rules - the defaults are built into the initializer rather than a separate `strictness` parameter — this means that they can be queried once the SandboxProfile has been created and are expressed in terms of the choices available to all sandbox profiles - there is separate control for whether or not to allow network access The new `SandboxProfile` is also a bit more ergonomic — as a struct, it can be copied, mutated, and carried around in a property before being used. Having the sandbox profile be a struct that generates the platform specifics as needed also could also provide a place with which to associate any cached/compiled representation for profiles that are largely static, for any platform that supports that. As cleanup, this commit also removes the pre-SwiftPM 5.3 specialities which were specific to running the package manifest in an interpreter rather than compiling and executing it. This functionality is no longer relevant since it isn't possible to run any manifest in the interpreter, no matter what tools version the package specifies. In this commit, the call sites have been adjusted so that they use the modified SandboxProfile API, but they still construct the profiles on-the-fly as before. A future change will make them instead be configured at an early point and then applied later when the sandboxed process is actually launched. Another future change could add the sandbox profile to `Process` as a property so that there is no API assumption that applying a sandbox necessarily involves just modifying the command line. rdar://91575256

abertelrud · 2022-04-19T17:57:34Z

Opened a new PR here: #4307 that replaces this one

abertelrud requested review from elsh, neonichu and tomerd as code owners January 12, 2022 03:16

abertelrud self-assigned this Jan 12, 2022

abertelrud added the ready Author believes the PR is ready to be merged & any feedback has been addressed label Jan 12, 2022

abertelrud commented Jan 12, 2022

View reviewed changes

Sources/Basics/Sandbox.swift Outdated Show resolved Hide resolved

abertelrud commented Jan 12, 2022

View reviewed changes

abertelrud force-pushed the eng/87417780-sandbox-should-always-allow-plugin-output-directory branch from 0b631ed to 836aabb Compare January 12, 2022 08:44

abertelrud commented Jan 12, 2022

View reviewed changes

abertelrud force-pushed the eng/87417780-sandbox-should-always-allow-plugin-output-directory branch from 836aabb to 692e86a Compare January 12, 2022 08:58

tomerd approved these changes Jan 12, 2022

View reviewed changes

neonichu approved these changes Jan 12, 2022

View reviewed changes

abertelrud marked this pull request as draft January 13, 2022 00:01

abertelrud changed the title ~~Sandbox blocks output to default plugin output directory when it's under <pkgdir>/.build~~ Rework the Sandbox enum into a SandboxProfile struct that can separate out point of configuration from point of application Jan 13, 2022

abertelrud added WIP Work in progress and removed ready Author believes the PR is ready to be merged & any feedback has been addressed labels Jan 13, 2022

abertelrud mentioned this pull request Jan 13, 2022

Sandbox blocks output to default plugin output directory when it's under <pkgdir>/.build #4009

Merged

abertelrud force-pushed the eng/87417780-sandbox-should-always-allow-plugin-output-directory branch 2 times, most recently from 16e9075 to af586c4 Compare April 19, 2022 08:27

abertelrud force-pushed the eng/87417780-sandbox-should-always-allow-plugin-output-directory branch from af586c4 to ccdc849 Compare April 19, 2022 17:33

abertelrud force-pushed the eng/87417780-sandbox-should-always-allow-plugin-output-directory branch from ccdc849 to e32cba8 Compare April 19, 2022 17:47

abertelrud closed this Apr 19, 2022

abertelrud deleted the eng/87417780-sandbox-should-always-allow-plugin-output-directory branch April 19, 2022 18:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Rework the `Sandbox` enum into a `SandboxProfile` struct that can separate out point of configuration from point of application #4005

Rework the `Sandbox` enum into a `SandboxProfile` struct that can separate out point of configuration from point of application #4005

Uh oh!

abertelrud commented Jan 12, 2022 •

edited

Loading

Uh oh!

Uh oh!

abertelrud Jan 12, 2022

Uh oh!

abertelrud commented Jan 12, 2022

Uh oh!

abertelrud commented Jan 12, 2022

Uh oh!

abertelrud Jan 12, 2022

Uh oh!

abertelrud Jan 12, 2022

Uh oh!

abertelrud commented Jan 12, 2022

Uh oh!

tomerd left a comment

Uh oh!

abertelrud commented Jan 12, 2022

Uh oh!

abertelrud commented Jan 12, 2022

Uh oh!

abertelrud commented Jan 13, 2022

Uh oh!

abertelrud commented Jan 13, 2022

Uh oh!

abertelrud commented Apr 19, 2022

Uh oh!

abertelrud commented Apr 19, 2022

Uh oh!

Uh oh!

Rework the Sandbox enum into a SandboxProfile struct that can separate out point of configuration from point of application #4005

Rework the Sandbox enum into a SandboxProfile struct that can separate out point of configuration from point of application #4005

Uh oh!

Conversation

abertelrud commented Jan 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation

Details

Uh oh!

Uh oh!

abertelrud Jan 12, 2022

Choose a reason for hiding this comment

Uh oh!

abertelrud commented Jan 12, 2022

Uh oh!

abertelrud commented Jan 12, 2022

Uh oh!

abertelrud Jan 12, 2022

Choose a reason for hiding this comment

Uh oh!

abertelrud Jan 12, 2022

Choose a reason for hiding this comment

Uh oh!

abertelrud commented Jan 12, 2022

Uh oh!

tomerd left a comment

Choose a reason for hiding this comment

Uh oh!

abertelrud commented Jan 12, 2022

Uh oh!

abertelrud commented Jan 12, 2022

Uh oh!

abertelrud commented Jan 13, 2022

Uh oh!

abertelrud commented Jan 13, 2022

Uh oh!

abertelrud commented Apr 19, 2022

Uh oh!

abertelrud commented Apr 19, 2022

Uh oh!

Uh oh!

Rework the `Sandbox` enum into a `SandboxProfile` struct that can separate out point of configuration from point of application #4005

Rework the `Sandbox` enum into a `SandboxProfile` struct that can separate out point of configuration from point of application #4005

abertelrud commented Jan 12, 2022 •

edited

Loading