Skip to content
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

api: add MaskedPaths and ReadonlyPaths options #36644

Merged
merged 1 commit into from Jun 7, 2018

Conversation

@jessfraz
Copy link
Contributor

jessfraz commented Mar 20, 2018

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

@AkihiroSuda

This comment has been minimized.

Copy link
Member

AkihiroSuda commented Mar 21, 2018

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

This comment has been minimized.

Copy link
Contributor Author

jessfraz commented Mar 21, 2018

Still would have the problem with read-only though

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)

This comment has been minimized.

@boaz0

boaz0 Mar 21, 2018

Member

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


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

This comment has been minimized.

@boaz0

boaz0 Mar 21, 2018

Member

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

)

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

This comment has been minimized.

@boaz0

boaz0 Mar 21, 2018

Member

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

@boaz0

This comment has been minimized.

Copy link
Member

boaz0 commented Mar 21, 2018

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

This comment has been minimized.

Copy link
Member

AkihiroSuda commented Mar 21, 2018

ok, design SGTM

@thaJeztah thaJeztah added this to backlog in maintainers-session Mar 22, 2018

@thaJeztah

This comment has been minimized.

Copy link
Member

thaJeztah commented Mar 22, 2018

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

This comment has been minimized.

Copy link
Contributor Author

jessfraz commented Mar 22, 2018

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

This comment has been minimized.

Copy link
Member

thaJeztah commented Mar 22, 2018

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

@jessfraz

This comment has been minimized.

Copy link
Contributor Author

jessfraz commented Mar 22, 2018

@justincormack

This comment has been minimized.

Copy link
Contributor

justincormack commented Mar 22, 2018

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

This comment has been minimized.

Copy link
Member

thaJeztah commented Apr 19, 2018

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

This comment has been minimized.

Copy link
Contributor Author

jessfraz commented Apr 20, 2018

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

This comment has been minimized.

Copy link
Member

AkihiroSuda commented May 1, 2018

@justincormack SGTY? ^^

@justincormack

This comment has been minimized.

Copy link
Contributor

justincormack commented May 1, 2018

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

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

@jessfraz jessfraz requested a review from vdemeester as a code owner May 11, 2018

@jessfraz

This comment has been minimized.

Copy link
Contributor Author

jessfraz commented May 11, 2018

updated and added integration tests

@jessfraz

This comment has been minimized.

Copy link
Contributor Author

jessfraz commented May 12, 2018

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

@cpuguy83

This comment has been minimized.

Copy link
Contributor

cpuguy83 commented May 12, 2018

Janky, that's your mommy! Be nice!

@@ -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) {

This comment has been minimized.

@cpuguy83

cpuguy83 May 12, 2018

Contributor

Can you move these to integration/container?

This comment has been minimized.

@jessfraz

jessfraz May 24, 2018

Author Contributor

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

This comment has been minimized.

@jessfraz

jessfraz May 31, 2018

Author Contributor

ok updated!

@thaJeztah

This comment has been minimized.

Copy link
Member

thaJeztah commented Jun 4, 2018

This one failed on https://jenkins.dockerproject.org/job/Docker-PRs-experimental/40894/console, https://jenkins.dockerproject.org/job/Docker-PRs-powerpc/10069/console, and https://jenkins.dockerproject.org/job/Docker-PRs-s390x/9970/console (Janky was aborted before that because of a flaky)

15:33:18 FAIL: docker_cli_run_test.go:1132: DockerSuite.TestRunProcWritableInPrivilegedContainers
15:33:18 
15:33:18 assertion failed: 
15:33:18 Command:  /usr/local/cli/docker run --privileged busybox sh -c touch /proc/sysrq-trigger
15:33:18 ExitCode: 1
15:33:18 Error:    exit status 1
15:33:18 Stdout:   
15:33:18 Stderr:   touch: /proc/sysrq-trigger: Read-only file system
15:33:18 
15:33:18 
15:33:18 Failures:
15:33:18 ExitCode was 1 expected 0
15:33:18 Expected no error

Looks like something that could be related

@jessfraz

This comment has been minimized.

Copy link
Contributor Author

jessfraz commented Jun 5, 2018

oh that one is me sorry ducks will fix

@jessfraz jessfraz force-pushed the jessfraz:rawaccess branch from 375f13c to 54abcaa Jun 5, 2018

api: add configurable MaskedPaths and ReadOnlyPaths to the API
This adds MaskedPaths and ReadOnlyPaths options to HostConfig for containers so
that a user can override the default values.

When the value sent through the API is nil the default is used.
Otherwise the default is overridden.

Adds integration tests for MaskedPaths and ReadonlyPaths.

Signed-off-by: Jess Frazelle <acidburn@microsoft.com>

@jessfraz jessfraz force-pushed the jessfraz:rawaccess branch from 54abcaa to 3694c1e Jun 5, 2018

@jessfraz

This comment has been minimized.

Copy link
Contributor Author

jessfraz commented Jun 6, 2018

ok should be good now :)

@vdemeester
Copy link
Member

vdemeester left a comment

LGTM 🔥

@boaz0

This comment has been minimized.

Copy link
Member

boaz0 commented Jun 6, 2018

LGTM 💯

@thaJeztah
Copy link
Member

thaJeztah left a comment

LGTM, thanks!

Tests were moved

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

7 of 8 checks passed

codecov/patch 0% of diff hit (target 50%)
Details
codecov/project No report found to compare against
Details
dco-signed All commits are signed
experimental Jenkins build Docker-PRs-experimental 40938 has succeeded
Details
janky Jenkins build Docker-PRs 49694 has succeeded
Details
powerpc Jenkins build Docker-PRs-powerpc 10106 has succeeded
Details
windowsRS1 Jenkins build Docker-PRs-WoW-RS1 21049 has succeeded
Details
z Jenkins build Docker-PRs-s390x 10010 has succeeded
Details

@jessfraz jessfraz deleted the jessfraz:rawaccess branch Jun 7, 2018

@AkihiroSuda

This comment has been minimized.

Copy link
Member

AkihiroSuda commented Jul 20, 2018

Any update about docker cli? ^^

@jessfraz

This comment has been minimized.

Copy link
Contributor Author

jessfraz commented Jul 29, 2018

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

jessfraz added a commit to jessfraz/kubernetes that referenced this pull request Jul 29, 2018

vendor: update docker cadvisor winterm
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>

jessfraz added a commit to jessfraz/kubernetes that referenced this pull request Aug 20, 2018

vendor: update docker cadvisor winterm
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>

jessfraz added a commit to jessfraz/kubernetes that referenced this pull request Aug 22, 2018

vendor: update docker cadvisor winterm
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>

jessfraz added a commit to jessfraz/kubernetes that referenced this pull request Aug 26, 2018

vendor: update docker cadvisor winterm
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>

jessfraz added a commit to jessfraz/kubernetes that referenced this pull request Aug 27, 2018

vendor: update docker cadvisor winterm
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>

jessfraz added a commit to jessfraz/kubernetes that referenced this pull request Aug 30, 2018

vendor: update docker cadvisor winterm
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>

@AkihiroSuda AkihiroSuda referenced this pull request Dec 27, 2018

Closed

(deleted) #762

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.