Skip to content

fix: survive locked-keychain notarization and add RC resume path - #1256

Merged
malpern merged 2 commits into
masterfrom
claude/youthful-bassi-e23580
Jul 31, 2026
Merged

fix: survive locked-keychain notarization and add RC resume path#1256
malpern merged 2 commits into
masterfrom
claude/youthful-bassi-e23580

Conversation

@malpern

@malpern malpern commented Jul 31, 2026

Copy link
Copy Markdown
Owner

Summary

The 2026-07-30 release candidate died at notarytool submit with "No Keychain password item found for profile: KeyPath-Profile" — nine minutes after release-doctor.sh had validated that exact profile in the same shell. Root cause (verified: the profile is absent from login.keychain-db): notarytool on Xcode 26.x stores profiles in the data-protection keychain, whose items are unreadable once the login session locks — so the credential vanished mid-build when the screen locked. security unlock-keychain cannot help (file keychains only). Recovery then required hand-stapling the extracted zip because a dev deploy had re-signed the installed app, changing its cdhash and detaching it from the eventual ticket.

Forensic note: build-and-sign.sh did abort at the failed submit (set -euo pipefail worked); the unstapled /Applications copy came from a later ui-deploy.sh re-sign. The guards below make both failure modes structurally impossible in the release path.

Changes

  • Lock-independent notarization auth. kp_notary_resolve_auth supports the file-based App Store Connect API key (--key/--key-id/--issuer); kp_notary_default_auth_from_environment prefers ~/.appstoreconnect/private_keys/AuthKey_XQ4565NYZ7.p8 when present (identifiers only, not secrets). Keychain-profile auth remains the fallback; KP_NOTARY_AUTH=keychain-profile forces it. release-doctor.sh now validates whichever auth path the build will actually use.
  • Explicit resume path. Scripts/release-candidate.sh --resume-notarization → new Scripts/notarize-resume.sh: verifies dist/KeyPath.zip still matches the recorded SHA-256, waits on the recorded submission (or resubmits the identical archive when nothing reached Apple; KP_NOTARY_SUBMISSION_ID disambiguates recorded candidates), staples dist/KeyPath.app, and deploys it verbatim. On failure, release-candidate.sh now prints exactly this resume command instead of a buried nonzero exit.
  • Never deploy unstapled / never re-sign on deploy. build-and-sign.sh refuses to deploy a bundle without a stapled ticket whenever notarization was requested (kp_staple_validate guard), and both deploy paths share new Scripts/lib/deploy-app.sh (stop app → ditto → relaunch; contains no codesign). Enforced by verify-release-signing-contract.sh and DeploymentScriptContractTests.
  • kp_notary_await_submission extracted from kp_notarize_zip so resume can wait on a recorded submission id without re-uploading.
  • Incident write-up in docs/bugs/2026-07-30-rc-notarization-keychain-lock.md; docs/process/release-process.md documents resume + credential guidance.

Test plan

  • TEST_FILTER='DeploymentScriptContractTests|SigningPipelineTests|ReleaseSigningContractTests' ./Scripts/run-tests-safe.sh — 33 passed, including 4 new SigningPipelineTests (API-key args used end-to-end, incomplete key config rejected before any notarytool call, kp_notary_await_submission resumes a recorded id without re-uploading, default-auth opt-out).
  • ./Scripts/verify-release-signing-contract.sh --source — passes with the new invariants.
  • Stubbed end-to-end smoke of notarize-resume.sh: accepted fast-path, wait-on-recorded-id, resubmit-when-nothing-reached-Apple, ambiguous-candidates abort with KP_NOTARY_SUBMISSION_ID override, and zip-SHA-mismatch abort.
  • Full gate TIMEOUT_SECONDS=1800 ./Scripts/test-full.sh: 5069 passed; 6 failures = the contract test this PR updates (now green, see above) plus 5 pre-existing KeyPathSnapshotTests reference-size mismatches (EasyView home-row timing ×3, launcher drawer, repair settings tab) in UI untouched by this script/docs-only diff — environment drift on the macOS 27 beta box.

Review gate

./Scripts/review-gate.sh: remote review gate selected — do not merge until the GitHub claude-review check passes.

🤖 Generated with Claude Code

The 2026-07-30 release candidate failed at notarytool submit with "No
Keychain password item found for profile: KeyPath-Profile" nine minutes
after release-doctor validated that same profile: notarytool stores
profiles in the data-protection keychain, which becomes unreadable when
the login session locks mid-build. Recovery then required hand-stapling
the extracted zip because a dev deploy had re-signed the installed app,
detaching it from its eventual ticket.

- Prefer the file-based App Store Connect API key for notarytool when
  present (no lock-state dependency); keychain-profile auth remains the
  fallback and can be forced with KP_NOTARY_AUTH=keychain-profile.
  release-doctor now validates whichever auth path the build will use.
- Add Scripts/release-candidate.sh --resume-notarization
  (Scripts/notarize-resume.sh): finish the recorded submission or
  resubmit the checksum-verified archive, staple dist/KeyPath.app, and
  deploy it verbatim - no rebuild, no re-sign.
- build-and-sign.sh refuses to deploy a bundle without a stapled ticket
  whenever notarization was requested, and both deploy paths share
  Scripts/lib/deploy-app.sh (ditto copy, never re-signs).
- Extract kp_notary_await_submission so resume can wait on a recorded
  submission id; enforce the new invariants in
  verify-release-signing-contract.sh; extend SigningPipelineTests.

Incident write-up: docs/bugs/2026-07-30-rc-notarization-keychain-lock.md

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@claude

claude Bot commented Jul 31, 2026

Copy link
Copy Markdown

Reviewed the diff. Overall solid — the notarization-resume path, the shared kp_deploy_bundle_to_applications extraction, the stapled-ticket guard before deploy, and the new tests (SigningPipelineTests, DeploymentScriptContractTests) all look sound and directly address the documented 2026-07-30 RC incident.

One finding worth a look:

Brittle status check in release-candidate.sh's new failure path (Scripts/release-candidate.sh, the build_status -ne 0 block):

if [[ -f "$state_file" ]] && ! grep -q '"status": "accepted"' "$state_file"; then

This does a raw string match against the notary recovery-state JSON instead of using the kp_notary_json_field helper that Scripts/notarize-resume.sh and Scripts/lib/signing.sh already use to read this exact file. Since release-candidate.sh doesn't source lib/signing.sh in this diff, it can't call that helper directly — but that also means this check is coupled to the literal whitespace/formatting of whatever writes dist/KeyPath.notary-submission.json (kp_write_notary_submit_failure_state et al., not shown in this diff). If that writer's formatting ever changes (compact JSON, different key ordering, etc.), this grep silently stops matching and the script prints "Notarization did not complete; the app was NOT deployed" even when notarization actually succeeded and a later step (e.g. staple/postcondition check) failed — misleading exactly the recovery-guidance path this PR is trying to make more reliable. Consider parsing via the shared JSON-field helper (sourcing lib/signing.sh, or extracting a tiny shared status-reader) instead of a literal grep.

Minor/non-blocking: Scripts/lib/signing.sh hardcodes a specific default App Store Connect Key ID and Issuer UUID (KP_NOTARY_DEFAULT_KEY_ID, KP_NOTARY_DEFAULT_ISSUER) plus a fixed ~/.appstoreconnect/... path. These aren't secrets and the code degrades gracefully (falls back to keychain-profile auth) when the key file is absent on another machine, so this is fine as-is — just flagging for awareness since it bakes one contributor's Apple Developer identifiers into a shared script.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: be8fecbc39

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment on lines +80 to +81
echo "🔁 No submission reached Apple; resubmitting the recorded archive..."
kp_notarize_zip "$ZIP_PATH" "$NOTARY_PROFILE"

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Refuse resubmission after an inconclusive history lookup

When submit fails without returning an ID, an empty candidateSubmissions list does not prove that nothing reached Apple: kp_notarize_zip also persists an empty list when the best-effort history request fails, returns malformed data, or has not exposed the new upload yet. In those cases this branch automatically uploads the same archive again, creating the duplicate submission the recovery safeguards are intended to prevent; require positive evidence that the history lookup succeeded and is authoritative, or require an explicit operator opt-in before resubmitting.

Useful? React with 👍 / 👎.

Comment thread Scripts/notarize-resume.sh Outdated
APP_BUNDLE="${DIST_DIR}/${APP_NAME}.app"
ZIP_PATH="${DIST_DIR}/${APP_NAME}.zip"
STATE_FILE=${KP_NOTARY_STATE_FILE:-"${ZIP_PATH%.zip}.notary-submission.json"}
NOTARY_PROFILE="${NOTARY_PROFILE:-KeyPath-Profile}"

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Restore the notary profile recorded in recovery state

If the failed release used a nondefault NOTARY_PROFILE with keychain-profile authentication, a later resume invocation without that environment override ignores the profile already stored in the state file and attempts KeyPath-Profile instead. This makes the advertised resume command fail authentication unless the operator remembers and repeats the original override; read the recorded profile as the fallback before using the hard-coded default.

Useful? React with 👍 / 👎.

Comment thread Scripts/lib/signing.sh
Comment on lines +30 to +34
if [ -n "${KP_NOTARY_KEY_PATH:-}" ]; then
return 0
fi
if [ "${KP_NOTARY_AUTH:-auto}" = "keychain-profile" ]; then
return 0

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Honor the explicit keychain-profile authentication override

When KP_NOTARY_AUTH=keychain-profile is set alongside an existing KP_NOTARY_KEY_PATH, this early return leaves the key path populated and kp_notary_resolve_auth consequently selects API-key authentication. Thus the documented force option cannot recover from a stale, inaccessible, or unwanted API key unless the caller also knows to unset the key variables; check the force setting before accepting KP_NOTARY_KEY_PATH, or make the resolver honor it directly.

Useful? React with 👍 / 👎.

- KP_NOTARY_AUTH=keychain-profile now beats a populated KP_NOTARY_KEY_PATH
  in kp_notary_resolve_auth, so a stale API key can be bypassed without
  unsetting variables (new SigningPipelineTests coverage).
- notarize-resume falls back to the profile recorded in the recovery state
  when NOTARY_PROFILE is not overridden.
- notarize-resume no longer treats an empty recorded candidate list as proof
  nothing reached Apple: it requires a successful notarytool history lookup
  now and refuses to resubmit when history is unavailable or shows fresh
  submissions of the archive (KP_NOTARY_SUBMISSION_ID overrides).
- release-candidate.sh reads the recovery status via kp_notary_json_field
  instead of grepping the JSON formatting.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@claude

claude Bot commented Jul 31, 2026

Copy link
Copy Markdown

Reviewed the diff (notarization auth refactor + --resume-notarization recovery path). The safety logic (SHA-pinning the recovery state to dist/KeyPath.zip, refusing to resubmit when Apple history shows a matching recent submission, never re-signing after staple) is sound, and the auth-resolution helpers in signing.sh are well covered by SigningPipelineTests. Two lower-severity notes:

  1. Scripts/release-candidate.sh stale-state false positive: the new failure branch checks dist/KeyPath.notary-submission.json for -f existence and non-accepted status to decide whether to print "Notarization did not complete... resume without rebuilding." If a previous run's state file is still on disk (e.g. dist/ isn't wiped before this attempt) and the current run fails for an unrelated reason (compile error, codesign failure) before ever reaching notarization, this message fires anyway and points the user at --resume-notarization, which would then fail with a SHA mismatch — confusing but not destructive. Consider gating this on evidence that this run reached the notarize step (e.g. compare the state file's timestamp/mtime against build start, or have build-and-sign.sh remove/rename the old state file at the start of each attempt).

  2. Test coverage gap on Scripts/notarize-resume.sh: this new 141-line script contains the actual recovery decision logic (SHA-pin guard, candidate-submission refusal, live-history safety check before resubmitting), but the only coverage added is SigningPipelineTests (exercises signing.sh helpers directly) and a DeploymentScriptContractTests/verify-release-signing-contract.sh grep check that the script calls kp_deploy_bundle_to_applications. None of the branches specific to notarize-resume.sh itself (mismatched zip SHA, non-empty candidateSubmissions, fresh history hit) are exercised by a test. Given this script exists specifically to prevent duplicate Apple submissions, it'd be worth a small script-level test (similar to the SigningPipelineTests pattern) covering at least the SHA-mismatch and candidate-refusal paths.

No security or correctness issues found in the auth-resolution/deploy-lib refactor itself.

@malpern
malpern merged commit fdf8eaf into master Jul 31, 2026
6 checks passed
@malpern
malpern deleted the claude/youthful-bassi-e23580 branch July 31, 2026 01:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant