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.

Sign up for GitHub

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

Add Architecture field to Seccomp configuration in Linux runtime #200

Merged
merged 3 commits into from
Sep 23, 2015
Merged

Add Architecture field to Seccomp configuration in Linux runtime #200

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
5087435
Add Architecture field to Seccomp configuration in Linux runtime
mheon Sep 23, 2015
5fd7dce
Add Seccomp constants to Linux runtime config
mheon Sep 23, 2015
215d0d9
Add Seccomp constants to description of Linux runtime spec
mheon Sep 23, 2015
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 34 additions & 1 deletion runtime-config-linux.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -319,11 +319,44 @@ For more information about Apparmor, see [Apparmor documentation](https://wiki.u
Seccomp provides application sandboxing mechanism in the Linux kernel.
Seccomp configuration allows one to configure actions to take for matched syscalls and furthermore also allows matching on values passed as arguments to syscalls.
For more information about Seccomp, see [Seccomp kernel documentation](https://www.kernel.org/doc/Documentation/prctl/seccomp_filter.txt)
The actions and operators are strings that match the definitions in seccomp.h from [libseccomp](/~https://github.com/seccomp/libseccomp) and are translated to corresponding values.
The actions, architectures, and operators are strings that match the definitions in seccomp.h from [libseccomp](/~https://github.com/seccomp/libseccomp) and are translated to corresponding values.
A valid list of constants as of Libseccomp v2.2.3 is contained below.
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't have a clear picture of how this sort of thing is going to get versioned. If I create a bundle against the v0.2.0 spec (assuming this PR lands in that version), am I always going to be allowed to use the architectures listed below? I.e. are runtimes compiled against earlier versions of libseccomp (that don't support all of these) nonconformant with v0.2.0? Will future versions of libseccomp always support these strings? I.e. are runtimes compiled against later versions of libseccomp (e.g. that no longer support SCMP_ARCH_X86) nonconformant with v0.2.0? Those seem like difficult requirements to enforce without binding the spec to a particular (range) of libseccomp versions, but without them I'm not sure how the bundle author should know the valid strings.

Copy link
Member

Choose a reason for hiding this comment

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

@mheon maybe we should not use the actual values that libseccomp expects because it would be hard to version and have spec stability if some low level detail like what strings libseccomp expects leaks out via the spec.

What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

On Wed, Sep 23, 2015 at 01:34:39PM -0700, Michael Crosby wrote:

@mheon maybe we should not use the actual values that libseccomp
expects because it would be hard to version and have spec stability
if some low level detail like what strings libseccomp expects leaks
out via the spec.

The bundle author will need to know what to use, regardless of whether
we list these in the spec ;). So one way is to code the allowed
values into the spec, so it's a contract between the spec and bundle
author. Another way is to allow contract directly between the
runtime's libseccomp and the bundle author (so the spec just provides
a way for the author to specify the libseccomp version they're writing
to). Maybe this is a use-case for my proposed version structure 1?
The bundle author would use something like:

"version": {
"openContainerSpecification": "0.2.0",
"libseccomp": "2.2.0",
}

And then be compatible with runtimes that support spec versions <0.3.0
which are compiled against libseccomp versions <3.0.0 (assuming
libseccomp semantically versions their header).

Copy link
Member

Choose a reason for hiding this comment

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

@wking No, the goal is to abstract away some of these lower level details and dependencies.

Copy link
Contributor

Choose a reason for hiding this comment

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

On Wed, Sep 23, 2015 at 02:00:38PM -0700, Michael Crosby wrote:

@wking No, the goal is to abstract away some of these lower level
details and dependencies.

If the goal is to make this spec the full bundle-author contract, then
I think you need an explicit list of allowed constants (although that
might be a link to libseccomp's v2.2.3 header. So long as there's no
ambiguity about whether a given constant is valid for this version of
the OCI spec or not). Then conformant runtimes will have to support
all of those values (although they could optionally support more).

Copy link
Contributor

Choose a reason for hiding this comment

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

On Wed, Sep 23, 2015 at 02:13:29PM -0700, W. Trevor King wrote:

Wed, Sep 23, 2015 at 02:00:38PM -0700, Michael Crosby:

@wking No, the goal is to abstract away some of these lower level
details and dependencies.

If the goal is to make this spec the full bundle-author contract, then
I think you need an explicit list of allowed constants…

This PR landed, so I've spun off my suggestion for more explicit (and
less libseccomp-dependent) phrasing into #201.


Architecture Constants
* `SCMP_ARCH_X86`
* `SCMP_ARCH_X86_64`
* `SCMP_ARCH_X32`
* `SCMP_ARCH_ARM`
* `SCMP_ARCH_AARCH64`
* `SCMP_ARCH_MIPS`
* `SCMP_ARCH_MIPS64`
* `SCMP_ARCH_MIPS64N32`
* `SCMP_ARCH_MIPSEL`
* `SCMP_ARCH_MIPSEL64`
* `SCMP_ARCH_MIPSEL64N32`

Action Constants:
* `SCMP_ACT_KILL`
* `SCMP_ACT_TRAP`
* `SCMP_ACT_ERRNO`
* `SCMP_ACT_TRACE`
* `SCMP_ACT_ALLOW`

Operator Constants:
* `SCMP_CMP_NE`
* `SCMP_CMP_LT`
* `SCMP_CMP_LE`
* `SCMP_CMP_EQ`
* `SCMP_CMP_GE`
* `SCMP_CMP_GT`
* `SCMP_CMP_MASKED_EQ`

```json
"seccomp": {
"defaultAction": "SCMP_ACT_ALLOW",
"architectures": [
"SCMP_ARCH_X86"
],
"syscalls": [
{
"name": "getcwd",
Expand Down
37 changes: 37 additions & 0 deletions runtime_config_linux.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -235,15 +235,52 @@ type Device struct {
// Seccomp represents syscall restrictions
type Seccomp struct {
DefaultAction Action `json:"defaultAction"`
Architectures []Arch `json:"architectures"`
Syscalls []*Syscall `json:"syscalls"`
}

// Additional architectures permitted to be used for system calls
// By default only the native architecture of the kernel is permitted
type Arch string
Copy link
Member

Choose a reason for hiding this comment

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

just a string? ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, there are 3 other types here which are just string... Didn't want this one to stand out :-)

Wouldn't mind changing all of them at once, though that might be worthy of a separate PR

Copy link
Member

Choose a reason for hiding this comment

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

The only other one that i see is Operator. Namespaces is a type because it has constants defined for it so its ok that it stays a type. Maybe if we define operators for type Operator string it would be better.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was also thinking of Action, which is another just-a-string.

I agree that adding the relevant constants to the config is a good idea - I'll see about doing that


const (
ArchX86 Arch = "SCMP_ARCH_X86"
ArchX86_64 Arch = "SCMP_ARCH_X86_64"
ArchX32 Arch = "SCMP_ARCH_X32"
ArchARM Arch = "SCMP_ARCH_ARM"
ArchAARCH64 Arch = "SCMP_ARCH_AARCH64"
ArchMIPS Arch = "SCMP_ARCH_MIPS"
ArchMIPS64 Arch = "SCMP_ARCH_MIPS64"
ArchMIPS64N32 Arch = "SCMP_ARCH_MIPS64N32"
ArchMIPSEL Arch = "SCMP_ARCH_MIPSEL"
ArchMIPSEL64 Arch = "SCMP_ARCH_MIPSEL64"
ArchMIPSEL64N32 Arch = "SCMP_ARCH_MIPSEL64N32"
)

// Action taken upon Seccomp rule match
type Action string

const (
ActKill Action = "SCMP_ACT_KILL"
ActTrap Action = "SCMP_ACT_TRAP"
ActErrno Action = "SCMP_ACT_ERRNO"
ActTrace Action = "SCMP_ACT_TRACE"
ActAllow Action = "SCMP_ACT_ALLOW"
)

// Operator used to match syscall arguments in Seccomp
type Operator string

const (
OpNotEqual Operator = "SCMP_CMP_NE"
OpLessThan Operator = "SCMP_CMP_LT"
OpLessEqual Operator = "SCMP_CMP_LE"
OpEqualTo Operator = "SCMP_CMP_EQ"
OpGreaterEqual Operator = "SCMP_CMP_GE"
OpGreaterThan Operator = "SCMP_CMP_GT"
OpMaskedEqual Operator = "SCMP_CMP_MASKED_EQ"
)

// Arg used for matching specific syscall arguments in Seccomp
type Arg struct {
Index uint `json:"index"`
Expand Down