ace: add Linux-specific seccomp isolator #621

lucab · 2016-06-02T08:29:58Z

This is a proposal for two Linux-specific security isolators based on seccomp filters. It follows closely the capabilities isolators, where two modes are provided: "remove-set" (blacklisting) and "retain-set" (whitelisting).

Given the huge number of syscalls that can be specified this way and the on-going discussion about sensible defaults, this specification doesn't hardcode any list but let implementations some freedom to provide groups/wildcards via @-prefixed special values. Users can decide to provide their own custom filters, or just opt-in/opt-out those implementations-provided filters.

Fixes #529

jellonek · 2016-06-02T09:01:33Z

actool/manifest.go

+	var seccomp types.AsIsolator
+	switch mode {
+	case "remove":
+		seccomp, err = types.NewLinuxSeccompRemoveSet(errno, strings.Split(patchSeccompSet, ",")...)


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.

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

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

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?

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

lucab · 2016-06-10T07:33:23Z

@alban amended as per your suggestions.

Maintainers PTAL, if fine I'd like to see this landing soon.

jonboulle · 2016-06-14T09:34:40Z

actool/manifest.go

@@ -250,6 +264,31 @@ func patchManifest(im *schema.ImageManifest) error {
 		}
 	}

+	if patchSeccompMode != "" {
+		i := 0


I'm confused by this change; seems like you're being careful to remove any existing isolator by the given name - but we weren't (and still aren't) doing this for the other cases (like patchRevokeCaps), instead just appending to the list.

I suppose it's underspecified in the spec that the list can contain multiple entries of a particular name, and we're arguably just skirting around this today by having GetByName return the last in the list. But can we split this out so that we perform consistently for the new isolator types, and then clear that up in a subsequent change?

Correct, I made it explicit in the two seccomp isolators, but behavior for others is not specified. At which level do you want to split this out? Something like introducing a ReplaceIsolatorsByName(Isolator, []string)?

hmm; I don't want to drag you into too much of a yakshave here. you've just unfortunately highlighted this issue that's now going to nag me :-)

Perhaps the minimal change to this diff to appease my pedantry would be to add sections to the other isolators to say that they "may be listed multiple times but only the last will be considered" or something to that effect. Then we're not conflating that behavioural change with the addition of seccomp stuff.

Then, subsequently, we could have ReplaceIsolatorsByName as you suggest, and tighten up the requirements for the other isolators too. WDYT?

I need to rework this anyway: as you tangentially highlighted, I'm replacing just the last isolators in the array instead of all of them. As such, I need something like ReplaceIsolatorsByName even if just for seccomp.

I don't think it makes much sense to partially clarify an unspecified behavior now in a half-baked way, just to re-visit and perhaps change it soon later. Plus, there are several other corner-cases which may be worth to address (eg. precedence of multiple isolators in pod+app scopes). Some details have been unspecified so far and I also like to disambiguate them, but I don't agree on rushing to do it here.

I don't think it's half-baked, it's just specifying the current (unfortunate) behaviour. How do you want to move forward here?

Leave it unspecified for the moment and then clarify a sane behavior once for all. I think somewhere else in the code there are already checks/assumptions about unique isolator entries, and I pretty much prefer tightening the specs instead of codifying accidental behaviors.

OK, can you file a follow up for that and we can get this in?

Sure! Follow-up is at #625.

jonboulle · 2016-06-15T12:09:08Z

@lucab ready for review?

lucab · 2016-06-15T12:11:53Z

@jonboulle rebased once more as travis catched an additional error case. It should be ready for another round now, yes.

jonboulle · 2016-06-15T14:17:04Z

schema/types/isolator.go

+		if err := i.Value().AssertValid(); err != nil {
+			return err
+		}
+		if typesMap[i.Name]+1 > i.Value().MaxInstances() {


This check returning an error about duplicates presumes that MaxInstances is always 1 - perhaps "isolators set contains too many of type %s" or something

Ack, changed to a more neutral wording.

lucab · 2016-06-15T21:53:51Z

@jonboulle latest revision accounts for your further review (also left some additional comments inline).

jonboulle · 2016-06-16T12:29:46Z

actool/manifest.go

@@ -284,6 +319,47 @@ 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.AsIsolator, error) {


Why does this return AsIsolator and not just Isolator?

jonboulle · 2016-06-16T12:32:14Z

schema/types/isolator.go

+			return err
+		}
+		if typesMap[i.Name]+1 > i.Value().MaxInstances() {
+			return fmt.Errorf(`isolators set contains too many instances of type %s"`, i.Name)


Check out https://github.com/appc/spec/blob/master/schema/types/errors.go#L26 for an alternative way to go here

Thanks for the hint, I see this is part of what we were discussing today. I think it won't fit with the surrounding style here, but I'll consider it for the future.

lucab · 2016-06-16T16:50:01Z

@jonboulle would you mind another round?

lucab · 2016-06-21T08:06:08Z

@jonboulle @alban anything else needing rework here?

alban · 2016-06-21T09:22:55Z

schema/types/isolator.go

+// 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) uint {
+	var numReplacements uint = 0


Why bothering counting numReplacements? the only caller of this function ignore the returned value.

That's its only way to signal if it actually replaced anything. While the current caller doesn't use it, I don't think it's a good idea to remove this observable return value completely.

jonboulle · 2016-06-21T13:32:28Z

A couple small open questions around style but generally this LGTM. @lucab let me know if you want to change those or just move forward with this as-is.

jonboulle · 2016-06-22T13:38:46Z

schema/types/isolator_linux_specific_test.go

+		set   []string
+		errno string
+
+		expectedSet   []LinuxSeccompEntry


you went in the wrong direction here - the rest of the file is pretty consistent with w/want :-(

jonboulle · 2016-06-22T13:39:17Z

LGTM

This was referenced Jun 2, 2016

spec: add seccomp isolator for Linux #529

Closed

[WIP] ace: initial seccomp proposal #620

Closed

app-level seccomp isolator rkt/rkt#1614

Closed

jellonek reviewed Jun 2, 2016
View reviewed changes

lucab force-pushed the to-upstream/seccomp-isolators branch 3 times, most recently from 920fc7e to 31925ae Compare June 7, 2016 07:58

jonboulle reviewed Jun 14, 2016
View reviewed changes

lucab force-pushed the to-upstream/seccomp-isolators branch 2 times, most recently from 3e2e69b to a655b6e Compare June 15, 2016 12:06

jonboulle reviewed Jun 15, 2016
View reviewed changes

lucab force-pushed the to-upstream/seccomp-isolators branch from a655b6e to 9aa8eda Compare June 15, 2016 21:44

jonboulle reviewed Jun 16, 2016
View reviewed changes

lucab force-pushed the to-upstream/seccomp-isolators branch from 9aa8eda to fc30bbf Compare June 16, 2016 16:24

lucab force-pushed the to-upstream/seccomp-isolators branch 2 times, most recently from b183ae9 to b931ddc Compare June 17, 2016 16:32

alban reviewed Jun 21, 2016
View reviewed changes

spec/ace: add two linux-specific seccomp isolators

d77287e

lucab force-pushed the to-upstream/seccomp-isolators branch from b931ddc to 1c5a245 Compare June 21, 2016 13:25

lucab added 2 commits June 21, 2016 16:05

schema/types: implement linux-specific seccomp isolators

65a06aa

actool: add support for patching seccomp isolators

682d131

lucab force-pushed the to-upstream/seccomp-isolators branch from 1c5a245 to 682d131 Compare June 21, 2016 14:06

jonboulle reviewed Jun 22, 2016
View reviewed changes

jonboulle merged commit 171e17c into appc:master Jun 22, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ace: add Linux-specific seccomp isolator #621

ace: add Linux-specific seccomp isolator #621

lucab commented Jun 2, 2016

jellonek Jun 2, 2016

jellonek Jun 2, 2016

jellonek Jun 2, 2016

lucab Jun 2, 2016

jellonek Jun 2, 2016 •

edited

Loading

lucab commented Jun 10, 2016

jonboulle Jun 14, 2016

lucab Jun 14, 2016

jonboulle Jun 14, 2016

lucab Jun 14, 2016

jonboulle Jun 14, 2016

lucab Jun 14, 2016

jonboulle Jun 14, 2016

lucab Jun 14, 2016 •

edited

Loading

jonboulle commented Jun 15, 2016

lucab commented Jun 15, 2016

jonboulle Jun 15, 2016

lucab Jun 15, 2016

lucab commented Jun 15, 2016

jonboulle Jun 16, 2016

jonboulle Jun 16, 2016

lucab Jun 16, 2016

lucab commented Jun 16, 2016

lucab commented Jun 21, 2016

alban Jun 21, 2016

lucab Jun 21, 2016

jonboulle commented Jun 21, 2016

jonboulle Jun 22, 2016

jonboulle commented Jun 22, 2016

ace: add Linux-specific seccomp isolator #621

ace: add Linux-specific seccomp isolator #621

Conversation

lucab commented Jun 2, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jellonek Jun 2, 2016 • edited Loading

Choose a reason for hiding this comment

lucab commented Jun 10, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lucab Jun 14, 2016 • edited Loading

Choose a reason for hiding this comment

jonboulle commented Jun 15, 2016

lucab commented Jun 15, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lucab commented Jun 15, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lucab commented Jun 16, 2016

lucab commented Jun 21, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonboulle commented Jun 21, 2016

Choose a reason for hiding this comment

jonboulle commented Jun 22, 2016

jellonek Jun 2, 2016 •

edited

Loading

lucab Jun 14, 2016 •

edited

Loading