fix(p2p): harden bootstrap trust - #72
Merged
Merged
Conversation
Require authenticated direct stores and explicit operator enrollment while hardening private keys and trust files across Unix and Windows. Sign local control artifacts, bind them to a protected store generation, and serialize async initialization shutdown. Signed-off-by: trvon <git@trevon.dev>
Deploying yams with
|
| Latest commit: |
435ff22
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://40dad1e1.yams.pages.dev |
| Branch Preview URL: | https://fix-p2p-bootstrap-trust.yams.pages.dev |
There was a problem hiding this comment.
🔵 Needs a closer look
It changes security-critical trust, migration, and platform-specific ACL/key-handling logic across multiple subsystems and should receive final human review.
Pull request overview
Hardens direct P2P bootstrap trust and key handling by requiring writer-authenticated operation stores and explicit operator enrollment, with migration behavior that fails closed on legacy/unsigned history. This fits into the daemon’s direct replication path (memory_sync + p2p manager/protocol) and tightens platform-specific filesystem/ACL enforcement.
Changes:
- Require usable writer-auth manifests for direct transport and propagate authenticated “control artifacts” (journals/checkpoints/history) with signed provenance and a per-store control scope.
- Make cold bootstrap require explicit operator enrollment and migrate peer trust with an enrollment-generation gate that invalidates legacy
pinned_by_operatorauthority. - Add Windows key/ACL hardening and ensure
advapi32linkage is available to daemon and fuzzing consumers.
File summaries
| File | Description |
|---|---|
| tools/fuzzing/meson.build | Links advapi32 on Windows for fuzzing targets that now depend on ACL APIs. |
| tests/unit/memory_sync/memory_sync_config_catch2_test.cpp | Adds tests for rejecting forged/unsigned control artifacts and surfacing legacy unsigned history under writer-auth. |
| tests/unit/daemon/service_manager_catch2_test.cpp | Adds coverage for rejecting insecure/unauthenticated direct P2P config and improperly protected key/trust files. |
| tests/unit/daemon/peer_registry_catch2_test.cpp | Updates tests for operator enrollment semantics and legacy pin migration behavior. |
| tests/unit/daemon/p2p_protocol_catch2_test.cpp | Extends protocol tests to assert operator-enrollment flags flow through trust decisions/handshake. |
| tests/unit/daemon/p2p_manager_catch2_test.cpp | Ensures P2P manager refuses unauthenticated/unrecovered memory-sync services and mismatched identities. |
| tests/unit/daemon/p2p_delta_catch2_test.cpp | Updates cold-bootstrap tests to require explicit operator enrollment (no TOFU-based bootstrap authority). |
| tests/unit/daemon/components/config_resolver_test.cpp | Adjusts config parsing expectations to require writer-auth for direct transport. |
| src/daemon/p2p/peer_registry.cpp | Adds operator_enrollment_generation and gates operator pins on re-enrollment. |
| src/daemon/p2p/p2p_protocol.cpp | Threads pinnedByOperator through trust preflight/commit and handshake results. |
| src/daemon/p2p/p2p_manager.cpp | Requires directP2pReady() (fully recovered + authenticated + identity match) before manager creation. |
| src/daemon/p2p/p2p_delta.cpp | Enforces operator enrollment for cold bootstrap on both send/receive paths. |
| src/daemon/meson.build | Propagates advapi32 dependency on Windows through the daemon library/dep. |
| src/daemon/components/ServiceManager.cpp | Implements owner-only key creation/loading across platforms, authenticated store marker creation, and direct-mode fail-closed gates. |
| src/daemon/components/ConfigResolver.cpp | Disables direct memory_sync when writer-auth requirements are not met. |
| include/yams/memory_sync/memory_sync.h | Introduces signed/verified control artifacts, control scope, and legacy-unsigned-history detection. |
| include/yams/memory_sync/memory_sync_service.h | Exposes readiness checks and propagates legacy-history observation into direct P2P gating. |
| include/yams/memory_sync/memory_sync_config.h | Adds control scope and pluggable key/trust-file readers for permission-checked loading. |
| include/yams/daemon/p2p/peer_registry.h | Updates peer registry API for new enrollment-generation behavior. |
| include/yams/daemon/p2p/p2p_protocol.h | Extends trust/handshake structures to carry pinnedByOperator and adds in-memory enrollment helper. |
| include/yams/daemon/components/ServiceManager.h | Adds test hooks for protected key write/load helpers. |
| include/yams/daemon/components/AsyncInitOrchestrator.h | Adds synchronization to avoid races between start/stop and future installation. |
| docs/p2p.md | Documents authenticated direct setup, manifest format, migration procedure, and enrollment requirement for cold bootstrap. |
Review details
Suppressed comments (2)
src/daemon/components/ServiceManager.cpp:383
- These ACL error messages run for both private keys and trust files, but they currently say "P2P trust file" unconditionally. Using a generic label (e.g., "P2P file") avoids confusing diagnostics when validating the identity/private key.
if ((!ownerSid && ownerOnlyRead && ace->Mask != 0) ||
(!ownerSid && (ace->Mask & writable) != 0)) {
return yams::Error{yams::ErrorCode::Unauthorized,
"P2P trust file ACL grants unsafe external access"};
}
src/daemon/components/ServiceManager.cpp:456
- On POSIX, readProtectedP2pFile is reused for both private keys and trust files, but these error messages mix the two (e.g., returning "P2P trust file..." and then "P2P identity key..." in the same code path). This can make failures ambiguous; consider formatting the message based on ownerOnlyRead (private key vs trust file).
return yams::Error{yams::ErrorCode::Unauthorized,
"P2P trust file must be an effective-user-owned regular file without "
"unsafe permissions"};
}
if (info.st_size <= 0 ||
static_cast<std::uintmax_t>(info.st_size) > static_cast<std::uintmax_t>(maxBytes)) {
closeFile();
return yams::Error{yams::ErrorCode::InvalidArgument,
"P2P identity key is empty or oversized"};
- Files reviewed: 23/23 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+366
to
+371
| const auto* ace = static_cast<const ACCESS_ALLOWED_ACE*>(rawAce); | ||
| auto* sid = const_cast<DWORD*>(&ace->SidStart); | ||
| if (!IsValidSid(sid) || GetLengthSid(sid) > header->AceSize - sidOffset) { | ||
| return yams::Error{yams::ErrorCode::Unauthorized, | ||
| "P2P trust file ACL contains an invalid SID"}; | ||
| } |
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
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
Stacked draft child of #71. This change closes the disclosed direct-P2P bootstrap-trust and private-key protection blockers without merging or deploying anything.
P2pManagerconstructionpinned_by_operatorbits until explicit re-enrollmentopen/fstat/O_NOFOLLOWon Unix and handle-based reparse, owner SID, protected-DACL, and ACE validation on Windowsadvapi32linkage to daemon and fuzz consumersBefore the fix:
connect ?pin=rows could retain false operator authorityP2pManagerinstances over unsigned schema-v3 servicesKnown stack blockers
This draft child is not independently mergeable or deployable. The umbrella still has separately tracked replicated-apply serialization, operator-status semantics, fuzzing, and final live gates.
AI disclosure
Extensive AI assistance was used for implementation, test generation, and adversarial review. All changes were locally inspected and validated; remaining stack blockers are disclosed above.
Stack
fix/p2p-transport-availability/ fix(p2p): harden transport availability #71fix/p2p-bootstrap-trustat435ff22518f206bb77617e312d1962cb573eec38Do not merge or deploy this child independently.