Add CLI flags for Swarm Service seccomp, AppArmor, and no-new-privileges #5698

dperny · 2024-12-16T15:16:58Z

- What I did

Add 3 flags to the docker service create and docker service update CLI commands to support the security options in moby/moby#46386.

--apparmor allows setting AppArmor to default or disabled.
--no-new-privileges does what it says on the tin
--seccomp allows either default, unconfined, or a file name of a JSON file with a custom seccomp profile.

- How I did it

Added CLI flags in the standard way. Mostly boilerplate.

- How to verify it

Added tests for the flags.

- Description for the changelog

* Added `--apparmor` flag to `docker service create` and `docker service update`. Allows configuring AppArmor as `default` or `disabled`.
* Added `--no-new-privileges` flag to `docker service create` and `docker service update`.
* Added `--seccomp` flag to `docker service create` and `docker service update`. Allows setting seccomp to `default`, `unconfined`, or a custom profile.

codecov-commenter · 2024-12-16T15:32:04Z

Codecov Report

Attention: Patch coverage is 87.36842% with 12 lines in your changes missing coverage. Please review.

Project coverage is 59.61%. Comparing base (b462778) to head (dc75d4b).
Report is 19 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #5698      +/-   ##
==========================================
+ Coverage   59.51%   59.61%   +0.09%     
==========================================
  Files         346      346              
  Lines       29376    29466      +90     
==========================================
+ Hits        17483    17566      +83     
- Misses      10923    10928       +5     
- Partials      970      972       +2

thaJeztah · 2024-12-16T21:24:44Z

cli/command/service/opts_test.go

+const testJson = `{
+  "json": "you betcha"
+}
+`


Looks like the linter complains on this one;

68.89 cli/command/service/opts_test.go:376:7: ST1003: const testJson should be testJSON (stylecheck) 68.89 const testJson = `{ 68.89 ^

Looks like this const is only used in the test below, so perhaps consider defining the const inside that test. Give that it's a very small JSON, perhaps put it on one line?

const testJSON = `{"json": "you betcha"}`

Oh; I guess that also means changing the file to be the same , well 😂

dperny · 2024-12-17T13:28:18Z

What is done:

All CLI flags
Compose file parsing

What needs to be done still:

Error messages for bad flag values in CLI
Evaluate possible problems with reading seccomp JSON file with os.ReadFile
Compose type conversion (Compose -> Docker API types)
Come up with way to ingest seccomp JSON file with Compose

What I have up now is ready for review, even in its incomplete state, but not ready for merging.

corhere · 2025-01-08T20:31:39Z

cli/command/service/opts.go

+			// TODO(dperny): is it safe/secure to unconditionally read a file like
+			// this? what if the file is REALLY BIG? is that a user problem for
+			// passing a too-big file, or an us problem for ingesting it
+			// unquestioningly?
+			data, err := os.ReadFile(options.seccomp)


IMO it's the user's problem. The flag value is always a file path, except when it isn't; not reading a local file is the astonishing case. It's on them if the CLI process gets OOM-killed because they chose the wrong file.

That being said, interpreting any value aside from the special cases as a file path leaves no room to add new special cases, like additional seccomp modes. Adding a new special case would result in the flag --seccomp=foo changing semantics from reading a local file to whatever the special case is --- a breaking change.

I suggest that only values containing a path separator character be interpreted as a path to a custom seccomp profile. That cleanly solves several problems at once.

Loading a custom profile from a file is always explicit and unambiguous. Users wanting to use a profile located in the CWD would have to pass --seccomp=./my-custom-profile.json in much the same way that a shell user has to type ./my-script.sh to run an executable from the CWD.

All values not containing a slash or backslash character (aside from default and unconfined) are reserved for future use.

corhere · 2025-01-08T20:35:24Z

cli/command/service/opts.go

+				// TODO(dperny): return this, or return "unrecognized option" or some such?
+				return nil, errors.Wrap(err, "unable to read seccomp custom profile file")


Return this. The CLI understands the option and what the user requested. We should tell the user why reading the file failed so they can take corrective action: fix the typo in the file name, sort out file permissions, etc.

Adds support for some security options to the docker service create and docker service update commands. These include: * --no-new-privileges, which does what it says on the tin * --seccomp, which sets seccomp options * --apparmor, which sets apparmor options Does not include compose file changes. Signed-off-by: Drew Erny <[email protected]>

Benehiko · 2025-01-15T11:14:40Z

cli/command/service/opts_test.go

+	// why make an invalid json file when we have one lying right there?
+	flags.Set("seccomp", "testdata/service-context-write-raw.golden")


I don't think we should re-use a golden file from another test here. We can rather create a malformed json file inside a temp directory.

fileName := filepath.Join(t.TempDir(), "malformed-json") err := os.WriteFile(fileName, []byte("not json"), 0o644) assert.NilError(t, err) flags.Set("seccomp", fileName)

Benehiko · 2025-01-15T11:36:32Z

cli/command/service/opts.go

+			// check if the file exists locally
+			if _, err := os.Stat(arg); errors.Is(err, os.ErrNotExist) {
+				return nil, errors.Errorf("unknown seccomp mode %q.", arg)
+			}
+			return nil, errors.Errorf(
+				"unknown seccomp mode %q. (did you mean custom a seccomp profile \"./%s\"?)",
+				arg, arg,
+			)


I'm not sure why it is necessary to check if the file exists if we expect to error anyway.

I would just remove the if statement and return
"unknown seccomp mode %q. a custom seccomp profile must be specified as a relative or absolute file path "./profile.json" or "/path/profile.json"."

Benehiko · 2025-01-15T11:40:08Z

cli/command/service/opts.go

+		}
+		data, err := os.ReadFile(options.seccomp)
+		if err != nil {
+			// TODO(dperny): return this, or return "unrecognized option" or some such?


forgot to remove?

Benehiko · 2025-01-15T11:42:52Z

cli/command/service/opts_test.go

+	oldwd, err := os.Getwd()
+	if err != nil {
+		t.Fatal(err)
+	}
+	if err := os.Chdir("testdata"); err != nil {
+		t.Fatal(err)
+	}
+	t.Cleanup(func() {
+		if err := os.Chdir(oldwd); err != nil {
+			panic(err)
+		}


I don't like these kind of tests that could impact all other tests from running properly. I would remove the chdir and just pass it filename. I suggested above to not check if the file exists since it's a bit superfluous. That way you won't need to chdir.

Benehiko · 2025-01-15T11:44:26Z

cli/command/service/opts_test.go

+	assert.Check(t, is.DeepEqual(privileges, &swarm.Privileges{
+		Seccomp: &swarm.SeccompOpts{
+			Mode:    swarm.SeccompModeCustom,
+			Profile: []byte(testJSON),


you can immediately use the json like so

Suggested change

Profile: []byte(testJSON),

Profile: []byte(`{

"json": "you betcha",

}`),

Benehiko · 2025-01-15T11:44:47Z

cli/command/service/opts_test.go

+const testJSON = `{
+  "json": "you betcha"
+}
+`


We can just remove this const.

thaJeztah reviewed Dec 16, 2024

View reviewed changes

dperny force-pushed the add-seccomp-apparmor-swarm branch 2 times, most recently from c1cec5b to b10de33 Compare December 17, 2024 13:27

dperny force-pushed the add-seccomp-apparmor-swarm branch 2 times, most recently from 2ba8aa5 to 7832807 Compare January 8, 2025 15:42

dperny marked this pull request as ready for review January 8, 2025 18:16

corhere reviewed Jan 8, 2025

View reviewed changes

dperny force-pushed the add-seccomp-apparmor-swarm branch 3 times, most recently from a5b78ec to b66fa05 Compare January 9, 2025 17:54

dperny requested a review from a team as a code owner January 9, 2025 17:54

dperny force-pushed the add-seccomp-apparmor-swarm branch from b66fa05 to dc75d4b Compare January 13, 2025 15:43

Benehiko reviewed Jan 15, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add CLI flags for Swarm Service seccomp, AppArmor, and no-new-privileges #5698

Add CLI flags for Swarm Service seccomp, AppArmor, and no-new-privileges #5698

dperny commented Dec 16, 2024

codecov-commenter commented Dec 16, 2024 •

edited

Loading

thaJeztah Dec 16, 2024

thaJeztah Dec 16, 2024 •

edited

Loading

dperny commented Dec 17, 2024 •

edited

Loading

corhere Jan 8, 2025

corhere Jan 8, 2025

Benehiko Jan 15, 2025

Benehiko Jan 15, 2025

Benehiko Jan 15, 2025

Benehiko Jan 15, 2025

Benehiko Jan 15, 2025

Benehiko Jan 15, 2025

		// TODO(dperny): return this, or return "unrecognized option" or some such?
		return nil, errors.Wrap(err, "unable to read seccomp custom profile file")

		// why make an invalid json file when we have one lying right there?
		flags.Set("seccomp", "testdata/service-context-write-raw.golden")

-			Profile: []byte(testJSON),
+			Profile: []byte(`{
+			"json": "you betcha",
+			}`),

Add CLI flags for Swarm Service seccomp, AppArmor, and no-new-privileges #5698

Are you sure you want to change the base?

Add CLI flags for Swarm Service seccomp, AppArmor, and no-new-privileges #5698

Conversation

dperny commented Dec 16, 2024

codecov-commenter commented Dec 16, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

thaJeztah Dec 16, 2024 • edited Loading

Choose a reason for hiding this comment

dperny commented Dec 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Dec 16, 2024 •

edited

Loading

thaJeztah Dec 16, 2024 •

edited

Loading

dperny commented Dec 17, 2024 •

edited

Loading