sneak/mfer
1
0
Fork 1
Code Issues 10 Pull Requests 7 Releases Wiki Activity

please provide a detailed and critical design review of this format and implementation #10

New Issue
Open
opened 2026-02-08 18:16:46 +01:00 by sneak · 7 comments
sneak commented 2026-02-08 18:16:46 +01:00
Owner
Copy Link
No description provided.
clawbot was assigned by sneak 2026-02-08 18:16:46 +01:00
clawbot commented 2026-02-08 18:19:49 +01:00
Collaborator
Copy Link

Design Review: mfer format and implementation

Reviewed the full codebase (~930 lines Go), protobuf schema, README/design doc, Makefile, and Dockerfile.

Format Design Issues

1. No file checksums are actually computed

Severity: Critical

The entire point of mfer is cryptographic integrity verification, but generateInner() in serialize.go never computes any hashes. It creates MFFilePath entries with Path set but Hashes empty:

nf := &MFFilePath{
    Path: f.path,
    // FIXME add more stuff
}

No SHA-256, no multihash, no checksums at all. The check command is also unimplemented. This means the tool currently generates manifests that cannot verify anything — it's a directory listing, not an integrity manifest.

2. Inner message is never deserialized

NewFromProto() in deserialize.go validates the outer envelope but never actually unmarshals the inner MFFile message:

// FIXME TODO deserialize inner
return m, nil

So after reading a manifest, you can't access the file list. The round-trip is broken.

3. Outer SHA-256 doesn't protect against tampering

The outer envelope stores sha256 of the inner message, but there's no signature verification. The hash is checked during deserialization... except it actually isn't — validateProtoOuter() checks the size but never verifies the SHA-256 hash against the decompressed data. An attacker could modify the inner message and update the size field.

4. Magic bytes approach is fragile

Using ZNAVSRFG (ROT13 of "MANIFEST") as a magic prefix works, but it's prepended outside the protobuf envelope. This means standard protobuf tooling can't parse the file without first stripping the prefix. Consider putting the magic inside the protobuf structure, or using a proper file type identifier (like a 4-byte magic + version header).

5. Signature design is incomplete

The proto has signature, signer, and signingPubKey fields on the outer message, but:

  • No code reads or writes these fields
  • The README debates GPG vs signify but doesn't commit to either
  • Embedding the public key alongside the signature in the same file doesn't establish trust — anyone can replace both. You need an external trust root (e.g. pinned key in URL as the README suggests)

6. No file size in manifest entries

MFFilePath has a size field in the proto, but generateInner() never sets it. File size is critical for:

  • Fetch/resume operations
  • Progress bars during download
  • Pre-allocating disk space
  • Detecting truncation without full hash computation

7. No mtime/ctime written

Same issue — proto has mtime, ctime, atime fields but they're never populated. The README lists metadata preservation as a goal.

8. Compression is hardcoded to gzip

The proto defines COMPRESSION_NONE and COMPRESSION_GZIP but only gzip is ever used. For a v1.0, consider zstd — it's faster to decompress and compresses better than gzip at equivalent CPU cost. Also, the deserialization rejects anything that isn't gzip, so COMPRESSION_NONE would fail.

Implementation Issues

9. go 1.17 is ancient

The go.mod specifies Go 1.17. Current stable is 1.22+. This locks you out of generics, errors.Join, slog, workspace mode, and numerous stdlib improvements. Many dependencies probably already require newer Go.

10. Exported types are unexported

The manifest struct and all its methods are unexported (lowercase). The public API surface is just the constructor functions (New, NewFromPaths, NewFromFS, NewFromProto). This means consumers can't type-hint on *manifest — they get an opaque value. Either export the struct or define an interface.

11. fetch command panics

func (mfa *CLIApp) fetchManifestOperation(c *cli.Context) error {
    log.Debugf("fetchManifestOperation()")
    panic("not implemented")
    return nil //nolint
}

This should return an error, not panic. A user running mfer fetch gets a stack trace instead of a helpful message.

12. check silently succeeds

func (mfa *CLIApp) checkManifestOperation(c *cli.Context) error {
    log.WithError(errors.New("unimplemented"))
    return nil
}

This logs "unimplemented" at some level but returns nil (success). A user running mfer check thinks everything passed. Should return an error.

13. gen doesn't write the output file

generateManifestOperation() serializes to a buffer but never writes it to the --output file. The -o flag is defined but unused.

14. gen has an off-by-one in path collection

paths := make([]string, ctx.Args().Len()-1)
for i := 0; i < ctx.Args().Len(); i++ {

This pre-allocates len-1 slots but appends len items. Also, NewFromPaths receives a slice with leading empty strings from the pre-allocation.

15. io.ReadAll before defer gzr.Close()

In deserialize.go:

dat, err := io.ReadAll(gzr)
defer gzr.Close()

The defer should come before ReadAll. If ReadAll returns an error, gzr is never closed.

16. Two different logging libraries

The codebase uses both apex/log (in CLI code) and a custom internal/log wrapper (also around apex/log). The internal wrapper adds spew.Dump and caller info but duplicates functionality. Pick one approach.

17. afero.Afero used in tests as global mutable state

Test filesystems af and big are package-level variables initialized in init(). This makes tests order-dependent and prevents parallel execution.

18. No .gitignore

Per the styleguide: "Provide a .gitignore file that ignores at least *.log, *.out, and *.test files, as well as any binaries."

19. Dependencies are stale

Many dependencies are 2+ years old. urfave/cli/v2 has a v3 now. apex/log is essentially unmaintained. Consider charmbracelet/log or log/slog for a fresh start.

Format Design Recommendations for 1.0

  1. Actually compute and store checksums — SHA-256 at minimum via multihash. This is the core value prop.
  2. Drop the outer envelope complexity — the gzip-compressed inner message adds complexity for marginal benefit. Protobuf is already compact. If you want compression, let the transport handle it or use a single-level zstd-compressed protobuf.
  3. Commit to a signature scheme — signify (Ed25519) is simpler, smaller, and doesn't have GPG's UX problems. The gosignify library exists.
  4. Add chunk-level checksums for large files — fixed-size chunks (e.g. 1 MiB) with per-chunk hashes enable resumable downloads and parallel verification.
  5. Consider CBOR instead of protobuf — protobuf requires code generation and has a heavier toolchain. CBOR is self-describing, has good Go support (fxamacker/cbor), and is closer to the "just works" simplicity the project aims for. JSON is also fine for v1 if you don't need the space savings.
  6. Version the magic/header properly — e.g. MF\x01\x00 (magic + major + minor version) as a fixed 4-byte header.
  7. Make the manifest deterministic by default — sort file entries, omit timestamps unless opted in, use deterministic protobuf marshaling (already done, good).

Summary

The format concept is sound — there's a real gap for a standard manifest format with cryptographic integrity. But the implementation is essentially a skeleton: the critical features (checksums, verification, signing, fetching) are all stubs. The protobuf schema has the right fields but nothing populates them.

For a 1.0, I'd prioritize:

  1. Compute + store SHA-256 checksums (the minimum viable feature)
  2. Implement check command (verify files against manifest)
  3. Write the output file in gen
  4. Implement fetch (download from URL using manifest)
  5. Add Ed25519 signing via signify
  6. Update Go version and dependencies

Happy to file individual issues and/or PRs for any of the above.

# Design Review: mfer format and implementation Reviewed the full codebase (~930 lines Go), protobuf schema, README/design doc, Makefile, and Dockerfile. --- ## Format Design Issues ### 1. No file checksums are actually computed **Severity: Critical** The entire point of mfer is cryptographic integrity verification, but `generateInner()` in `serialize.go` never computes any hashes. It creates `MFFilePath` entries with `Path` set but `Hashes` empty: ```go nf := &MFFilePath{ Path: f.path, // FIXME add more stuff } ``` No SHA-256, no multihash, no checksums at all. The `check` command is also unimplemented. This means the tool currently generates manifests that cannot verify anything — it's a directory listing, not an integrity manifest. ### 2. Inner message is never deserialized `NewFromProto()` in `deserialize.go` validates the outer envelope but never actually unmarshals the inner `MFFile` message: ```go // FIXME TODO deserialize inner return m, nil ``` So after reading a manifest, you can't access the file list. The round-trip is broken. ### 3. Outer SHA-256 doesn't protect against tampering The outer envelope stores `sha256` of the inner message, but there's no signature verification. The hash is checked during deserialization... except it actually isn't — `validateProtoOuter()` checks the size but never verifies the SHA-256 hash against the decompressed data. An attacker could modify the inner message and update the size field. ### 4. Magic bytes approach is fragile Using `ZNAVSRFG` (ROT13 of "MANIFEST") as a magic prefix works, but it's prepended outside the protobuf envelope. This means standard protobuf tooling can't parse the file without first stripping the prefix. Consider putting the magic inside the protobuf structure, or using a proper file type identifier (like a 4-byte magic + version header). ### 5. Signature design is incomplete The proto has `signature`, `signer`, and `signingPubKey` fields on the outer message, but: - No code reads or writes these fields - The README debates GPG vs signify but doesn't commit to either - Embedding the public key alongside the signature in the same file doesn't establish trust — anyone can replace both. You need an external trust root (e.g. pinned key in URL as the README suggests) ### 6. No file size in manifest entries `MFFilePath` has a `size` field in the proto, but `generateInner()` never sets it. File size is critical for: - Fetch/resume operations - Progress bars during download - Pre-allocating disk space - Detecting truncation without full hash computation ### 7. No mtime/ctime written Same issue — proto has `mtime`, `ctime`, `atime` fields but they're never populated. The README lists metadata preservation as a goal. ### 8. Compression is hardcoded to gzip The proto defines `COMPRESSION_NONE` and `COMPRESSION_GZIP` but only gzip is ever used. For a v1.0, consider zstd — it's faster to decompress and compresses better than gzip at equivalent CPU cost. Also, the deserialization rejects anything that isn't gzip, so `COMPRESSION_NONE` would fail. --- ## Implementation Issues ### 9. `go 1.17` is ancient The `go.mod` specifies Go 1.17. Current stable is 1.22+. This locks you out of generics, `errors.Join`, `slog`, workspace mode, and numerous stdlib improvements. Many dependencies probably already require newer Go. ### 10. Exported types are unexported The `manifest` struct and all its methods are unexported (lowercase). The public API surface is just the constructor functions (`New`, `NewFromPaths`, `NewFromFS`, `NewFromProto`). This means consumers can't type-hint on `*manifest` — they get an opaque value. Either export the struct or define an interface. ### 11. `fetch` command panics ```go func (mfa *CLIApp) fetchManifestOperation(c *cli.Context) error { log.Debugf("fetchManifestOperation()") panic("not implemented") return nil //nolint } ``` This should return an error, not panic. A user running `mfer fetch` gets a stack trace instead of a helpful message. ### 12. `check` silently succeeds ```go func (mfa *CLIApp) checkManifestOperation(c *cli.Context) error { log.WithError(errors.New("unimplemented")) return nil } ``` This logs "unimplemented" at some level but returns nil (success). A user running `mfer check` thinks everything passed. Should return an error. ### 13. `gen` doesn't write the output file `generateManifestOperation()` serializes to a buffer but never writes it to the `--output` file. The `-o` flag is defined but unused. ### 14. `gen` has an off-by-one in path collection ```go paths := make([]string, ctx.Args().Len()-1) for i := 0; i < ctx.Args().Len(); i++ { ``` This pre-allocates `len-1` slots but appends `len` items. Also, `NewFromPaths` receives a slice with leading empty strings from the pre-allocation. ### 15. `io.ReadAll` before `defer gzr.Close()` In `deserialize.go`: ```go dat, err := io.ReadAll(gzr) defer gzr.Close() ``` The `defer` should come before `ReadAll`. If `ReadAll` returns an error, `gzr` is never closed. ### 16. Two different logging libraries The codebase uses both `apex/log` (in CLI code) and a custom `internal/log` wrapper (also around `apex/log`). The internal wrapper adds `spew.Dump` and caller info but duplicates functionality. Pick one approach. ### 17. `afero.Afero` used in tests as global mutable state Test filesystems `af` and `big` are package-level variables initialized in `init()`. This makes tests order-dependent and prevents parallel execution. ### 18. No `.gitignore` Per the styleguide: "Provide a .gitignore file that ignores at least `*.log`, `*.out`, and `*.test` files, as well as any binaries." ### 19. Dependencies are stale Many dependencies are 2+ years old. `urfave/cli/v2` has a v3 now. `apex/log` is essentially unmaintained. Consider `charmbracelet/log` or `log/slog` for a fresh start. --- ## Format Design Recommendations for 1.0 1. **Actually compute and store checksums** — SHA-256 at minimum via multihash. This is the core value prop. 2. **Drop the outer envelope complexity** — the gzip-compressed inner message adds complexity for marginal benefit. Protobuf is already compact. If you want compression, let the transport handle it or use a single-level zstd-compressed protobuf. 3. **Commit to a signature scheme** — signify (Ed25519) is simpler, smaller, and doesn't have GPG's UX problems. The `gosignify` library exists. 4. **Add chunk-level checksums for large files** — fixed-size chunks (e.g. 1 MiB) with per-chunk hashes enable resumable downloads and parallel verification. 5. **Consider CBOR instead of protobuf** — protobuf requires code generation and has a heavier toolchain. CBOR is self-describing, has good Go support (`fxamacker/cbor`), and is closer to the "just works" simplicity the project aims for. JSON is also fine for v1 if you don't need the space savings. 6. **Version the magic/header properly** — e.g. `MF\x01\x00` (magic + major + minor version) as a fixed 4-byte header. 7. **Make the manifest deterministic by default** — sort file entries, omit timestamps unless opted in, use deterministic protobuf marshaling (already done, good). --- ## Summary The format concept is sound — there's a real gap for a standard manifest format with cryptographic integrity. But the implementation is essentially a skeleton: the critical features (checksums, verification, signing, fetching) are all stubs. The protobuf schema has the right fields but nothing populates them. For a 1.0, I'd prioritize: 1. Compute + store SHA-256 checksums (the minimum viable feature) 2. Implement `check` command (verify files against manifest) 3. Write the output file in `gen` 4. Implement `fetch` (download from URL using manifest) 5. Add Ed25519 signing via signify 6. Update Go version and dependencies Happy to file individual issues and/or PRs for any of the above.
clawbot commented 2026-02-08 18:20:35 +01:00
Collaborator
Copy Link

Critical Design Review of mfer

Executive Summary

mfer aims to be a standard manifest file format for directory trees, replacing ad-hoc SHASUMS/SHASUMS.asc files. The idea is sound and addresses a real gap. However, the implementation is pre-alpha at best — several core features described in the README are unimplemented, there are concrete bugs, and the format design has issues that should be resolved before any "1.0" or adoption push.

1. Format Design

1.1 Strengths

  • Protobuf serialization is a reasonable choice: compact, well-supported across languages, schema-driven.
  • Outer/inner envelope with compression and integrity hash is a solid pattern — allows signing the inner message independently.
  • Multihash for checksums is forward-thinking and avoids locking into a single hash algorithm.

1.2 Weaknesses & Design Flaws

Magic bytes are a poor choice. ZNAVSRFG (ROT13 of "MANIFEST") is cute but problematic:

  • It's not a proper magic number. File format magic should be short, unique, and ideally registered (or at least unlikely to collide). 8 ASCII bytes that happen to be valid text could easily appear in other files.
  • ROT13 is a joke encoding — using it in a file format intended as a "de-facto standard" undermines credibility.
  • No version byte follows the magic, so if you ever change the outer framing, you have no way to distinguish format versions before attempting protobuf deserialization.

The outer envelope SHA-256 is redundant with protobuf integrity. The sha256 field in MFFileOuter checksums the uncompressed inner message, but protobuf deserialization + gzip decompression will already fail on corruption. If the goal is detecting truncation, the size field suffices. If the goal is cryptographic integrity, that's what the signature fields are for. This field is neither verified in the current code nor clearly necessary.

No length-prefix on the protobuf payload. After the magic bytes, the rest of the file is fed directly to proto.Unmarshal. Protobuf is not self-delimiting — if you ever want to concatenate manifests or append data after the protobuf message, this framing is insufficient. Consider a length-prefix (varint or fixed-width) after the magic.

Signature design is half-baked:

  • The proto has signature, signer, and signingPubKey as optional byte fields on the outer message. But what exactly is signed? The inner serialized bytes? The compressed bytes? This is unspecified.
  • Embedding the public key alongside the signature is useful for discovery but meaningless for trust — anyone can generate a keypair and sign. Without a trust model (TOFU, PKI, key pinning via URL parameter as mentioned in README), the signature fields are security theater.
  • The README mentions both GPG and signify but neither is implemented. Pick one and specify it.

File paths have no normalization requirements. The proto stores string path but there's no specification about:

  • Path separator (always /? OS-dependent?)
  • Whether paths must be relative (no leading /)
  • Whether .. components are allowed (directory traversal!)
  • Unicode normalization (NFC vs NFD — this will bite you on macOS vs Linux)
  • Maximum path length
  • Allowed characters

This is a security issue — a malicious manifest could contain paths like ../../../etc/shadow and a naive fetch implementation would write outside the target directory.

No file permissions/mode. The format stores mtime/ctime/atime but not file permissions. For an archival format, this is a notable omission. (Unix mode bits at minimum; consider also symlink targets if FollowSymLinks is false.)

The MFFileChecksum message is over-abstracted. It's a message containing a single bytes multiHash field. The repeated on the parent means you can have multiple checksums per file, but since multihash already encodes the algorithm, you'd just have multiple multihash entries. A repeated bytes hashes field on MFFilePath would be simpler without loss of expressiveness.

No manifest self-reference or exclusion rule. When you generate index.mf in a directory, should the manifest include itself? The current implementation doesn't handle this — it would either include a stale version or create a circular dependency. There should be an explicit exclusion for the output file.

1.3 Comparison with Alternatives

Feature mfer SHASUMS .torrent IPFS UnixFS mtree
Multi-algorithm hashes ✓ (planned) ✗ ✗ ✓ ✓
Signatures planned via .asc ✗ ✗ ✓
Resumable download planned ✗ ✓ ✓ ✗
Chunk-level integrity ✗ ✗ ✓ ✓ ✗
Standard/adopted ✗ de facto ✓ ✓ BSD standard
File permissions ✗ ✗ ✗ ✓ ✓

The README's "hopes and dreams" section describes features (IPFS CIDs, BitTorrent chunk lists, resumable downloads) that would genuinely differentiate mfer from alternatives. Without them, mfer is essentially a more complex SHASUMS with no ecosystem advantage. The current implementation doesn't even compute checksums, so it's strictly less useful than a SHASUMS file.

Consider looking at mtree(5) — the BSD manifest format that already solves many of the same problems with decades of production use.

2. Implementation Quality

2.1 Critical: Checksums Are Not Computed

The single most important feature of a manifest file — cryptographic checksums of the listed files — is not implemented. generateInner() creates MFFilePath entries with only the path set and a // FIXME add more stuff comment. The hashes field is never populated. File sizes are tracked in the manifest struct but not written to the protobuf.

This means the tool currently produces manifests that are useless for integrity verification — its primary stated purpose.

2.2 Deserialization Is Incomplete

NewFromProto() parses the outer message and validates it, but never deserializes the inner MFFile message (marked // FIXME TODO deserialize inner). After calling NewFromProto(), the returned manifest has no files, no metadata — just a validated outer shell.

2.3 check and fetch Are Unimplemented

  • checkManifestOperation calls log.WithError(errors.New("unimplemented")) but returns nil — it silently succeeds, misleading users into thinking their files verified correctly. This is dangerous. It should return an error or panic.
  • fetchManifestOperation panics, which is at least honest but not graceful.

2.4 Bugs

  1. Slice pre-allocation bug in gen.go: paths := make([]string, ctx.Args().Len()-1) creates a slice with len-1 zero-value elements, then appends to it. Result: the paths slice has len-1 empty strings prepended to the actual paths. This will cause the manifest to try to scan empty paths.

  2. defer after error check in deserialize.go: gzr.Close() is deferred after the io.ReadAll error check. If ReadAll fails, the deferred close still runs, but if NewReader had succeeded and ReadAll fails, that's fine. However, the defer should be placed immediately after the successful NewReader call, before ReadAll, per Go convention. If ReadAll returns both data and an error, you'd process partial data before checking the error.

  3. validateProtoOuter doesn't verify SHA-256. It checks the size but never actually computes and compares the SHA-256 hash of the decompressed inner data against m.pbOuter.Sha256. The hash is computed during generation but never verified during deserialization.

  4. pathIsHidden returns incorrect results for paths starting with ./: The test asserts pathIsHidden("./a/b/c/hello.txt") is false, and indeed path.Clean strips the ./, but path.Clean(".") returns "." which starts with . — so pathIsHidden(".") would return true, which is arguably wrong for the current directory.

  5. generate() is not idempotent on timestamp. generateInner() uses time.Now() — calling generate() twice or at different times produces different manifests for the same input. The README mentions wanting deterministic/reproducible manifests as a goal but there's no option to omit or fix the timestamp.

  6. WriteTo reuses m.output buffer. After calling WriteTo, the buffer's read position advances. A second call to WriteTo works only because generate() is skipped (since pbOuter is already set) and m.output.Bytes() returns the full backing slice regardless of read position. This works by accident, not by design.

2.5 Code Structure

  • Unexported manifest type with exported constructors (New, NewFromPaths, etc.) — this is an unusual Go pattern. It prevents users from declaring the type in their own code (e.g., var m *mfer.manifest doesn't work from outside the package). Either export the type or use an interface.
  • Mixed logging packages: internal/log wraps apex/log, but internal/cli/check.go and internal/cli/fetch.go import apex/log directly, bypassing the wrapper.
  • internal/bork package defines ErrMissingMagic but it's never used — deserialize.go uses errors.New("invalid file format") instead.
  • No godoc comments on any exported functions or types.

2.6 Test Coverage

  • Tests only cover scanning and basic round-trip serialization/deserialization.
  • No tests for edge cases: empty directories, files with special characters in names, very large files, symlinks, permission errors, Unicode paths.
  • No tests for the CLI commands.
  • No fuzzing tests for the deserialization path — this is a security-relevant parser that accepts untrusted input.
  • Several test functions (in bork, cli, log, main) just assert true — placeholder tests that verify nothing.

2.7 Dependencies & Build

  • go 1.17 is ancient (EOL). Current Go is 1.22+.
  • Vendored dependencies are stored as compressed tarballs (vendor.tzst, modcache.tzst) committed to the repo — unconventional and makes the repo large. Standard go mod vendor or module proxy would be more maintainable.
  • The Dockerfile depends on sneak/builder:2022-12-08 — a custom base image that may not be publicly available, making the build non-reproducible for outside contributors.

3. Security Considerations

  1. Path traversal: No validation that file paths in a manifest are confined to the manifest's directory. A fetch implementation must sanitize paths, but the format doesn't mandate it.
  2. Decompression bombs: The inner message is gzip-compressed with no size limit on decompression. A small manifest could decompress to gigabytes. io.ReadAll on the gzip reader has no limit.
  3. No checksum verification on deserialization. As noted, the SHA-256 in the outer envelope is never checked.
  4. Signature fields are unimplemented — there's no way to verify manifest authenticity.
  5. The check command silently returns success without actually checking anything.

4. Recommendations

Immediate (before any release)

  1. Implement file checksums. This is the core value proposition. Without it, there's no reason to use mfer over ls -lR.
  2. Implement inner message deserialization.
  3. Fix the check command to either work or return an error.
  4. Fix the slice bug in gen.go.
  5. Verify SHA-256 during deserialization.
  6. Add path validation — reject absolute paths, .. components, and normalize separators.
  7. Add a decompression size limit.

Format changes (before 1.0)

  1. Specify path normalization rules in the proto comments or a separate spec document.
  2. Add file mode/permissions to MFFilePath.
  3. Add a length-prefix after the magic bytes.
  4. Choose and specify a signature scheme. I'd recommend signify over GPG — it's simpler, and the Go implementation exists.
  5. Document what bytes the signature covers.
  6. Consider whether the outer/inner envelope complexity is justified at this stage. A simpler single-message format with an optional detached signature file would be easier to implement correctly.
  7. Replace the ROT13 magic with something more conventional (e.g., \x00MF\x01 — null byte to prevent text editor misidentification, format identifier, version byte).

Engineering

  1. Export the manifest type or provide an interface.
  2. Update to a supported Go version.
  3. Add fuzzing tests for NewFromProto.
  4. Use ErrMissingMagic from the bork package or remove it.
  5. Consolidate logging — pick one approach.
  6. Write a format specification separate from the README. The proto file is a schema, not a spec — it doesn't describe semantics, validation rules, or processing requirements.

5. Summary

The problem statement is compelling and the project addresses a genuine need. The format design is reasonable in broad strokes but under-specified in critical details (path handling, signature semantics). The implementation is very early-stage — the two most important features (checksums and verification) are unimplemented, there are several bugs, and the check command is silently broken in a way that could give users false confidence in their data integrity.

Before pursuing adoption or a 1.0 release, the project needs: (1) actually computing and verifying checksums, (2) a proper format specification, (3) path security hardening, and (4) meaningful test coverage. The bones are there, but there's significant work remaining to deliver on the stated goals.

# Critical Design Review of mfer ## Executive Summary mfer aims to be a standard manifest file format for directory trees, replacing ad-hoc `SHASUMS`/`SHASUMS.asc` files. The idea is sound and addresses a real gap. However, the implementation is **pre-alpha at best** — several core features described in the README are unimplemented, there are concrete bugs, and the format design has issues that should be resolved before any "1.0" or adoption push. --- ## 1. Format Design ### 1.1 Strengths - **Protobuf serialization** is a reasonable choice: compact, well-supported across languages, schema-driven. - **Outer/inner envelope** with compression and integrity hash is a solid pattern — allows signing the inner message independently. - **Multihash for checksums** is forward-thinking and avoids locking into a single hash algorithm. ### 1.2 Weaknesses & Design Flaws **Magic bytes are a poor choice.** `ZNAVSRFG` (ROT13 of "MANIFEST") is cute but problematic: - It's not a proper magic number. File format magic should be short, unique, and ideally registered (or at least unlikely to collide). 8 ASCII bytes that happen to be valid text could easily appear in other files. - ROT13 is a joke encoding — using it in a file format intended as a "de-facto standard" undermines credibility. - No version byte follows the magic, so if you ever change the outer framing, you have no way to distinguish format versions before attempting protobuf deserialization. **The outer envelope SHA-256 is redundant with protobuf integrity.** The `sha256` field in `MFFileOuter` checksums the *uncompressed* inner message, but protobuf deserialization + gzip decompression will already fail on corruption. If the goal is detecting truncation, the `size` field suffices. If the goal is cryptographic integrity, that's what the signature fields are for. This field is neither verified in the current code nor clearly necessary. **No length-prefix on the protobuf payload.** After the magic bytes, the rest of the file is fed directly to `proto.Unmarshal`. Protobuf is not self-delimiting — if you ever want to concatenate manifests or append data after the protobuf message, this framing is insufficient. Consider a length-prefix (varint or fixed-width) after the magic. **Signature design is half-baked:** - The proto has `signature`, `signer`, and `signingPubKey` as optional byte fields on the *outer* message. But what exactly is signed? The inner serialized bytes? The compressed bytes? This is unspecified. - Embedding the public key alongside the signature is useful for discovery but meaningless for trust — anyone can generate a keypair and sign. Without a trust model (TOFU, PKI, key pinning via URL parameter as mentioned in README), the signature fields are security theater. - The README mentions both GPG and signify but neither is implemented. Pick one and specify it. **File paths have no normalization requirements.** The proto stores `string path` but there's no specification about: - Path separator (always `/`? OS-dependent?) - Whether paths must be relative (no leading `/`) - Whether `..` components are allowed (directory traversal!) - Unicode normalization (NFC vs NFD — this *will* bite you on macOS vs Linux) - Maximum path length - Allowed characters This is a **security issue** — a malicious manifest could contain paths like `../../../etc/shadow` and a naive `fetch` implementation would write outside the target directory. **No file permissions/mode.** The format stores mtime/ctime/atime but not file permissions. For an archival format, this is a notable omission. (Unix mode bits at minimum; consider also symlink targets if `FollowSymLinks` is false.) **The `MFFileChecksum` message is over-abstracted.** It's a message containing a single `bytes multiHash` field. The `repeated` on the parent means you can have multiple checksums per file, but since multihash already encodes the algorithm, you'd just have multiple multihash entries. A `repeated bytes hashes` field on `MFFilePath` would be simpler without loss of expressiveness. **No manifest self-reference or exclusion rule.** When you generate `index.mf` in a directory, should the manifest include itself? The current implementation doesn't handle this — it would either include a stale version or create a circular dependency. There should be an explicit exclusion for the output file. ### 1.3 Comparison with Alternatives | Feature | mfer | SHASUMS | .torrent | IPFS UnixFS | mtree | |---------|------|---------|----------|-------------|-------| | Multi-algorithm hashes | ✓ (planned) | ✗ | ✗ | ✓ | ✓ | | Signatures | planned | via .asc | ✗ | ✗ | ✓ | | Resumable download | planned | ✗ | ✓ | ✓ | ✗ | | Chunk-level integrity | ✗ | ✗ | ✓ | ✓ | ✗ | | Standard/adopted | ✗ | de facto | ✓ | ✓ | BSD standard | | File permissions | ✗ | ✗ | ✗ | ✓ | ✓ | The README's "hopes and dreams" section describes features (IPFS CIDs, BitTorrent chunk lists, resumable downloads) that would genuinely differentiate mfer from alternatives. Without them, mfer is essentially a more complex SHASUMS with no ecosystem advantage. The current implementation doesn't even compute checksums, so it's strictly less useful than a SHASUMS file. **Consider looking at mtree(5)** — the BSD manifest format that already solves many of the same problems with decades of production use. --- ## 2. Implementation Quality ### 2.1 Critical: Checksums Are Not Computed The single most important feature of a manifest file — **cryptographic checksums of the listed files — is not implemented.** `generateInner()` creates `MFFilePath` entries with only the path set and a `// FIXME add more stuff` comment. The `hashes` field is never populated. File sizes are tracked in the manifest struct but not written to the protobuf. This means the tool currently produces manifests that are **useless for integrity verification** — its primary stated purpose. ### 2.2 Deserialization Is Incomplete `NewFromProto()` parses the outer message and validates it, but never deserializes the inner `MFFile` message (marked `// FIXME TODO deserialize inner`). After calling `NewFromProto()`, the returned manifest has no files, no metadata — just a validated outer shell. ### 2.3 `check` and `fetch` Are Unimplemented - `checkManifestOperation` calls `log.WithError(errors.New("unimplemented"))` but **returns nil** — it silently succeeds, misleading users into thinking their files verified correctly. This is dangerous. It should return an error or `panic`. - `fetchManifestOperation` panics, which is at least honest but not graceful. ### 2.4 Bugs 1. **Slice pre-allocation bug in `gen.go`:** `paths := make([]string, ctx.Args().Len()-1)` creates a slice with `len-1` zero-value elements, then `append`s to it. Result: the paths slice has `len-1` empty strings prepended to the actual paths. This will cause the manifest to try to scan empty paths. 2. **`defer` after error check in `deserialize.go`:** `gzr.Close()` is deferred *after* the `io.ReadAll` error check. If `ReadAll` fails, the deferred close still runs, but if `NewReader` had succeeded and `ReadAll` fails, that's fine. However, the `defer` should be placed immediately after the successful `NewReader` call, before `ReadAll`, per Go convention. If `ReadAll` returns both data and an error, you'd process partial data before checking the error. 3. **`validateProtoOuter` doesn't verify SHA-256.** It checks the size but never actually computes and compares the SHA-256 hash of the decompressed inner data against `m.pbOuter.Sha256`. The hash is computed during generation but never verified during deserialization. 4. **`pathIsHidden` returns incorrect results for paths starting with `./`:** The test asserts `pathIsHidden("./a/b/c/hello.txt")` is false, and indeed `path.Clean` strips the `./`, but `path.Clean(".")` returns `"."` which starts with `.` — so `pathIsHidden(".")` would return true, which is arguably wrong for the current directory. 5. **`generate()` is not idempotent on timestamp.** `generateInner()` uses `time.Now()` — calling `generate()` twice or at different times produces different manifests for the same input. The README mentions wanting deterministic/reproducible manifests as a goal but there's no option to omit or fix the timestamp. 6. **`WriteTo` reuses `m.output` buffer.** After calling `WriteTo`, the buffer's read position advances. A second call to `WriteTo` works only because `generate()` is skipped (since `pbOuter` is already set) and `m.output.Bytes()` returns the full backing slice regardless of read position. This works by accident, not by design. ### 2.5 Code Structure - **Unexported `manifest` type with exported constructors** (`New`, `NewFromPaths`, etc.) — this is an unusual Go pattern. It prevents users from declaring the type in their own code (e.g., `var m *mfer.manifest` doesn't work from outside the package). Either export the type or use an interface. - **Mixed logging packages:** `internal/log` wraps `apex/log`, but `internal/cli/check.go` and `internal/cli/fetch.go` import `apex/log` directly, bypassing the wrapper. - **`internal/bork` package** defines `ErrMissingMagic` but it's never used — `deserialize.go` uses `errors.New("invalid file format")` instead. - **No godoc comments** on any exported functions or types. ### 2.6 Test Coverage - Tests only cover scanning and basic round-trip serialization/deserialization. - No tests for edge cases: empty directories, files with special characters in names, very large files, symlinks, permission errors, Unicode paths. - No tests for the CLI commands. - No fuzzing tests for the deserialization path — this is a security-relevant parser that accepts untrusted input. - Several test functions (in `bork`, `cli`, `log`, `main`) just assert `true` — placeholder tests that verify nothing. ### 2.7 Dependencies & Build - `go 1.17` is ancient (EOL). Current Go is 1.22+. - Vendored dependencies are stored as compressed tarballs (`vendor.tzst`, `modcache.tzst`) committed to the repo — unconventional and makes the repo large. Standard `go mod vendor` or module proxy would be more maintainable. - The Dockerfile depends on `sneak/builder:2022-12-08` — a custom base image that may not be publicly available, making the build non-reproducible for outside contributors. --- ## 3. Security Considerations 1. **Path traversal:** No validation that file paths in a manifest are confined to the manifest's directory. A `fetch` implementation must sanitize paths, but the format doesn't mandate it. 2. **Decompression bombs:** The inner message is gzip-compressed with no size limit on decompression. A small manifest could decompress to gigabytes. `io.ReadAll` on the gzip reader has no limit. 3. **No checksum verification on deserialization.** As noted, the SHA-256 in the outer envelope is never checked. 4. **Signature fields are unimplemented** — there's no way to verify manifest authenticity. 5. **The `check` command silently returns success** without actually checking anything. --- ## 4. Recommendations ### Immediate (before any release) 1. **Implement file checksums.** This is the core value proposition. Without it, there's no reason to use mfer over `ls -lR`. 2. **Implement inner message deserialization.** 3. **Fix the `check` command** to either work or return an error. 4. **Fix the slice bug in `gen.go`.** 5. **Verify SHA-256 during deserialization.** 6. **Add path validation** — reject absolute paths, `..` components, and normalize separators. 7. **Add a decompression size limit.** ### Format changes (before 1.0) 8. **Specify path normalization rules** in the proto comments or a separate spec document. 9. **Add file mode/permissions** to `MFFilePath`. 10. **Add a length-prefix** after the magic bytes. 11. **Choose and specify a signature scheme.** I'd recommend signify over GPG — it's simpler, and the Go implementation exists. 12. **Document what bytes the signature covers.** 13. **Consider whether the outer/inner envelope complexity is justified** at this stage. A simpler single-message format with an optional detached signature file would be easier to implement correctly. 14. **Replace the ROT13 magic** with something more conventional (e.g., `\x00MF\x01` — null byte to prevent text editor misidentification, format identifier, version byte). ### Engineering 15. **Export the `manifest` type** or provide an interface. 16. **Update to a supported Go version.** 17. **Add fuzzing tests** for `NewFromProto`. 18. **Use `ErrMissingMagic`** from the bork package or remove it. 19. **Consolidate logging** — pick one approach. 20. **Write a format specification** separate from the README. The proto file is a schema, not a spec — it doesn't describe semantics, validation rules, or processing requirements. --- ## 5. Summary The problem statement is compelling and the project addresses a genuine need. The format design is reasonable in broad strokes but under-specified in critical details (path handling, signature semantics). The implementation is very early-stage — the two most important features (checksums and verification) are unimplemented, there are several bugs, and the `check` command is silently broken in a way that could give users false confidence in their data integrity. Before pursuing adoption or a 1.0 release, the project needs: (1) actually computing and verifying checksums, (2) a proper format specification, (3) path security hardening, and (4) meaningful test coverage. The bones are there, but there's significant work remaining to deliver on the stated goals.
clawbot removed their assignment 2026-02-08 18:20:57 +01:00
sneak was assigned by clawbot 2026-02-08 18:20:57 +01:00
sneak commented 2026-02-08 18:26:40 +01:00
Author
Owner
Copy Link

redo this analysis with an understanding that those panics and fixmes are a list of unimplemented items, not a failure of implementation. obviously the implementation is incomplete; this is not working code. you are to analyze and criticize the design of the system and the parts that are already implemented. create an ordered itemized implementation plan for getting to a 1.0. let's go with zstd instead of gzip.

the outer envelope is for cryptographic integrity. if you know a way to do that without double protobufs, please let me know. no to cbor or json - the reasons for using protobuf are clear. i agree with determinism.

please also redo your analysis on branch next - main is way out of date.

redo this analysis with an understanding that those panics and fixmes are a list of unimplemented items, not a failure of implementation. obviously the implementation is incomplete; this is not working code. you are to analyze and criticize the design of the system and the parts that *are* already implemented. create an ordered itemized implementation plan for getting to a 1.0. let's go with zstd instead of gzip. the outer envelope is for cryptographic integrity. if you know a way to do that without double protobufs, please let me know. no to cbor or json - the reasons for using protobuf are clear. i agree with determinism. please also redo your analysis on branch `next` - `main` is way out of date.
sneak was unassigned by clawbot 2026-02-08 18:26:49 +01:00
clawbot self-assigned this 2026-02-08 18:26:49 +01:00
clawbot commented 2026-02-08 18:30:08 +01:00
Collaborator
Copy Link

Revised Design Review: mfer (next branch)

Reviewed: all source on next branch, including protobuf schema, mfer/ package, internal/ packages, CLI, and tests.

1. Format Design Critique

1.1 The Protobuf Schema

The schema is clean and well-structured. Specific observations:

Good decisions:

  • Separating MFFileOuter (envelope) from MFFile (content) is the right call for enabling signatures over a stable byte representation.
  • Using multihash for file checksums is forward-thinking — avoids algorithm lock-in.
  • UUID in both inner and outer for replay prevention is a solid design choice.
  • CompressionType enum with COMPRESSION_ZSTD — good, zstd is the right choice. ✓
  • Deterministic protobuf marshaling is already used throughout. ✓

Concerns:

  1. MFFileChecksum is an unnecessary indirection. It's a message with a single bytes multiHash field. Since multihash already self-describes the algorithm, repeated bytes hashes = 3 on MFFilePath would be equivalent and simpler. The extra message layer adds protobuf overhead (tag+length for the wrapper message) for every hash of every file. At scale (millions of files), this is measurable. If you plan to add per-hash metadata later (e.g. a verified_at timestamp), the wrapper is justified — otherwise simplify.

  2. No file permissions/mode field. For the archival use case (ExFAT drives, filesystem-independent checksums), this may not matter. But if mfer manifests are ever used for software distribution or filesystem restoration, the absence of permission bits will be a gap. Consider reserving a field number now even if you don't populate it yet (e.g. optional uint32 mode = 305).

  3. atime in the schema is questionable. Access time is volatile, non-deterministic, and often disabled (noatime). Including it in manifests means two manifests of the same directory tree at different times will differ. If deterministic manifests are a goal (they should be), atime should be removed or explicitly documented as "never set by default."

  4. Path semantics are unspecified in the schema. The proto has string path but no comments specifying: always forward-slash separated? Must be relative? No .. components? UTF-8 NFC normalized? This is the single most dangerous under-specification — it directly affects security (path traversal) and cross-platform compatibility (macOS NFD vs Linux NFC). I'd add proto comments specifying the invariants, and validate in code.

1.2 The Envelope / Inner Message Structure

The double-protobuf design serves a clear purpose: you need to sign bytes, and protobuf serialization is not canonically unique (field ordering, default-value omission, unknown fields). By serializing the inner MFFile to bytes, compressing, then wrapping those bytes in MFFileOuter.innerMessage, you get a stable byte sequence to hash and sign. This is sound.

However, there are alternatives worth considering:

Alternative: Canonical serialization + sign-then-wrap

Protobuf with deterministic marshaling (which you're already using via proto.MarshalOptions{Deterministic: true}) produces stable output for a given message from the same implementation. The Go protobuf docs note this is not guaranteed across implementations or versions, but in practice:

  • If you control both the writer and the spec, and mandate "serialize with deterministic=true, fields in number order, no unknown fields," you get de facto canonicality.
  • You could then sign the raw serialized MFFile bytes directly, and structure the file as: MAGIC | SignedEnvelope(signature, signer, signingPubKey, compression_type, compressed_inner_bytes).

This is essentially what you're doing now, but recognizing it explicitly as "the inner bytes ARE the canonical form" rather than treating it as an opaque blob. The double-protobuf is fine — I don't think there's a way to avoid it that doesn't sacrifice the integrity property. The key insight is that any sign-then-wrap scheme needs a stable byte representation of the signed content, and protobuf messages aren't that (in the general case), so wrapping the serialized bytes is correct.

One concrete improvement: The sha256 field on MFFileOuter currently hashes the compressed data. This is the right call — it lets you verify integrity before decompression (preventing decompression bombs from untrusted sources). But document this clearly, because it's a non-obvious choice. Someone reading the proto might assume it's a hash of the uncompressed inner message.

Alternative: Detached signature file

For distribution scenarios, a detached .mf.sig file alongside index.mf would allow verifying integrity without modifying the manifest format. This is simpler for tooling (any HTTP server can serve both files) and follows the SHASUMS + SHASUMS.asc pattern that users already understand. The embedded signature is better for single-file distribution though, so supporting both modes might be ideal.

1.3 Magic Bytes

ZNAVSRFG works. It's 8 bytes, ASCII-printable, and unlikely to collide. The ROT13 origin is irrelevant to functionality. My only suggestion: consider following it with a version byte (0x01) to enable future framing changes without requiring protobuf parsing to detect the version. Currently MFFileOuter.Version serves this purpose but requires successful deserialization to read.

2. Implemented Code Quality

Focusing on code that IS implemented and working (not stubs):

2.1 Serialization / Deserialization (serialize.go, deserialize.go)

Well done:

  • Deterministic marshaling throughout ✓
  • Hash verification before decompression ✓
  • UUID validation and cross-check between inner/outer ✓
  • Signature verification during deserialization ✓
  • Clean error propagation ✓

Issues:

  1. signatureString() format is ad-hoc. The canonical string MAGIC-UUID-MULTIHASH is fine, but hex-encoding the UUID and multihash means the signature covers a derived representation rather than raw bytes. This works but adds an unnecessary transformation. Signing the raw sha256 bytes (or the compressed innerMessage bytes directly) would be simpler and avoid potential encoding ambiguity. If you keep the string format, document it as part of the spec.

  2. No decompression size limit. io.ReadAll(zr) in deserializeInner() will happily decompress into unbounded memory. The size field exists — use it as a limit: io.LimitReader(zr, m.pbOuter.Size + some_slack). This prevents decompression bombs from malicious manifests.

  3. The size field stores uncompressed size but is checked after full decompression. You could check the zstd content size header (zstd frames include the uncompressed size) before decompressing, for early rejection of tampered manifests.

2.2 Builder (builder.go)

Well done:

  • Clean API with AddFile (hashing reader) and AddFileWithHash (pre-computed) ✓
  • Mutex protection for concurrent use ✓
  • Non-blocking progress channel sends ✓
  • Input validation on AddFileWithHash ✓

Issues:

  1. AddFile doesn't verify that bytes read match the declared size. The caller passes size as a parameter, and AddFile reads from the reader, but never checks that totalRead == FileSize(size). If they diverge (e.g., file changed between stat and read), the manifest entry has a size that doesn't match the hash. Add a post-read size check.

  2. 64KB read buffer is fine but could be documented. The chunk size affects progress granularity. For very large files, 256KB or 1MB would reduce syscall overhead. Minor.

2.3 Scanner (scanner.go)

Well done:

  • Two-phase design (enumerate then scan) is correct for UX — enables accurate progress bars ✓
  • Afero filesystem abstraction enables thorough testing ✓
  • Context cancellation support ✓
  • Symlink handling with explicit option ✓
  • IsHiddenPath is correct and well-tested ✓

Issues:

  1. totalBytes is tracked incrementally in Scanner but recalculated in ToManifest. The scanner maintains s.totalBytes during enumeration, but ToManifest re-sums all file sizes. Use the cached value.

  2. No deterministic file ordering. afero.Walk returns files in filesystem order, which varies by OS and filesystem. For reproducible manifests, sort s.files by path before writing. This is critical for determinism.

  3. Race condition in progress goroutine. In ToManifest, the progress goroutine captures scannedFiles and scannedBytes from the outer scope without synchronization. These are modified by the main loop. Use atomic operations or pass values through the channel.

2.4 Checker (checker.go)

Well done:

  • Comprehensive status types (OK, Missing, SizeMismatch, HashMismatch, Extra, Error) ✓
  • Size check before hash (fast-fail on cheap check) ✓
  • FindExtraFiles for detecting undeclared files ✓
  • Context cancellation ✓

Issues:

  1. errors.Is(err, errors.New("file does not exist")) always returns false (line in checkFile). errors.New creates a unique error value each call; errors.Is compares by identity, not string. The fallback afero.Exists call saves it, but the first comparison is dead code. Use os.IsNotExist(err) or errors.Is(err, fs.ErrNotExist).

  2. No progress rate-limiting in Checker. Unlike Scanner which sends progress at most once per second, Checker sends a progress update for every single file. For manifests with millions of small files, this floods the channel. Add the same 1-second throttling.

2.5 GPG Integration (gpg.go)

Well done:

  • Temporary keyring for verification (doesn't pollute user's keyring) ✓
  • Proper cleanup with defer os.RemoveAll ✓
  • Fingerprint extraction from embedded key ✓

Issues:

  1. Shelling out to gpg is fragile. The binary might not be installed, or might be a different version with different output formats. Consider using golang.org/x/crypto/openpgp (deprecated but stable) or the maintained fork github.com/ProtonMail/go-crypto for a pure-Go implementation. This would also make cross-compilation and testing much easier.

  2. No timeout on GPG subprocess. A hung GPG agent could block forever. Use exec.CommandContext with a timeout.

  3. GPG colon-delimited output parsing is brittle. If GPG changes its output format (it has before), the fingerprint extraction breaks silently. At minimum, add a test that verifies the parsing against known GPG output.

2.6 Fetch (internal/cli/fetch.go)

Well done:

  • Path traversal prevention via sanitizePath with good test coverage ✓
  • Atomic downloads (temp file + rename) ✓
  • Hash verification before rename ✓
  • Size verification ✓
  • Proper cleanup of temp files on failure ✓

Issues:

  1. fileURL := baseURL.String() + f.Path — this concatenates the file path without URL-encoding. Paths with spaces, #, ?, or other URL-special characters will break. Use url.JoinPath() or the BaseURL.JoinPath method that already exists in url.go.

  2. No concurrent downloads. For fetching many small files over high-latency connections, sequential HTTP GETs are slow. A worker pool with configurable concurrency would significantly improve fetch performance. (Not blocking for 1.0, but worth noting.)

  3. No retry logic. A single HTTP failure aborts the entire fetch. At minimum, retry transient errors (5xx, timeouts) with exponential backoff.

2.7 Code Duplication

There are two parallel implementations of Scanner and Checker:

  • mfer/scanner.go + mfer/checker.go (typed with custom types like FileSize, RelFilePath)
  • internal/scanner/scanner.go + internal/checker/checker.go (using raw int64, string)

The internal/ versions appear to be older and are used by some CLI code while the mfer/ versions are newer and better-typed. Pick one and delete the other. The mfer/ package versions are superior — they use semantic types and have better progress reporting.

2.8 Freshen Command (internal/cli/freshen.go)

Well done:

  • Incremental update (only rehash changed files) is a great feature for large archives ✓
  • Mtime-based change detection is the right heuristic ✓
  • Atomic write of updated manifest ✓

Issue: The IncludeDotfiles and FollowSymLinks flag names use Go-style PascalCase instead of kebab-case. The --IncludeDotfiles flag looks odd on the command line. The generate command has the same issue. Use Name: "include-dotfiles" as the primary with the Go name as an alias (which you already do for generate, but freshen reads ctx.Bool("IncludeDotfiles") which would need the PascalCase form).

3. The Double-Protobuf Question

Is there a cleaner way to achieve cryptographic integrity over a protobuf message?

The core problem: protobuf serialization is not guaranteed to be canonical across implementations. Two implementations serializing the same logical message may produce different bytes. Therefore, you can't sign a protobuf message — you must sign specific bytes.

Your current approach (serialize inner → compress → wrap bytes in outer → sign) is the standard solution. Alternatives:

  1. COSE (CBOR Object Signing and Encryption) — ruled out since you want protobuf.

  2. Protobuf Canonical Encoding: There is no official protobuf canonical form. Go's Deterministic: true is implementation-specific. However, you could define your own canonical rules (fields in number order, no unknown fields, varint minimal encoding) and document them as part of the mfer spec. This would let you sign the inner bytes directly without a wrapper. But you'd still need to store the signature somewhere, which means... an outer message. So you end up with the same structure.

  3. Sign the compressed bytes, store signature separately: MAGIC | compressed_inner_bytes | signature_trailer. This avoids the outer protobuf but requires custom framing for the trailer. Net complexity is similar.

Verdict: The double-protobuf is the right approach. It's well-understood (TLS, JWT, and many other protocols use sign-then-wrap), easy to implement in other languages, and the overhead of the outer protobuf envelope is negligible. Don't change it.

4. Implementation Plan to 1.0

Ordered by dependency and priority. Each item should be a PR.

Phase 1: Foundation (format correctness)

  1. Delete internal/scanner/ and internal/checker/ — consolidate on the mfer/ package versions. Update CLI code to use mfer.Scanner and mfer.Checker.

  2. Add deterministic file ordering — sort file entries by path (lexicographic, byte-order) before serialization in Builder.Build(). Add a test that generates a manifest twice and asserts byte-identical output.

  3. Add decompression size limit — use io.LimitReader in deserializeInner() with m.pbOuter.Size as the bound.

  4. Fix errors.Is dead code in checker — replace with os.IsNotExist(err) or errors.Is(err, fs.ErrNotExist).

  5. Fix AddFile to verify size — after reading, check totalRead == size and return an error on mismatch.

  6. Specify path invariants — add comments to proto: paths are UTF-8, forward-slash separated, relative, no .. components, no leading /. Validate in Builder.AddFile and Builder.AddFileWithHash.

Phase 2: CLI polish

  1. Fix flag naming — ensure all CLI flags use kebab-case as primary names (--include-dotfiles, --follow-symlinks). Read with kebab-case in code.

  2. Fix URL construction in fetch — use BaseURL.JoinPath() or url.JoinPath() instead of string concatenation.

  3. Add progress rate-limiting to Checker — throttle to once per second, matching Scanner behavior.

  4. Add --deterministic flag (or make it default) — omit createdAt timestamp, sort files. For reproducible manifests.

Phase 3: Robustness

  1. Replace GPG subprocess with pure-Go crypto — use github.com/ProtonMail/go-crypto for OpenPGP operations, or switch to Ed25519/signify (simpler, no key management complexity). This removes the runtime dependency on gpg being installed.

  2. Add timeout to any remaining subprocess calls.

  3. Add fuzzing tests for NewManifestFromReader — this parses untrusted input and should be fuzz-tested.

  4. Add retry logic to fetch — exponential backoff for transient HTTP errors.

Phase 4: Format finalization

  1. Remove atime from proto (or document it as deprecated/never-set). It hurts determinism.

  2. Reserve optional uint32 mode = 305 in MFFilePath for future file permission support.

  3. Add a version byte after magic — ZNAVSRFG\x01 for format version 1. Allows future framing changes.

  4. Write a format specification document — separate from README. Cover: magic format, outer message structure, compression, inner message structure, path invariants, signature scheme, canonical serialization requirements.

Phase 5: Release prep

  1. Update Go module path if needed (currently sneak.berlin/go/mfer in some imports, git.eeqj.de/sneak/mfer in proto).

  2. Audit all error messages for consistency and helpfulness.

  3. Add --version output to match SemVer format.

  4. Tag v1.0.0.

5. Summary

The next branch represents a significant leap from main. The core features work: manifest generation with SHA-256 multihash checksums, verification, fetch with integrity checking, incremental freshen, GPG signing/verification, zstd compression, and comprehensive tests (including a 500-iteration corruption detection test — nice).

The format design is sound. The double-protobuf envelope is the correct approach for cryptographic integrity over protobuf messages — I don't see a simpler alternative that preserves the same properties. The main design gaps are: unspecified path semantics (security risk), no decompression limit (DoS risk), and duplicated internal packages that should be consolidated.

The implemented code is generally high quality — good use of interfaces (afero.Fs), proper concurrency patterns, atomic file operations, and thorough test coverage. The issues I've flagged are real but mostly minor: a dead errors.Is comparison, missing size verification in AddFile, URL encoding in fetch, and the internal package duplication.

For 1.0, the biggest wins would be: consolidate the duplicate packages, add deterministic ordering, specify path rules, and write a format spec. The code is much closer to releasable than the previous review suggested.

# Revised Design Review: mfer (`next` branch) *Reviewed: all source on `next` branch, including protobuf schema, `mfer/` package, `internal/` packages, CLI, and tests.* --- ## 1. Format Design Critique ### 1.1 The Protobuf Schema The schema is clean and well-structured. Specific observations: **Good decisions:** - Separating `MFFileOuter` (envelope) from `MFFile` (content) is the right call for enabling signatures over a stable byte representation. - Using multihash for file checksums is forward-thinking — avoids algorithm lock-in. - UUID in both inner and outer for replay prevention is a solid design choice. - `CompressionType` enum with `COMPRESSION_ZSTD` — good, zstd is the right choice. ✓ - Deterministic protobuf marshaling is already used throughout. ✓ **Concerns:** 1. **`MFFileChecksum` is an unnecessary indirection.** It's a message with a single `bytes multiHash` field. Since multihash already self-describes the algorithm, `repeated bytes hashes = 3` on `MFFilePath` would be equivalent and simpler. The extra message layer adds protobuf overhead (tag+length for the wrapper message) for every hash of every file. At scale (millions of files), this is measurable. If you plan to add per-hash metadata later (e.g. a `verified_at` timestamp), the wrapper is justified — otherwise simplify. 2. **No file permissions/mode field.** For the archival use case (ExFAT drives, filesystem-independent checksums), this may not matter. But if mfer manifests are ever used for software distribution or filesystem restoration, the absence of permission bits will be a gap. Consider reserving a field number now even if you don't populate it yet (e.g. `optional uint32 mode = 305`). 3. **`atime` in the schema is questionable.** Access time is volatile, non-deterministic, and often disabled (`noatime`). Including it in manifests means two manifests of the same directory tree at different times will differ. If deterministic manifests are a goal (they should be), `atime` should be removed or explicitly documented as "never set by default." 4. **Path semantics are unspecified in the schema.** The proto has `string path` but no comments specifying: always forward-slash separated? Must be relative? No `..` components? UTF-8 NFC normalized? This is the single most dangerous under-specification — it directly affects security (path traversal) and cross-platform compatibility (macOS NFD vs Linux NFC). I'd add proto comments specifying the invariants, and validate in code. ### 1.2 The Envelope / Inner Message Structure The double-protobuf design serves a clear purpose: you need to sign *bytes*, and protobuf serialization is not canonically unique (field ordering, default-value omission, unknown fields). By serializing the inner `MFFile` to bytes, compressing, then wrapping those bytes in `MFFileOuter.innerMessage`, you get a stable byte sequence to hash and sign. This is sound. **However, there are alternatives worth considering:** #### Alternative: Canonical serialization + sign-then-wrap Protobuf *with deterministic marshaling* (which you're already using via `proto.MarshalOptions{Deterministic: true}`) produces stable output for a given message *from the same implementation*. The Go protobuf docs note this is not guaranteed across implementations or versions, but in practice: - If you control both the writer and the spec, and mandate "serialize with deterministic=true, fields in number order, no unknown fields," you get de facto canonicality. - You could then sign the raw serialized `MFFile` bytes directly, and structure the file as: `MAGIC | SignedEnvelope(signature, signer, signingPubKey, compression_type, compressed_inner_bytes)`. This is essentially what you're doing now, but recognizing it explicitly as "the inner bytes ARE the canonical form" rather than treating it as an opaque blob. The double-protobuf is fine — I don't think there's a way to avoid it that doesn't sacrifice the integrity property. The key insight is that **any sign-then-wrap scheme needs a stable byte representation of the signed content**, and protobuf messages aren't that (in the general case), so wrapping the serialized bytes is correct. **One concrete improvement:** The `sha256` field on `MFFileOuter` currently hashes the *compressed* data. This is the right call — it lets you verify integrity before decompression (preventing decompression bombs from untrusted sources). But document this clearly, because it's a non-obvious choice. Someone reading the proto might assume it's a hash of the *uncompressed* inner message. #### Alternative: Detached signature file For distribution scenarios, a detached `.mf.sig` file alongside `index.mf` would allow verifying integrity without modifying the manifest format. This is simpler for tooling (any HTTP server can serve both files) and follows the `SHASUMS` + `SHASUMS.asc` pattern that users already understand. The embedded signature is better for single-file distribution though, so supporting both modes might be ideal. ### 1.3 Magic Bytes `ZNAVSRFG` works. It's 8 bytes, ASCII-printable, and unlikely to collide. The ROT13 origin is irrelevant to functionality. My only suggestion: consider following it with a version byte (`0x01`) to enable future framing changes without requiring protobuf parsing to detect the version. Currently `MFFileOuter.Version` serves this purpose but requires successful deserialization to read. --- ## 2. Implemented Code Quality Focusing on code that IS implemented and working (not stubs): ### 2.1 Serialization / Deserialization (serialize.go, deserialize.go) **Well done:** - Deterministic marshaling throughout ✓ - Hash verification before decompression ✓ - UUID validation and cross-check between inner/outer ✓ - Signature verification during deserialization ✓ - Clean error propagation ✓ **Issues:** 1. **`signatureString()` format is ad-hoc.** The canonical string `MAGIC-UUID-MULTIHASH` is fine, but hex-encoding the UUID and multihash means the signature covers a derived representation rather than raw bytes. This works but adds an unnecessary transformation. Signing the raw `sha256` bytes (or the compressed `innerMessage` bytes directly) would be simpler and avoid potential encoding ambiguity. If you keep the string format, document it as part of the spec. 2. **No decompression size limit.** `io.ReadAll(zr)` in `deserializeInner()` will happily decompress into unbounded memory. The `size` field exists — use it as a limit: `io.LimitReader(zr, m.pbOuter.Size + some_slack)`. This prevents decompression bombs from malicious manifests. 3. **The `size` field stores uncompressed size but is checked after full decompression.** You could check the zstd content size header (zstd frames include the uncompressed size) before decompressing, for early rejection of tampered manifests. ### 2.2 Builder (builder.go) **Well done:** - Clean API with `AddFile` (hashing reader) and `AddFileWithHash` (pre-computed) ✓ - Mutex protection for concurrent use ✓ - Non-blocking progress channel sends ✓ - Input validation on `AddFileWithHash` ✓ **Issues:** 1. **`AddFile` doesn't verify that bytes read match the declared `size`.** The caller passes `size` as a parameter, and `AddFile` reads from the reader, but never checks that `totalRead == FileSize(size)`. If they diverge (e.g., file changed between stat and read), the manifest entry has a `size` that doesn't match the hash. Add a post-read size check. 2. **64KB read buffer is fine but could be documented.** The chunk size affects progress granularity. For very large files, 256KB or 1MB would reduce syscall overhead. Minor. ### 2.3 Scanner (scanner.go) **Well done:** - Two-phase design (enumerate then scan) is correct for UX — enables accurate progress bars ✓ - Afero filesystem abstraction enables thorough testing ✓ - Context cancellation support ✓ - Symlink handling with explicit option ✓ - `IsHiddenPath` is correct and well-tested ✓ **Issues:** 1. **`totalBytes` is tracked incrementally in Scanner but recalculated in `ToManifest`.** The scanner maintains `s.totalBytes` during enumeration, but `ToManifest` re-sums all file sizes. Use the cached value. 2. **No deterministic file ordering.** `afero.Walk` returns files in filesystem order, which varies by OS and filesystem. For reproducible manifests, sort `s.files` by path before writing. This is critical for determinism. 3. **Race condition in progress goroutine.** In `ToManifest`, the progress goroutine captures `scannedFiles` and `scannedBytes` from the outer scope without synchronization. These are modified by the main loop. Use atomic operations or pass values through the channel. ### 2.4 Checker (checker.go) **Well done:** - Comprehensive status types (OK, Missing, SizeMismatch, HashMismatch, Extra, Error) ✓ - Size check before hash (fast-fail on cheap check) ✓ - `FindExtraFiles` for detecting undeclared files ✓ - Context cancellation ✓ **Issues:** 1. **`errors.Is(err, errors.New("file does not exist"))` always returns false** (line in `checkFile`). `errors.New` creates a unique error value each call; `errors.Is` compares by identity, not string. The fallback `afero.Exists` call saves it, but the first comparison is dead code. Use `os.IsNotExist(err)` or `errors.Is(err, fs.ErrNotExist)`. 2. **No progress rate-limiting in Checker.** Unlike Scanner which sends progress at most once per second, Checker sends a progress update for every single file. For manifests with millions of small files, this floods the channel. Add the same 1-second throttling. ### 2.5 GPG Integration (gpg.go) **Well done:** - Temporary keyring for verification (doesn't pollute user's keyring) ✓ - Proper cleanup with `defer os.RemoveAll` ✓ - Fingerprint extraction from embedded key ✓ **Issues:** 1. **Shelling out to `gpg` is fragile.** The binary might not be installed, or might be a different version with different output formats. Consider using `golang.org/x/crypto/openpgp` (deprecated but stable) or the maintained fork `github.com/ProtonMail/go-crypto` for a pure-Go implementation. This would also make cross-compilation and testing much easier. 2. **No timeout on GPG subprocess.** A hung GPG agent could block forever. Use `exec.CommandContext` with a timeout. 3. **GPG colon-delimited output parsing is brittle.** If GPG changes its output format (it has before), the fingerprint extraction breaks silently. At minimum, add a test that verifies the parsing against known GPG output. ### 2.6 Fetch (internal/cli/fetch.go) **Well done:** - Path traversal prevention via `sanitizePath` with good test coverage ✓ - Atomic downloads (temp file + rename) ✓ - Hash verification before rename ✓ - Size verification ✓ - Proper cleanup of temp files on failure ✓ **Issues:** 1. **`fileURL := baseURL.String() + f.Path`** — this concatenates the file path without URL-encoding. Paths with spaces, `#`, `?`, or other URL-special characters will break. Use `url.JoinPath()` or the `BaseURL.JoinPath` method that already exists in `url.go`. 2. **No concurrent downloads.** For fetching many small files over high-latency connections, sequential HTTP GETs are slow. A worker pool with configurable concurrency would significantly improve fetch performance. (Not blocking for 1.0, but worth noting.) 3. **No retry logic.** A single HTTP failure aborts the entire fetch. At minimum, retry transient errors (5xx, timeouts) with exponential backoff. ### 2.7 Code Duplication There are two parallel implementations of Scanner and Checker: - `mfer/scanner.go` + `mfer/checker.go` (typed with custom types like `FileSize`, `RelFilePath`) - `internal/scanner/scanner.go` + `internal/checker/checker.go` (using raw `int64`, `string`) The `internal/` versions appear to be older and are used by some CLI code while the `mfer/` versions are newer and better-typed. **Pick one and delete the other.** The `mfer/` package versions are superior — they use semantic types and have better progress reporting. ### 2.8 Freshen Command (internal/cli/freshen.go) **Well done:** - Incremental update (only rehash changed files) is a great feature for large archives ✓ - Mtime-based change detection is the right heuristic ✓ - Atomic write of updated manifest ✓ **Issue:** The `IncludeDotfiles` and `FollowSymLinks` flag names use Go-style PascalCase instead of kebab-case. The `--IncludeDotfiles` flag looks odd on the command line. The `generate` command has the same issue. Use `Name: "include-dotfiles"` as the primary with the Go name as an alias (which you already do for generate, but freshen reads `ctx.Bool("IncludeDotfiles")` which would need the PascalCase form). --- ## 3. The Double-Protobuf Question **Is there a cleaner way to achieve cryptographic integrity over a protobuf message?** The core problem: protobuf serialization is not guaranteed to be canonical across implementations. Two implementations serializing the same logical message may produce different bytes. Therefore, you can't sign a protobuf *message* — you must sign specific *bytes*. Your current approach (serialize inner → compress → wrap bytes in outer → sign) is the standard solution. Alternatives: 1. **COSE (CBOR Object Signing and Encryption)** — ruled out since you want protobuf. 2. **Protobuf Canonical Encoding:** There is no official protobuf canonical form. Go's `Deterministic: true` is implementation-specific. However, you could define your own canonical rules (fields in number order, no unknown fields, varint minimal encoding) and document them as part of the mfer spec. This would let you sign the inner bytes directly without a wrapper. But you'd still need to store the signature *somewhere*, which means... an outer message. So you end up with the same structure. 3. **Sign the compressed bytes, store signature separately:** `MAGIC | compressed_inner_bytes | signature_trailer`. This avoids the outer protobuf but requires custom framing for the trailer. Net complexity is similar. **Verdict:** The double-protobuf is the right approach. It's well-understood (TLS, JWT, and many other protocols use sign-then-wrap), easy to implement in other languages, and the overhead of the outer protobuf envelope is negligible. Don't change it. --- ## 4. Implementation Plan to 1.0 Ordered by dependency and priority. Each item should be a PR. ### Phase 1: Foundation (format correctness) 1. **Delete `internal/scanner/` and `internal/checker/`** — consolidate on the `mfer/` package versions. Update CLI code to use `mfer.Scanner` and `mfer.Checker`. 2. **Add deterministic file ordering** — sort file entries by path (lexicographic, byte-order) before serialization in `Builder.Build()`. Add a test that generates a manifest twice and asserts byte-identical output. 3. **Add decompression size limit** — use `io.LimitReader` in `deserializeInner()` with `m.pbOuter.Size` as the bound. 4. **Fix `errors.Is` dead code in checker** — replace with `os.IsNotExist(err)` or `errors.Is(err, fs.ErrNotExist)`. 5. **Fix `AddFile` to verify size** — after reading, check `totalRead == size` and return an error on mismatch. 6. **Specify path invariants** — add comments to proto: paths are UTF-8, forward-slash separated, relative, no `..` components, no leading `/`. Validate in `Builder.AddFile` and `Builder.AddFileWithHash`. ### Phase 2: CLI polish 7. **Fix flag naming** — ensure all CLI flags use kebab-case as primary names (`--include-dotfiles`, `--follow-symlinks`). Read with kebab-case in code. 8. **Fix URL construction in fetch** — use `BaseURL.JoinPath()` or `url.JoinPath()` instead of string concatenation. 9. **Add progress rate-limiting to Checker** — throttle to once per second, matching Scanner behavior. 10. **Add `--deterministic` flag** (or make it default) — omit `createdAt` timestamp, sort files. For reproducible manifests. ### Phase 3: Robustness 11. **Replace GPG subprocess with pure-Go crypto** — use `github.com/ProtonMail/go-crypto` for OpenPGP operations, or switch to Ed25519/signify (simpler, no key management complexity). This removes the runtime dependency on `gpg` being installed. 12. **Add timeout to any remaining subprocess calls.** 13. **Add fuzzing tests** for `NewManifestFromReader` — this parses untrusted input and should be fuzz-tested. 14. **Add retry logic to fetch** — exponential backoff for transient HTTP errors. ### Phase 4: Format finalization 15. **Remove `atime` from proto** (or document it as deprecated/never-set). It hurts determinism. 16. **Reserve `optional uint32 mode = 305`** in `MFFilePath` for future file permission support. 17. **Add a version byte after magic** — `ZNAVSRFG\x01` for format version 1. Allows future framing changes. 18. **Write a format specification document** — separate from README. Cover: magic format, outer message structure, compression, inner message structure, path invariants, signature scheme, canonical serialization requirements. ### Phase 5: Release prep 19. **Update Go module path** if needed (currently `sneak.berlin/go/mfer` in some imports, `git.eeqj.de/sneak/mfer` in proto). 20. **Audit all error messages** for consistency and helpfulness. 21. **Add `--version` output to match SemVer** format. 22. **Tag v1.0.0.** --- ## 5. Summary The `next` branch represents a significant leap from `main`. The core features work: manifest generation with SHA-256 multihash checksums, verification, fetch with integrity checking, incremental freshen, GPG signing/verification, zstd compression, and comprehensive tests (including a 500-iteration corruption detection test — nice). The format design is sound. The double-protobuf envelope is the correct approach for cryptographic integrity over protobuf messages — I don't see a simpler alternative that preserves the same properties. The main design gaps are: unspecified path semantics (security risk), no decompression limit (DoS risk), and duplicated internal packages that should be consolidated. The implemented code is generally high quality — good use of interfaces (`afero.Fs`), proper concurrency patterns, atomic file operations, and thorough test coverage. The issues I've flagged are real but mostly minor: a dead `errors.Is` comparison, missing size verification in `AddFile`, URL encoding in fetch, and the internal package duplication. For 1.0, the biggest wins would be: consolidate the duplicate packages, add deterministic ordering, specify path rules, and write a format spec. The code is much closer to releasable than the previous review suggested.
clawbot removed their assignment 2026-02-08 18:30:33 +01:00
sneak was assigned by clawbot 2026-02-08 18:30:33 +01:00
sneak commented 2026-02-08 18:38:55 +01:00
Author
Owner
Copy Link

create a TODO.md in the root of branch next with the implementation steps at the end of your comment. add a section at the top with all of the design questions you had for me - such as the implicit ones around atime and redundant fields. i'll answer them inline in the TODO file and we'll have them documented for posterity.

create a TODO.md in the root of branch next with the implementation steps at the end of your comment. add a section at the top with all of the design questions you had for me - such as the implicit ones around atime and redundant fields. i'll answer them inline in the TODO file and we'll have them documented for posterity.
sneak removed their assignment 2026-02-08 18:39:04 +01:00
clawbot was assigned by sneak 2026-02-08 18:39:04 +01:00
clawbot commented 2026-02-08 18:40:38 +01:00
Collaborator
Copy Link

Created TODO.md on the next branch with:

  • 14 design questions at the top (numbered, with space for inline answers)
  • 22-item implementation plan at the bottom (checkboxes, organized in 5 phases)

sneak: edit the file directly on next to answer the design questions inline under each > *answer:* block.

Created [`TODO.md`](https://git.eeqj.de/sneak/mfer/src/branch/next/TODO.md) on the `next` branch with: - **14 design questions** at the top (numbered, with space for inline answers) - **22-item implementation plan** at the bottom (checkboxes, organized in 5 phases) sneak: edit the file directly on `next` to answer the design questions inline under each `> *answer:*` block.
clawbot commented 2026-02-08 18:40:42 +01:00
Collaborator
Copy Link

Created TODO.md on a branch off next with 10 design questions (with answer placeholders) and a 22-item phased implementation plan.

PR: #11

Once you answer the design questions inline, I can unassign myself from this issue and start working through the implementation items.

Created TODO.md on a branch off `next` with 10 design questions (with answer placeholders) and a 22-item phased implementation plan. PR: https://git.eeqj.de/sneak/mfer/pulls/11 Once you answer the design questions inline, I can unassign myself from this issue and start working through the implementation items.
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
No items
No Label
Milestone
Clear milestone
No items
No Milestone
Assignees
Clear assignees
No Assignees
clawbot
2 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: sneak/mfer#10
No description provided.
Delete Branch "%!s()"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?