Add support for exact list of capabilities + capAdd / capDrop refactor by olljanat · Pull Request #38380 · moby/moby

olljanat · 2018-12-16T15:24:29Z

- What I did
Added support for exact list of capabilities list of container capabilities which can be used instead of these CapAdd and CapDrop like discussed on #26849 (comment)

and refactored capAdd and capDrop to share as much code as possible with this new feature.

- How I did it
In cooperation with @thaJeztah 😉

- How to verify it
CI makes sure that existing functionality works like expected and new integration tests verify that new functionality works correctly.

- Description for the changelog

Add support for exact list of capabilities, support only OCI model
Support OCI model on CapAdd and CapDrop but remain backward compatibility
Create variable locally instead of declaring it at the top
Use const for magic "ALL" value
Rename cap variable as it overlaps with cap() built-in
Normalize and validate capabilities before use
Move validation for conflicting options to validateHostConfig()
TweakCapabilities: simplify logic to calculate capabilities

- Comments
First step to solve #25885 , #24862 and moby/swarmkit#1030
Replaces #26849
Dependency for moby/swarmkit#2795

codecov · 2018-12-16T17:51:24Z

Codecov Report

Merging #38380 into master will increase coverage by 0.01%.
The diff coverage is 16.66%.

@@            Coverage Diff             @@
##           master   #38380      +/-   ##
==========================================
+ Coverage    36.6%   36.61%   +0.01%     
==========================================
  Files         608      608              
  Lines       45224    45241      +17     
==========================================
+ Hits        16553    16567      +14     
+ Misses      26384    26383       -1     
- Partials     2287     2291       +4

olljanat · 2018-12-16T20:14:51Z

@thaJeztah looks working fine with moby/swarmkit#2795 so can you review and give your thoughts?

Docs updates are not included yet on purpose because I would get #37940 merged first and don't want cause conflicts with it.

thaJeztah

Thanks for working on this! I left some comments inline.

oci/oci.go

oci/caps/utils.go

oci/oci.go

api/types/container/host_config.go

olljanat · 2018-12-17T20:50:48Z

@thaJeztah updated based on feedback 😎

EDIT: Also modified to API comment to use same format than on #38381

thaJeztah · 2019-01-18T10:50:21Z

Also, if we change how we store this information in the container's spec on disk (not sure we do, just thinking out loud), we need to verify that the daemon will still support containers that are created with an older version, and doesn't doesn't prevent those from being started

olljanat · 2019-01-19T10:30:35Z

Also, if we change how we store this information in the container's spec on disk (not sure we do, just thinking out loud), we need to verify that the daemon will still support containers that are created with an older version, and doesn't doesn't prevent those from being started

No we don't change this. Container spec is stored like it is provided to API. Normalization for CapAdd / CapDrop happens when container starts.

There is quite many old test cases on

moby/integration-cli/docker_cli_run_test.go

Lines 977 to 1073 in c8ff5ec

    
           func (s *DockerSuite) TestRunCapDropInvalid(c *check.C) { 
        
           	// Not applicable for Windows as there is no concept of --cap-drop 
        
           	testRequires(c, DaemonIsLinux) 
        
           	out, _, err := dockerCmdWithError("run", "--cap-drop=CHPASS", "busybox", "ls") 
        
           	if err == nil { 
        
           		c.Fatal(err, out) 
        
           	} 
        
           } 
        
           func (s *DockerSuite) TestRunCapDropCannotMknod(c *check.C) { 
        
           	// Not applicable for Windows as there is no concept of --cap-drop or mknod 
        
           	testRequires(c, DaemonIsLinux) 
        
           	out, _, err := dockerCmdWithError("run", "--cap-drop=MKNOD", "busybox", "sh", "-c", "mknod /tmp/sda b 8 0 && echo ok") 
        
           	if err == nil { 
        
           		c.Fatal(err, out) 
        
           	} 
        
           	if actual := strings.Trim(out, "\r\n"); actual == "ok" { 
        
           		c.Fatalf("expected output not ok received %s", actual) 
        
           	} 
        
           } 
        
           func (s *DockerSuite) TestRunCapDropCannotMknodLowerCase(c *check.C) { 
        
           	// Not applicable for Windows as there is no concept of --cap-drop or mknod 
        
           	testRequires(c, DaemonIsLinux) 
        
           	out, _, err := dockerCmdWithError("run", "--cap-drop=mknod", "busybox", "sh", "-c", "mknod /tmp/sda b 8 0 && echo ok") 
        
           	if err == nil { 
        
           		c.Fatal(err, out) 
        
           	} 
        
           	if actual := strings.Trim(out, "\r\n"); actual == "ok" { 
        
           		c.Fatalf("expected output not ok received %s", actual) 
        
           	} 
        
           } 
        
           func (s *DockerSuite) TestRunCapDropALLCannotMknod(c *check.C) { 
        
           	// Not applicable for Windows as there is no concept of --cap-drop or mknod 
        
           	testRequires(c, DaemonIsLinux) 
        
           	out, _, err := dockerCmdWithError("run", "--cap-drop=ALL", "--cap-add=SETGID", "busybox", "sh", "-c", "mknod /tmp/sda b 8 0 && echo ok") 
        
           	if err == nil { 
        
           		c.Fatal(err, out) 
        
           	} 
        
           	if actual := strings.Trim(out, "\r\n"); actual == "ok" { 
        
           		c.Fatalf("expected output not ok received %s", actual) 
        
           	} 
        
           } 
        
           func (s *DockerSuite) TestRunCapDropALLAddMknodCanMknod(c *check.C) { 
        
           	// Not applicable for Windows as there is no concept of --cap-drop or mknod 
        
           	testRequires(c, DaemonIsLinux, NotUserNamespace) 
        
           	out, _ := dockerCmd(c, "run", "--cap-drop=ALL", "--cap-add=MKNOD", "--cap-add=SETGID", "busybox", "sh", "-c", "mknod /tmp/sda b 8 0 && echo ok") 
        
           	if actual := strings.Trim(out, "\r\n"); actual != "ok" { 
        
           		c.Fatalf("expected output ok received %s", actual) 
        
           	} 
        
           } 
        
           func (s *DockerSuite) TestRunCapAddInvalid(c *check.C) { 
        
           	// Not applicable for Windows as there is no concept of --cap-add 
        
           	testRequires(c, DaemonIsLinux) 
        
           	out, _, err := dockerCmdWithError("run", "--cap-add=CHPASS", "busybox", "ls") 
        
           	if err == nil { 
        
           		c.Fatal(err, out) 
        
           	} 
        
           } 
        
           func (s *DockerSuite) TestRunCapAddCanDownInterface(c *check.C) { 
        
           	// Not applicable for Windows as there is no concept of --cap-add 
        
           	testRequires(c, DaemonIsLinux) 
        
           	out, _ := dockerCmd(c, "run", "--cap-add=NET_ADMIN", "busybox", "sh", "-c", "ip link set eth0 down && echo ok") 
        
           	if actual := strings.Trim(out, "\r\n"); actual != "ok" { 
        
           		c.Fatalf("expected output ok received %s", actual) 
        
           	} 
        
           } 
        
           func (s *DockerSuite) TestRunCapAddALLCanDownInterface(c *check.C) { 
        
           	// Not applicable for Windows as there is no concept of --cap-add 
        
           	testRequires(c, DaemonIsLinux) 
        
           	out, _ := dockerCmd(c, "run", "--cap-add=ALL", "busybox", "sh", "-c", "ip link set eth0 down && echo ok") 
        
           	if actual := strings.Trim(out, "\r\n"); actual != "ok" { 
        
           		c.Fatalf("expected output ok received %s", actual) 
        
           	} 
        
           } 
        
           func (s *DockerSuite) TestRunCapAddALLDropNetAdminCanDownInterface(c *check.C) { 
        
           	// Not applicable for Windows as there is no concept of --cap-add 
        
           	testRequires(c, DaemonIsLinux) 
        
           	out, _, err := dockerCmdWithError("run", "--cap-add=ALL", "--cap-drop=NET_ADMIN", "busybox", "sh", "-c", "ip link set eth0 down && echo ok") 
        
           	if err == nil { 
        
           		c.Fatal(err, out) 
        
           	} 
        
           	if actual := strings.Trim(out, "\r\n"); actual == "ok" { 
        
           		c.Fatalf("expected output not ok received %s", actual) 
        
           	} 
        
           }

which should make sure that old configs works.

thaJeztah

Thanks @olljanat!

I left comments inline, but was going through these changes and that the validation only take place on docker start (not on docker create).

I have some ideas, but probably faster to show those instead of trying to explain them; I'll work on a branch so that you can incorporate those changes in your PR if you agree with them 🤗

Will post a link once if made those changes 👍

integration/container/create_test.go

oci/oci.go

integration/container/create_test.go

oci/caps/utils.go

daemon/oci_windows.go

daemon/oci_linux.go

daemon/oci_windows.go

thaJeztah · 2019-01-22T13:08:49Z

Pushed a branch here; I split my changes into several commits, to make it easier to grasp what I did 😅 master...thaJeztah:capabilities_support_move_to_create

(or diff compared to your branch; olljanat/moby@capabilities-support...thaJeztah:capabilities_support_move_to_create)

thaJeztah · 2019-01-22T13:11:22Z

oh, hold on, pushed one outdated commit; fixing

thaJeztah · 2019-01-22T13:18:23Z

done 👍

olljanat · 2019-01-22T16:54:45Z

I have some ideas, but probably faster to show those instead of trying to explain them; I'll work on a branch so that you can incorporate those changes in your PR if you agree with them

@thaJeztah those looks good on quick view. Will do some testing with them but I included them already so we can see result of CI.

olljanat · 2019-01-22T17:12:37Z

One "minor" thing. --cap-add or --cap-drop does not work anymore so CI should fail to it. Let's see. I will look if I find why it is so.

EDIT: Found it. Fixed on olljanat@d5404a7 but I will wait that CI finishes so we can see that it really finds those failures.

EDIT 2: Yes. Looks correct: https://jenkins.dockerproject.org/job/Docker-PRs/52704/console

olljanat · 2019-01-22T19:03:06Z

@thaJeztah I'm not sure if that is correct way but I rebased this now to one commit which is signed off by both of us (have seen that on some old commits too).

oci/caps/utils.go

thaJeztah · 2019-01-22T19:47:45Z

but I rebased this now to one commit which is signed off by both of us (have seen that on some old commits too).

That sounds ok 👍
(I don't need the credits for this, so no need to keep my commits separate)

- Add support for exact list of capabilities, support only OCI model - Support OCI model on CapAdd and CapDrop but remain backward compatibility - Create variable locally instead of declaring it at the top - Use const for magic "ALL" value - Rename `cap` variable as it overlaps with `cap()` built-in - Normalize and validate capabilities before use - Move validation for conflicting options to validateHostConfig() - TweakCapabilities: simplify logic to calculate capabilities Signed-off-by: Olli Janatuinen <olli.janatuinen@gmail.com> Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

olljanat · 2019-01-22T21:31:51Z

LGTM, but I do think we should fix the issue that you can't say CAP_SYS_ADMIN you have to say SYS_ADMIN before we ship this, and accept either form as this won't break anything.

@justincormack now this is handled. PTAL

thaJeztah

LGTM (saw I still had "changes requested")

thaJeztah · 2019-01-25T15:05:07Z

(We can look at the client normalising the values in a follow-up (probably limiting to uppercasing values before sending them over the API))

olljanat · 2019-01-25T16:08:32Z

@AkihiroSuda can you look this one again as it was heavily modified after your last review?

thaJeztah · 2019-01-28T15:35:58Z

I think this should be ready to go; merging.

@AkihiroSuda let me know if there's follow-ups to do after this is merged 🤗

AkihiroSuda

LGTM

olljanat · 2019-02-21T11:06:20Z

@presidenten no. up to date status is visible on this message: #25885 (comment)

Target is get complete solution out part of 19.06.

GordonTheTurtle added the status/0-triage label Dec 16, 2018

This was referenced Dec 16, 2018

Add capabilities list to container specification moby/swarmkit#2795

Merged

Missing from Swarmmode --cap-add #25885

Closed

olljanat force-pushed the capabilities-support branch from 59df37d to b6b0685 Compare December 16, 2018 17:51

olljanat force-pushed the capabilities-support branch from b6b0685 to 9500e14 Compare December 16, 2018 19:14

olljanat changed the title ~~WIP: Add support for exact list of capabilities~~ Add support for exact list of capabilities Dec 16, 2018

thaJeztah added area/api API status/1-design-review impact/changelog area/swarm and removed status/0-triage labels Dec 17, 2018

thaJeztah requested changes Dec 17, 2018

View reviewed changes

thaJeztah mentioned this pull request Dec 17, 2018

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

Closed

olljanat force-pushed the capabilities-support branch 3 times, most recently from e5eda15 to 912512b Compare December 17, 2018 20:14

olljanat force-pushed the capabilities-support branch 2 times, most recently from 6836f4e to 51ab0bc Compare December 19, 2018 22:11

olljanat requested a review from vdemeester as a code owner December 19, 2018 22:11

olljanat force-pushed the capabilities-support branch 6 times, most recently from ef14e5b to 9a77b03 Compare December 22, 2018 17:55

derek bot added the rebuild/powerpc label Dec 22, 2018

GordonTheTurtle removed the rebuild/powerpc label Dec 22, 2018

derek bot added the rebuild/windowsRS1 label Jan 18, 2019

GordonTheTurtle removed the rebuild/windowsRS1 label Jan 18, 2019

olljanat force-pushed the capabilities-support branch from 4fbc320 to f9ec660 Compare January 19, 2019 10:04

derek bot added the rebuild/experimental label Jan 19, 2019

GordonTheTurtle removed the rebuild/experimental label Jan 19, 2019

thaJeztah requested changes Jan 22, 2019

View reviewed changes

olljanat force-pushed the capabilities-support branch from fb46376 to 2fc46c0 Compare January 22, 2019 18:58

thaJeztah reviewed Jan 22, 2019

View reviewed changes

oci/caps/utils.go Outdated Show resolved Hide resolved

olljanat force-pushed the capabilities-support branch from 2fc46c0 to 80d7bfd Compare January 22, 2019 19:51

olljanat changed the title ~~Add support for exact list of capabilities~~ Add support for exact list of capabilities + capAdd / capDrop refactor Jan 22, 2019

derek bot added the rebuild/windowsRS1 label Jan 23, 2019

thaJeztah approved these changes Jan 25, 2019

View reviewed changes

justincormack approved these changes Jan 25, 2019

View reviewed changes

AkihiroSuda reviewed Jan 28, 2019

View reviewed changes

thaJeztah mentioned this pull request Apr 3, 2024

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

Open

Conversation

olljanat commented Dec 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Dec 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

olljanat commented Dec 16, 2018

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

olljanat commented Dec 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

thaJeztah commented Jan 18, 2019

Uh oh!

olljanat commented Jan 19, 2019

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

thaJeztah commented Jan 22, 2019

Uh oh!

thaJeztah commented Jan 22, 2019

Uh oh!

thaJeztah commented Jan 22, 2019

Uh oh!

olljanat commented Jan 22, 2019

Uh oh!

olljanat commented Jan 22, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

olljanat commented Jan 22, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

thaJeztah commented Jan 22, 2019

Uh oh!

olljanat commented Jan 22, 2019

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

thaJeztah commented Jan 25, 2019

Uh oh!

olljanat commented Jan 25, 2019

Uh oh!

thaJeztah commented Jan 28, 2019

Uh oh!

AkihiroSuda left a comment

Choose a reason for hiding this comment

Uh oh!

olljanat commented Feb 21, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

olljanat commented Dec 16, 2018 •

edited

Loading

codecov bot commented Dec 16, 2018 •

edited

Loading

olljanat commented Dec 17, 2018 •

edited

Loading

olljanat commented Jan 22, 2019 •

edited

Loading

olljanat commented Jan 22, 2019 •

edited

Loading