[feature]: add daemon flag to set no_new_priv as default for unprivileged containers by jmzwcn · Pull Request #29984 · moby/moby

jmzwcn · 2017-01-09T01:24:21Z

[feature]: add daemon flag to set no_new_priv as default for unprivileged containers #29951

Signed-off-by: Daniel Zhang [email protected]

justincormack · 2017-01-09T09:45:47Z

As commented previously, we need to also support the old syntax without =true if possible, and have tests for both. A test for disabling on the daemon (and another for re-enabling) is also needed.

jmzwcn · 2017-01-09T09:57:51Z

Actually old usage is still supported. I don't remove here.

justincormack · 2017-01-09T11:00:35Z

Ok, if you could add the tests then with the old and new forms, and a test of the daemon flag that would be great

jmzwcn · 2017-01-09T11:46:51Z

Sure, have done, please let me know if there is any problem.

justincormack · 2017-01-09T12:06:03Z

this match is not very precise

opt case: no-new-privileges|no-new-privileges=true|no-new-privileges=false
wrong input? no-new-privileges=abc?

should not match : really-no-new-privileges, no-new-privileges-please=true etc

justincormack · 2017-01-09T12:27:11Z

The test failure is

12:17:31 # github.com/docker/docker/daemon
12:17:31 daemon/daemon_unix_test.go:14: imported and not used: "github.com/docker/docker/volume/drivers" as volumedrivers
12:17:31 daemon/daemon_unix_test.go:283: undefined: drivers in drivers.volumedrivers

which needs fixing (maybe changed at the time you rebased?)

jmzwcn · 2017-01-09T12:38:29Z

Have updated, please review again.

clnperez · 2017-01-09T21:18:49Z

The way I read the linked issue this would be true by default. Is that not the case?

I think it is false by default. @justincormack , what do you think?

No, false by default, to be compatible with the current state, otherwise things will break.

Thanks for the clarification @justincormack.

Maybe default-no-new-privileges otherwise it looks like it's doing something to do the daemon.

clnperez · 2017-01-10T15:55:38Z

This one: #29951 Cleared up now, thanks.

…

On 01/09/2017 06:29 PM, Daniel Zhang wrote: jmzwcn commented on this pull request. > @@ -89,6 +90,7 @@ func (config *Config) InstallFlags(flags *pflag.FlagSet) { flags.Int64Var(&config.CPURealtimePeriod, "cpu-rt-period", 0, "Limit the CPU real-time period in microseconds") flags.Int64Var(&config.CPURealtimeRuntime, "cpu-rt-runtime", 0, "Limit the CPU real-time runtime in microseconds") flags.StringVar(&config.SeccompProfile, "seccomp-profile", "", "Path to seccomp profile") + flags.BoolVar(&config.NoNewPrivileges, "no-new-privileges", false, "Disable container processes from gaining new privileges") which link? I think It is false by default.

justincormack · 2017-01-12T15:12:29Z

Sorry, its a bit hectic with getting 1.13 released, I will take a look again soon...

…

On 12 Jan 2017 2:50 p.m., "Daniel Zhang" ***@***.***> wrote: Any review? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#29984 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAdcPH-3l4KTVb44FFr-nykK6mTRRxaLks5rRj2xgaJpZM4Ld3rr> .

justincormack · 2017-01-20T21:15:19Z

Don't make this unrelated change (MTU is normal anyway)

justincormack · 2017-01-20T21:20:17Z

lower in this file there is also a section "This is a full example of the allowed configuration options on Linux:" which should also have this added as "no-new-privileges":false

Seems not get what you said:
--no-new-privileges Disable container processes from gaining new privileges
how to modify?

sorry I just mean in this section https://github.com/docker/docker/blob/master/docs/reference/commandline/dockerd.md#linux-configuration-file it needs to be added in the example config file.

justincormack · 2017-01-20T21:22:31Z

Two minor comments but otherwise this looks fine, tested it thoroughly and it is working fine. Thanks very much for doing this.

cpuguy83 · 2017-02-06T18:11:42Z

@jmzwcn It can go in a platform file, just that it will need to be defined on each platform.

jmzwcn · 2017-02-07T10:02:09Z

@jessfraz , seems TestParseNNPSecurityOptions has covered that scenario.

@cpuguy83, already update, please let me know if any problem.

cpuguy83

Nice, looks much cleaner. Thanks!

LGTM

jmzwcn · 2017-02-16T08:55:47Z

The conflict happen again. If no problem, please merge ASAP. Thanks!

…ners. Signed-off-by: Daniel Zhang <[email protected]>

cpuguy83 · 2017-02-17T03:26:06Z

ping @justincormack

php-coder · 2017-03-23T11:14:30Z

+}
+
+// TestLegacyRunNoNewPrivSetuid checks that --security-opt=no-new-privileges prevents
+// effective uid transtions on executing setuid binaries.


s/transtions/transitions/

thaJeztah · 2017-05-12T10:08:25Z

The actual flag to set this got lost in a rebase, and was added again in #32944

GordonTheTurtle added the status/0-triage label Jan 9, 2017

jmzwcn mentioned this pull request Jan 9, 2017

[feature]: add daemon flag to set no_new_priv as default for unprivileged containers #29951

Closed

vdemeester added status/1-design-review and removed status/0-triage labels Jan 9, 2017

jmzwcn force-pushed the issueNNP branch from 2cdffaa to 6ed9d89 Compare January 9, 2017 11:45

jmzwcn force-pushed the issueNNP branch from 6ed9d89 to a457d4a Compare January 9, 2017 12:02

justincormack reviewed Jan 9, 2017

View reviewed changes

jmzwcn force-pushed the issueNNP branch from a457d4a to e6a69e0 Compare January 9, 2017 12:36

clnperez reviewed Jan 9, 2017

View reviewed changes

jmzwcn force-pushed the issueNNP branch 6 times, most recently from 13d259f to 22faa88 Compare January 12, 2017 13:00

justincormack reviewed Jan 20, 2017

View reviewed changes

justincormack added status/2-code-review impact/changelog and removed status/1-design-review labels Jan 20, 2017

justincormack added this to the 1.14.0 milestone Jan 20, 2017

jmzwcn force-pushed the issueNNP branch 12 times, most recently from ec2aa4b to dc6832a Compare February 7, 2017 07:55

cpuguy83 approved these changes Feb 7, 2017

View reviewed changes

GordonTheTurtle added the dco/no Automatically set by a bot when one of the commits lacks proper signature label Feb 8, 2017

jmzwcn force-pushed the issueNNP branch from 29e4d84 to ff85657 Compare February 8, 2017 04:17

GordonTheTurtle removed the dco/no Automatically set by a bot when one of the commits lacks proper signature label Feb 8, 2017

jmzwcn force-pushed the issueNNP branch from ff85657 to 4f56415 Compare February 8, 2017 07:04

GordonTheTurtle assigned anusha-ragunathan Feb 10, 2017

jmzwcn force-pushed the issueNNP branch from 4f56415 to b584026 Compare February 16, 2017 09:55

Add daemon flag to set no_new_priv as default for unprivileged contai…

d7fda01

…ners. Signed-off-by: Daniel Zhang <[email protected]>

jmzwcn force-pushed the issueNNP branch from b584026 to d7fda01 Compare February 16, 2017 11:06

anusha-ragunathan merged commit 6dd2a82 into moby:master Feb 17, 2017

albers mentioned this pull request Mar 14, 2017

Remove docs for dockerd --no-new-privileges #31813

Merged

php-coder reviewed Mar 23, 2017

View reviewed changes

cpuguy83 mentioned this pull request May 12, 2017

Add no-new-privileg flag #32944

Merged

Conversation

jmzwcn commented Jan 9, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

justincormack commented Jan 9, 2017

Uh oh!

jmzwcn commented Jan 9, 2017

Uh oh!

justincormack commented Jan 9, 2017

Uh oh!

jmzwcn commented Jan 9, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

justincormack commented Jan 9, 2017

Uh oh!

jmzwcn commented Jan 9, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jmzwcn Jan 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

clnperez commented Jan 10, 2017 via email

Uh oh!

justincormack commented Jan 12, 2017 via email

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

justincormack commented Jan 20, 2017

Uh oh!

cpuguy83 commented Feb 6, 2017

Uh oh!

jmzwcn commented Feb 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cpuguy83 left a comment

Choose a reason for hiding this comment

Uh oh!

jmzwcn commented Feb 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cpuguy83 commented Feb 17, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thaJeztah commented May 12, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

11 participants

jmzwcn commented Jan 9, 2017 •

edited

Loading

jmzwcn Jan 10, 2017 •

edited

Loading

jmzwcn commented Feb 7, 2017 •

edited

Loading

jmzwcn commented Feb 16, 2017 •

edited

Loading