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.

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

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
+273 −6
Open

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
129 changes: 123 additions & 6 deletions cli/command/service/opts.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,10 @@ package service

import (
"context"
"encoding/json"
"fmt"
"os"
"path/filepath"
"sort"
"strconv"
"strings"
Expand Down Expand Up @@ -531,6 +534,10 @@ type serviceOptions struct {
ulimits opts.UlimitOpt
oomScoreAdj int64

seccomp string
appArmor string
noNewPrivileges bool

resources resourceOptions
stopGrace opts.DurationOpt

Expand Down Expand Up @@ -660,6 +667,109 @@ func (options *serviceOptions) makeEnv() ([]string, error) {
return currentEnv, nil
}

func (options *serviceOptions) ToPrivileges(flags *pflag.FlagSet) (*swarm.Privileges, error) {
// we're going to go through several possible uses of the Privileges
// struct, which may or may not be used. If some stage uses it (after the
// first), we'll check if it's nil and create it if it hasn't been created
// yet.
var privileges *swarm.Privileges
if options.credentialSpec.String() != "" && options.credentialSpec.Value() != nil {
privileges = &swarm.Privileges{
CredentialSpec: options.credentialSpec.Value(),
}
}

if flags.Changed(flagNoNewPrivileges) {
if privileges == nil {
privileges = &swarm.Privileges{}
}
privileges.NoNewPrivileges = options.noNewPrivileges
}

if flags.Changed(flagAppArmor) {
if privileges == nil {
privileges = &swarm.Privileges{}
}
switch options.appArmor {
case "default":
privileges.AppArmor = &swarm.AppArmorOpts{
Mode: swarm.AppArmorModeDefault,
}
case "disabled":
privileges.AppArmor = &swarm.AppArmorOpts{
Mode: swarm.AppArmorModeDisabled,
}
default:
return nil, errors.Errorf(
"unknown AppArmor mode %q. Supported modes are %q and %q",
options.appArmor,
swarm.AppArmorModeDefault,
swarm.AppArmorModeDisabled,
)
}
}

if flags.Changed(flagSeccomp) {
if privileges == nil {
privileges = &swarm.Privileges{}
}
seccomp, err := options.ToSeccompOpts()
if err != nil {
return nil, err
}
privileges.Seccomp = seccomp
}

return privileges, nil
}

func (options *serviceOptions) ToSeccompOpts() (*swarm.SeccompOpts, error) {
switch arg := options.seccomp; arg {
case "default":
return &swarm.SeccompOpts{
Mode: swarm.SeccompModeDefault,
}, nil
case "unconfined":
return &swarm.SeccompOpts{
Mode: swarm.SeccompModeUnconfined,
}, nil
default:
dir, _ := filepath.Split(arg)
// if the directory is empty, this isn't a file path. Even though
// the user may be referring to a file in the local directory, for
// disambiguation's sake, we require a custom profile file to be
// given as a path.
if dir == "" {
// 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,
)
Comment on lines +743 to +750
Copy link
Member

Choose a reason for hiding this comment

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

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"."

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

Choose a reason for hiding this comment

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

forgot to remove?

return nil, errors.Wrap(err, "unable to read seccomp custom profile file")
}
// we're doing the user a favor here by refusing to pass garbage if
// they give invalid json.
if !json.Valid(data) {
return nil, errors.Errorf(
"unable to read seccomp custom profile file %q: not valid json",
options.seccomp,
)
}

return &swarm.SeccompOpts{
Mode: swarm.SeccompModeCustom,
Profile: data,
}, nil
}
}

// ToService takes the set of flags passed to the command and converts them
// into a service spec.
//
Expand Down Expand Up @@ -712,6 +822,11 @@ func (options *serviceOptions) ToService(ctx context.Context, apiClient client.N
return service, err
}

privileges, err := options.ToPrivileges(flags)
if err != nil {
return service, err
}

capAdd, capDrop := opts.EffectiveCapAddCapDrop(options.capAdd.GetAll(), options.capDrop.GetAll())

service = swarm.ServiceSpec{
Expand All @@ -730,6 +845,7 @@ func (options *serviceOptions) ToService(ctx context.Context, apiClient client.N
Dir: options.workdir,
User: options.user,
Groups: options.groups.GetAll(),
Privileges: privileges,
StopSignal: options.stopSignal,
TTY: options.tty,
ReadOnly: options.readOnly,
Expand Down Expand Up @@ -766,12 +882,6 @@ func (options *serviceOptions) ToService(ctx context.Context, apiClient client.N
EndpointSpec: options.endpoint.ToEndpointSpec(),
}

if options.credentialSpec.String() != "" && options.credentialSpec.Value() != nil {
service.TaskTemplate.ContainerSpec.Privileges = &swarm.Privileges{
CredentialSpec: options.credentialSpec.Value(),
}
}

return service, nil
}

Expand Down Expand Up @@ -886,6 +996,10 @@ func addServiceFlags(flags *pflag.FlagSet, options *serviceOptions, defaultFlagV
flags.StringVar(&options.update.order, flagUpdateOrder, "", flagDesc(flagUpdateOrder, `Update order ("start-first", "stop-first")`))
flags.SetAnnotation(flagUpdateOrder, "version", []string{"1.29"})

flags.StringVar(&options.seccomp, flagSeccomp, "", flagDesc(flagSeccomp, `Seccomp configuration ("default", "unconfined", or a path to a json custom seccomp profile)`))
flags.StringVar(&options.appArmor, flagAppArmor, "", flagDesc(flagAppArmor, `AppArmor mode ("default" or "disabled"`))
flags.BoolVar(&options.noNewPrivileges, flagNoNewPrivileges, false, flagDesc(flagNoNewPrivileges, "Disable container processes from gaining new privileges"))

flags.Uint64Var(&options.rollback.parallelism, flagRollbackParallelism, defaultFlagValues.getUint64(flagRollbackParallelism),
"Maximum number of tasks rolled back simultaneously (0 to roll back all at once)")
flags.SetAnnotation(flagRollbackParallelism, "version", []string{"1.28"})
Expand Down Expand Up @@ -937,6 +1051,7 @@ func addServiceFlags(flags *pflag.FlagSet, options *serviceOptions, defaultFlagV
}

const (
flagAppArmor = "apparmor"
flagCredentialSpec = "credential-spec" //nolint:gosec // ignore G101: Potential hardcoded credentials
flagPlacementPref = "placement-pref"
flagPlacementPrefAdd = "placement-pref-add"
Expand Down Expand Up @@ -1008,6 +1123,7 @@ const (
flagRollbackOrder = "rollback-order"
flagRollbackParallelism = "rollback-parallelism"
flagInit = "init"
flagSeccomp = "seccomp"
flagSysCtl = "sysctl"
flagSysCtlAdd = "sysctl-add"
flagSysCtlRemove = "sysctl-rm"
Expand All @@ -1023,6 +1139,7 @@ const (
flagUser = "user"
flagWorkdir = "workdir"
flagRegistryAuth = "with-registry-auth"
flagNoNewPrivileges = "no-new-privileges"
flagNoResolveImage = "no-resolve-image"
flagLogDriver = "log-driver"
flagLogOpt = "log-opt"
Expand Down
141 changes: 141 additions & 0 deletions cli/command/service/opts_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@ package service
import (
"context"
"fmt"
"os"
"strings"
"testing"
"time"

Expand Down Expand Up @@ -326,3 +328,142 @@ func TestToServiceSysCtls(t *testing.T) {
assert.NilError(t, err)
assert.Check(t, is.DeepEqual(service.TaskTemplate.ContainerSpec.Sysctls, expected))
}

func TestToPrivilegesAppArmor(t *testing.T) {
for _, mode := range []string{"default", "disabled"} {
flags := newCreateCommand(nil).Flags()
flags.Set("apparmor", mode)
o := newServiceOptions()
o.appArmor = mode
privileges, err := o.ToPrivileges(flags)
assert.NilError(t, err)
enumMode := swarm.AppArmorMode(mode)
assert.Check(t, is.DeepEqual(privileges, &swarm.Privileges{
AppArmor: &swarm.AppArmorOpts{
Mode: enumMode,
},
}))
}
}

func TestToPrivilegesAppArmorInvalid(t *testing.T) {
flags := newCreateCommand(nil).Flags()
flags.Set("apparmor", "invalid")
o := newServiceOptions()
o.appArmor = "invalid"

privileges, err := o.ToPrivileges(flags)
assert.ErrorContains(t, err, "AppArmor")
assert.Check(t, is.Nil(privileges))
}

func TestToPrivilegesSeccomp(t *testing.T) {
for _, mode := range []string{"default", "unconfined"} {
flags := newCreateCommand(nil).Flags()
flags.Set("seccomp", mode)
o := newServiceOptions()
o.seccomp = mode

privileges, err := o.ToPrivileges(flags)
assert.NilError(t, err)
enumMode := swarm.SeccompMode(mode)
assert.Check(t, is.DeepEqual(privileges, &swarm.Privileges{
Seccomp: &swarm.SeccompOpts{
Mode: enumMode,
},
}))
}
}

const testJSON = `{
"json": "you betcha"
}
`
Comment on lines +378 to +381
Copy link
Member

Choose a reason for hiding this comment

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

We can just remove this const.


func TestToPrivilegesSeccompCustomProfile(t *testing.T) {
flags := newCreateCommand(nil).Flags()
flags.Set("seccomp", "testdata/test-seccomp-valid.json")
o := newServiceOptions()
o.seccomp = "testdata/test-seccomp-valid.json"

privileges, err := o.ToPrivileges(flags)
assert.NilError(t, err)
assert.Check(t, is.DeepEqual(privileges, &swarm.Privileges{
Seccomp: &swarm.SeccompOpts{
Mode: swarm.SeccompModeCustom,
Profile: []byte(testJSON),
Copy link
Member

Choose a reason for hiding this comment

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

you can immediately use the json like so

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

},
}))
}

func TestToPrivilegesSeccompInvalidJson(t *testing.T) {
flags := newCreateCommand(nil).Flags()
// why make an invalid json file when we have one lying right there?
flags.Set("seccomp", "testdata/service-context-write-raw.golden")
Comment on lines +401 to +402
Copy link
Member

Choose a reason for hiding this comment

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

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)

o := newServiceOptions()
o.seccomp = "testdata/service-context-write-raw.golden"

privileges, err := o.ToPrivileges(flags)
assert.ErrorContains(t, err, "json")
assert.Check(t, is.Nil(privileges))
}

// TestToPrivilegesSeccompNotPath tests that if the user provides a valid
// filename but not as a path, we both fail the command (as the argument isn't
// a valid seccomp mode) and hint that the user should provide the path as a
// relative path.
func TestToPrivilegesSeccompNotPath(t *testing.T) {
// change the working directory in this test so that the file with no
// separators is a valid file. This will revert at the end of the test.
// Cannot use this with t.Parallel() tests.
// TODO(dperny): When we get to go 1.24, use t.Chdir instead.
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)
}
Comment on lines +420 to +430
Copy link
Member

Choose a reason for hiding this comment

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

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.

})
flags := newCreateCommand(nil).Flags()
flags.Set("seccomp", "test-seccomp-valid.json")
o := newServiceOptions()
o.seccomp = "test-seccomp-valid.json"

privileges, err := o.ToPrivileges(flags)
assert.ErrorContains(t, err, "unknown seccomp mode")
assert.ErrorContains(t, err, "did you mean")
t.Logf("%s", err)
assert.Check(t, is.Nil(privileges))
}

// TestToPrivilegesSeccompNotPathNotValid is like
// TestToPrivilegesSeccompNotPath except the argument isn't a valid file at
// all, so there's no hint.
func TestToPrivilegesSeccompNotPathNotValid(t *testing.T) {
flags := newCreateCommand(nil).Flags()
flags.Set("seccomp", "test-seccomp-valid.json")
o := newServiceOptions()
o.seccomp = "test-seccomp-valid.json"

privileges, err := o.ToPrivileges(flags)
assert.ErrorContains(t, err, "unknown seccomp mode")
t.Logf("%s", err)
assert.Check(t, is.Nil(privileges))
assert.Check(t, !strings.Contains(err.Error(), "did you mean"))
}

func TestToPrivilegesNoNewPrivileges(t *testing.T) {
flags := newCreateCommand(nil).Flags()
flags.Set("no-new-privileges", "true")
o := newServiceOptions()
o.noNewPrivileges = true

privileges, err := o.ToPrivileges(flags)
assert.NilError(t, err)
assert.Check(t, is.DeepEqual(privileges, &swarm.Privileges{NoNewPrivileges: true}))
}
3 changes: 3 additions & 0 deletions cli/command/service/testdata/test-seccomp-valid.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
{
"json": "you betcha"
}
3 changes: 3 additions & 0 deletions docs/reference/commandline/service_create.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ Create a new service

| Name | Type | Default | Description |
|:----------------------------------------------------|:------------------|:-------------|:----------------------------------------------------------------------------------------------------|
| `--apparmor` | `string` | | AppArmor mode (`default` or `disabled` |
| `--cap-add` | `list` | | Add Linux capabilities |
| `--cap-drop` | `list` | | Drop Linux capabilities |
| [`--config`](#config) | `config` | | Specify configurations to expose to the service |
Expand Down Expand Up @@ -45,6 +46,7 @@ Create a new service
| `--name` | `string` | | Service name |
| [`--network`](#network) | `network` | | Network attachments |
| `--no-healthcheck` | `bool` | | Disable any container-specified HEALTHCHECK |
| `--no-new-privileges` | `bool` | | Disable container processes from gaining new privileges |
| `--no-resolve-image` | `bool` | | Do not query the registry to resolve image digest and supported platforms |
| `--oom-score-adj` | `int64` | `0` | Tune host's OOM preferences (-1000 to 1000) |
| [`--placement-pref`](#placement-pref) | `pref` | | Add a placement preference |
Expand All @@ -65,6 +67,7 @@ Create a new service
| `--rollback-monitor` | `duration` | `0s` | Duration after each task rollback to monitor for failure (ns\|us\|ms\|s\|m\|h) (default 5s) |
| `--rollback-order` | `string` | | Rollback order (`start-first`, `stop-first`) (default `stop-first`) |
| `--rollback-parallelism` | `uint64` | `1` | Maximum number of tasks rolled back simultaneously (0 to roll back all at once) |
| `--seccomp` | `string` | | Seccomp configuration (`default`, `unconfined`, or a path to a json custom seccomp profile) |
| [`--secret`](#secret) | `secret` | | Specify secrets to expose to the service |
| `--stop-grace-period` | `duration` | | Time to wait before force killing a container (ns\|us\|ms\|s\|m\|h) (default 10s) |
| `--stop-signal` | `string` | | Signal to stop the container |
Expand Down
Loading
Loading