Skip to content

feat(license): persist VPP config + license in PERSISTENT_DATA_DIR so a runtime update can't wipe it - #179

Open
marconetsf wants to merge 2 commits into
developmentfrom
feat/vpp-license-persist-dir
Open

feat(license): persist VPP config + license in PERSISTENT_DATA_DIR so a runtime update can't wipe it#179
marconetsf wants to merge 2 commits into
developmentfrom
feat/vpp-license-persist-dir

Conversation

@marconetsf

@marconetsf marconetsf commented Aug 28, 2026

Copy link
Copy Markdown
Contributor

Problem

On runtime-v4 the device license blob is written next to the plugin config under $OPENPLC_DIR/build/vpp/<plugin>.license. install.sh does rm -rf $OPENPLC_DIR/build on a version update/rebuild, which silently deletes a purchased license and drops the device to demo after the 2h window. build/ is build output, never treated as state.

PERSISTENT_DATA_DIR (/var/lib/openplc-runtime native, /var/run/runtime container volume) already survives updates and holds the DB + .env, but the license did not live there.

Fix

Move the VPP config and its license sibling into PERSISTENT_DATA_DIR/vpp/, and rewrite each config_path in vpp_plugins.conf to that absolute path. The closed .so still finds them because the C loader passes config_path to the plugin verbatim — it only containment-checks path (the .so), which stays under build/vpp (it is code, rebuilt each upload).

Runtime-only. No editor, package, or C change. The license is no longer stored in the volatile build tree at all, so a version update can no longer delete it.

Changes

  • config.py: add VPP_DATA_DIR = PERSISTENT_DATA_DIR / "vpp".
  • plcapp_management.py (apply_vpp_plugin_conf): copy config + license into VPP_DATA_DIR (destination derived from the plugin basename, never the editor-supplied path), rewrite config_path via PluginsConfiguration.to_file, and best-effort migrate a pre-existing build/vpp .license.
  • vpp_license_debug.py (resolve_license_path): accept VPP_DATA_DIR as a second known root so 0x49/0x4A resolve there; still refuses .. and any path outside both roots.

Security

No wider write surface. validate_vpp_plugins_conf is unchanged: it confines the .so path to build/vpp, but config_path only to the runtime root (a wider set — the whole installation). So nothing here trusts config_path as a location: the persistent write destination is built by the runtime from a sanitized plugin basename, and the one-time migration reads only the fixed build/vpp/<name>.license — a forged config_path can steer neither the write nor the migration read. The read guard in resolve_license_path is widened to a second known root (VPP_DATA_DIR), not to "anywhere".

Tests

tests/pytest/plugins/: relocation + config_path rewrite; license survives a license-less re-upload; guard accepts the persistent dir and fails closed; 0x49/0x4A round-trip in the persistent dir. New tests/pytest/conftest.py points the runtime dirs at a temp location so webserver.config imports safely across the whole suite. Local run: 41 passed / 1 skipped (symlink test needs admin on Windows); the C-parity anchor tests run in CI.

Deploy / migration note

A device that is already licensed keeps working, via one of two paths depending on install.sh. The build/ wipe is conditional on build/CMakeCache.txt (install.sh:327-330), not unconditional:

  • Wipe does not fire → the old build/vpp/<name>.license survives, and the one-time migration copies it into the persistent dir on the next upload (this is exactly what that block is for).
  • Wipe fires → the old .license is gone before the new code runs, and the device re-activates automatically from its existing entitlement on the next editor connect (0x4A empty → activate → 0x49). No repurchase.

Either way, from then on the license lives in the persistent dir and survives updates.

🤖 Generated with Claude Code

…runtime updates

The v4 device license blob was written next to the plugin config under
$OPENPLC_DIR/build/vpp, which install.sh wipes (`rm -rf build`) on a runtime
version update -- silently deleting a purchased license and dropping the device
to demo after the 2h window.

Relocate the VPP config and its license sibling into PERSISTENT_DATA_DIR/vpp
(which survives updates, like the DB and .env) and rewrite each config_path in
vpp_plugins.conf to that absolute path. The closed .so still finds them because
the C loader passes config_path to the plugin verbatim (it only containment-checks
`path`, the .so, which stays under build/vpp). Runtime-only: no editor, package
or C change.

- config.py: add VPP_DATA_DIR = PERSISTENT_DATA_DIR/"vpp"
- apply_vpp_plugin_conf: copy config+license into VPP_DATA_DIR (dest derived from
  the plugin basename, never the editor path), rewrite config_path, best-effort
  migrate a pre-existing build/vpp .license
- resolve_license_path: accept VPP_DATA_DIR as a second known root so 0x49/0x4A
  resolve there; still refuses traversal / paths outside both roots
- tests: relocation+rewrite, survival across a license-less upload, guard accepts
  persistent + fails closed, 0x49/0x4A roundtrip in the persistent dir; new
  tests/pytest/conftest.py points the runtime dirs at a temp location

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015uUH3ZL5ehreMUf2dtanWD
@Gustavohsdp

Copy link
Copy Markdown

The design here is right, and the claim the whole thing rests on — that the C loader passes config_path through untouched — I verified in the C rather than taking it on trust. One blocking item, on a guard that used to exist in this function.

What I verified

Check Result
Premise: install.sh wipes build/ install.sh:329 — but conditional, see item 2
Base is current ✅ contains development
Core claim: C does no containment on config_path plugin_config.c:141-157 only strncpys it
C buffer fits the longer path MAX_PLUGIN_PATH_LEN 256 vs 63 chars
py_compile on all four Python files
pytest suite ⚠️ not run — my local Python is 3.9.6, the project requires ≥3.10

1. 🔴 Blocking — the containment guard on config_path was dropped, and the upstream validator is weaker than the description says

The old code validated before use:

dest_config = os.path.normpath(p.config_path)
if not is_inside_root(dest_config, runtime_root):
    build_state.log(f"[WARNING] VPP: config_path '{p.config_path}' escapes runtime root, skipping\n")
    continue

The new code uses the same input with no guard at all:

old_license = derive_license_path(os.path.normpath(p.config_path))
...
elif old_license and os.path.exists(old_license) and not os.path.exists(dest_license):
    shutil.copy2(old_license, dest_license)

The Security section says this is covered upstream:

"validate_vpp_plugins_conf unchanged — the editor's config_path must be inside build/vpp"

That isn't what it enforces. plcapp_management.py:333:

if p.config_path and not is_inside_root(against_root(p.config_path), runtime_root):
    return False, f"plugin '{p.name}' config_path '{p.config_path}' escapes the runtime root"

config_path is confined to the runtime root. Only path (the .so) is confined to build/vpp, at lines 328-332. That is a much wider set — the whole installation.

What it allows: a forged vpp_plugins.conf whose config_path names any X.json inside the runtime root makes old_license = X.license; if that file exists and the upload carries no license of its own, it is copied to VPP_DATA_DIR/<name>.license, where it can then be read back over FC 0x4A.

The impact is narrow, and it's worth saying why: it only reaches files ending in .license inside the runtime root, and a blob is device- and product-bound and signed — copying plugin A's blob to plugin B's name does not license B, because the .so checks product_id against its own. So this is not a licensing bypass; it is a narrow read primitive. But it is a guard that existed and is now gone, and restoring it is one line:

old_license = derive_license_path(os.path.join(runtime_root, p.config_path))
if old_license and not is_inside_root(old_license, runtime_root):
    old_license = None

Note I also swapped os.path.normpath(p.config_path) for os.path.join(runtime_root, ...). normpath resolves against the process cwd, while the validator uses against_root(), whose own docstring warns about exactly that — "Resolving against the process cwd instead would make the guard depend on where the caller happened to be." The two should agree.

2. 🟡 The deploy note overstates the wipe, and that misleads in the reassuring direction

install.sh:327-330:

if [ -d "$OPENPLC_DIR/build" ] && [ -f "$OPENPLC_DIR/build/CMakeCache.txt" ]; then
    echo "Cleaning existing build directory to ensure clean build..."
    rm -rf "$OPENPLC_DIR/build"
fi

The wipe is conditional on CMakeCache.txt being present. Two consequences:

  1. The problem is real but does not occur on every update — the description presents it as unconditional.
  2. More importantly, the deploy note says "the update that installs this wipes build/vpp before the new code runs, so the old .license is lost there." When the wipe does not fire, the old .license survives — and the migration block then does exactly what it was written for, rescuing the license in place.

So the note reads as if the migration can never fire on the update path, when in fact it is what saves the device in the cases where the wipe doesn't happen. Worth correcting in both directions, not least because that is the path carrying item 1's risk, so knowing when it runs matters.

3. 🟡 VPP_DATA_DIR.mkdir() runs at import, at module top level

VPP_DATA_DIR = PERSISTENT_DATA_DIR / "vpp"
VPP_DATA_DIR.mkdir(parents=True, exist_ok=True)

PERSISTENT_DATA_DIR also creates its directory, but from inside get_persistent_data_dir(). This one is a bare side effect at module scope.

It is the same class of import-time filesystem work that forced this PR to add tests/pytest/conftest.py, whose docstring says so directly: "config creates PERSISTENT_DATA_DIR / RUNTIME_DIR at import time; without this override the import would try to create /var/lib/openplc-runtime on a CI box." The PR both adds a new import-time mkdir and adds the workaround for import-time mkdirs.

It follows the existing pattern, so it isn't a new sin — but it deepens the one the same PR had to paper over, and a permission failure here is a hard boot failure at import. Moving it inside get_persistent_data_dir(), or creating it on demand in apply_vpp_plugin_conf, would line it up with the rest.

Nits

  • Configs and licenses accumulate in VPP_DATA_DIR with no cleanup path. For the license that is deliberate and correct; the config half is just residue.
  • The Security section would be stronger stating the real bound: "config_path is confined to the runtime root by the validator; the write destination is built by the runtime from a sanitised basename." As written it claims a tighter confinement than exists.

What's good

  1. The problem is real and correctly diagnosedbuild/ is build output being relied on as state, and install.sh:329 confirms it.
  2. The claim the whole design rests on holds. I went to the C: plugin_config.c:141-157 only strncpys plugin_related_config_path and hands it over; the containment check (parse_plugin_config_contained) applies to path. An absolute persistent config_path really does work.
  3. I checked the buffer, because that is how this class of change breaks silently. Growing a path from 40 to 59 characters into a fixed C buffer is exactly the trap: MAX_PLUGIN_PATH_LEN is 256 and the longest new path (/var/lib/openplc-runtime/vpp/<name>.license) is 63. Four times the headroom.
  4. The destination construction is the right shape. Deriving from p.name with a basename check and then is_inside_root is the correct order. I probed the corners: .. and . produce harmless filenames, absolute names are rejected by the basename check, and the Windows case (ntpath.join resetting on a drive-letter component) is caught by the second check. Defence in depth doing its job.
  5. The widened resolve_license_path really is a second root, not "anywhere" — and there is a test asserting it still fails closed outside both.
  6. The delivery tests got stronger. Dropping the PluginsConfiguration.from_file monkeypatch in favour of a real conf line that must pass validate_vpp_plugins_conf means the test can no longer pass against a stale fake — which is the failure mode the old comment on that test was already worried about.
  7. conftest.py fixes a real problem. Modules being skipped rather than tested is the worst kind of test failure, because it looks green.
  8. The "license survives a license-less re-upload" test pins the product requirement, not the implementation.

Summary

# Action Where Blocking?
1 Restore containment on old_license, and resolve against runtime_root rather than the cwd plcapp_management.py (~line 415) Yes — one line
2 Correct the deploy note: the wipe is conditional on CMakeCache.txt PR description No
3 Move the mkdir out of module scope config.py:112 No
Security section: config_path is confined to the runtime root, not build/vpp PR description No

One caveat on my verification: I could not run the suite (local Python 3.9.6 against a >=3.10 requirement), so 41 passed / 1 skipped remains author-attested. What I checked directly was the C side, the buffer size, the path guards by reasoning through their edge cases, and py_compile on all four Python files.

…ir (review)

Addresses Gustavo's review on #179.

1. (blocking) The one-time migration read its source from config_path, which
   validate_vpp_plugins_conf only confines to the runtime root (not build/vpp),
   so a forged vpp_plugins.conf could point it at any .license under the root
   and have it copied where 0x4A reads it back. Derive the source from the FIXED
   build/vpp/<name>.license instead (name is already basename-checked) -- that is
   exactly where a pre-change license lived and it cannot be steered elsewhere.
   New tests: the legitimate build/vpp migration still works, and a forged
   config_path pointing at another .license is ignored (verified failing on the
   pre-fix code via a worktree at the previous commit).

2. config.py: VPP_DATA_DIR no longer mkdir's at import (module scope). The dir is
   created on demand by apply_vpp_plugin_conf / _write_license_atomically; a bare
   import-time mkdir turns a permission failure into a hard import crash -- the
   very thing tests/pytest/conftest.py had to work around.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015uUH3ZL5ehreMUf2dtanWD
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.

2 participants