diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index a742ba7..caf7b2a 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -39,12 +39,55 @@ evidence. The short version: wrong line stays and is corrected below it, saying what it supersedes. - **A failing test comes before the fix.** A test for a claimed property is committed failing first, by name, and the fix commit refers to it. +- **Review has a written standard.** What a reviewer checks, and what makes a change acceptable, is + the *Code review* section of `docs/03-conventions.md`. Read it before opening a pull request: it + is the list your change will be held against, and most of it is about evidence rather than style. - **Major new functionality comes with tests for it, in the same change.** This is a policy, not a preference: a change that adds behaviour and no test for that behaviour is incomplete, and the question a reviewer will ask is which test fails if the change is reverted. A fixed bug gets a regression test naming the defect. The full policy is the *Tests* section of `docs/03-conventions.md`. +## Good first tasks + +Small, self-contained pieces of work suitable for someone who has not worked on this project +before. Each is real: it came out of the tree rather than being invented to fill this list, and +each names where to start. + +**Documentation and records** + +- **Check a document against the code it describes.** Pick any file in `docs/` and verify its + claims against the tree; anything that has drifted is a defect worth a pull request. Start with a + document whose slice number is old, since those have had the most time to drift. +- **Add a worked example to `docs/mcp-server.md`.** Connecting a client to Trinity's protocol + server is documented as configuration; an end-to-end transcript would be more use than another + paragraph. + +**Tests** + +- **Widen a population test.** `test/workflows_test.exs` and `test/spdx_headers_test.exs` each take + their population from `git ls-files`. Other rules in `docs/03-conventions.md` are still enforced + by review alone and could be enforced by a test the same way. Pick one and write it. +- **Add a test for a module below the coverage line.** `mix test --cover` prints per-module + coverage; anything well under the project total is a candidate. Small, useful, and it moves a + number the project tracks in `coverage.tsv`. + +**Code** + +- **Improve a refusal message.** The project's own rule is that a refusal says what was refused and + why, in the channel where it was asked. Some messages say less than that. `Trinity.Gateways.Cap` + and `Trinity.Permissions` are where to look. +- **Fix a `shellcheck` style note in a workflow.** `actionlint` reports two, both in + `.github/workflows/`. Small, visible, and it teaches the build. + +**If you want something larger,** the risk register (`docs/06-risk-register.md`) lists open risks +with their lift conditions, and R26 in particular is an unfixed intermittent test failure with +three candidate fixes recorded and none taken. That one is not a first task, but it is a real one. + +Before starting anything larger than a correction, open an issue describing it, so that two people +do not build the same thing. There is no expectation that a first contribution be large: a +documentation fix that makes the next reader's life easier is a contribution this project wants. + ## Commit messages Conventional Commits, with a scope that identifies the unit of work: diff --git a/docs/03-conventions.md b/docs/03-conventions.md index 058f566..eb1bde5 100644 --- a/docs/03-conventions.md +++ b/docs/03-conventions.md @@ -109,6 +109,69 @@ These are enforced by the gate where a tool can enforce them, and by review wher - **Tests do not reach the network.** Provider calls are mocked against behaviours; an opt-in tag exists for tests that deliberately exercise a real provider. +## Code review + +What a reviewer does, what they check, and what makes a change acceptable. Written down because a +review standard that lives in one person's head is not a standard, and because the first question +a reviewer of *this project* will ask is how its own changes are reviewed. + +### How review is conducted + +Every change reaches `main` through a pull request against a protected branch, with the full gate +green. There is no direct push and no exception for a one-line fix; a change small enough not to +need review is small enough not to need an exception. + +A pull request states what it changes and what it proves. The body carries the evidence: commands +and their output, or named tests and their results. A reviewer who has to run the change to find +out what it does is being asked to do the author's work. + +The reviewer is someone other than the author wherever the maintainer list allows it. Where it +does not, the change is still gated on the full build, and the project says plainly in +`docs/06-risk-register.md` (R22) that this is a gap rather than pretending the gate substitutes +for a second pair of eyes. + +### What must be checked + +A reviewer is responsible for forming an opinion on each of these, not for confirming that a tool +ran: + +1. **Is the change worth making?** Scope first. A change that does something nobody asked for is + rejected however well it is written, and anything found outside the stated scope belongs in a + follow-up rather than folded in quietly. +2. **Does the evidence support the claim?** Every "verified" names its command and its exit code. + A summary of output is not output. Where a test is claimed to prove a property, the reviewer + asks which test fails if the change is reverted, and whether that test could fail at all. +3. **Is the security argument still true?** If the change touches an effect, a permission, a + boundary, a token, a key or anything crossing a trust boundary, the reviewer checks it against + `docs/07-security-model.md` and `docs/10-assurance-case.md`, and rejects a change that makes a + statement in either of those documents false without correcting it. +4. **Does it fail safe?** A new default that permits, a new path that skips the gate, an error + handler that repairs malformed input instead of refusing it: each of these is a defect even + when every test passes. +5. **Is untrusted content still treated as untrusted?** Anything that moves content toward the + model, the prompt or an effect is checked for whether it preserves origin and taint. +6. **Do the records stay honest?** Corrections append; a document is not rewritten to look as + though it had always been right. A commit message that describes changes the commit does not + make, or omits changes it does make, is sent back. +7. **Is what is *not* done stated?** A change that leaves a gap and says so is acceptable. One that + leaves a gap silently is not. + +### What makes a change acceptable + +All of the following, together: + +- the gate is green, on the change as it will land rather than on an earlier version of it; +- the evidence in the pull request is reproducible by the reviewer from what is written there; +- major new functionality has tests in the same change, and a fixed defect has a regression test + naming it; +- documents affected by the change are updated in the same change, including the risk register and + the assurance case when the change touches what they claim; +- nothing in the change is outside its stated scope; +- the reviewer can say what the change does without reading the diff twice. + +A change failing any of these is returned with the reason. "Looks fine" is not a review, and +approving something a reviewer has not understood is worse than declining to review it. + ## Rules of evidence The standard a change is held to, and the reason the proof sections of this repository can be diff --git a/docs/11-security-review.md b/docs/11-security-review.md new file mode 100644 index 0000000..2cb7dc0 --- /dev/null +++ b/docs/11-security-review.md @@ -0,0 +1,153 @@ + + +# 11: Security review + +**Review of 2026-09-23.** Conducted by the maintainers, against the security requirements in +`docs/07-security-model.md` and the boundary and threat model in `docs/10-assurance-case.md`. +Supported by the tools the build runs on every commit, but the findings below are the ones a +person found, because a review that only reports what a tool reports is a tool run with a date on +it. + +This document is the record, not the certificate. It says what was examined, what was found, what +was done about each finding, and what remains open. A review with no findings would be evidence +that the review did not happen. + +## Scope + +**In scope.** The software in this repository: the agent core, the permission gate and effect +membrane, the receipt chain, the protocol client and server and their authorization, the messaging +gateways, the web interface, the build and its supply chain. + +**Out of scope, and why.** The host operating system, the model provider's conduct, any external +authorization server a deployment supplies, and physical access to the machine. Each is named as an +assumption in `docs/10-assurance-case.md`; a review cannot establish properties of things it does +not control, and claiming otherwise would weaken the parts it can establish. + +## Method + +1. The security requirements were read as claims and each was traced to the code that implements + it and the test that holds it. Claims C1 to C10 in the assurance case are the result. +2. The trust boundary was walked in both directions: what crosses inward and where it is labelled; + what crosses outward and what decided that it could. +3. The threat model's five adversaries were taken in turn and the tree examined for what each would + try. This is where the findings below came from. +4. Tool output was read as input to the review rather than as its conclusion: the compiler with + warnings as errors, the architectural boundary checker, Credo in strict mode, Sobelow, the + dependency and licence audits, and a CycloneDX bill validated against the specification. + +## Findings + +Ordered by what they cost, not by when they were found. Every one of these is a defect a human +found in something that was passing its tests at the time. + +### F1. Release-only compile failure, shipped and tagged (resolved) + +**Severity: high. Class: build integrity.** A plug's `authorize:` option was an anonymous function. +`Plug.Builder` calls `init/1` at compile time under `MIX_ENV=prod` and escapes the result, and a +closure cannot be escaped, so the production build failed while every test passed: the gate ran in +`:test`, where `init/1` is per request. The defect reached a tag before anything caught it. + +**Resolved.** Replaced with a captured public function, with a regression test that runs +`Macro.escape/1` over `Plug.init([])`. More usefully, the *class* was closed: `mix gate` now runs +`scripts/prod_check.sh`, which compiles `:prod`, assembles a release and evaluates it, so a +production-only defect fails on the branch that introduces it. + +**What it says about the review:** the gap was not in the code but in what the build exercised. A +test suite that never runs the configuration a user gets is not testing the product. + +### F2. Pairing codes from a non-cryptographic generator (resolved) + +**Severity: medium. Class: authentication.** Gateway pairing codes were generated with +`Enum.random/1`, which draws from a pseudo-random source that is not cryptographically secure. A +pairing code is what stands between an unknown account and a session. + +**Resolved.** `:crypto.strong_rand_bytes/1` over a 32-character alphabet, so that `256 rem 32 = 0` +and the mapping is unbiased, with a census test over the distribution. Found by reading the code +against the criterion for cryptographic randomness rather than by any tool. + +### F3. An architectural boundary that was never enforced (resolved) + +**Severity: medium. Class: architectural integrity.** `Trinity.MCP.Auth` declared `deps: []`, +which reads as "this package depends on nothing and can be extracted". It could not: a nested +sub-boundary inherits its ancestors' dependencies, so the declaration was a claim the compiler had +never checked. + +**Resolved.** Declared `top_level?: true`, which makes the compiler enforce it. **The finding that +matters is not the fix:** a declaration that looks like an enforced constraint but is not is worse +than no declaration, because it stops people checking. + +### F4. Workflow tokens broader than the work (resolved) + +**Severity: medium. Class: supply chain.** Two build workflows declared no token permissions at +all and ran with the repository default, which grants read on every scope; a third granted a +package write at the workflow level, where it applied to every job it would ever have. + +**Resolved.** All three declare `contents: read`, with writes declared by the individual job that +needs them, and a test now requires every workflow to declare a top-level block granting no write. +Found by acting on a Scorecard result rather than by reading it. + +### F5. A bill of materials that no consumer could validate (resolved) + +**Severity: low. Class: supply chain.** The first CycloneDX bill this project generated was not +valid CycloneDX. The generator emits a dependency's licence string as an SPDX identifier without +checking that SPDX defines it, and one dependency spells `BSD 2-Clause` where SPDX has +`BSD-2-Clause`. One field in one component of 132 invalidated the whole document. + +**Resolved.** Identifiers SPDX does not define are recorded as licence names, which is where +CycloneDX puts a licence it cannot resolve, against a vendored copy of the enumeration the +validator itself uses. Found by running the official validator instead of asserting the format. + +## Open + +Recorded here and in `docs/06-risk-register.md` rather than closed. Each names what would close it. + +### O1. No egress restriction by resolved address + +**Severity: medium. Class: server-side request forgery.** Every web fetch passes the permission +gate, so a person decides each one and the decision binds the exact arguments. There is no egress +allow-list and no check on the address a host resolves to, so an approved fetch to a host resolving +to a private or link-local address is not blocked by construction. `Trinity.NetworkGuard` blocks +outbound connections during the test suite and **is not a production control**; an earlier draft of +the assurance case described it as one, and that was corrected. + +**Closes when:** the fetch tool resolves the host and refuses private, loopback, link-local and +unique-local destinations unless a deployment opts in, with a test that a redirect to such an +address is refused as well as a direct request. + +### O2. An advisory in a transitive dependency of the desktop shell + +**Severity: low, scoped. Class: dependency.** `glib` 0.18.5 carries GHSA-wrw7-89jp-8q8g. It is +transitive through the GTK stack the desktop framework requires on Linux and is not fixable by a +direct bump. **Scope was established rather than assumed:** the headless release ships no desktop +shell, so a server deployment does not carry it. Recorded as R25. + +**Closes when:** a desktop framework release resolves `glib` 0.20 or later. + +### O3. Releases are not signed + +**Severity: medium at first release, none today. Class: distribution.** Every packaged artifact +carries build provenance attested through the build's own identity and recorded in a public +transparency log, which establishes where bytes were built. Nothing establishes that the project +intended to publish them, because the project publishes no releases yet. + +**Closes when:** the release pipeline signs artifacts with a project-held key whose private half is +not on the distribution host, with a documented verification procedure. Until then, publishing a +release would create the exposure rather than reveal it. + +### O4. One reviewer + +**Severity: medium. Class: process.** Every commit in the tree has one author. The build is the +only thing that has reviewed most changes, and a build cannot judge whether a change is worth +making. Two further maintainers now hold review and merge rights; the history does not yet show +them using it. + +**Closes when:** reviews and merged changes from a maintainer other than the author appear in the +git history. Recorded as R22. + +## What this review does not establish + +It does not establish the absence of vulnerabilities, and no review does. It establishes that the +security requirements were read as claims, that each was traced to the code and the test that hold +it, that the boundary was walked in both directions, and that the findings above were acted on. The +next review should begin by checking whether the open items above are still open, and whether the +five findings stayed fixed. diff --git a/docs/packaging.md b/docs/packaging.md index 09960ea..cf66b7f 100644 --- a/docs/packaging.md +++ b/docs/packaging.md @@ -215,34 +215,67 @@ from inside the mode). ## Build reproducibility, measured -Measured 2026-09-23, on this machine, for the OpenSSF Best Practices criterion `build_repeatable`, -which asks whether repeating the build from the same sources gives the same bit-for-bit result. -Recorded here because the answer is "partly", and a criterion answered "partly" is worth a -measurement rather than an adjective. - -**Method.** `MIX_ENV=prod mix compile --force` twice from the same tree, with the sha256 of every -`.beam` in `_build/prod/lib/trinity/ebin` compared between the two runs, and each differing file -then compared chunk by chunk with `:beam_lib.all_chunks/1`. - -**Result.** - -| | | -|---|---| -| Modules compiled | 243 | -| Byte-identical across two builds | 219 | -| Differing | 24 | -| Chunks responsible | `Dbgi` (debug info) in 18, `ExCk` (Elixir compile metadata) in 3; three more differ in container layout alone | -| **`Code` chunk, the executable bytecode** | **identical in all 243** | - -**What that means.** The compiled behaviour of this project is reproducible: recompiling the same -sources produces the same instructions every time. What is not reproducible is the metadata the -compiler attaches beside them, which is a known property of the BEAM toolchain rather than -something this project introduced, and which this project already had to account for elsewhere: -`Trinity.CorePolicy` hashes **stripped** beams precisely because the `Dbgi` chunk varies between -builds and between virtual machines, so a policy hash taken over unstripped beams would change -without the policy changing. - -**What is not measured here.** The packaged binary. A Burrito artifact wraps the release with a -launcher and embeds build-time information, and no claim is made that two package runs produce the -same bytes. That claim belongs with the signed release pipeline, where reproducibility becomes -something a third party might want to check rather than an internal property. +Measured 2026-09-23 for the OpenSSF Best Practices criteria `build_repeatable` (silver) and +`build_reproducible` (gold), which ask whether independent parties repeating the build from the +same sources get the same bit-for-bit result. Re-runnable by anyone: **`./scripts/check_reproducible.sh`**. +A reproducibility claim that cannot be re-run by the person doubting it is not worth making. + +### The release: reproducible + +Two release builds, each from a removed `_build`, compared file by file across the whole release +tree: + +``` +$ ./scripts/check_reproducible.sh +files in the release: 4561 +files differing: 4 + +expected, by design: + releases/COOKIE (a per-release secret; a fixed one would be the defect) + +non-deterministic dependencies (upstream's, not this project's): + lib/llm_db-2026.9.4/ebin/Elixir.LLMDB.Model.beam + lib/nx-0.13.1/ebin/Elixir.Nx.Defn.Kernel.beam + lib/req_llm-1.24.0/ebin/Elixir.ReqLLM.beam + +check_reproducible: PASS - every module this project compiles is bit-for-bit identical +across two independent builds from a clean tree. +``` + +**All 243 modules this project compiles are byte-identical.** Three things are not, and each is a +different kind of thing, which is why the script separates them rather than reporting one number: + +- `releases/COOKIE` is the Erlang distribution cookie, generated fresh per release. It is a secret + and it is *meant* to differ; a build that produced the same cookie every time would be a security + defect wearing reproducibility as a disguise. +- Three or four modules from `llm_db`, `nx` and `req_llm` vary between builds. The count itself + varies between runs, which says the non-determinism is in those dependencies' compile-time code + generation rather than in anything this project does. Nothing here can fix them; the honest + position is to name them. + +### The intermediate build tree: not reproducible, and why + +`_build` before the release step is a different matter, and the difference is instructive. Two +forced `MIX_ENV=prod mix compile --force` runs leave 24 of 243 modules differing, entirely in the +`Dbgi` (debug info) and `ExCk` (compile metadata) chunks; the `Code` chunk, the executable +bytecode, is identical in all 243. Setting `ERL_COMPILER_OPTIONS=deterministic` reduces that from +24 to 18 and does not close it, because what remains is abstract code carrying map literals whose +ordering follows the atom table, which differs between two separate virtual machines. + +The release is reproducible anyway, and not by accident: `mix release` strips beams by default, and +the chunks it strips are exactly the ones that varied. The project already depended on this fact +from a different direction, long before the criterion was looked at: `Trinity.CorePolicy` hashes +**stripped** beams, because a policy hash taken over unstripped ones would change without the +policy changing. + +### What is not claimed + +**The packaged desktop binary.** Burrito wraps the release with a launcher and embeds build-time +information, and no claim is made that two package runs produce the same bytes. That belongs with +the signed release pipeline, where reproducibility stops being an internal property and becomes +something a third party has reason to check. + +**A build environment pinned for others.** Reproducibility here is measured on one machine with the +toolchain in `.tool-versions`. Making it reproducible *for an independent party* additionally +requires that party to have the same toolchain, which the FIPS container leg already demonstrates +is expressible as an image digest, but which is not yet published as the defined build environment. diff --git a/scripts/check_reproducible.sh b/scripts/check_reproducible.sh new file mode 100755 index 0000000..c30c541 --- /dev/null +++ b/scripts/check_reproducible.sh @@ -0,0 +1,78 @@ +#!/usr/bin/env bash +# SPDX-FileCopyrightText: Sudo Apt Holdings LLC +# SPDX-License-Identifier: Apache-2.0 +# +# Is this project's build reproducible? Builds the release twice from a clean tree and compares +# every file in it, byte for byte. +# +# This is not in `mix gate`: it is two full production release builds, which is minutes rather +# than seconds, and the property it checks changes when the toolchain or a dependency changes +# rather than when a line of application code does. It is here so that the claim in +# docs/packaging.md can be re-run by anyone who doubts it, which is the only thing that makes a +# reproducibility claim worth making. +# +# Usage: ./scripts/check_reproducible.sh [release_name] +# Exit 0 when every file this project produces is identical across the two builds. +set -euo pipefail + +RELEASE="${1:-headless}" +WORK="$(mktemp -d)" +trap 'rm -rf "$WORK"' EXIT + +# The Erlang distribution cookie is generated fresh for every release and is a secret. It is +# *supposed* to differ between builds; a fixed one would be a defect, not reproducibility. +EXPECTED_DIFFERENT='releases/COOKIE' + +build_and_hash() { + local out="$1" + rm -rf _build/prod + # ERL_AFLAGS is cleared for the same reason mix gate clears it: building deps fetches over TLS, + # which OTP's ssl cannot do in FIPS mode. + MIX_ENV=prod ERL_AFLAGS= mix release "$RELEASE" --overwrite >/dev/null + ( cd "_build/prod/rel/$RELEASE" && find . -type f | sort | xargs sha256sum ) \ + | sed 's|trinity-[0-9][0-9.]*|trinity-VSN|g' > "$out" +} + +echo "building $RELEASE (1 of 2)..." +build_and_hash "$WORK/a" +echo "building $RELEASE (2 of 2)..." +build_and_hash "$WORK/b" + +total=$(wc -l < "$WORK/a") +diff <(cat "$WORK/a") <(cat "$WORK/b") | grep '^<' | awk '{print $3}' | sed 's|^\./||' > "$WORK/differing" || true +differing=$(wc -l < "$WORK/differing") + +echo +echo "files in the release: $total" +echo "files differing: $differing" + +# Split what differs into "this project's own output" and everything else, because they mean +# different things: ours is a defect, a dependency's is upstream's, and the cookie is by design. +ours=$(grep -E '/trinity-VSN/' "$WORK/differing" || true) +cookie=$(grep -Fx "$EXPECTED_DIFFERENT" "$WORK/differing" || true) +theirs=$(grep -vE "/trinity-VSN/|^${EXPECTED_DIFFERENT}\$" "$WORK/differing" || true) + +if [ -n "$cookie" ]; then + echo + echo "expected, by design:" + echo " releases/COOKIE (a per-release secret; a fixed one would be the defect)" +fi + +if [ -n "$theirs" ]; then + echo + echo "non-deterministic dependencies (upstream's, not this project's):" + echo "$theirs" | sed 's/^/ /' +fi + +if [ -n "$ours" ]; then + echo + echo "THIS PROJECT'S OWN OUTPUT IS NOT REPRODUCIBLE:" + echo "$ours" | sed 's/^/ /' + echo + echo "check_reproducible: FAIL" + exit 1 +fi + +echo +echo "check_reproducible: PASS - every module this project compiles is bit-for-bit identical" +echo "across two independent builds from a clean tree."