feat(sync): envelope-encrypt the namespace-map event log (P4-SEC-02 + P4-SEC-07 foundation)#25
Merged
Merged
Conversation
… P4-SEC-07 foundation) Encrypt event-log payloads at the hub boundary with XChaCha20-Poly1305 under a per-epoch Workspace Content Key (WCK) wrapped with age to approved device recipients. The hub stores only enc.v1 ciphertext carriers; the signed carrier (ID/DeviceID/Seq/HLC/DeviceSig) stays plaintext so ordering, dedup, and Ed25519 verification are unchanged. init bootstraps epoch 1; devices approve grants all held epochs; devices revoke/lost rotates for go-forward forward secrecy; Pull ingests grants in HLC order then decrypts. Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
Address PR #25 review findings. Pull previously returned an error on the first enc.v1 event it could not decrypt or whose epoch it did not hold; the sync caller does `return err`, so a single un-decryptable object (wrong-key cross-device epoch collision, corruption/forgery, an unknown envelope version, or an unexpected plaintext/downgrade event) aborted the whole batch and never advanced the pull cursor — permanently wedging that device's sync and never reaching ApplyEvents' quarantine + safe-cursor machinery. That is the exact self-DoS the untrusted-hub model must resist. Pull now degrades instead of aborting: - missing epoch key (grant not yet propagated) truncates the batch — the decryptable prefix is returned and applies, the cursor advances up to but not past the event, and the next sync retries once the grant arrives; - a held-epoch decrypt failure, a malformed/unknown envelope, and a non-grant plaintext event (anti-downgrade) are each skipped with a loud warning and Pull continues. Bad events are still never applied (no unauthenticated data enters the log), but one bad object can no longer brick a device. This also de-fangs the acknowledged P4-SEC-07 epoch-collision case (log + skip, not wedge) and removes the anti-downgrade brick for a stale/pre-envelope hub. Rewrote the anti-downgrade/missing-epoch/unknown-version tests to assert the skip/truncate contract and added TestEncryptedHubPoisonEventDoesNotWedge. Updated spec/07, spec/16, spec/18. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughThis PR implements zero-knowledge envelope encryption for the workspace event log. It adds enc.v1 event encryption primitives, a workspace keyring for epoch-based Workspace Content Keys (WCK), an EncryptedHub decorator wrapping hub Push/Pull, storage migrations, CLI grant/rotate/bootstrap wiring, e2e test scripts, and supporting documentation. ChangesWorkspace content key envelope encryption
Sequence Diagram(s)sequenceDiagram
participant DeviceA
participant Keyring
participant EncryptedHub
participant Hub
participant DeviceB
DeviceA->>Keyring: EnsureBootstrap (mint epoch 1 WCK)
DeviceA->>EncryptedHub: Push(events)
EncryptedHub->>Keyring: WCK(epoch)
EncryptedHub->>Hub: store enc.v1 carriers
DeviceA->>Keyring: GrantAllEpochs(DeviceB recipient)
DeviceA->>EncryptedHub: Push(grant events)
EncryptedHub->>Hub: store grant events
DeviceB->>EncryptedHub: Pull(afterHLC)
EncryptedHub->>Hub: fetch events
EncryptedHub->>Keyring: IngestGrant(device.key.granted)
EncryptedHub->>DeviceB: decrypted plaintext events
DeviceA->>Keyring: Rotate (on revoke DeviceB)
Keyring-->>DeviceA: epoch+1 grants to remaining devices
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
internal/cli/hub.go (1)
114-116: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winMake the remote hub ID include the backend identity.
This value is described as the per-hub cursor key, but both
r2://bucket-aands3://bucket-bfor the same workspace collapse tor2:<workspace_id>. Repointing a workspace at a different bucket or endpoint would therefore reuse the old cursor against a different event log and can skip unseen history. Include at least scheme + bucket (and endpoint override if it changes the namespace) in the hub ID.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/cli/hub.go` around lines 114 - 116, The remote hub ID returned from the hub creation path in hub.R2Hub should not be keyed only by WorkspaceID, because that collapses different backends into the same cursor namespace. Update the hub ID construction in the logic that returns hub.R2Hub to include backend identity information such as the scheme and bucket, and any endpoint override when it changes the namespace, so rerouting a workspace to another remote does not reuse an old sync cursor.internal/cli/devices.go (1)
133-145: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winValidate the device keys before marking it approved.
enroll --approverefuses approval without a signing key, butdevices approvesetstrust_state='approved'first and only then tries the best-effort WCK grant. That lets an operator create an approved device that cannot be signature-verified and may not even have an age recipient to receive grants. Mirror the approval preconditions here and fail beforeSetDeviceTrustState.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/cli/devices.go` around lines 133 - 145, The devices approval flow in `SetDeviceTrustState` is missing the same preconditions enforced by `enroll --approve`, so it can mark a device approved before confirming it has the required signing key and age recipient. Update the approval path in the device command handler to validate the device keys first, and return an error before calling `store.SetDeviceTrustState` when those prerequisites are not met. Keep the WCK grant in `grantWorkspaceKeyToApprovedDevice` as a follow-up only after validation succeeds and approval is actually applied.
🧹 Nitpick comments (1)
cmd/devstrap/testdata/script/sync_encrypted.txtar (1)
46-55: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winForward-secrecy claim isn't actually verified.
The script rotates the WCK on revoke and re-checks that the hub still stores ciphertext, but never proves the actual security property: that
dev_bcan no longer decrypt content written under the new epoch. Consider adding a post-rotation step where device A adds new content and syncs, then asserting device B (using its now-superseded epoch key) cannot decrypt/materialize it.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/devstrap/testdata/script/sync_encrypted.txtar` around lines 46 - 55, The sync_encrypted.txtar test only checks that revoke rotates the WCK and keeps ciphertext in the hub, but it does not verify that the revoked device loses access to post-rotation data. Extend the scenario after devices revoke dev_b and the subsequent sync by having dev_a create new content under the rotated epoch, sync it, then attempt to decrypt/materialize that content with dev_b and assert failure. Use the existing devstrap sync/devices revoke flow and the dev_a/dev_b handles in this script to locate the right place to add the post-rotation access check.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/cli/devices.go`:
- Around line 149-156: The revoke/lost flow in devices.go is treating
rotateWorkspaceKeyOnRevoke as best-effort, which can leave future payloads
encrypted under the old WCK epoch. Update the revoke path in the device
trust-state handling to check and propagate the error from
rotateWorkspaceKeyOnRevoke, or persist a rotation-required state before
returning success. Make the change in the branch that handles trustState ==
"revoked" || trustState == "lost" so the command does not report a successful
revoke if key rotation fails.
In `@internal/cli/hub.go`:
- Around line 40-45: The hubFromOptions flow now always returns an EncryptedHub,
but it does not ensure a bootstrap path for upgraded single-device workspaces
with CurrentKeyEpoch()==0, so first normal Push can still fail. Update
hubFromOptions to detect the unbootstrapped encrypted case and call the same
bootstrap/repair logic used by init and the approval helper (EnsureBootstrap or
an equivalent explicit upgrade path) before returning dssync.EncryptedHub, so
the wrapped hub is ready for first sync.
In `@internal/cli/init.go`:
- Around line 95-97: The bootstrap path is using a key directory recomputed from
the raw options instead of the normalized one. Update the init flow in `init` so
`buildKeyring` receives and uses the cleaned `paths.KeyDir()` produced after
`paths.Home` normalization, matching `ensureLocalDeviceIdentity`. This keeps
`EnsureBootstrap` and the local identity/epoch-1 WCK in the same directory even
when the home path is relative or rewritten.
In `@internal/devicekeys/devicekeys.go`:
- Around line 260-267: Add input validation in FileStore.WriteWCK and the
matching StoreWCK path so secret material is rejected before any
filesystem/keychain write happens. In the WriteWCK flow, after
validateWorkspaceID, also reject epoch <= 0 and require len(wck) == 32 before
os.MkdirAll/os.WriteFile; mirror the same checks in the StoreWCK-related code
path to keep behavior consistent. Use the existing WriteWCK and StoreWCK symbols
to locate both implementations and ensure invalid grants never create orphan
entries or persist unusable keys.
- Around line 260-268: The FileStore.WriteWCK path only sets 0600 on initial
creation, so existing fallback WCK files may retain broader permissions after
rotation. Update WriteWCK (and the wckPath write flow) to explicitly enforce
0600 after the write, either by calling chmod on the target file or by writing
to a temp file and atomically replacing it. Keep the fix localized to
FileStore.WriteWCK so rotated WCK files always end up with the expected
restrictive mode.
In `@internal/state/migrations/00013_workspace_keys.sql`:
- Around line 10-28: Add schema-level validation to prevent invalid WCK epochs
from being stored in the workspace key tables. Update the CREATE TABLE
definitions for workspace_keys and workspace_key_grants in the
00013_workspace_keys migration to enforce a positive epoch value with a CHECK
constraint on the epoch column. Keep the rest of the schema unchanged, and make
sure both table definitions use the same constraint so malformed key/grant
metadata cannot persist.
In `@internal/state/store.go`:
- Around line 1158-1164: Reject non-positive epochs in RecordKeyGrantTx and the
other key-grant writer methods so malformed payloads never reach the database.
Add an epoch > 0 validation at the start of RecordKeyGrantTx, and apply the same
guard to the related writers referenced in the review so they fail fast before
any INSERT OR IGNORE executes. Use the existing Tx method names and keep the
error returned directly from these entry points.
In `@internal/sync/encryptedhub.go`:
- Around line 59-75: Authenticate device.key.granted before it can advance the
active epoch: in EncryptedHub.Pull, verify the sender signature/trust before
calling Keyring.IngestGrant, and reject any grant whose key material differs
from an epoch already held locally. Update the Keyring/IngestGrant path so it
cannot overwrite an existing epoch with different WCK data, and keep the Push
flow (CurrentEpoch/Prime/WCK) relying only on trusted, immutable epoch state.
In `@internal/sync/eventcrypt.go`:
- Around line 191-193: ParseEncryptedEnvelope should reject enc.v1 envelopes
whose epoch is zero or negative, instead of treating them as valid and letting
EncryptedHub.Pull hit the missing-key path. Add a non-positive epoch guard in
ParseEncryptedEnvelope alongside the existing envelopeVersion check, returning
an ErrUnknownEnvelopeVersion-style error with the event ID and epoch details.
Mirror the same validation in EncryptEvent so malformed or impossible hub data
is never emitted in the first place.
In `@internal/sync/events.go`:
- Around line 492-500: The EventDeviceKeyGranted handling currently bypasses
authenticity checks, so forged grants can be ingested by EncryptedHub.Pull and
stored by Keyring.IngestGrant before any verification. Add an authentication
gate for device.key.granted in the event validation path used by
internal/sync/events.go, and make sure EventDeviceKeyGranted is only accepted
after it has been verified or otherwise proven trustworthy. Keep the bootstrap
exception intact, but ensure the grant cannot be applied or recorded unless it
passes the new authenticity check.
In `@internal/workspacekeys/keyring.go`:
- Around line 105-128: Ensure the key epoch transition is serialized and atomic
across Keyring.EnsureBootstrap and Keyring.Rotate, since both mint/store WCKs
without a lifecycle lock and RecordKeyEpoch can ignore concurrent inserts. Guard
the full bootstrap/rotation flow with a single operation mutex or transaction,
and only advance/publish the new epoch after KeyStore.StoreWCK, all grant/audit
writes, and k.Store.RecordKeyEpoch succeed. Also make sure Push only observes
the finalized epoch after the transition is durably committed.
In `@spec/15_SECURITY_THREAT_MODEL.md`:
- Line 132: The threat model text incorrectly says the file-backed hub exists
only for tests, but the shipped CLI still supports the local backend through
`--hub-file` and `hub: file:<path>`. Update the wording in this section to
describe it as a local/non-production backend instead of test-only, keeping the
rest of the `Hub-backend trust model` description aligned with the actual
`internal/cli/hub.go` surface and the production `Cloudflare R2` backend
distinction.
In `@spec/18_WORK_LOG.md`:
- Around line 51-61: Remove the stale `golangci-lint`/`gofmt` follow-up from the
Follow-ups list in `spec/18_WORK_LOG.md` because the same entry already records
those checks as completed validation. Update the work log entry so it stays
internally consistent, keeping the remaining follow-ups (like P4-SEC-07 and
P4-SEC-08) intact and ensuring the `Follow-ups` section reflects only unresolved
items.
---
Outside diff comments:
In `@internal/cli/devices.go`:
- Around line 133-145: The devices approval flow in `SetDeviceTrustState` is
missing the same preconditions enforced by `enroll --approve`, so it can mark a
device approved before confirming it has the required signing key and age
recipient. Update the approval path in the device command handler to validate
the device keys first, and return an error before calling
`store.SetDeviceTrustState` when those prerequisites are not met. Keep the WCK
grant in `grantWorkspaceKeyToApprovedDevice` as a follow-up only after
validation succeeds and approval is actually applied.
In `@internal/cli/hub.go`:
- Around line 114-116: The remote hub ID returned from the hub creation path in
hub.R2Hub should not be keyed only by WorkspaceID, because that collapses
different backends into the same cursor namespace. Update the hub ID
construction in the logic that returns hub.R2Hub to include backend identity
information such as the scheme and bucket, and any endpoint override when it
changes the namespace, so rerouting a workspace to another remote does not reuse
an old sync cursor.
---
Nitpick comments:
In `@cmd/devstrap/testdata/script/sync_encrypted.txtar`:
- Around line 46-55: The sync_encrypted.txtar test only checks that revoke
rotates the WCK and keeps ciphertext in the hub, but it does not verify that the
revoked device loses access to post-rotation data. Extend the scenario after
devices revoke dev_b and the subsequent sync by having dev_a create new content
under the rotated epoch, sync it, then attempt to decrypt/materialize that
content with dev_b and assert failure. Use the existing devstrap sync/devices
revoke flow and the dev_a/dev_b handles in this script to locate the right place
to add the post-rotation access check.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 376e7386-d0ad-4e84-a5e9-357ef739c3b0
📒 Files selected for processing (33)
cmd/devstrap/testdata/script/sync_encrypted.txtarcmd/devstrap/testdata/script/sync_materialize.txtardocs/audits/README.mdgo.modinternal/cli/devices.gointernal/cli/hub.gointernal/cli/init.gointernal/cli/root_test.gointernal/devicekeys/devicekeys.gointernal/devicekeys/devicekeys_test.gointernal/hub/r2.gointernal/state/migrations/00013_workspace_keys.sqlinternal/state/store.gointernal/state/store_test.gointernal/sync/doc.gointernal/sync/encryptedhub.gointernal/sync/encryptedhub_test.gointernal/sync/eventcrypt.gointernal/sync/eventcrypt_test.gointernal/sync/events.gointernal/sync/hub.gointernal/workspacekeys/keyring.gointernal/workspacekeys/keyring_test.gospec/00_START_HERE.mdspec/03_SYSTEM_ARCHITECTURE.mdspec/07_NAMESPACE_AND_SYNC_MODEL.mdspec/09_SECRETS_AND_ENVIRONMENT.mdspec/12_DATA_MODEL_SQLITE.mdspec/13_CLI_DAEMON_API.mdspec/14_MVP_ROADMAP_AND_BACKLOG.mdspec/15_SECURITY_THREAT_MODEL.mdspec/16_TEST_PLAN.mdspec/18_WORK_LOG.md
Comment on lines
149
to
+156
| if trustState == "revoked" || trustState == "lost" { | ||
| // P4-SEC-07: rotate the WCK epoch so go-forward events encrypt | ||
| // under a key the revoked device does not hold. The revoked | ||
| // device is already excluded from ApprovedRecipients (its | ||
| // trust_state was just changed), so Rotate grants the new epoch | ||
| // only to remaining approved devices. Skip silently if no epoch | ||
| // was ever bootstrapped (pre-envelope workspace). | ||
| rotateWorkspaceKeyOnRevoke(cmd.Context(), stderr, opts, store) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🔴 Critical | 🏗️ Heavy lift
Do not make WCK rotation best-effort on revoke/lost.
If rotateWorkspaceKeyOnRevoke fails, the command still reports the device as revoked and future event-log payloads continue under the old epoch. That leaves the revoked device able to decrypt new namespace-map traffic with the WCK it already holds, which breaks the forward-secrecy guarantee this flow is meant to enforce. Treat rotation failure as a command error or persist an explicit “rotation required before next sync” state.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/cli/devices.go` around lines 149 - 156, The revoke/lost flow in
devices.go is treating rotateWorkspaceKeyOnRevoke as best-effort, which can
leave future payloads encrypted under the old WCK epoch. Update the revoke path
in the device trust-state handling to check and propagate the error from
rotateWorkspaceKeyOnRevoke, or persist a rotation-required state before
returning success. Make the change in the branch that handles trustState ==
"revoked" || trustState == "lost" so the command does not report a successful
revoke if key rotation fails.
Comment on lines
40
to
+45
| func hubFromOptions(ctx context.Context, opts *options, store *state.Store, hubFile string) (dssync.Hub, string, error) { | ||
| backend, hubID, err := selectBackendHub(ctx, opts, store, hubFile) | ||
| if err != nil { | ||
| return nil, "", err | ||
| } | ||
| return dssync.EncryptedHub{Hub: backend, Keyring: buildKeyring(opts, store)}, hubID, nil |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Add an upgrade/bootstrap path before returning an encrypted hub.
This now wraps every backend in EncryptedHub, but only init and the approval helper call EnsureBootstrap. An upgraded single-device workspace with CurrentKeyEpoch()==0 still has no WCK for its first normal event sync, so the first Push fails with no CLI path here to repair it. Please bootstrap on first encrypted sync or add an explicit upgrade path before returning the wrapped hub.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/cli/hub.go` around lines 40 - 45, The hubFromOptions flow now always
returns an EncryptedHub, but it does not ensure a bootstrap path for upgraded
single-device workspaces with CurrentKeyEpoch()==0, so first normal Push can
still fail. Update hubFromOptions to detect the unbootstrapped encrypted case
and call the same bootstrap/repair logic used by init and the approval helper
(EnsureBootstrap or an equivalent explicit upgrade path) before returning
dssync.EncryptedHub, so the wrapped hub is ready for first sync.
Comment on lines
+95
to
+97
| kr := buildKeyring(opts, store) | ||
| if _, err := kr.EnsureBootstrap(cmd.Context()); err != nil { | ||
| return fmt.Errorf("bootstrap workspace key: %w", err) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Bootstrap is using the pre-normalized key directory.
paths.Home is normalized on Lines 43-47 and ensureLocalDeviceIdentity uses that cleaned path, but buildKeyring(opts, store) recomputes opts.paths().KeyDir() from the original options. With a relative or rewritten home, init can store the local identity and epoch-1 WCK in different directories, and later syncs will not be able to reload the WCK. Thread the normalized paths.KeyDir() into keyring construction here.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/cli/init.go` around lines 95 - 97, The bootstrap path is using a key
directory recomputed from the raw options instead of the normalized one. Update
the init flow in `init` so `buildKeyring` receives and uses the cleaned
`paths.KeyDir()` produced after `paths.Home` normalization, matching
`ensureLocalDeviceIdentity`. This keeps `EnsureBootstrap` and the local
identity/epoch-1 WCK in the same directory even when the home path is relative
or rewritten.
Comment on lines
+260
to
+267
| func (s FileStore) WriteWCK(workspaceID string, epoch int64, wck []byte) error { | ||
| if err := validateWorkspaceID(workspaceID); err != nil { | ||
| return err | ||
| } | ||
| if err := os.MkdirAll(s.Dir, 0o700); err != nil { | ||
| return fmt.Errorf("create device key directory: %w", err) | ||
| } | ||
| if err := os.WriteFile(s.wckPath(workspaceID, epoch), []byte(base64.StdEncoding.EncodeToString(wck)+"\n"), 0o600); err != nil { |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Validate epoch and WCK length before persisting secret material.
StoreWCK can be called with a grant-derived epoch before state records the epoch. Reject epoch <= 0 and len(wck) != 32 here as well, so invalid grants cannot leave orphan keychain/file entries or persist unusable keys.
Also applies to: 291-302
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/devicekeys/devicekeys.go` around lines 260 - 267, Add input
validation in FileStore.WriteWCK and the matching StoreWCK path so secret
material is rejected before any filesystem/keychain write happens. In the
WriteWCK flow, after validateWorkspaceID, also reject epoch <= 0 and require
len(wck) == 32 before os.MkdirAll/os.WriteFile; mirror the same checks in the
StoreWCK-related code path to keep behavior consistent. Use the existing
WriteWCK and StoreWCK symbols to locate both implementations and ensure invalid
grants never create orphan entries or persist unusable keys.
Comment on lines
+260
to
+268
| func (s FileStore) WriteWCK(workspaceID string, epoch int64, wck []byte) error { | ||
| if err := validateWorkspaceID(workspaceID); err != nil { | ||
| return err | ||
| } | ||
| if err := os.MkdirAll(s.Dir, 0o700); err != nil { | ||
| return fmt.Errorf("create device key directory: %w", err) | ||
| } | ||
| if err := os.WriteFile(s.wckPath(workspaceID, epoch), []byte(base64.StdEncoding.EncodeToString(wck)+"\n"), 0o600); err != nil { | ||
| return fmt.Errorf("write workspace key: %w", err) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify the fallback writer now repairs permissions and has an overwrite regression test.
rg -nP 'func \(s FileStore\) WriteWCK|os\.WriteFile|os\.Chmod|os\.Rename' internal/devicekeys/devicekeys.go
rg -nP 'WriteWCK|wck file permissions|0o600' internal/devicekeys/devicekeys_test.goRepository: Reederey87/DevStrap
Length of output: 1126
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== internal/devicekeys/devicekeys.go =="
sed -n '240,285p' internal/devicekeys/devicekeys.go | cat -n
echo
echo "== internal/devicekeys/devicekeys_test.go =="
sed -n '200,255p' internal/devicekeys/devicekeys_test.go | cat -nRepository: Reederey87/DevStrap
Length of output: 4465
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== devicekeys.go relevant section =="
sed -n '46,220p' internal/devicekeys/devicekeys.go | cat -nRepository: Reederey87/DevStrap
Length of output: 7162
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "WriteWCK\(" internal . -g '*.go'Repository: Reederey87/DevStrap
Length of output: 779
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '285,320p' internal/devicekeys/devicekeys.go | cat -nRepository: Reederey87/DevStrap
Length of output: 2102
Force 0600 after writing fallback WCK files. os.WriteFile(..., 0o600) only sets mode on creation; rewriting an existing file can leave it readable if it was created with broader permissions. Add a chmod or atomic replace path so rotated WCK files always end up 0600.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/devicekeys/devicekeys.go` around lines 260 - 268, The
FileStore.WriteWCK path only sets 0600 on initial creation, so existing fallback
WCK files may retain broader permissions after rotation. Update WriteWCK (and
the wckPath write flow) to explicitly enforce 0600 after the write, either by
calling chmod on the target file or by writing to a temp file and atomically
replacing it. Keep the fix localized to FileStore.WriteWCK so rotated WCK files
always end up with the expected restrictive mode.
Comment on lines
+191
to
+193
| if env.Version != envelopeVersion { | ||
| return env, fmt.Errorf("%w: event %s has envelope version %d, want %d", ErrUnknownEnvelopeVersion, event.ID, env.Version, envelopeVersion) | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Reject non-positive epochs in the envelope.
ParseEncryptedEnvelope currently accepts epoch <= 0 as a valid enc.v1 payload. In EncryptedHub.Pull, that falls into the “missing key” path and truncates the batch forever, because no legitimate bootstrap/rotate flow can ever mint epoch 0 or a negative epoch. Please fail these envelopes closed here, and mirror the same guard in EncryptEvent, so malformed hub data is skipped instead of wedging sync.
Suggested guard
if env.Version != envelopeVersion {
return env, fmt.Errorf("%w: event %s has envelope version %d, want %d", ErrUnknownEnvelopeVersion, event.ID, env.Version, envelopeVersion)
}
+ if env.Epoch < 1 {
+ return env, fmt.Errorf("parse envelope event %s: invalid epoch %d", event.ID, env.Epoch)
+ }
return env, nil
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if env.Version != envelopeVersion { | |
| return env, fmt.Errorf("%w: event %s has envelope version %d, want %d", ErrUnknownEnvelopeVersion, event.ID, env.Version, envelopeVersion) | |
| } | |
| if env.Version != envelopeVersion { | |
| return env, fmt.Errorf("%w: event %s has envelope version %d, want %d", ErrUnknownEnvelopeVersion, event.ID, env.Version, envelopeVersion) | |
| } | |
| if env.Epoch < 1 { | |
| return env, fmt.Errorf("parse envelope event %s: invalid epoch %d", event.ID, env.Epoch) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/sync/eventcrypt.go` around lines 191 - 193, ParseEncryptedEnvelope
should reject enc.v1 envelopes whose epoch is zero or negative, instead of
treating them as valid and letting EncryptedHub.Pull hit the missing-key path.
Add a non-positive epoch guard in ParseEncryptedEnvelope alongside the existing
envelopeVersion check, returning an ErrUnknownEnvelopeVersion-style error with
the event ID and epoch details. Mirror the same validation in EncryptEvent so
malformed or impossible hub data is never emitted in the first place.
Comment on lines
+492
to
+500
| case EventDeviceKeyGranted: | ||
| // P4-SEC-07: record the grant audit row transactionally with the event | ||
| // insert. The secret WCK is ingested into the keychain by the | ||
| // EncryptedHub decorator during Pull (on the recipient device only); | ||
| // this case only records the non-secret membership audit on every | ||
| // device that applies the grant. Grant events are intentionally NOT in | ||
| // mustVerifyEvent so a newly-approved device can ingest its first WCK | ||
| // during the pre-enrollment bootstrap window (SEC-04); they inherit | ||
| // SEC-04 fail-closed trust once that lands. |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Authenticate device.key.granted before accepting it.
EncryptedHub.Pull ingests every grant before application, and Keyring.IngestGrant stores any wrapped WCK addressed to the local recipient. Since this event type is explicitly excluded from mustVerifyEvent, a malicious hub can forge a grant to the device’s public age recipient and overwrite an epoch key locally, which turns all enc.v1 events for that epoch into undecryptable data. The bootstrap path still needs an authenticity gate before these events are ingested or recorded.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/sync/events.go` around lines 492 - 500, The EventDeviceKeyGranted
handling currently bypasses authenticity checks, so forged grants can be
ingested by EncryptedHub.Pull and stored by Keyring.IngestGrant before any
verification. Add an authentication gate for device.key.granted in the event
validation path used by internal/sync/events.go, and make sure
EventDeviceKeyGranted is only accepted after it has been verified or otherwise
proven trustworthy. Keep the bootstrap exception intact, but ensure the grant
cannot be applied or recorded unless it passes the new authenticity check.
Comment on lines
+105
to
+128
| func (k *Keyring) EnsureBootstrap(ctx context.Context) (int64, error) { | ||
| epoch, err := k.Store.CurrentKeyEpoch(ctx) | ||
| if err != nil { | ||
| return 0, err | ||
| } | ||
| if epoch > 0 { | ||
| return epoch, nil | ||
| } | ||
| if err := k.resolve(ctx); err != nil { | ||
| return 0, err | ||
| } | ||
| const firstEpoch = int64(1) | ||
| wck, err := dssync.NewWCK() | ||
| if err != nil { | ||
| return 0, err | ||
| } | ||
| if err := k.KeyStore.StoreWCK(ctx, k.workspaceID, firstEpoch, wck); err != nil { | ||
| return 0, err | ||
| } | ||
| if err := k.Store.RecordKeyEpoch(ctx, firstEpoch); err != nil { | ||
| return 0, err | ||
| } | ||
| k.cacheWCK(firstEpoch, wck) | ||
| return firstEpoch, nil |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Make epoch transitions single-writer and atomic.
EnsureBootstrap and Rotate both mint/store new WCKs outside a lifecycle lock, and Rotate records next as the current epoch before all grant events/audit rows are durable. Because RecordKeyEpoch is INSERT OR IGNORE, concurrent callers can generate different key bytes for the same epoch; because Push uses the max held epoch, any failure after Line 194 leaves this device encrypting under a key other approved devices never received. Please guard the whole transition with one operation mutex/transaction and only publish the new epoch after the grants are recorded successfully.
Also applies to: 178-221
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/workspacekeys/keyring.go` around lines 105 - 128, Ensure the key
epoch transition is serialized and atomic across Keyring.EnsureBootstrap and
Keyring.Rotate, since both mint/store WCKs without a lifecycle lock and
RecordKeyEpoch can ignore concurrent inserts. Guard the full bootstrap/rotation
flow with a single operation mutex or transaction, and only advance/publish the
new epoch after KeyStore.StoreWCK, all grant/audit writes, and
k.Store.RecordKeyEpoch succeed. Also make sure Push only observes the finalized
epoch after the transition is durably committed.
| - no raw Git mirror by default. | ||
|
|
||
| Hub-backend trust model (`HUB-*`): the hub is a **two-plane zero-knowledge store** — (1) a signed, HLC-ordered append-only event log (the namespace map) and (2) a content-addressed encrypted blob store (`age_blob:<sha256>`) for env values and non-git/draft content. Repo content never transits the hub; it rides git's own transport via blobless clone/fetch from each project's existing remote. The backend is pluggable behind one Hub interface: the chosen production backend is **Cloudflare R2** (S3 API, client-side age encryption, namespaced by `workspace_id`) — any S3-compatible store reuses the same interface — and a file-backed local backend exists **only for tests**. Either backend sees only ciphertext plus the signed map — it cannot read code, secrets, or drafts. | ||
| Hub-backend trust model (`HUB-*`): the hub is a **two-plane zero-knowledge store** — (1) a signed, HLC-ordered append-only event log (the namespace map) whose payloads are **envelope-encrypted** (`enc.v1`, XChaCha20-Poly1305 under a per-epoch Workspace Content Key, `P4-SEC-02`/`SEC-07`, shipped) and (2) a content-addressed encrypted blob store (`age_blob:<sha256>`) for env values and non-git/draft content. Repo content never transits the hub; it rides git's own transport via blobless clone/fetch from each project's existing remote. The backend is pluggable behind one Hub interface: the chosen production backend is **Cloudflare R2** (S3 API, client-side encryption, namespaced by `workspace_id`) — any S3-compatible store reuses the same interface — and a file-backed local backend exists **only for tests**. Either backend sees only ciphertext plus the signed carrier map — it cannot read code, secrets, drafts, or event payloads. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
The file-backed hub is not test-only.
This says the local file backend exists “only for tests”, but internal/cli/hub.go still exposes --hub-file and hub: file:<path> for real CLI use. Please rephrase this as a local/non-production backend so the threat model matches the shipped surface. As per coding guidelines, "After the last codebase modification in a session, review every file in spec/ and update each file as needed so the specs remain accurate, complete, and current before handoff."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@spec/15_SECURITY_THREAT_MODEL.md` at line 132, The threat model text
incorrectly says the file-backed hub exists only for tests, but the shipped CLI
still supports the local backend through `--hub-file` and `hub: file:<path>`.
Update the wording in this section to describe it as a local/non-production
backend instead of test-only, keeping the rest of the `Hub-backend trust model`
description aligned with the actual `internal/cli/hub.go` surface and the
production `Cloudflare R2` backend distinction.
Source: Coding guidelines
Comment on lines
+51
to
+61
| Follow-ups: | ||
| - P4-SEC-07 full: workspace-ID pairing across devices (spec/07 §211 anticipates provisioning the same logical `ws_...` id; currently each `init` mints a separate workspace id, and the joining device's bootstrap WCK is overwritten by the origin device's grant on first pull — functional but not the intended shared-workspace model). | ||
| - P4-SEC-08 (hub-side grant verification / anti-replay) remains open. | ||
| - Hub-based WCK recovery for a solo device that loses its keychain (self-grant removed to avoid epoch collision; a re-grant from another device is the recovery path in a multi-device workspace). | ||
| - `golangci-lint run` + `gofmt -w cmd internal` to be run before PR. | ||
|
|
||
| Review fix (subagent review of PR #25) — make `EncryptedHub.Pull` non-wedging: | ||
| - The original `Pull` returned an error on the *first* enc.v1 event it could not decrypt or whose epoch it did not hold, and the caller (`internal/cli/sync.go`) does `return err`, so the whole batch aborted and the pull cursor never advanced. Since `Pull(afterHLC)` only returns events with HLC past the cursor, a single un-decryptable object (wrong-key cross-device epoch collision, corruption, forgery, or an unexpected plaintext/downgrade event) permanently wedged that device's sync and never reached `ApplyEvents`' quarantine + safe-cursor machinery. This is the exact self-DoS the zero-knowledge/untrusted-hub model must resist. | ||
| - `Pull` now degrades instead of aborting: a **missing epoch key** (grant not yet propagated) **truncates** the batch — the decryptable prefix is returned so it applies and the cursor advances up to but not past that event, and the next sync retries once the grant arrives; a **held-epoch decrypt failure**, a **malformed/unknown envelope**, and a **non-grant plaintext event** (anti-downgrade) are each **skipped with a loud `logging.Logger(ctx).Warn`** and Pull continues. Bad events are still never applied (the security property holds — no unauthenticated data enters the log), but one bad object can no longer brick a device. This also de-fangs the acknowledged P4-SEC-07 epoch-collision case (it now logs + skips on the affected device rather than wedging) and removes the anti-downgrade brick for a stale/pre-envelope hub. | ||
| - Tests: rewrote `TestEncryptedHubAntiDowngrade`/`TestEncryptedHubMissingEpoch`/`TestEncryptedHubUnknownVersion` to assert the skip/truncate contract, and added `TestEncryptedHubPoisonEventDoesNotWedge` (good events on either side of a wrong-key epoch-1 poison event still deliver). The typed sentinels remain in use (`ErrMissingWorkspaceKey` still guards `Push`; `ErrUnknownEnvelopeVersion`/`ErrPlaintextEventFromHub` from `ParseEncryptedEnvelope`). | ||
| - Validated: `gofmt -w cmd internal`; `go run github.com/golangci/golangci-lint/v2/cmd/golangci-lint@v2.12.0 run` — 0 issues; `go run ./cmd/spec-drift --base origin/main --head HEAD` — passed; `go test -race ./...` — green (incl. `cmd/devstrap` e2e and `internal/cli`). |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Remove the stale lint/gofmt follow-up.
This entry still lists golangci-lint run / gofmt -w cmd internal under Follow-ups, but the same entry already records them as completed validation a few lines later. Keeping both makes the work log internally inconsistent. As per coding guidelines, "After the last codebase modification in a session, review every file in spec/ and update each file as needed so the specs remain accurate, complete, and current before handoff."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@spec/18_WORK_LOG.md` around lines 51 - 61, Remove the stale
`golangci-lint`/`gofmt` follow-up from the Follow-ups list in
`spec/18_WORK_LOG.md` because the same entry already records those checks as
completed validation. Update the work log entry so it stays internally
consistent, keeping the remaining follow-ups (like P4-SEC-07 and P4-SEC-08)
intact and ensuring the `Follow-ups` section reflects only unresolved items.
Source: Coding guidelines
Reederey87
added a commit
that referenced
this pull request
Jul 1, 2026
… spec recommendations (#28) * docs(audit): sixth-pass design & implementation audit (post-PR-#25) + spec recommendations Sixth-pass audit of trunk 8c739b8 (PR #25 — live R2 hub + envelope-encryption foundation) via a verification-driven nine-dimension multi-agent workflow (review -> adversarial per-finding verification -> synthesis), anchored by six exa-backed best-practice research topics. 43 findings (P1=5, P2=25, P3=13); ~22 candidates were refuted or dropped as duplicates during verification. Headlines: the envelope layer still trusts the hub (P6-SEC-01 unverified grant ingestion; P6-SYNC-01 whole-batch abort wedges the cursor); the now-live hub GC deletes live draft blobs (P6-HUB-01, P6-DATA-01); a universal 2-minute git timeout silently breaks eager materialization of large repos (P6-GIT-01). - Add docs/audits/AUDIT_RECOMMENDATIONS_2026-07-01_PASS6.md (43 findings with file:line evidence, actionable steps, concrete examples, research notes, and a P4/P5 backlog appendix). - Reconcile docs/audits/README.md per convention #3: add the pass-6 index row + open backlog; move shipped P4-SEC-02 to a Recently-shipped section; correct P4-SEC-05 to partial; scope P4-SEC-07 to its open remainder (applies P6-DOC-02). - Weave recommendations into 16 spec files (a "Pass 6 audit recommendations" section each). Apply the pure-doc fixes directly: spec/00 stale sync comment + missing command (P6-DOC-03), spec/13 status block + env-rotate/hub-gc docs (P6-DOC-01), and internal/workspacekeys tracks_code on spec/07/09/15 (P6-DOC-04). Docs/spec-only cycle; no Go changed. Validated: gofmt clean, spec-drift passed, go test -race ./... green (content-staleness tests confirm no command/migration reference dropped). golangci-lint's only hit is a pre-existing, already-nolint'd gosec G115 in the untouched internal/sync/eventcrypt.go (CI's pinned version honors the suppression). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * docs(audit): address review nits — annotate applied P6-DOC-* fixes; refresh pass-4 index count Internal review (APPROVE-WITH-NITS) flagged two ledger-tracking inconsistencies: - The Pass-6 table header said 'all 43 open' while the P6-DOC-* documentation fixes were applied in this PR; annotate P6-DOC-01/03/04 accordingly and reword the header to '40 open; 3 pure-doc fixes applied'. - Refresh the stale Pass-4 index count (was '~19 shipped (PR #20), ~25 open'). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * docs(audit): address CodeRabbit review — sharpen example correctness - spec/07 (P6-SEC-02): widen kid from 32-bit to full sha256 digest (collision resistance) - spec/06 (P6-XP-04): keep Linux D-Bus substring cases; layer sentinels on top (don't swap out) - spec/03 (P6-HUB-04): re-validate retention marker per pull + reject HLC regression (no indefinite cache) - spec/15 (P6-SEC-01): require grant verification before any keyring mutation, even transiently - spec/12 (P6-DATA-06): SUM(trust_state='local')=1, not COUNT (non-null expr counts all rows) - README (ledger): MD028 separator between adjacent dated blockquotes Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
Reederey87
added a commit
that referenced
this pull request
Jul 2, 2026
…WCK (P6-SEC-01a) (#31) EncryptedHub.Pull called IngestGrant on the raw, unverified hub batch before any signature/trust check, so a malicious/zero-knowledge hub could forge a device.key.granted event wrapping an attacker-chosen Workspace Content Key to the victim's own recipient and read all future namespace events — a full break of the PR #25 envelope layer. - Add an EncryptedHub.Verify seam; Pull verifies each grant carrier BEFORE IngestGrant and skips (never ingests) on failure. The refused carrier still flows to ApplyEvents and lands in the P6-SYNC-01 quarantine. - Export (*state.Store).VerifyRemoteEvent (content-hash self-check + trust/ signature verification, mirroring insertEvent's permanent-failure set); hubFromOptions wires it in, so the trust regime is identical to the apply path: fail-closed once any device is approved, P4-SEC-04 bootstrap window before enrollment. Verify == nil preserves prior behavior for unit tests. - Closes P6-SEC-01 step (a). Steps (b) overwrite-refusal and (c) verified-epoch gating land with PR 3/3's (epoch,kid) + founder/join split. - Tests: Pull refuses/ingests/nil-verifier back-compat; VerifyRemoteEvent regime table; content-hash-mismatch rejection; malicious-hub acceptance TestSyncRejectsForgedGrantBeforeWCKIngest (forged grant at epoch 2^40 wrapped to the victim recipient -> CurrentKeyEpoch unchanged, no WCK file written, one quarantine conflict). - Specs 00/07/12/14/15/16 + work log updated. Part 2/3 of the PASS6 hub-trust workstream. Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
This was referenced Jul 2, 2026
Merged
Merged
Merged
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Encrypts the namespace-map event log at the hub boundary (P4-SEC-02) using envelope encryption with a per-epoch Workspace Content Key (P4-SEC-07 foundation).
What changed
internal/sync/eventcrypt.go): XChaCha20-Poly1305 (chacha20poly1305.NewX, 24-byte random nonce) under a 32-byte per-epoch WCK; AAD =event.ID || uint64(epoch). The carrier (ID/DeviceID/Seq/HLC/DeviceSig) stays plaintext so hub ordering, dedup, and Ed25519 verification are unchanged.enc.v1sentinel + typed errors.workspace_keys+workspace_key_grants): epoch metadata + grant audit in SQLite (the wrapped WCK rides the event payload, never SQLite). Schema 12→13.internal/workspacekeys/keyring.go):EnsureBootstrap/GrantAllEpochs/Rotate/IngestGrant/Prime. WCK stored in OS keychain / 0600 file fallback (devicekeys.HybridStore); age-wrapped (X25519) to approved recipients.internal/sync/encryptedhub.go):EncryptedHubwraps FileHub/R2Hub. Push encrypts non-grants; Pull ingests grants in HLC order then decrypts; anti-downgrade on plaintext; blob ops passthrough.hubFromOptionswraps both backends inEncryptedHub;initbootstraps epoch 1;devices approve/enroll --approvegrants all epochs;devices revoke/lostrotates; newdevices recipienthelper.device.key.granted): plaintext on the hub (payload is age-wrapped); recorded inapplyEventTxbut not inmustVerifyEvent(bootstrap chicken-and-egg).What the hub sees
Only
enc.v1ciphertext carriers + signed carrier fields +device.key.grantedevents (whose payloads are age-wrapped WCKs). No paths, remotes, types, or payload content in plaintext.Validation
gofmt -w cmd internalcleangolangci-lint run— 0 issuesgo run ./cmd/spec-drift --base origin/main --head HEAD— passedgo test -race ./...— all greensync_encrypted.txtar): hub stores onlyenc.v1(grep enc.v1+! grepplaintext path/remote); two-device decrypt after enroll+approve; revoke rotates to epoch 2.Follow-ups
ws_id).Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
Summary by CodeRabbit
New Features
Bug Fixes
Documentation