Codesign XCFrameworks for darwin add to app#183399
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces code signing for XCFrameworks in add-to-app scenarios for Darwin platforms. The changes are well-structured, introducing a DarwinAddToAppCodesigning helper to encapsulate the new logic for finding a signing identity and signing the artifacts. The new functionality is correctly integrated into the build ios-framework, build macos-framework, and build swift-package commands, and is accompanied by thorough tests. I've identified a couple of correctness issues in the logic for finding a signing identity when multiple certificates are present, and have provided suggestions for fixes.
|
Could you clarify which solution in the design doc does this PR correspond to? |
Sorry, I thought it was obvious. See the "Add-to-App" section of the doc |
Ah it was obvious, I missed the part that the "solutions" list is for non-add-to-app cases. To sanity check that I understand - this is to codesign the xcframeworks (e.g. |
Correct |
| } | ||
| return result; | ||
| } | ||
|
|
There was a problem hiding this comment.
I moved this function to the new "darwin_add_to_app.dart" file since it's used in both the build ios-framework and build swift-package command
Gotcha. I think in the long run we should have these binaries actually signed by Flutter LLC. I filed #183422 |
| '--obfuscate', | ||
| '--split-debug-info=symbols', | ||
| '--codesign-identity', | ||
| '-', |
There was a problem hiding this comment.
what does the final - mean?
There was a problem hiding this comment.
This does "ad-hoc" signing. See man codesign > SIGNING IDENTITIES
| Future<void> _checkCodeSignature(String pathToArtifact) async { | ||
| final stderrBuffer = StringBuffer(); | ||
| await eval('codesign', <String>['-dv', pathToArtifact], canFail: true, stderr: stderrBuffer); | ||
| if (!stderrBuffer.toString().contains('Signature=adhoc')) { |
There was a problem hiding this comment.
is it possible for Signature to have other values? maybe just contains('Signature=')?
There was a problem hiding this comment.
We're specifically doing ad-hoc codesigning in this test
| /// This class provides methods to find the codesigning identity [getCodesignIdentity] and | ||
| /// codesign files/directories [codesign]. It also has special logic for codesigning | ||
| /// Flutter XCFrameworks [codesignFlutterXCFramework]. | ||
| class DarwinAddToAppCodesigning { |
There was a problem hiding this comment.
is this only used in app-to-app? do we plan to re-use this for similar bugs in non-add-to-app
There was a problem hiding this comment.
Yes, this is only for add-to-app. Generally code-signing is done through part of the build. Since Add-to-App does not do a normal build through xcodebuild, it has be code-signed separately.
There was a problem hiding this comment.
Since Add-to-App does not do a normal build through xcodebuild, it has be code-signed separately.
did you mean to say "Since Add-to-App does not do a normal build through flutter build, ..."?
There was a problem hiding this comment.
No, I specifically mean xcodebuild. Usually it's code-singed during xcodebuild, which makes the EXPANDED_CODE_SIGN_IDENTITY build setting available. We don't use xcodebuild on the Flutter project for the add-to-app commands and therefore do not have access to the EXPANDED_CODE_SIGN_IDENTITY
There was a problem hiding this comment.
I may be missing something. My understanding is that for add-to-app projects, xcodebuild is what Xcode uses under the hood when they hit "Run" button. So I was confused about your previous reply. Could you elaborate more on why this is only a problem for add-to-app but not regular flutter app? Thanks
There was a problem hiding this comment.
This code is executed when running flutter build ios-framework, flutter build macos-framework - not when you're running from Xcode. This command generates and code-signs the XCFrameworks that the user needs to embed in their app. See "Use frameworks" tabs in https://docs.flutter.dev/add-to-app/ios/project-setup#embed-a-flutter-module-in-your-ios-app.
There was a problem hiding this comment.
Regular flutter apps do not generate XCFrameworks that are manually embedded in the app. They use a build script to codesign the Flutter/App frameworks mid-build.
There was a problem hiding this comment.
Thanks! I missed the part that XCFramework is only used in add-to-app but not regular app.
|
I added/changed some new logic in the latest commit:
Both of these changes are to help avoid Xcode build failures due to code-signing identity changing. See https://developer.apple.com/documentation/xcode/verifying-the-origin-of-your-xcframeworks#Diagnose-build-failures-caused-by-code-signature-changes |
When the Flutter.xcframework (and various [other plugins flagged as Privacy-impacting](https://developer.apple.com/support/third-party-SDK-requirements/)) are embedded in a native app using our add to app instructions, it is required for them to be code-signed before build. This PR will codesign all produced XCFrameworks with either the the signing identity passed into the command, found through the Flutter app's Xcode project's settings, or found through the Flutter config. If it's unable to find an identity or it finds more than one, it'll throw and ask the user to pass in an identity. See [go/flutter-ios-privacy-impacting-sdks-codesign-requirement](http://goto.google.com/flutter-ios-privacy-impacting-sdks-codesign-requirement) for explanation of code signature requirement. Fixes flutter#148300. ## Pre-launch Checklist - [x] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [x] I read the [AI contribution guidelines] and understand my responsibilities, or I am not using AI tools. - [x] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [x] I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement]. - [x] I signed the [CLA]. - [x] I listed at least one issue that this PR fixes in the description above. - [x] I updated/added relevant documentation (doc comments with `///`). - [x] I added new tests to check the change I am making, or this PR is [test-exempt]. - [x] I followed the [breaking change policy] and added [Data Driven Fixes] where supported. - [x] All existing and new tests are passing. If you need help, consider asking for advice on the #hackers-new channel on [Discord]. **Note**: The Flutter team is currently trialing the use of [Gemini Code Assist for GitHub](https://developers.google.com/gemini-code-assist/docs/review-github-code). Comments from the `gemini-code-assist` bot should not be taken as authoritative feedback from the Flutter team. If you find its comments useful you can update your code accordingly, but if you are unsure or disagree with the feedback, please feel free to wait for a Flutter team member's review for guidance on which automated comments should be addressed. <!-- Links --> [Contributor Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#overview [AI contribution guidelines]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#ai-contribution-guidelines [Tree Hygiene]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md [test-exempt]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#tests [Flutter Style Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md [Features we expect every widget to implement]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md#features-we-expect-every-widget-to-implement [CLA]: https://cla.developers.google.com/ [flutter/tests]: https://github.com/flutter/tests [breaking change policy]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#handling-breaking-changes [Discord]: https://github.com/flutter/flutter/blob/main/docs/contributing/Chat.md [Data Driven Fixes]: https://github.com/flutter/flutter/blob/main/docs/contributing/Data-driven-Fixes.md
When the Flutter.xcframework (and various [other plugins flagged as Privacy-impacting](https://developer.apple.com/support/third-party-SDK-requirements/)) are embedded in a native app using our add to app instructions, it is required for them to be code-signed before build. This PR will codesign all produced XCFrameworks with either the the signing identity passed into the command, found through the Flutter app's Xcode project's settings, or found through the Flutter config. If it's unable to find an identity or it finds more than one, it'll throw and ask the user to pass in an identity. See [go/flutter-ios-privacy-impacting-sdks-codesign-requirement](http://goto.google.com/flutter-ios-privacy-impacting-sdks-codesign-requirement) for explanation of code signature requirement. Fixes flutter#148300. ## Pre-launch Checklist - [x] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [x] I read the [AI contribution guidelines] and understand my responsibilities, or I am not using AI tools. - [x] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [x] I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement]. - [x] I signed the [CLA]. - [x] I listed at least one issue that this PR fixes in the description above. - [x] I updated/added relevant documentation (doc comments with `///`). - [x] I added new tests to check the change I am making, or this PR is [test-exempt]. - [x] I followed the [breaking change policy] and added [Data Driven Fixes] where supported. - [x] All existing and new tests are passing. If you need help, consider asking for advice on the #hackers-new channel on [Discord]. **Note**: The Flutter team is currently trialing the use of [Gemini Code Assist for GitHub](https://developers.google.com/gemini-code-assist/docs/review-github-code). Comments from the `gemini-code-assist` bot should not be taken as authoritative feedback from the Flutter team. If you find its comments useful you can update your code accordingly, but if you are unsure or disagree with the feedback, please feel free to wait for a Flutter team member's review for guidance on which automated comments should be addressed. <!-- Links --> [Contributor Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#overview [AI contribution guidelines]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#ai-contribution-guidelines [Tree Hygiene]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md [test-exempt]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#tests [Flutter Style Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md [Features we expect every widget to implement]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md#features-we-expect-every-widget-to-implement [CLA]: https://cla.developers.google.com/ [flutter/tests]: https://github.com/flutter/tests [breaking change policy]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#handling-breaking-changes [Discord]: https://github.com/flutter/flutter/blob/main/docs/contributing/Chat.md [Data Driven Fixes]: https://github.com/flutter/flutter/blob/main/docs/contributing/Data-driven-Fixes.md
When the Flutter.xcframework (and various other plugins flagged as Privacy-impacting) are embedded in a native app using our add to app instructions, it is required for them to be code-signed before build.
This PR will codesign all produced XCFrameworks with either the the signing identity passed into the command, found through the Flutter app's Xcode project's settings, or found through the Flutter config. If it's unable to find an identity or it finds more than one, it'll throw and ask the user to pass in an identity.
See go/flutter-ios-privacy-impacting-sdks-codesign-requirement for explanation of code signature requirement.
Fixes #148300.
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.
Note: The Flutter team is currently trialing the use of Gemini Code Assist for GitHub. Comments from the
gemini-code-assistbot should not be taken as authoritative feedback from the Flutter team. If you find its comments useful you can update your code accordingly, but if you are unsure or disagree with the feedback, please feel free to wait for a Flutter team member's review for guidance on which automated comments should be addressed.