From 869c69f4cdd6dc6e493bc3102e764fa11df930e1 Mon Sep 17 00:00:00 2001 From: Xilai Zhang Date: Mon, 22 Jan 2024 15:18:59 -0800 Subject: [PATCH 1/2] [codesign] codesign framework bundle (#3429) **context:** https://github.com/flutter/flutter/issues/140934 **what I learnt:** 1. out of our current [release bundles downloaded from gcs](https://pantheon.corp.google.com/storage/browser/flutter_infra_release/flutter/4f18bb4dcb99d7e0d94aeff2ffa74e6ddef301cd;tab=objects?authuser=0&project=flutter-infra&prefix=&forceOnObjectsSortingFiltering=false), only Flutter.xcframework is not codesigned. Both FlutterMacOS.framework and Flutter.xcframework/*/Flutter.framework are already codesigned. I will explain in more details on why this happens in bullet point 5 below.
The existing bundles are verified using `codesign --verify --verbose --display `. 2. According to Eskimo's reply in [this post](https://developer.apple.com/forums/thread/661852) and [this other post](https://developer.apple.com/forums/thread/129678). For nested bundle / framework we should codesign them from the inside-out. And thus in this PR, we codesign the binaries and sub-bundles inside our bundle first, and then codesign the entire bundle. 3. Both .xcframework and .framework direcotry/folders qualify as bundles.
According to the [in-depth developer doc on code signing](https://developer.apple.com/library/archive/documentation/Security/Conceptual/CodeSigningGuide/Procedures/Procedures.html): `Bundles must have their Info.plist in the proper location. For app bundles, this is in Contents. For frameworks, this is in Versions/Current/Resources.` So from the look of our folders, FluttMacOs.framework and Flutter.xcframework/*/Flutter.framework seem to qualify as bundles, but Flutter.xcframework doesn't. However, when running the codesign tool it recognizes Flutter.xcframework as bundle: `Flutter.xcframework/: signed bundle`. 4. We are currently signing version `Current` in the bundle, which I think would be good enough.
According to the [in-depth developer doc on code signing](https://developer.apple.com/library/archive/documentation/Security/Conceptual/CodeSigningGuide/Procedures/Procedures.html): `When you manually code sign a framework, only the current version, the one pointed at by the Versions/Current symbolic link, is signed by default.` For our bundle, we only have Version A and Version Current, and since Version Current is a symlink to Version A, we codesigned all of our versions. 5. [my theory] some bundles are already codesigned because the main executable of the bundle is codesigned
Eskimo hinted in [this post](https://developer.apple.com/forums/thread/129678) that `The only exception to this is the main executable of a bundle, where signing the bundle takes care of signing its main executable`. And [Apple Technical Note 2206](https://developer.apple.com/library/archive/technotes/tn2206/_index.html) pointed out that `Most frameworks contain a single version and can in fact be signed directly.`
This could provide a theory on how our `FlutterMacOS.framework` bundle is already code signed because the major executable` Versions/A/FlutterMacOS` is codesigned. And our `Flutter.xcFramework/*/Flutter.framework` bundle is codesigned because the major executable `Flutter.xcFramework/*/Flutter.framework/Flutter` is code signed. However, .xcFramework itself is not already codesigned and will be included through the change in this PR. **Tested**: The output artifact of this PR is [built and tested](https://github.com/flutter/cocoon/tree/main/cipd_packages/codesign) The output artifact contains a code signed xcFramework, verified using the command mentioned in bullet point 1. **TODO**: deploy this current version of codesign and set cipd label to 'live' when this PR is merged. --- .../lib/src/file_codesign_visitor.dart | 19 ++++- .../test/file_codesign_visitor_test.dart | 81 +++++++++++++++++++ 2 files changed, 96 insertions(+), 4 deletions(-) diff --git a/cipd_packages/codesign/lib/src/file_codesign_visitor.dart b/cipd_packages/codesign/lib/src/file_codesign_visitor.dart index 31f7a6844..042c05185 100644 --- a/cipd_packages/codesign/lib/src/file_codesign_visitor.dart +++ b/cipd_packages/codesign/lib/src/file_codesign_visitor.dart @@ -255,6 +255,10 @@ update these file paths accordingly. } log.info('Child file of directory ${directory.basename} is ${entity.basename}'); } + final String directoryExtension = directory.basename.split('.').last; + if (directoryExtension == 'framework' || directoryExtension == 'xcframework') { + await codesignAtPath(binaryOrBundlePath: directory.absolute.path); + } } /// Unzip an [EmbeddedZip] and visit its children. @@ -314,17 +318,24 @@ update these file paths accordingly. if (dryrun) { return; } + await codesignAtPath(binaryOrBundlePath: binaryFile.absolute.path, entitlementCurrentPath: entitlementCurrentPath); + } + + Future codesignAtPath({ + required String binaryOrBundlePath, + String? entitlementCurrentPath, + }) async { final List args = [ '/usr/bin/codesign', '--keychain', 'build.keychain', // specify the keychain to look for cert - '-f', // force + '-f', // force. Needed to overwrite signature if major executable of bundle is already signed before bundle is signed. '-s', // use the cert provided by next argument codesignCertName, - binaryFile.absolute.path, + binaryOrBundlePath, '--timestamp', // add a secure timestamp '--options=runtime', // hardened runtime - if (fileWithEntitlements.contains(entitlementCurrentPath)) ...[ + if (entitlementCurrentPath != '' && fileWithEntitlements.contains(entitlementCurrentPath)) ...[ '--entitlements', entitlementsFile.absolute.path, ], @@ -338,7 +349,7 @@ update these file paths accordingly. } throw CodesignException( - 'Failed to codesign ${binaryFile.absolute.path} with args: ${args.join(' ')}\n' + 'Failed to codesign binary or bundle at $binaryOrBundlePath with args: ${args.join(' ')}\n' 'stdout:\n${(result.stdout as String).trim()}' 'stderr:\n${(result.stderr as String).trim()}', ); diff --git a/cipd_packages/codesign/test/file_codesign_visitor_test.dart b/cipd_packages/codesign/test/file_codesign_visitor_test.dart index 53a2ee5fa..31e0b677d 100644 --- a/cipd_packages/codesign/test/file_codesign_visitor_test.dart +++ b/cipd_packages/codesign/test/file_codesign_visitor_test.dart @@ -561,6 +561,87 @@ void main() { ); }); + test('visitDirectory codesigns framework bundle', () async { + fileSystem + ..file('${rootDirectory.path}/remote_zip_6/non_bundle/file_a').createSync(recursive: true) + ..file('${rootDirectory.path}/remote_zip_6/bundle.xcframework/bundle.framework/file_b') + .createSync(recursive: true); + final Directory testDirectory = fileSystem.directory('${rootDirectory.path}/remote_zip_6'); + processManager.addCommands([ + FakeCommand( + command: [ + 'file', + '--mime-type', + '-b', + '${rootDirectory.absolute.path}/remote_zip_6/non_bundle/file_a', + ], + stdout: 'other_files', + ), + FakeCommand( + command: [ + 'file', + '--mime-type', + '-b', + '${rootDirectory.absolute.path}/remote_zip_6/bundle.xcframework/bundle.framework/file_b', + ], + stdout: 'other_files', + ), + FakeCommand( + command: [ + '/usr/bin/codesign', + '--keychain', + 'build.keychain', + '-f', + '-s', + randomString, + '${rootDirectory.absolute.path}/remote_zip_6/bundle.xcframework/bundle.framework', + '--timestamp', + '--options=runtime', + ], + exitCode: 0, + ), + FakeCommand( + command: [ + '/usr/bin/codesign', + '--keychain', + 'build.keychain', + '-f', + '-s', + randomString, + '${rootDirectory.absolute.path}/remote_zip_6/bundle.xcframework', + '--timestamp', + '--options=runtime', + ], + exitCode: 0, + ), + ]); + await codesignVisitor.visitDirectory( + directory: testDirectory, + parentVirtualPath: '', + ); + final Set messages = records + .where((LogRecord record) => record.level == Level.INFO) + .map((LogRecord record) => record.message) + .toSet(); + expect(messages, contains('Visiting directory ${rootDirectory.path}/remote_zip_6/non_bundle')); + expect( + messages, + contains('Visiting directory ${rootDirectory.path}/remote_zip_6/bundle.xcframework/bundle.framework'), + ); + expect( + messages, + contains( + 'Executing: /usr/bin/codesign --keychain build.keychain -f -s $randomString ${rootDirectory.path}/remote_zip_6/bundle.xcframework/bundle.framework --timestamp --options=runtime\n', + ), + ); + expect( + messages, + contains( + 'Executing: /usr/bin/codesign --keychain build.keychain -f -s $randomString ${rootDirectory.path}/remote_zip_6/bundle.xcframework --timestamp --options=runtime\n', + ), + ); + }); + test('visitBinary codesigns binary with / without entitlement', () async { codesignVisitor = cs.FileCodesignVisitor( codesignCertName: randomString, From a22d1adc7de976201758500d2ee44d0a1ec8f121 Mon Sep 17 00:00:00 2001 From: keyonghan <54558023+keyonghan@users.noreply.github.com> Date: Tue, 23 Jan 2024 19:58:06 -0800 Subject: [PATCH 2/2] Skip flake status check when target no longer exists (#3441) Fixes: https://github.com/flutter/flutter/issues/142073 --- .../file_flaky_issue_and_pr.dart | 22 ++++++++++++++----- .../file_flaky_issue_and_pr_test.dart | 22 +++++++++++++++++++ 2 files changed, 39 insertions(+), 5 deletions(-) diff --git a/app_dart/lib/src/request_handlers/file_flaky_issue_and_pr.dart b/app_dart/lib/src/request_handlers/file_flaky_issue_and_pr.dart index 10ab0bc95..6bca5d3cd 100644 --- a/app_dart/lib/src/request_handlers/file_flaky_issue_and_pr.dart +++ b/app_dart/lib/src/request_handlers/file_flaky_issue_and_pr.dart @@ -55,11 +55,7 @@ class FileFlakyIssueAndPR extends ApiRequestHandler { final Map nameToExistingPR = await getExistingPRs(gitHub, slug); int filedIssueAndPRCount = 0; for (final BuilderStatistic statistic in builderStatisticList) { - // Skip if ignore_flakiness is specified. - if (getIgnoreFlakiness(statistic.name, ciYaml)) { - continue; - } - if (statistic.flakyRate < _threshold) { + if (shouldSkip(statistic, ciYaml, targets)) { continue; } @@ -93,6 +89,22 @@ class FileFlakyIssueAndPR extends ApiRequestHandler { }); } + bool shouldSkip(BuilderStatistic statistic, CiYaml ciYaml, List targets) { + // Skips if the target has been removed from .ci.yaml. + if (!targets.map((e) => e.name).toList().contains(statistic.name)) { + return true; + } + // Skips if ignore_flakiness is specified. + if (getIgnoreFlakiness(statistic.name, ciYaml)) { + return true; + } + // Skips if the flaky percentage is below the threshold. + if (statistic.flakyRate < _threshold) { + return true; + } + return false; + } + double get _threshold => double.parse(request!.uri.queryParameters[kThresholdKey]!); Future _fileIssueAndPR( diff --git a/app_dart/test/request_handlers/file_flaky_issue_and_pr_test.dart b/app_dart/test/request_handlers/file_flaky_issue_and_pr_test.dart index f5ec497f2..1c44b27e8 100644 --- a/app_dart/test/request_handlers/file_flaky_issue_and_pr_test.dart +++ b/app_dart/test/request_handlers/file_flaky_issue_and_pr_test.dart @@ -672,6 +672,28 @@ void main() { expect(result['Status'], 'success'); }); + + test('skips when the target doesn not exist', () { + final YamlMap? ci = loadYaml(ciYamlContent) as YamlMap?; + final pb.SchedulerConfig unCheckedSchedulerConfig = pb.SchedulerConfig()..mergeFromProto3Json(ci); + final CiYaml ciYaml = CiYaml( + slug: Config.flutterSlug, + branch: Config.defaultBranch(Config.flutterSlug), + config: unCheckedSchedulerConfig, + ); + final BuilderStatistic builderStatistic = BuilderStatistic( + name: 'Mac_android test', + flakyRate: 0.5, + flakyBuilds: ['103', '102', '101'], + succeededBuilds: ['203', '202', '201'], + recentCommit: 'abc', + flakyBuildOfRecentCommit: '103', + flakyNumber: 3, + totalNumber: 6, + ); + final List targets = unCheckedSchedulerConfig.targets; + expect(handler.shouldSkip(builderStatistic, ciYaml, targets), true); + }); }); test('retrieveMetaTagsFromContent can work with different newlines', () async {