From d5d222d0e411b3c26c36cca4e468263395106b71 Mon Sep 17 00:00:00 2001 From: Ayla Croft Date: Wed, 23 Sep 2026 16:33:55 -0400 Subject: [PATCH] docs: close four gold criteria, and measure the two that were guesses Four of the fourteen open gold criteria needed writing rather than building, and two more needed measuring rather than assuming. Taken together while the rest of the project is still being built out, because each is cheaper now than after another twenty slices of history. build_reproducible: met, for the release. scripts/check_reproducible.sh builds the release twice from a removed _build and compares every file. All 243 modules this project compiles are byte-identical. Three things are not, and the script separates them because they mean different things: releases/COOKIE is a per-release secret that is supposed to differ, and three modules from llm_db, nx and req_llm vary between builds in a way nothing here can fix. The intermediate _build tree is not reproducible and docs/packaging.md now says why: 24 of 243 modules differ in their Dbgi and ExCk chunks while the Code chunk is identical in all 243, ERL_COMPILER_OPTIONS=deterministic takes that to 18 and no further, and mix release strips exactly those chunks. The project already relied on this from the other direction, since CorePolicy hashes stripped beams for the same reason. test_branch_coverage80: not applicable, and measured rather than argued. The criterion is conditional on a FLOSS tool existing for the language. Every Elixir coverage tool wraps Erlang's cover, and cover:analyse/3 has no clause for branch: it raises function_clause. code_review_standards: a Code review section in docs/03-conventions.md saying how review is conducted, the seven things a reviewer must form an opinion on, and the six conditions a change must meet together. Most of it is about evidence rather than style, and it says plainly that approving something a reviewer has not understood is worse than declining to review it. small_tasks: a Good first tasks section in CONTRIBUTING.md. Every task in it came out of the tree rather than being invented to fill the list. security_review: docs/11-security-review.md, dated, scoped, with its method and its findings. Five resolved, each a defect a person found in something that was passing its tests at the time, and four open with what would close each. A review with no findings would be evidence that the review did not happen. Signed-off-by: Ayla Croft --- CONTRIBUTING.md | 43 ++++++++++ docs/03-conventions.md | 63 ++++++++++++++ docs/11-security-review.md | 153 ++++++++++++++++++++++++++++++++++ docs/packaging.md | 95 ++++++++++++++------- scripts/check_reproducible.sh | 78 +++++++++++++++++ 5 files changed, 401 insertions(+), 31 deletions(-) create mode 100644 docs/11-security-review.md create mode 100755 scripts/check_reproducible.sh 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."