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

Add Architecture field to Seccomp configuration in Linux runtime #200

Merged
merged 3 commits into from
Sep 23, 2015

Conversation

mheon
Copy link
Contributor

@mheon mheon commented Sep 23, 2015

By default, Seccomp filters will only permit syscalls to be made using the
native architecture of the kernel. This is fine for most use cases, but breaks
others (such as running 32-bit code in a container on a host with a 64-bit
kernel). This PR adds a field to specify additional architectures which may
make syscalls in a container secured by Seccomp.

As with other the other aspects of Seccomp, Libseccomp's seccomp.h is used to provide string constants which can be used to represent these additional architectures.

By default, Seccomp filters will only permit syscalls to be made using the
native architecture of the kernel. This is fine for most use cases, but breaks
others (such as running 32-bit code in a container on a host with a 64-bit
kernel). This patch adds a field to specify additional architectures which may
make syscalls.

Signed-off-by: Matthew Heon <mheon@redhat.com>
@mheon
Copy link
Contributor Author

mheon commented Sep 23, 2015

Ping @crosbymichael

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

Signed-off-by: Matthew Heon <mheon@redhat.com>
Signed-off-by: Matthew Heon <mheon@redhat.com>
@mheon
Copy link
Contributor Author

mheon commented Sep 23, 2015

@crosbymichael Added the relevant constants into the runtime config. I think they're valuable to have in the Go version of the spec, probably less so in the markdown description - considering converting that to a table so I can show descriptions

@crosbymichael
Copy link
Member

LGTM

@@ -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.

@mrunalp
Copy link
Contributor

mrunalp commented Sep 23, 2015

LGTM

mrunalp pushed a commit that referenced this pull request Sep 23, 2015
Add Architecture field to Seccomp configuration in Linux runtime
@mrunalp mrunalp merged commit 83e5943 into opencontainers:master Sep 23, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants