Check CAP_LAST_CAP while setting privileged #138

haiyanmeng · 2016-07-21T16:39:36Z

The PR checks whether each cap inside capability.List() is <= capability.CAP_LAST_CAP before adding it into finalCapList.
This avoids add unsupported cap into finalCapList. For example, CAP_AUDIT_READ is added into the kernel since Linux 3.16, and not supported on RHEL7.

Signed-off-by: Haiyan Meng hmeng@redhat.com

haiyanmeng · 2016-07-21T16:40:08Z

@mrunalp , PTAL.

wking · 2016-07-21T16:45:55Z

On Thu, Jul 21, 2016 at 09:39:36AM -0700, hmeng-19 wrote:

The PR checks whether each cap inside capability.List() is <=
capability.CAP_LAST_CAP before adding it into finalCapList.

This is host-specific (e.g. maybe you're using a host with an old
kernel to build a container for a host with a new kernel). ‘validate’
has a --host-specific flag to cover this sort of thing 1. Maybe we
should promote that to a global option?

haiyanmeng · 2016-07-21T16:49:08Z

@wking , I am testing octools on RHEL7 recently.
After running the following command, CAP_AUDIT_READ was also put into config.json. However, rhel7 does not support this cap, because its kernel is 3.10.

ocitools generate --privileged --tty --output=config.json --uidmappings=0:0:1024 --gidmappings=0:0:1024

wking · 2016-07-21T16:56:11Z

On Thu, Jul 21, 2016 at 09:49:08AM -0700, hmeng-19 wrote:

After running the following command, CAP_AUDIT_READ was also put
into config.json. However, rhel7 does not support this cap,
because its kernel is 3.10.

That's only a problem if you're also trying to run the container on
the same kernel that was used to generate the config. A system
running Linux 3.10 is capable of generating a config that uses
CAP_AUDIT_READ (or a Solaris config, or a Windows config). It just
can't run containers using those configs.

A --host-specific flag says “Don't be generic, I want to
generate/validate for the current host”. For you, that would end up
looking like:

$ ocitools --host-specific generate --privileged --tty --output=config.json --uidmappings=0:0:1024 --gidmappings=0:0:1024

haiyanmeng · 2016-07-21T17:01:54Z

@wking , sounds good. It would be nice we can take care both cases:

create config.json with ocitools and use the config.json with runc on the same machine;
create config.json with ocitools and use the config.json with runc on different machines;

It is easy to picture both use cases.

wking · 2016-07-21T17:07:28Z

On Thu, Jul 21, 2016 at 10:01:54AM -0700, hmeng-19 wrote:

It would be nice we can take care both cases:

Agreed, that's why I'm suggesting a global option ;).

haiyanmeng · 2016-07-21T17:08:23Z

@mrunalp , how do you think about this?

mrunalp · 2016-07-21T17:09:47Z

Sounds good to me.

haiyanmeng · 2016-07-21T19:13:54Z

@mrunalp , @wking , PTAL.

wking · 2016-07-21T19:28:39Z

cmd/ocitools/validate.go

@@ -84,7 +84,7 @@ var bundleValidateCommand = cli.Command{
 			return fmt.Errorf("The root path %q is not a directory.", rootfsPath)
 		}

-		hostCheck := context.Bool("host-specific")
+		hostCheck := hostSpecific || context.Bool("host-specific")


I don't think we want both a global and a per-command --host-specific here. We should drop the per-command option.

And I'm not all that familiar with urfave/cli, but do we really need the package-global hostSpecific? I'd expect you to be able to pull that from the context, even though it's set via a global variable.

I don't think we want both a global and a per-command --host-specific here. We should drop the per-command option.

I am okay with drop the per-command --host-specific option once @mrunalp and you are okay with it.
I keep it for now just in case some important users are using it.

And I'm not all that familiar with urfave/cli, but do we really need the package-global hostSpecific? I'd expect you to be able to pull that from the context, even though it's set via a global variable.

I use a global var here to avoid multiple func calls to context.GlobalBool. However, it does not make too much difference.

Ah, here's an example of accessing a global option.

On Thu, Jul 21, 2016 at 12:31:53PM -0700, hmeng-19 wrote:

I keep it for now just in case some important users are using it.

We haven't tagged an ocitools release yet or said anything about
forward compat in it's command-line API, so I think we're free to
change whatever we want. But if we know of any consumers, giving them
a heads up on the change would be good (ideally as part of a
release-tagging process ;).

On Thu, Jul 21, 2016 at 12:33:31PM -0700, hmeng-19 wrote:

I use a global var here to avoid multiple func calls to
context.GlobalBool.

What's the drawback to multiple GlobalBool calls? I like that more
than a package global, since it keeps the “single, unambiguous,
authoritative” representation of that setting 1.

Yeah, multiple calls to GlobalBool are fine. Also, can drop per command option in favor of global.

haiyanmeng · 2016-07-21T20:37:48Z

@mrunalp , @wking , PTAL.

wking · 2016-07-21T21:26:37Z

man/ocitools.1.md

+  Log level (panic, fatal, error, warn, info, or debug) (default: "error").
+
+**--host-specific**
+  Generate host-specific configs or do host-specific validations.


This section (including the following paragraph) needs to be genericized now that it's covering both generate and validate.

This comment has been addressed.

haiyanmeng · 2016-07-21T22:03:14Z

@wking , @mrunalp , PTAL.

mrunalp · 2016-07-21T23:01:33Z

cmd/ocitools/generate.go

 	privileged := false
 	if context.IsSet("privileged") {
 		privileged = context.Bool("privileged")
 	}
-	g.SetupPrivileged(privileged)
+	g.SetupPrivileged(privileged, hostSpecific)


Thinking about this more, I think that we shouldn't pass this as part of the API. We should have a call to enable hostSpecific on the generate object and then use that variable in individual APIs

g := generate.New() g.EnableHostSpecific() g.SetupPrivileged() ... g.SaveToFile()

On Thu, Jul 21, 2016 at 04:01:34PM -0700, Mrunal Patel wrote:

privileged := false if context.IsSet("privileged") { privileged = context.Bool("privileged") }

g.SetupPrivileged(privileged)

g.SetupPrivileged(privileged, hostSpecific)

Thinking about this more, I think that we shouldn't pass this as
part of the API. We should have a call to enable hostSpecific on the
generate object and then use that variable in individual APIs

g := generate.New() g.EnableHostSpecific() g.SetupPrivileged()

This works for me. I think it depends on the setting though; I prefer
seccomp-only to be a Save(ToFile) parameter and not a Generator-wide
setting 1. The threshold is “can we think of a reason why a single
Generator would need different values in different contexts?”, and
that makes me a bit jumpy (my crystal ball isn't always that clear ;).
But in this case a Generator setting seems safe enough.

haiyanmeng · 2016-07-22T16:39:58Z

@mrunalp , PTAL.

haiyanmeng · 2016-07-22T16:45:36Z

@mrunalp , EnableHostSpecific has a pointer receiver. Do you think I should change all the other receivers of other generate methods to be pointers?
I can also change hostSpecific to be a package-level var.

wking · 2016-07-22T17:09:42Z

generate/generate.go

@@ -21,7 +21,8 @@ var (

 // Generator represents a generator for a container spec.
 type Generator struct {
-	spec *rspec.Spec
+	spec         *rspec.Spec
+	hostSpecific bool


Why make this private with a setter (and no getter)? Is there a reason to not make it public?

I currently did not think of any cases where Generator.hostSpsecific is queried outside the package.

On Fri, Jul 22, 2016 at 10:16:03AM -0700, hmeng-19 wrote:

@@ -21,7 +21,8 @@ var (

// Generator represents a generator for a container spec.
type Generator struct {

spec *rspec.Spec

spec *rspec.Spec

hostSpecific bool

I currently did not think of any cases where
Generator.hostSpsecific is queried outside the package.

“I can't think of any reason you'd care” is different from “we don't
want people touching this directly”. Since a public field doesn't
require getters/setters, I'd rather keep things public unless we have
reasons to make them private.

wking · 2016-07-22T17:10:10Z

On Fri, Jul 22, 2016 at 09:45:37AM -0700, hmeng-19 wrote:

Do you think I should change all the other receivers of other
generate methods to be pointers?

I think so, but this should probably be a separate commit, since it's
going to be noisy. Copy-by-value Generators relying on the underlying
Generator.spec being a pointer seems like it could be confusing.

wking · 2016-07-22T17:10:17Z

man/ocitools.1.md

+**--host-specific**
+  Generate host-specific configs or do host-specific validations.
+
+  By default, generator addes process capabilities into the config without


“addes” → “adds”

Accept. Thanks.

haiyanmeng · 2016-07-22T17:19:01Z

I think so, but this should probably be a separate commit, since it's
going to be noisy. Copy-by-value Generators relying on the underlying
Generator.spec being a pointer seems like it could be confusing.

@wking , sounds good. I will work on it after the PR is done.

haiyanmeng · 2016-07-22T17:26:56Z

@wking , @mrunalp , PTAL.

haiyanmeng · 2016-07-25T18:45:13Z

@wking , @mrunalp , I added getHostSpecific method and rebased the PR on the upstream master. PTAL.

wking · 2016-07-25T18:53:36Z

On Mon, Jul 25, 2016 at 11:45:14AM -0700, hmeng-19 wrote:

@wking , @mrunalp , I added getHostSpecific method and rebased the
PR on the upstream master. PTAL.

Go suggests not using a ‘Get’ prefix 1, and I'd still rather just
make the property public and skip the getter/setter 2.

haiyanmeng · 2016-07-25T20:31:51Z

@wking , @mrunalp , I added a new commit which:
converts hostSpecific into a public field HostSpecific;
replace GetSpec with Spec.
PTAL.
I will squash the commits before it is ready to be merged.

mrunalp · 2016-07-26T18:12:32Z

LGTM

haiyanmeng · 2016-07-26T18:31:37Z

@mrunalp , PTAL.
I rebased the PR on top of the upstream master and squashed the commits into a single one.

wking · 2016-07-26T21:07:13Z

cmd/ocitools/generate.go

+		gp.HostSpecific = true
+	}
+
+	g := *gp


I don't think we need this. It seemed to compile and run fine for me with the following change on top of 751e18b:

diff --git a/cmd/ocitools/generate.go b/cmd/ocitools/generate.go index ab1f6cc..a6423a0 100644 --- a/cmd/ocitools/generate.go +++ b/cmd/ocitools/generate.go @@ -96,12 +96,11 @@ var generateCommand = cli.Command{ }, } -func setupSpec(gp *generate.Generator, context *cli.Context) error { +func setupSpec(g *generate.Generator, context *cli.Context) error { if context.GlobalBool("host-specific") { - gp.HostSpecific = true + g.HostSpecific = true } - g := *gp spec := g.Spec() if len(spec.Version) == 0 { @@ -374,7 +373,7 @@ func checkNs(nsMaps map[string]string, nsName string) bool { return true } -func setupLinuxNamespaces(g generate.Generator, needsNewUser bool, nsMaps map[string]string) { +func setupLinuxNamespaces(g *generate.Generator, needsNewUser bool, nsMaps map[string]string) { for _, nsName := range generate.Namespaces { if !checkNs(nsMaps, nsName) && !(needsNewUser && nsName == "user") { continue

wking · 2016-07-26T21:17:28Z

On Tue, Jul 26, 2016 at 11:31:38AM -0700, hmeng-19 wrote:

I rebased the PR on top of the upstream master and squashed the
commits into a single one.

Except for 1 751e18b looks good to me.

I'd rather see this v1.0.0-rc1-compatible change based on the
v1.0.0.rc1 branch so we could have it in both master and that branch
without cherry picking and subsequent post-v1.0.0.rc1 merge
duplication. More on this in #140 and the comments after 2.

Signed-off-by: Haiyan Meng <hmeng@redhat.com>

haiyanmeng · 2016-07-27T13:42:08Z

@wking , thanks for pointing out. Fixed it. PTAL.

wking · 2016-07-27T15:59:26Z

bf3f0d4 looks good to me, although I'd still rather not have to
cherry-pick it back into v1.0.0.rc1 1.

haiyanmeng · 2016-07-27T16:00:44Z

@mrunalp , PTAL.

mrunalp · 2016-07-27T17:19:43Z

LGTM

haiyanmeng force-pushed the fix_privileged_caplist branch from fa672ae to 16626ef Compare July 21, 2016 19:12

wking reviewed Jul 21, 2016
View reviewed changes

haiyanmeng force-pushed the fix_privileged_caplist branch from 16626ef to 06cdef1 Compare July 21, 2016 20:37

wking reviewed Jul 21, 2016
View reviewed changes

haiyanmeng force-pushed the fix_privileged_caplist branch from 06cdef1 to 9125c6d Compare July 21, 2016 22:01

mrunalp reviewed Jul 21, 2016
View reviewed changes

haiyanmeng force-pushed the fix_privileged_caplist branch from 9125c6d to 530f103 Compare July 22, 2016 16:38

wking reviewed Jul 22, 2016
View reviewed changes

haiyanmeng force-pushed the fix_privileged_caplist branch from 530f103 to 6435494 Compare July 22, 2016 17:26

This was referenced Jul 22, 2016

Use oci-seccomp-gen library for adding seccomp configurations #128

Closed

Update runtime-spec package #140

Merged

haiyanmeng force-pushed the fix_privileged_caplist branch from 6435494 to 8b6cb85 Compare July 25, 2016 18:44

haiyanmeng force-pushed the fix_privileged_caplist branch from 08fd3a7 to 751e18b Compare July 26, 2016 18:30

wking reviewed Jul 26, 2016
View reviewed changes

Check CAP_LAST_CAP while setting privileged

bf3f0d4

Signed-off-by: Haiyan Meng <hmeng@redhat.com>

haiyanmeng force-pushed the fix_privileged_caplist branch from 751e18b to bf3f0d4 Compare July 27, 2016 13:41

mrunalp merged commit bc8aadb into opencontainers:master Jul 27, 2016

haiyanmeng deleted the fix_privileged_caplist branch July 27, 2016 17:20

This was referenced Jul 27, 2016

generate: fix capability.List() for cap_last_cap not exist #157

Merged

Use seccomp library for adding seccomp configuration flags #159

Merged

Check CAP_LAST_CAP while setting privileged #164

Merged

wking mentioned this pull request Sep 8, 2016

add validate_test.go to do unit test #188

Merged

Check CAP_LAST_CAP while setting privileged #138

Check CAP_LAST_CAP while setting privileged #138

Conversation

haiyanmeng commented Jul 21, 2016

haiyanmeng commented Jul 21, 2016

wking commented Jul 21, 2016

haiyanmeng commented Jul 21, 2016

wking commented Jul 21, 2016

haiyanmeng commented Jul 21, 2016

wking commented Jul 21, 2016

haiyanmeng commented Jul 21, 2016

mrunalp commented Jul 21, 2016

haiyanmeng commented Jul 21, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

haiyanmeng Jul 21, 2016 • edited Loading

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

haiyanmeng commented Jul 21, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

haiyanmeng commented Jul 21, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

haiyanmeng commented Jul 22, 2016

haiyanmeng commented Jul 22, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wking commented Jul 22, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

haiyanmeng commented Jul 22, 2016

haiyanmeng commented Jul 22, 2016

haiyanmeng commented Jul 25, 2016

wking commented Jul 25, 2016

haiyanmeng commented Jul 25, 2016

mrunalp commented Jul 26, 2016

haiyanmeng commented Jul 26, 2016

Choose a reason for hiding this comment

wking commented Jul 26, 2016

haiyanmeng commented Jul 27, 2016

wking commented Jul 27, 2016

haiyanmeng commented Jul 27, 2016

mrunalp commented Jul 27, 2016

haiyanmeng Jul 21, 2016 •

edited

Loading