Never make a credential only a person can give, and give it once per provider (hq issue 164, ADR 0337) #298
No Assignees
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: novox/mesh-controller#298
Reference in New Issue
Block a user
Fixes issue 164. Decision: novox/hq ADR 0337 (accepted 2026-10-13 on the operator's Approve).
What changes
internal/catalogue): an offer may say"credential": {"issued-by": "outside"}and must then name no own secret and no tool; any issuer other thansoftwareoroutsideis refused (ADR 0337 §1, widening ADR 0305 §1).Manifest.IssuedOutsideOf,OffersIssuedOutside.SecretFornever makes a value for such a provision. Its one mint path asks the catalogue and answersNotGivenErrorinstead (§2).GivenPairSecretreads such a pair as given only when its row isacceptedand sealed to the current keys of both ends. It never writes; a value the mesh made before the rule, or one sealed under an old key, is not given (§2, §8).remakeSharedrefuses, andSharedSecretForanswers not given, for a provider's own secret issued outside (ADR 0228).RotateSecretrefuses such a provision, naming the accept line (§9).AcceptSecretForProvisionseals one value to every consumer named, to the provider and to the operator, each rowacceptedunder one generation, and says which consumers have no sealing key (§3).GivenPairsOflists the provisions a consumer holds a given value for.outsideis read withGivenPairSecret. While it is not given, the consumer receives no credential file and its machine is not refused (§4). The plan reads aNotGivenErrorfromSharedSecretForthe same way.secret accept <provider's node> <provider module> <provision>gives such a credential once for its provider. It resolves every machine to find each consumer that binds the provision from that provider, seals the value to them all, and sends the holding machines (§3, issue 165). The per-pair form (--provider) is refused for such a provision, naming this line. The value is read from the terminal or--from, never through a verb, and is never printed.link.Waitandinventory.Waitcarryprovision, so a wait names exactly one secret, one setting or one provision (§5). The controller excuses a provision wait only when the module requires the provision, a provider offers it issued outside, and no given value is held for the pair under the current keys (§6). The consumer'sneeds-operatorcondition says "give ", and its summary names the onesecret acceptline for the provider (§7).Not in this PR (follow-up issues)
go/healthand the node-engine must carryprovisionin a wait end to end.issued-by: outside, and their consumers' probes must go (§11, issue 167). Until they declare it, nothing changes on any machine.Tests (fail on main: the new identifiers do not exist there; the replay fails behaviourally)
internal/catalogue/issued_outside_test.go: the offer is read, and every bad shape is refused.internal/inventory/outside_test.go(store tests, listed intestdata/store-tests):cmd/mesh-controller/outside_wait_test.go: the wait is excused only under §6; the condition is in plain words and names the accept line; one accept reaches the consumers its provider answers.cmd/mesh-controller/outside_accept_test.go(store test): a made value is placed before the rule and not after; the per-pair form is refused naming the provider's form; one accept is placed for both consumers.cmd/mesh-controller/replay164_test.go, TestReplay164: the replay, written with only pre-fix code.go run ./cmd/prove R164in mesh-lab says "PROVED: fails before its fix, passes on its fix" (register entry: mesh-lab fix/164-replay).Checks run locally
merge-check.shagainst a throwaway store passed everything except two failures:TestATierPutsABrokenModuleBackAtOnce, which failed under a load of 25 or more and passes 3 of 3 alone.Do not merge without an independent review.
Delivery
novox/mesh-controller@c9dd07f919a2— delivered since 2026-10-13T09:58:59ZDelivery plan — builds build-agent, mesh-controller → ace, g14, novox, shanks; no bus step
Machines
Transitions
The commit's note under
refs/notes/mesh-plankeeps every transition:git log --notes=mesh-plan.Merge check at
cfe63a47Judged: head
cfe63a4738aemerged into trunk1826b68a22b5; read beside it: mesh-catalogf35b06750452, mesh-controller-main1826b68a22b5, mesh-host7a41e120635b, mesh-tools954e1a53eef9mesh/merge-gate: PASS — every machine composes with the change as it did without (4 of 4 compose) (modules: build-agent, mesh-controller)mesh/repo-check: PASS — its merge-check.sh passedDelivery plan — builds build-agent, mesh-controller → ace, g14, novox, shanks; no bus step
Run by the build seat on ace as
build-1791882924477134278(builds --log build-1791882924477134278).Verdict: approve at
cfe63a4738Independent review for issue 164, against ADR 0337 (accepted, version 8). I read the whole diff (18 files), the callers of every changed function, and the media catalogue's current manifests of sonarr, qbittorrent and nzbget.
WHAT I CHECKED, AND WHAT I SAW
Nothing is minted for issued-by outside. SecretFor's one mint path now asks the catalogue first and answers NotGivenError for such a provision, after the held-row read and before MakeWithOperator, so no caller of SecretFor can have a value made. The plan does not call SecretFor for such a need at all: issuedOutsideBy picks GivenPairSecret by the need's own provider module, and GivenPairSecret only reads, never writes. remakeShared refuses a provider's own secret that says issued-by outside, and SharedSecretFor answers NotGivenError for it before it would call remakeShared. RotateSecret refuses such a provision and names the accept line. The store tests TestACredentialIssuedOutsideIsNeverMade and TestASharedOwnSecretIssuedOutsideIsNeverMade show no row and no value for a pair with no row, for a newly bound consumer and after a key change.
A software-issued offer is unchanged (ADR 0305 §7). The parser's software branch is the same code under a new else-if; the plan's SharedOwn and SecretFor branches are untouched for offers that do not say outside. The ADR 0305 tests pass. The one edit to them (internal/catalogue/issued_credential_test.go) changes only the expected words of the refusal of a foreign issuer, from "the one issuer an offer may name" to "the issuers an offer may name"; ADR 0337 §12 asks for those tests unchanged, so this is a small deviation in letter, not in what is checked.
A stored row from before the change is safe. A made row sealed to the current keys is answered by GivenPairSecret as not given ("the value held was made by the mesh"), and the plan places no file for it; the store test covers this. A rollback to main is safe: there is no migration, the rows AcceptSecretForProvision writes use the existing columns with origin accepted and a generation, which main's SecretFor reads as held, and the stored catalogue is read with json.Unmarshal rather than the parser, so main still reads a manifest that says issued-by outside. Main would then mint again for such an offer, which is the pre-fix behaviour, not a failure.
The waiting consumer gets the needs-operator condition naming the accept. checkWait now requires exactly one of secret, setting or provision; checkProvisionWait excuses a provision wait only under §6's three conditions; checkWaiting fills Act with "secret accept ", and the condition's words say "give " with the line in the summary. TestAWaitForAProvisionIsExcusedOnlyWhenTheStoreBearsItOut and TestAWaitForAProvisionNamesTheOneAcceptForItsProvider cover it.
The tests fail on main, and the replay uses only pre-fix code. On main
1826b68the new catalogue, inventory and cmd tests do not compile (IssuedOutsideOf, OffersIssuedOutside, IsNotGiven, Wait.Provision and consumersFrom do not exist there). TestReplay164, laid over main alone, fails on behaviour: "the send placed a credential for tv-api (requests.secret-tv-api), which only a person can give and nobody gave" and "the mesh holds a value for tv-api that nobody gave (origin made)". It uses register, assign, planFor, declarationFor, AcceptSecretForPair and SecretsOf, all on main. mesh-lab #102'sgo run ./cmd/prove R164says PROVED.Runs, niced on the laptop against a throwaway store: with -race, the new and neighbouring tests of internal/catalogue, internal/inventory and cmd/mesh-controller (Outside, OneAcceptForAProvider, Replay164, Wait, Secret, Rotate, Issued, Replay401, Replay201) all pass. Without -race, the whole of internal/inventory, internal/catalogue and internal/link pass. go vet is clean and gofmt lists no changed file. The whole of cmd/mesh-controller also passes without -race (328 s).
FINDINGS, NONE BLOCKING
A. The provider half still carries a pre-rule made value. grantsFor reads SecretsFrom, which returns every pair row with this provider, and puts its for_provider value on the grant as Sealed, without asking whether the offer says outside or whether the row was made. So a row the mesh made before this change is still sent to the provider's machine. ADR 0337 §2 says such a value "is never delivered again". Nothing is affected today, because no manifest says outside yet, but it should be closed before issue 618 makes the six offers say outside: leave made rows of an outside offer out of SecretsFrom (or out of the grant's Sealed), with a store test.
B. Four places judge "issued outside" by the provision's name across the whole catalogue, not by the provider the pair binds: SecretFor and RotateSecret (through issuedOutside), and readWaitFacts and checkProvisionWait (through OffersIssuedOutside); GivenPairsOf also ignores which provider a given row came from. The plan itself uses the bound provider. Today every provision name in both catalogues is offered by one module (the only shared name is route), so nothing changes, but if two modules ever offered one name with different issuers, the mesh would stop making the credential of the one that is not outside. A follow-up should pass the provider module through.
C. When the provider module is assigned to more than one machine, the accept line names "" instead of the machine the consumer binds. ADR 0337 §7 wants the one line for the provider; the consumer's bound provider is known from its resolution. Minor.
D. A design point for issue 618, not for this pull request. The current manifests of sonarr, qbittorrent and nzbget already offer their credential as their own secret (ADR 0158: "credential": {"own": "api-key"} or {"own": "password"}), and each writes the mesh's value into the app before it starts (sonarr's api-key-init writes ApiKey into config.xml; qbittorrent's webui-login-init writes the WebUI password hash; nzbget reads FILE__NZBGET_PASS). So for these providers the mesh's value does work today, and ADR 0337 §11, which moves them to issued-by outside, would replace a working credential with one accept per provider. Issue 618 should be judged against the manifests as they are before anyone builds it.
STATUSES AT THIS HEAD (read 11:36)
mesh/merge-gate: "pending — out of date: judged against trunk
1826b68a22, the trunk is now 404b4409e70e; judged again on a recheck or when it is merged". mesh/repo-check: the same words. mesh/delivery: "pending — checking". Neither required check has a verdict on this head against the current main, and I ran the tests on the base1826b68, not on404b4409. The approval is of the code; ask mesh-delivery.recheck and merge only once both required checks are green and read.Merge check at
cfe63a47Judged: head
cfe63a4738aemerged into trunk404b4409e70emesh/merge-gate: FAIL — headcfe63a4738does not merge cleanly into trunk404b4409e7: in conflict testdata/store-tests (modules: build-agent, mesh-controller)mesh/repo-check: FAIL — headcfe63a4738does not merge cleanly into trunk404b4409e7: in conflict testdata/store-testsDelivery plan — builds build-agent, mesh-controller → ace, g14, novox, shanks; no bus step
Run by the build seat on shanks as
build-1791884212717223933(builds --log build-1791884212717223933).A Servarr app's API key, an indexer's key and a downloader's password are values the provider's software holds; the pair path made a random value for every consumer of them, sealed it to both ends and reported success, and the provider refused it. An offer may now say `"credential": {"issued-by": "outside"}`: SecretFor never makes a value for it, the plan reads such a pair as given or not given and sends the consumer no credential file until a person gives it, without refusing its machine; remakeShared and SharedSecretFor never make a provider's own secret issued outside either. `secret accept <provider's node> <provider module> <provision>` seals one value to every consumer that binds the provision from that provider, the provider and the operator, under one generation; the per-pair form is refused for such a provision, and rotate names the accept. A consumer may wait for a provision's credential: the controller excuses the wait only when the store bears it out, and its needs-operator condition names the one accept line. Adds the replay of issue 164 (TestReplay164), written with only what the controller had before.cfe63a4738toc9dd07f919Verdict: approve at
c9dd07f919This re-reviews the rebase of the head I approved in review 187 (
cfe63a4738) onto main404b4409e7.The rebase changes only testdata/store-tests.
git range-diff 1826b68a22b5..cfe63a47 404b4409e70e..c9dd07f9shows one commit, and its only difference is the context of testdata/store-tests. The file at the new head keeps #296's line "./cmd/mesh-controller TestReplay614ANewSDKPlansTheToolchainAndThenItsBundles" and is followed by this pull request's five lines unchanged: the three internal/inventory store tests, TestOneAcceptForAProviderIsPlacedForEveryConsumer and TestReplay164. Leaving that file aside, the added and removed lines of the pull request's diff are identical before and after the rebase (18 files, 1207 insertions, 26 deletions). The new head's parent is404b4409e7.I ran these niced on the laptop against a throwaway store.
go vetof internal/inventory, internal/catalogue and cmd/mesh-controller is clean.go test ./internal/inventorypasses in full (25 s). With -race, the three new inventory store tests pass. TestReplay164, TestOneAcceptForAProviderIsPlacedForEveryConsumer and the provision-wait tests in cmd/mesh-controller pass on the new base. The findings A to D of review 187 still stand, and none of them blocks the merge.The statuses at this head, read at 11:40, are: mesh/merge-gate "pending — building on shanks since 11:38" (after "waiting in the build queue since 11:38") and mesh/delivery "pending — checking". No mesh/repo-check status is posted yet. Merge only once both required checks are green and read.
Merge check at
c9dd07f9Judged: head
c9dd07f919a2merged into trunk404b4409e70e; read beside it: mesh-catalog45d01e77c26d, mesh-controller-main404b4409e70e, mesh-host7a41e120635b, mesh-tools954e1a53eef9mesh/merge-gate: PASS — every machine composes with the change as it did without (4 of 4 compose) (run 2) (modules: build-agent, mesh-controller)mesh/repo-check: PASS — its merge-check.sh passed (run 2)Delivery plan — builds build-agent, mesh-controller → ace, g14, novox, shanks; no bus step
Run by the build seat on shanks as
build-1791884327303169448(builds --log build-1791884327303169448).