feat(settlement): SessionLease wire contract as a stdlib-only subpackage (BLO-22364) - #10
Merged
Conversation
…package (BLO-22364) SessionLease is the control-plane <-> edge contract: trafficcontrol mints leases, the multicast edge verifies them on the CMSD delivery path (cmd/caddy/sender, util/http). It had no shared home, so it lived as two byte-identical copies -- trafficcontrol blockcast/settlement/session_lease.go and multicast settlement/session_lease.go -- kept in sync by hand. That arrangement drifted twice in two days. Once when review fixes were pushed to an already-merged branch and reached nothing, leaving multicast main missing an IPv6-respelling bypass fix for hours; once again this evening when the trafficcontrol side of a verification-key fix landed and the multicast side did not. Neither drift produced a warning: the copies are separate files in separate repositories, so nothing compares them. multicast-api is the module both sides already require, at the same version, and it depends on neither -- so it can hold the contract without a dependency cycle and without a new repository. This lands the shared copy only. The two existing copies stay until their repositories are moved over, so nothing breaks mid-flight. Placed in its own subpackage rather than the root api package, deliberately. The multicast edge builds for js/wasm, wasip1/wasm and TinyGo, and the root package cannot go there: locker.go imports xsync, util.go imports linkdata/deadlock, and types_persist.go / delivery_persist.go import lib/pq, which TinyGo cannot compile -- the constraint delivery_wasm.go already exists for. Go links per package, so a stdlib-only subpackage is reachable from those targets even though the module requires all three. Verified, not assumed: go/wasip1/js builds pass, and TinyGo 0.41.1 links a main that calls every exported symbol (crypto/x509 and crypto/ecdsa both compile). go list -deps reports no third-party package at all. TestPackageImportsOnlyStdlib guards that property where the mistake would be made. It is hermetic -- no toolchain, no network -- which matters because this module has no CI workflows at all, so a build-based gate would not run. One convenience import here would silently un-build the edge, and the failure would surface in a different repository long after the commit that caused it. The two moved files are byte-identical to trafficcontrol master at 29c49e6729a8 (sha256 91ce2a56... / 991354362a32...), so this commit adds no behavior. The security fixes they carry -- verifier-owned key copies, the one-JSON-value decode -- came from that review history. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
🔗 Paperclip issue: BLO-22364 |
1 similar comment
|
🔗 Paperclip issue: BLO-22364 |
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 85b9e25
Critical Issues (0)
Important Issues (1)
- [tests / gstack]
settlement/stdlib_only_test.go:33— The stdlib-only property is described as load-bearing, but this repository has no CI checks and dependency tests are not run when trafficcontrol or multicast test their own code. Consequently, neither this guard nor the copied security regression suite runs automatically before merge or tag, so a future third-party import or verification regression can still land silently. Add a required workflow that runs at leastgo test ./settlement/...plus the js/wasm and wasip1/wasm builds before treating this repository as the canonical security-contract home.
Suggestions (1)
- [tests]
settlement/stdlib_only_test.go:75—checked == 0cannot exercise the stated guard-the-guard behavior because the currently executing_test.gofile is itself counted. Count at least one non-test.gofile (or specifically requiresession_lease.go) if the intent is to detect removal/renaming of the production package.
Strengths
session_lease.goandsession_lease_test.goare byte-identical to trafficcontrol commit29c49e6729a8; the reviewed SHA-256 values match the PR description.- The
settlementsubpackage boundary is sound:go list -depscontains no third-party packages, and bothGOOS=js GOARCH=wasmandGOOS=wasip1 GOARCH=wasmbuilds passed. Module-level requirements do not force the rootapipackage or its dependencies into this package's build. go test ./settlement/...andgo vet ./settlement/...passed. The full repository test run reaches the new package successfully but remains red in the pre-existingfec/TestESIRangeFromRangeListBorderCasetest.- The package is a reasonable shared home once it receives security-grade CI. A dedicated module would add release coordination without improving the package-level dependency isolation demonstrated here.
- The proposed ordering avoids a runtime behavior window because this PR has no importers. It does temporarily create a third mutable source copy, so multicast#420 must land before tagging and the two consumer migrations should follow immediately rather than allowing independent edits during the transition.
Recommended Action
- Add an enforced CI path for the canonical settlement package before merge/tag.
- Land multicast#420 before tagging this module, then migrate both consumers without an open-ended three-copy interval.
- Tighten the guard-the-guard count opportunistically.
The SessionLease doc comment said "the signed v1 supplier-edge lease from BLO-17643 section 4" and every one of its 17 fields was undocumented. A reader with the file open could not tell what LCUMHOrigin holds, which fields the signature covers, or that the timestamps are nanoseconds -- they had to go find a ticket, and the ticket is not versioned with the code. That matters more now than it did when this was an internal file. This is a wire contract with two independent implementations, so a detail one side treats as incidental is a message the other side rejects. The invariants belong in the field docs, where the mistake would be made. Documented from the implementation, not from memory: - the signed preimage is sha256(domain separator + RecordKind + NUL + canonical JSON), covering every field except RecordDigest and Signature - timestamps are Unix NANOSECONDS and exceed 2^53, so round-tripping them through an ECMAScript number corrupts the preimage - Issue canonicalizes Source/Group through net.ParseIP().String() before signing and the limiter keys on that same form -- the pair is what stops a respelled address from buying a second concurrent lease - MaxBeaconGapNS is pinned, so a minter cannot widen its own billing window - SettlementVersion mismatches fail with ErrUnsupportedSettlement - PublicKey is verifier-owned, and why a retained caller pointer was not - LCUMHOrigin is opaque here: signed and ASCII-checked, never interpreted Two claims were wrong on the first pass and corrected against the code: Lifetime is REJECTED outside its range, not clamped, and the version check is an explicit Verify branch rather than an implicit consequence of signing. The README documents the thing that is invisible from any single file: this module is depended on by both trafficcontrol and multicast and depends on neither, which is why a shared type has nowhere else to live -- and why the edge's wasm/TinyGo constraint makes per-package stdlib-only discipline the rule for anything the edge imports. Documentation only. Verified rather than asserted: parsed both revisions with go/ast, dropped all comments, normalized whitespace, and compared -- 604 tokens each, identical. (A naive // strip would have been wrong here; the file contains spiffe:// literals.) Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 5a26330
Prior Findings Dispositioned (1)
- prior:85b9e25 important 1 — still-present —
README.md:106— The exact head explicitly states that the module has no CI workflows, and GitHub reports zero check runs and zero commit statuses for this SHA. The settlement tests and wasm builds therefore remain advisory local receipts rather than enforced merge/tag gates.
Critical Issues (0)
Important Issues (1)
- prior:85b9e25 important 1 [tests / gstack]
README.md:106— The canonical security-contract package still has no enforced CI path.TestPackageImportsOnlyStdlib, the copied security regression suite, and the wasm builds can all regress without any required check running before merge or tag.- Add a required workflow that runs at least
go test ./settlement/...,go vet ./settlement/..., and thejs/wasmandwasip1/wasmbuilds before making this module the canonical home.
- Add a required workflow that runs at least
Suggestions (1)
- [tests]
settlement/stdlib_only_test.go:75—checked == 0cannot detect removal of every production file because the executing_test.gofiles count themselves. Count non-test.gofiles, or explicitly requiresession_lease.go, if this is intended to guard against production-file removal or build-tag mistakes.
Strengths
- The second commit is documentation-only relative to
85b9e251; the new comments accurately describe lifetime rejection, canonical address handling, signature coverage, nanosecond timestamps, pinned beacon gap, opaqueLCUMHOrigin, and verifier-owned public-key state. go test ./settlement/...,go vet ./settlement/..., and bothGOOS=js GOARCH=wasmandGOOS=wasip1 GOARCH=wasmbuilds passed at the reviewed head.session_lease_test.goremains byte-identical to trafficcontrol29c49e6729a8with SHA-256991354362a32bafa939e4e356427ccfa36aeb175e5ba1051575451bae0c3ba90.- The subpackage boundary remains technically sound, and the staged consumer migration avoids changing runtime behavior in this PR. The dedicated-module alternative would add release coordination without improving the demonstrated package-level dependency isolation.
Recommended Action
- Add enforced CI for the settlement package before merge or tag.
- Land the live multicast fix before tagging, then migrate both consumers without leaving an open-ended three-copy interval.
- Tighten the guard-the-guard count opportunistically.
Addresses both review findings on this PR. Important: the module had no CI at all, so nothing re-ran before a merge or a tag. TestPackageImportsOnlyStdlib, the lease signature and identity-binding regression suite, and the wasm builds were local receipts. Making this the canonical home of a security contract without an enforced path meant a third-party import or a verification regression could still land silently -- which is the same failure mode the re-home exists to end, moved one level up. The new workflow runs go vet, go test, and the js/wasm and wasip1/wasm builds on every pull request and on push to main. Three choices in it are deliberate and documented inline: no paths filter, because a path-filtered REQUIRED check never reports on PRs that miss the paths and blocks them forever; synchronize kept, because the property is "no third-party import crept in" and imports creep in on later pushes, and this is a sub-minute GitHub-hosted job rather than a heavy self-hosted suite; and one job, so branch protection has one context to track. Scoped to ./settlement/... on purpose. `go test ./...` is already RED on main -- fec/TestESIRangeFromRangeListBorderCase fails, predating this branch -- and a gate that is red on day one teaches everyone to ignore it. Widening to ./... is a follow-up that belongs with fixing that test, not with this contract. The workflow still needs to be added to branch protection to actually be required; the file alone only makes the signal exist. Suggestion: the guard-the-guard could not fire. It counted every .go file in the directory, and this test file is itself one of them, so the count could never reach zero while the test was running. It now counts only NON-test files, which is the case that matters: production source renamed, deleted, or excluded by a build tag would leave the import scan with nothing real to inspect and its silence would mean nothing. Both branches are now mutation-verified rather than assumed. Renaming session_lease.go to a _test.go file -- which still compiles, so the test really runs -- fails with "no non-test .go files in this package"; adding a compiling xsync import fails naming that import. Each restores to green. Also adds settlement/README.md: why the package is here at all, the two constraints that are easy to break (stdlib-only, and byte-level details being semantics rather than style), and the rules for changing a contract whose two consumers pin different versions by construction. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 4813466
Prior Findings Dispositioned (1)
- prior:85b9e25 important 1 — still-present —
.github/workflows/settlement.yml:37— The workflow now creates the intended PR/main signal, but this exact tree only triggerspushformain(not tags), the repository exposes no ruleset requiringsettlement, and the exact head has no check run or status. The Actions outage explains the missing receipt, but current-head evidence is still insufficient to prove an enforced merge/tag path.
Critical Issues (0)
Important Issues (2)
- prior:85b9e25 important 1 [tests / gstack]
.github/workflows/settlement.yml:37— The canonical security-contract package still lacks a demonstrated enforced path. The workflow is a useful signal, but it is not currently observable as a required check andpush.branches: [main]does not validate a tag when it is created.- After Actions recovers and this workflow runs green, require the
settlementcontext formain; also add a tag trigger or an equivalent release rule that validates the exact tagged SHA.
- After Actions recovers and this workflow runs green, require the
- [tests / native-codex]
.github/workflows/settlement.yml:70— The package's TinyGo compatibility remains a load-bearing stated constraint, but CI only invokes the standard Go toolchain. Standardjs/wasmandwasip1/wasmbuilds do not prove that TinyGo can compile and linkcrypto/x509,crypto/ecdsa, or future stdlib usage.- Pin the supported TinyGo version and link a small probe that exercises the exported contract, matching the verification procedure documented in
settlement/README.md.
- Pin the supported TinyGo version and link a small probe that exercises the exported contract, matching the verification procedure documented in
Suggestions (1)
- [tests]
settlement/stdlib_only_test.go:53— The non-test count fixes deletion and_test.gorenaming, butgo/parserignores build-constraint semantics: a production file excluded by//go:buildis still parsed and counted. Either evaluate build constraints for the supported targets or narrow the comment at lines 79-82 so it does not claim to catch that case.
Strengths
- The new workflow has least-privilege permissions, avoids the required-check
paths:trap, includessynchronize, and keeps one stable status context. - Exact-head local verification passed:
go test ./settlement/...,go vet ./settlement/...,GOOS=js GOARCH=wasm go build ./settlement/..., andGOOS=wasip1 GOARCH=wasm go build ./settlement/.... - The inert guard now correctly counts non-test production files, and its failure message explains why a vacuous pass is unsafe.
settlement/README.mdclearly records signature-byte invariants, versioning, timestamp precision, address canonicalization, and staged consumer migration rules.- The live multicast drift is closed before this shared copy is tagged or imported, so this PR itself still changes no runtime behavior.
Recommended Action
- Establish and verify the required merge/tag gate after Actions recovers.
- Add TinyGo link coverage for the compatibility claim the package is designed around.
- Correct or implement the build-tag guard claim opportunistically.
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.
Why
SessionLeaseis the control-plane ↔ edge contract: trafficcontrol mints leases, the multicast edge verifies them on the CMSD delivery path (cmd/caddy/sender/session_lease.go,util/http/cmsd_lease.go). It has no shared home today, so it exists as two byte-identical copies kept in sync by hand.That has drifted twice in two days, both times silently:
mainran for hours missing an IPv6-respelling bypass of the one-live-lease limiter.Nothing warns you: separate files in separate repos, so nothing compares them. Only
shasumcatches it, and only if you remember to run it.multicast-apiis the module both sides already require, at the same version, and it depends on neither — so it can hold the contract with no dependency cycle and no new repository:Scope
This lands the shared copy only. Both existing copies stay until their repos are moved over, so nothing breaks mid-flight. Follow-ups, in order:
Why a subpackage, not the root
apipackageThe multicast edge builds for js/wasm, wasip1/wasm, and TinyGo. The root package cannot go there:
locker.gopuzpuzpuz/xsync/v3util.golinkdata/deadlocktypes_persist.go,delivery_persist.golib/pqThat's the constraint
delivery_wasm.goalready exists for. Go links per package, so a stdlib-only subpackage stays reachable from those targets even though the module requires all three.Verified, not assumed:
crypto/x509andcrypto/ecdsawere the real risk and both compile under TinyGo 0.41.1.The guard
TestPackageImportsOnlyStdlibasserts the property where the mistake would be made. It's hermetic —go/parseronly — which matters because this module has no CI workflows at all, so a build-based gate wouldn't run. One convenience import would silently un-build the edge, and the failure would surface in a different repository long after the commit that caused it.Mutation-verified: added a compiling
xsyncimport → test failed naming it → removed → passed.Contents
settlement/session_lease.gomaster@29c49e6729a8, with documentation added (see below)settlement/session_lease_test.go991354362a32bafa…settlement/stdlib_only_test.goREADME.mdCorrection to an earlier version of this description
The first revision of this PR said both files were byte-identical to trafficcontrol master. That was true at
85b9e251and is no longer true ofsession_lease.go, which now carries documentation the trafficcontrol copy does not have.The doc comment there read "SessionLease is the signed v1 supplier-edge lease from BLO-17643 section 4" and all 17 fields were undocumented — a reader had to leave the code to learn what the contract is, and the ticket isn't versioned with the code. For a wire contract with two implementations, that's where divergence comes from. Now documented, from the implementation: what the signature covers, that timestamps are nanoseconds past 2^53, the
net.ParseIP().String()canonicalization that closes the respelling hole, the pinnedMaxBeaconGapNS, and whyPublicKeyis verifier-owned.Still no behavior change, and verified rather than asserted: parsed both revisions with
go/ast, dropped all comments, normalized whitespace, compared — 604 tokens each, identical. (A naive//strip would have been wrong; the file containsspiffe://literals. My first attempt at this check was also wrong —go runswallowed the input file as source, both outputs were empty, and diffing two empty files "passed". Caught and redone with a compiled binary.)Two documented claims were wrong on the first pass and corrected against the code:
Lifetimeis rejected outside its range, not clamped, and the version check is an explicitVerifybranch.Risk
Nothing imports this package yet, so it cannot affect any running system until step 3.
Reviewer notes
multicast-apithe right home, or does a settlement lease belong elsewhere? This is the first security-relevant code in this module — everything else is delivery vocabulary (FState, XML marshalling, ranges, FEC types). A module that previously could not have a signature-verification bug now can. That's a real change in character and the main thing worth pushing back on.go.modis untouched — no new dependencies.