diff --git a/.github/scripts/sign_macos.sh b/.github/scripts/sign_macos.sh index 58c92e7..0bdd9d1 100755 --- a/.github/scripts/sign_macos.sh +++ b/.github/scripts/sign_macos.sh @@ -120,10 +120,26 @@ A renewal is a DIFFERENT certificate and internal/legal has to move with it." } # The certificate that ACTUALLY signed a bundle, read back out of it. +# 🔴 Nothing here changes directory, and that is the whole of what this function +# learned on 2026-08-28, on the first real release. +# +# It used to cd into the temporary directory and pass the bundle path after +# that, because --extract-certificates looked like a file NAME. The path it was +# given is relative - sign_release.py passes a directory under the home +# directory - so after the cd it named nothing. codesign answered "No such file +# or directory", no certificate came out, and the script stopped the release +# saying the bundle "was signed by a DIFFERENT certificate, got none" about a +# bundle that was correctly signed, by the pinned certificate, with a timestamp. +# A refusal that is right about there being a problem and wrong about what it is +# costs more than a silent one, because it sends somebody to look at the card. +# +# The prefix takes a path. Measured rather than assumed: with the prefix written +# as "$tmp/cert" and the bundle path left exactly as it arrives, cert0 appears +# and hashes to the pinned digest. certificate_of() { local bundle="$1" tmp tmp="$(mktemp -d)" - ( cd "$tmp" && codesign -d --extract-certificates=cert "$bundle" >/dev/null 2>&1 ) + codesign -d --extract-certificates="$tmp/cert" "$bundle" >/dev/null 2>&1 if [ ! -f "$tmp/cert0" ]; then rm -rf "$tmp" echo "none" @@ -172,8 +188,15 @@ Nothing has been handed back." # Apple takes a zip, a pkg or a dmg, and will not take the tar.gz we publish. # So the zip exists only to carry the bundle there. What gets stapled and # republished is the bundle itself. + # The same trap as certificate_of, one step further on, and it was measured + # the same day rather than met later: this cd'd into the unpacked directory + # and then wrote to a path that was relative to where the script STARTED, so + # the zip was never created and the next line would have stopped the release + # with "could not zip". Without the cd, ditto is given both paths as they + # arrive and --keepParent still puts tfg-gui.app at the top of the archive, + # which is the shape notarisation wants - read back out of the zip. local zip="${work}/notarise.zip" - ( cd "$work" && ditto -c -k --keepParent "$(basename "$app")" "$zip" ) || + ditto -c -k --keepParent "$app" "$zip" || die "could not zip $(basename "$app") for notarisation" local out diff --git a/.github/workflows/attest-release.yml b/.github/workflows/attest-release.yml index 675f16c..2bf7710 100644 --- a/.github/workflows/attest-release.yml +++ b/.github/workflows/attest-release.yml @@ -56,8 +56,21 @@ jobs: set -euo pipefail mkdir -p signed cd signed - gh release download "$TAG" --pattern '*.zip' --pattern '*.tar.gz' \ - --pattern '*.spdx.json' --pattern 'verify-SHA256SUMS.txt' + # The checksums first, because they are the list of what to fetch. + # + # 🔴 This used to be a hand written list of patterns beside them, and + # the first real release is what said why that cannot work: the + # patterns fetched the archives, the SBOM and the checksums, and the + # checksums also describe the provenance bundle - which nothing in + # that list matched. sha256sum then reported "No such file or + # directory" for a file that was on the release page all along, and + # the run stopped after the signing was already done. + # + # Two lists of what a release holds is one list too many. The names + # come out of the checksums now, so the thing being verified is also + # the thing that says what to verify, and adding an asset cannot make + # them disagree again. + gh release download "$TAG" --pattern 'verify-SHA256SUMS.txt' actual="$(sha256sum verify-SHA256SUMS.txt | cut -d' ' -f1)" echo "the release carries: $actual" if [ "$actual" != "$CLAIMED" ]; then @@ -65,6 +78,20 @@ jobs: echo "::error::was dispatched for $CLAIMED - something changed in between" exit 1 fi + # Collected before anything is fetched, because gh reads stdin and a + # download inside the read loop eats the rest of the list. + names=() + while read -r _ name; do + [ -n "${name:-}" ] && names+=("$name") + done < verify-SHA256SUMS.txt + if [ "${#names[@]}" -eq 0 ]; then + echo "::error::verify-SHA256SUMS.txt names no files, so there is nothing to attest" + exit 1 + fi + echo "the checksums describe ${#names[@]} file(s)" + for name in "${names[@]}"; do + gh release download "$TAG" --pattern "$name" + done # And the checksums have to describe the files that came with them, # because everything below is a statement about that list. sha256sum -c verify-SHA256SUMS.txt @@ -96,7 +123,7 @@ jobs: # missing file rather than a wrong flag. run: | set -euo pipefail - promised="https://spdx.dev/Document" + promised="https://spdx.dev/Document/v2.3" actual="$(python3 - "$BUNDLE" <<'PY' import base64, json, sys bundle = json.load(open(sys.argv[1], encoding="utf-8")) diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index 0d1ddb4..f8b7352 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -465,7 +465,7 @@ jobs: echo echo "\`\`\`" echo "gh attestation verify -R ${GITHUB_REPOSITORY} \\" - echo " --predicate-type https://spdx.dev/Document" + echo " --predicate-type https://spdx.dev/Document/v2.3" echo "\`\`\`" echo echo "The predicate type has to be given. \`gh\` asks for build provenance unless told" diff --git a/.github/workflows/verify-release.yml b/.github/workflows/verify-release.yml index 19bbe26..d52732e 100644 --- a/.github/workflows/verify-release.yml +++ b/.github/workflows/verify-release.yml @@ -222,7 +222,7 @@ jobs: gh attestation verify "$linux" -R "$GITHUB_REPOSITORY" echo "what is inside, on a signed file: $windows" gh attestation verify "$windows" -R "$GITHUB_REPOSITORY" \ - --predicate-type https://spdx.dev/Document + --predicate-type https://spdx.dev/Document/v2.3 - name: the same two commands with no network to the API shell: bash @@ -237,7 +237,7 @@ jobs: --bundle ./*.provenance.sigstore.json gh attestation verify "$windows" -R "$GITHUB_REPOSITORY" \ --bundle ./*.sbom.sigstore.json \ - --predicate-type https://spdx.dev/Document + --predicate-type https://spdx.dev/Document/v2.3 - name: every Windows program is signed, stamped, and by OUR certificate shell: pwsh @@ -289,7 +289,7 @@ jobs: run: | set -euo pipefail gh release view "$TAG" --repo "$GITHUB_REPOSITORY" --json body --jq .body > notes.md - for promised in "gh attestation verify" "--predicate-type https://spdx.dev/Document" "verify-SHA256SUMS.txt"; do + for promised in "gh attestation verify" "--predicate-type https://spdx.dev/Document/v2.3" "verify-SHA256SUMS.txt"; do grep -qF -- "$promised" notes.md || { echo "the release notes never mention: $promised" echo "This job just ran it, so the page and the check disagree about what a person should do." diff --git a/internal/guard/attestation_test.go b/internal/guard/attestation_test.go index 6bff93f..e427ba0 100644 --- a/internal/guard/attestation_test.go +++ b/internal/guard/attestation_test.go @@ -3,6 +3,7 @@ package guard import ( "os" "path/filepath" + "regexp" "strings" "testing" @@ -138,6 +139,96 @@ func TestTheReleaseMakesItsDocumentAndHandsItOver(t *testing.T) { // And the notes tell somebody how to use any of it. A statement nobody knows // about is a statement nobody checks. +// What the attesting half downloads is named BY the checksums, not beside them. +// +// Found by the first real release, on 2026-08-28, after the signing was already +// done. That job fetched the release with a hand written list of patterns - +// archives, the SBOM, the checksums - and then ran sha256sum -c over the +// checksums. The checksums also describe the provenance bundle, which no +// pattern matched, so sha256sum said "No such file or directory" about a file +// that was sitting on the release page the whole time, and the run stopped. +// +// Two lists of what a release holds is one list too many, and the failure mode +// is not a missing file: it is a check that reports a problem with the release +// when the problem is in the checker. That is expensive here, because it lands +// at the one moment when somebody is holding a card and half a release. +// +// So the names come out of the file being verified. Adding an asset cannot make +// the two disagree again, because there is only one list. +func TestTheAttestingHalfFetchesWhatTheChecksumsName(t *testing.T) { + attest := workflowText(t, "attest-release.yml") + + if strings.Contains(attest, "--pattern '*.zip'") { + t.Error("attest-release.yml fetches the release with its own list of patterns. " + + "That list is a second description of what a release holds, and when it " + + "disagrees with the checksums the run stops with a message about the release " + + "rather than about itself.\n" + + "What to do: read the names out of verify-SHA256SUMS.txt and fetch those.") + } + if !strings.Contains(attest, "done < verify-SHA256SUMS.txt") { + t.Error("attest-release.yml does not take the list of files to fetch from " + + "verify-SHA256SUMS.txt, so nothing keeps what it downloads and what it " + + "verifies in step") + } +} + +// Every workflow that names the SBOM predicate type names the SAME one. +// +// This fact is written five times across three files, and it cannot be written +// once: the notes tell a person what to type, the attesting half checks that +// promise against the statement it just made, and the verifying half runs the +// command for real. They are three different jobs in three different files and +// a workflow cannot read a constant out of another one. +// +// What it cost when they disagreed, on the first real release, on +// 2026-08-28: the notes promised https://spdx.dev/Document, the statement +// carried https://spdx.dev/Document/v2.3, and the attesting half stopped the +// release. It was RIGHT to - the promise was written from the action's +// documentation rather than from an attestation, and the provenance plan said +// out loud that it was unmeasured - but the value that was wrong was written in +// five places and only one of them was checked. +// +// The one that matters most is not the check. It is the pair of commands a +// person types out of the release notes: with the wrong URI, gh answers "no +// attestation found", which reads exactly like a release nobody attested. +func TestEveryWorkflowNamesTheSamePredicateType(t *testing.T) { + dir := filepath.Join(repoRoot(t), ".github", "workflows") + entries, err := os.ReadDir(dir) + if err != nil { + t.Skipf("no workflows here: %v", err) + } + + uri := regexp.MustCompile(`https://spdx\.dev/[A-Za-z0-9./-]*`) + found := map[string][]string{} + for _, entry := range entries { + if entry.IsDir() || !strings.HasSuffix(entry.Name(), ".yml") { + continue + } + body, err := os.ReadFile(filepath.Join(dir, entry.Name())) + if err != nil { + t.Fatalf("reading %s: %v", entry.Name(), err) + } + for _, match := range uri.FindAllString(string(body), -1) { + found[match] = append(found[match], entry.Name()) + } + } + + if len(found) == 0 { + t.Fatal("no workflow names the SBOM predicate type, so this guard checked nothing") + } + if len(found) > 1 { + for value, files := range found { + t.Errorf("%q is named in %v", value, files) + } + t.Error("the workflows disagree about the SBOM predicate type. One of them tells a " + + "person what to type, one checks that promise against the statement, and one " + + "runs the command for real - so a disagreement here is a release page whose " + + "own instructions answer \"no attestation found\".\n" + + "What to do: the value is whatever the statement actually carries. Read it out " + + "of a bundle rather than out of documentation.") + } +} + func TestTheReleaseNotesSayHowToCheckWhatWasDownloaded(t *testing.T) { job := releasePublishJob(t) var notes string diff --git a/internal/guard/macossigning_test.go b/internal/guard/macossigning_test.go index ff4ea6c..ead7b7a 100644 --- a/internal/guard/macossigning_test.go +++ b/internal/guard/macossigning_test.go @@ -204,6 +204,45 @@ func TestTheIconMacOSReadsCarriesEverySizeItIsAskedFor(t *testing.T) { t.Logf("%d entries, every size macOS asks for, %d bytes", len(found), len(body)) } +// The macOS signing script never changes directory. +// +// Found by the first real release, on 2026-08-28, and it stopped that release +// twice in two consecutive steps. Every path this script works with is derived +// from the directory it is handed, and sign_release.py hands it one relative to +// the home directory. Two commands ran inside a subshell that had cd'd +// somewhere else first, so the paths they were given stopped meaning anything: +// +// - certificate_of cd'd into a temporary directory and then asked codesign +// about the bundle. codesign answered "No such file or directory", no +// certificate came out, and the script refused the release saying the +// bundle was "signed by a DIFFERENT certificate, got none" - about a bundle +// that was correctly signed by the pinned certificate, with a timestamp, +// chaining to the Apple Root CA. Measured after the fact: the same command +// with the path resolved gives exactly the pinned digest; +// - the zip for notarisation cd'd into the unpacked bundle and wrote to a +// path relative to where the script started, so the file was never created. +// That one had not been reached yet and would have stopped the next step. +// +// Asked as "no cd at all" rather than "the paths are absolute", because that is +// the property that can be read off the file. If a directory change is ever +// genuinely needed here, make every path absolute first and this guard is the +// conversation about it. +// +// Why nothing caught this before: the rehearsal of 2026-08-28 ran the script +// by hand with an absolute directory, and sign_release.py passes a relative one. +// The command was proven, the call was not. +func TestTheMacSigningScriptDoesNotDependOnWhereItIsRunFrom(t *testing.T) { + script := macSigningScript(t) + + if strings.Contains(script, "cd \"") { + t.Error("sign_macos.sh changes directory somewhere. Every path in it comes from the " + + "directory it is handed, and sign_release.py hands it a relative one - so a cd " + + "silently changes what those paths mean.\n" + + "What happened when this was last true: codesign reported no certificate for a " + + "correctly signed bundle, and the release stopped saying the wrong thing about why.") + } +} + // The pin is a digest with a date, and the script derives its selector from it. // // Same shape as the Windows pin and for the same reason: codesign selects by