feat: parse profiles in the core, and have the Python SDK stop parsing by dzerik · Pull Request #185 · multikernel/sandlock

dzerik · 2026-08-04T14:08:30Z

Closes the second half of #174.

The problem

The Python SDK carried its own TOML parser and its own copies of the string
micro-grammars, and they had already drifted from the core in two places:

parse_memory_size accepted fractions and a T suffix that
ByteSize::parse rejects, so memory = "1.5T" was valid through the SDK
and invalid through the CLI.
time_start went through int(), so an RFC 3339 stamp loaded in the CLI
and raised through the SDK.

A third grammar sat unused in the dataclass, time_start_timestamp, with
naive-means-UTC semantics that nothing else in the project shares.

The drift was not an accident of implementation. Two independent parsers
that must agree byte for byte will diverge, because nothing makes them
agree except attention.

What this does

sandlock_profile_parse takes TOML text and returns canonical JSON with
every micro-grammar already resolved: mounts as {virt, host, ro} objects,
sizes as integer bytes, time_start as epoch seconds, bind ports as
expanded integer lists. The SDK's remaining job is a field-for-field copy
into its dataclass, so introspection, dataclasses.replace and preset
composition keep working. Unknown keys are rejected on both sides, so a
future schema change fails at load time instead of being mis-parsed
silently.

sandbox_to_json was not reusable as-is: it re-emits mounts as V:H:ro
spec strings, which would have put string parsing straight back into the
SDK. The canonical form emits structured mounts instead.

The canonical ro is the effective setting for the virtual path, not the
flag written on one spec. The core keys read-only mounts by virtual path
(Sandbox::fs_mount_ro is a list of virtual paths, and
chroot/dispatch.rs::is_mount_ro matches on it), so two specs sharing a
virtual path share one verdict. Reporting the written flag would describe a
policy no layer applies. One residual gap is documented rather than hidden:
a mount nested under a read-only one is write-denied at run time while its
canonical ro stays false, and closing that needs (virt, host) keying.

Breaking changes

Public Python API, deliberately and without shims:

max_memory: str | int | None   ->  int | None
max_disk:   str | None         ->  int | None
time_start: float | str | None ->  float | None
fs_mount:   Mapping[str, str]  ->  Sequence[Mount]

Mount(virt, host, ro) is new and mirrors the canonical field names.
fs_mount becoming a sequence is what lets a read-only mount be expressed
at all from Python; it reaches the C ABI through the fs_mount_ro setter
added in #180. tomli is gone from the dependencies.

parse_memory_size, Sandbox.memory_bytes() and
Sandbox.time_start_timestamp() are removed. The first two were the SDK's
byte-size grammar and its accessor, the third the unused grammar named
above. Nothing replaces them: the resolved value is the field.

on_error loaded from a profile now defaults to COMMIT where it defaulted
to ABORT. The canonical form always resolves both branch actions, and the
SDK copies what it is handed, so a profile silent about the error path gets
the core's answer rather than the dataclass's second opinion. This is
deliberate, since the CLI, a profile and the Go SDK have always meant COMMIT
for that policy, but it changes what happens to a COW branch for a profile
already in use, and it changes it silently. Only the profile path moves; the
dataclass default is untouched here.

Two core fixes that came out of this

Rebuilding a builder from a parsed profile ran extend_net_allow_for_http
a second time over an allowlist that already held its derived entries, so
the helper is idempotent now, with a test.
ByteSize::parse multiplies with checked_mul. The unchecked multiply
wrapped in release builds, so memory = "17179869184G" parsed cleanly and
installed a ceiling of zero bytes, with nothing reported anywhere and the
guest SIGKILLed on its first allocation. It is an out-of-range error now.

One gap left open and pinned

Verified against the CLI message for message on every grammar: the same
profile loads identically, or fails identically, through both paths, with
one exception.

sandlock_sandbox_builder_time_start takes a uint64 of seconds, so a
stamp the core keeps in full loads from a profile and then cannot be handed
to a builder. "2026-01-01T00:00:00.5Z" and any instant before 1970 are
what that costs. The SDK refuses them by name instead of wrapping a negative
value through an unsigned setter, and
test_time_start_the_c_abi_cannot_carry_is_refused_loudly holds it there.
Closing the gap means changing that setter's signature, which is the third
PR in this series.

Position in the series

This is the first of three. The next two are stacked on it:

this PR: profile parsing moves into the core.
a rejected setter argument is latched instead of coerced.
sizes and timestamps cross the C ABI as strings; both SDKs stop parsing.

Each is reviewable on its own and each is green on its own.

Testing

cargo test -p sandlock-core --lib: 730 pass at this commit.
cargo test -p sandlock-core --test integration -- --test-threads=1: green.
cargo test --workspace --exclude sandlock-core: green.
Python: the profile suites pass; a new adversarial suite
(profile_canonical_adversarial.rs, 447 lines) drives the canonical form
against malformed and boundary input.
The cbindgen header is regenerated in this commit and matches CI's
git diff --exit-code gate.

congwang-mk · 2026-08-05T23:02:11Z

Not a review, just a note: I am planning to cut the release in a few days, since this PR is fairly large, I'd suggest to defer it to the next release. WDYT?

dzerik · 2026-08-06T06:57:56Z

Agreed, and thanks for saying so before the release rather than after. Deferring all three is the right call: the volume is genuinely large, and a release is the worst moment to take a breaking change, which is most of what this series is.

One thing worth flagging, since it makes the three look even heavier than they are. They are a stack, and because they come from a fork they can only target main, so each one carries its predecessors. What is actually new in each:

PR	shown on the page	new in this PR
#185	+4187/-620, 28 files	+4187/-620, 28 files
#186	+5745/-975, 34 files	+1570/-367, 14 files
#187	+9874/-1574, 61 files	+4327/-797, 46 files

Tests are 58 to 71 percent of each. That does not make them small, and I am not arguing the point: even the new-only figures are several times the size of anything recently merged here. Just noting it so the review order is clear whenever you get to them: #185, then #186, then #187, each on top of the last.

I have opened #190 with one commit lifted out of this PR: ByteSize::parse multiplies without a check, so in a release build memory = "17179869184G" wraps to a ceiling of zero and the guest is SIGKILLed on its first allocation while /proc/meminfo reports it unlimited. It is 40 lines including the test, independent of everything else here, and reachable from every surface that takes a size. If it fits the release, take it; if not, it can ride along with this batch later.

No rush on the rest from my side.

Closes the second half of multikernel#174. The SDK carried its own TOML parser and its own grammars, and they had already diverged from the core in two places you found: `parse_memory_size` accepted fractions and a `T` suffix that `ByteSize::parse` rejects, and `time_start` went through `int()`, so an RFC 3339 stamp worked in the CLI and raised through the SDK. A third grammar sat unused in the dataclass, `time_start_timestamp`, with naive-means-UTC semantics. `sandlock_profile_parse` takes TOML text and returns canonical JSON with every micro-grammar already resolved: mounts as `{virt, host, ro}` objects, sizes as integer bytes, `time_start` as epoch seconds. The SDK's remaining job is a field-for-field copy into its dataclass, so introspection, `dataclasses.replace` and preset composition keep working. Unknown keys are rejected on both sides, so future drift fails at load time instead of mis-parsing silently. `sandbox_to_json` was not reusable as-is: it re-emits mounts as `V:H:ro` spec strings, which would have put string parsing straight back into the SDK. The canonical form emits structured mounts instead. Its `ro` is the effective setting for the virtual path, not the flag written on one spec: the core keys read-only mounts by virtual path (`Sandbox::fs_mount_ro` is a list of virtual paths), so two specs sharing a virtual path share one verdict, and reporting the written flag would describe a policy no layer applies. Public Python API changes, deliberately and without shims: max_memory: str | int | None -> int | None max_disk: str | None -> int | None time_start: float | str | None -> float | None fs_mount: Mapping[str, str] -> Sequence[Mount] The `time_start` row understates one break, because the annotation did: python/README.md documented the field as `datetime | float | str | None`, and the builder duck-typed anything carrying a `.timestamp()`. A `datetime` is therefore refused from here on, and an aware one needs that call spelled out. A naive one never had a defined meaning on this field anyway, since it was read in whatever zone the host happened to be in. `Mount(virt, host, ro)` is new and mirrors the canonical field names. `fs_mount` becoming a sequence is what lets a read-only mount be expressed at all from Python; it reaches the C ABI through the `fs_mount_ro` setter added in multikernel#180. `tomli` is gone from the dependencies. The memory-accounting tests under python/tests/test_sandbox.py spell their ceilings as integer bytes, and the disk-quota tests next to them are converted here for the same reason; a size string is profile syntax and the core resolves it, so `max_memory` gets it back as a string only once the C ABI setter takes one, later in this series. Two more changes to the same surface, both consequences of the SDK no longer holding an opinion of its own: - `on_error` loaded from a profile now defaults to COMMIT where it defaulted to ABORT. The canonical form always resolves both branch actions, and the SDK copies what it is handed, so a profile that says nothing about the error path gets the core's answer rather than the dataclass's second opinion. Deliberate, since the CLI, a profile and the Go SDK have always meant COMMIT for that policy, but it changes what happens to a COW branch for a profile already in use, and it changes it silently. Only the profile path moves; the dataclass default is untouched here. - `parse_memory_size`, `Sandbox.memory_bytes()` and `Sandbox.time_start_timestamp()` are removed. The first two were the SDK's byte-size grammar and its accessor, the third the unused third grammar named above. Nothing replaces them: the resolved value is the field. One core change came out of this rather than the SDK: rebuilding a builder from a parsed profile ran `extend_net_allow_for_http` a second time over an allowlist that already held its derived entries, so the helper is now idempotent, with a test. An earlier revision of this commit also carried the `checked_mul` fix in `ByteSize::parse`. That landed on its own as 80ffbb6 and is no longer part of this diff; the tests here that pin `byte size out of range` on `memory = "17179869184G"` now ride on the merged fix. Verified against the CLI message for message on every grammar: the same profile loads identically, or fails identically, through both paths, with one gap left open and pinned rather than papered over. `sandlock_sandbox_builder_time_start` takes a `uint64` of seconds, so a stamp the core keeps in full loads from a profile and then cannot be handed to a builder: `"2026-01-01T00:00:00.5Z"` and any instant before 1970 are what that costs. The SDK refuses them by name instead of wrapping a negative value through an unsigned setter, and `test_time_start_the_c_abi_cannot_carry_is_refused_loudly` holds it there. Closing the gap means changing that setter's signature, which is a later commit in this series.

congwang-mk

Automated review of this PR (8 finder angles, individually verified; 9 CONFIRMED, 1 PLAUSIBLE posted inline).

One additional PLAUSIBLE note, cut by the findings cap: a profile with duplicate virtual-path mounts (mount = ["/w:/h1", "/w:/h2"]) changes meaning through the SDK. The old SDK collapsed to a last-wins dict (only /h2 registered); the new path registers both and the core's longest-prefix dispatch picks the first entry, so the effective backing host flips from /h2 to /h1. This matches what the CLI already did, so it is the parity goal working as intended, but the PR description never mentions that a duplicate-virt profile's effective host changes; worth a line in the description.

Verified and refuted (not posted): the on_error ABORT-to-COMMIT flip, run-time refusal of fractional/pre-epoch time_start, and policy_from_dict removal are documented or private-API; the FFI error-protocol duplication and CanonicalBranchAction parallel type are deliberate and drift-safe; the clone-and-double-walk, O(n^2) ro scan, and pretty-JSON candidates are negligible on this cold path.

congwang-mk · 2026-08-10T22:42:30Z

+    for spec in specs {
+        let rule = HttpRule::parse(spec).map_err(SandlockError::Sandbox)?;
+        out.push(CanonicalHttpRule {
+            spec: super::format_http_rule(&rule),


[CONFIRMED] CanonicalHttpRule.spec does not round-trip paths with percent-encoded trailing whitespace, so the SDK silently enforces a different HTTP rule than the CLI.

Profile [http] allow = ["GET example.com/admin%20"]: normalize_path (http.rs:75) decodes to path "/admin " and format_http_rule (profile.rs:405) renders the spec with a bare trailing space and never re-encodes. The Python SDK forwards rule["spec"] verbatim (_profile.py:228-243) to sandlock_sandbox_builder_http_allow, where HttpRule::parse runs s.trim() (http.rs:22) and collapses the path to "/admin". The CLI allows exactly GET /admin%20 while the SDK loading the same profile allows GET /admin instead: a silently shifted allow rule. %09/%0A (tab/newline) hit the same trim. Re-percent-encoding trim-sensitive bytes in format_http_rule would close it.

congwang-mk · 2026-08-10T22:42:30Z

+/// Derived entries a caller already carries are not added twice. A policy can
+/// be taken apart and rebuilt (`sandlock run --profile-file` rebuilds a builder
+/// from the parsed profile, then applies flag overrides on top), and the
+/// rebuilt net allowlist arrives here already holding the entries this


[CONFIRMED] A deny-only profile with any [http] rules still fails the CLI rebuild with a spurious "mutually exclusive" error; this comment claims the round-trip is safe, but the failure fires before this function runs.

Profile with [network] deny = ["1.2.3.4"] plus [http] ports/rules: build() runs the allow/deny exclusivity check (builder.rs:976) before extend_net_allow_for_http (builder.rs:992) derives at least one NetAllow entry, so the first build passes and the resulting Sandbox has both lists non-empty. The CLI rebuild (main.rs:502-506) feeds the derived entry back as a user spec; on the second build() the exclusivity check sees allow+deny both non-empty and errors with zero override flags. push_unique cannot help because the error fires before extend runs, yet this doc comment and the new idempotency test assert the taken-apart-and-rebuilt flow is handled.

congwang-mk · 2026-08-10T22:42:30Z

            - ``net_allow: ["api.example.com:443"]``
            - ``env: {"KEY": "value"}``
-            - ``max_memory: "256M"``
+            - ``max_memory: 268435456`` (bytes)


[CONFIRMED] Third-party MCP tool manifests using the previously documented string form "sandlock:max_memory": "256M" now raise an unhandled TypeError at session registration, aborting every tool from that session.

capabilities_from_mcp_tool copies sandlock:* annotation values verbatim (lines 130-142) and policy_for_tool passes them into Sandbox(**kwargs) (lines 105-110) with no try/except; the new __post_init__ (sandbox.py:446-451) raises a raw TypeError. The string form was documented on main (python/README.md:715) and pinned in test_mcp.py. This docstring hunk updates the example, but the data comes from external manifests that do not update with the SDK. add_mcp_session builds all policies eagerly (mcp/_sandbox.py:168-174), so one legacy-annotated external tool kills registration for the whole session with no PolicyError or per-tool denial. Consider catching TypeError here and mapping it to the policy-denial path.

congwang-mk · 2026-08-10T22:42:30Z

@@ -740,6 +790,46 @@ def _encode(s: str) -> bytes:
        raise ValueError(f"NUL byte in string argument: {result!r}")


[CONFIRMED] Profiles whose canonical values carry an embedded NUL load successfully but every run()/start() raises this raw ValueError, while the CLI runs the same profile.

[network] allow = ["tcp://ex\u0000ample.com"]: the adversarial test a_nul_inside_a_value_is_carried_whole_not_truncated pins the NUL riding intact into the canonical JSON; profile_parse's b"\0" in encoded check only catches raw NULs in the TOML text, and json.loads restores the real NUL from the \u0000 escape. load_profile() builds the dataclass fine; the lazy native build at first run() then hits this _encode ValueError, a late non-PolicyError failure for a profile the pure-Rust CLI path accepts and runs. Rejecting NUL-bearing canonical strings at load time (or in the core) keeps the parity contract.

congwang-mk · 2026-08-10T22:42:30Z


+def _bind_ports(value: Any) -> list:
+    _check_keys(value, ("any", "ports"), "bind ports")
+    return ["*"] if value["any"] else list(value["ports"])


[CONFIRMED] Materializing the canonical port-range expansion onto Sandbox.net_allow_bind turns one spec into up to 64512 list entries and 64512 individual FFI builder calls per create()/run().

Profile allow_bind = ["1024-65535"]: the old SDK stored the single spec string and made one FFI call; the canonical form expands ranges (canonical.rs:197-204), this returns list(value["ports"]) verbatim, and _build_from_policy loops one sandlock_sandbox_builder_net_allow_bind call per port (_sdk.py:1196-1197), each with encode plus a Box round-trip (ffi lib.rs:464-473). Every repr, dataclasses.replace, and merge_cli_overrides now handles a 64512-element list. The FFI already accepts comma-joined specs, so batching (or keeping ranges compact on the dataclass) avoids the amplification.

congwang-mk · 2026-08-10T22:42:30Z

+    seconds, nanos = value["seconds"], value["nanoseconds"]
+    if nanos == 0:
+        return seconds
+    return seconds + nanos / 1_000_000_000


[CONFIRMED] The float64 conversion silently swallows sub-second time_start fractions below the double's ulp, contradicting the "refused loudly" guarantee.

time_start = "2026-01-01T00:00:00.0000001Z": canonical carries {seconds: 1767225600, nanoseconds: 100}, but seconds + nanos / 1_000_000_000 rounds to exactly 1767225600.0 (the ulp is about 238ns at this magnitude; fractions up to 119ns vanish), so _epoch_seconds sees a whole number and accepts it, silently dropping the offset the CLI would virtualize, while a 0.5s fraction is refused loudly. Whether a profile is refused thus depends on the year and the fraction size. Checking the exact nanoseconds field here (before the lossy float add) closes the hole; test_cli_parity.py:671 only exercises 0.5s.

congwang-mk · 2026-08-10T22:42:30Z

+            "time_start before the Unix epoch is not supported by the "
+            f"sandlock_sandbox_builder_time_start ABI: {value}"
+        )
+    if value != int(value):


[CONFIRMED] _epoch_seconds leaks uncontrolled error types for NaN/inf and silently masks time_start=2**64 to epoch 0 through ctypes.

Sandbox(time_start=float("nan")).run(): value < 0 is False for NaN, then this line's int(value) raises the raw ValueError "cannot convert float NaN to integer" instead of the crafted message; float("inf") raises OverflowError. Unlike _bytes_limit there is no upper-bound check, and ctypes c_uint64 conversion silently masks 2**64 to 0 (verified by execution), so the sandbox runs with virtualized time at the 1970 epoch: a silent misconfiguration. Only reachable via direct assignment to the public Sandbox.time_start, but that is supported API; an isfinite plus range check here mirrors _bytes_limit.

congwang-mk · 2026-08-10T22:42:30Z

+        # syntax it came from. Accepting the syntax here would mean a second
+        # parser for the same grammar, which is what made a profile mean one
+        # thing through the CLI and another through this SDK.
+        for attr in ("max_memory", "max_disk"):


[PLAUSIBLE] __post_init__ guards only str for these fields, so datetime (which worked on main and is not in the breaking-changes list), float byte counts, bool, and negative time_start construct fine and fail only deep inside run()/create() with differently worded errors.

Sandbox(time_start=datetime.now(timezone.utc)) was accepted on main via hasattr(time_start, "timestamp") (old _sdk.py:1145); it now constructs and dies at first run() with a late TypeError from _epoch_seconds. Sandbox(max_memory=True) or time_start=-5 likewise pass construction and fail at build time via _bytes_limit/_epoch_seconds (_sdk.py:794-830). Where and how a bad value fails depends on its Python type, and the datetime break is undocumented. Running the full _bytes_limit/_epoch_seconds checks here keeps one validation site and fail-fast timing.

congwang-mk · 2026-08-10T22:42:30Z

+        None => BranchAction::default(),
+    };
+
+    Ok(CanonicalProfile {


[CONFIRMED] resolve() and resolve_net_rules copy ProfileInput/NetRule field-by-field with plain field access and no exhaustive destructuring or completeness test, so a future field is silently dropped from canonical JSON.

A contributor adds a key to ProfileInput (or NetRule, which already grew all_ports with serde(default)); parse_input accepts it, this function still compiles because it only names fields it already copies (lines 362-428, 482-508), and no test compares ProfileInput's key set to the emitted JSON. Python's _check_keys validates against _SECTIONS, which equally lacks the new key, so the profile works through the CLI while every binding silently ignores the key: the exact drift class this PR exists to eliminate. Exhaustive destructuring (no .. rest patterns) makes the compiler enforce the mirror.

congwang-mk · 2026-08-10T22:42:30Z

        return;
    }

+    fn push_unique(net_allow: &mut Vec<NetAllow>, rule: NetAllow) {


[CONFIRMED] Exact-equality dedup only handles the unchanged-inputs rebuild; when derivation inputs differ between passes, pass-1 residue survives as grants nothing requested, wider than a single-pass build.

Profile with bare http.ports (pass 1 derives AnyIp:[80] via the empty-rules branch, lines 217-224) run with --http-allow "GET api.example.com/*": pass 2 derives only the concrete host, but the carried AnyIp:[80] entry survives as a user spec (NetAllow has no provenance field), defeating the "concrete hosts tighten the allowlist" contract stated at lines 180-181. The residue predates this PR, but the new comment and idempotency test only cover the identical-inputs case; tagging derived entries or re-deriving from the raw profile instead of the effective policy fixes it fully.

congwang-mk · 2026-08-23T05:26:19Z

@dzerik any comment or update?

This was referenced Aug 4, 2026

builder: latch a rejected setter argument instead of coercing it #186

Open

ffi: take sizes and timestamps as strings; both SDKs stop parsing #187

Open

dzerik mentioned this pull request Aug 6, 2026

fix(core): refuse a byte size that does not fit instead of wrapping it to zero #190

Merged

dzerik force-pushed the feat/core-canonical-profile branch from 89d9b11 to 59e811d Compare August 9, 2026 09:41

congwang-mk reviewed Aug 10, 2026

View reviewed changes

		@@ -740,6 +790,46 @@ def _encode(s: str) -> bytes:
		raise ValueError(f"NUL byte in string argument: {result!r}")

Uh oh!

Conversation

dzerik commented Aug 4, 2026

The problem

What this does

Breaking changes

Two core fixes that came out of this

One gap left open and pinned

Position in the series

Testing

Uh oh!

congwang-mk commented Aug 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dzerik commented Aug 6, 2026

Uh oh!

congwang-mk left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

congwang-mk commented Aug 23, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

congwang-mk commented Aug 5, 2026 •

edited

Loading