Add extension API for Linux sandbox control #1234
Conversation
cd-work
force-pushed
the
landlock_versioning
branch
from
bd91108
to
522b0f3
Compare
This patch adds two new fields to the `runSandboxed` extension API which allow controlling the minimum required Linux landlock ABI version and whether the command should still be executed when sandboxing is not possible. With the `best_effort` flag set, this also allows running extensions on Windows now with the sandbox disabled when this flag is set.
| // Do not add any default exceptions for strict sandboxes. | ||
| if strict { | ||
| return Ok(birdcage); | ||
| } |
There was a problem hiding this comment.
Moved this inside the function since I think it's better to handle it all in one place.
| Err(_) if best_effort => { | ||
| log::debug!("Landlock v{min_landlock_abi} is not supported, skipping sandbox"); | ||
| return Ok(()); | ||
| }, |
There was a problem hiding this comment.
Technically sandboxing can still fail when add_exception fails or lock fails. However this shouldn't be a landlock ABI error at that point so I don't think best_effort should apply?
There might be some scenarios we want to include into best_effort in the future, we could for example make ignore an error in lock when both seccomp and namespaces are blocked. But with our current behavior we should support pretty much every system so I don't think there's a need to relax things even more.
There was a problem hiding this comment.
Even though these new features were added as arguments to a hidden phylum sandbox command, should there still be a CHANGELOG.md entry?
Local testing has not happened yet since there are stray entries in cli/Cargo.toml preventing building. Plus, it looks like this is a draft now and there might be more changes coming. Local testing will happen after this PR comes out of draft.
| @@ -64,7 +64,7 @@ once_cell = "1.12.0" | |||
| deno_runtime = { version = "0.122.0" } | |||
| deno_core = { version = "0.199.0" } | |||
| deno_ast = { version = "0.27.2", features = ["transpiling"] } | |||
| birdcage = { version = "0.3.1" } | |||
| birdcage = { version = "0.3.1", path = "../../birdcage" } | |||
There was a problem hiding this comment.
Is this path entry left over from local development?
There was a problem hiding this comment.
That's why this is still a draft, will need to wait for the birdcage PR to be merged.
| #[serde(default)] | ||
| ignore_sandboxing_errors: bool, | ||
| #[serde(default)] | ||
| minimum_landlock_version: u8, |
There was a problem hiding this comment.
These need to be documented in extensions/phylum.ts
| let abi = match min_landlock_abi { | ||
| 3 => LANDLOCK_ABI::V3, | ||
| 2 => LANDLOCK_ABI::V2, | ||
| _ => LANDLOCK_ABI::V1, |
There was a problem hiding this comment.
This seems risky... If Landlock v4 comes out and somebody tries to set min_landlock_abi to 4 before we make an update, it's unexpected that they would get v1...
I think the intent was 0 | 1 => LANDLOCK_ABI::V1, which I support. But values greater than 3 should error as unsupported...
There was a problem hiding this comment.
I just wanted to avoid having to error out, but I suppose I could just do that.
| if !process.exceptions.best_effort { | ||
| Err(anyhow!("Extension sandboxing is not supported on this platform")) | ||
| } |
There was a problem hiding this comment.
Missing return...
| if !process.exceptions.best_effort { | |
| Err(anyhow!("Extension sandboxing is not supported on this platform")) | |
| } | |
| if !process.exceptions.best_effort { | |
| return Err(anyhow!("Extension sandboxing is not supported on this platform")); | |
| } |
| if !process.exceptions.best_effort { | ||
| Err(anyhow!("Extension sandboxing is not supported on this platform")) | ||
| } | ||
|
|
There was a problem hiding this comment.
We've got a debug message when the min landlock version isn't met. We should probably have a similar message here (and anywhere else that we use best_effort in the future).
| log::debug!("Sandboxing unsupported on this platform, skipping sandbox") | |
|
Made obsolete with phylum-dev/birdcage#47. |
This patch adds two new fields to the
runSandboxedextension API which allow controlling the minimum required Linux landlock ABI version and whether the command should still be executed when sandboxing is not possible.