Let xcodebuild authenticate with an App Store Connect API key (1.10.0) - #74
Conversation
`-allowProvisioningUpdates` lets Xcode create or refresh a provisioning profile, but on its own it can only do that through a signed-in Xcode account. On a machine without a usable one -- CI, or a developer who only has an API key -- xcodebuild cannot fetch the profile and quietly settles for a cached wildcard instead. Any app with an entitlement then fails, and fails misleadingly: error: Provisioning profile "tvOS Team Provisioning Profile: *" doesn't include the Game Center capability. That names the capability the wildcard profile lacks, not the credential that is actually missing, so the obvious next step is to go checking the App ID's capabilities in the developer portal -- where everything is already correct. The App ID in the case that prompted this had Game Center enabled all along; what it had no tvOS development profile. xcodebuild accepts an API key as the alternative, so forward one when the environment provides all three of: APP_STORE_CONNECT_KEY_PATH the .p8 private key APP_STORE_CONNECT_KEY_ID its key id APP_STORE_CONNECT_ISSUER_ID the issuer id Absent or incomplete, nothing is added and a machine with a working Xcode account behaves exactly as before. Set but pointing at a file that does not exist earns a warning rather than a silent fallback, because staying quiet there is indistinguishable from never having configured it -- and the build goes on to fail with the capability message above, several minutes later and nowhere near the cause. Only the key id is ever logged; the key's contents are read by xcodebuild, never by this process. `resolveAuthenticationArgs` is top-level and takes its environment, file system and logger as arguments so the tests can drive every branch directly rather than restating the logic.
e4b1758 to
4066b27
Compare
MAUstaoglu
changed the title
`shared.sh` only recompiles bin/cache/flutter-tvos.snapshot when the git revision changes, so editing lib/ and running the CLI silently executes the previously compiled snapshot. The change does not look ignored, it looks wrong: same build, same error, byte for byte. Fixing it in the tool was the first instinct, and measuring talked me out of it. Detecting a dirty tree costs ~28ms of git status on every invocation -- about 6% of a 493ms startup -- paid by every user of a public CLI to help only the few people editing it. A note costs nothing and reaches the same people.
4066b27 to
83db85a
Compare
MAUstaoglu
changed the title
83db85a to
c799086
Compare
MAUstaoglu
changed the title
Minor rather than patch: the API-key path is a new capability, and it is additive -- absent the three environment variables nothing changes for a machine with a working Xcode account. Note that CONTRIBUTING.md asks contributors to leave the version alone and file under [Unreleased], because maintainers assign numbers when cutting a release. This is that call being made deliberately rather than the convention being missed.
MAUstaoglu
changed the title
DenisovAV
left a comment
DenisovAV
left a comment
There was a problem hiding this comment.
The premise checks out — -authenticationKeyPath / -authenticationKeyID / -authenticationKeyIssuerID are exact per man xcodebuild, all-three-or-nothing matches Apple's documented requirement, and the argument only reaching device builds is right. What follows is about the paths where the feature is configured but doesn't take.
The stated principle isn't implemented for the case CI actually hits
application.dart:447-453 returns const <String>[] with no output at any verbosity whenever the trio is incomplete or any value is empty. The missing-file branch got a warning on the reasoning that "silently falling back looks identical to never having configured it" — partial configuration is that same failure with a likelier cause, and gets nothing.
In GitHub Actions an undefined secret interpolates to the empty string, not to an absent variable:
APP_STORE_CONNECT_KEY_ID: ${{ secrets.ASC_KEY_ID }} # secret missing → ""All three variables are set. The operator has demonstrably configured the feature. The gate treats it as unconfigured, says nothing, and the build dies minutes later with the misleading capability error this PR exists to eliminate — with nothing in the log connecting the two.
The gate is correct to refuse a partial flag set; the defect is that it is mute. Presence in the environment map separates "configured something" from "configured nothing" at no cost.
Two things make this more likely than it looks:
- The variable names are asymmetric.
APP_STORE_CONNECT_KEY_PATH,APP_STORE_CONNECT_KEY_ID, thenAPP_STORE_CONNECT_ISSUER_ID, dropping theKEY_. The flag it maps to is-authenticationKeyIssuerID, soAPP_STORE_CONNECT_KEY_ISSUER_IDis the natural guess, and it is silently ignored. README.mddocuments none of the three. They appear only inCHANGELOG.mdand the dartdoc, so a user is guessing from the flag name. The README diff here is the version bump alone.
The success path is equally silent: printTrace at :465 is a literal no-op in StdoutLogger (logger.dart:563, class opens at :427), so at default verbosity "working", "half-configured" and "never configured" are indistinguishable from the log.
A literal ~ is never expanded, which breaks the documented CI form
The CHANGELOG and dartdoc both show:
export APP_STORE_CONNECT_KEY_PATH=~/.appstoreconnect/private_keys/AuthKey_XXXX.p8Unquoted in an interactive shell that works, because the shell expands it first. A CI env: block, a quoted assignment, a .env file and an Xcode scheme variable all deliver a literal ~:
absolute('~/.appstoreconnect/…/AuthKey_ABC.p8') → /~/.appstoreconnect/…/AuthKey_ABC.p8
existsSync() → false
The warning fires, but reads "…does not exist" and never names the tilde as the cause. Same line, same class: values are not trimmed, so KEY_ID=$(cat keyid) or any secret store that appends a newline forwards it verbatim — and a trailing \n in a path renders a warning whose path looks correct because the newline is invisible.
The warning lands where this file says warnings are useless
resolveAuthenticationArgs is called during construction of the argument list at :615-620, inside the try that follows startProgress('Running Xcode build...') at :588. It renders — printWarning pauses the spinner — but at the top of a multi-minute build, and if the build then fails, :628-630 dump the whole of xcodebuild's stdout and stderr on top of it.
:635-639 states the rule this violates, about its own migration guards: emitted after the build "so they are the last thing on screen — pod install and the multi-minute Xcode spinner would otherwise scroll an earlier warning out of view." Hoisting the call next to _resolveSigningArgs at :586 puts it above the spinner.
"Only the key id is ever logged" is false two ways
The claim is in the dartdoc (:436-437) and the CHANGELOG.
:459-462logs the full key path, andtvos_code_signing_test.dart:226deliberately pins it there.- xcodebuild echoes its own command line into stdout, and
:629pipes that toprintErroron failure — so path, key id and issuer id all land in visible output on every failed device build.
The .p8 contents genuinely never pass through the CLI, so nothing secret leaks. It is an explicit security claim that doesn't hold as written.
The tests don't hold what the description says they hold
resolveAuthenticationArgsis top-level and takes its environment, file system and logger as arguments, so the tests drive every branch directly
Four of eight decision points are unpinned. Mutating the source and re-running:
| mutation | result |
|---|---|
| delete the spread at the xcodebuild call site | survives |
drop the !buildInfo.simulator guard at the call site |
survives |
drop the .isEmpty guards |
survives |
drop fileSystem.path.absolute(keyPath) |
survives |
delete the existsSync() block |
fails |
| swap the key id and issuer id values | fails |
printWarning → printTrace |
fails |
The first is the one that matters: the feature can be disconnected from the build entirely and the suite stays green.
treats an empty value as unset passes for the wrong reason — it only empties path, and an empty path falls through to existsSync(''), which is also false. Remove the .isEmpty guards and it still returns [], now with a garbage warning reading "…but does not exist". Empty KEY_ID and ISSUER_ID are never tested, and empty-not-null is the realistic CI shape of an unset secret.
path.absolute() is load-bearing rather than cosmetic: existsSync() resolves against the CLI's cwd while xcodebuild runs with workingDirectory: tvosProjectDir.path, so without it a relative key path passes the existence check and is then handed to xcodebuild one directory deeper. Every test passes an already-absolute path, so the line is invisible to them.
The new function took NativeTvosBundle's documentation
The function was inserted between the class's doc comment and the class, with no blank line between :416 (/// 6. Invoke xcodebuild targeting appletvos or appletvsimulator SDK) and :417. The merged block now documents resolveAuthenticationArgs, whose summary sentence reads "Orchestrates the native tvOS build via xcodebuild" followed by the six-step build pipeline, and NativeTvosBundle at :476 has no doc comment at all.
Two notes on files this PR already touches
CONTRIBUTING.md:47 says "There are 74 unit tests in test/general/" — there are 436, across 40 files.
The new snapshot section is right about the mechanism (shared.sh:32-46: a git checkout keys off rev-parse HEAD, a non-git install shasums bin and lib), but "an edit under lib/ or bin/ therefore does nothing until you remove it" holds only for uncommitted edits — committing moves HEAD and rebuilds.
Unrelated to this change, but in the group your new tests sit beside: Code signing - simulator vs device asserts expect(const <String>[], isEmpty) and expect(isSimulator, isFalse) against local constants. Neither test can fail. This PR is the first thing in that file to import production code at all.
The gate was right to refuse a partial flag set -- xcodebuild rejects the
key arguments unless all three are present -- but it returned an empty
list with no output at any verbosity, which is the exact failure this
feature exists to prevent. An undefined GitHub Actions secret interpolates
to the empty string rather than to an absent variable, so all three
variables are set, the operator has demonstrably configured this, and the
build still died minutes later with the misleading capability error and
nothing in the log connecting the two.
Presence in the environment map separates "configured nothing" from
"configured something broken" at no cost, and the warning names the
missing variable -- worth doing because the names are asymmetric
(_KEY_PATH, _KEY_ID, then _ISSUER_ID), so APP_STORE_CONNECT_KEY_ISSUER_ID
is the natural guess and was silently ignored.
Also on that path:
- Expand a leading `~`. An interactive shell expands it before the CLI
sees it, but a CI env: block, a quoted assignment, a .env file and an
Xcode scheme variable all deliver it literally, where absolute() turned
it into /~/... and existsSync failed with a warning that never named
the tilde as the cause.
- Trim values. `KEY_ID=$(cat keyid)` and most secret stores append a
newline; in a path the invisible \n renders a warning whose path looks
perfectly correct.
- printStatus rather than printTrace on success. StdoutLogger.printTrace
is a literal no-op, so at default verbosity "working", "half-configured"
and "never configured" were indistinguishable from the log.
- Resolve the args before startProgress. The warning rendered, but at the
top of a multi-minute build and under xcodebuild's full stdout/stderr
if the build then failed -- the same reason the migration guards below
are deliberately emitted after the build.
Extract the xcodebuild argv into NativeTvosBundle.xcodebuildArgs, matching
aotAssembleArgs and aotLinkArgs. Four decision points were unpinned
before: the feature could be unhooked from the build entirely, the
simulator guard dropped, the .isEmpty guards dropped and absolute()
dropped, all with the suite still green. `treats an empty value as unset`
passed for the wrong reason -- it only emptied the path, which then fell
through to existsSync(''), also false.
Tests: 5 -> 17, and all eight mutations in that table now fail. Drops the
two cases in `Code signing - simulator vs device`, which asserted against
local constants and could not fail; the new wiring group makes the same
two claims against production code.
Full suite 388 passing / 58 failing, against the same 58 on dev.
README documented none of the three variables -- they appeared only in CHANGELOG.md and the dartdoc, so a user was guessing them from the xcodebuild flag names, and guessing wrong on the issuer one. Adds them to the Code signing section, where the engine-signing variables already live, with the failure they fix and a note that the third has no KEY_. "Only the key id is ever logged" was false two ways: the warning path logs the full key path, and xcodebuild echoes its own command line into stdout, which the caller dumps to printError on every failed device build -- so path, key id and issuer id all reach visible output. Nothing secret leaks, because the .p8 contents genuinely never pass through the CLI, but it was an explicit security claim that did not hold as written. States what is actually true instead, in both the CHANGELOG and the dartdoc.
"There are 74 unit tests in test/general/" -- there are roughly 450, across 40 files. Gives an approximate figure rather than an exact one, which is what went stale, and adds the baseline a contributor actually needs: the suite is not green, dev reports 58 failures at HEAD, so compare against that rather than against zero. The snapshot section was right about the mechanism but overstated it: shared.sh keys off `git rev-parse HEAD`, so committing an edit moves HEAD and rebuilds. It is *uncommitted* edits that are silently ignored, which is the edit-and-try loop specifically.
|
All seven addressed, in three commits. The mute partial-configuration path was the real defect — the gate now tests presence in the environment map, so an empty-string CI secret is recognised as configured and warned about, naming the missing variable and the The security claim was wrong both ways you describe; the dartdoc and CHANGELOG now say the On the tests — fair hit. Extracted CONTRIBUTING: count corrected (approximate, since exactness is what went stale) plus the 58-failure baseline, and the snapshot note now says uncommitted edits. 388 passing / 58 failing, against the same 58 on |
The previous commit asserted the suite is not green and that dev reports 58 failures at HEAD. That is wrong, and the number was an artifact of how I ran it. Both dev and this branch are green: 431 and 446 passing, zero failing, which is exactly the +15 net tests this branch adds. The 58 came from running `dart test` without the resolved TMPDIR that CI passes. Flutter's test harness installs an FS guard that resolves symlinks when computing the allowed temp root but not on the path it checks; $TMPDIR is /var/folders/... and /var is a symlink to /private/var, so the two never compare equal. The root cause is in this file: the documented command omitted the prefix, so anyone following CONTRIBUTING hits ~58 phantom failures that look like real breakage. test/README.md has carried the explanation and the correct invocation all along, and CI uses it -- CONTRIBUTING just never picked it up. Adds the prefix to the command, states the suite is green, and explains the trap rather than baking a bogus baseline into the docs.
|
Correction to my previous comment: the 58-failure baseline is wrong, and so was the same claim in the PR description. Both
The 58 came from running The reason I hit it is in Apologies for putting a made-up number in front of you twice — the original "373 → 378, same 58 on dev" in the description was the same artifact. |
1.9.0 stopped publishing origin-signed artifacts and signs the engine locally instead, but it signed with `Developer ID Application`. Only a team's Account Holder can create one, there is a per-team cap, and a tvOS developer has no other reason to hold one -- it is a macOS outside-the-App-Store certificate. A developer without one got a warning on an otherwise successful build and an unsigned engine in the bundle, then ITMS-91065 at submission. That is a regression against 1.4.0-1.8.0, where the published artifact carried the maintainer's signature and nothing was asked of the developer. It reached a customer, who upgraded and was rejected on a submission that worked before. `Apple Distribution` is the certificate every developer who can upload to TestFlight already has, so signing now asks nothing extra of anyone. Confirmed against Apple rather than reasoned about: Crown Breaker 1.2.1 build 23, engine signed `Apple Distribution` before embedding, processed to VALID and cleared external testing. The same app's unsigned builds (16, 17, 19, 20) are INVALID and its Developer-ID builds (21, 22) are VALID. Developer ID also satisfies the check and is what flutter.dev signs its own engine with, but it is deliberately not accepted: nobody shipping a tvOS app needs one, so supporting it would only add a branch that almost never fires. A development certificate is still refused -- it is not known to satisfy the check, and silently using one would hide the real problem. Restoring the old certificate type fails the suite, so the gap cannot reopen silently.
26eea54 to
3d01f80
Compare
DenisovAV
left a comment
DenisovAV
left a comment
There was a problem hiding this comment.
Verified against 3d01f80 by mutating the source and re-running, not from the description.
The API-key work
All four survivors from the previous round are dead, and the branches the fix introduced are pinned too:
| mutation | result |
|---|---|
drop authenticationArgs from the xcodebuild argv |
fails |
| drop the id/issuer emptiness guards | fails (−4) |
drop fileSystem.path.absolute() |
fails |
drop the .trim() on all three values |
fails |
never expand a leading ~ |
fails |
| warn when nothing is configured | fails (−2) |
| never warn on partial configuration | fails (−3) |
printStatus back to printTrace |
fails |
| drop the simulator guard on resolution | survives |
The survivor is benign for the argv — xcodebuildArgs carries its own if (!isSimulator) ...authenticationArgs, and that guard is pinned by the first mutation, so the flags still cannot reach a simulator build. What is unpinned is the diagnostics: with the outer guard gone, a simulator build on a machine with the three variables exported would emit the printStatus line, or a provisioning-fallback warning, for a build that is never signed. Worth a test only if you want the messaging pinned; correctness is covered.
Resolving before existsSync rather than after is a better fix than the one I asked for. Previously the check ran against the raw value while the flag carried the absolutised one; now both use resolved, so what is checked and what xcodebuild receives cannot diverge — that removes the relative-path discrepancy at the source instead of compensating for it. _expandTilde taking environment rather than reading HOME from the process keeps the no-HOME branch testable, and passing the ~ through with a warning that says so is the right fallback.
The suite is green at 447 passing, 0 failing with the documented invocation, and CONTRIBUTING.md now carries the resolved-TMPDIR prefix that test/README.md had all along. That is worth more than the count it replaced: the previous line sent anyone following it into ~58 phantom failures that read as real breakage.
The engine-signing commit
The regression is real and the evidence is sound. Restricting to Developer ID Application is a genuine break from 1.4.0–1.8.0: only a team's Account Holder can create one, there is a per-team cap, and a tvOS developer has no other reason to hold a macOS outside-the-App-Store certificate — so the practical outcome was an unsigned engine and ITMS-91065 on submissions that passed before the upgrade. The Crown Breaker builds separate the variables properly: 23 VALID and cleared external review with the engine signed Apple Distribution, 16/17/19/20 INVALID unsigned, 21/22 VALID with Developer ID.
One thing to confirm you meant, because it changed between revisions. The earlier version accepted Apple Distribution in addition to Developer ID Application; this one accepts it instead. That turns a purely additive fix into a substitution, and it narrows what 1.9.0 required: a developer who created a Developer ID because 1.9.0's warning told them to, and who has no Apple Distribution, goes from signed to unsigned. In practice that set is close to empty — nobody uploads to App Store Connect without a distribution certificate — but the CHANGELOG's "No action is needed on an existing project" is written for the additive version, and it is the one line a reader in that position would rely on. Either widen it back or say plainly that Developer ID is no longer used.
Two comments did not follow the code:
tvos_engine_signing.dart:30— "Signs engine artifacts with a Developer ID from the local keychain.":247— "a developer without a Developer ID can still build and run locally"
Mutations: reverting kIdentityPrefix to Developer ID Application: fails (−5), and adding Apple Development: to the match fails (−7), so the 1.9.0 behaviour cannot return silently and the refusal of development certificates is pinned. Widening the match to accept Developer ID Application as well survives — harmless, since both satisfy the check, but "deliberately the only accepted type" is a claim nothing holds.
Scope
This is a customer-facing regression fix for a submission path that is broken today, sitting in a PR titled for API-key authentication that has now been through two review rounds. Bundling it means the fix waits on the feature, the 1.10.0 entry describes two unrelated changes under one heading, and the regression is invisible to anyone reading release notes for the signing behaviour that bit them. Cherry-picking it onto its own PR against dev costs nothing and can land immediately.
Approving on the API-key work. Whether the signing fix rides along is your call — it should not be what delays it.
2 participants
Targets
devperCONTRIBUTING.md.Version: 1.9.0 → 1.10.0, at the maintainer's request. Minor rather than patch — the API-key path is a new capability and is additive. For the record,
CONTRIBUTING.mdasks contributors to leave the version alone and file under[Unreleased]; this is that call being made deliberately, not the convention being missed.The problem
-allowProvisioningUpdateslets Xcode create or refresh a provisioning profile, but on its own it can only do so through a signed-in Xcode account. On a machine without a usable one — CI, or a developer who only has an API key — xcodebuild cannot fetch the profile and quietly settles for a cached wildcard one instead.Any app with an entitlement then fails, and fails misleadingly:
That names the capability the wildcard lacks, not the credential that is actually missing. The obvious next step is to go auditing the App ID's capabilities in the developer portal — where everything is already correct.
How it came up
An Apple TV device build of an app carrying
com.apple.developer.game-centerfailed with exactly that. The App ID wasUNIVERSALand already hadGAME_CENTERenabled; querying the App Store Connect API showed the real gap was that noTVOS_APP_DEVELOPMENTprofile existed for that bundle at all — iOS, App Store, watch and Mac profiles were all present.With the key forwarded, xcodebuild created the missing profile and the same build signed, installed and launched on the device unchanged.
The change
Forward
-authenticationKeyPath/-authenticationKeyID/-authenticationKeyIssuerIDwhen the environment supplies all three:Additive. Set none of them and nothing is added: a machine with a working Xcode account behaves exactly as before, silently.
Anything between that and a working setup is reported. xcodebuild requires all three or none, so a partial set is not forwarded — but it is not swallowed either. The gate tests presence in the environment, not a non-empty value, because an undefined CI secret interpolates to the empty string rather than to an absent variable: all three are set, the operator has demonstrably configured this, and treating it as unconfigured loses the only signal there is. The warning names the missing variable, including the
APP_STORE_CONNECT_KEY_ISSUER_IDguess that the asymmetric naming invites. A key path that does not exist warns too, and says so when a literal~is the reason.Input handling. Values are trimmed —
KEY_ID=$(cat keyid)and most secret stores append a newline, and in a path the invisible\nrenders a warning whose path looks perfectly correct. A leading~is expanded againstHOME: an interactive shell expands it before the CLI sees it, but a CIenv:block, a quoted assignment, a.envfile and an Xcode scheme variable all deliver it literally. The path is made absolute, becauseexistsSyncresolves against the CLI's cwd while xcodebuild runs withworkingDirectory: tvosProjectDir.path.Output. A successful handover names the key id at default verbosity, so "working", "half-configured" and "never configured" can be told apart from the log. The
.p8contents never pass through the CLI — only the path is read, and xcodebuild opens the file itself. The path, key id and issuer id are not secret and do appear in output: on that success line, and in xcodebuild's own echoed command line, which the caller dumps toprintErroron a failed build.The args are resolved before
startProgress, not inside the argument list, so a warning is not emitted at the top of a multi-minute build and then buried under xcodebuild's stdout and stderr — the same reasoning the migration guards below already follow.Tests
17 cases in
test/general/tvos_code_signing_test.dartcovering the forwarding path, each way the trio can be incomplete or empty, trimming, tilde expansion with and withoutHOME, relative-path resolution, the missing-file fallback, and the success log.resolveAuthenticationArgstakes its environment, file system and logger as arguments, so those drive it directly. The call site needed extracting to be reachable at all:NativeTvosBundle.xcodebuildArgsnow assembles the argv, following the existingaotAssembleArgs/aotLinkArgspattern, and is tested for forwarding the signing and authentication args on a device build and neither on a simulator. Without it the feature could be unhooked from the build entirely with the suite still green.Also drops the two cases in
Code signing - simulator vs device, which assertedexpect(const <String>[], isEmpty)andexpect(isSimulator, isFalse)against local constants and could not fail. The new group makes both claims against production code.Suite is green, run as CI runs it:
dev(cc9caf5) 431 passing / 0 failing, this branch 446 / 0 — the +15 being the net change here.dart analyze --fatal-infos lib/ test/ bin/clean.Also included: two docs fixes
README.mddocumented none of the three variables — they appeared only inCHANGELOG.mdand the dartdoc, so a user was guessing them from the xcodebuild flag names, and guessing wrong on the issuer. They are now in the Code signing section alongside the engine-signing ones.CONTRIBUTING.md: the documented test command omitted the resolvedTMPDIRthat.github/workflows/test.ymlpasses andtest/README.mdexplains, so anyone following it hits ~58 phantom failures from the FS guard's symlink mismatch — which is exactly how the bogus baseline above got into this description. Prefix added, and the stale "74 unit tests" corrected (there are ~450 across 40 files).It also gains a note on a gotcha that cost me a build cycle:
shared.shonly recompilesbin/cache/flutter-tvos.snapshotwhengit rev-parse HEADchanges, so an uncommitted edit underlib/silently runs the previously compiled snapshot — same build, same error, byte for byte, reading as a wrong fix rather than one that never executed. I first fixed this intool_revisionand then measured it, which argued against shipping it: detecting a dirty tree costs ~28 ms ofgit statuson every invocation, about 6% of a 493 ms startup, paid by every user of a public CLI to help only the few editing it. Happy to split either docs change out.