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

Fix updating /sys/fs/cgroup mount to 'rw' #1085

Merged
merged 4 commits into from
Jul 18, 2019

Conversation

smira
Copy link
Collaborator

@smira smira commented Jul 17, 2019

There were two bugs: Mount was matched by Type which is actually
cgroup, not sysfs. And the second problem was that copy of the value
was modified, not value in the slice.

Signed-off-by: Andrey Smirnov smirnov.andrey@gmail.com

There were two bugs: Mount was matched by Type which is actually
`cgroup`, not `sysfs`. And the second problem was that copy of the value
was modified, not value in the slice.

Signed-off-by: Andrey Smirnov <smirnov.andrey@gmail.com>
@tonistiigi
Copy link
Member

This should be for both sysfs and cgroup. There are actaally functions already WithWriteableSysfs, WithWriteableCgroupfs that you can just call.

@tonistiigi
Copy link
Member

Or actually, there is WithPrivileged that should do all of this. Can clean up some handling in here.

@tonistiigi
Copy link
Member

Also, add checks for these in the existing security tests in client_test while you're at it.

@smira
Copy link
Collaborator Author

smira commented Jul 18, 2019

@tonistiigi I'm not really an expert in all of these low-level mounts, but looks like oci.With... options should be applied before we do oci.GenerateSpec, and the code here is doing more of changes after the fact. Was your point that I should move all these more into oci.With... options instead of hand-rolled changes to oci.Spec after oci.GenerateSpec is called?

@smira
Copy link
Collaborator Author

smira commented Jul 18, 2019

As it seems it's not 100% the same what can be done with oci.With... vs. what is there in executor/oci/spec_unix.go.

@GordonTheTurtle
Copy link
Collaborator

Please sign your commits following these rules:
https://github.com/moby/moby/blob/master/CONTRIBUTING.md#sign-your-work
The easiest way to do this is to amend the last commit:

$ git clone -b "sysfs-cgroup-rw-fix" git@github.com:smira/buildkit.git somewhere
$ cd somewhere
$ git rebase -i HEAD~842361581128
editor opens
change each 'pick' to 'edit'
save the file and quit
$ git commit --amend -s --no-edit
$ git rebase --continue # and repeat the amend for each commit
$ git push -f

Amending updates the existing PR. You DO NOT need to open a new one.

Signed-off-by: Andrey Smirnov <smirnov.andrey@gmail.com>
@smira
Copy link
Collaborator Author

smira commented Jul 18, 2019

Updated with helpers, but only for rw re-mounts.

Signed-off-by: Andrey Smirnov <smirnov.andrey@gmail.com>
AllowedEntitlements: allowedEntitlements,
}, nil)

if secMode == securitySandbox || sb.Rootless() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: maybe just skip the test in rootless as this isn't expected behavior but current limitations

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks, updated

Signed-off-by: Andrey Smirnov <smirnov.andrey@gmail.com>
smira added a commit to smira/talos that referenced this pull request Jul 18, 2019
This relies on two PRs to the buildkit, which aren't merged yet, so I
had to do some overrides to apply them:

* moby/buildkit#1081
* moby/buildkit#1085

Signed-off-by: Andrey Smirnov <smirnov.andrey@gmail.com>
smira added a commit to smira/talos that referenced this pull request Jul 18, 2019
This relies on two PRs to the buildkit, which aren't merged yet, so I
had to do some overrides to apply them:

* moby/buildkit#1081
* moby/buildkit#1085

Signed-off-by: Andrey Smirnov <smirnov.andrey@gmail.com>
@tiborvass
Copy link
Collaborator

I tried to get the test to fail without the fix, but did not succeed.

@smira
Copy link
Collaborator Author

smira commented Jul 18, 2019

@tiborvass fails for me:

        require.go:794: 
            	Error Trace:	client_test.go:520
            	            				run.go:172
            	Error:      	Received unexpected error:
            	            	rpc error: code = Unknown desc = failed to build LLB: executor failed running [mkdir /sys/fs/cgroup/cpuset/securitytest]: process returned non-zero exit code: 1
            	            	failed to solve
            	            	github.com/moby/buildkit/client.(*Client).solve.func2
            	            		/src/client/solve.go:203
            	            	golang.org/x/sync/errgroup.(*Group).Go.func1
            	            		/src/vendor/golang.org/x/sync/errgroup/errgroup.go:58
            	            	runtime.goexit
            	            		/usr/local/go/src/runtime/asm_amd64.s:1337
            	Test:       	TestClientIntegration/TestSecurityModeSysfs/worker=containerd-1.0/secmode=insecure

@tiborvass
Copy link
Collaborator

Yes, user error, my bad.

@tiborvass tiborvass merged commit fd2d8e6 into moby:master Jul 18, 2019
smira added a commit to smira/talos that referenced this pull request Jul 19, 2019
This relies on two PRs to the buildkit:

* moby/buildkit#1081
* moby/buildkit#1085

Sysfs fix was merged to upstream, so updated tag, while using
`Dockerfile` slug I can switch to dockerfile2llb with support for
`--security=insecure`.

Signed-off-by: Andrey Smirnov <smirnov.andrey@gmail.com>
andrewrynhard pushed a commit to siderolabs/talos that referenced this pull request Jul 19, 2019
This relies on two PRs to the buildkit:

* moby/buildkit#1081
* moby/buildkit#1085

Sysfs fix was merged to upstream, so updated tag, while using
`Dockerfile` slug I can switch to dockerfile2llb with support for
`--security=insecure`.

Signed-off-by: Andrey Smirnov <smirnov.andrey@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants