add cli integration for masked and readonly paths #1347

jessfraz · 2018-09-06T14:47:02Z

I wanted to make sure this design was okay before going any further.

This adds flags --masked-paths and --readonly-paths to the cli as a follow up from moby/moby#36644 cc @AkihiroSuda

Happy to change the design in any way you want then will add docs and tests.

Signed-off-by: Jess Frazelle <[email protected]>

AkihiroSuda · 2018-09-07T03:23:34Z

cli/command/container/opts.go

@@ -183,6 +187,8 @@ func addFlags(flags *pflag.FlagSet) *containerOptions {
 	flags.StringVarP(&copts.user, "user", "u", "", "Username or UID (format: <name|uid>[:<group|gid>])")
 	flags.StringVarP(&copts.workingDir, "workdir", "w", "", "Working directory inside the container")
 	flags.BoolVar(&copts.autoRemove, "rm", false, "Automatically remove the container when it exits")
+	flags.Var(&copts.maskedPaths, "masked-paths", "Denote the paths to be masked for the container, empty implies none")


empty implies default, not "none"?

It implies none, of "paths to be masked for the containers"

AkihiroSuda · 2018-09-07T03:24:46Z

e2e/container/run_test.go

@@ -50,6 +50,19 @@ func TestRunWithContentTrust(t *testing.T) {
 	})
 }

+func TestRunMaskedPaths(t *testing.T) {


Especially we need to make sure nil and []string{} are treated differently

yeah was about to add tests but waited also im confused as to if the tests from docker/docker will be moved here too?

@jessfraz new tests from docker/docker should only test the API — or be unit tests 😝). Any tests using the cli should indeed be there.

AkihiroSuda · 2018-09-07T03:26:17Z

cli/command/container/opts.go

+	for _, p := range paths {
+		if p == "none" {
+			if len(paths) > 1 {
+				return nil, errors.New("Passing 'none' and other paths is not allowed")


nit: error string should not be capitalized

rroller · 2018-10-01T17:21:54Z

Any update on this and if it'll get merged?

AkihiroSuda · 2018-10-09T04:05:33Z

Can we also have --unmask-proc for simplicity?

tiborvass · 2018-10-11T01:41:29Z

@jessfraz @AkihiroSuda @vdemeester So I know this may be controversial but what do you think of putting these behind --security-opt like so --security-opt readonly-paths=... and --security-opt masked-paths=... ?

I know it's ugly because in the API it's not a security opt, but the rationale for this suggestion is that it's not necessarily obvious for users that this may have a security implication as defaults are overridden.

If we agree on this, we could have the API accept either security opts or regular run options.

Let me know what you think!

AkihiroSuda · 2018-10-12T10:38:41Z

FYI: it turned out that unshare -rmn mount -t sysfs none /sys (for /sys/class/net) requires rw/unmasked sysfs

rootless-containers/rootlesskit#23

AkihiroSuda · 2018-10-12T10:58:42Z

Maybe:

--security-opt readonly-paths (low-level, rarely used)
--security-opt masked-paths (ditto)
--security-opt unconfined-paths (frequently used as --security-opt unconfined-paths=/proc,/sys)

thaJeztah · 2018-10-24T20:34:23Z

If we agree on this, we could have the API accept either security opts or regular run options.

Wondering if the API must reflect the UX, I don't think that's strictly required.

--security-opt unconfined-paths (frequently used as --security-opt unconfined-paths=/proc,/sys)

@AkihiroSuda wondering how that option would work in conjunction with the other options; should we error (conflicting options)?

I recall the original discussion on the API side of things that we preferred explicit over implicit, so providing your own set of "masked" and/or "read only" paths. That was of course from the API perspective, so UX can differentiate from that.

If we use unconfined-paths; how would the CLI determine the default set of masked paths? (I don't think there's currently an endpoint that would allow querying the default set from the daemon, other than inspecting an already created container)

jessfraz · 2018-10-25T17:04:25Z

I'm okay with any of the options listed above :)

tonistiigi · 2018-10-25T21:14:27Z

--security-opt unconfined-paths (frequently used as --security-opt unconfined-paths=/proc,/sys)

I don't think you can actually do --security-opt unconfined-paths=/proc,/sys because the API only supports whitelist and modifications to the default set are not allowed. Not sure if there is any usecase for actually specifying a special path from CLI though as it is quite an effort required from the user to remember to whole list of possibly insecure paths. Usually you just want to either allow all or make an excpetion to the default. How about just --security-opt systempaths=unconfined. (or protectedpaths, securitypaths, ...)

tiborvass · 2018-10-25T21:45:32Z

@tonistiigi If it's systempaths=unconfined then what's the other value for systempaths= ? confined ?

tonistiigi · 2018-10-25T21:51:46Z

@tiborvass There is no other value. If we want then confined would mean the daemon default and if there is a good usecase we could add more names. I'm fine with a constant value. I noticed that --security-opt no-new-privileges=false uses a bool so we could use that as well.

scher200 · 2018-12-13T04:00:01Z

@tonistiigi how is this going? I am exited to see this working

tonistiigi · 2018-12-13T10:12:57Z

How about just --security-opt systempaths=unconfined. (or protectedpaths, securitypaths, ...)
or --security-opt unmaskedsystempaths=true --security-opt rawsystempaths=true

Does anyone have objections to this? I'm good with any of them.

@thaJeztah Might be good to add this to maintainer session today.

thaJeztah · 2018-12-14T16:18:40Z

Does anyone have objections to this? I'm good with any of them.

UX-wise, I like that option, so (name to be discussed);

--security-opt systempaths=unconfined

Would send the equivalent of --masked-paths=[]

Concern:

systempaths=unconfined would disable all masked paths
because of the above, doing the wrong thing ("just unmask the whole shebang) is easier than doing the right thing ("specify which paths you want to unmask")

I think it's important when dealing with security configuration to make "doing the right thing" easier than "doing the wrong thing".

So; do we also want to provide a more fine-grained approach on the CLI; an option to manually provide exactly which paths to mask/mark readonly?

If yes: that would raise the question #1347 (comment)

how would the CLI determine the default set of masked paths? (I don't think there's currently an endpoint that would allow querying the default set from the daemon, other than inspecting an already created container)

Approach could then be;

Implement a defaults endpoint for containers (I think that's useful for other purposes as well, such as showing defaults for flags/options; especially if those defaults are configurable on the daemon), for example;

GET /v1.40/containers/-/defaults (- indenting as "no id" ? idk)
GET /v1.40/containers/-/json (ugh .. the trailing /json sucks haha)

When creating a container;

Fetch the defaults
Patch the defaults with changes from the CLI (?) (---security-opt unconfined-paths=/proc,/sys)
Send the resulting masked-paths and readonly-paths over the API

@justincormack @jessfraz I know you're better at security than I am, so wondering what you think, or what suggestions you have 😅

Adding this to next week's maintainers meeting as well (this week didn't happen, due to many people travelling or at KubeCon)

justincormack · 2018-12-27T12:17:34Z

I don't think setting the defaults makes sense, it is too confusing for different people to have different defaults. A security option to remove them all seems ok, and an API option to set them, and maybe a security option eg systempaths=/path1,/path2,... although its going to be hard to drive. Unconfined seems the most likely case and most consistent eg with seccomp.

imbolo · 2018-12-29T04:39:48Z

Any update ?

scher200 · 2019-01-25T05:30:50Z

when in this expected to be released?

tiborvass · 2019-03-21T22:24:43Z

Let's roll with --security-opt systempaths=unconfined and call it a day. We can add more things later. If no one answers in disagreement, I'll consider it an agreement. Ping @thaJeztah @justincormack

biblix · 2019-03-26T23:23:37Z

Hi @jessfraz, https://github.com/genuinetools/img#running-with-docker led me here. While this PR is still pending, is there a way to take your fix here and test img build in a unprivileged container in Kubernetes?

thaJeztah · 2019-03-28T22:59:00Z

Let's roll with --security-opt systempaths=unconfined and call it a day. We can add more things later. If no one answers in disagreement, I'll consider it an agreement.

OK, so;

--security-opt systempaths=unconfined would set masked-paths=[] and readonly-paths=[]
no option yet to set them individually
no option yet to set an individual list of paths to (un)mask, make read-only/writable

Correct?

tiborvass · 2019-03-28T23:45:38Z

@thaJeztah yes. cc @justincormack

vdemeester · 2019-03-29T08:16:24Z

Needs a rebase 😅

thaJeztah · 2019-03-29T10:23:29Z

Needs a rebase 😅

Also needs an actual implementation of the proposed changes #1347 (comment)

@tiborvass were you planning on making those?

thaJeztah

let me "request changes" pending the proposed changes 😅

jessfraz · 2019-04-04T22:39:37Z

ah okay is this on me? I can do that

tiborvass · 2019-04-04T22:44:31Z

@jessfraz I was about to do it but feel free!

thaJeztah · 2019-04-05T11:51:59Z

@tiborvass @jessfraz I see @martencassel started working on the implementation; see #1808

jessfraz · 2019-04-06T14:55:07Z

Thank you!!

…

On Fri, Apr 5, 2019 at 7:52 AM Sebastiaan van Stijn < ***@***.***> wrote: @tiborvass <https://github.com/tiborvass> @jessfraz <https://github.com/jessfraz> I see @martencassel <https://github.com/martencassel> started working on the implementation; see #1808 <#1808> — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1347 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABYNbI0BBWxwEA_cZ68cw-XZNcav0tEKks5vdzjkgaJpZM4WdGEv> .

-- Jessie Frazelle 4096R / D4C4 DD60 0D66 F65A 8EFC 511E 18F3 685C 0022 BFF3 pgp.mit.edu <http://pgp.mit.edu/pks/lookup?op=get&search=0x18F3685C0022BFF3>

elgalu · 2019-05-30T20:41:13Z

#1808 is merged

tiborvass · 2019-06-04T05:47:48Z

Thank you @jessfraz and @martencassel !

add cli integration for masked and readonly paths

94d2334

Signed-off-by: Jess Frazelle <[email protected]>

GordonTheTurtle added the status/0-triage label Sep 6, 2018

vdemeester requested review from thaJeztah and vdemeester September 6, 2018 14:55

vdemeester added status/1-design-review and removed status/0-triage labels Sep 6, 2018

AkihiroSuda reviewed Sep 7, 2018

View reviewed changes

rroller mentioned this pull request Sep 19, 2018

img command in docker containers, is it supported? genuinetools/img#159

Closed

This was referenced Oct 12, 2018

Propose securityContext.sysMount: Unmasked to Kubernetes rootless-containers/rootlesskit#24

Open

update Dockerfile, with Giuseppe's newuidmap rootless-containers/rootlesskit#23

Merged

thaJeztah mentioned this pull request Dec 15, 2018

Proposal: Allow user to set own service defaults moby/swarmkit#2794

Open

AkihiroSuda mentioned this pull request Jan 15, 2019

Masked /proc/asound moby/moby#38299

Merged

scher200 mentioned this pull request Jan 21, 2019

does the readme need an update genuinetools/img#211

Open

cstrahan mentioned this pull request Mar 21, 2019

Option to make sandbox work in Docker with its default seccomp filter NixOS/nix#2719

Closed

justincormack approved these changes Mar 29, 2019

View reviewed changes

thaJeztah requested changes Mar 29, 2019

View reviewed changes

martencassel mentioned this pull request Apr 4, 2019

add cli integration for unconfined systempaths #1808

Merged

tiborvass closed this Jun 4, 2019

zhangyoufu mentioned this pull request Jun 4, 2019

upstream patched, README.md needs update genuinetools/img#246

Closed

thaJeztah mentioned this pull request Apr 3, 2024

Proposal: decompose --privileged into individual --security-opt options moby/moby#47663

Open

add cli integration for masked and readonly paths #1347

add cli integration for masked and readonly paths #1347

Conversation

jessfraz commented Sep 6, 2018

AkihiroSuda Sep 7, 2018

Choose a reason for hiding this comment

jpic Nov 10, 2018

Choose a reason for hiding this comment

AkihiroSuda Sep 7, 2018

Choose a reason for hiding this comment

AkihiroSuda Sep 7, 2018

Choose a reason for hiding this comment

jessfraz Sep 10, 2018

Choose a reason for hiding this comment

vdemeester Sep 10, 2018

Choose a reason for hiding this comment

AkihiroSuda Sep 7, 2018

Choose a reason for hiding this comment

rroller commented Oct 1, 2018

AkihiroSuda commented Oct 9, 2018

tiborvass commented Oct 11, 2018

AkihiroSuda commented Oct 12, 2018 • edited Loading

AkihiroSuda commented Oct 12, 2018

thaJeztah commented Oct 24, 2018

jessfraz commented Oct 25, 2018

tonistiigi commented Oct 25, 2018

tiborvass commented Oct 25, 2018

tonistiigi commented Oct 25, 2018

scher200 commented Dec 13, 2018

tonistiigi commented Dec 13, 2018

thaJeztah commented Dec 14, 2018

justincormack commented Dec 27, 2018

imbolo commented Dec 29, 2018

scher200 commented Jan 25, 2019

tiborvass commented Mar 21, 2019

biblix commented Mar 26, 2019

thaJeztah commented Mar 28, 2019

tiborvass commented Mar 28, 2019

vdemeester commented Mar 29, 2019

thaJeztah commented Mar 29, 2019

thaJeztah left a comment

Choose a reason for hiding this comment

jessfraz commented Apr 4, 2019

tiborvass commented Apr 4, 2019

thaJeztah commented Apr 5, 2019

jessfraz commented Apr 6, 2019 via email

elgalu commented May 30, 2019

tiborvass commented Jun 4, 2019

AkihiroSuda commented Oct 12, 2018 •

edited

Loading