diff --git a/packages/client/README.md b/packages/client/README.md index 045180b..bc8df59 100644 --- a/packages/client/README.md +++ b/packages/client/README.md @@ -272,7 +272,12 @@ async def main(): asyncio.run(main()) ``` -Pass `"*"` instead of a reference list to materialize every skill the store holds. +Pass `"*"` instead of a reference list to materialize every skill the store holds — but know +what you are asking for. `"*"` materializes the **whole project library**, which puts every +skill's `description` into the agent's context, including skills no AI Config references and +skills belonging to other teams. `write_skills(skill_refs(...), root)` is the form used above +because it materializes only what the resolved variation actually asked for; reach for `"*"` +when you genuinely want the whole library on disk. **`skills` is now a validated field.** Config parsing fails closed on a `skills` value that is not a list of `{key, version}` objects (key matching `^[a-z0-9][a-z0-9-]*$`, version an @@ -406,6 +411,19 @@ never writes through a symlink; writes are atomic (temp file, `fsync`, rename) a `0644`; and if the manifest is unreadable it performs no destructive action at all. Removing a skill from a variation is how revocation works — the next reconcile prunes it. +**Platform bound: the descriptor-pinned guarantee is POSIX-only.** On POSIX every destructive +step — the open, the rename, the unlink — runs relative to a directory descriptor opened +`O_RDONLY|O_DIRECTORY|O_NOFOLLOW` and held for the whole reconcile, so a directory swapped for +a symlink *after* its checks cannot redirect a write or a delete: the descriptor names the +inode that was checked, which closes the swap window rather than narrowing it. Windows has no +`*at()` syscall family, so there `write_skills` falls back to a per-component `lstat` check +taken immediately before each step. That floor is a check-then-use race rather than a closed +window: an attacker who already holds **write permission on the managed root** can still win +it. Windows reparse-point checks (`GetFileAttributesW` / `FILE_FLAG_OPEN_REPARSE_POINT`) are +deliberately not implemented in this release, and Windows is not a tested platform for it — +neither SDK repository has a Windows CI runner. Treat write permission on the managed root as +the security boundary on every platform, and on Windows as the *only* one. + **One exception, and it is what makes a crashed reconcile recoverable.** A file at a managed path whose bytes are *already byte-identical* to the content LaunchDarkly resolved is adopted — recorded in the manifest and reported `skipped_current` — rather than refused. @@ -477,6 +495,54 @@ to key its own map however the transport underneath does. > what was hashed. The SDK never parses or interprets them; if you want the frontmatter, > decode and parse the content on your side. +#### Privilege separation: the agent must not be able to rewrite its own skills + +**The recommended deployment runs `write_skills` as a different identity than the agent.** +Reconcile as one user, run the agent as another. Everything the reconcile puts on disk is +owner-write-only, and set explicitly rather than inherited from your umask: skill files and +the manifest at `0644` (via `fchmod` on the descriptor, so it cannot be redirected), the +per-skill `//` directories at `0755`, and the execute bit never set on anything. +Those modes are only a defense if the two identities actually differ — under a single identity +they describe a directory the agent can freely rewrite. + +**What to verify, as the identity that will run the agent.** The SDK cannot check this for you +(see below), so make it a deployment step: confirm the agent's identity has no write access to + +- the managed root itself, +- the per-skill directories `//` and the files `//SKILL.md`, +- the manifest at `/.launchdarkly-skills.json`. + +```bash +# Run as the agent's user. Every line should print DENIED. +root=.claude/skills +for target in "$root" "$root/.launchdarkly-skills.json" "$root"/*/ "$root"/*/SKILL.md; do + [ -e "$target" ] || continue + if [ -w "$target" ]; then echo "WRITABLE — fix this: $target"; else echo "DENIED: $target"; fi +done +``` + +Note that the managed root's own mode is **yours, not the SDK's**: `write_skills` creates only +that one leaf directory and does so with your umask, precisely because the root is a path you +chose. Own it — `chown reconcile-user:agent-group` and `chmod 0755` on the root is the shape +that makes the rest of the tree's modes mean something. + +**Why this is the mitigation that matters.** A `SKILL.md` is agent *instructions*. An agent +that can write its own skills directory can rewrite its own instructions, and an agent +processing untrusted input is exactly the thing that might be induced to do so. Write access +to the manifest is worse than write access to a skill, because the manifest is what tells the +*next* reconcile which paths the SDK owns and may delete: an agent that can edit it can keep a +skill LaunchDarkly has revoked, or aim the SDK's own delete path at something it should not +touch. `write_skills` re-validates every manifest entry from scratch for exactly that reason — +it treats that file as untrusted input, never as authorization — but an agent that cannot edit +it at all is the stronger position, and only your deployment can provide that. + +**The SDK deliberately does not report whether the root is writable.** There is no such field +on `ReconcileReport`, and its absence is a decision rather than an oversight. The SDK knows +only its own identity, which trivially has write access — it just wrote there. It cannot know +which identity will later run the agent, so any check it could make would answer a different +question than the one that matters, and would read as reassurance exactly where caution is +wanted. You know both identities; the SDK knows one. + --- ### Utility Helpers diff --git a/packages/client/agents.md b/packages/client/agents.md index a7b4ce1..2558f9c 100644 --- a/packages/client/agents.md +++ b/packages/client/agents.md @@ -510,6 +510,31 @@ resolving to the skill directory's `(st_dev, st_ino)`) instead of by comparing p A spy must `fstat` the descriptor **inside** the intercepted call — the implementation closes it as soon as the write returns. +**The platform bound is POSIX-only, and that is a decision — do not quietly "fix" it.** +Windows reparse-point checks (`GetFileAttributesW`, `FILE_FLAG_OPEN_REPARSE_POINT`) are not +implemented because Windows is not a supported or tested platform for this release: there is +no Windows CI runner in either repository, so the checks would ship unverified, and the +TypeScript SDK could not match them at all — Node exposes no `*at()` family on *any* +platform, so its racy floor is universal rather than Windows-only. Implementing them in +Python alone would break cross-language parity and trade a documented bound for an unverified +one. Two follow-on facts: on Windows write permission on the managed root is the only +boundary, which is why the privilege-separated deployment is documented as the mitigation +rather than as advice; and this bound retroactively lowers the priority of the reserved-device-name +work above — keep that code, but do not read it as evidence that Windows is hardened. If +Windows becomes a supported platform, revisit both together, and add the CI runner first. + +**Privilege separation is the deployment-side half of this, and `ReconcileReport` must not +grow a writability field.** The recommended deployment runs the reconcile as a different +identity than the agent, so the `0644`/`0755` modes above actually deny something: the agent +reads its instructions and cannot rewrite them or the manifest. That is the mitigation for a +prompt-injected agent editing its own skills. The security review asked for the report to +surface whether the managed root is writable; we declined, and the reasoning is load-bearing +rather than a preference. The SDK knows only its *own* identity, which trivially has write +access — it just wrote there — and cannot know which identity will later run the agent. Any +check it could perform would answer a different question than the one asked and would create +false confidence exactly where caution is wanted. The operator's verification steps live in +the README instead. Do not add the field. + ### Deferred: bounded retries `timeout` is implemented — a monotonic deadline, checked before each retrieval, before diff --git a/packages/client/src/launchdarkly_ai_server/safe_fs.py b/packages/client/src/launchdarkly_ai_server/safe_fs.py index 73eef87..43c21e5 100644 --- a/packages/client/src/launchdarkly_ai_server/safe_fs.py +++ b/packages/client/src/launchdarkly_ai_server/safe_fs.py @@ -11,6 +11,29 @@ re-resolving a name — which is what closes the swap window rather than merely narrowing it. Where the platform has no ``*at()`` syscall family (Windows) the identical sequence runs against full paths, the per-component ``lstat`` floor. + +**Platform bound — this guarantee is POSIX-only, deliberately.** On POSIX the +descriptor walk closes the swap window. On Windows it does not exist: there is no +``*at()`` family, so the ``lstat`` floor is all that runs, and a floor is a +check-then-use race rather than a closed window. The remedy would be +reparse-point checks (``GetFileAttributesW``, or opening with +``FILE_FLAG_OPEN_REPARSE_POINT``) and it is **not implemented, by decision rather +than by oversight**: Windows is not a supported or tested platform for this +release, and neither SDK repository has a Windows CI runner, so the checks would +ship untested — and the TypeScript SDK could not match them in any case, because +Node exposes no ``*at()`` family on *any* platform. Shipping them in Python alone +would break the cross-language parity the two SDKs are held to and would trade a +documented bound for an unverified one. + +Two consequences worth stating plainly rather than discovering later. First, on +Windows write permission on the managed root is the *only* boundary, so the +privilege-separated deployment the README documents is not advice there but the +mitigation. Second, this bound retroactively lowers the priority of the Windows +reserved-device-name work in ``skills_fs.py`` (``_WINDOWS_RESERVED_NAMES``): that +code stays, because it is cheap and it keeps a managed root written on Linux +usable when read from Windows, but it should not be read as evidence that Windows +is a hardened target. It is not. Revisit both together if Windows becomes +supported. """ from __future__ import annotations diff --git a/packages/client/tests/test_skills_fs.py b/packages/client/tests/test_skills_fs.py index 223a326..6d5ab2d 100644 --- a/packages/client/tests/test_skills_fs.py +++ b/packages/client/tests/test_skills_fs.py @@ -1394,6 +1394,136 @@ async def test_corrupt_manifest_file_is_not_destroyed(self, root: Path) -> None: assert _manifest_path(root).read_text(encoding="utf-8") == "{not json at all" +# The three literal cases the security review names for the prune path. +# +# The distinction from ``TestCorruptManifest`` above is the whole point: a +# corrupt manifest suppresses every destructive action wholesale, so those tests +# say nothing about these. Each manifest here is *well-formed* — parseable, +# a ``manifestVersion`` this release understands, a real ``entries`` map, and an +# entry whose ``key`` is a perfectly valid skill key that is genuinely absent +# from the requested set. The implementation has every input it needs to prune +# and must refuse anyway, because the recorded *path* is not one this SDK could +# have written. +HOSTILE_RECORDED_PATHS: list[str] = [ + # Absolute: the classic. A recorded path read back and unlinked as-is is a + # delete of an attacker-chosen file with the reconcile's privileges. + "/etc/passwd", + # Traversing: the same attack for an implementation that rejects a leading + # slash and then joins the rest onto the root. + "../../../etc/passwd", +] + + +class _UnlinkSpy: + """Records every ``os.unlink`` while delegating to the real one. + + Asserting only that ``/etc/passwd`` still exists proves nothing: the test + process cannot delete it anyway, so that assertion passes against an + implementation with no path check at all — permissions would be doing the + work. What has teeth is that the removal is never *attempted*: the refusal + happens above the syscall, on a path the SDK recomputes rather than trusts. + """ + + def __init__(self) -> None: + self.targets: list[str] = [] + + def install(self, monkeypatch: pytest.MonkeyPatch) -> _UnlinkSpy: + real = os.unlink + + def spy(path: Any, *args: Any, **kwargs: Any) -> None: + self.targets.append(os.fsdecode(path)) + real(path, *args, **kwargs) + + # ``safe_fs_module.os`` *is* the ``os`` module, so this covers both the + # descriptor-relative ``os.unlink(name, dir_fd=...)`` and the + # ``Path.unlink`` used on the no-``*at()`` floor. + monkeypatch.setattr(safe_fs_module.os, "unlink", spy) + return self + + +class TestHostileManifestPrune: + """A well-formed manifest naming a path this SDK could not have written. + + The manifest is untrusted input. It is a plain file on the customer's disk + that anything with write access to the managed root can edit, and ``prune`` + is the one code path in the SDK that deletes. So a recorded path never + authorizes its own removal: it must match ``/SKILL.md`` for a + re-validated key, and the target is recomputed from the *current* managed + root instead of being read back out of the entry. + """ + + @pytest.mark.parametrize("recorded", HOSTILE_RECORDED_PATHS) + async def test_recorded_path_outside_the_root_is_refused( + self, root: Path, recorded: str, monkeypatch: pytest.MonkeyPatch + ) -> None: + spy = _UnlinkSpy().install(monkeypatch) + _write_manifest( + root, + { + "manifestVersion": 1, + "entries": {recorded: _entry("a", 1, SKILL_BODY)}, + }, + ) + + report = await write_skills([], root) + + assert report.ok is False + action = _actions_by_key(report)["a"] + assert action.action == "error" + assert action.error is not None + # The refusal is about ownership of the path, not about the file's state. + assert "could own" in action.error + assert [a for a in report.actions if a.action == "removed"] == [] + # Nothing was even attempted, let alone completed. + assert spy.targets == [] + assert Path("/etc/passwd").exists() + # Left in place rather than tidied away: dropping the entry would let a + # single hostile edit erase the SDK's own record of what it manages. + assert recorded in _read_manifest(root)["entries"] + + async def test_entry_under_a_since_symlinked_parent_is_refused( + self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch + ) -> None: + """The recorded path is the SDK's own, and is still not enough. + + Here the entry is exactly what a legitimate reconcile writes — + ``a/SKILL.md`` under key ``a`` — so the shape check that catches the two + cases above passes. What changed is the disk underneath it: ``/a`` + is now a symlink to somewhere else. This is the case a validate-then-act + implementation fails, because the manifest and the entry are both + entirely legitimate; only the current state of the parent is not. + """ + root = tmp_path / "skills" + root.mkdir() + elsewhere = tmp_path / "elsewhere" + elsewhere.mkdir() + victim = elsewhere / "SKILL.md" + victim.write_text("victim content\n", encoding="utf-8") + + # Managed legitimately first, so the manifest entry is one this SDK + # really did write... + managed = _place_managed(root, "a", SKILL_BODY) + # ...then the parent directory is swapped for a link out of the root. + managed.unlink() + (root / "a").rmdir() + (root / "a").symlink_to(elsewhere, target_is_directory=True) + + spy = _UnlinkSpy().install(monkeypatch) + report = await write_skills([], root) + + assert report.ok is False + action = _actions_by_key(report)["a"] + assert action.action == "error" + assert action.error is not None + assert "symlink" in action.error + assert [a for a in report.actions if a.action == "removed"] == [] + assert spy.targets == [] + # The file the symlink pointed at is untouched, and so is the link. + assert victim.read_text(encoding="utf-8") == "victim content\n" + assert (root / "a").is_symlink() + assert "a/SKILL.md" in _read_manifest(root)["entries"] + + class TestWriteSkillsTelemetry: """Materialized / revoked signals from write_skills."""