Skip to content

Whitelist syscalls linked to CAP_SYS_NICE in default seccomp profile #37242

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.

Sign up for GitHub

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

Jump to bottom
Merged
thaJeztah merged 1 commit into from
Jul 3, 2018
Merged

Whitelist syscalls linked to CAP_SYS_NICE in default seccomp profile #37242

thaJeztah merged 1 commit into from
Jul 3, 2018

Conversation

@nvcastet
Copy link
Contributor

@nvcastet nvcastet commented Jun 8, 2018

@thaJeztah
Copy link
Member

thaJeztah commented Jun 8, 2018

ping @justincormack PTAL

@thaJeztah
Copy link
Member

thaJeztah commented Jun 8, 2018

@nvcastet looks like you need to regenerate some files;

16:50:05 The result of go generate ./profiles/seccomp/ differs
16:50:05 
16:50:05  M profiles/seccomp/default.json
16:50:05 
16:50:05 Please re-run go generate ./profiles/seccomp/
16:50:05

@thaJeztah thaJeztah added the status/failing-ci Indicates that the PR in its current state fails the test suite label Jun 8, 2018
@thaJeztah
Copy link
Member

thaJeztah commented Jun 8, 2018

(and likely squash the two commits)

justincormack
justincormack reviewed Jun 8, 2018
View reviewed changes
profiles/seccomp/default.json Outdated
Copy link
Contributor

@justincormack justincormack Jun 8, 2018

Choose a reason for hiding this comment

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

I think you will need to keep the original file ending without a new line to get the validation to work.

Copy link
Contributor Author

@nvcastet nvcastet Jun 9, 2018

Choose a reason for hiding this comment

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

Will do.

justincormack
justincormack reviewed Jun 8, 2018
View reviewed changes
profiles/seccomp/seccomp_default.go Outdated
Copy link
Contributor

@justincormack justincormack Jun 8, 2018

Choose a reason for hiding this comment

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

name_to_handle_at is nothing to do with CAP_SYS_NICE it is gated by CAP_DAC_READ_SEARCH and there are other reasons for excluding it.

Copy link
Contributor Author

@nvcastet nvcastet Jun 9, 2018

Choose a reason for hiding this comment

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

@justincormack I am totally fine removing it. But in that case the documentation would need to be updated at https://docs.docker.com/engine/security/seccomp/. Search for name_to_handle_at, it is mentioned Already gated by CAP_SYS_NICE..

Copy link
Contributor Author

@nvcastet nvcastet Jun 9, 2018

Choose a reason for hiding this comment

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

Opened docker/docs#6854

@nvcastet nvcastet mentioned this pull request Jun 9, 2018
Closed
@nvcastet nvcastet force-pushed the fix_sys_nice_seccomp branch 2 times, most recently from 6d17fd4 to d6b767f Compare June 11, 2018 12:57
@codecov
Copy link

codecov bot commented Jun 11, 2018 •
edited
Loading

Codecov Report

❗ No coverage uploaded for pull request base (master@e259323). Click here to learn what that means.
The diff coverage is 0%.

@@            Coverage Diff            @@
##             master   #37242   +/-   ##
=========================================
  Coverage          ?   35.32%           
=========================================
  Files             ?      609           
  Lines             ?    45011           
  Branches          ?        0           
=========================================
  Hits              ?    15898           
  Misses            ?    26959           
  Partials          ?     2154

@nvcastet
Copy link
Contributor Author

nvcastet commented Jun 11, 2018

@thaJeztah Would you know why the DockerSwarmSuite.TestAPISwarmLeaderElection test keeps failing?

14:20:57 FAIL: docker_api_swarm_test.go:296: DockerSwarmSuite.TestAPISwarmLeaderElection
14:20:57 
14:20:57 [d4789021c34ee] waiting for daemon to start
14:20:57 [d4789021c34ee] daemon started
14:20:57 
14:20:57 [da4194801ef22] waiting for daemon to start
14:20:57 [da4194801ef22] daemon started
14:20:57 
14:20:57 [decc49a95fe76] waiting for daemon to start
14:20:57 [decc49a95fe76] daemon started
14:20:57 
14:20:57 [d4789021c34ee] exiting daemon
14:20:57 assertion failed: error is not nil: Error response from daemon: rpc error: code = DeadlineExceeded desc = context deadline exceeded
14:20:57 [da4194801ef22] daemon started
14:20:57 Attempt #2: daemon is still running with pid 22957
14:20:57 [da4194801ef22] exiting daemon
14:20:57 [decc49a95fe76] exiting daemon

@thaJeztah
Copy link
Member

thaJeztah commented Jun 11, 2018

Looks like that one is marked "flaky"; #32673

@thaJeztah thaJeztah added rebuild/* and removed status/failing-ci Indicates that the PR in its current state fails the test suite labels Jun 11, 2018
@nvcastet nvcastet force-pushed the fix_sys_nice_seccomp branch 2 times, most recently from 701d53b to 700b4b4 Compare June 14, 2018 15:01
@nvcastet
Copy link
Contributor Author

nvcastet commented Jun 15, 2018

@thaJeztah Thanks. Do you know if it is possible to retrigger just the PR jobs that failed (here janky and windowsRS1)?

@nvcastet nvcastet force-pushed the fix_sys_nice_seccomp branch from 700b4b4 to 47dfff6 Compare June 20, 2018 12:32
@thaJeztah
Copy link
Member

thaJeztah commented Jun 20, 2018

Hm, CI doesn't seem to restart; I asked internally if someone has access to do so

@justincormack PTAL

@thaJeztah
Copy link
Member

thaJeztah commented Jun 21, 2018

Failure on PowerPC can be ignored;

13:56:35 FAIL: docker_cli_daemon_test.go:1800: DockerDaemonSuite.TestDaemonNoSpaceLeftOnDeviceError
13:56:35 
13:56:35 [decb810dce5dc] waiting for daemon to start
13:56:35 [decb810dce5dc] exiting daemon
13:56:35 docker_cli_daemon_test.go:1817:
13:56:35     s.d.Start(c, "--data-root", filepath.Join(testDir, "test-mount"))
13:56:35 /go/src/github.com/docker/docker/internal/test/daemon/daemon.go:203:
13:56:35     t.Fatalf("Error starting daemon with arguments: %v", args)
13:56:35 ... Error: Error starting daemon with arguments: [--data-root /tmp/no-space-left-on-device-test924999940/test-mount]
13:56:35

@n4ss
Copy link

n4ss commented Jun 21, 2018

LGTM !

@nvcastet
Copy link
Contributor Author

nvcastet commented Jun 28, 2018

@thaJeztah @justincormack Anything else needed to merge this PR?

@justincormack
Copy link
Contributor

justincormack commented Jun 29, 2018

Its slightly odd gating these all by CAP_SYS_NICE but I think it makes sense.

thaJeztah
thaJeztah approved these changes Jun 29, 2018
View reviewed changes
Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

LGTM

@thaJeztah
Copy link
Member

thaJeztah commented Jun 29, 2018

@nvcastet will you do a follow up PR in the documentation repo?

@nvcastet
Copy link
Contributor Author

nvcastet commented Jul 2, 2018 •
edited
Loading

@thaJeztah Documentation PR was created at: docker/docs#6861

@thaJeztah
Copy link
Member

thaJeztah commented Jul 3, 2018

Thanks!

Failures look to be flaky tests, so I'll go ahead and merge

@thaJeztah thaJeztah merged commit 6273dff into moby:master Jul 3, 2018
@yosifkit yosifkit mentioned this pull request May 13, 2019
Merged
This was referenced Jul 11, 2022
Closed
Merged
@thaJeztah thaJeztah mentioned this pull request Jan 24, 2023
Merged
@filimonov filimonov mentioned this pull request Oct 21, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@thaJeztah thaJeztah thaJeztah approved these changes

+1 more reviewer

@justincormack justincormack justincormack approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@stevvooe stevvooe

Labels

area/security/seccomp impact/changelog impact/documentation status/4-merge

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

syscalls linked to CAP_SYS_NICE are not whitelisted in default seccomp profile

7 participants

@nvcastet @thaJeztah @n4ss @justincormack @stevvooe @yongtang @GordonTheTurtle