sneak/vaultik
1
0
Fork 0
Code Issues 19 Pull Requests 1 Actions Packages Projects Releases Wiki Activity

Docs misstate the remote layout and understate the privacy guarantee #67

New Issue
Open
opened 2026-08-09 03:41:53 +02:00 by clawbot · 2 comments
clawbot commented 2026-08-09 03:41:53 +02:00
Collaborator
Copy Link

Three documents describe a remote layout and a privacy posture that the
code no longer has. One of them claims a weakness that was
deliberately fixed, which is the serious part: a security-relevant doc
telling users they leak metadata they do not actually leak.

The code

internal/snapshot/remotekey.go:35-41 — RemoteSnapshotKey returns
hex(SHA256(SHA256("vaultik|" + id))), and gates both the
metadata/<remote-key>/ path and the manifest's snapshot_id field.
TODO.md:37 records this landing on 2026-06-26.

The docs

  • docs/REPOSTRUCTURE.md, "Privacy Implications" — claims an observer
    can determine "When backups were taken (from snapshot IDs)" and "Which
    hostname created backups (from snapshot IDs)". Both are false; the
    hashing specifically prevents this. For a tool whose headline feature
    is "modern encryption" (README:41), shipping a doc that understates the
    guarantee is worse than shipping no doc.
  • README:284-291 — draws metadata/<snapshot_id>/db.zst.age, i.e. a
    plaintext ID as the directory name.
  • README:299 — "Snapshot IDs follow the format
    <hostname>_<snapshot-name>_<RFC3339-timestamp> (e.g.
    server1_home_2025-06-01T12:00:00Z)", presented as the on-disk
    directory name.
  • ARCHITECTURE.md:368-371 — same plaintext layout.

Definition of done

  1. docs/REPOSTRUCTURE.md's "Privacy Implications" section accurately
    describes what an observer of the destination store can and cannot
    infer, given hashed snapshot keys. State plainly what is still
    observable (blob count, blob sizes, upload timing, total volume) so
    the section stays honest in both directions rather than swinging to
    overclaiming.
  2. README:284-291 and ARCHITECTURE.md:368-371 show the real layout with
    a hashed key, with a worked example that matches what
    RemoteSnapshotKey actually produces.
  3. README:299 keeps the human-facing ID format but states explicitly that
    this ID is never written to the destination in plaintext, and
    points at the hashing function.
  4. The derivation (double SHA-256 over the vaultik| domain-separated
    ID) is documented once, in one place, and the other two documents link
    to it rather than restating it.
  5. Docs only — no code changes. If writing this up reveals an actual
    metadata leak, stop and file a separate issue rather than papering
    over it.
  6. make fmt run over the changed markdown; make check green.
Three documents describe a remote layout and a privacy posture that the code no longer has. One of them claims a **weakness that was deliberately fixed**, which is the serious part: a security-relevant doc telling users they leak metadata they do not actually leak. ## The code `internal/snapshot/remotekey.go:35-41` — `RemoteSnapshotKey` returns `hex(SHA256(SHA256("vaultik|" + id)))`, and gates both the `metadata/<remote-key>/` path and the manifest's `snapshot_id` field. `TODO.md:37` records this landing on 2026-06-26. ## The docs - `docs/REPOSTRUCTURE.md`, "Privacy Implications" — claims an observer can determine "When backups were taken (from snapshot IDs)" and "Which hostname created backups (from snapshot IDs)". **Both are false**; the hashing specifically prevents this. For a tool whose headline feature is "modern encryption" (README:41), shipping a doc that understates the guarantee is worse than shipping no doc. - README:284-291 — draws `metadata/<snapshot_id>/db.zst.age`, i.e. a plaintext ID as the directory name. - README:299 — "Snapshot IDs follow the format `<hostname>_<snapshot-name>_<RFC3339-timestamp>` (e.g. `server1_home_2025-06-01T12:00:00Z`)", presented as the on-disk directory name. - `ARCHITECTURE.md:368-371` — same plaintext layout. ## Definition of done 1. `docs/REPOSTRUCTURE.md`'s "Privacy Implications" section accurately describes what an observer of the destination store can and cannot infer, given hashed snapshot keys. State plainly what **is** still observable (blob count, blob sizes, upload timing, total volume) so the section stays honest in both directions rather than swinging to overclaiming. 2. README:284-291 and `ARCHITECTURE.md:368-371` show the real layout with a hashed key, with a worked example that matches what `RemoteSnapshotKey` actually produces. 3. README:299 keeps the human-facing ID format but states explicitly that this ID is **never** written to the destination in plaintext, and points at the hashing function. 4. The derivation (double SHA-256 over the `vaultik|` domain-separated ID) is documented once, in one place, and the other two documents link to it rather than restating it. 5. Docs only — no code changes. If writing this up reveals an actual metadata leak, stop and file a separate issue rather than papering over it. 6. `make fmt` run over the changed markdown; `make check` green.
clawbot added this to the 1.0.0 milestone 2026-08-09 03:41:53 +02:00
clawbot commented 2026-08-09 04:56:35 +02:00
Author
Collaborator
Copy Link

STOP — do not work this issue as written. I got a load-bearing fact
wrong, and acting on the current text would put a false claim into a
security document.

What I got wrong

This issue says docs/REPOSTRUCTURE.md is wrong to claim an observer can
determine "When backups were taken", and instructs whoever picks it up to
rewrite that section as a fixed weakness. The timestamp claim is
correct. Backup times are observable.

internal/snapshot/snapshot.go:842-850:

// Create manifest. SnapshotID in the unencrypted manifest is the
// double-SHA256 remote key, not the human ID, so the public bytes
// don't reveal hostname/snapshot-name/timestamp metadata.
manifest := &Manifest{
    SnapshotID:          RemoteSnapshotKey(snapshotID),
    Timestamp:           time.Now().UTC().Format(time.RFC3339),
    ...
}

The comment asserts the public bytes reveal no timestamp; the next line
writes a plaintext RFC3339 timestamp into exactly those bytes. The
manifest is stored at metadata/<remote-key>/manifest.json.zst — zstd
only, no .age, so it is unencrypted at rest and readable by anyone
who can list the destination.

internal/snapshot/manifest.go:15-28 — the full set of unencrypted
fields is snapshot_id (hashed), timestamp (plaintext),
blob_count, total_compressed_size, and per-blob hash +
compressed_size.

Corrected picture

Hashing the snapshot ID protects the hostname and snapshot name.
It does not protect the timestamp, because the timestamp is published
separately in the clear. So of REPOSTRUCTURE.md's two claims:

  • "Which hostname created backups (from snapshot IDs)" — genuinely fixed
    by the hashing.
  • "When backups were taken" — still true, via the manifest, not the
    ID. Right conclusion, wrong stated mechanism.

Revised scope for this issue

  1. Correct the mechanism in REPOSTRUCTURE.md while keeping the
    conclusion that backup times are observable. Do not delete that
    warning.
  2. Enumerate honestly what the unencrypted manifest exposes: backup time,
    blob count, total compressed size, and per-blob hashes and sizes —
    which together give an observer a size-and-timing profile of every
    snapshot.
  3. Fix README:284-291 / :299 and ARCHITECTURE.md:368-371 to show the
    hashed layout, as originally scoped. That part of this issue was
    correct.
  4. Fix the false code comment at snapshot.go:842-844.

Whether the plaintext timestamp is acceptable, or the manifest should be
encrypted, is a separate design question — filed as its own issue and not
to be decided here.

My error: I inferred the privacy posture from remotekey.go's doc
comment (which makes the same overreaching claim) instead of reading what
is actually serialized. In a threat-model document, understating a leak
is the worst direction to be wrong in, so I would rather flag this loudly
than quietly edit the issue body.

**STOP — do not work this issue as written.** I got a load-bearing fact wrong, and acting on the current text would put a false claim into a security document. ## What I got wrong This issue says `docs/REPOSTRUCTURE.md` is wrong to claim an observer can determine "When backups were taken", and instructs whoever picks it up to rewrite that section as a fixed weakness. **The timestamp claim is correct.** Backup times are observable. `internal/snapshot/snapshot.go:842-850`: ```go // Create manifest. SnapshotID in the unencrypted manifest is the // double-SHA256 remote key, not the human ID, so the public bytes // don't reveal hostname/snapshot-name/timestamp metadata. manifest := &Manifest{ SnapshotID: RemoteSnapshotKey(snapshotID), Timestamp: time.Now().UTC().Format(time.RFC3339), ... } ``` The comment asserts the public bytes reveal no timestamp; the next line writes a plaintext RFC3339 timestamp into exactly those bytes. The manifest is stored at `metadata/<remote-key>/manifest.json.zst` — zstd only, **no `.age`**, so it is unencrypted at rest and readable by anyone who can list the destination. `internal/snapshot/manifest.go:15-28` — the full set of unencrypted fields is `snapshot_id` (hashed), `timestamp` (**plaintext**), `blob_count`, `total_compressed_size`, and per-blob `hash` + `compressed_size`. ## Corrected picture Hashing the snapshot ID protects the **hostname** and **snapshot name**. It does not protect the timestamp, because the timestamp is published separately in the clear. So of `REPOSTRUCTURE.md`'s two claims: - "Which hostname created backups (from snapshot IDs)" — genuinely fixed by the hashing. - "When backups were taken" — **still true**, via the manifest, not the ID. Right conclusion, wrong stated mechanism. ## Revised scope for this issue 1. Correct the **mechanism** in `REPOSTRUCTURE.md` while **keeping** the conclusion that backup times are observable. Do not delete that warning. 2. Enumerate honestly what the unencrypted manifest exposes: backup time, blob count, total compressed size, and per-blob hashes and sizes — which together give an observer a size-and-timing profile of every snapshot. 3. Fix README:284-291 / :299 and `ARCHITECTURE.md:368-371` to show the hashed layout, as originally scoped. That part of this issue was correct. 4. Fix the false code comment at `snapshot.go:842-844`. Whether the plaintext timestamp is acceptable, or the manifest should be encrypted, is a separate design question — filed as its own issue and not to be decided here. My error: I inferred the privacy posture from `remotekey.go`'s doc comment (which makes the same overreaching claim) instead of reading what is actually serialized. In a threat-model document, understating a leak is the worst direction to be wrong in, so I would rather flag this loudly than quietly edit the issue body.
clawbot commented 2026-08-09 16:10:34 +02:00
Author
Collaborator
Copy Link

STOP LIFTED — this issue is unblocked and ready to work.

sneak has ruled on #81: the unencrypted manifest's plaintext timestamps
and per-blob size profile are an accepted, previously made design
decision, not a defect. #81 is closed. So the threat model documents
that exposure as intended behaviour; it is not something to fix, and this
issue must not propose fixing it.

Corrected scope — supersedes both the issue body and my earlier STOP

The original body was wrong in one direction and my STOP over-corrected in
another. What is actually true:

  1. docs/REPOSTRUCTURE.md's conclusion was right all along. Backup
    times are observable. Its error is only the stated mechanism — it
    attributes them to snapshot IDs, when in fact IDs are hashed and the
    timestamp is published separately, in the clear, in
    metadata/<remote-key>/manifest.json.zst. Keep the warning; correct the
    mechanism. Do not delete the warning — that is what my original
    text wrongly instructed.
  2. Hashing genuinely protects hostname and snapshot name. That part of
    the original issue was correct: RemoteSnapshotKey is
    hex(SHA256(SHA256("vaultik|" + id))), one-way, and the manifest stores
    the hash rather than the human ID.
  3. State the manifest's exposure plainly and completely, as intended
    behaviour a user can evaluate: backup time, blob count, total
    compressed size, and per-blob hashes and sizes — which together give an
    observer a timing-and-size profile per snapshot. Presented as a
    documented property, not a caveat or an apology.
  4. Note that the timing channel is not closable by encrypting the manifest
    anyway — object creation times and per-object sizes remain visible at
    the storage layer on both s3:// and file://. This is why the
    accepted design is coherent rather than merely convenient.
  5. README:284-291 / :299 and ARCHITECTURE.md:368-371 still show the
    remote layout with plaintext snapshot IDs as directory names. That part
    of the original issue stands unchanged — correct them to the hashed
    layout with a worked example matching what RemoteSnapshotKey
    produces, and make clear the human ID is never written to the
    destination in plaintext.

Carried over from #81: two false code comments

These are genuine defects — comments asserting the opposite of the
adjacent code — and are in scope here as documentation fixes:

  • internal/snapshot/snapshot.go:842-844 claims the public bytes "don't
    reveal hostname/snapshot-name/timestamp metadata", and the very next
    statement writes Timestamp: time.Now().UTC().Format(time.RFC3339) into
    those bytes.
  • internal/snapshot/remotekey.go's doc comment overreaches the same way.

Correct both to describe what is actually written. Comment text only —
no behaviour change. If the code turns out not to match any accurate
description, stop and report rather than changing behaviour under a docs
issue.

Definition of done

  1. Items 1-5 above addressed.
  2. Both false comments corrected.
  3. The derivation (double SHA-256 over the vaultik|-prefixed ID)
    documented once, with the other documents linking to it rather than
    restating.
  4. Docs and comments only — zero behaviour change, and nothing that
    proposes altering the manifest format. That decision is settled.
  5. make fmt over changed markdown; script/cibuild exits 0, verified
    per the three-part recipe (expected ok count, zero (cached)
    markers, plausible wall time).
**STOP LIFTED — this issue is unblocked and ready to work.** `sneak` has ruled on #81: the unencrypted manifest's plaintext timestamps and per-blob size profile are an **accepted, previously made design decision**, not a defect. #81 is closed. So the threat model documents that exposure as intended behaviour; it is not something to fix, and this issue must not propose fixing it. ## Corrected scope — supersedes both the issue body and my earlier STOP The original body was wrong in one direction and my STOP over-corrected in another. What is actually true: 1. **`docs/REPOSTRUCTURE.md`'s conclusion was right all along.** Backup times *are* observable. Its error is only the stated **mechanism** — it attributes them to snapshot IDs, when in fact IDs are hashed and the timestamp is published separately, in the clear, in `metadata/<remote-key>/manifest.json.zst`. Keep the warning; correct the mechanism. **Do not delete the warning** — that is what my original text wrongly instructed. 2. **Hashing genuinely protects hostname and snapshot name.** That part of the original issue was correct: `RemoteSnapshotKey` is `hex(SHA256(SHA256("vaultik|" + id)))`, one-way, and the manifest stores the hash rather than the human ID. 3. **State the manifest's exposure plainly and completely**, as intended behaviour a user can evaluate: backup time, blob count, total compressed size, and per-blob hashes and sizes — which together give an observer a timing-and-size profile per snapshot. Presented as a documented property, not a caveat or an apology. 4. Note that the timing channel is not closable by encrypting the manifest anyway — object creation times and per-object sizes remain visible at the storage layer on both `s3://` and `file://`. This is why the accepted design is coherent rather than merely convenient. 5. **README:284-291 / :299 and `ARCHITECTURE.md:368-371`** still show the remote layout with plaintext snapshot IDs as directory names. That part of the original issue stands unchanged — correct them to the hashed layout with a worked example matching what `RemoteSnapshotKey` produces, and make clear the human ID is never written to the destination in plaintext. ## Carried over from #81: two false code comments These are genuine defects — comments asserting the opposite of the adjacent code — and are in scope here as documentation fixes: - `internal/snapshot/snapshot.go:842-844` claims the public bytes "don't reveal hostname/snapshot-name/**timestamp** metadata", and the very next statement writes `Timestamp: time.Now().UTC().Format(time.RFC3339)` into those bytes. - `internal/snapshot/remotekey.go`'s doc comment overreaches the same way. Correct both to describe what is actually written. **Comment text only — no behaviour change.** If the code turns out not to match any accurate description, stop and report rather than changing behaviour under a docs issue. ## Definition of done 1. Items 1-5 above addressed. 2. Both false comments corrected. 3. The derivation (double SHA-256 over the `vaultik|`-prefixed ID) documented once, with the other documents linking to it rather than restating. 4. Docs and comments only — zero behaviour change, and nothing that proposes altering the manifest format. That decision is settled. 5. `make fmt` over changed markdown; `script/cibuild` exits 0, verified per the three-part recipe (expected `ok` count, zero `(cached)` markers, plausible wall time).
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
merge-ready
All checks pass, reviewed, rebased, ready for merge
 needs-checks
make check does not pass cleanly
 needs-rebase
PR has merge conflicts or is behind main
 needs-review
Code review not yet done
 needs-rework
Code review found issues to fix
No Label
Milestone
No items
No Milestone 1.0.0
Projects
Clear projects
No project
Assignees
Clear assignees
clawbot sneak
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: sneak/vaultik#67
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?