Skip to content
This repository has been archived by the owner on Aug 14, 2020. It is now read-only.

ace: add Linux-specific seccomp isolator #621

Merged
merged 3 commits into from
Jun 22, 2016
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
81 changes: 77 additions & 4 deletions actool/manifest.go
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,8 @@ var (
patchMounts string
patchPorts string
patchIsolators string
patchSeccompMode string
patchSeccompSet string

catPrettyPrint bool

Expand All @@ -69,6 +71,8 @@ var (
[--ports=query,protocol=tcp,port=8080[:query2,...]]
[--supplementary-groups=gid1,gid2,...]
[--isolators=resource/cpu,request=50m,limit=100m[:resource/memory,...]]
[--seccomp-mode=remove|retain[,errno=EPERM]]
[--seccomp-set=syscall1,syscall2,...]]
[--replace]
INPUT_ACI_FILE
[OUTPUT_ACI_FILE]`,
Expand Down Expand Up @@ -99,6 +103,8 @@ func init() {
cmdPatchManifest.Flags.StringVar(&patchMounts, "mounts", "", "Replace mount points")
cmdPatchManifest.Flags.StringVar(&patchPorts, "ports", "", "Replace ports")
cmdPatchManifest.Flags.StringVar(&patchIsolators, "isolators", "", "Replace isolators")
cmdPatchManifest.Flags.StringVar(&patchSeccompMode, "seccomp-mode", "", "Enable and configure seccomp isolator")
cmdPatchManifest.Flags.StringVar(&patchSeccompSet, "seccomp-set", "", "Set of syscalls for seccomp isolator enforcing")

cmdCatManifest.Flags.BoolVar(&catPrettyPrint, "pretty-print", false, "Print with better style")
}
Expand Down Expand Up @@ -211,7 +217,11 @@ func patchManifest(im *schema.ImageManifest) error {
if err != nil {
return fmt.Errorf("cannot parse capability %q: %v", patchCaps, err)
}
app.Isolators = append(app.Isolators, caps.AsIsolator())
isolator, err = caps.AsIsolator()
if err != nil {
return err
}
app.Isolators = append(app.Isolators, *isolator)
}
if patchRevokeCaps != "" {
isolator := app.Isolators.GetByName(types.LinuxCapabilitiesRevokeSetName)
Expand All @@ -225,7 +235,11 @@ func patchManifest(im *schema.ImageManifest) error {
if err != nil {
return fmt.Errorf("cannot parse capability %q: %v", patchRevokeCaps, err)
}
app.Isolators = append(app.Isolators, caps.AsIsolator())
isolator, err = caps.AsIsolator()
if err != nil {
return err
}
app.Isolators = append(app.Isolators, *isolator)
}

if patchMounts != "" {
Expand All @@ -250,6 +264,18 @@ func patchManifest(im *schema.ImageManifest) error {
}
}

// Parse seccomp args and override existing seccomp isolators
if patchSeccompMode != "" {
Copy link
Member

Choose a reason for hiding this comment

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

actool patch-manifest could return an error when patchSeccompMode == "" but patchSeccompSet != "".

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 realized I didn't make it clear and my code is making some assumptions that set may be empty. On a second thought, I think I am not sure there are actual usecases for this. Should I make it explicit that set MUST NOT be empty and just kill this corner case?

Copy link
Member

Choose a reason for hiding this comment

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

I was referring to the other side: if the set is not empty but the mode is not given (e.g. the user called actool patch-manifest --seccomp-set=chroot). In this case, the current code in actool would just ignore the seccomp parameter and not add any isolator. I think actool should return an error in that case instead of silently doing nothing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, fixed that. Opinions regarding the other question?

Copy link
Member

Choose a reason for hiding this comment

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

I'm ok with saying set MUST NOT be empty. It's easier to change that in that direction if we ever need to allow empty set later.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ack, done.

seccompIsolator, err := parseSeccompArgs(patchSeccompMode, patchSeccompSet)
if err != nil {
return err
}
seccompReps := []types.ACIdentifier{types.LinuxSeccompRemoveSetName, types.LinuxSeccompRetainSetName}
app.Isolators.ReplaceIsolatorsByName(*seccompIsolator, seccompReps)
} else if patchSeccompSet != "" {
return fmt.Errorf("--seccomp-set specified without --seccomp-mode")
}

if patchIsolators != "" {
isolators := strings.Split(patchIsolators, ":")
for _, is := range isolators {
Expand All @@ -260,14 +286,16 @@ func patchManifest(im *schema.ImageManifest) error {

_, ok := types.ResourceIsolatorNames[name]

if name == types.LinuxNoNewPrivilegesName {
switch name {
case types.LinuxNoNewPrivilegesName:
ok = true
kv := strings.Split(is, ",")
if len(kv) != 2 {
return fmt.Errorf("isolator %s: invalid format", name)
}

isolatorStr = fmt.Sprintf(`{ "name": "%s", "value": %s }`, name, kv[1])
case types.LinuxSeccompRemoveSetName, types.LinuxSeccompRetainSetName:
ok = false
}

if !ok {
Expand All @@ -284,6 +312,51 @@ func patchManifest(im *schema.ImageManifest) error {
return nil
}

// parseSeccompArgs parses seccomp mode and set CLI flags, preparing an
// appropriate seccomp isolator.
func parseSeccompArgs(patchSeccompMode string, patchSeccompSet string) (*types.Isolator, error) {
// Parse mode flag and additional keyed arguments.
var errno, mode string
args := strings.Split(patchSeccompMode, ",")
for _, a := range args {
kv := strings.Split(a, "=")
switch len(kv) {
case 1:
// mode, either "remove" or "retain"
mode = kv[0]
case 2:
// k=v argument, only "errno" allowed for now
if kv[0] == "errno" {
errno = kv[1]
} else {
return nil, fmt.Errorf("invalid seccomp-mode optional argument: %s", a)
}
Copy link
Member

Choose a reason for hiding this comment

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

else return error?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Defensively, sounds like a good idea.

default:
return nil, fmt.Errorf("cannot parse seccomp-mode argument: %s", a)
}
}

// Instantiate an Isolator with the content specified by the --seccomp-set parameter.
var err error
var seccomp types.AsIsolator
switch mode {
case "remove":
seccomp, err = types.NewLinuxSeccompRemoveSet(errno, strings.Split(patchSeccompSet, ",")...)
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like errno could be unset in this place if it's not in cmdline parameters. Is this desired?
Also this looks like errno should be declared as errno := "EPERM" around 326 line.

Copy link
Contributor

Choose a reason for hiding this comment

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

Other possibility - command line help could be misinterpreted as me as example shows ;)

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, docs are more clear in this, so IMO cmdline help is not so clear.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

errno is optional, so an empty string is a desired default. Command line help marks it as an optional parameter:

[--seccomp-mode=remove|retain[,errno=EPERM]]

In which way you don't find it clear? Any way I can improve it to be more immediate to grasp?

Copy link
Contributor

@jellonek jellonek Jun 2, 2016

Choose a reason for hiding this comment

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

Sorry for mess. It was unclear for me which errno values could be used in this place. After a break - it's clear for me to comment AFTER reading WHOLE diff...

case "retain":
seccomp, err = types.NewLinuxSeccompRetainSet(errno, strings.Split(patchSeccompSet, ",")...)
default:
err = fmt.Errorf("unknown seccomp mode %s", mode)
}
if err != nil {
return nil, fmt.Errorf("cannot parse seccomp isolator: %s", err)
}
seccompIsolator, err := seccomp.AsIsolator()
if err != nil {
return nil, err
}
return seccompIsolator, nil
}

// extractManifest iterates over the tar reader and locate the manifest. Once
// located, the manifest can be printed, replaced or patched.
func extractManifest(tr *tar.Reader, tw *tar.Writer, printManifest bool, newManifest []byte) error {
Expand Down
11 changes: 11 additions & 0 deletions examples/image.json
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,17 @@
{
"name": "os/linux/no-new-privileges",
"value": true
},
{
"name": "os/linux/seccomp-remove-set",
"value": {
"errno": "EACCESS",
"set": [
"clock_settime",
"clock_adjtime",
"reboot"
]
}
}
],
"mountPoints": [
Expand Down
3 changes: 3 additions & 0 deletions schema/types/app.go
Original file line number Diff line number Diff line change
Expand Up @@ -86,5 +86,8 @@ func (a *App) assertValid() error {
if err := a.Environment.assertValid(); err != nil {
return err
}
if err := a.Isolators.assertValid(); err != nil {
return err
}
return nil
}
52 changes: 52 additions & 0 deletions schema/types/isolator.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,10 +16,14 @@ package types

import (
"encoding/json"
"errors"
"fmt"
)

var (
isolatorMap map[ACIdentifier]IsolatorValueConstructor

ErrIncompatibleIsolator = errors.New("isolators set contains incompatible types")
)

func init() {
Expand All @@ -40,6 +44,29 @@ func AddIsolatorName(n ACIdentifier, ns map[ACIdentifier]struct{}) {
// and PodManifest schemas.
type Isolators []Isolator

// assertValid checks that every single isolator is valid and that
// the whole set is well built
func (isolators Isolators) assertValid() error {
typesMap := make(map[ACIdentifier]bool)
for _, i := range isolators {
if err := i.Value().AssertValid(); err != nil {
return err
}
if _, ok := typesMap[i.Name]; ok {
if !i.Value().multipleAllowed() {
return fmt.Errorf(`isolators set contains too many instances of type %s"`, i.Name)
}
}
for _, c := range i.Value().Conflicts() {
if _, found := typesMap[c]; found {
return ErrIncompatibleIsolator
}
}
typesMap[i.Name] = true
}
return nil
}

// GetByName returns the last isolator in the list by the given name.
func (is *Isolators) GetByName(name ACIdentifier) *Isolator {
var i Isolator
Expand All @@ -52,6 +79,22 @@ func (is *Isolators) GetByName(name ACIdentifier) *Isolator {
return nil
}

// ReplaceIsolatorsByName overrides matching isolator types with a new
// isolator, deleting them all and appending the new one instead
func (is *Isolators) ReplaceIsolatorsByName(newIs Isolator, oldNames []ACIdentifier) {
var i Isolator
for j := len(*is) - 1; j >= 0; j-- {
i = []Isolator(*is)[j]
for _, name := range oldNames {
if i.Name == name {
*is = append((*is)[:j], (*is)[j+1:]...)
}
}
}
*is = append((*is)[:], newIs)
return
}

// Unrecognized returns a set of isolators that are not recognized.
// An isolator is not recognized if it has not had an associated
// constructor registered with AddIsolatorValueConstructor.
Expand All @@ -69,8 +112,17 @@ func (is *Isolators) Unrecognized() Isolators {
// serialized as any arbitrary JSON blob. Specific Isolator types should
// implement this interface to facilitate unmarshalling and validation.
type IsolatorValue interface {
// UnmarshalJSON unserialize a JSON-encoded isolator
UnmarshalJSON(b []byte) error
// AssertValid returns a non-nil error value if an IsolatorValue is not valid
// according to appc spec
AssertValid() error
// Conflicts returns a list of conflicting isolators types, which cannot co-exist
// together with this IsolatorValue
Conflicts() []ACIdentifier
// multipleAllowed specifies whether multiple isolator instances are allowed
// for this isolator type
multipleAllowed() bool
}

// Isolator is a model for unmarshalling isolator types from their JSON-encoded
Expand Down
Loading