add cli integration for unconfined systempaths #1808

martencassel · 2019-04-04T21:49:53Z

- What I did
I've implemented the proposed changes in #1347 (comment).
- Description for the changelog
Added --security-opt systempaths=unconfined support.

codecov-io · 2019-04-04T21:57:27Z

Codecov Report

Merging #1808 into master will decrease coverage by <.01%.
The diff coverage is 47.05%.

@@            Coverage Diff             @@
##           master    #1808      +/-   ##
==========================================
- Coverage   56.31%   56.31%   -0.01%     
==========================================
  Files         308      308              
  Lines       21391    21408      +17     
==========================================
+ Hits        12047    12055       +8     
- Misses       8465     8472       +7     
- Partials      879      881       +2

codecov-io · 2019-04-04T21:57:28Z

Codecov Report

Merging #1808 into master will increase coverage by 0.02%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #1808      +/-   ##
==========================================
+ Coverage   56.31%   56.34%   +0.02%     
==========================================
  Files         308      308              
  Lines       21391    21403      +12     
==========================================
+ Hits        12047    12059      +12     
  Misses       8465     8465              
  Partials      879      879

thaJeztah

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

thaJeztah · 2019-04-05T11:54:35Z

cli/command/container/opts.go

@@ -825,6 +832,23 @@ func parseSecurityOpts(securityOpts []string) ([]string, error) {
 	return securityOpts, nil
 }

+func parseSystemPaths(securityOpts []string) ([]string, []string, []string, error) {
+	var maskedPaths []string
+	var readonlyPaths []string


nit: could be;

Suggested change

var readonlyPaths []string

var maskedPaths , readonlyPaths []string

thaJeztah · 2019-04-05T12:00:57Z

cli/command/container/opts.go

+	filtered := securityOpts[:0]
+	for _, opt := range securityOpts {
+		con := strings.SplitN(opt, "=", 2)
+		if con[0] == "systempaths" && con[1] == "unconfined" {


Looks like we don't need the split (and don't strip whitespace etc) at this point, so this could be simplified to

Suggested change

if con[0] == "systempaths" && con[1] == "unconfined" {

if opt == "systempaths=unconfined" {

thaJeztah · 2019-04-05T12:03:36Z

cli/command/container/opts.go

@@ -485,6 +485,11 @@ func parse(flags *pflag.FlagSet, copts *containerOptions, serverOS string) (*con
 		return nil, err
 	}

+	securityOpts, maskedPaths, readonlyPaths, err := parseSystemPaths(securityOpts)
+	if err != nil {


I see we never return an error; although it's good practice to keep this signature, this function is not exported, so I think it's ok to drop the error return for now, and remove this error-handling

thaJeztah · 2019-04-05T12:15:02Z

cli/command/container/opts.go

@@ -825,6 +832,23 @@ func parseSecurityOpts(securityOpts []string) ([]string, error) {
 	return securityOpts, nil
 }

+func parseSystemPaths(securityOpts []string) ([]string, []string, []string, error) {


I realise this function is not exported, but it might be important to document what it does (especially because it filters the securityOpts; perhaps add a short GoDoc; something along the line of:

Suggested change

func parseSystemPaths(securityOpts []string) ([]string, []string, []string, error) {

// parseSystemPaths checks if `systempaths=unconfined` security option is set,

// and returns the `MaskedPaths` and `ReadonlyPaths` accordingly. An updated

// list of security options is returned with this option removed, because the

// `unconfined` option is handled client-side, and should not be sent to the

// daemon.

func parseSystemPaths(securityOpts []string) ([]string, []string, []string, error) {

(suggestions for better wording welcome! 😂)

Although I'm generally not a fan of named return variables, I'm a bit on the fence if we should use them here (for readability), as three []string returns might be difficult to read/grasp;

Suggested change

func parseSystemPaths(securityOpts []string) ([]string, []string, []string, error) {

func parseSystemPaths(securityOpts []string) (filtered, maskedPaths, readonlyPaths []string) {

I'd love to get feedback from others in that as well though 😅

thaJeztah · 2019-04-05T12:34:24Z

cli/command/container/opts_test.go

@@ -800,3 +800,37 @@ func TestValidateDevice(t *testing.T) {
 		}
 	}
 }
+
+func TestParseSystemPaths(t *testing.T) {
+	valids := []string{


Would be good to test other combinations (i.e., make sure if doesn't touch other values if not set, and that invalid/unknown options are ignored; quick write-up for that;

func TestParseSystemPaths(t *testing.T) { tests := []struct { doc string in, out, masked, readonly []string }{ { doc: "not set", in: []string{}, out: []string{}, }, { doc: "not set, preserve other options", in: []string{ "seccomp=unconfined", "apparmor=unconfined", "label=user:USER", "foo=bar", }, out: []string{ "seccomp=unconfined", "apparmor=unconfined", "label=user:USER", "foo=bar", }, }, { doc: "unconfined", in: []string{"systempaths=unconfined"}, out: []string{}, masked: []string{}, readonly: []string{}, }, { doc: "unconfined and other options", in: []string{"foo=bar", "systempaths=unconfined", "bar=baz"}, out: []string{"foo=bar", "bar=baz"}, masked: []string{}, readonly: []string{}, }, { doc: "unknown option", in: []string{"foo=bar", "systempaths=unkown", "bar=baz"}, out: []string{"foo=bar", "systempaths=unkown", "bar=baz"}, }, } for _, tc := range tests { securityOpts, maskedPaths, readonlyPaths, err := parseSystemPaths(tc.in) assert.NilError(t, err) assert.DeepEqual(t, securityOpts, tc.out) assert.DeepEqual(t, maskedPaths, tc.masked) assert.DeepEqual(t, readonlyPaths, tc.readonly) } }

thaJeztah · 2019-04-05T12:36:45Z

ping @tiborvass @jessfraz PTAL 🤗

martencassel · 2019-04-05T13:32:55Z

I've implemented your suggested changes. I will change unkown to unknown, linting failed on that.

thaJeztah · 2019-04-05T13:35:26Z

Oh! The linter detected my typo 😂

thaJeztah · 2019-04-05T13:37:38Z

Oh! when you're updating, could you fix your sign-off to use your real name (instead of your github handle)?

martencassel <marten.cassel@gmail.com>

Guessing it's Marten Cassel, so should then be:

Marten Cassel <marten.cassel@gmail.com>

thaJeztah

thanks! LGTM after the sign-off is updated 😅

…ent suggested changes Signed-off-by: Mårten Cassel <marten.cassel@gmail.com>

martencassel · 2019-04-05T14:20:37Z

Updated commit sign-off.

thaJeztah

LGTM!

After this, we'll have to update the (reference) docs to mention the new option, and check if the compose-file format needs changes to accommodate (swarm services not yet support this option, so for docker-compose standalone)

AkihiroSuda · 2019-04-16T04:48:20Z

ping @tiborvass @jessfraz

PhilippWendler · 2022-10-14T06:18:51Z

It seems the documentation / https://docs.docker.com/reference/cli/docker/container/run/#security-opt was forgotten to be updated and this option remains undocumented. Could this be added to the docs?

GordonTheTurtle added the status/0-triage label Apr 4, 2019

thaJeztah mentioned this pull request Apr 5, 2019

add cli integration for masked and readonly paths #1347

Closed

thaJeztah requested changes Apr 5, 2019

View reviewed changes

thaJeztah added docs/revisit impact/changelog kind/docs status/2-code-review impact/documentation and removed status/0-triage kind/docs labels Apr 5, 2019

thaJeztah requested changes Apr 5, 2019

View reviewed changes

add cli integration for unconfined systempaths with unit test, implem…

5bc9f49

…ent suggested changes Signed-off-by: Mårten Cassel <marten.cassel@gmail.com>

thaJeztah approved these changes Apr 5, 2019

View reviewed changes

tiborvass approved these changes Apr 16, 2019

View reviewed changes

tiborvass merged commit 2432af7 into docker:master Apr 16, 2019

GordonTheTurtle added this to the 19.03.0 milestone Apr 16, 2019

albers mentioned this pull request Apr 25, 2019

Add bash completion for --security-opt systempaths=unconfined #1849

Merged

rvolgers mentioned this pull request Apr 27, 2019

Last upstream patch was merged, README should be updated genuinetools/img#235

Open

thaJeztah mentioned this pull request May 13, 2019

[19.03 backport] backport bash completion scripts #1876

Merged

etungsten mentioned this pull request Feb 27, 2024

Add systempaths=unconfined security option containerd/nerdctl#2842

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add cli integration for unconfined systempaths #1808

add cli integration for unconfined systempaths #1808

martencassel commented Apr 4, 2019 •

edited

Loading

codecov-io commented Apr 4, 2019

codecov-io commented Apr 4, 2019 •

edited

Loading

thaJeztah left a comment

thaJeztah Apr 5, 2019

thaJeztah Apr 5, 2019

thaJeztah Apr 5, 2019

thaJeztah Apr 5, 2019

thaJeztah Apr 5, 2019

thaJeztah commented Apr 5, 2019

martencassel commented Apr 5, 2019 •

edited

Loading

thaJeztah commented Apr 5, 2019

thaJeztah commented Apr 5, 2019

thaJeztah left a comment

martencassel commented Apr 5, 2019 •

edited

Loading

thaJeztah left a comment

AkihiroSuda commented Apr 16, 2019

PhilippWendler commented Oct 14, 2022 •

edited by thaJeztah

Loading

	var readonlyPaths []string
	var maskedPaths , readonlyPaths []string

	if con[0] == "systempaths" && con[1] == "unconfined" {
	if opt == "systempaths=unconfined" {

-func parseSystemPaths(securityOpts []string) ([]string, []string, []string, error) {
+// parseSystemPaths checks if `systempaths=unconfined` security option is set,
+// and returns the `MaskedPaths` and `ReadonlyPaths` accordingly. An updated
+// list of security options is returned with this option removed, because the
+// `unconfined` option is handled client-side, and should not be sent to the
+// daemon.
+func parseSystemPaths(securityOpts []string) ([]string, []string, []string, error) {

	func parseSystemPaths(securityOpts []string) ([]string, []string, []string, error) {
	func parseSystemPaths(securityOpts []string) (filtered, maskedPaths, readonlyPaths []string) {

add cli integration for unconfined systempaths #1808

add cli integration for unconfined systempaths #1808

Conversation

martencassel commented Apr 4, 2019 • edited Loading

codecov-io commented Apr 4, 2019

Codecov Report

codecov-io commented Apr 4, 2019 • edited Loading

Codecov Report

thaJeztah left a comment

Choose a reason for hiding this comment

thaJeztah Apr 5, 2019

Choose a reason for hiding this comment

thaJeztah Apr 5, 2019

Choose a reason for hiding this comment

thaJeztah Apr 5, 2019

Choose a reason for hiding this comment

thaJeztah Apr 5, 2019

Choose a reason for hiding this comment

thaJeztah Apr 5, 2019

Choose a reason for hiding this comment

thaJeztah commented Apr 5, 2019

martencassel commented Apr 5, 2019 • edited Loading

thaJeztah commented Apr 5, 2019

thaJeztah commented Apr 5, 2019

thaJeztah left a comment

Choose a reason for hiding this comment

martencassel commented Apr 5, 2019 • edited Loading

thaJeztah left a comment

Choose a reason for hiding this comment

AkihiroSuda commented Apr 16, 2019

PhilippWendler commented Oct 14, 2022 • edited by thaJeztah Loading

martencassel commented Apr 4, 2019 •

edited

Loading

codecov-io commented Apr 4, 2019 •

edited

Loading

martencassel commented Apr 5, 2019 •

edited

Loading

martencassel commented Apr 5, 2019 •

edited

Loading

PhilippWendler commented Oct 14, 2022 •

edited by thaJeztah

Loading