-
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?
Conversation
Rewrite BuildCommandTests.getTaskAllowEntitlement to check for entitlements using codesign. Add macOS entitlement during build process using swift build. Fix wording in warning related to get-task-allow command line options.
| if provisioningSourceData.sdkRoot.contains("simulator") { | ||
| additionalEntitlements["get-task-allow"] = .plBool(true) | ||
| } |
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.
I’ve left this here to not introduce some regression by accident, but it feels like it maybe should be handled by my code.
I’m confused though, because on one hand there’s comment in swift-build source code (https://github.com/swiftlang/swift-build/blob/51fa0aee68406cf54fe25c0bb553678315d87113/Sources/SWBTaskExecution/TaskActions/ProcessProductEntitlementsTaskAction.swift#L259) stating when building for simulator, signed entitlements should only contain get-task-allow, but on the other hand when checking what entitlements Xcode adds to products it produces for xcode targets (so “app” or “framework” managed by xcode projects, not SwiftPM) I’ve only seen get-task-allow in signed entitlements of application binary built for actual iphone (tested on macOS 26, Xcode 26.1).
But I don’t think SwiftPM can build executables for iOS running on actual device, because as far as I know such executables need provisioning profile and SwiftPM doesn’t allow specifying one (I think?).
Due to that I’m not sure what exact rules are about adding this entitlement. I’d appreciate advice about this code, whether it can stay like this or some changes are necessary, maybe new test needs to be added which builds for iphonesimulator?
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.
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
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.
From the looks of it, this code was copied from Swift Build's swbuild testing CLI, which I think I added for testing purposes as a minimal and incomplete replica of what Xcode does. So there's not much risk of regression because nothing is really using this.
But I don’t think SwiftPM can build executables for iOS running on actual device, because as far as I know such executables need provisioning profile and SwiftPM doesn’t allow specifying one (I think?).
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).
|
With this code |
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.
Thanks for the PR! This change looks like a good improvement to me, but I'd like to double check a few details and give other folks a chance to review as well, which might take a few days. In the meantime I left a handful of minor comments.
With this code BuildCommandsTestCase.doesNotRebuildWithFlags(data:flags:) test fails for swift build backend and debug configuration. I think it’s because even though swift build does not rebuild the binary, it is still modified by codesign process executed by swift build. I guess easy “fix” could be passing —disable-get-task-allow-entitlement in that test, but it doesn’t seem elegant/right.
I’d appreciate advice about what to do with this test.
Looks like you exposed a case where the task's signature was pulling in some irrelevant settings overrides and invalidating it too often. With your patch layered on top of swiftlang/swift-build#920, I'm now seeing the test pass
| } | ||
|
|
||
| private final class PlanningOperationDelegate: SWBPlanningOperationDelegate, Sendable { | ||
| private let shouldEnableMacOsDebuggingEntitlement: Bool |
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:
| private let shouldEnableMacOsDebuggingEntitlement: Bool | |
| private let shouldEnableMacOSDebuggingEntitlement: Bool |
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.
| ) 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 |
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.
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?
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.
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 if identity == "-" || identity.isEmpty instead of overriding the identity.
| // 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" |
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.
This might be a good place to put the CODE_SIGN_IDENTITY override if we move it out of the delegate method
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.
Instead of turning this on, we should probably make sure DEPLOYMENT_POSTPROCESSING=YES is set in PIF generation for release builds.
|
|
||
| @Test( | ||
| .SWBINTTODO("Test failed because swiftbuild doesn't output precis codesign commands. Once swift run works with swiftbuild the test can be investigated."), | ||
| .SWBINTTODO( |
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.
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
| if identity == "-" { | ||
| let signedEntitlements = provisioningSourceData.entitlementsDestination == "Signature" | ||
| let signedEntitlements = provisioningSourceData | ||
| .entitlementsDestination == "Signature" && provisioningSourceData.sdkRoot.contains("iphoneos") |
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.
I believe the second part of this condition should be "not macOS" rather than "is iOS", maybe @jakepetroules can confirm
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.
I think this shouldn't check for platform at all.
| if provisioningSourceData.sdkRoot.contains("simulator") { | ||
| additionalEntitlements["get-task-allow"] = .plBool(true) | ||
| } |
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.
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
| } | ||
|
|
||
| private final class PlanningOperationDelegate: SWBPlanningOperationDelegate, Sendable { | ||
| private let shouldEnableMacOsDebuggingEntitlement: Bool |
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.
| if identity == "-" { | ||
| let signedEntitlements = provisioningSourceData.entitlementsDestination == "Signature" | ||
| let signedEntitlements = provisioningSourceData | ||
| .entitlementsDestination == "Signature" && provisioningSourceData.sdkRoot.contains("iphoneos") |
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.
I think this shouldn't check for platform at all.
| additionalEntitlements["get-task-allow"] = .plBool(true) | ||
| } | ||
|
|
||
| if shouldEnableMacOsDebuggingEntitlement { |
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.
I think it should be com.apple.security.get-task-allow if macOS or simulator, else get-task-allow.
| // 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" |
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.
Instead of turning this on, we should probably make sure DEPLOYMENT_POSTPROCESSING=YES is set in PIF generation for release builds.
| request: request, | ||
| delegate: PlanningOperationDelegate(), | ||
| delegate: PlanningOperationDelegate(shouldEnableMacOsDebuggingEntitlement: self.buildParameters | ||
| .triple.darwinPlatform == .macOS && self.buildParameters.debuggingParameters |
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.
Shouldn't need to check for macOS here.
| ) 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 |
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.
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 if identity == "-" || identity.isEmpty instead of overriding the identity.
| if provisioningSourceData.sdkRoot.contains("simulator") { | ||
| additionalEntitlements["get-task-allow"] = .plBool(true) | ||
| } |
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.
From the looks of it, this code was copied from Swift Build's swbuild testing CLI, which I think I added for testing purposes as a minimal and incomplete replica of what Xcode does. So there's not much risk of regression because nothing is really using this.
But I don’t think SwiftPM can build executables for iOS running on actual device, because as far as I know such executables need provisioning profile and SwiftPM doesn’t allow specifying one (I think?).
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).
Implement enable/disable-get-task-allow for swift build backend
Motivation:
Resolve issue #8378
Modifications:
Rewrite of BuildCommandTest.getTaskAllowedEntitlement to check for entitlements using
codesign. Also use build type as test parameter to shorten test code slightly.In
SwiftBuildSystemPlanningOperationsDelegatereceives argument used to decide whether add macOSget-task-allowentitlement when Swift Build requests provisioning data. When we want to add entitlement we need to use at least ad-hoc signing so code also ensures it is done. It is only added on macOS.Added settings override for swift-build instructing to not remove get-task-allow when we want to have it.
Also fixed wording in warning produced when user uses get-task-allow command line options on unsupported platforms.
Result:
When building with swift-build backend:
--enable-get-task-allow-entitlementwill have macOS get-task-allow entitlement