api: add MaskedPaths and ReadonlyPaths options #36644

jessfraz · 2018-03-20T17:53:13Z

This adds a RawAccess option to HostConfig for containers so that a user
can pass in an array of paths to not be set as masked or readonly.

closes #36597

- What I did

Added new API options for Masked Paths and ReadonlyPaths to the container HostConfig

- How I did it

Added it to the API and oci package.

- How to verify it

I wrote a test.

- Description for the changelog
RawAccess allows a set of paths to be not set as masked or readonly

- A picture of a cute animal (not mandatory but encouraged)

AkihiroSuda · 2018-03-21T03:39:11Z

As discussed in opencontainers/runc#1658 (comment) ,
I think we (and runc/runtime-spec maintainers) should consider allowing mount /path even when /path/a is masked, as (IIUC) an alternative to this PR.

jessfraz · 2018-03-21T03:43:01Z

Still would have the problem with read-only though

boaz0 · 2018-03-21T07:36:42Z

oci/raw_access.go

+		for i, p := range s.Linux.MaskedPaths {
+			matched, err := filepath.Match(rawpath, p)
+			if err != nil {
+				return fmt.Errorf("masked paths: checking if %s matches %s failed: %v", rawpath, p, err)


Small nitpick/suggestion: maybe errors.Wrapf instead of fmt.Errorf?

boaz0 · 2018-03-21T07:36:46Z

oci/raw_access.go

+
+// SetRawAccess removes the given raw access paths from the specs MaskedPaths and ReadonlyPaths.
+func SetRawAccess(s *specs.Spec, rawaccess []string) error {
+	// Iterate over the rawaccess paths.


Small nitpick/suggestion: IMHO, this comment is a redundancy.

boaz0 · 2018-03-21T07:36:48Z

oci/raw_access.go

+)
+
+// SetRawAccess removes the given raw access paths from the specs MaskedPaths and ReadonlyPaths.
+func SetRawAccess(s *specs.Spec, rawaccess []string) error {


Small nitpick/suggestion: can you change from rawaccess to rawAccessPaths?

boaz0 · 2018-03-21T07:41:08Z

It looks like the CI failures aren't related to the change:

powerpc failed due to timeout
janky failed due to hack/generate-swagger-api.sh. Although, no changes to the API were made:

18:04:17 The result of hack/generate-swagger-api.sh differs
18:04:17 
18:04:17 diff --git a/api/types/container/container_wait.go b/api/types/container/container_wait.go
18:04:17 index 9e3910a6b..06b0f0207 100644
18:04:17 --- a/api/types/container/container_wait.go
18:04:17 +++ b/api/types/container/container_wait.go
18:04:17 @@ -7,14 +7,6 @@ package container
18:04:17  // See hack/generate-swagger-api.sh
18:04:17  // ----------------------------------------------------------------------------
18:04:17  
18:04:17 -// ContainerWaitOKBodyError container waiting error, if any
18:04:17 -// swagger:model ContainerWaitOKBodyError
18:04:17 -type ContainerWaitOKBodyError struct {
18:04:17 -
18:04:17 -	// Details of an error
18:04:17 -	Message string `json:"Message,omitempty"`
18:04:17 -}
18:04:17 -
18:04:17  // ContainerWaitOKBody OK response to ContainerWait operation
18:04:17  // swagger:model ContainerWaitOKBody
18:04:17  type ContainerWaitOKBody struct {
18:04:17 @@ -27,3 +19,11 @@ type ContainerWaitOKBody struct {
18:04:17  	// Required: true
18:04:17  	StatusCode int64 `json:"StatusCode"`
18:04:17  }
18:04:17 +
18:04:17 +// ContainerWaitOKBodyError container waiting error, if any
18:04:17 +// swagger:model ContainerWaitOKBodyError
18:04:17 +type ContainerWaitOKBodyError struct {
18:04:17 +
18:04:17 +	// Details of an error
18:04:17 +	Message string `json:"Message,omitempty"`
18:04:17 +}
18:04:17 
18:04:17 Please update api/swagger.yaml with any api changes, then 
18:04:17 run `hack/generate-swagger-api.sh`.
``

AkihiroSuda · 2018-03-21T15:04:10Z

ok, design SGTM

thaJeztah · 2018-03-22T18:37:09Z

Discussing in the maintainers meeting, and generally looks like we're okay with the functionality. There's still some discussion on design, and possible alternatives

Some concerns that were brought up:

If this will be exposed through the CLI, we need to look closely at the UX, as it may not be as clear as (say) --cap-add SYS_ADMIN that you're giving more privileged to the container, so we should print a warning there (assuming this is gonna be added to the CLI)
Another thing brought up is that we should error if someone tries to "unmask" a path that wasn't masked ?

Alternative approaches being discussed:

instead of providing what paths to exclude from the defaults, should we have an option to set which paths to mask (i.e., fully override the defaults)?
@AkihiroSuda's suggestion to use the mounts API was also discussed; wondering if this would work (inconclusive 😅)

jessfraz · 2018-03-22T18:43:06Z

agree with documenting it to make sure its clear.

I am okay with any design you all want so I'll update accordingly when it's decided :)

thaJeztah · 2018-03-22T19:13:09Z

Thanks! Yes, I'll pester some people to see if we can come to a decision asap 😂

jessfraz · 2018-03-22T19:18:14Z

no worries I know you all are busy

…

On Thu, Mar 22, 2018 at 3:14 PM, Sebastiaan van Stijn < ***@***.***> wrote: Thanks! Yes, I'll pester some people to see if we can come to a decision asap 😂 — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#36644 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABYNbGpLYp4Uq6FzOlmVqbjDteCMBillks5tg_gIgaJpZM4SyXtM> .

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

justincormack · 2018-03-22T22:49:36Z

I dont think it makes sense to error if unmasking a path thats not masked, they vary by docker version quite a it so its better to be able to over specify.

thaJeztah · 2018-04-19T20:28:53Z

Brought this up in a maintainers meeting again to see if I could get things moving. Looks like generally people were in favor of;

Instead of providing what paths to exclude from the defaults, provide al list of paths to mask (i.e., fully override the defaults)

Also; for the time being; make this an API-only change (no option on the CLI yet to configure this)

@justincormack let me know if you agree with the above ^^

jessfraz · 2018-04-20T21:34:05Z

Also; for the time being; make this an API-only change (no option on the CLI yet to configure this)

oh cool, i will just wait on confirmation, thanks so much for moving this along :) you all are the dopest

AkihiroSuda · 2018-05-01T07:04:01Z

@justincormack SGTY? ^^

justincormack · 2018-05-01T14:10:46Z

Yes an override list (possibly empty) seems cleanest. That way you dont need to know which ones to remove. SGTM.

jessfraz · 2018-05-11T22:10:47Z

updated and added integration tests

jessfraz · 2018-05-12T00:09:17Z

janky is mad at me, its like he doesn't remember who his mom is

cpuguy83 · 2018-05-12T02:10:35Z

Janky, that's your mommy! Be nice!

cpuguy83 · 2018-05-12T02:12:36Z

integration-cli/docker_api_create_test.go

@@ -113,3 +115,134 @@ func (s *DockerSuite) TestAPICreateWithInvalidHealthcheckParams(c *check.C) {
 	expected = fmt.Sprintf("StartPeriod in Healthcheck cannot be less than %s", container.MinimumDuration)
 	c.Assert(getErrorMessage(c, buf), checker.Contains, expected)
 }
+
+func (s *DockerSuite) TestAPICreateWithCustomMaskedPaths(c *check.C) {


Can you move these to integration/container?

ah so I had them there and they weren't working but will try again

ok updated!

jessfraz · 2018-06-06T03:39:30Z

ok should be good now :)

vdemeester

LGTM 🔥

boaz0 · 2018-06-06T10:55:37Z

LGTM 💯

thaJeztah

LGTM, thanks!

Tests were moved

AkihiroSuda · 2018-07-20T01:31:27Z

Any update about docker cli? ^^

jessfraz · 2018-07-29T14:45:06Z

oh will do, sorry been busy with other work things, i promise it is on the list :) probably will open a proposal next week

This vendor change was purely for the changes in docker to allow for setting the Masked and Read-only paths. See: moby/moby#36644 But because of the docker dep update it also needed cadvisor to be updated and winterm due to changes in pkg/tlsconfig in docker See: google/cadvisor#1967 Signed-off-by: Jess Frazelle <acidburn@microsoft.com>

GordonTheTurtle added the status/0-triage label Mar 20, 2018

jessfraz mentioned this pull request Mar 20, 2018

add ProcMount option kubernetes/community#1934

Merged

boaz0 added status/1-design-review and removed status/0-triage labels Mar 21, 2018

boaz0 reviewed Mar 21, 2018

View reviewed changes

GordonTheTurtle assigned crosbymichael Apr 4, 2018

AkihiroSuda mentioned this pull request Apr 18, 2018

Run kaniko as non-root user within the container GoogleContainerTools/kaniko#105

Open

jessfraz force-pushed the rawaccess branch from 0164eb8 to e9a272c Compare May 11, 2018 22:09

jessfraz requested a review from vdemeester as a code owner May 11, 2018 22:09

cpuguy83 previously requested changes May 12, 2018

View reviewed changes

jessfraz mentioned this pull request May 24, 2018

Add a ProcMount option to the SecurityContext & AllowedProcMountTypes to PodSecurityPolicy kubernetes/kubernetes#64283

Merged

jessfraz force-pushed the rawaccess branch from e9a272c to 8e743c3 Compare May 31, 2018 19:45

jessfraz force-pushed the rawaccess branch from 54abcaa to 3694c1e Compare June 5, 2018 16:34

AkihiroSuda added the rebuild/janky label Jun 6, 2018

GordonTheTurtle removed the rebuild/janky label Jun 6, 2018

vdemeester approved these changes Jun 6, 2018

View reviewed changes

thaJeztah approved these changes Jun 7, 2018

View reviewed changes

thaJeztah added status/2-code-review impact/api impact/changelog and removed status/1-design-review labels Jun 7, 2018

thaJeztah merged commit 1fe0e49 into moby:master Jun 7, 2018

jessfraz deleted the rawaccess branch June 7, 2018 01:18

jessfraz mentioned this pull request Sep 6, 2018

add cli integration for masked and readonly paths docker/cli#1347

Closed

thaJeztah mentioned this pull request Oct 31, 2018

Add --cap-add as and --cap-drop to swarm services #26849

Closed

AkihiroSuda mentioned this pull request Dec 27, 2018

(deleted) moby/buildkit#762

Closed

cstrahan mentioned this pull request Mar 21, 2019

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

Closed

thaJeztah mentioned this pull request Sep 11, 2019

new security-opt: privileged-without-host-devices (for safe DinD with Kata) #39702

Closed

jnewland mentioned this pull request Dec 28, 2019

Reduce privileges granted to docker container urcomputeringpal/actions-runner-kubernetes#1

Open

geofft mentioned this pull request Apr 14, 2020

Rootless containers don't work from unprivileged non-root Docker container (operation not permitted for mounting procfs) opencontainers/runc#1658

Closed

thaJeztah mentioned this pull request Sep 17, 2020

Set masked and readonly paths based on default Unix spec containerd/cri#1570

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

api: add MaskedPaths and ReadonlyPaths options #36644

api: add MaskedPaths and ReadonlyPaths options #36644

jessfraz commented Mar 20, 2018 •

edited

Loading

AkihiroSuda commented Mar 21, 2018

jessfraz commented Mar 21, 2018

boaz0 Mar 21, 2018

boaz0 Mar 21, 2018

boaz0 Mar 21, 2018

boaz0 commented Mar 21, 2018

AkihiroSuda commented Mar 21, 2018

thaJeztah commented Mar 22, 2018

jessfraz commented Mar 22, 2018

thaJeztah commented Mar 22, 2018

jessfraz commented Mar 22, 2018 via email

justincormack commented Mar 22, 2018

thaJeztah commented Apr 19, 2018

jessfraz commented Apr 20, 2018

AkihiroSuda commented May 1, 2018

justincormack commented May 1, 2018

jessfraz commented May 11, 2018

jessfraz commented May 12, 2018

cpuguy83 commented May 12, 2018

cpuguy83 May 12, 2018

jessfraz May 24, 2018

jessfraz May 31, 2018

jessfraz commented Jun 6, 2018

vdemeester left a comment

boaz0 commented Jun 6, 2018

thaJeztah left a comment

AkihiroSuda commented Jul 20, 2018

jessfraz commented Jul 29, 2018

api: add MaskedPaths and ReadonlyPaths options #36644

api: add MaskedPaths and ReadonlyPaths options #36644

Conversation

jessfraz commented Mar 20, 2018 • edited Loading

AkihiroSuda commented Mar 21, 2018

jessfraz commented Mar 21, 2018

boaz0 Mar 21, 2018

Choose a reason for hiding this comment

boaz0 Mar 21, 2018

Choose a reason for hiding this comment

boaz0 Mar 21, 2018

Choose a reason for hiding this comment

boaz0 commented Mar 21, 2018

AkihiroSuda commented Mar 21, 2018

thaJeztah commented Mar 22, 2018

jessfraz commented Mar 22, 2018

thaJeztah commented Mar 22, 2018

jessfraz commented Mar 22, 2018 via email

justincormack commented Mar 22, 2018

thaJeztah commented Apr 19, 2018

jessfraz commented Apr 20, 2018

AkihiroSuda commented May 1, 2018

justincormack commented May 1, 2018

jessfraz commented May 11, 2018

jessfraz commented May 12, 2018

cpuguy83 commented May 12, 2018

cpuguy83 May 12, 2018

Choose a reason for hiding this comment

jessfraz May 24, 2018

Choose a reason for hiding this comment

jessfraz May 31, 2018

Choose a reason for hiding this comment

jessfraz commented Jun 6, 2018

vdemeester left a comment

Choose a reason for hiding this comment

boaz0 commented Jun 6, 2018

thaJeztah left a comment

Choose a reason for hiding this comment

AkihiroSuda commented Jul 20, 2018

jessfraz commented Jul 29, 2018

jessfraz commented Mar 20, 2018 •

edited

Loading