Skip to content

make the signed artifact agree that myuplink is read-only - #65

Open
HuggeK wants to merge 3 commits into
srcfl:mainfrom
HuggeK:fix/myuplink-read-only-artifact
Open

make the signed artifact agree that myuplink is read-only#65
HuggeK wants to merge 3 commits into
srcfl:mainfrom
HuggeK:fix/myuplink-read-only-artifact

Conversation

@HuggeK

@HuggeK HuggeK commented Jul 31, 2026

Copy link
Copy Markdown
Collaborator

Warning

Do not merge before srcfl/ftw#743 has landed and shipped.
A read-only myuplink reaching a host without that change loses its token
refresh 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 control field. It infers control
from the presence of a driver_command entrypoint:

controls = has_driver_command and not declares_read_only

myuplink's entrypoint exists and returns false unconditionally, so the
channel publishes it control_enabled: true, read_only: false, with a write
permission. This makes it declare read_only = true in its own DRIVER table —
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_post with a function that raises, and
myuplink POSTs to /oauth/token for the token it reads with. So it declares
auth_post_path, and the generated guard confines the POST rather than
trusting it:

host.http_post = function(url, ...)
    local path = type(url) == "string" and url:match("^[^?]*") or ""
    if path:sub(-#__sourceful_ftw_auth_path) == __sourceful_ftw_auth_path then
        return __sourceful_ftw_http_post(url, ...)
    end
    error("this driver declares itself read-only: POST is allowed only for authentication")
end

A path rather than a URL because base_url is config-overridable — a hardcoded
URL 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 myuplink calls one, and
only http_post. deye, pixii and solaredge call modbus_write but
publish 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_happen fails if any driver other
than myuplink loses the blanket denial, and checks that every other read-only
driver still has neither auth_post_path nor http.post.

Why CI is red here, which is the second thing needing a decision

drivers/tests/conftest.py exempts a driver byte-identical to baselines/ftw
from catalog conventions, and says a driver "stops being exempt the moment
someone edits it"
. Adding two metadata fields to the DRIVER table costs
myuplink that exemption and lights up 7 convention checks it 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 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 myuplink diverging from FTW's copy. I did not want to
pick 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 tree check confirms the bump covers it.

🤖 Generated with Claude Code

@HuggeK
HuggeK requested a review from frahlg July 31, 2026 12:17
@HuggeK
HuggeK marked this pull request as ready for review July 31, 2026 12:26

frahlg commented Aug 1, 2026

Copy link
Copy Markdown
Member

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 myuplink that declares itself read-only without the host granting the auth_post_path exemption, which — as the PR description says — loses its token refresh and stops reading. The drivers check failing here is expected (7 catalog-convention checks myuplink was never written to meet, once it loses its baselines/ftw byte-identity exemption) and is itself flagged in the PR body as needing a maintainer's call: exempt those checks for a cloud driver, or accept the divergence from FTW's copy.

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 miravoss26 left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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: false matches each driver's actual driver_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 drivers check 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>
@frahlg
frahlg force-pushed the fix/myuplink-read-only-artifact branch from d79aa5b to 1cbb730 Compare August 4, 2026 07:35
Signed-off-by: Fredrik Ahlgren <fredrik@sourceful-labs.com>
@frahlg
frahlg force-pushed the fix/myuplink-read-only-artifact branch from 1cbb730 to 8afa969 Compare August 4, 2026 07:36

@miravoss26 miravoss26 left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 notes BASE_URL is config-overridable, so a site config pointing it elsewhere would let the POST (refresh_token + client_secret in the body) go wherever base_url points, 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.

@HuggeK HuggeK changed the title BLOCKED on srcfl/ftw#743 — make the signed artifact agree that myuplink is read-only make the signed artifact agree that myuplink is read-only Aug 5, 2026

@miravoss26 miravoss26 left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 pcall wrapping around host.http_post/host.json_decode is the right fix for the crash-on-network-error class of bug, and error messages are still logged with detail (decode_err now included). The connectivity-aware test split (skip_if_cloud) correctly scopes the local-URL-pattern tests to local drivers only, so cloud drivers like myuplink aren't held to a config.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_manifest still 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, since base_url is config-overridable), and there's real test coverage forcing the exemption to stay scoped (test_signing_in_is_declared_or_it_does_not_happen asserts myuplink is the only driver with the POST exemption baked into the artifact). One nit, non-blocking: in _load_channel, the exemption path does permissions += 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_manifest would catch it if that ever grew, but it reads as "add everything" where "add http.post" would say what's actually intended.
  • dco check is failing — needs a sign-off before this can merge, otherwise the code looks solid from my read.

frahlg commented Aug 6, 2026

Copy link
Copy Markdown
Member

Automated maintenance pass: re-checked, status changed but still not merging.

What's cleared since the last pass (2026-08-01):

  • srcfl/ftw#743 has landed and shipped: RuntimePolicy.AuthPostPath is in the ftw CHANGELOG.md under 1.16.0, and the host-side guard confinement (h.RuntimePolicy.AuthPostPath in go/internal/drivers/host.go) is present at ftw's current main (verified by cloning srcfl/ftw and reading the source directly). The "do not merge before this ships" warning at the top of this PR is resolved.
  • The "maintainer's call" the PR description asked for — exempt 7 catalog-convention checks for a cloud driver, or accept divergence — was made in commit 8afa969 (fix(myuplink): handle cloud driver contract, 2026-08-04): drivers/tests/conftest.py gained a manifest connectivity reader, and test_http_drivers.py's local-URL checks (http:// scheme, config.port, base-URL-from-config) now skip cloud drivers instead of misapplying to one. test_calls_emit_in_polltest_emits_in_poll accepts host.emit_metric too. That's the right fix — it corrects the rule rather than carving out an exemption.

What's blocking now:

  • dco is failing again, and mergeable_state is dirty. The failure is on commit a0d5d73, Merge branch 'main' into fix/myuplink-read-only-artifact — a merge commit with no Signed-off-by trailer, same shape as the dependabot DCO failure on chore(deps): Bump cryptography from 48.0.1 to 50.0.0 #74. It also didn't resolve cleanly (GitHub reports the PR as conflicting with main right now).

This needs a human (or the PR author) to redo that merge — git fetch origin main && git merge origin/main (or rebase), resolve the conflicts, and git commit -s — then push. Once DCO and the merge conflict clear, this looks ready from the driver/catalog-convention side: drivers, packages, signed channel accepts this tree, public-boundary and history-secret-scan were all green as of the last completed run.


Generated by Claude Code

@frahlg

frahlg commented Aug 7, 2026

Copy link
Copy Markdown
Member

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants