From 62717614d9204ce0b26c32f2dfa81ace07ba29c9 Mon Sep 17 00:00:00 2001 From: DonislawDev Date: Sun, 6 Sep 2026 18:32:29 +0200 Subject: [PATCH] fix: the documents say what this tool does about signing and about locking Two findings of the outside security review, both about a document being read by somebody deciding whether to trust something. SECURITY.md said the released binaries are not signed and that signing is not set up. Every part of that had stopped being true: the release workflow writes notes saying the Windows binaries are signed and the macOS ones are signed and notarised by Apple, and internal/legal/codesign.go pins the certificate by the SHA-256 of its DER bytes. There is a guard family around the wording of those notes and nothing was watching this document. The effect is not cosmetic. SECURITY.md is what a cautious person reads before deciding whether to trust a download, and telling them the signature they can see is not ours is the reasoning that leads somebody to ignore a mismatch. It now names which platforms are signed, and the guard reads that from the release notes rather than from a list repeated here. Re-measured while there: repository secrets, zero, on 2026-09-06. The encryption setting described which readers open which scheme and never said that a locked archive from this tool offers no confidentiality. That is not a defect in the design and the review said so - the salt comes from the run seed rather than from crypto/rand because a random one would give two runs of one recipe different bytes, which untouchable rule 3 forbids, and the password is in the manifest because a fixture nobody can open checks nothing. What was missing is the sentence where somebody meets it: aes-256 means something else everywhere else it is written. The declaration carries it, so both surfaces show it, and README says it too. Two guards, two mutations, both caught. Co-Authored-By: Claude Opus 5 --- CHANGELOG.md | 11 +++ README.md | 6 ++ SECURITY.md | 19 ++-- internal/format/archive/archive.go | 5 +- internal/guard/documentedclaims_test.go | 126 ++++++++++++++++++++++++ 5 files changed, 159 insertions(+), 8 deletions(-) create mode 100644 internal/guard/documentedclaims_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index c0d270f..6ee1951 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -139,6 +139,17 @@ because it turns other people's test suites red. ### Changed +- **The `encryption` setting now says that a locked archive is a fixture rather + than protection.** Nothing about the files changes - the sentence `tfg formats + zip` prints, and the one the window shows beside the field, gained the two + facts that were missing: the key is worked out from the run seed, so the same + recipe gives the same archive on every machine, and the password is written + into the manifest beside the file. + + Both are deliberate and both are what makes these archives useful for testing + a reader. `aes-256` means something else everywhere else it is written, which + is why it is now said out loud. `README.md` says it too. + - **Files are written over several threads, so a run of many files is several times faster.** They used to be written one after another. diff --git a/README.md b/README.md index 211ea70..8ba4a86 100644 --- a/README.md +++ b/README.md @@ -486,6 +486,12 @@ tfg generate --format jpg --size 500kb --set width=1920 --set height=1080 --set wrong size.** The message names the format, the smallest it can be, the reason for that floor and what to do instead. `tfg formats` lists every floor. +**A locked archive is a fixture, not protection.** `password` and `encryption` +produce an archive that real readers open with a password, which is what makes it +useful for testing one. It offers no confidentiality: the key is worked out from +the run seed so that the same recipe gives the same archive on every machine, and +the password is written into the manifest beside the file so a test can open it. + ## 🧾 The manifest Written next to the files at the end of every run, including a run that was diff --git a/SECURITY.md b/SECURITY.md index 4e99f1f..15bf24c 100644 --- a/SECURITY.md +++ b/SECURITY.md @@ -50,11 +50,15 @@ the files a manifest lists and nothing else. **It writes only inside the output directory.** A file name is a name rather than a path, and the manifest name too. Paths that would leave the directory, including -through a symbolic link, are refused. +through a symbolic link, are refused. Every file is written under a temporary name +first, and those names are claimed rather than created - so a link left at one of +them by somebody else is a refusal rather than a way out of the directory. -**The released binaries are not signed.** Signing is not set up, the release notes -say so, and your operating system will warn you. Verify a download against -`verify-SHA256SUMS.txt` from the same release. +**The Windows and macOS downloads are signed**, and the macOS ones are notarised by +Apple, so they start without a warning about an unknown developer. **The Linux ones +are not signed**, because desktop Linux has no equivalent to sign them with. Verify +any download against `verify-SHA256SUMS.txt` from the same release, and the build +provenance attestation published with it. ### In scope @@ -74,9 +78,10 @@ say so, and your operating system will warn you. Verify a download against ## Secrets and permissions in this repository -**There are no repository secrets.** Measured on 2026-08-27: zero. Every workflow -runs on the per-job token GitHub issues for the run, and nothing else is stored -here. +**There are no repository secrets.** Measured again on 2026-09-06: zero. Every +workflow runs on the per-job token GitHub issues for the run, and nothing else is +stored here. Signing happens on the owner's machine, against a key on a card, so +the release workflow never holds one either. **Access is scoped per workflow.** The main suite, the dependency review and the release workflow all declare `contents: read` at the top, and the single job that diff --git a/internal/format/archive/archive.go b/internal/format/archive/archive.go index f93ad60..88a1657 100644 --- a/internal/format/archive/archive.go +++ b/internal/format/archive/archive.go @@ -188,7 +188,10 @@ var axes = map[string]format.Property{ Default: NoEncryption, Detail: "How the archive is locked. AES is the WinZip scheme, and some readers cannot open it " + "at all. ZipCrypto is the old one every reader opens and nothing modern trusts, and some " + - "of them hand back the encrypted bytes without saying so.", + "of them hand back the encrypted bytes without saying so. " + + "A locked archive from this tool is a fixture rather than protection - the key is worked out " + + "from the run seed, so the same recipe gives the same archive on any machine, and the password " + + "is written into the manifest beside it.", }, } diff --git a/internal/guard/documentedclaims_test.go b/internal/guard/documentedclaims_test.go new file mode 100644 index 0000000..b0da7f3 --- /dev/null +++ b/internal/guard/documentedclaims_test.go @@ -0,0 +1,126 @@ +package guard + +import ( + "os" + "path/filepath" + "strings" + "testing" + + "github.com/donislawdev/TestingFilesGenerator/internal/format" + _ "github.com/donislawdev/TestingFilesGenerator/internal/format/all" + "github.com/donislawdev/TestingFilesGenerator/internal/format/archive" +) + +// The security policy says what the release actually does about signing. +// +// It said the opposite until 2026-09-06: "The released binaries are not signed. +// Signing is not set up, the release notes say so, and your operating system +// will warn you." Every part of that had stopped being true. The release +// workflow writes notes saying the Windows binaries are signed and the macOS +// ones are signed and notarised by Apple, internal/legal/codesign.go pins the +// certificate by the SHA-256 of its DER bytes, and there is a guard family +// around the wording of those notes. +// +// The effect of a stale sentence there is not cosmetic, and that is why this +// exists. SECURITY.md is the one document a cautious person reads before +// deciding whether to trust a download. Telling them the signature they can see +// is not one the project makes is the exact reasoning that leads somebody to +// ignore a mismatch, or to skip a check they have been told there is nothing to +// do. +// +// Tied to the release notes rather than to a list written here, so the two +// cannot drift apart again. Found by an outside review on 2026-09-05. +func TestTheSecurityPolicySaysWhatTheReleaseDoesAboutSigning(t *testing.T) { + policy := readRepoFile(t, "SECURITY.md") + notes := workflowText(t, "release.yml") + + if strings.Contains(policy, "binaries are not signed") || + strings.Contains(policy, "Signing is not set up") { + t.Error("SECURITY.md still says the released binaries are not signed.\n" + + "They are: the Windows ones are signed and the macOS ones are signed and notarised. " + + "A person who reads that sentence has been told the signature they can see is not ours.") + } + + // What the release notes claim, and therefore what the policy has to agree + // with. Asking the workflow rather than repeating its words keeps one + // source of truth for the day a platform is added or dropped. + claims := map[string]string{ + "Windows binaries are signed": "Windows", + "macOS binaries are signed and notarised": "macOS", + "Linux binaries are not signed": "Linux", + } + checked := 0 + for inNotes, platform := range claims { + if !strings.Contains(notes, inNotes) { + continue + } + checked++ + if !strings.Contains(policy, platform) { + t.Errorf("the release notes say %q and SECURITY.md never mentions %s.\n"+ + "That document is where somebody decides whether to trust a download, so it has "+ + "to name which platforms carry a signature and which do not.", inNotes, platform) + } + } + if checked == 0 { + t.Fatal("no sentence about signing was found in release.yml, so this guard compared " + + "SECURITY.md against nothing. If the wording of the notes changed, change the claims " + + "above with it rather than leaving a check that asks nothing.") + } +} + +// The setting that locks an archive says what locking it does not give. +// +// This is not a defect in the design and the review that raised it said so. The +// salt comes from the run seed rather than from crypto/rand on purpose: a +// random one would give two runs of one recipe different bytes, which +// untouchable rule 3 forbids. The password is in the manifest on purpose too. +// Both are right for a tool that produces fixtures. +// +// What was missing is the sentence where somebody meets it. "aes-256" carries a +// meaning everywhere else it is written, and a person choosing it here gets an +// archive that offers no confidentiality at all: same recipe, same seed, same +// password gives the same key on any machine, and the password is written down +// beside the file. +// +// Asked of the declaration rather than of the README, because the declaration +// is what both surfaces show - tfg formats prints it and the window puts it +// beside the field. +func TestTheLockingSettingSaysItIsAFixtureRatherThanProtection(t *testing.T) { + zip, err := format.Get("zip") + if err != nil { + t.Fatalf("zip is not registered, so this guard has nothing to read: %v", err) + } + + var detail string + for _, p := range zip.Properties { + if p.Name == archive.Encryption { + detail = p.Detail + } + } + if detail == "" { + t.Fatalf("zip declares no %q setting with a description, so this guard asks nothing", + archive.Encryption) + } + + // The two concrete reasons it is not protection. A rewrite that drops both + // has dropped the meaning, whatever else it says. + for _, word := range []string{"seed", "manifest"} { + if !strings.Contains(detail, word) { + t.Errorf("the description of %q never mentions the %s:\n %s\n"+ + "The two reasons a locked archive from this tool is not protection are that the "+ + "key comes from the run seed and that the password is written into the manifest. "+ + "Somebody choosing aes-256 here has to be told that, because that name means "+ + "something else everywhere they have met it before.", archive.Encryption, word, detail) + } + } +} + +// readRepoFile reads a file from the root of the repository. +func readRepoFile(t *testing.T, name string) string { + t.Helper() + raw, err := os.ReadFile(filepath.Join(repoRoot(t), name)) + if err != nil { + t.Fatalf("reading %s: %v", name, err) + } + return string(raw) +}