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

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

Merged
merged 1 commit into from Feb 17, 2017

Conversation

@jmzwcn
Contributor

jmzwcn commented Jan 9, 2017

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

Signed-off-by: Daniel Zhang jmzwcn@gmail.com

@justincormack

This comment has been minimized.

Show comment
Hide comment
@justincormack

justincormack Jan 9, 2017

Contributor

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.

Contributor

justincormack commented Jan 9, 2017

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

This comment has been minimized.

Show comment
Hide comment
@jmzwcn

jmzwcn Jan 9, 2017

Contributor

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

Contributor

jmzwcn commented Jan 9, 2017

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

@justincormack

This comment has been minimized.

Show comment
Hide comment
@justincormack

justincormack Jan 9, 2017

Contributor

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

Contributor

justincormack commented Jan 9, 2017

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

This comment has been minimized.

Show comment
Hide comment
@jmzwcn

jmzwcn Jan 9, 2017

Contributor

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

Contributor

jmzwcn commented Jan 9, 2017

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

Show outdated Hide outdated daemon/daemon_unix.go Outdated
@justincormack

This comment has been minimized.

Show comment
Hide comment
@justincormack

justincormack Jan 9, 2017

Contributor

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?)

Contributor

justincormack commented Jan 9, 2017

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

This comment has been minimized.

Show comment
Hide comment
@jmzwcn

jmzwcn Jan 9, 2017

Contributor

Have updated, please review again.

Contributor

jmzwcn commented Jan 9, 2017

Have updated, please review again.

Show outdated Hide outdated daemon/config_unix.go Outdated
@clnperez

This comment has been minimized.

Show comment
Hide comment
@clnperez

clnperez Jan 10, 2017

Contributor
Contributor

clnperez commented Jan 10, 2017

@justincormack

This comment has been minimized.

Show comment
Hide comment
@justincormack

justincormack Jan 12, 2017

Contributor
Contributor

justincormack commented Jan 12, 2017

@justincormack

This comment has been minimized.

Show comment
Hide comment
@justincormack

justincormack Jan 20, 2017

Contributor

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

Contributor

justincormack commented Jan 20, 2017

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

@jmzwcn

This comment has been minimized.

Show comment
Hide comment
@jmzwcn

jmzwcn Jan 21, 2017

Contributor

Any question?

Contributor

jmzwcn commented Jan 21, 2017

Any question?

@justincormack

This comment has been minimized.

Show comment
Hide comment
@justincormack
Contributor

justincormack commented Jan 21, 2017

Show outdated Hide outdated daemon/daemon_unix.go Outdated
@LK4D4

This comment has been minimized.

Show comment
Hide comment
@LK4D4

LK4D4 Jan 31, 2017

Contributor

ping @cpuguy83 were your comments addressed?

Contributor

LK4D4 commented Jan 31, 2017

ping @cpuguy83 were your comments addressed?

@jessfraz

This comment has been minimized.

Show comment
Hide comment
@jessfraz

jessfraz Jan 31, 2017

Contributor

omg my dreams are coming trueeeeeee

Contributor

jessfraz commented Jan 31, 2017

omg my dreams are coming trueeeeeee

Show outdated Hide outdated daemon/daemon_unix.go Outdated
@jessfraz

This comment has been minimized.

Show comment
Hide comment
@jessfraz

jessfraz Jan 31, 2017

Contributor

might be nice to also add an integration test for the daemon where no_new_privs is true but a container is run with privileged and then one where you override no_new_privs to false on the container. 😇

Contributor

jessfraz commented Jan 31, 2017

might be nice to also add an integration test for the daemon where no_new_privs is true but a container is run with privileged and then one where you override no_new_privs to false on the container. 😇

@cpuguy83

This comment has been minimized.

Show comment
Hide comment
@cpuguy83

cpuguy83 Feb 6, 2017

Contributor

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

Contributor

cpuguy83 commented Feb 6, 2017

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

@jmzwcn

This comment has been minimized.

Show comment
Hide comment
@jmzwcn

jmzwcn Feb 7, 2017

Contributor

@jessfraz , seems TestParseNNPSecurityOptions has covered that scenario.

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

Contributor

jmzwcn commented Feb 7, 2017

@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

This comment has been minimized.

Show comment
Hide comment
@jmzwcn

jmzwcn Feb 16, 2017

Contributor

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

Contributor

jmzwcn commented Feb 16, 2017

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

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

Signed-off-by: Daniel Zhang <jmzwcn@gmail.com>
@cpuguy83

This comment has been minimized.

Show comment
Hide comment
@cpuguy83
Contributor

cpuguy83 commented Feb 17, 2017

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

4 checks passed

dco-signed All commits are signed
experimental Jenkins build Docker-PRs-experimental 30826 has succeeded
Details
janky Jenkins build Docker-PRs 39441 has succeeded
Details
windowsRS1 Jenkins build Docker-PRs-WoW-RS1 10503 has succeeded
Details
}
// TestLegacyRunNoNewPrivSetuid checks that --security-opt=no-new-privileges prevents
// effective uid transtions on executing setuid binaries.

This comment has been minimized.

@php-coder

php-coder Mar 23, 2017

s/transtions/transitions/

@php-coder

php-coder Mar 23, 2017

s/transtions/transitions/

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah May 12, 2017

Member

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

Member

thaJeztah commented May 12, 2017

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment