Codesign w/ debugging entitlement for backtraces on macOS #7010

MaxDesiatov · 2023-10-16T16:26:47Z

Motivation:

The new Swift backtracer will only function on macOS if:

SWIFT_BACKTRACE environment variable has its enable option set to yes or tty, and
The program you are running was signed with the com.apple.security.get-task-allow entitlement.

Xcode automatically signs all local builds with that entitlement by default; without it the Swift backtracer won't work.

SwiftPM should sign Debug binaries with the entitlement by default. It should also have an option to sign Release binaries with the entitlement, noting that they should not be distributed in that state but that crashes may only manifest in release builds (where the optimizer is fully enabled).

Modifications:

Added new --enable-get-task-allow-entitlement and --disable-get-task-allow-entitlement CLI flags. Made it enabled by default for debug builds on macOS. When enabled, applying codesigning with com.apple.security.get-task-allow in LLBuildManifestBuilder+Product.swift.

Result:

Backtraces work on macOS when built with SwiftPM.

Resolves rdar://112065568.

MaxDesiatov · 2023-10-16T16:27:00Z

@swift-ci test

neonichu · 2023-10-16T16:57:11Z

I'm a little bit concerned by us having to duplicate all binaries to achieve this. I'm not 100% sure how this works in Xcode's build system, but I believe it doesn't require this kind of workaround. cc @m-i-r-z-a @owenv
Not sure about the naming here, I don't feel like I would understand that --enable-unsafe-debugging does what it does here. Maybe we should just call it --enable-backtraces and make it an error to use on non-macOS since it won't do anything there today.

MaxDesiatov · 2023-10-16T17:00:50Z

I'm a little bit concerned by us having to duplicate all binaries to achieve this.

It doesn't duplicate binaries though, only writes the linked binary to a different location when backtraces are enabled. We move the binary to the final location after codesigning is completed to satisfy the inputs of the phony product rule. So far this is the only way I found to make it work for shell commands that don't have outputs different from inputs (like the codesign command).

MaxDesiatov · 2023-10-16T17:03:56Z

@swift-ci test

owenv · 2023-10-16T17:08:21Z

llbuild has an is-mutated key which can be used in combination with virtual output nodes to order a series of tasks which modify inputs in-place. There's some brief documentation of it here: https://github.com/apple/swift-llbuild/blob/a3b8d34be319f1b3ba8de53c434386419dec1f4f/docs/buildsystem.rst#L387

tomerd · 2023-10-16T17:13:38Z

llbuild has an is-mutated key which can be used in combination with virtual output nodes to order a series of tasks which modify inputs in-place. There's some brief documentation of it here: https://github.com/apple/swift-llbuild/blob/a3b8d34be319f1b3ba8de53c434386419dec1f4f/docs/buildsystem.rst#L387

this may also be interesting in the context of plugins? cc @neonichu @abertelrud

al45tair · 2023-10-17T10:49:06Z

llbuild has an is-mutated key which can be used in combination with virtual output nodes to order a series of tasks which modify inputs in-place.

This sounds like the right way to make this work.

Also, I wonder whether it would be worth providing more general support for code signing and entitlements in SwiftPM, so that e.g. a user could specify in the package manifest that they'd like their program signed, and maybe provide a list of entitlements they'd like it to have?

al45tair · 2023-10-17T10:51:36Z

Sources/Build/BuildDescription/ProductBuildDescription.swift

@@ -367,6 +367,10 @@ public final class ProductBuildDescription: SPMBuildCore.ProductBuildDescription

        return flags
    }
+
+    func codeSigningArguments(plistPath: AbsolutePath, binaryPath: AbsolutePath) -> [String] {
+        ["codesign", "--force", "--sign", "-", "--entitlements", plistPath.pathString, binaryPath.pathString]


It might be a good idea to have a command line option to let you specify the code signing identity (here we're using -).

IMO that should come as a separate PR to make this facility a bit more general.

Sounds good. I only mention it because people who are aware of how this works will pretty quickly notice that we're signing things and ask how to sign with a different identity.

That's fair, but signing with a different identity or with a different entitlement requires a public API design and likely an evolution proposal. Scoping this to one specific case means we don't have to block backtraces with this potential future design work.

Sure. No objection from me.

neonichu · 2023-10-17T18:08:40Z

Also, I wonder whether it would be worth providing more general support for code signing and entitlements in SwiftPM, so that e.g. a user could specify in the package manifest that they'd like their program signed, and maybe provide a list of entitlements they'd like it to have?

I think that would be useful, but in my mind, it's very separable from this. We need to design APIs for configuring this etc.

Dependency of #7010 Added per the llbuild documentation at https://github.com/apple/swift-llbuild/blob/a3b8d34be319f1b3ba8de53c434386419dec1f4f/docs/buildsystem.rst#L387 and a corresponding test at https://github.com/apple/swift-llbuild/blob/a3b8d34be319f1b3ba8de53c434386419dec1f4f/tests/BuildSystem/Build/mutable-outputs.llbuild#L66. I also took the opportunity to get rid of deprecated TSC dependencies in `ManifestWriter.swift`, replacing it with string interpolation instead.

Dependency of #7010. Added per the llbuild documentation at https://github.com/apple/swift-llbuild/blob/a3b8d34be319f1b3ba8de53c434386419dec1f4f/docs/buildsystem.rst#L387 and a corresponding test at https://github.com/apple/swift-llbuild/blob/a3b8d34be319f1b3ba8de53c434386419dec1f4f/tests/BuildSystem/Build/mutable-outputs.llbuild#L66. Newly introduced attributes are not used anywhere yet in this PR, so technically it is NFC. I also took the opportunity to get rid of deprecated TSC dependencies in `ManifestWriter.swift`, replacing it with string interpolation instead.

al45tair · 2023-10-19T12:12:23Z

Not sure about the naming here, I don't feel like I would understand that --enable-unsafe-debugging does what it does here. Maybe we should just call it --enable-backtraces and make it an error to use on non-macOS since it won't do anything there today.

If we're calling it --enable-backtraces, I think it should work on Linux as well, since someone might have set SWIFT_BACKTRACE=enable=no in their environment, and in that case it could just set enable=yes instead.

The reasoning behind --enable-unsafe-debugging was that it will also allow the use of various other tools (like swift-inspect) which have the same requirement for entitlements that the backtracer has. But that has the downside that it's more macOS specific, whereas --enable-backtraces makes some sense for swift run on Linux too.

al45tair · 2023-10-19T13:25:21Z

Note: if we're changing SWIFT_BACKTRACE, it should leave any other settings alone (it's a comma-separated list of key=value settings).

neonichu · 2023-10-19T16:05:21Z

My objections to the current name are:

it only does backtraces but has a generic name
it doesn't say that it is macOS only, so users may think they're getting something which they are not

If we can make it explicitly macOS-only and have future plans for more functionality, the name seems fine to me.

al45tair · 2023-10-19T16:06:16Z

I don't mind --enable-backtraces, to be clear. If we do that, it can just work everywhere. It will as a side effect also make e.g. swift-inspect work on macOS.

MaxDesiatov · 2023-10-19T16:09:42Z

Yeah, there's a case for --disable-backtraces to be respected on Linux, otherwise we'd have to call it --enable-macos-backtraces and --disable-macos-backtraces for clarity, and I'd prefer the former.

MaxDesiatov · 2023-10-19T17:32:34Z

IIUC @tomerd prefers --enable-get-task-allow-entitlement and --disable-get-task-allow-entitlement flags to control this. Otherwise I hope we arrived at a consensus?

MaxDesiatov · 2023-10-19T17:43:59Z

@swift-ci test

MaxDesiatov · 2023-10-19T19:47:57Z

@swift-ci test windows

MaxDesiatov · 2023-10-20T13:16:55Z

@swift-ci test

MaxDesiatov · 2023-10-20T13:17:03Z

@swift-ci test windows

MaxDesiatov · 2023-10-20T14:19:07Z

@swift-ci test

MaxDesiatov · 2023-10-20T14:23:44Z

@swift-ci test

MaxDesiatov · 2023-10-20T14:59:59Z

@swift-ci test

MaxDesiatov · 2023-10-20T15:00:27Z

@swift-ci test windows

MaxDesiatov · 2023-10-20T15:31:45Z

@swift-ci test

MaxDesiatov · 2023-10-20T16:26:20Z

@swift-ci test

MaxDesiatov · 2023-10-20T16:32:26Z

@swift-ci test windows

tomerd · 2023-10-23T17:06:44Z

Sources/CoreCommands/Options.swift

+    public var enableGetTaskAllowEntitlement: Bool = false
+
+    @Flag(help: .hidden)
+    public var disableGetTaskAllowEntitlement: Bool = false


you can probably marge this into one optional Bool setting

It doesn't look @Flag supports optional types, unless I'm something? Maybe @natecook1000 or @rauhul can clarify?

What is the difference between true/false/nil behaviors?

IIRC I had to provide a default value, but the default value depended on values of other options. I assume false would mean --disable..., true would mean --enable..., and nil would mean neither were passed on CLI?

Hmm so you want to infer true/false based on other flags unless explicitly specified... that is reasonable. I don't think we support Optional Flags yet, but I wonder if you could use an EnumerableFlag instead.

You can actually have a boolean flag as long as there's an inversion, which I think is exactly what you want in this case. The declaration would look like:

@Flag(inversion: .prefixedEnableDisable) var getTaskAllowEntitlement: Bool?

It looks like we might not allow an = nil in there, which may have been what you ran into. That's also an issue we should be able to fix pretty easily.

Ooh good catch!

yes, this is what I had in mind. @MaxDesiatov

My bad, it actually does support an optional default value, at least with .prefixedEnableDisable it works great. I'll submit a follow-up PR.

Addressed in #7028

tomerd

seems good to me

tomerd · 2023-10-23T21:03:49Z

Sources/Build/BuildManifest/LLBuildManifestBuilder+Product.swift

@@ -33,13 +33,19 @@ extension LLBuildManifestBuilder {
            testInputs = []
        }

+        // Create a phony node to represent the entire target.
+        let targetName = try buildProduct.product.getLLBuildTargetName(config: self.buildConfig)


please check if this has performance implications, I vaguely remeber @neonichu ran into something like that with this API, tho no 100% sure

This phony node is not new, it's only moved from previous L62 where it was declared too late for us. I had to move it up to this line.

### Motivation: Following up on feedback to #7010, it's better to have a single optional boolean option than two non-optional booleans. ### Modifications: Replaced `var enableGetTaskAllowEntitlement = false` and `var disableGetTaskAllowEntitlement = false` with `var getTaskAllowEntitlement: Bool? = nil`, which has an additional `inversion: .prefixedEnableDisable` configuration. ### Result: Code handling these flags is simpler and easier to read.

MaxDesiatov added the enhancement label Oct 16, 2023

MaxDesiatov requested a review from al45tair October 16, 2023 16:26

MaxDesiatov self-assigned this Oct 16, 2023

MaxDesiatov requested review from neonichu and tomerd as code owners October 16, 2023 16:26

al45tair reviewed Oct 17, 2023

View reviewed changes

MaxDesiatov force-pushed the maxd/debugging-entitlement branch from 519bbe1 to a5d824e Compare October 18, 2023 08:34

MaxDesiatov mentioned this pull request Oct 18, 2023

llbuild: Support is-mutated/is-command-timestamp nodes #7020

Merged

MaxDesiatov added 5 commits October 19, 2023 10:22

Codesign w/ debugging entitlement for backtraces on macOS

8f20b7a

Add code signing build rules

a59ca1a

Propagate CLI options to BuildParameters.Debugging

4388dc8

Fix duplicate rule name

0780df6

Add missing file to CMakeLists.txt

ca34514

Address PR feedback

f7b42ad

MaxDesiatov force-pushed the maxd/debugging-entitlement branch from a5d824e to f7b42ad Compare October 19, 2023 17:30

Update CHANGELOG.md

586500b

MaxDesiatov requested a review from al45tair October 19, 2023 17:36

Add LLBuildManifestBuilderTests.swift

281f216

Add more tests for new options and build parameters

2ecaa53

Clean up formatting

a0ca764

Fix tests build error on Linux

0101ced

Separate stdout/stderr in BuildResult

9ea74f5

Fix comparison to stdout instead of stderr on Linux

15007d9

MaxDesiatov enabled auto-merge (squash) October 20, 2023 18:14

tomerd reviewed Oct 23, 2023

View reviewed changes

tomerd approved these changes Oct 23, 2023

View reviewed changes

MaxDesiatov merged commit 855b129 into main Oct 23, 2023
5 checks passed

MaxDesiatov deleted the maxd/debugging-entitlement branch October 23, 2023 17:09

tomerd reviewed Oct 23, 2023

View reviewed changes

MaxDesiatov mentioned this pull request Oct 23, 2023

[NFC] CoreCommands: simplify *get-task-allow-entitlement flags #7028

Merged

Codesign w/ debugging entitlement for backtraces on macOS #7010

Codesign w/ debugging entitlement for backtraces on macOS #7010

Conversation

MaxDesiatov commented Oct 16, 2023 • edited

Motivation:

Modifications:

Result:

MaxDesiatov commented Oct 16, 2023

neonichu commented Oct 16, 2023

MaxDesiatov commented Oct 16, 2023 • edited

MaxDesiatov commented Oct 16, 2023

owenv commented Oct 16, 2023

tomerd commented Oct 16, 2023

al45tair commented Oct 17, 2023

Choose a reason for hiding this comment

MaxDesiatov Oct 17, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MaxDesiatov Oct 17, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

neonichu commented Oct 17, 2023

al45tair commented Oct 19, 2023

al45tair commented Oct 19, 2023

neonichu commented Oct 19, 2023

al45tair commented Oct 19, 2023 • edited

MaxDesiatov commented Oct 19, 2023

MaxDesiatov commented Oct 19, 2023 • edited

MaxDesiatov commented Oct 19, 2023

MaxDesiatov commented Oct 19, 2023

MaxDesiatov commented Oct 20, 2023

MaxDesiatov commented Oct 20, 2023

MaxDesiatov commented Oct 20, 2023

MaxDesiatov commented Oct 20, 2023

MaxDesiatov commented Oct 20, 2023

MaxDesiatov commented Oct 20, 2023

MaxDesiatov commented Oct 20, 2023

MaxDesiatov commented Oct 20, 2023

MaxDesiatov commented Oct 20, 2023

tomerd Oct 23, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rauhul Oct 23, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MaxDesiatov Oct 23, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomerd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MaxDesiatov commented Oct 16, 2023 •

edited

MaxDesiatov commented Oct 16, 2023 •

edited

MaxDesiatov Oct 17, 2023 •

edited

MaxDesiatov Oct 17, 2023 •

edited

al45tair commented Oct 19, 2023 •

edited

MaxDesiatov commented Oct 19, 2023 •

edited

tomerd Oct 23, 2023 •

edited

rauhul Oct 23, 2023 •

edited

MaxDesiatov Oct 23, 2023 •

edited