Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement json frontend #9

Merged
merged 12 commits into from
Sep 23, 2021
27 changes: 27 additions & 0 deletions .buildkite/custom-tests.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
{
"tests": [
{
"test_name": "build-gnu-json",
"command": "RUSTFLAGS=\"-D warnings\" cargo build --release --features=json",
"platform": [
"x86_64",
"aarch64"
]
},
{
"test_name": "build-musl-json",
"command": "RUSTFLAGS=\"-D warnings\" cargo build --release --features=json --target {target_platform}-unknown-linux-musl",
"platform": [
"x86_64",
"aarch64"
]
},
{
"test_name": "validate-syscall-tables",
"command": "tools/generate_syscall_tables.sh --test",
"platform": [
"x86_64"
]
}
]
}
2 changes: 1 addition & 1 deletion .github/dependabot.yml
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ updates:
- package-ecosystem: cargo
directory: "/"
schedule:
interval: daily
interval: weekly
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why?

Copy link
Member

Choose a reason for hiding this comment

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

There are too many ops related PRs otherwise. This is a change at the rust-vmm org level.

open-pull-requests-limit: 10
- package-ecosystem: gitsubmodule
directory: "/"
Expand Down
7 changes: 6 additions & 1 deletion Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -9,5 +9,10 @@ keywords = ["seccomp", "jail", "sandbox"]
license = "Apache-2.0 OR BSD-3-Clause"
edition = "2018"
andreeaflorescu marked this conversation as resolved.
Show resolved Hide resolved

[features]
json = ["serde", "serde_json"]

[dependencies]
libc = ">=0.2.98"
libc = "^0.2.39"
serde = { version = "^1.0.27", features = ["derive"], optional = true}
serde_json = {version = "^1.0.9", optional = true}
7 changes: 4 additions & 3 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -256,9 +256,12 @@ let filters: BpfMap = seccompiler::compile_from_json(
categories to BPF programs.

```rust
pub type BpfMap = HashMap<String, Arc<BpfProgram>>;
pub type BpfMap = HashMap<String, BpfProgram>;
Copy link
Member

Choose a reason for hiding this comment

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

nit: can you add some details about what is changed in the documentation.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This type was not exported beforehand, it's introduced by this PR.
It was using an Arc in the beginning as a leftover from the use case we have in Firecracker.
But it's actually an implementation detail that needs to be handled in Firecracker (i.e. Map the BpfProgram to an Arc), since it's usecase-specific.

Copy link
Member

Choose a reason for hiding this comment

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

I meant to add some details in the commit message, sorry about that.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

```

Note that, in order to use the JSON functionality, you need to add the `json`
feature when importing the library.

For **Rust filters**, it’s enough to perform a `try_into()` cast, from a
`SeccompFilter` to a `BpfProgram`:

Expand All @@ -284,8 +287,6 @@ seccompiler::apply_filter(&bpf_prog)?;
It’s interesting to note that installing the filter does not take ownership or
invalidate the BPF program, thanks to the kernel which performs a
`copy_from_user` on the program before installing it.
This is why `BpfMap` entries map to `Arc<BpfProgram>`, so that they can
be shared across threads of the same category, avoiding copies.

## Seccomp best practices

Expand Down
4 changes: 2 additions & 2 deletions coverage_config_aarch64.json
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
{
"coverage_score": 0,
"exclude_path": "tests/integration_tests.rs",
"crate_features": ""
"exclude_path": "tests/integration_tests.rs,tests/json.rs",
"crate_features": "json"
}
6 changes: 3 additions & 3 deletions coverage_config_x86_64.json
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
{
"coverage_score": 87.3,
"exclude_path": "tests/integration_tests.rs",
"crate_features": ""
"coverage_score": 93.3,
"exclude_path": "tests/integration_tests.rs,tests/json.rs",
"crate_features": "json"
}
2 changes: 1 addition & 1 deletion src/backend/filter.rs
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,7 @@ impl SeccompFilter {
/// ```
///
/// [`SeccompRule`]: struct.SeccompRule.html
/// [`SeccompAction`]: struct.SeccompAction.html
/// [`SeccompAction`]: enum.SeccompAction.html
pub fn new(
rules: BTreeMap<i64, Vec<SeccompRule>>,
mismatch_action: SeccompAction,
Expand Down
20 changes: 18 additions & 2 deletions src/backend/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,9 @@ pub use condition::SeccompCondition;
pub use filter::SeccompFilter;
pub use rule::SeccompRule;

#[cfg(feature = "json")]
use serde::Deserialize;

use core::fmt::Formatter;
use std::convert::TryFrom;
use std::fmt::Display;
Expand All @@ -26,7 +29,7 @@ use bpf::{
pub use bpf::{sock_filter, BpfProgram, BpfProgramRef};

/// Backend Result type.
pub type Result<T> = std::result::Result<T, Error>;
Copy link
Member

Choose a reason for hiding this comment

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

Why is this now private?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Well it was not required to be public.
Users of the library will not interact directly with the Backend and therefore do not need access to this type.
I think there should only be one Result type exported and that is the Result from src/lib.rs

type Result<T> = std::result::Result<T, Error>;

/// Backend-related errors.
#[derive(Debug, PartialEq)]
Expand All @@ -43,6 +46,8 @@ pub enum Error {
InvalidTargetArch(String),
}

impl std::error::Error for Error {}

impl Display for Error {
fn fmt(&self, f: &mut Formatter) -> std::fmt::Result {
use self::Error::*;
Expand Down Expand Up @@ -101,6 +106,11 @@ impl TryFrom<&str> for TargetArch {
}

/// Comparison to perform when matching a condition.
#[cfg_attr(
feature = "json",
derive(Deserialize),
serde(rename_all = "snake_case")
)]
#[derive(Clone, Debug, PartialEq)]
pub enum SeccompCmpOp {
/// Argument value is equal to the specified value.
Expand All @@ -120,6 +130,7 @@ pub enum SeccompCmpOp {
}

/// Seccomp argument value length.
#[cfg_attr(feature = "json", derive(Deserialize), serde(rename_all = "lowercase"))]
#[derive(Clone, Debug, PartialEq)]
pub enum SeccompCmpArgLen {
/// Argument value length is 4 bytes.
Expand All @@ -129,6 +140,11 @@ pub enum SeccompCmpArgLen {
}

/// Actions that a seccomp filter can return for a syscall.
#[cfg_attr(
feature = "json",
derive(Deserialize),
serde(rename_all = "snake_case")
)]
#[derive(Clone, Debug, PartialEq)]
pub enum SeccompAction {
/// Allows syscall.
Expand All @@ -154,7 +170,7 @@ impl From<SeccompAction> for u32 {
///
/// * `action` - The [`SeccompAction`] that the kernel will take.
///
/// [`SeccompAction`]: struct.SeccompAction.html
/// [`SeccompAction`]: enum.SeccompAction.html
fn from(action: SeccompAction) -> Self {
match action {
SeccompAction::Allow => SECCOMP_RET_ALLOW,
Expand Down
2 changes: 1 addition & 1 deletion src/backend/rule.rs
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@ impl SeccompRule {
) {
// Tries to detect whether prepending the current condition will produce an unjumpable
// offset (since BPF conditional jumps are a maximum of 255 instructions, which is
// std::u8::MAX).
// u8::MAX).
if offset.checked_add(CONDITION_MAX_LEN + 1).is_none() {
// If that is the case, three additional helper jumps are prepended and the offset
// is reset to 1.
Expand Down
Loading