Repository navigation
Conversation
…just its programs (#181) Each Windows build lists the SHA-256 of its files that are not executable images in SemanticRouter.Files.dll (deploy/update-file-list.cs write), which vpk signs with the rest of the release. With Updates:Publisher set, the app requires that list to be the publisher's and the package's other files to match it exactly, so a runtimeconfig.json or deps.json changed, added or removed after signing is refused. package-installer.sh checks the packed update against the list before anything is uploaded. Refs #181 Co-Authored-By: Claude <noreply@anthropic.com>
siebe41
left a comment
There was a problem hiding this comment.
Needs changes: the list format and the hash check are sound, but there are security findings about what the check still lets through, and the docs and CHANGELOG claim more coverage than the code gives yet.
Review at bf0cc1d (fresh eyes). CI is green. I pushed nothing.
Security findings held back from this thread
One High and two Medium findings are about packages a feed attacker could still get installed past the publisher check. At least the High one also applies to the released Updates:Publisher code on main, not only to this PR. Under SECURITY.md that belongs in a private report, not a public PR thread, so the details, the reproduction and the proposed fixes are written up for the maintainer outside this thread. Until they're fixed, the "every file" wording in the CHANGELOG and docs should stay out.
Low: nothing proves a vpk-signed SemanticRouter.Files.dll passes before every policy-managed client depends on it
deploy/package-installer.sh:88 (check) compares hashes but never checks that the list was actually signed. The tests fake the signature, and the one real-Authenticode test now stops at "no list".
If Trusted Signing skips this no-code library (a future --signExclude, or a signing quirk), CI is green and every client with Updates:Publisher refuses the release. When AZURE_SIGN_METADATA is set, have the release step check that SemanticRouter.Files.dll has a valid signature from the release certificate (for example with Get-AuthenticodeSignature).
What's fine
- Format reader: reads the PE without loading it, with bounds checks; any unreadable list is refused.
- Fixture test: ties
deploy/update-file-list.cs writeto the app's reader. - Inside
lib/app/: handled correctly: added, removed and changed data files, and a listed file swapped for a PE. - Other platforms: Windows-only, with Linux and macOS behaviour unchanged.
- Cost: hashing is streamed, so it's negligible next to the existing WinVerifyTrust pass.
- Bookkeeping: CHANGELOG Security entry present; no public API touched.
…and the release checks that the list of files is signed The list of files must carry the installer's signature before a release is uploaded, or every desktop app with a publisher policy would refuse it. Co-Authored-By: Claude <noreply@anthropic.com>
|
Review addressed (merged
Verification: build 0 warnings, |
Decisions for Andrew
deploy/update-file-list.cs writeputs the SHA-256 of every published file that is not a Windows executable image into a resource-only library,SemanticRouter.Files.dll, in the app folder. vpk already signs every unsigned PE file with the release's Trusted Signing certificate, so the list gets signed with no change to the signing steps or secrets. The publisher check treats it as the app's own code (it must be the publisher's, not Microsoft's), reads it, and requires the package's other files to be exactly the listed ones with the listed hashes.Alternatives: a Windows catalog (
makecat+.cat), which vpk doesn't sign and which needs CTL parsing orCryptCATAdmin*P/Invoke to read; or a detached signature over the whole.nupkg, which needs a signing step outside vpk (Trusted Signing only signs formats it knows). Both mean more machinery for the same coverage.sq.versionmust be a byte-for-byte copy of the package's.nuspec. vpk writes it intolib/app/after the list is made, so it can't be in the list. I checked v0.5.0's realwin-x64-full.nupkg: the two files are identical.Updates:Publisher. That covers every release up to 0.5.0, but those are unsigned and already refused under this policy, so nothing that worked before stops working.-full.nupkgfiles included (checksumsjob), so P07b can check each update against its attestation. My suggestion for its decision: warn where verification can't run (Linux, attestation API unreachable, a release from before attestations), and refuse where it ran and failed. docs/desktop.md already says plainly that, until then, an update is trusted on GitHub's word.What changed
deploy/update-file-list.cs(new, a .NET 10 file-based app likethird-party-notices.cs):write <publish-dir>writesSemanticRouter.Files.dll. It is a deterministic library with no code (same files, same bytes) and one managed resource,files.json:{"files":{"<path>":"<sha256>"}}.check <full.nupkg>reads the list back from a Velopack package. It fails on a missing list, a file not in the list, a changed or missing file, a duplicate entry, or asq.versionthat isn't the.nuspec.deploy/publish-desktop.sh: forwin-*, runswriteas the last step, so the list includesTHIRD-PARTY-NOTICES.txt.deploy/package-installer.sh: forwin-*, runscheckon the full package right after packing. The CIinstaller (windows)job, the release'sinstaller-windowsjob and the signing dry run all go through this script, so a file added between publish and pack fails the build. Otherwise it would only show up as every policy-managed updater refusing the release.src/SemanticRouter.Desktop/UpdateFileList.cs(new): reads the list (System.Reflection.MetadataPEReader, never loads the assembly) and compares the package's non-executable files with it.UpdatePublisher.RefusesWindows:lib/app/path twice (case-insensitive), because which copy gets checked and which installed would depend on the zip reader.SemanticRouter.Files.dllto be signed by the publisher.HostFileRefuseschecks on top of the list.UpdateControlTests):On_Windows_every_other_file_must_be_as_the_publishers_signed_list_says: an unchanged package passes. Refused: aruntimeconfig.jsonchanged after signing (even in a way the old host-file check allowed), an added file, a removed file, a wrongsq.version, a duplicate entry, no list, an unreadable list, and a list signed by Microsoft instead of the publisher.The_list_of_files_is_read_as_deploy_update_file_list_writes_it: readstests/fixtures/update-file-list/SemanticRouter.Files.dll, which the deploy script wrote, with the app's reader. This ties the two implementations of the format together.installer-windowsjob's "Check the signature" step also takeslib/app/SemanticRouter.Files.dllout of this release's*-full.nupkgand, when the release is meant to be signed, fails unless its signature isValidand from the installer's certificate subject. Unsigned releases get an "unsigned" summary line, as the installer does. docs/releases.md#code-signing says so.Why
#181: with a publisher policy, only the MZ images in
lib/app/were checked on Windows. The loose data files,SemanticRouter.runtimeconfig.jsonandSemanticRouter.deps.jsonmost of all, decide what the self-contained .NET host loads and how, and nothing covered them. #238 added a check of where those two files point. This PR covers the app's data files with a signed list: a data file in the app that differs from what the publisher signed is refused. That is the issue's second "Done when", tested with a package whoseruntimeconfig.jsonwas changed.The third "Done when" (the environment override gated by policy, with a test of
DesktopUpdater.Feed) was already on main:SourceVariableAllowed,Updates:AllowCustomFeed, HTTPS-only feeds.How it was verified
On this PC (Windows):
dotnet test tests/SemanticRouter.Tests -c Release --filter UpdateControlTests: 39/39 passed.dotnet build -c Release: succeeded, 0 warnings.dotnet format --verify-no-changes: clean.dotnet test -c Release(full suite,VSTEST_CONNECTION_TIMEOUT=900, withOPENAI_API_KEYunset): 1462/1464 passed. The two failures wereServiceTests.The_router_records_itself_while_it_runs_and_reports_an_address_in_useandDesktopTests.The_tray_reads_the_routers_own_status, both socket-bound, timing out with the other sprint lanes loading the machine. Both pass when run alone (2/2), and neither touches the update code.dotnet packwas not run: no packed library changed (the desktop app isn't packable).Against the real v0.5.0 package (downloaded with
gh api …/releases/assets/<id>):lib/app/holds 12 non-PE files. 11 come from the publish (notices,*.xmldocs,deps.json,runtimeconfig.json, import.libfiles, the png). The 12th is vpk'ssq.version, which is byte-identical to the root.nuspec.SemanticRouter.exe,Squirrel.exeandhostfxr.dll.writelisted 11 files. Running it twice gave identical bytes. .NET loads the result (AssemblyName.GetAssemblyName→SemanticRouter.Files, Version=1.0.0.0, resourcefiles.json).checkpasses on the repacked package (withsq.version). It fails with a specific message on each of six variants: runtimeconfig changed, file added, file removed,sq.versionchanged, duplicate entry, no list.After the review (3ef4177)
dotnet build -c Release: 0 warnings.UpdateControlTests: 39/39.dotnet format --verify-no-changes: clean.OPENAI_API_KEYunset): 1463/1464. The one failure,ServiceTests.The_router_records_itself_while_it_runs_and_reports_an_address_in_use, is the shared-stop-event bug Stop only the Windows router a record names, which ends a Windows test flake (#225) #335 fixes; it passes alone.installers/win-x64folders that hold the previous release's full package as well: unsigned → exit 0 with two "unsigned" summary lines;SIGNING=signedwith an unsigned list → fails naming the expected subject; a list signed by the installer's certificate (this PC'sdotnet.exestanding in for both) → passes; no package for this version → fails. A real signed run is still impossible here (A release publishes NuGet packages, the image and a public GitHub release before the installers are signed, and the signing path has never run #220).Other platforms and the workflows
writeandcheckonly run forwin-*rids. Those are published and packed onwindows-latestunder bash, anddotnet run --fileworks the same everywhere. macOS updates are checked bycodesign --deep --stricton the bundle, and Linux updates are refused under the policy; neither uses the list.installer-build(win-x64, no secrets) writes the list as part ofpublish-desktop.sh.installer-windowspacksPUBLISH_DIR, vpk signs the list with the other unsigned PE files (--azureTrustedSignFile), andcheckthen runs against the signed package before anything is uploaded. Signing changes only PE files, and the list covers only non-PE files, so signing can't invalidate it.Verifyruns, so the check always sees the full set of files.checkstep in CI'sinstaller (windows)job runs on the next CI run of this PR.Not done
The review's held-back findings (one High, two Medium): not fixed in this public PR. They are written up for a private report (SECURITY.md); until they are fixed, the docs and CHANGELOG claim the app's data files, not every file.
The default path (no policy): verifying updates against the release's attestation, or a baked-in publisher once Signed, verifiable releases: signatures, provenance, SBOM and checksums #102 lands, plus the warn-vs-refuse decision. That's queued as P07b.
"Once the certificates exist, release builds verify the publisher of every update by default" stays open until the certificates exist (Signed, verifiable releases: signatures, provenance, SBOM and checksums #102).
Revocation: the Authenticode check of the list still doesn't check revocation. That's unchanged and documented.
Refs #181