-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Implement enable/disable-get-task-allow for swift build (#8378) #9380
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -112,13 +112,28 @@ func withSession( | |
| } | ||
|
|
||
| private final class PlanningOperationDelegate: SWBPlanningOperationDelegate, Sendable { | ||
| private let shouldEnableMacOsDebuggingEntitlement: Bool | ||
|
|
||
| init(shouldEnableMacOsDebuggingEntitlement: Bool) { | ||
| self.shouldEnableMacOsDebuggingEntitlement = shouldEnableMacOsDebuggingEntitlement | ||
| } | ||
|
|
||
| public func provisioningTaskInputs( | ||
| targetGUID: String, | ||
| provisioningSourceData: SWBProvisioningTaskInputsSourceData | ||
| ) async -> SWBProvisioningTaskInputs { | ||
| let identity = provisioningSourceData.signingCertificateIdentifier | ||
| // if we need to add debug entitlement we have to do codesigning, so we need to ensure at least ad-hoc signing | ||
| let identity = if provisioningSourceData.signingCertificateIdentifier | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Instead of falling back to "-" here, I wonder if this would be a little easier to follow if we added "CODE_SIGN_IDENTITY=-" as an override in makeBuildParameters. @jakepetroules do you have any opinions?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we shouldn't need to provide an override at all. On macOS, CODE_SIGN_IDENTITY defaults to ad-hoc signing if no DEVELOPMENT_TEAM is set, which is already a good default for the SwiftPM CLI. I don't remember off the top of my head what signingCertificateIdentifier being empty means, but if we need to handle that case you could just adjust the branch below to do |
||
| .isEmpty && shouldEnableMacOsDebuggingEntitlement && provisioningSourceData.supportsEntitlements | ||
| { | ||
| "-" | ||
| } else { | ||
| provisioningSourceData.signingCertificateIdentifier | ||
| } | ||
|
|
||
| if identity == "-" { | ||
| let signedEntitlements = provisioningSourceData.entitlementsDestination == "Signature" | ||
| let signedEntitlements = provisioningSourceData | ||
| .entitlementsDestination == "Signature" && provisioningSourceData.sdkRoot.contains("iphoneos") | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I believe the second part of this condition should be "not macOS" rather than "is iOS", maybe @jakepetroules can confirm
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this shouldn't check for platform at all. |
||
| ? provisioningSourceData.productTypeEntitlements.merging( | ||
| ["application-identifier": .plString(provisioningSourceData.bundleIdentifier)], | ||
| uniquingKeysWith: { _, new in new } | ||
|
|
@@ -132,6 +147,16 @@ private final class PlanningOperationDelegate: SWBPlanningOperationDelegate, Sen | |
| ).merging(provisioningSourceData.projectEntitlements ?? [:], uniquingKeysWith: { _, new in new }) | ||
| : [:] | ||
|
|
||
| var additionalEntitlements: [String: SWBPropertyListItem] = [:] | ||
|
|
||
| if provisioningSourceData.sdkRoot.contains("simulator") { | ||
| additionalEntitlements["get-task-allow"] = .plBool(true) | ||
| } | ||
|
Comment on lines
+152
to
+154
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I’ve left this here to not introduce some regression by accident, but it feels like it maybe should be handled by my code.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It may be ok to drop this, but I need to investigate a bit. This code was initially just the bare minimum needed to bring up the new backend so it may not all be strictly correct
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. From the looks of it, this code was copied from Swift Build's
Technically builds can pick a suitable one by default, but the bigger problem is that SwiftPM presently provides no way to define an app bundle target, which is the only deployable product type (bare executables will not work). |
||
|
|
||
| if shouldEnableMacOsDebuggingEntitlement { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it should be |
||
| additionalEntitlements["com.apple.security.get-task-allow"] = .plBool(true) | ||
| } | ||
|
|
||
| return SWBProvisioningTaskInputs( | ||
| identityHash: "-", | ||
| identityName: "-", | ||
|
|
@@ -140,7 +165,7 @@ private final class PlanningOperationDelegate: SWBPlanningOperationDelegate, Sen | |
| profilePath: nil, | ||
| designatedRequirements: nil, | ||
| signedEntitlements: signedEntitlements.merging( | ||
| provisioningSourceData.sdkRoot.contains("simulator") ? ["get-task-allow": .plBool(true)] : [:], | ||
| additionalEntitlements, | ||
| uniquingKeysWith: { _, new in new } | ||
| ), | ||
| simulatedEntitlements: simulatedEntitlements, | ||
|
|
@@ -726,7 +751,10 @@ public final class SwiftBuildSystem: SPMBuildCore.BuildSystem { | |
|
|
||
| let operation = try await session.createBuildOperation( | ||
| request: request, | ||
| delegate: PlanningOperationDelegate(), | ||
| delegate: PlanningOperationDelegate(shouldEnableMacOsDebuggingEntitlement: self.buildParameters | ||
| .triple.darwinPlatform == .macOS && self.buildParameters.debuggingParameters | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Shouldn't need to check for macOS here. |
||
| .shouldEnableDebuggingEntitlement | ||
| ), | ||
| retainBuildDescription: true | ||
| ) | ||
|
|
||
|
|
@@ -1054,7 +1082,9 @@ public final class SwiftBuildSystem: SPMBuildCore.BuildSystem { | |
| private static func constructDebuggingSettingsOverrides(from parameters: BuildParameters.Debugging) -> [String: String] { | ||
| var settings: [String: String] = [:] | ||
| // TODO: debugInfoFormat: https://github.com/swiftlang/swift-build/issues/560 | ||
| // TODO: shouldEnableDebuggingEntitlement: Enable/Disable get-task-allow | ||
| if parameters.shouldEnableDebuggingEntitlement { | ||
| settings["ENTITLEMENTS_DONT_REMOVE_GET_TASK_ALLOW"] = "YES" | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This might be a good place to put the CODE_SIGN_IDENTITY override if we move it out of the delegate method
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Instead of turning this on, we should probably make sure |
||
| } | ||
| // TODO: omitFramePointer: https://github.com/swiftlang/swift-build/issues/561 | ||
| return settings | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -10,19 +10,21 @@ | |
| // | ||
| //===----------------------------------------------------------------------===// | ||
|
|
||
| import Foundation | ||
| import _InternalTestSupport | ||
| import Basics | ||
| @testable import Commands | ||
| @testable import CoreCommands | ||
| import Foundation | ||
| import PackageGraph | ||
| import PackageLoading | ||
| import PackageModel | ||
| import enum PackageModel.BuildConfiguration | ||
| import SPMBuildCore | ||
| import _InternalTestSupport | ||
| import enum SWBUtil.PropertyList | ||
| import enum SWBUtil.PropertyListItem | ||
| import Testing | ||
| import TSCTestSupport | ||
| import Workspace | ||
| import Testing | ||
|
|
||
| struct BuildResult { | ||
| let binPath: AbsolutePath | ||
|
|
@@ -1280,7 +1282,9 @@ struct BuildCommandTestCases { | |
| } | ||
|
|
||
| @Test( | ||
| .SWBINTTODO("Test failed because swiftbuild doesn't output precis codesign commands. Once swift run works with swiftbuild the test can be investigated."), | ||
| .SWBINTTODO( | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We're working towards phasing out the "--build-system xcode" option because it's functionality is a strict subset of "--build-system swiftbuild", so I think we should probably drop this TODO and stop attempting to run this test in that mode |
||
| "Implement get-task-allow entitlement for xcode build system" | ||
| ), | ||
| .tags( | ||
| .Feature.CommandLineArguments.DisableGetTaskAllowEntitlement, | ||
| .Feature.CommandLineArguments.EnableGetTaskAllowEntitlement, | ||
|
|
@@ -1289,75 +1293,112 @@ struct BuildCommandTestCases { | |
| .tags( | ||
| .Feature.CommandLineArguments.BuildSystem, | ||
| ), | ||
| arguments: SupportedBuildSystemOnPlatform, | ||
| arguments: getBuildData(for: SupportedBuildSystemOnPlatform), | ||
| ) | ||
| func getTaskAllowEntitlement( | ||
| buildSystem: BuildSystemProvider.Kind, | ||
| data: BuildData, | ||
| ) async throws { | ||
| try await withKnownIssue(isIntermittent: (ProcessInfo.hostOperatingSystem == .linux)) { | ||
| let buildSystem = data.buildSystem | ||
| let buildConfiguration = data.config | ||
| try await withKnownIssue(isIntermittent: ProcessInfo.hostOperatingSystem == .linux) { | ||
| try await fixture(name: "ValidLayouts/SingleModule/ExecutableNew") { fixturePath in | ||
| #if os(macOS) | ||
| // try await building with default parameters. This should succeed. We build verbosely so we get full command | ||
| // lines. | ||
| var buildResult = try await build(["-v"], packagePath: fixturePath, configuration: .debug, buildSystem: buildSystem,) | ||
|
|
||
| // TODO verification of the ad-hoc code signing can be done by `swift run` of the executable in these cases once swiftbuild build system is working with that | ||
| #expect(buildResult.stdout.contains("codesign --force --sign - --entitlements")) | ||
| #if os(macOS) | ||
| func codesignDisplay(execPath: AbsolutePath) async throws | ||
| -> (AsyncProcessResult.ExitStatus, PropertyListItem?) | ||
| { | ||
| let args = ["codesign", "-d", "--entitlements", "-", "--xml", execPath.pathString] | ||
| let result = try await AsyncProcess.popen(arguments: args) | ||
| let entitlements: PropertyListItem? = if case .success(let output) = result.output, | ||
| !output.isEmpty | ||
| { | ||
| try PropertyList.fromBytes(output) | ||
| } else { | ||
| nil | ||
| } | ||
|
|
||
| return (result.exitStatus, entitlements) | ||
| } | ||
|
|
||
| buildResult = try await build(["-v"], packagePath: fixturePath, configuration:.debug, buildSystem: buildSystem,) | ||
| func verify(entitlements: PropertyListItem?, getTaskAllowRequired: Bool) { | ||
| if getTaskAllowRequired { | ||
| guard let entitlements, case .plDict(let dict) = entitlements else { | ||
| Issue.record("Missing expected entitlements") | ||
| return | ||
| } | ||
|
|
||
| #expect(buildResult.stdout.contains("codesign --force --sign - --entitlements")) | ||
| #expect(dict["com.apple.security.get-task-allow"] == .plBool(true)) | ||
| } else { | ||
| #expect(entitlements == nil) | ||
| } | ||
| } | ||
|
|
||
| // Build with different combinations of the entitlement flag and debug/release build configurations. | ||
| let execName = "ExecutableNew" | ||
|
|
||
| buildResult = try await build( | ||
| ["--enable-get-task-allow-entitlement", "-v"], | ||
| var buildResult = try await build( | ||
| ["-v"], | ||
| packagePath: fixturePath, | ||
| configuration: .release, | ||
| buildSystem: buildSystem, | ||
| configuration: buildConfiguration, | ||
| cleanAfterward: false, | ||
| buildSystem: buildSystem | ||
| ) | ||
| var ( | ||
| exitStatus, | ||
| entitlements | ||
| ) = try await codesignDisplay(execPath: buildResult.binPath.appending(execName)) | ||
|
|
||
| // codesign performs basic verification in display mode, which is enough to confirm ad-hoc signature | ||
| // if verification fails (eg. no signature) termination code will be 1 | ||
| // though on Apple Silicon binary will always be signed because linker signs it by default | ||
| #expect(exitStatus == .terminated(code: 0)) | ||
| verify(entitlements: entitlements, getTaskAllowRequired: buildConfiguration == .debug) | ||
|
|
||
| #expect(buildResult.stdout.contains("codesign --force --sign - --entitlements")) | ||
| try await executeSwiftPackage(fixturePath, extraArgs: ["clean"], buildSystem: buildSystem) | ||
|
|
||
| buildResult = try await build( | ||
| ["--enable-get-task-allow-entitlement", "-v"], | ||
| ["--enable-get-task-allow-entitlement"], | ||
| packagePath: fixturePath, | ||
| configuration: .debug, | ||
| buildSystem: buildSystem, | ||
| configuration: buildConfiguration, | ||
| cleanAfterward: false, | ||
| buildSystem: buildSystem | ||
| ) | ||
| ( | ||
| exitStatus, | ||
| entitlements | ||
| ) = try await codesignDisplay(execPath: buildResult.binPath.appending(execName)) | ||
|
|
||
| #expect(exitStatus == .terminated(code: 0)) | ||
| verify(entitlements: entitlements, getTaskAllowRequired: true) | ||
|
|
||
| #expect(buildResult.stdout.contains("codesign --force --sign - --entitlements")) | ||
| try await executeSwiftPackage(fixturePath, extraArgs: ["clean"], buildSystem: buildSystem) | ||
|
|
||
| buildResult = try await build( | ||
| ["--disable-get-task-allow-entitlement", "-v"], | ||
| ["--disable-get-task-allow-entitlement"], | ||
| packagePath: fixturePath, | ||
| configuration: .debug, | ||
| buildSystem: buildSystem, | ||
| configuration: buildConfiguration, | ||
| cleanAfterward: false, | ||
| buildSystem: buildSystem | ||
| ) | ||
|
|
||
| #expect(!buildResult.stdout.contains("codesign --force --sign - --entitlements")) | ||
|
|
||
| buildResult = try await build( | ||
| ["--disable-get-task-allow-entitlement", "-v"], | ||
| ( | ||
| exitStatus, | ||
| entitlements | ||
| ) = try await codesignDisplay(execPath: buildResult.binPath.appending(execName)) | ||
|
|
||
| #expect(exitStatus == .terminated(code: 0)) | ||
| verify(entitlements: entitlements, getTaskAllowRequired: false) | ||
| #else | ||
| var buildResult = try await build( | ||
| ["-v"], | ||
| packagePath: fixturePath, | ||
| configuration: .release, | ||
| buildSystem: buildSystem, | ||
| configuration: buildConfiguration, | ||
| buildSystem: buildSystem | ||
| ) | ||
|
|
||
| #expect(!buildResult.stdout.contains("codesign --force --sign - --entitlements")) | ||
| #else | ||
| var buildResult = try await build(["-v"], packagePath: fixturePath, configuration: .debug, buildSystem: buildSystem,) | ||
|
|
||
| #expect(!buildResult.stdout.contains("codesign --force --sign - --entitlements")) | ||
|
|
||
| buildResult = try await build(["-v"], packagePath: fixturePath, configuration: .release,buildSystem: buildSystem,) | ||
|
|
||
| #expect(!buildResult.stdout.contains("codesign --force --sign - --entitlements")) | ||
|
|
||
| buildResult = try await build( | ||
| ["--disable-get-task-allow-entitlement", "-v"], | ||
| packagePath: fixturePath, | ||
| configuration: .release, | ||
| configuration: buildConfiguration, | ||
| buildSystem: buildSystem, | ||
| ) | ||
|
|
||
|
|
@@ -1367,20 +1408,16 @@ struct BuildCommandTestCases { | |
| buildResult = try await build( | ||
| ["--enable-get-task-allow-entitlement", "-v"], | ||
| packagePath: fixturePath, | ||
| configuration: .release, | ||
| configuration: buildConfiguration, | ||
| buildSystem: buildSystem, | ||
| ) | ||
|
|
||
| #expect(!buildResult.stdout.contains("codesign --force --sign - --entitlements")) | ||
| #expect(buildResult.stderr.contains(SwiftCommandState.entitlementsMacOSWarning)) | ||
| #endif | ||
|
|
||
| buildResult = try await build(["-v"], packagePath: fixturePath, configuration: .release, buildSystem: buildSystem) | ||
|
|
||
| #expect(!buildResult.stdout.contains("codesign --force --sign - --entitlements")) | ||
| #endif | ||
| } | ||
| } when: { | ||
| [.swiftbuild, .xcode].contains(buildSystem) && ProcessInfo.hostOperatingSystem != .linux | ||
| [.xcode].contains(buildSystem) && ProcessInfo.hostOperatingSystem != .linux | ||
| } | ||
| } | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
capitalization nit:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should just be
shouldEnableDebuggingEntitlement, since this applies to all Apple platforms, not just macOS.