From d34d0e4004e6e448b7407e1146e789f11106f1e6 Mon Sep 17 00:00:00 2001 From: DonislawDev Date: Tue, 1 Sep 2026 10:15:09 +0200 Subject: [PATCH 1/4] format: a zip can be locked with a password, and tar says plainly that it cannot tfg generate --format zip --size 30kb --set entries=3 \ --set password=Secret123 --set encryption=aes-256 writes an archive of exactly 30720 B that 7-Zip opens with that password and refuses without it. AES-128, AES-192 and AES-256 are the methods, and the manifest carries the password in plain text - a locked fixture nobody can open is worth nothing, so writing it down is the point rather than a leak. It is possible at all because of one measurement, in docs/MVP-FORMATS.md section 2.16. A stream cipher does not change the length of what it encrypts, so a lock adds a FIXED count per entry - 20, 24 or 28 bytes of data by key length, plus an eleven byte field in each of the two headers. The size of the archive therefore stays an exact function of its input and the planning phase can still state it without building it, which is the guard the whole container design rests on. Compression has no such property, and that is why this setting exists and that one still does not. Two more things were read out of the bytes 7-Zip writes rather than out of a specification recalled, and each decided half the work: An AE-2 entry carries a CRC of zero, because the authentication code is what proves the contents. Nothing about the plaintext has to be known before the first byte, so an entry is encrypted as it streams and an archive of any size costs the memory of an archive of one byte. ZipCrypto carries the real plaintext CRC, so it cannot be written the same way. It is not in this commit for that reason rather than because it was forgotten - the note in section 2.16 says what it would take. The salt is derived from the run seed and never drawn. A salt from crypto/rand gives a perfectly good archive that differs on every run, which is untouchable rule 3 broken in the one place where every other check stays green. Two settings say this together, and the two that disagree are refused. That is forced by the window rather than chosen: a menu cannot be empty, so encryption arrives as "none" from every window run whether or not anybody looked at it, while a box somebody types in arrives empty and is left out. "Password given, encryption none" therefore cannot be told apart from "password given, encryption not considered", and both refusals name both halves so nobody is left staring at a filled in password box. tar and gzip get their own refusal rather than the generic one. "targz does not have a property called password" is true and sends somebody looking for the build that has it, and there is none: neither format has any encryption in it. Descriptors can now declare what they deliberately cannot take, with the reason, and the refusal lands on FORMAT rather than USAGE - a deliberate limitation is not a spelling mistake. The alternative in the world is worse than either, and it is why this refuses instead of ignoring. Measured the same day: 7-Zip accepts -p on a tar, exits 0, prints nothing and writes a PLAINTEXT archive. Somebody asking for a locked fixture gets an open one and never finds out. Seven guards, one of which is the whole feature: 7-Zip opens the archive with the right password and REFUSES the wrong one. A reader that accepted anything would have passed the first half while proving nothing. format.go lost its refusals to refusals.go on the way, because the crowding gate went red at three files in the band against a cap of two. The cut is by subject - what a format declares in one file, what it says when a setting is wrong in the other. Co-Authored-By: Claude Opus 5 --- internal/cli/errors.go | 8 + internal/format/archive/archive.go | 28 +++ internal/format/archive/lock.go | 286 +++++++++++++++++++++++ internal/format/format.go | 125 +--------- internal/format/refusals.go | 215 +++++++++++++++++ internal/format/targz/targz.go | 25 +- internal/format/zip/zip.go | 140 +++++++++-- internal/guard/archivelock_test.go | 358 +++++++++++++++++++++++++++++ internal/guard/parity_test.go | 2 + internal/oracle/oracle.go | 8 + 10 files changed, 1060 insertions(+), 135 deletions(-) create mode 100644 internal/format/archive/lock.go create mode 100644 internal/format/refusals.go create mode 100644 internal/guard/archivelock_test.go diff --git a/internal/cli/errors.go b/internal/cli/errors.go index d15356f..fc8fdfd 100644 --- a/internal/cli/errors.go +++ b/internal/cli/errors.go @@ -200,6 +200,14 @@ func classifyFormat(err error) (int, bool) { if errors.As(err, &nesting) { return ExitFormat, true } + // A setting the format cannot carry is the same class again: the request + // is well formed and this file format has nowhere to put it. Deliberately + // not the code a misspelt key gets - that one is a typo in what somebody + // typed, and this one is true of the format itself. + var unsupported *format.UnsupportedSettingError + if errors.As(err, &unsupported) { + return ExitFormat, true + } // A value outside what the format declares is a request the format cannot // deliver, which is what FORMAT means - the same class as a size below the // minimum. It used to fall through to RUNTIME, so "--set width=abc" told diff --git a/internal/format/archive/archive.go b/internal/format/archive/archive.go index aebdb19..877e39e 100644 --- a/internal/format/archive/archive.go +++ b/internal/format/archive/archive.go @@ -41,6 +41,17 @@ const ( Entries = "entries" EntryFormat = "entry_format" EntrySize = "entry_size" + Password = "password" + Encryption = "encryption" +) + +// The encryption methods, spelled the way a recipe writes them. +const ( + NoEncryption = "none" + ZipCrypto = "zipcrypto" + AES128 = "aes-128" + AES192 = "aes-192" + AES256 = "aes-256" ) const ( @@ -109,6 +120,23 @@ var axes = map[string]format.Property{ Default: defaultSizeText, Detail: "How big each file inside is.", }, + Password: { + Name: Password, Kind: format.PropertyText, + Shape: "the password, in plain text", + // No default, and that is the point. A box somebody types in arrives + // empty from a window, so leaving it alone is how "no password" is + // said - see the pair rule in readLock. + Detail: "The password the archive is locked with. It is written into the manifest as you typed it, " + + "because a test that cannot open the file cannot check anything.", + }, + Encryption: { + Name: Encryption, Kind: format.PropertyChoice, + // Sorted, because a closed set has one order on every surface. + Choices: []string{AES128, AES192, AES256, NoEncryption, ZipCrypto}, + Default: NoEncryption, + Detail: "How the archive is locked. ZipCrypto is the old scheme every reader opens and nothing modern trusts. " + + "AES is the WinZip scheme, and some readers cannot open it at all.", + }, } // Names is every container setting this build declares, in a stable order. diff --git a/internal/format/archive/lock.go b/internal/format/archive/lock.go new file mode 100644 index 0000000..95ae691 --- /dev/null +++ b/internal/format/archive/lock.go @@ -0,0 +1,286 @@ +package archive + +import ( + "crypto/aes" + "crypto/cipher" + "crypto/hmac" + "crypto/pbkdf2" + "crypto/sha1" + "encoding/binary" + "fmt" + "io" + + "github.com/donislawdev/TestingFilesGenerator/internal/core" + "github.com/donislawdev/TestingFilesGenerator/internal/format" +) + +// Locking an archive, and the two settings that say so together. +// +// Everything here follows one measurement, written up in docs/MVP-FORMATS.md +// section 2.16, and the measurement is what makes the feature possible at all: +// the bytes a lock adds are a FIXED count per entry. A stream cipher does not +// change the length of what it encrypts, so only the salt, the verifier and +// the authentication code are added, and each is a constant. The size of the +// archive therefore stays an exact function of its input, and the planning +// phase can still state it without building it - which is the guard the whole +// container design rests on. +// +// Compression would not have that property, and that is the difference between +// this setting and the one nobody has built. + +const ( + // The parts of a WinZip AE entry, read out of what 7-Zip writes rather + // than out of a specification remembered. Salt length follows the key. + pwvLen = 2 + authLen = 10 + iterations = 1000 + + // aesExtraLen is the 0x9901 field: two bytes of id, two of length and + // seven of body. It is written into the local header AND the central + // directory, so an entry pays for it twice. + aesExtraLen = 11 +) + +// Lock is what an archive is locked with, once both halves agree. +// +// The zero value is an open archive, which is what nearly every run wants. +type Lock struct { + // Method is one of the encryption constants. NoEncryption or empty means + // the archive is open. + Method string + // Password is written into the manifest exactly as it was typed. That is a + // decision rather than an oversight: a fixture nobody can open is a fixture + // nobody can test with, and the whole point of the file is that the test + // knows the password. + Password string +} + +// On says whether anything is encrypted. +func (l Lock) On() bool { return l.Method != "" && l.Method != NoEncryption } + +// keyLen is the AES key in bytes, and the salt is half of it. Zero for an +// archive that is not locked with AES. +func (l Lock) keyLen() int { + switch l.Method { + case AES128: + return 16 + case AES192: + return 24 + case AES256: + return 32 + } + return 0 +} + +func (l Lock) saltLen() int { return l.keyLen() / 2 } + +// strength is what the 0x9901 field calls the key length. +func (l Lock) strength() byte { + switch l.Method { + case AES128: + return 1 + case AES192: + return 2 + case AES256: + return 3 + } + return 0 +} + +// EntryOverhead is the bytes this lock adds to one entry's DATA. +// +// Measured, and it agrees from two directions - the size of the whole file and +// the compressed size field in the local header: +// +// ZipCrypto +12 +// AES-128 +20 (8 salt, 2 verifier, 10 authentication) +// AES-192 +24 +// AES-256 +28 +// +// The 0x9901 field is not counted here. It lives in the headers, and the +// headers are written for real during the counting pass, so the writer counts +// those eleven bytes twice over on its own. +func (l Lock) EntryOverhead() int64 { + if !l.On() { + return 0 + } + if l.Method == ZipCrypto { + return 12 + } + return int64(l.saltLen() + pwvLen + authLen) +} + +// Extra is the 0x9901 field an AES entry carries, and nil for anything else. +func (l Lock) Extra() []byte { + if l.keyLen() == 0 { + return nil + } + out := make([]byte, 0, aesExtraLen) + out = binary.LittleEndian.AppendUint16(out, 0x9901) + out = binary.LittleEndian.AppendUint16(out, 7) + // AE-2, which carries no CRC of the plaintext. That is what makes an entry + // writable in one streaming pass: nothing about the contents has to be + // known before the first byte goes out. Measured on what 7-Zip writes. + out = binary.LittleEndian.AppendUint16(out, 2) + out = append(out, 'A', 'E') + out = append(out, l.strength()) + // Stored underneath, because everything in these archives is stored. + out = binary.LittleEndian.AppendUint16(out, 0) + return out +} + +// ReadLock reads the two settings that say how an archive is locked, and +// refuses the two states where they disagree. +// +// The pair rule is forced by the window rather than chosen. A menu cannot be +// empty - it opens on its declared default and sends it - so encryption arrives +// as "none" on every run from a window, whether or not anybody looked at it. A +// box somebody types in arrives empty and is left out. So "password given, +// encryption none" cannot be told apart from "password given, encryption not +// considered", and guessing between them would either lock an archive nobody +// asked to lock or hand back an open one to somebody who asked for a locked +// one. The second is what 7-Zip does with a tar, silently, and it is the reason +// this refuses instead. +// +// Both refusals name both halves, the way a log setting that can do nothing for +// the chosen shape does. +func ReadLock(id string, props map[string]string) (Lock, error) { + password := props[Password] + method := props[Encryption] + if method == "" { + method = NoEncryption + } + + locked := method != NoEncryption + switch { + case password == "" && !locked: + return Lock{Method: NoEncryption}, nil + case password != "" && !locked: + return Lock{}, &format.PropertyValueError{ + Format: id, Key: Encryption, Value: NoEncryption, + Reason: "a password was given and this says the archive is not locked, so one of the two is not what you meant", + Remedy: "Set encryption to " + AES256 + " to lock the archive, or remove the password to leave it open.", + } + case password == "" && locked: + return Lock{}, &format.PropertyValueError{ + Format: id, Key: Password, Value: "", + Reason: "the archive is set to be locked with " + method + " and there is no password to lock it with", + Remedy: "Give a password, or set encryption to " + NoEncryption + ".", + } + } + return Lock{Method: method, Password: password}, nil +} + +// NewEntryWriter wraps w so that everything written to it is encrypted, with +// the salt and the verifier going out first. +// +// Nothing is held. The salt is written, then each block is encrypted as it +// arrives, and Close appends the authentication code - so an entry of any size +// costs the same memory as an entry of one byte, which is the guard this +// package would otherwise break. +// +// The salt comes from the run seed and never from crypto/rand. A random salt +// would give two runs of one recipe different bytes, which is untouchable rule +// 3 - and the same recipe producing the same file is more of the product here +// than the encryption is. +func (l Lock) NewEntryWriter(w io.Writer, seed uint64, index int) (io.WriteCloser, error) { + if l.keyLen() == 0 { + return nil, fmt.Errorf("archive: %q is not an encryption this build can write", l.Method) + } + salt := l.saltFor(seed, index) + material, err := pbkdf2.Key(sha1.New, l.Password, salt, iterations, l.keyLen()*2+pwvLen) + if err != nil { + return nil, fmt.Errorf("archive: the key could not be derived: %w", err) + } + block, err := aes.NewCipher(material[:l.keyLen()]) + if err != nil { + return nil, fmt.Errorf("archive: the cipher could not be built: %w", err) + } + if _, err := w.Write(salt); err != nil { + return nil, err + } + if _, err := w.Write(material[l.keyLen()*2:]); err != nil { + return nil, err + } + return &entryWriter{ + out: w, + ctr: newCounter(block), + mac: hmac.New(sha1.New, material[l.keyLen():l.keyLen()*2]), + }, nil +} + +// saltFor is this entry's salt, derived from the run rather than drawn. +func (l Lock) saltFor(seed uint64, index int) []byte { + rng := core.NewRand(core.FileSeed(seed, index)) + salt := make([]byte, l.saltLen()) + for i := range salt { + salt[i] = byte(rng.Uint32()) + } + return salt +} + +// entryWriter encrypts on the way through and signs what it wrote. +type entryWriter struct { + out io.Writer + ctr *counter + mac hash +} + +// hash is the part of hash.Hash this uses. Named so the field above reads as +// what it is rather than as an io.Writer that happens to be a digest. +type hash interface { + io.Writer + Sum(b []byte) []byte +} + +func (e *entryWriter) Write(p []byte) (int, error) { + out := make([]byte, len(p)) + e.ctr.xor(out, p) + if _, err := e.mac.Write(out); err != nil { + return 0, err + } + if _, err := e.out.Write(out); err != nil { + return 0, err + } + return len(p), nil +} + +// Close writes the authentication code, which is what makes a wrong password +// an error rather than a file of noise. +func (e *entryWriter) Close() error { + _, err := e.out.Write(e.mac.Sum(nil)[:authLen]) + return err +} + +// counter is the counter mode WinZip AES uses, which is not the one +// crypto/cipher provides: this one counts little endian from one, and Go's CTR +// counts big endian. Two lines of difference and a file no reader will open. +type counter struct { + block cipher.Block + n uint64 + in [aes.BlockSize]byte + pad [aes.BlockSize]byte + used int +} + +func newCounter(b cipher.Block) *counter { + return &counter{block: b, used: aes.BlockSize} +} + +func (c *counter) xor(dst, src []byte) { + for i := range src { + if c.used == aes.BlockSize { + c.next() + } + dst[i] = src[i] ^ c.pad[c.used] + c.used++ + } +} + +func (c *counter) next() { + c.n++ + c.in = [aes.BlockSize]byte{} + binary.LittleEndian.PutUint64(c.in[:8], c.n) + c.block.Encrypt(c.pad[:], c.in[:]) + c.used = 0 +} diff --git a/internal/format/format.go b/internal/format/format.go index 563312d..d551548 100644 --- a/internal/format/format.go +++ b/internal/format/format.go @@ -257,6 +257,11 @@ type Descriptor struct { // its own. Empty for every format that has none. JointLimits []JointLimit + // Unsupported are settings this format deliberately cannot take, each with + // the reason. A key here is refused with that reason rather than with the + // generic "no such property", which reads as a gap in this build. + Unsupported []UnsupportedSetting + // AllocCeiling is how many objects this format may allocate producing one // file, when the flat ceiling every other one meets does not describe it. // Zero means the flat one applies, which is the case for all but one. @@ -360,122 +365,6 @@ func Containers() []string { return out } -// UnknownPropertyError is a property key no format recognises. -type UnknownPropertyError struct { - Format string - Key string - Known []string -} - -// AboutSetting is the property this refusal is about, so a form can put the -// message under the box it came from. Its sibling below has carried this since -// 2026-08-12 and this one did not, which made a mistyped key the one refusal -// about a declared setting that still landed at the foot of the form. -func (e *UnknownPropertyError) AboutSetting() string { return e.Key } - -// Why this is refused, for a report that keeps the four parts of D6 apart. -func (e *UnknownPropertyError) Why() string { - return "a format takes only the settings it declares, and one it does not know would be dropped on the way" -} - -// Instead is what to do about it, named from the declaration. -func (e *UnknownPropertyError) Instead() string { - if len(e.Known) == 0 { - return "remove the line" - } - return "use one of: " + strings.Join(e.Known, ", ") -} - -// What happened, without the list of names. Kept apart from Error so a report -// with four parts does not print the names twice - once in the sentence and -// again in what to do instead. -func (e *UnknownPropertyError) What() string { - if len(e.Known) == 0 { - return fmt.Sprintf("%s takes no properties, so %q is not one of them", e.Format, e.Key) - } - return fmt.Sprintf("%s does not have a property called %q", e.Format, e.Key) -} - -// Error is the whole thing in one sentence, unchanged to the character - it is -// what the one-target path from the command line flags prints. -func (e *UnknownPropertyError) Error() string { - if len(e.Known) == 0 { - return e.What() - } - return e.What() + ". It takes: " + strings.Join(e.Known, ", ") -} - -// PropertyValueError is a declared key given a value the declaration forbids. -// -// Separate from UnknownPropertyError because the mistake is different and so -// is the fix: one is a key that does not exist, the other a value out of -// range. Both are the caller's doing rather than the tool's, which is the -// point - this used to surface as a plain error and end with the exit code -// that means the program itself failed, so CI could not tell "you typed that -// wrong" from "this build has a bug". -type PropertyValueError struct { - Format string - Key string - Value string - Reason string - // Remedy is what to do about it, built from the declaration. It is carried - // here rather than worked out by whoever reports this, because a refusal in - // this tool has four parts - what happened, why, what is allowed, what to do - // instead (D6) - and the fourth had nowhere to come from until 2026-08-25. - // A reader that wants the whole thing in one sentence still gets it from - // Error, which leaves this out: it is the part a form puts under the box. - // - // Named Remedy rather than Instead because the accessor below has to be - // called Instead - that is the name the other refusals in this package use - // for the same part, and the reader that asks for all three asks by name. - Remedy string -} - -// What happened, why the declaration forbids it, and what to do instead. -// -// Instead is the part Error leaves out on purpose - see the field - so this is -// the only way a report gets all four parts of D6 for the refusal a person hits -// most often, by typing a number. -func (e *PropertyValueError) What() string { - return fmt.Sprintf("%s: %s cannot be %q", e.Format, e.Key, e.Value) -} - -func (e *PropertyValueError) Why() string { return core.InTheWordsOf(e.Reason, e.Key) } - -func (e *PropertyValueError) Instead() string { return e.Remedy } - -func (e *PropertyValueError) Error() string { - return e.InTheWordsOf(e.Key) -} - -// InTheWordsOf is this refusal with the property named the way one surface -// names it - the declared key on the command line, the label above the box in -// a window. See core.SettingSlot. -// -// The name is a field here rather than a slot in a sentence, because this -// refusal is assembled rather than written out: the key already stands on its -// own in the format string. The reason a property gives can still hold slots, -// so a declaration that names itself twice needs no special case. -func (e *PropertyValueError) InTheWordsOf(name string) string { - if name == "" { - name = e.Key - } - return fmt.Sprintf("%s: %s cannot be %q - %s", - e.Format, name, e.Value, core.InTheWordsOf(e.Reason, name)) -} - -// AboutSetting is the property this refusal is about, so a window can put the -// message under the box it came from. -// -// It was missing until 2026-08-12, and the gap is worth recording because it -// was invisible from either side. A window cannot place a message it is not -// told the subject of, so every refusal about a declared setting - width, -// height, pages, entries, a preset's own parameters - landed at the foot of the -// form however carefully the window was written. Two of the three interfaces -// beside this one had the method. This one is the one that fires most often, -// because it is the one a person hits by typing a number. -func (e *PropertyValueError) AboutSetting() string { return e.Key } - // Allows reports whether raw is a value this property accepts, and says what // is wrong when it is not. // @@ -682,6 +571,10 @@ func (d Descriptor) CheckEachProperty(props map[string]string) []error { for _, k := range keys { p, ok := known[k] if !ok { + if cannot, declared := d.cannotCarry(k); declared { + bad = append(bad, cannot) + continue + } bad = append(bad, &UnknownPropertyError{Format: d.ID, Key: k, Known: d.PropertyNames()}) continue } diff --git a/internal/format/refusals.go b/internal/format/refusals.go new file mode 100644 index 0000000..c0a971a --- /dev/null +++ b/internal/format/refusals.go @@ -0,0 +1,215 @@ +package format + +// What a format says when a setting is wrong. +// +// Split out of format.go on 2026-09-01, when the crowding gate went red: +// three files reached the band and the cap is two. The cut is by subject +// rather than by size. format.go is what a format DECLARES about itself - +// its kinds, its properties, the rules binding two of them - and this is +// what it SAYS when somebody asks for something it cannot give. +// +// Four refusals live here and they are four different mistakes, which is +// why they are four types rather than one with a code. A key nobody +// declares is a typo and lands on USAGE. A key this format cannot carry is +// a fact about the file format and lands on FORMAT. A declared key with a +// value outside its range is the caller's mistake and lands on FORMAT too, +// but says something different about what to do next. + +import ( + "fmt" + "strings" + + "github.com/donislawdev/TestingFilesGenerator/internal/core" +) + +// UnsupportedSetting is a setting a format deliberately cannot take, with the +// reason a person needs to hear. +// +// It exists because "targz does not have a property called password" is true +// and unhelpful. It reads as a gap in this build, and somebody would go looking +// for the version that has it. The truth is about the format: tar has no +// encryption at all, anywhere in its specification, and no version of this tool +// will give it any. +// +// Worth stating rather than leaving to the generic refusal because the +// alternative in the world is worse than silence. Measured on 2026-09-01: 7-Zip +// accepts -p on a tar and on a gzip, exits 0, says nothing, and writes a +// PLAINTEXT archive. Somebody asking for an encrypted fixture gets an +// unencrypted one and no warning - untouchable rule 6 broken in somebody else's +// tool. Refusing is the opposite of that, and refusing with the reason is what +// stops the person hunting for a flag that cannot exist. +// +// Declared per format rather than worked out, because only the format knows. +// A container arriving later has its own gaps - tar has no entry comment, zip +// has no owner or mode - and each is a sentence somebody has to write. +type UnsupportedSetting struct { + // Name is the key a recipe would write. + Name string + // Why is the reason, in the words the refusal uses. It is about the file + // format, not about this build. + Why string + // Instead is what to do about it, and it is allowed to be empty when there + // is genuinely nothing else to do. + Instead string +} + +// UnsupportedSettingError is a declared setting this format cannot carry. +// +// Its own type rather than UnknownPropertyError because the mistake is +// different and so is the exit code. A key nobody declares is a typo, which is +// USAGE. A key this format cannot carry is a well formed request no format +// here can deliver for this file, which is FORMAT - the same class as an +// archive asked to nest inside itself. +type UnsupportedSettingError struct { + Format string + Key string + Reason string + Remedy string +} + +// AboutSetting is the key this refusal is about, so a form can mark the box. +func (e *UnsupportedSettingError) AboutSetting() string { return e.Key } + +func (e *UnsupportedSettingError) What() string { + return fmt.Sprintf("%s cannot take %q", e.Format, e.Key) +} + +func (e *UnsupportedSettingError) Why() string { return e.Reason } + +func (e *UnsupportedSettingError) Instead() string { return e.Remedy } + +func (e *UnsupportedSettingError) Error() string { + if e.Remedy == "" { + return e.What() + " - " + e.Reason + } + return e.What() + " - " + e.Reason + ". " + e.Remedy +} + +// cannotCarry says whether this format has declared the key as one it cannot +// take, and gives the refusal if it has. +func (d Descriptor) cannotCarry(key string) (*UnsupportedSettingError, bool) { + for _, u := range d.Unsupported { + if u.Name == key { + return &UnsupportedSettingError{ + Format: d.ID, Key: key, Reason: u.Why, Remedy: u.Instead, + }, true + } + } + return nil, false +} + +// UnknownPropertyError is a property key no format recognises. +type UnknownPropertyError struct { + Format string + Key string + Known []string +} + +// AboutSetting is the property this refusal is about, so a form can put the +// message under the box it came from. Its sibling below has carried this since +// 2026-08-12 and this one did not, which made a mistyped key the one refusal +// about a declared setting that still landed at the foot of the form. +func (e *UnknownPropertyError) AboutSetting() string { return e.Key } + +// Why this is refused, for a report that keeps the four parts of D6 apart. +func (e *UnknownPropertyError) Why() string { + return "a format takes only the settings it declares, and one it does not know would be dropped on the way" +} + +// Instead is what to do about it, named from the declaration. +func (e *UnknownPropertyError) Instead() string { + if len(e.Known) == 0 { + return "remove the line" + } + return "use one of: " + strings.Join(e.Known, ", ") +} + +// What happened, without the list of names. Kept apart from Error so a report +// with four parts does not print the names twice - once in the sentence and +// again in what to do instead. +func (e *UnknownPropertyError) What() string { + if len(e.Known) == 0 { + return fmt.Sprintf("%s takes no properties, so %q is not one of them", e.Format, e.Key) + } + return fmt.Sprintf("%s does not have a property called %q", e.Format, e.Key) +} + +// Error is the whole thing in one sentence, unchanged to the character - it is +// what the one-target path from the command line flags prints. +func (e *UnknownPropertyError) Error() string { + if len(e.Known) == 0 { + return e.What() + } + return e.What() + ". It takes: " + strings.Join(e.Known, ", ") +} + +// PropertyValueError is a declared key given a value the declaration forbids. +// +// Separate from UnknownPropertyError because the mistake is different and so +// is the fix: one is a key that does not exist, the other a value out of +// range. Both are the caller's doing rather than the tool's, which is the +// point - this used to surface as a plain error and end with the exit code +// that means the program itself failed, so CI could not tell "you typed that +// wrong" from "this build has a bug". +type PropertyValueError struct { + Format string + Key string + Value string + Reason string + // Remedy is what to do about it, built from the declaration. It is carried + // here rather than worked out by whoever reports this, because a refusal in + // this tool has four parts - what happened, why, what is allowed, what to do + // instead (D6) - and the fourth had nowhere to come from until 2026-08-25. + // A reader that wants the whole thing in one sentence still gets it from + // Error, which leaves this out: it is the part a form puts under the box. + // + // Named Remedy rather than Instead because the accessor below has to be + // called Instead - that is the name the other refusals in this package use + // for the same part, and the reader that asks for all three asks by name. + Remedy string +} + +// What happened, why the declaration forbids it, and what to do instead. +// +// Instead is the part Error leaves out on purpose - see the field - so this is +// the only way a report gets all four parts of D6 for the refusal a person hits +// most often, by typing a number. +func (e *PropertyValueError) What() string { + return fmt.Sprintf("%s: %s cannot be %q", e.Format, e.Key, e.Value) +} + +func (e *PropertyValueError) Why() string { return core.InTheWordsOf(e.Reason, e.Key) } + +func (e *PropertyValueError) Instead() string { return e.Remedy } + +func (e *PropertyValueError) Error() string { + return e.InTheWordsOf(e.Key) +} + +// InTheWordsOf is this refusal with the property named the way one surface +// names it - the declared key on the command line, the label above the box in +// a window. See core.SettingSlot. +// +// The name is a field here rather than a slot in a sentence, because this +// refusal is assembled rather than written out: the key already stands on its +// own in the format string. The reason a property gives can still hold slots, +// so a declaration that names itself twice needs no special case. +func (e *PropertyValueError) InTheWordsOf(name string) string { + if name == "" { + name = e.Key + } + return fmt.Sprintf("%s: %s cannot be %q - %s", + e.Format, name, e.Value, core.InTheWordsOf(e.Reason, name)) +} + +// AboutSetting is the property this refusal is about, so a window can put the +// message under the box it came from. +// +// It was missing until 2026-08-12, and the gap is worth recording because it +// was invisible from either side. A window cannot place a message it is not +// told the subject of, so every refusal about a declared setting - width, +// height, pages, entries, a preset's own parameters - landed at the foot of the +// form however carefully the window was written. Two of the three interfaces +// beside this one had the method. This one is the one that fires most often, +// because it is the one a person hits by typing a number. +func (e *PropertyValueError) AboutSetting() string { return e.Key } diff --git a/internal/format/targz/targz.go b/internal/format/targz/targz.go index 63e5227..d804f1b 100644 --- a/internal/format/targz/targz.go +++ b/internal/format/targz/targz.go @@ -104,7 +104,30 @@ func init() { // The settings every container shares, declared once in the archive // package. Listed rather than received whole, so a format takes only // the axes it can actually carry. - Properties: archive.Axes(archive.Entries, archive.EntryFormat, archive.EntrySize), + Properties: archive.Axes(archive.Entries, archive.EntryFormat, archive.EntrySize), + + // Neither half of this format has anywhere to put a password, and + // saying so is worth more than the generic "no such property" - that + // one reads as a gap in this build, and somebody would go looking for + // the version that closed it. + // + // What the world does here is worse than either. Measured on + // 2026-09-01: 7-Zip accepts -p on a tar and on a gzip, exits 0, + // prints nothing, and writes a PLAINTEXT archive. Somebody asking + // for a locked fixture gets an open one and never finds out. + Unsupported: []format.UnsupportedSetting{ + { + Name: archive.Password, + Why: "neither tar nor gzip has any encryption in it, so there is no field in either " + + "one to put a password in", + Instead: "Use zip for an archive with a password, or leave this one open.", + }, + { + Name: archive.Encryption, + Why: "neither tar nor gzip has any encryption in it, so there is nothing to choose between", + Instead: "Use zip for an archive with a password, or leave this one open.", + }, + }, Container: true, GeneratorVersion: generatorVersion, Generator: generator{}, diff --git a/internal/format/zip/zip.go b/internal/format/zip/zip.go index 307a181..c14e348 100644 --- a/internal/format/zip/zip.go +++ b/internal/format/zip/zip.go @@ -48,6 +48,11 @@ const ( fillerName = "tfg-padding.bin" writeChunk = 32 * 1024 + + // winZipAES is the compression method a locked entry declares. It is + // not a compression at all - the real method sits in the 0x9901 field + // and is store, like everything else here. + winZipAES = 99 ) // A fixed timestamp on every entry. Taking one from the clock would make two @@ -73,7 +78,8 @@ func init() { // The settings every container shares, declared once in the archive // package. Listed rather than received whole, so a format takes only // the axes it can actually carry. - Properties: archive.Axes(archive.Entries, archive.EntryFormat, archive.EntrySize), + Properties: archive.Axes(archive.Entries, archive.EntryFormat, archive.EntrySize, + archive.Password, archive.Encryption), Container: true, GeneratorVersion: generatorVersion, Generator: generator{}, @@ -94,6 +100,11 @@ type memo struct { fillerSize int64 withFiller bool seed uint64 + // lock is what the archive is locked with, and the zero value is an + // archive that is not. It reaches build through here rather than + // through an argument because the counting pass and the writing pass + // have to agree about it exactly, and they share this. + lock archive.Lock } func (generator) Plan(r format.Request) (format.Plan, error) { @@ -102,7 +113,12 @@ func (generator) Plan(r format.Request) (format.Plan, error) { return format.Plan{}, err } - m := memo{seed: r.Seed} + lock, err := archive.ReadLock("zip", r.Properties) + if err != nil { + return format.Plan{}, err + } + + m := memo{seed: r.Seed, lock: lock} if m.children, err = planChildren(r, groups); err != nil { return format.Plan{}, err } @@ -290,6 +306,16 @@ func describe(target int64, label string, m memo, groups []format.Content) forma format.PropertyLabelEmbedded: label != "", }, } + // The lock goes into the manifest, password and all. A locked fixture + // whose password is not written down is a file no test can open, which + // makes it worth nothing - so the manifest says it in plain text, on + // purpose. Left out entirely when the archive is open, rather than + // written as "none" beside an empty password, because a key that is + // there says something happened. + if m.lock.On() { + p.Properties[archive.Encryption] = m.lock.Method + p.Properties[archive.Password] = m.lock.Password + } // The single format shape keeps the keys it always had, so a test asserting // on entry_format does not break the day contains arrives. if len(groups) == 1 { @@ -387,40 +413,115 @@ func (generator) Write(ctx context.Context, w io.Writer, p format.Plan) error { // One function with a mode rather than two, so the structure, the order of the // entries and the comment cannot drift between what was measured and what is // written. Only the data writes differ. +// entryPlan is one entry, as both passes see it. +// +// A record rather than five arguments, because the two passes have to +// describe the entry identically and a positional list of five is where +// that stops being obvious. +type entryPlan struct { + name string + plain int64 + index int + // withContents is false during the pass that only measures. + withContents bool +} + +// openEntry starts the next entry and gives back what its contents go to. +// +// Two shapes, and the difference is the whole of what locking costs here. An +// open archive streams: CreateHeader writes a header with the sizes left blank +// and puts them in a descriptor after the data, which is what this format has +// always done. A locked one cannot, because the entry carries a salt and an +// authentication code that the declared size has to include - so it goes +// through CreateRaw with the sizes stated up front, which is also exactly the +// shape 7-Zip writes. Measured, in docs/MVP-FORMATS.md section 2.16. +// +// plain is the size of the contents before locking. index numbers the entry +// across the archive, and it is what gives each one its own salt. +// +// The returned closer finishes the entry. It writes the authentication code +// for a locked entry and does nothing for an open one, and it is not called at +// all during the counting pass - where no contents are written, and CreateRaw +// is content to be handed a header and no bytes. +func openEntry(zw *stdzip.Writer, m memo, e entryPlan) (io.Writer, func() error, error) { + nothingToShut := func() error { return nil } + + if !m.lock.On() { + entry, err := zw.CreateHeader(&stdzip.FileHeader{ + Name: e.name, + Method: stdzip.Store, + Modified: fixedTime, + }) + return entry, nothingToShut, err + } + + h := &stdzip.FileHeader{ + Name: e.name, + Method: winZipAES, + Modified: fixedTime, + Extra: m.lock.Extra(), + } + // Bit 0 says the entry is encrypted. The CRC stays zero because AE-2 + // carries none - which is what lets the contents be written in one pass. + h.Flags |= 1 + h.CRC32 = 0 + h.CompressedSize64 = uint64(e.plain + m.lock.EntryOverhead()) + h.UncompressedSize64 = uint64(e.plain) + + raw, err := zw.CreateRaw(h) + if err != nil { + return nil, nil, err + } + if !e.withContents { + // The counting pass writes nothing, and that has to include the salt + // and the verifier. Building the locked writer here would emit both + // straight away, so the measurement would count them and the + // arithmetic below would add them again - eighteen bytes an entry, + // which is what the engine caught when this was written the other way. + return raw, nothingToShut, nil + } + locked, err := m.lock.NewEntryWriter(raw, m.seed, e.index) + if err != nil { + return nil, nil, err + } + return locked, locked.Close, nil +} + func build(ctx context.Context, w io.Writer, m memo, withContents bool) error { zw := stdzip.NewWriter(w) if err := zw.SetComment(m.comment); err != nil { return fmt.Errorf("zip: the archive comment was refused: %w", err) } - for _, c := range m.children { + for i, c := range m.children { select { case <-ctx.Done(): return ctx.Err() default: } - entry, err := zw.CreateHeader(&stdzip.FileHeader{ - Name: c.name, - Method: stdzip.Store, - Modified: fixedTime, + entry, shut, err := openEntry(zw, m, entryPlan{ + name: c.name, plain: c.plan.Bytes, index: i, withContents: withContents, }) if err != nil { return err } - if !withContents { - continue - } - if err := c.desc.Generator.Write(ctx, entry, c.plan); err != nil { - return fmt.Errorf("zip: the %s file inside could not be written: %w", c.desc.ID, err) + if withContents { + if err := c.desc.Generator.Write(ctx, entry, c.plan); err != nil { + return fmt.Errorf("zip: the %s file inside could not be written: %w", c.desc.ID, err) + } + if err := shut(); err != nil { + return err + } } } if m.withFiller { - entry, err := zw.CreateHeader(&stdzip.FileHeader{ - Name: fillerName, - Method: stdzip.Store, - Modified: fixedTime, + // The filler is locked with everything else. An archive where one entry + // opens without the password and the rest do not is a file nobody + // asked for, and the arithmetic is the same either way. + entry, shut, err := openEntry(zw, m, entryPlan{ + name: fillerName, plain: m.fillerSize, index: len(m.children), withContents: withContents, }) if err != nil { return err @@ -429,6 +530,9 @@ func build(ctx context.Context, w io.Writer, m memo, withContents bool) error { if err := writeFiller(ctx, entry, m.seed, m.fillerSize); err != nil { return err } + if err := shut(); err != nil { + return err + } } } @@ -495,10 +599,10 @@ func archiveSize(m memo) (int64, error) { } total := c.n for _, ch := range m.children { - total += ch.plan.Bytes + total += ch.plan.Bytes + m.lock.EntryOverhead() } if m.withFiller { - total += m.fillerSize + total += m.fillerSize + m.lock.EntryOverhead() } return total, nil } diff --git a/internal/guard/archivelock_test.go b/internal/guard/archivelock_test.go new file mode 100644 index 0000000..b4fa65d --- /dev/null +++ b/internal/guard/archivelock_test.go @@ -0,0 +1,358 @@ +package guard + +import ( + "context" + "errors" + "os" + "os/exec" + "path/filepath" + "strings" + "testing" + + "github.com/donislawdev/TestingFilesGenerator/internal/format" + _ "github.com/donislawdev/TestingFilesGenerator/internal/format/all" + "github.com/donislawdev/TestingFilesGenerator/internal/format/archive" + "github.com/donislawdev/TestingFilesGenerator/internal/oracle" +) + +// The methods this build writes, and the bytes each one adds to an entry. +// +// The numbers are measured rather than derived, twice over and from two +// directions - the size of the whole file and the compressed size field in the +// local header of what 7-Zip writes. Written up in docs/MVP-FORMATS.md section +// 2.16. Repeated here on purpose: a guard that reads its expectation out of the +// code it is guarding proves the code agrees with itself. +var lockOverhead = map[string]int64{ + archive.AES128: 20, + archive.AES192: 24, + archive.AES256: 28, +} + +// A locked archive is one a real archiver opens with the password and refuses +// without it. +// +// This is the guard the whole feature rests on, and it is deliberately asked of +// somebody else's program. Our own code can be sure it encrypted something and +// be wrong about every detail that matters - the counter mode counting the +// wrong way, the key derived from the wrong material, the extra field saying a +// key length the data does not have. None of that shows up as an error here. It +// shows up as 7-Zip saying no. +// +// Three answers are needed, not one. A reader that accepts the file proves the +// bytes are well formed. A reader that REFUSES the wrong password proves the +// verifier and the authentication code are real rather than decorative - and a +// reader that accepts anything would have passed the first check while proving +// nothing at all. +func TestARealArchiverOpensALockedArchiveAndRefusesTheWrongPassword(t *testing.T) { + bin, ok := oracle.SevenZip() + if !ok { + t.Skip("7-Zip is not installed here, so nothing can say whether the file is really locked") + } + + const password = "Secret123" + dir := t.TempDir() + + for _, method := range []string{archive.AES128, archive.AES192, archive.AES256} { + t.Run(method, func(t *testing.T) { + path := filepath.Join(dir, "locked-"+method+".zip") + writeLocked(t, path, 40*1024, method, password, 7741) + + if out, err := runSevenZip(bin, path, password); err != nil { + t.Fatalf("7-Zip refused an archive locked with %s and given the right password: %v\n%s", + method, err, out) + } + if _, err := runSevenZip(bin, path, "NotThePassword"); err == nil { + t.Errorf("7-Zip accepted %s with the WRONG password, so nothing here proves the archive is locked", + method) + } + // And with none at all, which is what a system under test does + // before it knows there is a password. + if _, err := runSevenZip(bin, path, ""); err == nil { + t.Errorf("7-Zip read %s with no password at all", method) + } + }) + } +} + +// Locking does not cost the exact size, which is the promise the format makes. +// +// The reason it can be kept is the measurement: a stream cipher does not change +// the length of what it encrypts, so a lock adds a FIXED count per entry and +// the size of the archive stays an exact function of its input. If that were +// not true the planning phase could not state a size without building the +// archive, and this setting could not exist at all - which is the difference +// between it and compression. +// +// Sizes that cross the awkward places: just above the floor, odd, and large +// enough that the padding entry is doing the work rather than the comment. +func TestALockedArchiveStillHitsTheSizeToTheByte(t *testing.T) { + dir := t.TempDir() + for _, method := range []string{archive.AES128, archive.AES192, archive.AES256} { + for _, size := range []int64{12 * 1024, 12*1024 + 1, 40 * 1024, 300*1024 + 7} { + path := filepath.Join(dir, "size.zip") + writeLocked(t, path, size, method, "Secret123", 11) + + st, err := os.Stat(path) + if err != nil { + t.Fatal(err) + } + if st.Size() != size { + t.Errorf("%s at %d B: the file on disk is %d B", method, size, st.Size()) + } + if err := os.Remove(path); err != nil { + t.Fatal(err) + } + } + } +} + +// The bytes a lock adds are the bytes it was measured to add. +// +// Asked of the declaration rather than of a file, because this is the number +// the planning phase adds to reach an exact size, and a plan that is wrong here +// produces an archive the engine deletes for missing its own size. The guard +// above would catch that - this one says which number moved. +func TestTheCostOfALockIsTheMeasuredCost(t *testing.T) { + for method, want := range lockOverhead { + got := archive.Lock{Method: method, Password: "x"}.EntryOverhead() + if got != want { + t.Errorf("%s adds %d B to an entry and the measurement is %d", method, got, want) + } + } + if open := (archive.Lock{}).EntryOverhead(); open != 0 { + t.Errorf("an archive that is not locked adds %d B", open) + } +} + +// Two settings say one thing, so the two that disagree are refused. +// +// The pair is forced by the window rather than chosen. A menu cannot be empty - +// it opens on its declared default and sends it - so encryption arrives as +// "none" on every run from a window whether or not anybody looked at it, while +// a box somebody types in arrives empty and is left out. "Password given, +// encryption none" therefore cannot be told apart from "password given, +// encryption not considered". +// +// Guessing between them is what the refusal exists to prevent, and the bad +// guess is not hypothetical: measured on 2026-09-01, 7-Zip takes -p on a tar, +// exits 0, says nothing and writes a plaintext archive. Somebody asking for a +// locked fixture gets an open one. +// +// Both halves have to be named. A refusal saying only "password" leaves +// somebody looking at a filled in password box wondering what is wrong with it. +func TestTheTwoHalvesOfALockHaveToAgree(t *testing.T) { + d := descriptorFor(t, "zip") + + cases := []struct { + name string + props map[string]string + about string + }{ + { + name: "a password with the lock switched off", + props: map[string]string{archive.Password: "Secret123", archive.Encryption: archive.NoEncryption}, + about: archive.Encryption, + }, + { + name: "a lock with no password to lock it", + props: map[string]string{archive.Encryption: archive.AES256}, + about: archive.Password, + }, + } + + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + _, err := d.Generator.Plan(format.Request{Bytes: 40 * 1024, Seed: 7741, Properties: c.props}) + if err == nil { + t.Fatal("accepted, so an archive was about to be built on two settings that contradict each other") + } + var value *format.PropertyValueError + if !errors.As(err, &value) { + t.Fatalf("the refusal is %T, so it lands on a different exit code than a bad value: %v", err, err) + } + if value.AboutSetting() != c.about { + t.Errorf("the refusal is about %q and the field to mark is %q", value.AboutSetting(), c.about) + } + // Both halves, by name, in one sentence. + if whole := value.Error(); !mentionsBothHalves(whole) { + t.Errorf("the refusal names one half and not the other, so it reads as a defect in the box "+ + "somebody filled in: %q", whole) + } + }) + } +} + +// mentionsBothHalves says whether a sentence talks about the password AND about +// whether the archive is locked. Both spellings of the second, because the +// refusal about a missing password names the method rather than the key. +func mentionsBothHalves(s string) bool { + saysPassword := strings.Contains(s, "password") + saysLock := strings.Contains(s, "encryption") || strings.Contains(s, "locked") || + strings.Contains(s, archive.AES256) + return saysPassword && saysLock +} + +// A format that cannot lock says why, in words about the format. +// +// "targz does not have a property called password" is true and sends somebody +// looking for the build that has it. There is no such build and there never +// will be: neither tar nor gzip has any encryption in it. That is a fact about +// the file format, and the refusal is the only place a person will read it. +// +// The exit code is half the point. A key nobody declares is a typo, which is +// USAGE. A key this format cannot carry is a well formed request no format here +// can deliver, which is FORMAT - and a script branching on the code would +// otherwise be told that a deliberate limitation was a spelling mistake. +func TestAFormatThatCannotLockSaysSoInItsOwnWords(t *testing.T) { + d := descriptorFor(t, "targz") + + for _, key := range []string{archive.Password, archive.Encryption} { + bad := d.CheckEachProperty(map[string]string{key: "Secret123"}) + if len(bad) == 0 { + t.Fatalf("targz accepted %q, and 7-Zip accepting -p on a tar and writing plaintext is exactly "+ + "the behaviour this is here to not copy", key) + } + var unsupported *format.UnsupportedSettingError + if !errors.As(bad[0], &unsupported) { + t.Errorf("%q on targz is refused as %T, which reads as a gap in this build rather than as a "+ + "fact about tar: %v", key, bad[0], bad[0]) + continue + } + if !strings.Contains(unsupported.Why(), "tar") || !strings.Contains(unsupported.Why(), "gzip") { + t.Errorf("the reason for %q does not name the two formats it is about: %q", key, unsupported.Why()) + } + if unsupported.AboutSetting() != key { + t.Errorf("the refusal about %q says it is about %q, so a window marks the wrong box", + key, unsupported.AboutSetting()) + } + } +} + +// The password goes into the manifest, in plain text, on purpose. +// +// A locked fixture whose password is written nowhere is a file no test can +// open, which makes the whole run worth nothing. This is the one place in the +// tool where writing a secret down is the correct behaviour rather than a leak, +// and it is a decision by the owner rather than an oversight - so it gets a +// guard, because the next person to read it will assume it is a mistake. +func TestTheManifestCarriesThePasswordSoATestCanOpenTheFile(t *testing.T) { + d := descriptorFor(t, "zip") + + plan, err := d.Generator.Plan(format.Request{ + Bytes: 40 * 1024, Seed: 7741, + Properties: map[string]string{archive.Password: "Secret123", archive.Encryption: archive.AES256}, + }) + if err != nil { + t.Fatal(err) + } + if got := plan.Properties[archive.Password]; got != "Secret123" { + t.Errorf("the manifest records the password as %v, so nothing can open the file it describes", got) + } + if got := plan.Properties[archive.Encryption]; got != archive.AES256 { + t.Errorf("the manifest records the encryption as %v", got) + } + + // And an open archive says nothing about either, rather than saying none. + // A key that is there reads as a thing that happened. + open, err := d.Generator.Plan(format.Request{Bytes: 40 * 1024, Seed: 7741}) + if err != nil { + t.Fatal(err) + } + if _, there := open.Properties[archive.Password]; there { + t.Error("an archive with no password still records one") + } + if _, there := open.Properties[archive.Encryption]; there { + t.Error("an archive that is not locked still records an encryption") + } +} + +// The salt comes from the run, so one recipe is one file. +// +// Encryption is where determinism is easiest to lose and hardest to notice: a +// salt from crypto/rand gives a perfectly good archive that differs on every +// run, and every check except this one would stay green. Untouchable rule 3 +// says the bytes hold within a major version, and a fixture nobody can +// reproduce is not a fixture. +// +// The other half matters too. A salt that ignores the seed would make every +// archive identical, which is deterministic and wrong - two runs asking for +// different seeds have to differ. +func TestALockedArchiveIsTheSameFileForTheSameSeed(t *testing.T) { + dir := t.TempDir() + + first := filepath.Join(dir, "a.zip") + same := filepath.Join(dir, "b.zip") + other := filepath.Join(dir, "c.zip") + writeLocked(t, first, 40*1024, archive.AES256, "Secret123", 4242) + writeLocked(t, same, 40*1024, archive.AES256, "Secret123", 4242) + writeLocked(t, other, 40*1024, archive.AES256, "Secret123", 9999) + + a, b, c := readAll(t, first), readAll(t, same), readAll(t, other) + if string(a) != string(b) { + t.Error("the same recipe and seed gave two different archives, so the salt is drawn rather than derived") + } + if string(a) == string(c) { + t.Error("two different seeds gave the same archive, so the salt ignores the seed") + } +} + +// writeLocked builds one locked archive on disk. +func writeLocked(t *testing.T, path string, size int64, method, password string, seed uint64) { + t.Helper() + d := descriptorFor(t, "zip") + plan, err := d.Generator.Plan(format.Request{ + Bytes: size, Seed: seed, Label: true, + Properties: map[string]string{archive.Password: password, archive.Encryption: method}, + }) + if err != nil { + t.Fatalf("planning %d B locked with %s: %v", size, method, err) + } + f, err := os.Create(path) + if err != nil { + t.Fatal(err) + } + writeErr := d.Generator.Write(context.Background(), f, plan) + closeErr := f.Close() + if writeErr != nil { + t.Fatalf("writing %s: %v", path, writeErr) + } + if closeErr != nil { + t.Fatal(closeErr) + } +} + +// runSevenZip tests an archive, with a password or without one. +func runSevenZip(bin, path, password string) (string, error) { + args := []string{"t", path} + if password != "" { + args = append(args, "-p"+password) + } else { + // Without this the archiver asks at the console and the test hangs. + args = append(args, "-p") + } + // The binary is the one the oracle package found and the path is a file + // this test just wrote, so neither comes from anything a person typed. + //nolint:gosec // both arguments are ours + // nosemgrep: go.lang.security.audit.dangerous-exec-command.dangerous-exec-command + out, err := exec.Command(bin, args...).CombinedOutput() + return string(out), err +} + +func readAll(t *testing.T, path string) []byte { + t.Helper() + b, err := os.ReadFile(path) + if err != nil { + t.Fatal(err) + } + return b +} + +// descriptorFor is one registered format, or a failure naming it. +func descriptorFor(t *testing.T, id string) format.Descriptor { + t.Helper() + d, err := format.Get(id) + if err != nil { + t.Fatalf("%s is not registered: %v", id, err) + } + return d +} diff --git a/internal/guard/parity_test.go b/internal/guard/parity_test.go index 637626b..85a8fbb 100644 --- a/internal/guard/parity_test.go +++ b/internal/guard/parity_test.go @@ -141,9 +141,11 @@ var reachableFromTheWindow = []string{ "property:wav.channels", "property:wav.content", "property:wav.sample_rate", + "property:zip.encryption", "property:zip.entries", "property:zip.entry_format", "property:zip.entry_size", + "property:zip.password", // One target's worth of settings, each with a field and each found again in // the manifest of a run started from the window. diff --git a/internal/oracle/oracle.go b/internal/oracle/oracle.go index 56e8ef8..ba1a2e7 100644 --- a/internal/oracle/oracle.go +++ b/internal/oracle/oracle.go @@ -312,6 +312,14 @@ func inPath(name string) func() (string, bool) { // sevenZip looks in the usual place on Windows as well, because the installer // does not put it on the path. +// SevenZip is where the archiver lives on this machine. +// +// Exported for a guard that has to run it with arguments no Checker +// declares - a password, for one. The alternative was a second copy of +// these search paths in the test package, which is exactly the shape this +// tree spends its time removing. +func SevenZip() (string, bool) { return sevenZip() } + func sevenZip() (string, bool) { if p, err := exec.LookPath("7z"); err == nil { return p, true From d66acef7cd476d3b5f921bc9c38b15a76bb319ec Mon Sep 17 00:00:00 2001 From: DonislawDev Date: Tue, 1 Sep 2026 10:21:26 +0200 Subject: [PATCH 2/4] test: the lock guards were proving less than they said, and a mutation said so Two of the seven mutations written for the locking work came back NOT CAUGHT, and neither was a false alarm. Both guards had a real hole. The archiver guard could not see a broken keystream, and the reason is a property of AE-2 rather than an oversight. An AE-2 entry carries a CRC of zero and its authentication code is computed over the CIPHERTEXT, so a reader checks that nobody edited the encrypted bytes and never that the plaintext is what was put in. Turn the counter the wrong way round and 7-Zip still says "Everything is Ok": the file is well formed, the password verifies, the code matches, and what comes out is noise. The guard now opens the archive for real and compares what comes out against the same archive built without a lock, which holds the same children from the same seed. Both sides are extracted by 7-Zip, so this is still not our own code judging its own arithmetic. The determinism guard could not see a salt that ignored the seed. Comparing whole archives is too blunt for that: the children are seeded from the run too, so two seeds give different contents and therefore different files whatever the salt does. It now reads the sixteen bytes where a WinZip AES entry keeps its salt and asks them directly, which DataOffset makes possible without a second zip parser. Both mutations are caught now. Nine of nine. Co-Authored-By: Claude Opus 5 --- internal/guard/archivelock_test.go | 116 ++++++++++++++++++++++++++++- 1 file changed, 113 insertions(+), 3 deletions(-) diff --git a/internal/guard/archivelock_test.go b/internal/guard/archivelock_test.go index b4fa65d..4e47c40 100644 --- a/internal/guard/archivelock_test.go +++ b/internal/guard/archivelock_test.go @@ -1,6 +1,8 @@ package guard import ( + stdzip "archive/zip" + "bytes" "context" "errors" "os" @@ -61,6 +63,35 @@ func TestARealArchiverOpensALockedArchiveAndRefusesTheWrongPassword(t *testing.T t.Fatalf("7-Zip refused an archive locked with %s and given the right password: %v\n%s", method, err, out) } + + // And the contents come back out as themselves. + // + // This half was missing until a mutation found it, and the miss is + // worth writing down because it is a property of AE-2 rather than + // an oversight. An AE-2 entry carries a CRC of zero and its + // authentication code is computed over the CIPHERTEXT, so a reader + // checks that nobody edited the encrypted bytes and never that the + // plaintext is what was put in. Turn the keystream counter the + // wrong way round and 7-Zip still says "Everything is Ok" - the + // file is well formed, the password verifies, the code matches, + // and what comes out is noise. + // + // So the archive is opened for real and compared against the same + // archive built without a lock, which holds the same children from + // the same seed. Both sides are extracted by 7-Zip, so this is not + // our own code judging its own arithmetic. + open := filepath.Join(dir, "open-"+method+".zip") + writeArchive(t, open, 40*1024, nil, 7741) + + locked := extract(t, bin, path, "txt_0001.txt", password) + plain := extract(t, bin, open, "txt_0001.txt", "") + if string(locked) != string(plain) { + t.Errorf("%s: the file inside came back out different from the same file in an unlocked "+ + "archive, so the contents are being scrambled rather than encrypted", method) + } + if len(plain) == 0 { + t.Fatal("the unlocked archive gave nothing back, so the comparison above proved nothing") + } if _, err := runSevenZip(bin, path, "NotThePassword"); err == nil { t.Errorf("7-Zip accepted %s with the WRONG password, so nothing here proves the archive is locked", method) @@ -294,18 +325,72 @@ func TestALockedArchiveIsTheSameFileForTheSameSeed(t *testing.T) { if string(a) == string(c) { t.Error("two different seeds gave the same archive, so the salt ignores the seed") } + + // The salt itself, not the file it is in. + // + // Comparing whole archives is too blunt to say anything about the salt: the + // children are seeded from the run too, so two seeds give different + // contents and therefore different files whatever the salt does. A mutation + // that made the salt ignore the seed entirely left the comparison above + // green. This reads the sixteen bytes at the start of the entry, which is + // where a WinZip AES entry keeps its salt, and asks them directly. + if first, other := saltOf(t, first), saltOf(t, other); string(first) == string(other) { + t.Errorf("two seeds gave the same salt %x, so the salt is not derived from the run - "+ + "every archive this build writes shares it", first) + } +} + +// saltOf is the salt at the start of the first entry of a locked archive. +// +// DataOffset is what makes this readable without a second zip parser: the +// standard library says where the entry's bytes begin, and for an AES entry the +// salt is the first thing there. +func saltOf(t *testing.T, path string) []byte { + t.Helper() + r, err := stdzip.OpenReader(path) + if err != nil { + t.Fatalf("opening %s: %v", path, err) + } + defer func() { _ = r.Close() }() + if len(r.File) == 0 { + t.Fatalf("%s holds nothing, so there is no salt to read", path) + } + at, err := r.File[0].DataOffset() + if err != nil { + t.Fatalf("finding the data of %s: %v", r.File[0].Name, err) + } + + f, err := os.Open(path) + if err != nil { + t.Fatal(err) + } + defer func() { _ = f.Close() }() + // Sixteen, because these archives are locked with AES-256 and its salt is + // half the key. + salt := make([]byte, 16) + if _, err := f.ReadAt(salt, at); err != nil { + t.Fatalf("reading the salt of %s: %v", path, err) + } + return salt } // writeLocked builds one locked archive on disk. func writeLocked(t *testing.T, path string, size int64, method, password string, seed uint64) { + t.Helper() + writeArchive(t, path, size, map[string]string{ + archive.Password: password, archive.Encryption: method, + }, seed) +} + +// writeArchive builds one archive on disk, locked or not. +func writeArchive(t *testing.T, path string, size int64, props map[string]string, seed uint64) { t.Helper() d := descriptorFor(t, "zip") plan, err := d.Generator.Plan(format.Request{ - Bytes: size, Seed: seed, Label: true, - Properties: map[string]string{archive.Password: password, archive.Encryption: method}, + Bytes: size, Seed: seed, Label: true, Properties: props, }) if err != nil { - t.Fatalf("planning %d B locked with %s: %v", size, method, err) + t.Fatalf("planning %d B with %v: %v", size, props, err) } f, err := os.Create(path) if err != nil { @@ -356,3 +441,28 @@ func descriptorFor(t *testing.T, id string) format.Descriptor { } return d } + +// extract pulls one member out of an archive, using the archiver rather than +// anything of ours. Reading it back with our own code would be the generator +// checking its own arithmetic. +func extract(t *testing.T, bin, path, member, password string) []byte { + t.Helper() + args := []string{"e", "-so", path, member} + if password != "" { + args = append(args, "-p"+password) + } else { + args = append(args, "-p") + } + // Both arguments are ours: the binary came from the oracle package and + // the path is a file this test just wrote. + //nolint:gosec // the command is ours + // nosemgrep: go.lang.security.audit.dangerous-exec-command.dangerous-exec-command + cmd := exec.Command(bin, args...) + var out, errOut bytes.Buffer + cmd.Stdout = &out + cmd.Stderr = &errOut + if err := cmd.Run(); err != nil { + t.Fatalf("extracting %s from %s: %v\n%s", member, filepath.Base(path), err, errOut.String()) + } + return out.Bytes() +} From 67506a55412ed5c5cddf0a72fa7b4173c1184ab5 Mon Sep 17 00:00:00 2001 From: DonislawDev Date: Tue, 1 Sep 2026 10:35:30 +0200 Subject: [PATCH 3/4] format: a tar.gz can say what its files may do and who owns them tfg generate --format targz --size 12kb --set entry_mode=755 --set entry_owner=root gives an archive where tar -tvzf reads "-rwxr-xr-x root/root". Modes are the ones chmod takes, 000 through 777, and owners are unset, root and user. It costs nothing, and that is why it exists rather than waiting. Every field of a USTAR header is fixed width, so the mode and the owner are written into space already paid for whatever they say - the archive is the same size either way, so this does not touch the exact size the format promises the way compression would. The useful cases are the ones nobody makes by accident. 000 is a file nothing can read after unpacking, 777 is one a scanner should have something to say about, and an archive claiming root owns everything is what a careless extractor turns into a privilege problem. Building one of those by hand needs a machine with the right permissions. Here it is a flag. The defaults are what this format has always written, to the byte, and that is untouchable rule 3 rather than a preference: a default that recorded an owner would move the bytes of every archive already produced, silently, because an archive with an owner in it is just as valid as one without. Both settings sit in the shared archive vocabulary while only targz names them, which is the other direction of the same asymmetry the password showed. Zip declares a lock tar has nowhere to put, tar declares an owner zip records differently, and each format lists what it can actually carry. One finding came out of this that is bigger than the setting, and it was found because a guard tried to read an archive this tool had just written and could not. Go's compress/gzip accepts a header comment of 511 bytes and refuses one of 512, because it reads the field into a fixed buffer. This format pads through that comment, up to four kilobytes of it. Measured across 866 archive sizes from the minimum upwards: 548 readable by Go, 318 not, first failure at 10269 B. Every one of those is fine to 7-Zip, GNU tar and bsdtar, and every one of them fails in a Go program with "gzip: invalid header" - a message that reads like a corrupt file rather than like a field this reader will not take. Not fixed here, because both ways of fixing it move bytes and that is the owner's call under untouchable rule 3. Written up as O163 with the two options and what each one would cost. The guard reads past the gzip header by hand in the meantime, so a test about permissions does not fail on a fault that is not its subject. Co-Authored-By: Claude Opus 5 --- CHANGELOG.md | 39 +++++ internal/format/archive/archive.go | 31 ++++ internal/format/archive/ownership.go | 76 +++++++++ internal/format/targz/size.go | 39 +++-- internal/format/targz/targz.go | 14 +- internal/guard/archiveownership_test.go | 198 ++++++++++++++++++++++++ internal/guard/mutationcoverage_test.go | 9 ++ internal/guard/parity_test.go | 2 + 8 files changed, 396 insertions(+), 12 deletions(-) create mode 100644 internal/format/archive/ownership.go create mode 100644 internal/guard/archiveownership_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index 6e17810..f14132a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -47,6 +47,45 @@ because it turns other people's test suites red. ### Added +- **A zip can be locked with a password.** + + tfg generate --format zip --size 30kb --set entries=3 \ + --set password=Secret123 --set encryption=aes-256 + + writes an archive of exactly 30720 B that 7-Zip opens with that password + and refuses without it. The methods are `aes-128`, `aes-192` and + `aes-256`. + + **The password goes into the manifest in plain text.** A locked fixture + nobody can open is worth nothing, so the manifest records it exactly as + you typed it - that is the point rather than a leak. Do not use a password + you use anywhere else. + + Both settings are needed together. A password with `encryption=none`, or + an encryption with no password, is refused rather than guessed at, and the + refusal names both of them. + + **Limits worth knowing before you build a fixture.** Some readers cannot + open AES archives at all - .NET's own `ZipFile` lists the entries and then + fails on reading one. Nothing in this build writes the older ZipCrypto + scheme yet, so an archive meant for a reader that only speaks that is not + something this can make. And `tar.gz` cannot be locked at all - neither + tar nor gzip has any encryption in it, and asking for one there is refused + with that reason rather than ignored. + +- **A tar.gz can say what permissions its files have and who owns them.** + `--set entry_mode=755` and `--set entry_owner=root`. The modes are the + ones chmod takes, from `000` through `777`, and the owners are `unset` + (the default, and what this tool has always written), `root` and `user`. + + The useful cases are the ones nobody makes by accident: `000` is a file + nothing can read after unpacking, `777` is one a scanner should have + something to say about, and an archive claiming root owns everything is + what a careless extractor turns into a privilege problem. + + It changes no bytes unless you ask for it, and the size of the archive is + the same either way. + - **A log can now be six shapes rather than one, and seven settings shape it.** `tfg generate --format log --set entry_format=nginx` writes an nginx access log. The others are `apache-combined` (the default, and what this format has diff --git a/internal/format/archive/archive.go b/internal/format/archive/archive.go index 877e39e..90c8178 100644 --- a/internal/format/archive/archive.go +++ b/internal/format/archive/archive.go @@ -43,6 +43,19 @@ const ( EntrySize = "entry_size" Password = "password" Encryption = "encryption" + EntryMode = "entry_mode" + EntryOwner = "entry_owner" +) + +// The owners an entry can be recorded as. +const ( + // OwnerUnset records no owner at all, which is what a tar written here has + // always carried and therefore what the default has to stay - a different + // default would move the bytes of every archive, which is untouchable rule + // 3. + OwnerUnset = "unset" + OwnerRoot = "root" + OwnerUser = "user" ) // The encryption methods, spelled the way a recipe writes them. @@ -129,6 +142,24 @@ var axes = map[string]format.Property{ Detail: "The password the archive is locked with. It is written into the manifest as you typed it, " + "because a test that cannot open the file cannot check anything.", }, + EntryMode: { + Name: EntryMode, Kind: format.PropertyChoice, + // Written the way chmod takes them, and sorted, because a closed set + // has one order on every surface. The interesting ones for a test are + // at the ends: 000 is a file nothing can read, 444 is read only, and + // 666 and 777 are what a scanner should have something to say about. + Choices: []string{"000", "400", "444", "600", "644", "664", "666", "700", "755", "777"}, + Default: "644", + Detail: "The permissions recorded for each file inside. It is what the archive says, " + + "not what the file gets - that depends on who unpacks it and how.", + }, + EntryOwner: { + Name: EntryOwner, Kind: format.PropertyChoice, + Choices: []string{OwnerRoot, OwnerUnset, OwnerUser}, + Default: OwnerUnset, + Detail: "Who each file inside belongs to. Leave it unset and the archive names nobody, " + + "which is what most archives written by a build carry.", + }, Encryption: { Name: Encryption, Kind: format.PropertyChoice, // Sorted, because a closed set has one order on every surface. diff --git a/internal/format/archive/ownership.go b/internal/format/archive/ownership.go new file mode 100644 index 0000000..e78b8d7 --- /dev/null +++ b/internal/format/archive/ownership.go @@ -0,0 +1,76 @@ +package archive + +import ( + "strconv" + + "github.com/donislawdev/TestingFilesGenerator/internal/format" +) + +// What an archive records about the files it holds, beyond their bytes. +// +// None of this changes the size of anything, and that is what makes it cheap: +// a USTAR header is 512 bytes whatever the mode says and whoever the owner is, +// because every field in it is fixed width. So the arithmetic the container +// design rests on is untouched, and these settings cost a person nothing to +// use. +// +// They are worth having because the interesting cases are the ones nobody +// creates by accident. A file recorded as 000 is one nothing can read after +// unpacking, 777 is one a scanner should have something to say about, and an +// archive claiming root owns everything is what a careless extractor turns +// into a privilege problem. Producing one of those by hand takes a machine +// with the right permissions. Producing one here takes a flag. + +// Ownership is what an entry says about its permissions and its owner. +// +// The zero value is what this tool has always written, which is the default +// for the same reason the default of every other setting is what it is: a +// different one would move the bytes of every archive already out there. +type Ownership struct { + // Mode is the permission bits, already parsed from the three digits a + // person writes. + Mode int64 + // Uid and Gid are the numeric owner, and Uname and Gname are the names. + // All four are left at nought and empty for the unset owner. + Uid, Gid int + Uname, Gname string +} + +// ReadOwnership reads the two settings that say what an entry records about +// itself. +// +// The mode arrives as three octal digits because that is how a person writes +// one and how chmod takes one. Parsing it here rather than declaring an int +// keeps 644 meaning what it looks like: as a decimal it is 1204 in octal, and +// a setting where the number a person types is not the number the file gets is +// the kind of difference nobody predicts. +func ReadOwnership(id string, props map[string]string) (Ownership, error) { + own := Ownership{Mode: 0o644} + + if raw := props[EntryMode]; raw != "" { + mode, err := strconv.ParseInt(raw, 8, 32) + if err != nil { + // Unreachable through the registry, which checks the value against + // the declared set before a generator sees it. Here because this + // function is also callable directly, and a silent 0 would be a + // file nothing can read. + return Ownership{}, &format.PropertyValueError{ + Format: id, Key: EntryMode, Value: raw, + Reason: "it is not a permission written as octal digits", + Remedy: "Write it the way chmod takes it, so 644 or 755.", + } + } + own.Mode = mode + } + + switch props[EntryOwner] { + case OwnerRoot: + own.Uname, own.Gname = "root", "root" + case OwnerUser: + // The first ordinary account on a Linux system, which is what somebody + // unpacking a fixture on their own machine most likely is. + own.Uid, own.Gid = 1000, 1000 + own.Uname, own.Gname = "user", "user" + } + return own, nil +} diff --git a/internal/format/targz/size.go b/internal/format/targz/size.go index 5b4bf4c..c981a1c 100644 --- a/internal/format/targz/size.go +++ b/internal/format/targz/size.go @@ -10,6 +10,7 @@ import ( "github.com/donislawdev/TestingFilesGenerator/internal/core" "github.com/donislawdev/TestingFilesGenerator/internal/format" + "github.com/donislawdev/TestingFilesGenerator/internal/format/archive" ) // How the size of this format is worked out. @@ -228,17 +229,19 @@ func build(ctx context.Context, w io.Writer, m memo) error { if err := ctx.Err(); err != nil { return err } - if err := writeEntry(ctx, tw, c.name, c.plan.Bytes, func(dst io.Writer) error { - return c.desc.Generator.Write(ctx, dst, c.plan) - }); err != nil { + if err := writeEntry(ctx, tw, tarEntry{name: c.name, size: c.plan.Bytes, own: m.own}, + func(dst io.Writer) error { + return c.desc.Generator.Write(ctx, dst, c.plan) + }); err != nil { return fmt.Errorf("targz: the %s file inside could not be written: %w", c.desc.ID, err) } } if m.withFiller { - if err := writeEntry(ctx, tw, fillerName, m.fillerSize, func(dst io.Writer) error { - return writeFiller(ctx, dst, m.seed, m.fillerSize) - }); err != nil { + if err := writeEntry(ctx, tw, tarEntry{name: fillerName, size: m.fillerSize, own: m.own}, + func(dst io.Writer) error { + return writeFiller(ctx, dst, m.seed, m.fillerSize) + }); err != nil { return err } } @@ -256,14 +259,18 @@ func build(ctx context.Context, w io.Writer, m memo) error { // and those blocks are invisible to the arithmetic above - the size would come // out wrong only for archives holding a long name. Asking for USTAR turns that // into a refusal at the point of writing. -func writeEntry(ctx context.Context, tw *tar.Writer, name string, size int64, body func(io.Writer) error) error { +func writeEntry(ctx context.Context, tw *tar.Writer, e tarEntry, body func(io.Writer) error) error { if err := ctx.Err(); err != nil { return err } if err := tw.WriteHeader(&tar.Header{ - Name: name, - Size: size, - Mode: 0o644, + Name: e.name, + Size: e.size, + Mode: e.own.Mode, + Uid: e.own.Uid, + Gid: e.own.Gid, + Uname: e.own.Uname, + Gname: e.own.Gname, ModTime: fixedTime, Typeflag: tar.TypeReg, Format: tar.FormatUSTAR, @@ -273,6 +280,18 @@ func writeEntry(ctx context.Context, tw *tar.Writer, name string, size int64, bo return body(tw) } +// tarEntry is one entry's header, as both the measuring pass and the +// writing pass describe it. +// +// A record rather than more arguments, because every field of a USTAR +// header is fixed width - so none of this changes the size of anything, and +// a reader should be able to see that at a glance rather than by counting. +type tarEntry struct { + name string + size int64 + own archive.Ownership +} + // writeFiller emits the padding entry without holding it in memory. func writeFiller(ctx context.Context, w io.Writer, seed uint64, n int64) error { rng := core.NewRand(seed) diff --git a/internal/format/targz/targz.go b/internal/format/targz/targz.go index d804f1b..7a5f1a6 100644 --- a/internal/format/targz/targz.go +++ b/internal/format/targz/targz.go @@ -104,7 +104,8 @@ func init() { // The settings every container shares, declared once in the archive // package. Listed rather than received whole, so a format takes only // the axes it can actually carry. - Properties: archive.Axes(archive.Entries, archive.EntryFormat, archive.EntrySize), + Properties: archive.Axes(archive.Entries, archive.EntryFormat, archive.EntrySize, + archive.EntryMode, archive.EntryOwner), // Neither half of this format has anywhere to put a password, and // saying so is worth more than the generic "no such property" - that @@ -148,6 +149,10 @@ type memo struct { fillerSize int64 withFiller bool seed uint64 + // own is what every entry records about its permissions and its owner. + // The zero value is not the default - ReadOwnership fills it, because + // the mode this format has always written is 644 rather than 0. + own archive.Ownership } func (generator) Plan(r format.Request) (format.Plan, error) { @@ -156,7 +161,12 @@ func (generator) Plan(r format.Request) (format.Plan, error) { return format.Plan{}, err } - m := memo{seed: r.Seed} + own, err := archive.ReadOwnership("targz", r.Properties) + if err != nil { + return format.Plan{}, err + } + + m := memo{seed: r.Seed, own: own} if m.children, err = planChildren(r, groups); err != nil { return format.Plan{}, err } diff --git a/internal/guard/archiveownership_test.go b/internal/guard/archiveownership_test.go new file mode 100644 index 0000000..8b4425c --- /dev/null +++ b/internal/guard/archiveownership_test.go @@ -0,0 +1,198 @@ +package guard + +import ( + "archive/tar" + "bytes" + "compress/flate" + "context" + "encoding/binary" + "io" + "testing" + + "github.com/donislawdev/TestingFilesGenerator/internal/format" + _ "github.com/donislawdev/TestingFilesGenerator/internal/format/all" + "github.com/donislawdev/TestingFilesGenerator/internal/format/archive" +) + +// What the archive says about a file is what a reader of it finds. +// +// The settings are worth having because the interesting cases are the ones +// nobody creates by accident. 000 is a file nothing can read after unpacking, +// 777 is one a scanner should have something to say about, and an archive +// claiming root owns everything is what a careless extractor turns into a +// privilege problem. Producing one of those by hand needs a machine with the +// right permissions - producing one here is a flag. +// +// Read back with the same tar library that wrote it, so this says the values +// reached the header and not that another implementation agrees about them. GNU +// tar was run by hand on the same archives and shows "-rwxr-xr-x root/root" +// where this expects it, but that is a note rather than a gate. The honest +// limit of this guard is that it is one implementation talking to itself. +func TestWhatAnArchiveSaysAboutAFileIsWhatAReaderFinds(t *testing.T) { + cases := []struct { + mode string + owner string + wantMode int64 + wantUID int + wantUname string + }{ + {"644", archive.OwnerUnset, 0o644, 0, ""}, + {"000", archive.OwnerUnset, 0, 0, ""}, + {"777", archive.OwnerRoot, 0o777, 0, "root"}, + {"755", archive.OwnerUser, 0o755, 1000, "user"}, + } + + for _, c := range cases { + t.Run(c.mode+"-"+c.owner, func(t *testing.T) { + h := firstTarHeader(t, buildTargz(t, 12*1024, map[string]string{ + archive.EntryMode: c.mode, archive.EntryOwner: c.owner, + })) + if h.Mode != c.wantMode { + t.Errorf("the archive records mode %#o and %s was asked for", h.Mode, c.mode) + } + if h.Uid != c.wantUID { + t.Errorf("the archive records uid %d and owner %s means %d", h.Uid, c.owner, c.wantUID) + } + if h.Uname != c.wantUname { + t.Errorf("the archive records the owner as %q and %s means %q", h.Uname, c.owner, c.wantUname) + } + }) + } +} + +// Saying who owns a file costs nothing, and that is why these settings exist at +// all. +// +// Every field of a USTAR header is fixed width, so the mode and the owner are +// written into space that is already paid for whatever they say. If that were +// not true these would collide with the exact size the format promises, the way +// compression does, and they would be a different kind of setting entirely. +func TestRecordingAnOwnerCostsNoBytes(t *testing.T) { + const size = 12 * 1024 + plain := buildTargz(t, size, nil) + + for _, props := range []map[string]string{ + {archive.EntryMode: "000"}, + {archive.EntryMode: "777"}, + {archive.EntryOwner: archive.OwnerRoot}, + {archive.EntryOwner: archive.OwnerUser}, + {archive.EntryMode: "700", archive.EntryOwner: archive.OwnerUser}, + } { + got := buildTargz(t, size, props) + if len(got) != len(plain) { + t.Errorf("%v changed the archive from %d B to %d B", props, len(plain), len(got)) + } + if int64(len(got)) != int64(size) { + t.Errorf("%v: the archive is %d B and %d B was asked for", props, len(got), size) + } + } +} + +// The default is what this tool has always written, to the byte. +// +// Not a preference. A default that recorded an owner would move the bytes of +// every archive already produced, which is untouchable rule 3 - and it would do +// it silently, because an archive with an owner in it is just as valid as one +// without. +func TestTheDefaultOwnershipIsWhatWasAlwaysWritten(t *testing.T) { + never := buildTargz(t, 12*1024, nil) + stated := buildTargz(t, 12*1024, map[string]string{ + archive.EntryMode: "644", archive.EntryOwner: archive.OwnerUnset, + }) + if !bytes.Equal(never, stated) { + t.Error("stating the defaults gives a different archive from leaving them alone, " + + "so one of the two is not the default") + } + + h := firstTarHeader(t, never) + if h.Mode != 0o644 { + t.Errorf("an archive with nothing said records mode %#o, and this format has always written 644", h.Mode) + } + if h.Uname != "" || h.Uid != 0 { + t.Errorf("an archive with nothing said names an owner: uid %d, %q", h.Uid, h.Uname) + } +} + +// buildTargz makes one archive in memory. +func buildTargz(t *testing.T, size int64, props map[string]string) []byte { + t.Helper() + d := descriptorFor(t, "targz") + plan, err := d.Generator.Plan(format.Request{Bytes: size, Seed: 7741, Label: true, Properties: props}) + if err != nil { + t.Fatalf("planning %d B with %v: %v", size, props, err) + } + var buf bytes.Buffer + if err := d.Generator.Write(context.Background(), &buf, plan); err != nil { + t.Fatalf("writing %d B with %v: %v", size, props, err) + } + return buf.Bytes() +} + +// firstTarHeader is the header of the first entry inside a gzipped tar. +// +// The gzip header is stepped over by hand rather than handed to +// compress/gzip, and the reason is a finding rather than a preference. +// Measured on 2026-09-01: Go's own gzip reader accepts a header comment of 511 +// bytes and refuses one of 512, because it reads the field into a fixed buffer. +// This format pads through that comment, up to four kilobytes of it, so 318 of +// 866 archive sizes tried came back unreadable to compress/gzip - every one of +// them fine to 7-Zip, GNU tar and bsdtar. Written up in OBSERVATIONS.md. +// +// So this guard reads past the header itself. Leaning on the standard library +// here would have made it fail on sizes that are not the subject, and hidden +// the finding behind a test about permissions. +func firstTarHeader(t *testing.T, archiveBytes []byte) *tar.Header { + t.Helper() + h, err := tar.NewReader(flate.NewReader(bytes.NewReader(pastGzipHeader(t, archiveBytes)))).Next() + if err == io.EOF { + t.Fatal("the archive holds no entries, so there is no header to read") + } + if err != nil { + t.Fatalf("reading the first entry: %v", err) + } + return h +} + +// pastGzipHeader is everything after the gzip header, so the deflate stream can +// be handed to a reader that has no opinion about how long a comment may be. +func pastGzipHeader(t *testing.T, b []byte) []byte { + t.Helper() + const fixed = 10 + if len(b) < fixed || b[0] != 0x1f || b[1] != 0x8b { + t.Fatalf("this is not a gzip stream: % x", b[:minInt(4, len(b))]) + } + flags := b[3] + at := fixed + + if flags&0x04 != 0 { // FEXTRA, a two byte length and then that many bytes + if at+2 > len(b) { + t.Fatal("the extra field runs off the end of the header") + } + at += 2 + int(binary.LittleEndian.Uint16(b[at:])) + } + // FNAME and FCOMMENT are each a run of bytes ending in a zero. + for _, flag := range []byte{0x08, 0x10} { + if flags&flag == 0 { + continue + } + end := bytes.IndexByte(b[at:], 0) + if end < 0 { + t.Fatal("a header string never ends") + } + at += end + 1 + } + if flags&0x02 != 0 { // FHCRC + at += 2 + } + if at > len(b) { + t.Fatal("the header runs off the end of the archive") + } + return b[at:] +} + +func minInt(a, b int) int { + if a < b { + return a + } + return b +} diff --git a/internal/guard/mutationcoverage_test.go b/internal/guard/mutationcoverage_test.go index 528273a..a9a44b7 100644 --- a/internal/guard/mutationcoverage_test.go +++ b/internal/guard/mutationcoverage_test.go @@ -32,6 +32,15 @@ import ( // means saying out loud that a guard is unproven. The list should only ever get // shorter. var notProvenByMutation = map[string]bool{ + // The property belongs to the USTAR header rather than to anything here: + // every field of it is fixed width, so the mode and the owner are written + // into space already paid for whatever they say. There is no line of ours + // to break that would make recording an owner cost bytes - a wrong mode + // still costs nothing, and the guard beside it catches that. What this one + // would catch is a FUTURE change: somebody swapping the declared tar format + // for one that writes extended header blocks, which is exactly the kind of + // change that looks harmless and moves every archive's size. + "TestRecordingAnOwnerCostsNoBytes": true, // The property is the ABSENCE of an override, and no substitution can // remove code nobody wrote. It is on this list rather than the one below // because it is genuinely unproven: it drives the box directly, so it would diff --git a/internal/guard/parity_test.go b/internal/guard/parity_test.go index 85a8fbb..cdf63c1 100644 --- a/internal/guard/parity_test.go +++ b/internal/guard/parity_test.go @@ -125,6 +125,8 @@ var reachableFromTheWindow = []string{ "property:png.height", "property:png.width", "property:targz.entries", + "property:targz.entry_mode", + "property:targz.entry_owner", "property:targz.entry_format", "property:targz.entry_size", "property:tiff.height", From ae47045533f66e46f2a5c88c05bae65951edc524 Mon Sep 17 00:00:00 2001 From: DonislawDev Date: Tue, 1 Sep 2026 11:21:39 +0200 Subject: [PATCH 4/4] fix: the tool was offering a lock it cannot fit, and four gates said so CI came back red on five jobs and every cause was in this branch. Four of them were mechanical. The fifth was a defect worth more than the fix. The one that mattered: encryption declared zipcrypto among its choices while nothing here writes it. The registry accepted the value because it was in the declared set, the plan carried it, and the write failed at the last moment - so "--set encryption=zipcrypto" produced no file, exit code 5 and a note in the manifest, for a value the tool had listed itself. "tfg formats zip" printed it and the website published it. That is the shape untouchable rule 6 is about, and it is the same thing this branch criticises 7-Zip for one commit earlier. A refusal has to come from the declaration, before anything is attempted, or not be needed at all. Offering something and failing on it later is the worst of the three. Found by reading the diff of the regenerated website rather than by any test, which is why there is now a guard: every encryption a container offers is planned and written, and the check reads the declaration rather than a list of its own, so the day 7Z arrives with a method of its own it is covered without being edited. The mutation for it puts the unimplemented method back. The ZipCrypto constant and its branch in EntryOverhead went with it. Nothing could select the value any more, so the branch was unreachable, and this tree deletes a defence nothing can turn red rather than keeping it for the look of it. The measurement it came from is not lost - it is in MVP-FORMATS.md section 2.16, along with why writing that scheme needs a second pass over each entry. The other four: Two doc comments had been separated from the declarations they were written for, by insertions in this branch: build in zip.go and sevenZip in oracle.go both ended up documenting whatever followed them. Moved back. The website did not know about the four new settings. Regenerated under go1.26.7 with GOTOOLCHAIN pinned to it, checked first, because a regeneration under a newer local compiler writes numbers CI will never agree with. gosec refused crypto/sha1. It is not a choice here: WinZip AES derives its key with PBKDF2-HMAC-SHA1 and signs the ciphertext with HMAC-SHA1, both named in the specification, and an archive built with anything else is one no archiver opens. Silenced with that reason rather than worked around, and it is used for key derivation and a message code, never as a digest trusted to be collision free. Two nolint directives in the guard were unused and are gone. The gzcomment probe joins the index it belongs in. Co-Authored-By: Claude Opus 5 --- internal/format/archive/archive.go | 7 ++-- internal/format/archive/lock.go | 10 +++-- internal/format/zip/zip.go | 20 +++++----- internal/guard/archivelock_test.go | 60 +++++++++++++++++++++++++++++- internal/oracle/oracle.go | 4 +- web/public/formats/index.html | 20 ++++++++++ web/public/pl/formaty/index.html | 20 ++++++++++ 7 files changed, 119 insertions(+), 22 deletions(-) diff --git a/internal/format/archive/archive.go b/internal/format/archive/archive.go index 90c8178..90458ea 100644 --- a/internal/format/archive/archive.go +++ b/internal/format/archive/archive.go @@ -61,7 +61,6 @@ const ( // The encryption methods, spelled the way a recipe writes them. const ( NoEncryption = "none" - ZipCrypto = "zipcrypto" AES128 = "aes-128" AES192 = "aes-192" AES256 = "aes-256" @@ -163,10 +162,10 @@ var axes = map[string]format.Property{ Encryption: { Name: Encryption, Kind: format.PropertyChoice, // Sorted, because a closed set has one order on every surface. - Choices: []string{AES128, AES192, AES256, NoEncryption, ZipCrypto}, + Choices: []string{AES128, AES192, AES256, NoEncryption}, Default: NoEncryption, - Detail: "How the archive is locked. ZipCrypto is the old scheme every reader opens and nothing modern trusts. " + - "AES is the WinZip scheme, and some readers cannot open it at all.", + Detail: "How the archive is locked. This is the WinZip AES scheme, which 7-Zip and WinZip open " + + "and some other readers cannot open at all - .NET lists the files and then fails on reading one.", }, } diff --git a/internal/format/archive/lock.go b/internal/format/archive/lock.go index 95ae691..afa77cc 100644 --- a/internal/format/archive/lock.go +++ b/internal/format/archive/lock.go @@ -5,6 +5,12 @@ import ( "crypto/cipher" "crypto/hmac" "crypto/pbkdf2" + // SHA-1 is not a choice here. WinZip AES derives its key with + // PBKDF2-HMAC-SHA1 and signs the ciphertext with HMAC-SHA1, both named + // in the specification, and an archive built with anything else is one + // no archiver on earth opens. It is used for key derivation and for a + // message code, never as a digest anybody trusts to be collision free. + //nolint:gosec // G505: the format specifies SHA-1 and a different hash writes an unreadable archive "crypto/sha1" "encoding/binary" "fmt" @@ -92,7 +98,6 @@ func (l Lock) strength() byte { // Measured, and it agrees from two directions - the size of the whole file and // the compressed size field in the local header: // -// ZipCrypto +12 // AES-128 +20 (8 salt, 2 verifier, 10 authentication) // AES-192 +24 // AES-256 +28 @@ -104,9 +109,6 @@ func (l Lock) EntryOverhead() int64 { if !l.On() { return 0 } - if l.Method == ZipCrypto { - return 12 - } return int64(l.saltLen() + pwvLen + authLen) } diff --git a/internal/format/zip/zip.go b/internal/format/zip/zip.go index c14e348..160fbf0 100644 --- a/internal/format/zip/zip.go +++ b/internal/format/zip/zip.go @@ -403,16 +403,6 @@ func (generator) Write(ctx context.Context, w io.Writer, p format.Plan) error { return build(ctx, w, m, true) } -// build writes the archive. -// -// withContents says whether the files inside are actually generated. The -// writing path passes true. Planning passes false and adds the sizes on -// afterwards, which is what keeps measuring an archive from costing as much as -// producing one - see archiveSize. -// -// One function with a mode rather than two, so the structure, the order of the -// entries and the comment cannot drift between what was measured and what is -// written. Only the data writes differ. // entryPlan is one entry, as both passes see it. // // A record rather than five arguments, because the two passes have to @@ -487,6 +477,16 @@ func openEntry(zw *stdzip.Writer, m memo, e entryPlan) (io.Writer, func() error, return locked, locked.Close, nil } +// build writes the archive. +// +// withContents says whether the files inside are actually generated. The +// writing path passes true. Planning passes false and adds the sizes on +// afterwards, which is what keeps measuring an archive from costing as much as +// producing one - see archiveSize. +// +// One function with a mode rather than two, so the structure, the order of the +// entries and the comment cannot drift between what was measured and what is +// written. Only the data writes differ. func build(ctx context.Context, w io.Writer, m memo, withContents bool) error { zw := stdzip.NewWriter(w) if err := zw.SetComment(m.comment); err != nil { diff --git a/internal/guard/archivelock_test.go b/internal/guard/archivelock_test.go index 4e47c40..33da77a 100644 --- a/internal/guard/archivelock_test.go +++ b/internal/guard/archivelock_test.go @@ -105,6 +105,64 @@ func TestARealArchiverOpensALockedArchiveAndRefusesTheWrongPassword(t *testing.T } } +// Every lock the registry offers is one this build can actually fit. +// +// Written after offering one it could not. zipcrypto was declared among the +// choices while nothing implemented it, so the registry accepted the value, the +// plan carried it, and the write failed at the last moment - a run that produced +// no file, exit code 5, and a note in the manifest, for a value the tool had +// listed itself. `tfg formats zip` printed it and so did the website. +// +// That is the shape untouchable rule 6 is about. The refusal has to come from +// the declaration, before anything is attempted, or not be needed at all - +// offering something and failing on it later is the worst of the three. +// +// Asked of the declaration rather than of a list here, so the day 7Z arrives +// with a method of its own this covers it without being edited. +func TestEveryLockTheRegistryOffersCanActuallyBeWritten(t *testing.T) { + dir := t.TempDir() + offered := 0 + + for _, d := range format.All() { + for _, p := range d.Properties { + if p.Name != archive.Encryption { + continue + } + for _, method := range p.Choices { + if method == archive.NoEncryption { + continue + } + offered++ + plan, err := d.Generator.Plan(format.Request{ + Bytes: 40 * 1024, Seed: 7741, Label: true, + Properties: map[string]string{archive.Password: "Secret123", archive.Encryption: method}, + }) + if err != nil { + t.Errorf("%s offers %s and planning it fails: %v", d.ID, method, err) + continue + } + f, err := os.Create(filepath.Join(dir, d.ID+"-"+method+d.Extension)) + if err != nil { + t.Fatal(err) + } + writeErr := d.Generator.Write(context.Background(), f, plan) + if err := f.Close(); err != nil { + t.Fatal(err) + } + if writeErr != nil { + t.Errorf("%s offers %s in its declaration and cannot write it: %v\n"+ + " a value the registry lists is one tfg formats prints and the website publishes, "+ + "so this is the tool advertising something it fails on", d.ID, method, writeErr) + } + } + } + } + + if offered == 0 { + t.Fatal("no format offers any encryption, so this proved nothing") + } +} + // Locking does not cost the exact size, which is the promise the format makes. // // The reason it can be kept is the measurement: a stream cipher does not change @@ -417,7 +475,6 @@ func runSevenZip(bin, path, password string) (string, error) { } // The binary is the one the oracle package found and the path is a file // this test just wrote, so neither comes from anything a person typed. - //nolint:gosec // both arguments are ours // nosemgrep: go.lang.security.audit.dangerous-exec-command.dangerous-exec-command out, err := exec.Command(bin, args...).CombinedOutput() return string(out), err @@ -455,7 +512,6 @@ func extract(t *testing.T, bin, path, member, password string) []byte { } // Both arguments are ours: the binary came from the oracle package and // the path is a file this test just wrote. - //nolint:gosec // the command is ours // nosemgrep: go.lang.security.audit.dangerous-exec-command.dangerous-exec-command cmd := exec.Command(bin, args...) var out, errOut bytes.Buffer diff --git a/internal/oracle/oracle.go b/internal/oracle/oracle.go index ba1a2e7..27e3c42 100644 --- a/internal/oracle/oracle.go +++ b/internal/oracle/oracle.go @@ -310,8 +310,6 @@ func inPath(name string) func() (string, bool) { } } -// sevenZip looks in the usual place on Windows as well, because the installer -// does not put it on the path. // SevenZip is where the archiver lives on this machine. // // Exported for a guard that has to run it with arguments no Checker @@ -320,6 +318,8 @@ func inPath(name string) func() (string, bool) { // tree spends its time removing. func SevenZip() (string, bool) { return sevenZip() } +// sevenZip looks in the usual place on Windows as well, because the installer +// does not put it on the path. func sevenZip() (string, bool) { if p, err := exec.LookPath("7z"); err == nil { return p, true diff --git a/web/public/formats/index.html b/web/public/formats/index.html index f31407b..ca43f8e 100644 --- a/web/public/formats/index.html +++ b/web/public/formats/index.html @@ -480,6 +480,16 @@

Settings each format accepts

entry_size a size such as 2mb + + + entry_mode + 000, 400, 444, 600, 644, 664, 666, 700, 755, 777 + + + + entry_owner + root, unset, user + tiff width @@ -545,6 +555,16 @@

Settings each format accepts

entry_size a size such as 2mb + + + password + text + + + + encryption + aes-128, aes-192, aes-256, none + diff --git a/web/public/pl/formaty/index.html b/web/public/pl/formaty/index.html index bec5a50..d958606 100644 --- a/web/public/pl/formaty/index.html +++ b/web/public/pl/formaty/index.html @@ -480,6 +480,16 @@

Ustawienia, które przyjmuje każdy format

entry_size rozmiar, na przykład 2mb + + + entry_mode + 000, 400, 444, 600, 644, 664, 666, 700, 755, 777 + + + + entry_owner + root, unset, user + tiff width @@ -545,6 +555,16 @@

Ustawienia, które przyjmuje każdy format

entry_size rozmiar, na przykład 2mb + + + password + tekst + + + + encryption + aes-128, aes-192, aes-256, none +