make the signed artifact agree that myuplink is read-only - #65
Conversation
|
Automated maintenance pass: confirmed still blocked, not merging. Checked srcfl/ftw#743 directly — still open, not merged, so the companion host-side change this PR depends on hasn't shipped. Merging here first would ship a No action taken. Re-check once srcfl/ftw#743 lands and ships, and once a maintainer has decided the catalog-convention question above. Generated by Claude Code |
miravoss26
left a comment
There was a problem hiding this comment.
Reviewed the diff. Flips control: false on five drivers (myuplink, nibe_local, pixii_pv, solis_string, tibber) whose driver_command refuses unconditionally, so the catalog Control badge stops promising what the code declines; adds test_control_claims.py to hold the claim to the code; myuplink 1.1.1 → 1.2.0 because the new read_only / auth_post_path metadata rides the signed artifact.
- The security-relevant bit is myuplink's
auth_post_path = "/oauth/token": it narrows the read-only guard to allow POST only to the sign-in path and refuse it elsewhere. I screened the enforcing half on srcfl/ftw#743 already (path-exact, signed-manifest fenced, adversarial tests; the path-not-host match is a non-blocker). This is the consistent driver-side declaration. control: falsematches each driver's actualdriver_command(four say "Read-only: no actuation" on the line above the refusal). Correct.- Correctly self-blocked: don't merge before ftw#743 lands and ships, and the red
driverscheck is expected until then.
Sound from my read, but it's blocked-by-design on ftw#743 and CONFLICTING, so a human owns the sequencing and the merge. Not in my allowlist regardless.
Not ready to merge. Two blockers found while building it, both external to this diff -- recorded here so the work is not lost. What this does: myuplink declares read_only = true and auth_post_path = "/oauth/token" in its DRIVER table. The channel then publishes it read_only with control_enabled false, and the generated guard permits host.http_post only for a URL whose path ends in the declared path, refusing it everywhere else. So the flag enforces "this POST is authentication" rather than asserting it. A path rather than a URL because base_url is config-overridable. Blocker 1 -- FTW rejects it. go/internal/driverrepo/sourceful.go, validateSourcefulReadOnlyContract, allows a read-only package only http.get, modbus.read, mqtt.subscribe, serial.read and returns "read-only package requests write-capable permission" for anything else. Publishing this without an FTW change first makes every host refuse myuplink at install. Blocker 2 -- editing the Lua forfeits the FTW-baseline exemption. drivers/tests/conftest.py exempts a driver byte-identical to baselines/ftw and says so explicitly: it "stops being exempt the moment someone edits it". Adding two metadata fields costs the exemption and lights up 7 catalog-convention checks this driver was never written to meet -- among them "should use 'http://' scheme" and "reference config.port" for an HTTPS cloud API, and "should call host.emit()" when emitting only metrics is the driver's stated design. The catalog half is already shippable and separate: see the branch fix/myuplink-read-only-catalog (srcfl#64), which is green. Signed-off-by: HuggeK <48095810+HuggeK@users.noreply.github.com> Co-authored-by: HuggeK <48095810+HuggeK@users.noreply.github.com>
d79aa5b to
1cbb730
Compare
Signed-off-by: Fredrik Ahlgren <fredrik@sourceful-labs.com>
1cbb730 to
8afa969
Compare
miravoss26
left a comment
There was a problem hiding this comment.
Marks myuplink correctly as read_only with a scoped auth_post_path exemption for the token-refresh POST, plus pcall-wraps the HTTP/JSON calls so a failed request or bad body hits the retry path instead of erroring the driver. Good fix for the underlying bug (channel was inferring write-capable from the mere presence of driver_command, even though it unconditionally refuses every command).
tools/ftw_repository.py— the generated POST guard matches on a path suffix, not host+path (path:sub(-#auth_post_path) == auth_post_path). Your own comment notesBASE_URLis config-overridable, so a site config pointing it elsewhere would let the POST (refresh_token + client_secret in the body) go whereverbase_urlpoints, as long as it still ends in/oauth/token. Still a big tightening from the current unguarded state (myuplink currently has no guard at all since it's inferred control-capable) — flagging for the record, not blocking.- CI is red by design here per your own note: 7 catalog-convention checks light up on a cloud driver that lost its byte-identical-to-baseline exemption. That's the maintainer call you're already asking for (exempt those checks for cloud drivers, or accept the divergence) — not something I can resolve from a diff read.
Test coverage looks solid (test_auth_post_path_must_be_a_path_and_must_mean_something, test_signing_in_is_declared_or_it_does_not_happen).
Needs a human decision on the CI-convention question, and per your own warning: not before #743 lands. Nothing here blocks review from my read.
miravoss26
left a comment
There was a problem hiding this comment.
Bumps myuplink to 1.2.0: wraps its cloud HTTP/JSON calls in pcall so a network failure or malformed response hits the driver's normal retry/error-log path instead of an uncaught error, and introduces a scoped auth_post_path mechanism so a read_only driver can still POST to sign in (token exchange) without gaining general write access — the generated guard only lets a POST through when its URL path ends with the declared path, everything else stays denied exactly as before.
- Correctness: the
pcallwrapping aroundhost.http_post/host.json_decodeis the right fix for the crash-on-network-error class of bug, and error messages are still logged with detail (decode_errnow included). The connectivity-aware test split (skip_if_cloud) correctly scopes the local-URL-pattern tests to local drivers only, so cloud drivers likemyuplinkaren't held to aconfig.host/http://rule that never applied to them. - Security: the read-only + write-guard system is the interesting part here, and it's handled well —
_validate_manifeststill rejects any write permission beyond the one declared exemption, the path match is a suffix check on the pre-query-string path (not the whole URL, sincebase_urlis config-overridable), and there's real test coverage forcing the exemption to stay scoped (test_signing_in_is_declared_or_it_does_not_happenassertsmyuplinkis the only driver with the POST exemption baked into the artifact). One nit, non-blocking: in_load_channel, the exemption path doespermissions += PROTOCOL_WRITE_PERMISSIONS[protocol](all write perms for the protocol) rather than just adding"http.post"— today that's equivalent since HTTP's write-permission set is exactly{http.post}, and_validate_manifestwould catch it if that ever grew, but it reads as "add everything" where "add http.post" would say what's actually intended. dcocheck is failing — needs a sign-off before this can merge, otherwise the code looks solid from my read.
|
Automated maintenance pass: re-checked, status changed but still not merging. What's cleared since the last pass (2026-08-01):
What's blocking now:
This needs a human (or the PR author) to redo that merge — Generated by Claude Code |
|
Queue-order update: I am not rebasing #65 yet. The PR has no unresolved inline review threads, and its product checks are green apart from DCO. The current head still has a conflicting unsigned merge commit. More importantly, older #46, #60, and #66 own the same generated catalog files (CHANGELOG.md, devices.yaml, index.yaml, and support status). #46 still needs NIBE HIL evidence; #60 still needs a live Heishamon topic check; #66 is stacked on #60. Those older changes keep right of way. Once they clear, the next step here is a signed rebase onto main, full make check, then merge. |
Warning
Do not merge before srcfl/ftw#743 has landed and shipped.
A read-only
myuplinkreaching a host without that change loses its tokenrefresh and stops reading. CI here is also expected red — see the last
section.
This is the second half of #64. #64 corrects the catalog label for five
drivers that are badged Control while refusing every command. This corrects
the signed artifact for the one of the five where that is not just a label.
The channel does not read the manifest's
controlfield. It infers controlfrom the presence of a
driver_commandentrypoint:myuplink's entrypoint exists and returnsfalseunconditionally, so thechannel publishes it
control_enabled: true,read_only: false, with a writepermission. This makes it declare
read_only = truein its ownDRIVERtable —the mechanism the channel already documents as authoritative.
The problem that makes this more than a one-line change
The read-only guard replaces
host.http_postwith a function that raises, andmyuplinkPOSTs to/oauth/tokenfor the token it reads with. So it declaresauth_post_path, and the generated guard confines the POST rather thantrusting it:
A path rather than a URL because
base_urlis config-overridable — a hardcodedURL would break any site pointing the driver elsewhere.
Every other read-only driver was checked, and none is affected
Rebuilding the channel and cross-referencing each read-only driver's source
against the functions the guard replaces: only
myuplinkcalls one, andonly
http_post.deye,pixiiandsolaredgecallmodbus_writebutpublish control-capable, so they are never guarded.
The exemption is opt-in and asserted to stay that way:
test_signing_in_is_declared_or_it_does_not_happenfails if any driver otherthan
myuplinkloses the blanket denial, and checks that every other read-onlydriver still has neither
auth_post_pathnorhttp.post.Why CI is red here, which is the second thing needing a decision
drivers/tests/conftest.pyexempts a driver byte-identical tobaselines/ftwfrom catalog conventions, and says a driver "stops being exempt the moment
someone edits it". Adding two metadata fields to the
DRIVERtable costsmyuplinkthat exemption and lights up 7 convention checks it was neverwritten to meet — among them
should use 'http://' schemeandreference config.portfor an HTTPS cloud API, andshould call host.emit()whenemitting metrics only is the driver's stated design.
So this needs a maintainer's call: mark those checks as not applying to a
cloud driver, or accept
myuplinkdiverging from FTW's copy. I did not want topick either inside a labelling fix.
Version moved 1.1.1 → 1.2.0: the Lua changed, so the artifact bytes moved.
The
signed channel accepts this treecheck confirms the bump covers it.🤖 Generated with Claude Code