Repository navigation
docs: Define trusted-computer boundary - #59
BenWestgate merged 8 commits into
Conversation
This comment has been minimized.
This comment has been minimized.
BenWestgate
left a comment
There was a problem hiding this comment.
AI-generated review (Claude), posted at the maintainer's request.
ACK b06aedb, nits only.
- Invariant 6 now matches what
create/secret/share/correct/xprvactually do. - Nit: the trusted-computer definition is written out three times (SECURITY.md, model.md, guide.md). Consider one definition in model.md and links elsewhere so they can't drift.
|
On the “trusted computer” duplication nit: keep the short operational definition in all three places. |
|
Release-gate sequencing note: review/merge #57 before final review of this docs PR. #57 and #59 both touch |
59a29bc to
580b8f8
Compare
BenWestgate
left a comment
There was a problem hiding this comment.
AI-generated review, posted at the maintainer's request.
ACK 580b8f8.
|
Agent release-gate review at exact head |
580b8f8 to
ede98a8
Compare
BenWestgate
left a comment
There was a problem hiding this comment.
Codex current-head re-review: ACK ede98a8.
The security documentation now defines “trusted computer” consistently, distinguishes intentional recovery/export output from accidental secret disclosure, and warns that shell command text can persist even when stdin keeps secrets out of argv. The CLI footer matches that contract and the one prior help-text nit is resolved. Exact-head Python-package run 643 and Bitcoin Core wallet-fixture run 30 succeeded.
No code or security-contract blocker found; ready for human integration review after #23.
BenWestgate
left a comment
There was a problem hiding this comment.
Codex current-head review at ede98a8: no findings. The trusted-computer/offline boundary and secret-output exceptions are made explicit without weakening argument/logging protections.
BenWestgate
left a comment
There was a problem hiding this comment.
AI-assisted current-head review: ACK ede98a8. The trusted-computer definition is consistent across SECURITY.md, the model, invariants, and user guide; invariant 6 now correctly distinguishes intentional recovery/export output from accidental logging/diagnostics; and the CLI safety footer warns about shell command text and redirected sources without claiming stdin is magically non-retentive. The restack preserves the master-xprv/private-descriptor channel boundaries. Exact-head Python/Core CI is green and all review threads are resolved.
|
@codex review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Codex Review: Didn't find any major issues. Nice work! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
ede98a8 to
818144b
Compare
Should we repeat ourselves, why or why not? |
|
AI-assisted response: A short warning belongs where users act; the technical rationale belongs only in the security model. Keep one actionable sentence in the guide, a brief boundary in SECURITY.md, and link to the model. No repeated technical paragraphs. |
|
@codex review |
|
Codex Review: Didn't find any major issues. Chef's kiss. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
7433ee9 to
c0d9ebf
Compare
Make the secret-output invariant literal about intentional recovery/export output, define the trusted-computer boundary consistently, and warn that shell command text can be retained even when stdin is safe from argv exposure. Update the shared CLI safety footer and its exact-help regression. Validation: focused help regression; Ruff check/format; strict mypy for the parser; git diff --check. The full generic-HRP module still reaches the pre-existing bip32 test dependency tracked by #3/#6 and fixed by #7. fixes #4
Use plain language in security guidance and CLI help. Keep the security model's detailed constraints and correct the help test. refs #4
c0d9ebf to
974657f
Compare
BenWestgate
left a comment
There was a problem hiding this comment.
Remove "until you shut it down" the strongest assurance is an offline PC that never goes back online. Such as CryoStick from Bails.
|
Nit: the trusted-computer definition is written out three times (SECURITY.md, model.md, guide.md). Consider one definition in model.md and links elsewhere so they can't drift. The trusted computer definition should be written in SECURITY.md and linked to by guide.md or summarized in one sentence there. |
Define trusted computer once in SECURITY.md and link the model, invariants and user guide to it. Ground encryption and permanent offline-signing advice in Bitcoin Core v32.0 sources, removing the shutdown-only wording and repeated output explanation. No runtime or reportability changes. Checked the complete diff, relative anchors, upstream source links, policy resolution, line widths and whitespace. Application tests were not run for prose-only edits. Refs #4, #59.
|
AI-assisted update at the maintainer's request, following the merged tool-neutral contribution policy. Addressed the deduplication feedback and the latest offline-computer review in
Checked the complete diff, new local anchors, the three upstream source targets, policy resolution, whitespace and line widths. No application tests were run for this prose-only follow-up. Fresh CI/review must cover this head. The broader guide review and help-snapshot cleanup remain tracked by #194/#195; those two older threads remain open rather than claiming those tasks are complete. |
Require exclusive control, centralize network requirements, and make clear that every sensitive operation uses the trusted computer. Restore the guide's explicit encryption recommendation and the original Python memory limitation. Shorten the malware and intentional-output wording. Timestamped rescans still capture a private descriptor from Core and re-import it over stdin; describe that path rather than removing its secret-channel requirement. Preserve invariant numbering with a link. Checked the exact rescan call path, documentation diff, anchors, line widths, whitespace and policy resolution. No application tests were run for prose-only changes. Refs #4, #59.
Apply the reviewer's active wording while preserving exclusive control and trust in the operating system and other software. Meaning unchanged. Checked the two-line prose diff and line widths; no runtime tests needed. Refs #59.
BenWestgate
left a comment
There was a problem hiding this comment.
LGTM
we will do a polish up later on the SECURITY docs before v1 but they're correct and good enough for now.
f976e44
into
codex/63-private-core-descriptors
Define “trusted computer” once in SECURITY.md and link to it from the user guide, security model, and invariants. This removes three competing versions of the advice and keeps the user guide short.
The definition is this project's operational requirement, informed by Bitcoin Core rather than presented as a quotation. Encryption guidance follows Core v32.0's encryption dialog and wallet documentation: encryption cannot fully protect funds from malware. The offline-signing tutorial supports keeping the signing computer permanently disconnected; “until shutdown” is removed.
The disclosure invariant permits intentional recovery output while prohibiting unrelated disclosures. Timestamped rescans still capture a private descriptor from Core and re-import it over stdin; the invariant now describes that exact path. Computer/network requirements live in SECURITY.md, with invariant numbering preserved through a link. The guide retains its explicit wallet-encryption recommendation, and the original Python memory-limit paragraph is restored. Existing shorter CLI help and its matching test update remain. No runtime behavior or vulnerability-reporting criteria change.
Review size: six files, +39/−26 lines (previously +46/−29). The latest follow-up,
0d4ac62, applies the reviewer's direct wording for exclusive control and software trust.e2a95f1addresses the other wording changes and clarifies the timestamped-rescan secret channel.Validation: inspected the complete diff against the current PR base; checked new relative anchors, the three upstream source targets, policy resolution, line widths, and whitespace. No application tests were run solely for these prose edits. Fresh CI and peer review of this head remain required. Broader user-guide simplification and brittle help snapshots remain tracked by #194 and #195.
Fixes #4. AI tools assisted with this maintainer-directed update.