From 0db61ff6da4be7dfed636dce2805de1ab2cb80ef Mon Sep 17 00:00:00 2001 From: Olli Janatuinen Date: Mon, 9 Sep 2019 20:24:51 +0300 Subject: [PATCH 1/4] stack: Support cap_add and cap_drop on services Signed-off-by: Olli Janatuinen Signed-off-by: Albin Kerouanton --- cli/compose/convert/service.go | 2 ++ cli/compose/convert/service_test.go | 26 ++++++++++++++++++++++++++ cli/compose/types/types.go | 2 -- 3 files changed, 28 insertions(+), 2 deletions(-) diff --git a/cli/compose/convert/service.go b/cli/compose/convert/service.go index d7f64d1863..7bcce00217 100644 --- a/cli/compose/convert/service.go +++ b/cli/compose/convert/service.go @@ -147,6 +147,8 @@ func Service( Isolation: container.Isolation(service.Isolation), Init: service.Init, Sysctls: service.Sysctls, + CapabilityAdd: service.CapAdd, + CapabilityDrop: service.CapDrop, }, LogDriver: logDriver, Resources: resources, diff --git a/cli/compose/convert/service_test.go b/cli/compose/convert/service_test.go index 72e27f2f06..b7363a07fa 100644 --- a/cli/compose/convert/service_test.go +++ b/cli/compose/convert/service_test.go @@ -623,3 +623,29 @@ func TestConvertUpdateConfigParallelism(t *testing.T) { }) assert.Check(t, is.Equal(parallel, updateConfig.Parallelism)) } + +func TestConvertServiceCapAddAndCapDrop(t *testing.T) { + // test default behavior + result, err := Service("1.41", Namespace{name: "foo"}, composetypes.ServiceConfig{}, nil, nil, nil, nil) + assert.NilError(t, err) + assert.Check(t, is.DeepEqual(result.TaskTemplate.ContainerSpec.CapabilityAdd, []string(nil))) + assert.Check(t, is.DeepEqual(result.TaskTemplate.ContainerSpec.CapabilityDrop, []string(nil))) + + // with some values + service := composetypes.ServiceConfig{ + CapAdd: []string{ + "SYS_NICE", + "CAP_NET_ADMIN", + }, + CapDrop: []string{ + "CHOWN", + "DAC_OVERRIDE", + "CAP_FSETID", + "CAP_FOWNER", + }, + } + result, err = Service("1.41", Namespace{name: "foo"}, service, nil, nil, nil, nil) + assert.NilError(t, err) + assert.Check(t, is.DeepEqual(result.TaskTemplate.ContainerSpec.CapabilityAdd, service.CapAdd)) + assert.Check(t, is.DeepEqual(result.TaskTemplate.ContainerSpec.CapabilityDrop, service.CapDrop)) +} diff --git a/cli/compose/types/types.go b/cli/compose/types/types.go index d490d2122c..fd748d62b2 100644 --- a/cli/compose/types/types.go +++ b/cli/compose/types/types.go @@ -9,8 +9,6 @@ import ( // UnsupportedProperties not yet supported by this implementation of the compose file var UnsupportedProperties = []string{ "build", - "cap_add", - "cap_drop", "cgroupns_mode", "cgroup_parent", "devices", From c6ec4e081ec45bc7418c34cb448dd61427c2181b Mon Sep 17 00:00:00 2001 From: Albin Kerouanton Date: Wed, 29 Jul 2020 14:35:51 +0200 Subject: [PATCH 2/4] service: Add --cap-add & --cap-drop to service cmds Signed-off-by: Albin Kerouanton --- cli/command/service/formatter.go | 29 ++++++++++++ cli/command/service/opts.go | 12 +++++ cli/command/service/update.go | 39 ++++++++++++++++ cli/command/service/update_test.go | 71 ++++++++++++++++++++++++++++++ 4 files changed, 151 insertions(+) diff --git a/cli/command/service/formatter.go b/cli/command/service/formatter.go index 6d30be9427..57d0d08766 100644 --- a/cli/command/service/formatter.go +++ b/cli/command/service/formatter.go @@ -97,6 +97,15 @@ ContainerSpec: {{- if .ContainerUser }} User: {{ .ContainerUser }} {{- end }} +{{- if .HasCapabilities }} +Capabilities: +{{- if .HasCapabilityAdd }} + Add: {{ .CapabilityAdd }} +{{- end }} +{{- if .HasCapabilityDrop }} + Drop: {{ .CapabilityDrop }} +{{- end }} +{{- end }} {{- if .ContainerSysCtls }} SysCtls: {{- range $k, $v := .ContainerSysCtls }} @@ -532,6 +541,26 @@ func (ctx *serviceInspectContext) Ports() []swarm.PortConfig { return ctx.Service.Endpoint.Ports } +func (ctx *serviceInspectContext) HasCapabilities() bool { + return len(ctx.Service.Spec.TaskTemplate.ContainerSpec.CapabilityAdd) > 0 || len(ctx.Service.Spec.TaskTemplate.ContainerSpec.CapabilityDrop) > 0 +} + +func (ctx *serviceInspectContext) HasCapabilityAdd() bool { + return len(ctx.Service.Spec.TaskTemplate.ContainerSpec.CapabilityAdd) > 0 +} + +func (ctx *serviceInspectContext) HasCapabilityDrop() bool { + return len(ctx.Service.Spec.TaskTemplate.ContainerSpec.CapabilityDrop) > 0 +} + +func (ctx *serviceInspectContext) CapabilityAdd() string { + return strings.Join(ctx.Service.Spec.TaskTemplate.ContainerSpec.CapabilityAdd, ", ") +} + +func (ctx *serviceInspectContext) CapabilityDrop() string { + return strings.Join(ctx.Service.Spec.TaskTemplate.ContainerSpec.CapabilityDrop, ", ") +} + const ( defaultServiceTableFormat = "table {{.ID}}\t{{.Name}}\t{{.Mode}}\t{{.Replicas}}\t{{.Image}}\t{{.Ports}}" diff --git a/cli/command/service/opts.go b/cli/command/service/opts.go index d473c5fcec..9fbd846556 100644 --- a/cli/command/service/opts.go +++ b/cli/command/service/opts.go @@ -506,6 +506,8 @@ type serviceOptions struct { dnsOption opts.ListOpts hosts opts.ListOpts sysctls opts.ListOpts + capAdd opts.ListOpts + capDrop opts.ListOpts resources resourceOptions stopGrace opts.DurationOpt @@ -549,6 +551,8 @@ func newServiceOptions() *serviceOptions { dnsSearch: opts.NewListOpts(opts.ValidateDNSSearch), hosts: opts.NewListOpts(opts.ValidateExtraHost), sysctls: opts.NewListOpts(nil), + capAdd: opts.NewListOpts(nil), + capDrop: opts.NewListOpts(nil), } } @@ -716,6 +720,8 @@ func (options *serviceOptions) ToService(ctx context.Context, apiClient client.N Healthcheck: healthConfig, Isolation: container.Isolation(options.isolation), Sysctls: opts.ConvertKVStringsToMap(options.sysctls.GetAll()), + CapabilityAdd: options.capAdd.GetAll(), + CapabilityDrop: options.capDrop.GetAll(), }, Networks: networks, Resources: resources, @@ -818,6 +824,10 @@ func addServiceFlags(flags *pflag.FlagSet, opts *serviceOptions, defaultFlagValu flags.StringVar(&opts.hostname, flagHostname, "", "Container hostname") flags.SetAnnotation(flagHostname, "version", []string{"1.25"}) flags.Var(&opts.entrypoint, flagEntrypoint, "Overwrite the default ENTRYPOINT of the image") + flags.Var(&opts.capAdd, flagCapAdd, "Add Linux capabilities") + flags.SetAnnotation(flagCapAdd, "version", []string{"1.41"}) + flags.Var(&opts.capDrop, flagCapDrop, "Drop Linux capabilities") + flags.SetAnnotation(flagCapDrop, "version", []string{"1.41"}) flags.Var(&opts.resources.limitCPU, flagLimitCPU, "Limit CPUs") flags.Var(&opts.resources.limitMemBytes, flagLimitMemory, "Limit Memory") @@ -1001,6 +1011,8 @@ const ( flagConfigAdd = "config-add" flagConfigRemove = "config-rm" flagIsolation = "isolation" + flagCapAdd = "cap-add" + flagCapDrop = "cap-drop" ) func validateAPIVersion(c swarm.ServiceSpec, serverAPIVersion string) error { diff --git a/cli/command/service/update.go b/cli/command/service/update.go index 78230e2281..5a7dcd4331 100644 --- a/cli/command/service/update.go +++ b/cli/command/service/update.go @@ -505,6 +505,7 @@ func updateService(ctx context.Context, apiClient client.NetworkAPIClient, flags } updateString(flagStopSignal, &cspec.StopSignal) + updateCapabilities(flags, cspec) return nil } @@ -1349,3 +1350,41 @@ func updateCredSpecConfig(flags *pflag.FlagSet, containerSpec *swarm.ContainerSp containerSpec.Privileges.CredentialSpec = credSpec } } + +func updateCapabilities(flags *pflag.FlagSet, containerSpec *swarm.ContainerSpec) { + var addToRemove, dropToRemove map[string]struct{} + capAdd := containerSpec.CapabilityAdd + capDrop := containerSpec.CapabilityDrop + + // First add the capabilities passed to --cap-add to the list of requested caps + if flags.Changed(flagCapAdd) { + caps := flags.Lookup(flagCapAdd).Value.(*opts.ListOpts).GetAll() + capAdd = append(capAdd, caps...) + + dropToRemove = buildToRemoveSet(flags, flagCapAdd) + } + + // And add the capabilities passed to --cap-drop to the list of dropped caps + if flags.Changed(flagCapDrop) { + caps := flags.Lookup(flagCapDrop).Value.(*opts.ListOpts).GetAll() + capDrop = append(capDrop, caps...) + + addToRemove = buildToRemoveSet(flags, flagCapDrop) + } + + // Then take care of removing caps passed to --cap-drop from the list of requested caps + containerSpec.CapabilityAdd = make([]string, 0, len(capAdd)) + for _, cap := range capAdd { + if _, exists := addToRemove[cap]; !exists { + containerSpec.CapabilityAdd = append(containerSpec.CapabilityAdd, cap) + } + } + + // And remove the caps passed to --cap-add from the list of caps to drop + containerSpec.CapabilityDrop = make([]string, 0, len(capDrop)) + for _, cap := range capDrop { + if _, exists := dropToRemove[cap]; !exists { + containerSpec.CapabilityDrop = append(containerSpec.CapabilityDrop, cap) + } + } +} diff --git a/cli/command/service/update_test.go b/cli/command/service/update_test.go index 7a2511815b..b8e6dccbb4 100644 --- a/cli/command/service/update_test.go +++ b/cli/command/service/update_test.go @@ -1342,3 +1342,74 @@ func TestUpdateCredSpec(t *testing.T) { }) } } + +func TestUpdateCaps(t *testing.T) { + tests := []struct { + // name is the name of the testcase + name string + // flagAdd is the value passed to --cap-add + flagAdd []string + // flagDrop is the value passed to --cap-drop + flagDrop []string + // spec is the original ContainerSpec, before being updated + spec *swarm.ContainerSpec + // expectedAdd is the set of requested caps the ContainerSpec should have once updated + expectedAdd []string + // expectedDrop is the set of dropped caps the ContainerSpec should have once updated + expectedDrop []string + }{ + { + name: "Add new caps", + flagAdd: []string{"NET_ADMIN"}, + flagDrop: []string{}, + spec: &swarm.ContainerSpec{}, + expectedAdd: []string{"NET_ADMIN"}, + expectedDrop: []string{}, + }, + { + name: "Drop new caps", + flagAdd: []string{}, + flagDrop: []string{"CAP_MKNOD"}, + spec: &swarm.ContainerSpec{}, + expectedAdd: []string{}, + expectedDrop: []string{"CAP_MKNOD"}, + }, + { + name: "Add a previously dropped cap", + flagAdd: []string{"NET_ADMIN"}, + flagDrop: []string{}, + spec: &swarm.ContainerSpec{ + CapabilityDrop: []string{"NET_ADMIN"}, + }, + expectedAdd: []string{"NET_ADMIN"}, + expectedDrop: []string{}, + }, + { + name: "Drop a previously requested cap", + flagAdd: []string{}, + flagDrop: []string{"CAP_MKNOD"}, + spec: &swarm.ContainerSpec{ + CapabilityAdd: []string{"CAP_MKNOD"}, + }, + expectedAdd: []string{}, + expectedDrop: []string{"CAP_MKNOD"}, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + flags := newUpdateCommand(nil).Flags() + for _, cap := range tc.flagAdd { + flags.Set(flagCapAdd, cap) + } + for _, cap := range tc.flagDrop { + flags.Set(flagCapDrop, cap) + } + + updateCapabilities(flags, tc.spec) + + assert.DeepEqual(t, tc.spec.CapabilityAdd, tc.expectedAdd) + assert.DeepEqual(t, tc.spec.CapabilityDrop, tc.expectedDrop) + }) + } +} From 190c64b41581029c44162f724a06e60227cb526f Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Tue, 25 Aug 2020 13:03:06 +0200 Subject: [PATCH 3/4] Service cap-add/cap-drop: improve handling of combinations and special "ALL" value When creating and updating services, we need to avoid unneeded service churn. The interaction of separate lists to "add" and "drop" capabilities, a special ("ALL") capability, as well as a "relaxed" format for accepted capabilities (case-insensitive, `CAP_` prefix optional) make this rather involved. This patch updates how we handle `--cap-add` / `--cap-drop` when _creating_ as well as _updating_, with the following rules/assumptions applied: - both existing (service spec) and new (values passed through flags or in the compose-file) are normalized and de-duplicated before use. - the special "ALL" capability is equivalent to "all capabilities" and taken into account when normalizing capabilities. Combining "ALL" capabilities and other capabilities is therefore equivalent to just specifying "ALL". - adding capabilities takes precedence over dropping, which means that if a capability is both set to be "dropped" and to be "added", it is removed from the list to "drop". - the final lists should be sorted and normalized to reduce service churn - no validation of capabilities is handled by the client. Validation is delegated to the daemon/server. When deploying a service using a docker-compose file, the docker-compose file is *mostly* handled as being "declarative". However, many of the issues outlined above also apply to compose-files, so similar handling is applied to compose files as well to prevent service churn. Signed-off-by: Sebastiaan van Stijn --- cli/command/service/opts.go | 6 +- cli/command/service/update.go | 86 +++++++--- cli/command/service/update_test.go | 158 ++++++++++++++++--- cli/compose/convert/service.go | 6 +- cli/compose/convert/service_test.go | 67 +++++--- contrib/completion/bash/docker | 10 ++ contrib/completion/zsh/_docker | 2 + docs/reference/commandline/service_create.md | 2 + docs/reference/commandline/service_update.md | 2 + opts/capabilities.go | 78 +++++++++ opts/capabilities_test.go | 119 ++++++++++++++ 11 files changed, 466 insertions(+), 70 deletions(-) create mode 100644 opts/capabilities.go create mode 100644 opts/capabilities_test.go diff --git a/cli/command/service/opts.go b/cli/command/service/opts.go index 9fbd846556..053a76ea5e 100644 --- a/cli/command/service/opts.go +++ b/cli/command/service/opts.go @@ -689,6 +689,8 @@ func (options *serviceOptions) ToService(ctx context.Context, apiClient client.N return service, err } + capAdd, capDrop := opts.EffectiveCapAddCapDrop(options.capAdd.GetAll(), options.capDrop.GetAll()) + service = swarm.ServiceSpec{ Annotations: swarm.Annotations{ Name: options.name, @@ -720,8 +722,8 @@ func (options *serviceOptions) ToService(ctx context.Context, apiClient client.N Healthcheck: healthConfig, Isolation: container.Isolation(options.isolation), Sysctls: opts.ConvertKVStringsToMap(options.sysctls.GetAll()), - CapabilityAdd: options.capAdd.GetAll(), - CapabilityDrop: options.capDrop.GetAll(), + CapabilityAdd: capAdd, + CapabilityDrop: capDrop, }, Networks: networks, Resources: resources, diff --git a/cli/command/service/update.go b/cli/command/service/update.go index 5a7dcd4331..d7f7d95bd9 100644 --- a/cli/command/service/update.go +++ b/cli/command/service/update.go @@ -505,7 +505,10 @@ func updateService(ctx context.Context, apiClient client.NetworkAPIClient, flags } updateString(flagStopSignal, &cspec.StopSignal) - updateCapabilities(flags, cspec) + + if anyChanged(flags, flagCapAdd, flagCapDrop) { + updateCapabilities(flags, cspec) + } return nil } @@ -1351,40 +1354,71 @@ func updateCredSpecConfig(flags *pflag.FlagSet, containerSpec *swarm.ContainerSp } } +// updateCapabilities calculates the list of capabilities to "drop" and to "add" +// after applying the capabilities passed through `--cap-add` and `--cap-drop` +// to the existing list of added/dropped capabilities in the service spec. +// +// Adding capabilities takes precedence over "dropping" the same capability, so +// if both `--cap-add` and `--cap-drop` are specifying the same capability, the +// `--cap-drop` is ignored. +// +// Capabilities to "drop" are removed from the existing list of "added" +// capabilities, and vice-versa (capabilities to "add" are removed from the existing +// list of capabilities to "drop"). +// +// Capabilities are normalized, sorted, and duplicates are removed to prevent +// service tasks from being updated if no changes are made. If a list has the "ALL" +// capability set, then any other capability is removed from that list. func updateCapabilities(flags *pflag.FlagSet, containerSpec *swarm.ContainerSpec) { - var addToRemove, dropToRemove map[string]struct{} - capAdd := containerSpec.CapabilityAdd - capDrop := containerSpec.CapabilityDrop + var ( + toAdd, toDrop map[string]bool - // First add the capabilities passed to --cap-add to the list of requested caps + capDrop = opts.CapabilitiesMap(containerSpec.CapabilityDrop) + capAdd = opts.CapabilitiesMap(containerSpec.CapabilityAdd) + ) if flags.Changed(flagCapAdd) { - caps := flags.Lookup(flagCapAdd).Value.(*opts.ListOpts).GetAll() - capAdd = append(capAdd, caps...) - - dropToRemove = buildToRemoveSet(flags, flagCapAdd) + toAdd = opts.CapabilitiesMap(flags.Lookup(flagCapAdd).Value.(*opts.ListOpts).GetAll()) } - - // And add the capabilities passed to --cap-drop to the list of dropped caps if flags.Changed(flagCapDrop) { - caps := flags.Lookup(flagCapDrop).Value.(*opts.ListOpts).GetAll() - capDrop = append(capDrop, caps...) - - addToRemove = buildToRemoveSet(flags, flagCapDrop) + toDrop = opts.CapabilitiesMap(flags.Lookup(flagCapDrop).Value.(*opts.ListOpts).GetAll()) } - // Then take care of removing caps passed to --cap-drop from the list of requested caps - containerSpec.CapabilityAdd = make([]string, 0, len(capAdd)) - for _, cap := range capAdd { - if _, exists := addToRemove[cap]; !exists { - containerSpec.CapabilityAdd = append(containerSpec.CapabilityAdd, cap) + // First remove the capabilities to "drop" from the service's exiting + // list of capabilities to "add". If a capability is both added and dropped + // on update, then "adding" takes precedence. + for c := range toDrop { + if !toAdd[c] { + delete(capAdd, c) + capDrop[c] = true } } - // And remove the caps passed to --cap-add from the list of caps to drop - containerSpec.CapabilityDrop = make([]string, 0, len(capDrop)) - for _, cap := range capDrop { - if _, exists := dropToRemove[cap]; !exists { - containerSpec.CapabilityDrop = append(containerSpec.CapabilityDrop, cap) - } + // And remove the capabilities we're "adding" from the service's existing + // list of capabilities to "drop". + // + // "Adding" capabilities takes precedence over "dropping" them, so if a + // capability is set both as "add" and "drop", remove the capability from + // the service's list of dropped capabilities (if present). + for c := range toAdd { + delete(capDrop, c) + capAdd[c] = true } + + // Now that the service's existing lists are updated, apply the new + // capabilities to add/drop to both lists. Sort the lists to prevent + // unneeded updates to service-tasks. + containerSpec.CapabilityDrop = capsList(capDrop) + containerSpec.CapabilityAdd = capsList(capAdd) +} + +func capsList(caps map[string]bool) []string { + if caps[opts.AllCapabilities] { + return []string{opts.AllCapabilities} + } + var out []string + for c := range caps { + out = append(out, c) + } + sort.Strings(out) + return out } diff --git a/cli/command/service/update_test.go b/cli/command/service/update_test.go index b8e6dccbb4..c2a2357c9f 100644 --- a/cli/command/service/update_test.go +++ b/cli/command/service/update_test.go @@ -1358,52 +1358,170 @@ func TestUpdateCaps(t *testing.T) { // expectedDrop is the set of dropped caps the ContainerSpec should have once updated expectedDrop []string }{ + { + // Note that this won't be run as updateCapabilities is gated by anyChanged(flags, flagCapAdd, flagCapDrop) + name: "Empty spec, no updates", + spec: &swarm.ContainerSpec{}, + }, + { + // Note that this won't be run as updateCapabilities is gated by anyChanged(flags, flagCapAdd, flagCapDrop) + name: "No updates", + spec: &swarm.ContainerSpec{ + CapabilityAdd: []string{"CAP_MOUNT", "CAP_NET_ADMIN"}, + CapabilityDrop: []string{"CAP_CHOWN", "CAP_SYS_ADMIN"}, + }, + expectedAdd: []string{"CAP_MOUNT", "CAP_NET_ADMIN"}, + expectedDrop: []string{"CAP_CHOWN", "CAP_SYS_ADMIN"}, + }, + { + // Note that this won't be run as updateCapabilities is gated by anyChanged(flags, flagCapAdd, flagCapDrop) + name: "Empty updates", + flagAdd: []string{}, + flagDrop: []string{}, + spec: &swarm.ContainerSpec{ + CapabilityAdd: []string{"CAP_MOUNT", "CAP_NET_ADMIN"}, + CapabilityDrop: []string{"CAP_CHOWN", "CAP_SYS_ADMIN"}, + }, + expectedAdd: []string{"CAP_MOUNT", "CAP_NET_ADMIN"}, + expectedDrop: []string{"CAP_CHOWN", "CAP_SYS_ADMIN"}, + }, + { + // Note that this won't be run as updateCapabilities is gated by anyChanged(flags, flagCapAdd, flagCapDrop) + name: "Normalize cap-add only", + flagAdd: []string{}, + flagDrop: []string{}, + spec: &swarm.ContainerSpec{ + CapabilityAdd: []string{"ALL", "CAP_MOUNT", "CAP_NET_ADMIN"}, + }, + expectedAdd: []string{"ALL"}, + expectedDrop: nil, + }, + { + // Note that this won't be run as updateCapabilities is gated by anyChanged(flags, flagCapAdd, flagCapDrop) + name: "Normalize cap-drop only", + spec: &swarm.ContainerSpec{ + CapabilityDrop: []string{"ALL", "CAP_MOUNT", "CAP_NET_ADMIN"}, + }, + expectedDrop: []string{"ALL"}, + }, { name: "Add new caps", - flagAdd: []string{"NET_ADMIN"}, + flagAdd: []string{"CAP_NET_ADMIN"}, flagDrop: []string{}, spec: &swarm.ContainerSpec{}, - expectedAdd: []string{"NET_ADMIN"}, - expectedDrop: []string{}, + expectedAdd: []string{"CAP_NET_ADMIN"}, + expectedDrop: nil, }, { name: "Drop new caps", flagAdd: []string{}, - flagDrop: []string{"CAP_MKNOD"}, + flagDrop: []string{"CAP_NET_ADMIN"}, spec: &swarm.ContainerSpec{}, - expectedAdd: []string{}, - expectedDrop: []string{"CAP_MKNOD"}, + expectedAdd: nil, + expectedDrop: []string{"CAP_NET_ADMIN"}, }, { name: "Add a previously dropped cap", - flagAdd: []string{"NET_ADMIN"}, + flagAdd: []string{"CAP_NET_ADMIN"}, flagDrop: []string{}, spec: &swarm.ContainerSpec{ - CapabilityDrop: []string{"NET_ADMIN"}, + CapabilityDrop: []string{"CAP_NET_ADMIN"}, }, - expectedAdd: []string{"NET_ADMIN"}, - expectedDrop: []string{}, + expectedAdd: []string{"CAP_NET_ADMIN"}, + expectedDrop: nil, }, { - name: "Drop a previously requested cap", - flagAdd: []string{}, - flagDrop: []string{"CAP_MKNOD"}, + name: "Drop a previously requested cap, and add a new one", + flagAdd: []string{"CAP_CHOWN"}, + flagDrop: []string{"CAP_NET_ADMIN"}, spec: &swarm.ContainerSpec{ - CapabilityAdd: []string{"CAP_MKNOD"}, + CapabilityAdd: []string{"CAP_NET_ADMIN"}, }, - expectedAdd: []string{}, - expectedDrop: []string{"CAP_MKNOD"}, + expectedDrop: []string{"CAP_NET_ADMIN"}, + expectedAdd: []string{"CAP_CHOWN"}, + }, + { + name: "Add caps to service that has ALL caps has no effect", + flagAdd: []string{"CAP_NET_ADMIN"}, + spec: &swarm.ContainerSpec{ + CapabilityAdd: []string{"ALL"}, + }, + expectedAdd: []string{"ALL"}, + expectedDrop: nil, + }, + { + name: "Add takes precedence on empty spec", + flagAdd: []string{"CAP_NET_ADMIN"}, + flagDrop: []string{"CAP_NET_ADMIN"}, + spec: &swarm.ContainerSpec{}, + expectedAdd: []string{"CAP_NET_ADMIN"}, + expectedDrop: nil, + }, + { + name: "Add takes precedence on existing spec", + flagAdd: []string{"CAP_NET_ADMIN"}, + flagDrop: []string{"CAP_NET_ADMIN"}, + spec: &swarm.ContainerSpec{ + CapabilityAdd: []string{"CAP_NET_ADMIN"}, + CapabilityDrop: []string{"CAP_NET_ADMIN"}, + }, + expectedAdd: []string{"CAP_NET_ADMIN"}, + expectedDrop: nil, + }, + { + name: "Drop all, and add new caps", + flagAdd: []string{"CAP_CHOWN"}, + flagDrop: []string{"ALL"}, + spec: &swarm.ContainerSpec{ + CapabilityAdd: []string{"CAP_NET_ADMIN", "CAP_MOUNT"}, + CapabilityDrop: []string{"CAP_NET_ADMIN", "CAP_MOUNT"}, + }, + expectedAdd: []string{"CAP_CHOWN", "CAP_MOUNT", "CAP_NET_ADMIN"}, + expectedDrop: []string{"ALL"}, + }, + { + name: "Add all caps", + flagAdd: []string{"ALL"}, + flagDrop: []string{"CAP_NET_ADMIN", "CAP_SYS_ADMIN"}, + spec: &swarm.ContainerSpec{ + CapabilityAdd: []string{"CAP_NET_ADMIN"}, + CapabilityDrop: []string{"CAP_CHOWN"}, + }, + expectedAdd: []string{"ALL"}, + expectedDrop: []string{"CAP_CHOWN", "CAP_NET_ADMIN", "CAP_SYS_ADMIN"}, + }, + { + name: "Drop all, and add all", + flagAdd: []string{"ALL"}, + flagDrop: []string{"ALL"}, + spec: &swarm.ContainerSpec{ + CapabilityAdd: []string{"CAP_NET_ADMIN"}, + CapabilityDrop: []string{"CAP_CHOWN"}, + }, + expectedAdd: []string{"ALL"}, + expectedDrop: []string{"CAP_CHOWN"}, + }, + { + name: "Caps are normalized and sorted", + flagAdd: []string{"bbb", "aaa", "cAp_bBb", "cAp_aAa"}, + flagDrop: []string{"zzz", "yyy", "cAp_yYy", "cAp_yYy"}, + spec: &swarm.ContainerSpec{ + CapabilityAdd: []string{"ccc", "CAP_DDD"}, + CapabilityDrop: []string{"www", "CAP_XXX"}, + }, + expectedAdd: []string{"CAP_AAA", "CAP_BBB", "CAP_CCC", "CAP_DDD"}, + expectedDrop: []string{"CAP_WWW", "CAP_XXX", "CAP_YYY", "CAP_ZZZ"}, }, } for _, tc := range tests { t.Run(tc.name, func(t *testing.T) { flags := newUpdateCommand(nil).Flags() - for _, cap := range tc.flagAdd { - flags.Set(flagCapAdd, cap) + for _, c := range tc.flagAdd { + _ = flags.Set(flagCapAdd, c) } - for _, cap := range tc.flagDrop { - flags.Set(flagCapDrop, cap) + for _, c := range tc.flagDrop { + _ = flags.Set(flagCapDrop, c) } updateCapabilities(flags, tc.spec) diff --git a/cli/compose/convert/service.go b/cli/compose/convert/service.go index 7bcce00217..1b15fa36ca 100644 --- a/cli/compose/convert/service.go +++ b/cli/compose/convert/service.go @@ -117,6 +117,8 @@ func Service( } } + capAdd, capDrop := opts.EffectiveCapAddCapDrop(service.CapAdd, service.CapDrop) + serviceSpec := swarm.ServiceSpec{ Annotations: swarm.Annotations{ Name: name, @@ -147,8 +149,8 @@ func Service( Isolation: container.Isolation(service.Isolation), Init: service.Init, Sysctls: service.Sysctls, - CapabilityAdd: service.CapAdd, - CapabilityDrop: service.CapDrop, + CapabilityAdd: capAdd, + CapabilityDrop: capDrop, }, LogDriver: logDriver, Resources: resources, diff --git a/cli/compose/convert/service_test.go b/cli/compose/convert/service_test.go index b7363a07fa..0607f3957b 100644 --- a/cli/compose/convert/service_test.go +++ b/cli/compose/convert/service_test.go @@ -625,27 +625,54 @@ func TestConvertUpdateConfigParallelism(t *testing.T) { } func TestConvertServiceCapAddAndCapDrop(t *testing.T) { - // test default behavior - result, err := Service("1.41", Namespace{name: "foo"}, composetypes.ServiceConfig{}, nil, nil, nil, nil) - assert.NilError(t, err) - assert.Check(t, is.DeepEqual(result.TaskTemplate.ContainerSpec.CapabilityAdd, []string(nil))) - assert.Check(t, is.DeepEqual(result.TaskTemplate.ContainerSpec.CapabilityDrop, []string(nil))) - - // with some values - service := composetypes.ServiceConfig{ - CapAdd: []string{ - "SYS_NICE", - "CAP_NET_ADMIN", + tests := []struct { + title string + in, out composetypes.ServiceConfig + }{ + { + title: "default behavior", }, - CapDrop: []string{ - "CHOWN", - "DAC_OVERRIDE", - "CAP_FSETID", - "CAP_FOWNER", + { + title: "some values", + in: composetypes.ServiceConfig{ + CapAdd: []string{"SYS_NICE", "CAP_NET_ADMIN"}, + CapDrop: []string{"CHOWN", "CAP_NET_ADMIN", "DAC_OVERRIDE", "CAP_FSETID", "CAP_FOWNER"}, + }, + out: composetypes.ServiceConfig{ + CapAdd: []string{"CAP_NET_ADMIN", "CAP_SYS_NICE"}, + CapDrop: []string{"CAP_CHOWN", "CAP_DAC_OVERRIDE", "CAP_FOWNER", "CAP_FSETID"}, + }, + }, + { + title: "adding ALL capabilities", + in: composetypes.ServiceConfig{ + CapAdd: []string{"ALL", "CAP_NET_ADMIN"}, + CapDrop: []string{"CHOWN", "CAP_NET_ADMIN", "DAC_OVERRIDE", "CAP_FSETID", "CAP_FOWNER"}, + }, + out: composetypes.ServiceConfig{ + CapAdd: []string{"ALL"}, + CapDrop: []string{"CAP_CHOWN", "CAP_DAC_OVERRIDE", "CAP_FOWNER", "CAP_FSETID", "CAP_NET_ADMIN"}, + }, + }, + { + title: "dropping ALL capabilities", + in: composetypes.ServiceConfig{ + CapAdd: []string{"CHOWN", "CAP_NET_ADMIN", "DAC_OVERRIDE", "CAP_FSETID", "CAP_FOWNER"}, + CapDrop: []string{"ALL", "CAP_NET_ADMIN", "CAP_FOO"}, + }, + out: composetypes.ServiceConfig{ + CapAdd: []string{"CAP_CHOWN", "CAP_DAC_OVERRIDE", "CAP_FOWNER", "CAP_FSETID", "CAP_NET_ADMIN"}, + CapDrop: []string{"ALL"}, + }, }, } - result, err = Service("1.41", Namespace{name: "foo"}, service, nil, nil, nil, nil) - assert.NilError(t, err) - assert.Check(t, is.DeepEqual(result.TaskTemplate.ContainerSpec.CapabilityAdd, service.CapAdd)) - assert.Check(t, is.DeepEqual(result.TaskTemplate.ContainerSpec.CapabilityDrop, service.CapDrop)) + for _, tc := range tests { + tc := tc + t.Run(tc.title, func(t *testing.T) { + result, err := Service("1.41", Namespace{name: "foo"}, tc.in, nil, nil, nil, nil) + assert.NilError(t, err) + assert.Check(t, is.DeepEqual(result.TaskTemplate.ContainerSpec.CapabilityAdd, tc.out.CapAdd)) + assert.Check(t, is.DeepEqual(result.TaskTemplate.ContainerSpec.CapabilityDrop, tc.out.CapDrop)) + }) + } } diff --git a/contrib/completion/bash/docker b/contrib/completion/bash/docker index 38f6491b97..75b703801e 100644 --- a/contrib/completion/bash/docker +++ b/contrib/completion/bash/docker @@ -3672,6 +3672,8 @@ _docker_service_update() { # and `docker service update` _docker_service_update_and_create() { local options_with_args=" + --cap-add + --cap-drop --endpoint-mode --entrypoint --health-cmd @@ -3830,6 +3832,14 @@ _docker_service_update_and_create() { esac case "$prev" in + --cap-add) + __docker_complete_capabilities_addable + return + ;; + --cap-drop) + __docker_complete_capabilities_droppable + return + ;; --config|--config-add|--config-rm) __docker_complete_configs return diff --git a/contrib/completion/zsh/_docker b/contrib/completion/zsh/_docker index 40b2796bcb..4d28ac48e8 100644 --- a/contrib/completion/zsh/_docker +++ b/contrib/completion/zsh/_docker @@ -1961,6 +1961,8 @@ __docker_service_subcommand() { opts_help=("(: -)--help[Print usage]") opts_create_update=( + "($help)*--cap-add=[Add Linux capabilities]:capability: " + "($help)*--cap-drop=[Drop Linux capabilities]:capability: " "($help)*--constraint=[Placement constraints]:constraint: " "($help)--endpoint-mode=[Placement constraints]:mode:(dnsrr vip)" "($help)*"{-e=,--env=}"[Set environment variables]:env: " diff --git a/docs/reference/commandline/service_create.md b/docs/reference/commandline/service_create.md index bcd877f5a1..13e40d50d3 100644 --- a/docs/reference/commandline/service_create.md +++ b/docs/reference/commandline/service_create.md @@ -12,6 +12,8 @@ Usage: docker service create [OPTIONS] IMAGE [COMMAND] [ARG...] Create a new service Options: + --cap-add list Add Linux capabilities + --cap-drop list Drop Linux capabilities --config config Specify configurations to expose to the service --constraint list Placement constraints --container-label list Container labels diff --git a/docs/reference/commandline/service_update.md b/docs/reference/commandline/service_update.md index 440bb2341d..fc5bd424cd 100644 --- a/docs/reference/commandline/service_update.md +++ b/docs/reference/commandline/service_update.md @@ -13,6 +13,8 @@ Update a service Options: --args command Service command args + --cap-add list Add Linux capabilities + --cap-drop list Drop Linux capabilities --config-add config Add or update a config file on a service --config-rm list Remove a configuration file --constraint-add list Add or update a placement constraint diff --git a/opts/capabilities.go b/opts/capabilities.go new file mode 100644 index 0000000000..1a2e19a89d --- /dev/null +++ b/opts/capabilities.go @@ -0,0 +1,78 @@ +package opts + +import ( + "sort" + "strings" +) + +const ( + // AllCapabilities is a special value to add or drop all capabilities + AllCapabilities = "ALL" +) + +// NormalizeCapability normalizes a capability by upper-casing, trimming white space +// and adding a CAP_ prefix (if not yet present). This function also accepts the +// "ALL" magic-value, as used by CapAdd/CapDrop. +// +// This function only handles rudimentary formatting; no validation is performed, +// as the list of available capabilities can be updated over time, thus should be +// handled by the daemon. +func NormalizeCapability(cap string) string { + cap = strings.ToUpper(strings.TrimSpace(cap)) + if cap == AllCapabilities { + return cap + } + if !strings.HasPrefix(cap, "CAP_") { + cap = "CAP_" + cap + } + return cap +} + +// CapabilitiesMap normalizes the given capabilities and converts them to a map. +func CapabilitiesMap(caps []string) map[string]bool { + normalized := make(map[string]bool) + for _, c := range caps { + normalized[NormalizeCapability(c)] = true + } + return normalized +} + +// EffectiveCapAddCapDrop normalizes and sorts capabilities to "add" and "drop", +// and returns the effective capabilities to include in both. +// +// "CapAdd" takes precedence over "CapDrop", so capabilities included in both +// lists are removed from the list of capabilities to drop. The special "ALL" +// capability is also taken into account. +// +// Duplicates are removed, and the resulting lists are sorted. +func EffectiveCapAddCapDrop(add, drop []string) (capAdd, capDrop []string) { + var ( + addCaps = CapabilitiesMap(add) + dropCaps = CapabilitiesMap(drop) + ) + + if addCaps[AllCapabilities] { + // Special case: "ALL capabilities" trumps any other capability added. + addCaps = map[string]bool{AllCapabilities: true} + } + if dropCaps[AllCapabilities] { + // Special case: "ALL capabilities" trumps any other capability added. + dropCaps = map[string]bool{AllCapabilities: true} + } + for c := range dropCaps { + if addCaps[c] { + // Adding a capability takes precedence, so skip dropping + continue + } + capDrop = append(capDrop, c) + } + + for c := range addCaps { + capAdd = append(capAdd, c) + } + + sort.Strings(capAdd) + sort.Strings(capDrop) + + return capAdd, capDrop +} diff --git a/opts/capabilities_test.go b/opts/capabilities_test.go new file mode 100644 index 0000000000..cb61b79396 --- /dev/null +++ b/opts/capabilities_test.go @@ -0,0 +1,119 @@ +package opts + +import ( + "strconv" + "testing" + + "gotest.tools/v3/assert" +) + +func TestNormalizeCapability(t *testing.T) { + tests := []struct{ in, out string }{ + {in: "ALL", out: "ALL"}, + {in: "FOO", out: "CAP_FOO"}, + {in: "CAP_FOO", out: "CAP_FOO"}, + {in: "CAPFOO", out: "CAP_CAPFOO"}, + + // case-insensitive handling + {in: "aLl", out: "ALL"}, + {in: "foO", out: "CAP_FOO"}, + {in: "cAp_foO", out: "CAP_FOO"}, + + // white space handling. strictly, these could be considered "invalid", + // but are a likely situation, so handling these for now. + {in: " ALL ", out: "ALL"}, + {in: " FOO ", out: "CAP_FOO"}, + {in: " CAP_FOO ", out: "CAP_FOO"}, + {in: " ALL ", out: "ALL"}, + {in: " FOO ", out: "CAP_FOO"}, + {in: " CAP_FOO ", out: "CAP_FOO"}, + + // weird values: no validation takes place currently, so these + // are handled same as values above; we could consider not accepting + // these in future + {in: "SOME CAP", out: "CAP_SOME CAP"}, + {in: "_FOO", out: "CAP__FOO"}, + } + + for _, tc := range tests { + tc := tc + t.Run(tc.in, func(t *testing.T) { + assert.Equal(t, NormalizeCapability(tc.in), tc.out) + }) + } +} + +func TestEffectiveCapAddCapDrop(t *testing.T) { + type caps struct { + add, drop []string + } + + tests := []struct { + in, out caps + }{ + { + in: caps{ + add: []string{"one", "two"}, + drop: []string{"one", "two"}, + }, + out: caps{ + add: []string{"CAP_ONE", "CAP_TWO"}, + }, + }, + { + in: caps{ + add: []string{"CAP_ONE", "cap_one", "CAP_TWO"}, + drop: []string{"one", "cap_two"}, + }, + out: caps{ + add: []string{"CAP_ONE", "CAP_TWO"}, + }, + }, + { + in: caps{ + add: []string{"CAP_ONE", "CAP_TWO"}, + drop: []string{"CAP_ONE", "CAP_THREE"}, + }, + out: caps{ + add: []string{"CAP_ONE", "CAP_TWO"}, + drop: []string{"CAP_THREE"}, + }, + }, + { + in: caps{ + add: []string{"ALL"}, + drop: []string{"CAP_ONE", "CAP_TWO"}, + }, + out: caps{ + add: []string{"ALL"}, + drop: []string{"CAP_ONE", "CAP_TWO"}, + }, + }, + { + in: caps{ + add: []string{"ALL", "CAP_ONE"}, + }, + out: caps{ + add: []string{"ALL"}, + }, + }, + { + in: caps{ + drop: []string{"ALL", "CAP_ONE"}, + }, + out: caps{ + drop: []string{"ALL"}, + }, + }, + } + + for i, tc := range tests { + tc := tc + t.Run(strconv.Itoa(i), func(t *testing.T) { + add, drop := EffectiveCapAddCapDrop(tc.in.add, tc.in.drop) + assert.DeepEqual(t, add, tc.out.add) + assert.DeepEqual(t, drop, tc.out.drop) + + }) + } +} From 95037299cb02bf2f431949e4d08f44bd05ece8dc Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Tue, 25 Aug 2020 17:28:59 +0200 Subject: [PATCH 4/4] Service cap-add/cap-drop: handle updates as "tri-state" Adding/removing capabilities when updating a service is considered a tri-state; - if the capability was previously "dropped", then remove it from "CapabilityDrop", but do NOT add it to "CapabilityAdd". However, if the capability was not yet in the service's "CapabilityDrop", then simply add it to the service's "CapabilityAdd" - likewise, if the capability was previously "added", then remove it from "CapabilityAdd", but do NOT add it to "CapabilityDrop". If the capability was not yet in the service's "CapabilityAdd", then simply add it to the service's "CapabilityDrop". In other words, given a service with the following: | CapDrop | CapAdd | | -------------- | ------------- | | CAP_SOME_CAP | | When updating the service, and applying `--cap-add CAP_SOME_CAP`, the previously dropped capability is removed: | CapDrop | CapAdd | | -------------- | ------------- | | | | When updating the service a second time, applying `--cap-add CAP_SOME_CAP`, capability is now added: | CapDrop | CapAdd | | -------------- | ------------- | | | CAP_SOME_CAP | Signed-off-by: Sebastiaan van Stijn --- cli/command/service/update.go | 59 ++++++++++++++++++++++++++++-- cli/command/service/update_test.go | 16 ++++++-- 2 files changed, 68 insertions(+), 7 deletions(-) diff --git a/cli/command/service/update.go b/cli/command/service/update.go index d7f7d95bd9..8adcdfa8f2 100644 --- a/cli/command/service/update.go +++ b/cli/command/service/update.go @@ -1369,6 +1369,37 @@ func updateCredSpecConfig(flags *pflag.FlagSet, containerSpec *swarm.ContainerSp // Capabilities are normalized, sorted, and duplicates are removed to prevent // service tasks from being updated if no changes are made. If a list has the "ALL" // capability set, then any other capability is removed from that list. +// +// Adding/removing capabilities when updating a service is handled as a tri-state; +// +// - if the capability was previously "dropped", then remove it from "CapabilityDrop", +// but NOT added to "CapabilityAdd". However, if the capability was not yet in +// the service's "CapabilityDrop", then it's simply added to the service's "CapabilityAdd" +// - likewise, if the capability was previously "added", then it's removed from +// "CapabilityAdd", but NOT added to "CapabilityDrop". If the capability was +// not yet in the service's "CapabilityAdd", then simply add it to the service's +// "CapabilityDrop". +// +// In other words, given a service with the following: +// +// | CapDrop | CapAdd | +// | -------------- | ------------- | +// | CAP_SOME_CAP | | +// +// When updating the service, and applying `--cap-add CAP_SOME_CAP`, the previously +// dropped capability is removed: +// +// | CapDrop | CapAdd | +// | -------------- | ------------- | +// | | | +// +// After updating the service a second time, applying `--cap-add CAP_SOME_CAP`, +// capability is now added: +// +// | CapDrop | CapAdd | +// | -------------- | ------------- | +// | | CAP_SOME_CAP | +// func updateCapabilities(flags *pflag.FlagSet, containerSpec *swarm.ContainerSpec) { var ( toAdd, toDrop map[string]bool @@ -1386,10 +1417,20 @@ func updateCapabilities(flags *pflag.FlagSet, containerSpec *swarm.ContainerSpec // First remove the capabilities to "drop" from the service's exiting // list of capabilities to "add". If a capability is both added and dropped // on update, then "adding" takes precedence. + // + // Dropping a capability when updating a service is considered a tri-state; + // + // - if the capability was previously "added", then remove it from + // "CapabilityAdd", and do NOT add it to "CapabilityDrop" + // - if the capability was not yet in the service's "CapabilityAdd", + // then simply add it to the service's "CapabilityDrop" for c := range toDrop { if !toAdd[c] { - delete(capAdd, c) - capDrop[c] = true + if capAdd[c] { + delete(capAdd, c) + } else { + capDrop[c] = true + } } } @@ -1399,9 +1440,19 @@ func updateCapabilities(flags *pflag.FlagSet, containerSpec *swarm.ContainerSpec // "Adding" capabilities takes precedence over "dropping" them, so if a // capability is set both as "add" and "drop", remove the capability from // the service's list of dropped capabilities (if present). + // + // Adding a capability when updating a service is considered a tri-state; + // + // - if the capability was previously "dropped", then remove it from + // "CapabilityDrop", and do NOT add it to "CapabilityAdd" + // - if the capability was not yet in the service's "CapabilityDrop", + // then simply add it to the service's "CapabilityAdd" for c := range toAdd { - delete(capDrop, c) - capAdd[c] = true + if capDrop[c] { + delete(capDrop, c) + } else { + capAdd[c] = true + } } // Now that the service's existing lists are updated, apply the new diff --git a/cli/command/service/update_test.go b/cli/command/service/update_test.go index c2a2357c9f..cbae2ddd11 100644 --- a/cli/command/service/update_test.go +++ b/cli/command/service/update_test.go @@ -1427,7 +1427,7 @@ func TestUpdateCaps(t *testing.T) { spec: &swarm.ContainerSpec{ CapabilityDrop: []string{"CAP_NET_ADMIN"}, }, - expectedAdd: []string{"CAP_NET_ADMIN"}, + expectedAdd: nil, expectedDrop: nil, }, { @@ -1437,8 +1437,8 @@ func TestUpdateCaps(t *testing.T) { spec: &swarm.ContainerSpec{ CapabilityAdd: []string{"CAP_NET_ADMIN"}, }, - expectedDrop: []string{"CAP_NET_ADMIN"}, expectedAdd: []string{"CAP_CHOWN"}, + expectedDrop: nil, }, { name: "Add caps to service that has ALL caps has no effect", @@ -1449,6 +1449,16 @@ func TestUpdateCaps(t *testing.T) { expectedAdd: []string{"ALL"}, expectedDrop: nil, }, + { + name: "Drop ALL caps, then add new caps to service that has ALL caps", + flagAdd: []string{"CAP_NET_ADMIN"}, + flagDrop: []string{"ALL"}, + spec: &swarm.ContainerSpec{ + CapabilityAdd: []string{"ALL"}, + }, + expectedAdd: []string{"CAP_NET_ADMIN"}, + expectedDrop: nil, + }, { name: "Add takes precedence on empty spec", flagAdd: []string{"CAP_NET_ADMIN"}, @@ -1488,7 +1498,7 @@ func TestUpdateCaps(t *testing.T) { CapabilityDrop: []string{"CAP_CHOWN"}, }, expectedAdd: []string{"ALL"}, - expectedDrop: []string{"CAP_CHOWN", "CAP_NET_ADMIN", "CAP_SYS_ADMIN"}, + expectedDrop: []string{"CAP_CHOWN", "CAP_SYS_ADMIN"}, }, { name: "Drop all, and add all",