From effa09a42f57898427bb55aac23ff646378586e7 Mon Sep 17 00:00:00 2001 From: MatthieuFin Date: Tue, 31 Aug 2021 17:03:25 +0200 Subject: [PATCH 1/6] Add full support for setting securityContext for restic restore container Signed-off-by: MatthieuFin --- go.mod | 1 + pkg/restore/restic_restore_action.go | 18 +-- pkg/util/kube/security_context.go | 10 +- pkg/util/kube/security_context_test.go | 181 +++++++++++++++++++++++-- 4 files changed, 186 insertions(+), 24 deletions(-) diff --git a/go.mod b/go.mod index bfea33926..bcce08eb7 100644 --- a/go.mod +++ b/go.mod @@ -43,6 +43,7 @@ require ( k8s.io/kube-aggregator v0.19.12 sigs.k8s.io/cluster-api v0.3.11-0.20210106212952-b6c1b5b3db3d sigs.k8s.io/controller-runtime v0.7.1-0.20201215171748-096b2e07c091 + sigs.k8s.io/yaml v1.2.0 // indirect ) replace github.com/gogo/protobuf => github.com/gogo/protobuf v1.3.2 diff --git a/pkg/restore/restic_restore_action.go b/pkg/restore/restic_restore_action.go index c20af38c7..f72923efe 100644 --- a/pkg/restore/restic_restore_action.go +++ b/pkg/restore/restic_restore_action.go @@ -139,11 +139,11 @@ func (a *ResticRestoreAction) Execute(input *velero.RestoreItemActionExecuteInpu ) } - runAsRoot, runAsGroup, allowPrivilegeEscalation := getSecurityContext(log, config) + runAsUser, runAsGroup, allowPrivilegeEscalation, secCtx := getSecurityContext(log, config) - securityContext, err := kube.ParseSecurityContext(runAsRoot, runAsGroup, allowPrivilegeEscalation) + securityContext, err := kube.ParseSecurityContext(runAsUser, runAsGroup, allowPrivilegeEscalation, secCtx) if err != nil { - log.Errorf("Using default resource values, couldn't parse resource requirements: %s.", err) + log.Errorf("Using default securityContext values, couldn't parse securityContext requirements: %s.", err) } initContainerBuilder := newResticInitContainerBuilder(image, string(input.Restore.UID)) @@ -245,15 +245,17 @@ func getResourceLimits(log logrus.FieldLogger, config *corev1.ConfigMap) (string return config.Data["cpuLimit"], config.Data["memLimit"] } - -// getSecurityContext extracts securityContext runAsUser, runAsGroup, and allowPrivilegeEscalation from a ConfigMap. -func getSecurityContext(log logrus.FieldLogger, config *corev1.ConfigMap) (string, string, string) { +// getSecurityContext extracts securityContext runAsUser, runAsGroup, allowPrivilegeEscalation, and securityContext from a ConfigMap. +func getSecurityContext(log logrus.FieldLogger, config *corev1.ConfigMap) (string, string, string, string) { if config == nil { log.Debug("No config found for plugin") - return "", "", "" + return "", "", "", "" } - return config.Data["secCtxRunAsUser"], config.Data["secCtxRunAsGroup"], config.Data["secCtxAllowPrivilegeEscalation"] + return config.Data["secCtxRunAsUser"], + config.Data["secCtxRunAsGroup"], + config.Data["secCtxAllowPrivilegeEscalation"], + config.Data["secCtx"] } // TODO eventually this can move to pkg/plugin/framework since it'll be used across multiple diff --git a/pkg/util/kube/security_context.go b/pkg/util/kube/security_context.go index 2011515ec..22c2c4ce8 100644 --- a/pkg/util/kube/security_context.go +++ b/pkg/util/kube/security_context.go @@ -21,9 +21,10 @@ import ( "github.com/pkg/errors" corev1 "k8s.io/api/core/v1" + "sigs.k8s.io/yaml" ) -func ParseSecurityContext(runAsUser string, runAsGroup string, allowPrivilegeEscalation string) (corev1.SecurityContext, error) { +func ParseSecurityContext(runAsUser string, runAsGroup string, allowPrivilegeEscalation string, secCtx string) (corev1.SecurityContext, error) { securityContext := corev1.SecurityContext{} if runAsUser != "" { @@ -53,5 +54,12 @@ func ParseSecurityContext(runAsUser string, runAsGroup string, allowPrivilegeEsc securityContext.AllowPrivilegeEscalation = &parsedAllowPrivilegeEscalation } + if secCtx != "" { + err := yaml.UnmarshalStrict([]byte(secCtx), &securityContext) + if err != nil { + return securityContext, errors.WithStack(errors.Errorf(`Security context secCtx error: "%s"`, err)) + } + } + return securityContext, nil } diff --git a/pkg/util/kube/security_context_test.go b/pkg/util/kube/security_context_test.go index 2e20974bc..8ad72e49c 100644 --- a/pkg/util/kube/security_context_test.go +++ b/pkg/util/kube/security_context_test.go @@ -30,6 +30,7 @@ func TestParseSecurityContext(t *testing.T) { runAsUser string runAsGroup string allowPrivilegeEscalation string + secCtx string } tests := []struct { name string @@ -37,33 +38,184 @@ func TestParseSecurityContext(t *testing.T) { wantErr bool expected *corev1.SecurityContext }{ - {"valid security context", args{"1001", "999", "true"}, false, &corev1.SecurityContext{ - RunAsUser: pointInt64(1001), - RunAsGroup: pointInt64(999), - AllowPrivilegeEscalation: boolptr.True(), - }}, + { + "valid security context", + args{"1001", "999", "true", ``}, + false, + &corev1.SecurityContext{ + RunAsUser: pointInt64(1001), + RunAsGroup: pointInt64(999), + AllowPrivilegeEscalation: boolptr.True(), + }, + }, + { + "valid security context with override runAsUser", + args{"1001", "999", "true", `runAsUser: 2000`}, + false, + &corev1.SecurityContext{ + RunAsUser: pointInt64(2000), + RunAsGroup: pointInt64(999), + AllowPrivilegeEscalation: boolptr.True(), + }, + }, + { + "valid securityContext with comments only secCtx key", + args{"", "", "",` +capabilities: + drop: + - ALL + add: + - cap1 + - cap2 +sELinuxOptions: + user: userLabel + role: roleLabel + type: typeLabel + level: levelLabel +# user www-data +runAsUser: 3333 +# group www-data +runAsGroup: 3333 +runAsNonRoot: true +readOnlyRootFilesystem: true +allowPrivilegeEscalation: false`}, + false, + &corev1.SecurityContext{ + RunAsUser: pointInt64(3333), + RunAsGroup: pointInt64(3333), + Capabilities: &corev1.Capabilities{ + Drop: []corev1.Capability{"ALL"}, + Add: []corev1.Capability{"cap1", "cap2"}, + }, + SELinuxOptions: &corev1.SELinuxOptions{ + User: "userLabel", + Role: "roleLabel", + Type: "typeLabel", + Level: "levelLabel", + }, + RunAsNonRoot: boolptr.True(), + ReadOnlyRootFilesystem: boolptr.True(), + AllowPrivilegeEscalation: boolptr.False(), + }, + }, + { + "valid securityContext with secCtx key override runAsUser runAsGroup and allowPrivilegeEscalation", + args{"1001", "999", "true",` +capabilities: + drop: + - ALL + add: + - cap1 + - cap2 +sELinuxOptions: + user: userLabel + role: roleLabel + type: typeLabel + level: levelLabel +# user www-data +runAsUser: 3333 +# group www-data +runAsGroup: 3333 +runAsNonRoot: true +readOnlyRootFilesystem: true +allowPrivilegeEscalation: false`}, + false, + &corev1.SecurityContext{ + RunAsUser: pointInt64(3333), + RunAsGroup: pointInt64(3333), + Capabilities: &corev1.Capabilities{ + Drop: []corev1.Capability{"ALL"}, + Add: []corev1.Capability{"cap1", "cap2"}, + }, + SELinuxOptions: &corev1.SELinuxOptions{ + User: "userLabel", + Role: "roleLabel", + Type: "typeLabel", + Level: "levelLabel", + }, + RunAsNonRoot: boolptr.True(), + ReadOnlyRootFilesystem: boolptr.True(), + AllowPrivilegeEscalation: boolptr.False(), + }, + }, { "another valid security context", - args{"1001", "999", "false"}, false, &corev1.SecurityContext{ + args{"1001", "999", "false", ""}, + false, + &corev1.SecurityContext{ RunAsUser: pointInt64(1001), RunAsGroup: pointInt64(999), AllowPrivilegeEscalation: boolptr.False(), }, }, - {"security context without runAsGroup", args{"1001", "", ""}, false, &corev1.SecurityContext{ + {"security context without runAsGroup", args{"1001", "", "", ""}, false, &corev1.SecurityContext{ RunAsUser: pointInt64(1001), }}, - {"security context without runAsUser", args{"", "999", ""}, false, &corev1.SecurityContext{ + {"security context without runAsUser", args{"", "999", "", ""}, false, &corev1.SecurityContext{ RunAsGroup: pointInt64(999), }}, - {"empty context without runAsUser", args{"", "", ""}, false, &corev1.SecurityContext{}}, - {"invalid security context runAsUser", args{"not a number", "", ""}, true, nil}, - {"invalid security context runAsGroup", args{"", "not a number", ""}, true, nil}, - {"invalid security context allowPrivilegeEscalation", args{"", "", "not a bool"}, true, nil}, + {"empty context without runAsUser", args{"", "", "", ""}, false, &corev1.SecurityContext{}}, + { + "invalid securityContext secCtx unknown key", + args{"", "", "",` +capabilitiesUnknownkey: + drop: + - ALL + add: + - cap1 + - cap2 +# user www-data +runAsUser: 3333 +# group www-data +runAsGroup: 3333 +runAsNonRoot: true +readOnlyRootFilesystem: true +allowPrivilegeEscalation: false`}, + true, nil, + }, + { + "invalid securityContext secCtx wrong value type string instead of bool", + args{"", "", "",` +capabilitiesUnknownkey: + drop: + - ALL + add: + - cap1 + - cap2 +# user www-data +runAsUser: 3333 +# group www-data +runAsGroup: 3333 +runAsNonRoot: plop +readOnlyRootFilesystem: true +allowPrivilegeEscalation: false`}, + true, nil, + }, + { + "invalid securityContext secCtx wrong value type string instead of int", + args{"", "", "",` +capabilitiesUnknownkey: + drop: + - ALL + add: + - cap1 + - cap2 +# user www-data +runAsUser: plop +# group www-data +runAsGroup: 3333 +runAsNonRoot: true +readOnlyRootFilesystem: true +allowPrivilegeEscalation: false`}, + true, nil, + }, + {"invalid security context runAsUser", args{"not a number", "", "", ""}, true, nil}, + {"invalid security context runAsGroup", args{"", "not a number", "", ""}, true, nil}, + {"invalid security context allowPrivilegeEscalation", args{"", "", "not a bool", ""}, true, nil}, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - got, err := ParseSecurityContext(tt.args.runAsUser, tt.args.runAsGroup, tt.args.allowPrivilegeEscalation) + got, err := ParseSecurityContext(tt.args.runAsUser, tt.args.runAsGroup, tt.args.allowPrivilegeEscalation, tt.args.secCtx) if tt.wantErr { assert.Error(t, err) return @@ -74,8 +226,7 @@ func TestParseSecurityContext(t *testing.T) { tt.expected = &corev1.SecurityContext{} } - assert.Equal(t, tt.expected.RunAsUser, got.RunAsUser) - assert.Equal(t, tt.expected.RunAsGroup, got.RunAsGroup) + assert.Equal(t, *tt.expected, got) }) } } From b0fb9f799b279b5170ce1c48f5393536d868520c Mon Sep 17 00:00:00 2001 From: MatthieuFin Date: Tue, 31 Aug 2021 17:21:12 +0200 Subject: [PATCH 2/6] Add doc for new secCtx cm key and missing secCtxAllowPrivilegeEscalation. Signed-off-by: MatthieuFin --- site/content/docs/main/restic.md | 20 ++++++++++++++++++-- 1 file changed, 18 insertions(+), 2 deletions(-) diff --git a/site/content/docs/main/restic.md b/site/content/docs/main/restic.md index 8873c1ff5..096abda6e 100644 --- a/site/content/docs/main/restic.md +++ b/site/content/docs/main/restic.md @@ -393,11 +393,27 @@ data: # If not set, it will default to "128Mi". A value of "0" is treated as unbounded. memLimit: 128Mi - # "secCtxRunAsUser sets the securityContext.runAsUser value on the restic init containers during restore." + # "secCtxRunAsUser" sets the securityContext.runAsUser value on the restic init containers during restore. secCtxRunAsUser: 1001 - # "secCtxRunAsGroup sets the securityContext.runAsGroup value on the restic init containers during restore." + # "secCtxRunAsGroup" sets the securityContext.runAsGroup value on the restic init containers during restore. secCtxRunAsGroup: 999 + + # "secCtxAllowPrivilegeEscalation" sets the securityContext.allowPrivilegeEscalation value on the restic init containers during restore. + secCtxAllowPrivilegeEscalation: false + + # "secCtx" sets the securityContext object value on the restic init containers during restore. + # This key override `secCtxRunAsUser`, `secCtxRunAsGroup`, `secCtxAllowPrivilegeEscalation` if `secCtx.runAsUser`, `secCtx.runAsGroup` or `secCtx.allowPrivilegeEscalation` are set. + secCtx: | + capabilities: + drop: + - ALL + add: [] + allowPrivilegeEscalation: false + readOnlyRootFilesystem: true + runAsUser: 1001 + runAsGroup: 999 + ``` ## Troubleshooting From c4e53b936540cd711b4bbb0b50b1752d003b05b7 Mon Sep 17 00:00:00 2001 From: MatthieuFin Date: Tue, 31 Aug 2021 17:25:20 +0200 Subject: [PATCH 3/6] add changelog Signed-off-by: MatthieuFin --- changelogs/unreleased/4084-MatthieuFin | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelogs/unreleased/4084-MatthieuFin diff --git a/changelogs/unreleased/4084-MatthieuFin b/changelogs/unreleased/4084-MatthieuFin new file mode 100644 index 000000000..c4bc33833 --- /dev/null +++ b/changelogs/unreleased/4084-MatthieuFin @@ -0,0 +1 @@ +restic: add full support for setting SecurityContext for restore init container from configMap. From 338af4e5842f378b64588944c85fddec901eb1b6 Mon Sep 17 00:00:00 2001 From: MatthieuFin Date: Tue, 31 Aug 2021 17:27:18 +0200 Subject: [PATCH 4/6] update dependancies Signed-off-by: MatthieuFin --- go.mod | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/go.mod b/go.mod index bcce08eb7..7b5e196db 100644 --- a/go.mod +++ b/go.mod @@ -43,7 +43,7 @@ require ( k8s.io/kube-aggregator v0.19.12 sigs.k8s.io/cluster-api v0.3.11-0.20210106212952-b6c1b5b3db3d sigs.k8s.io/controller-runtime v0.7.1-0.20201215171748-096b2e07c091 - sigs.k8s.io/yaml v1.2.0 // indirect + sigs.k8s.io/yaml v1.2.0 ) replace github.com/gogo/protobuf => github.com/gogo/protobuf v1.3.2 From 08e4138c16f8f042f72024ba8b0b75ffebe8c7c5 Mon Sep 17 00:00:00 2001 From: MatthieuFin Date: Tue, 31 Aug 2021 17:43:09 +0200 Subject: [PATCH 5/6] Fix lint issue and test failed Signed-off-by: MatthieuFin --- pkg/restore/restic_restore_action.go | 9 ++--- pkg/restore/restic_restore_action_test.go | 2 +- pkg/util/kube/security_context_test.go | 42 +++++++++++------------ 3 files changed, 27 insertions(+), 26 deletions(-) diff --git a/pkg/restore/restic_restore_action.go b/pkg/restore/restic_restore_action.go index f72923efe..91b4a6761 100644 --- a/pkg/restore/restic_restore_action.go +++ b/pkg/restore/restic_restore_action.go @@ -245,6 +245,7 @@ func getResourceLimits(log logrus.FieldLogger, config *corev1.ConfigMap) (string return config.Data["cpuLimit"], config.Data["memLimit"] } + // getSecurityContext extracts securityContext runAsUser, runAsGroup, allowPrivilegeEscalation, and securityContext from a ConfigMap. func getSecurityContext(log logrus.FieldLogger, config *corev1.ConfigMap) (string, string, string, string) { if config == nil { @@ -252,10 +253,10 @@ func getSecurityContext(log logrus.FieldLogger, config *corev1.ConfigMap) (strin return "", "", "", "" } - return config.Data["secCtxRunAsUser"], - config.Data["secCtxRunAsGroup"], - config.Data["secCtxAllowPrivilegeEscalation"], - config.Data["secCtx"] + return config.Data["secCtxRunAsUser"], + config.Data["secCtxRunAsGroup"], + config.Data["secCtxAllowPrivilegeEscalation"], + config.Data["secCtx"] } // TODO eventually this can move to pkg/plugin/framework since it'll be used across multiple diff --git a/pkg/restore/restic_restore_action_test.go b/pkg/restore/restic_restore_action_test.go index 10148db68..b218f4b88 100644 --- a/pkg/restore/restic_restore_action_test.go +++ b/pkg/restore/restic_restore_action_test.go @@ -117,7 +117,7 @@ func TestResticRestoreActionExecute(t *testing.T) { defaultCPURequestLimit, defaultMemRequestLimit, // limits ) - securityContext, _ := kube.ParseSecurityContext("", "", "") + securityContext, _ := kube.ParseSecurityContext("", "", "", "") var ( restoreName = "my-restore" diff --git a/pkg/util/kube/security_context_test.go b/pkg/util/kube/security_context_test.go index 8ad72e49c..f4b9ccb7c 100644 --- a/pkg/util/kube/security_context_test.go +++ b/pkg/util/kube/security_context_test.go @@ -60,7 +60,7 @@ func TestParseSecurityContext(t *testing.T) { }, { "valid securityContext with comments only secCtx key", - args{"", "", "",` + args{"", "", "", ` capabilities: drop: - ALL @@ -81,16 +81,16 @@ readOnlyRootFilesystem: true allowPrivilegeEscalation: false`}, false, &corev1.SecurityContext{ - RunAsUser: pointInt64(3333), - RunAsGroup: pointInt64(3333), - Capabilities: &corev1.Capabilities{ + RunAsUser: pointInt64(3333), + RunAsGroup: pointInt64(3333), + Capabilities: &corev1.Capabilities{ Drop: []corev1.Capability{"ALL"}, - Add: []corev1.Capability{"cap1", "cap2"}, + Add: []corev1.Capability{"cap1", "cap2"}, }, - SELinuxOptions: &corev1.SELinuxOptions{ - User: "userLabel", - Role: "roleLabel", - Type: "typeLabel", + SELinuxOptions: &corev1.SELinuxOptions{ + User: "userLabel", + Role: "roleLabel", + Type: "typeLabel", Level: "levelLabel", }, RunAsNonRoot: boolptr.True(), @@ -100,7 +100,7 @@ allowPrivilegeEscalation: false`}, }, { "valid securityContext with secCtx key override runAsUser runAsGroup and allowPrivilegeEscalation", - args{"1001", "999", "true",` + args{"1001", "999", "true", ` capabilities: drop: - ALL @@ -121,16 +121,16 @@ readOnlyRootFilesystem: true allowPrivilegeEscalation: false`}, false, &corev1.SecurityContext{ - RunAsUser: pointInt64(3333), - RunAsGroup: pointInt64(3333), - Capabilities: &corev1.Capabilities{ + RunAsUser: pointInt64(3333), + RunAsGroup: pointInt64(3333), + Capabilities: &corev1.Capabilities{ Drop: []corev1.Capability{"ALL"}, - Add: []corev1.Capability{"cap1", "cap2"}, + Add: []corev1.Capability{"cap1", "cap2"}, }, - SELinuxOptions: &corev1.SELinuxOptions{ - User: "userLabel", - Role: "roleLabel", - Type: "typeLabel", + SELinuxOptions: &corev1.SELinuxOptions{ + User: "userLabel", + Role: "roleLabel", + Type: "typeLabel", Level: "levelLabel", }, RunAsNonRoot: boolptr.True(), @@ -157,7 +157,7 @@ allowPrivilegeEscalation: false`}, {"empty context without runAsUser", args{"", "", "", ""}, false, &corev1.SecurityContext{}}, { "invalid securityContext secCtx unknown key", - args{"", "", "",` + args{"", "", "", ` capabilitiesUnknownkey: drop: - ALL @@ -175,7 +175,7 @@ allowPrivilegeEscalation: false`}, }, { "invalid securityContext secCtx wrong value type string instead of bool", - args{"", "", "",` + args{"", "", "", ` capabilitiesUnknownkey: drop: - ALL @@ -193,7 +193,7 @@ allowPrivilegeEscalation: false`}, }, { "invalid securityContext secCtx wrong value type string instead of int", - args{"", "", "",` + args{"", "", "", ` capabilitiesUnknownkey: drop: - ALL From a57298254f2d90dc09532ad7228464e1b3bb45ad Mon Sep 17 00:00:00 2001 From: MatthieuFin Date: Thu, 24 Feb 2022 12:09:04 +0100 Subject: [PATCH 6/6] Fix typo on tests fields name and add another test with gesture of errors wanted on equals Signed-off-by: MatthieuFin --- pkg/util/kube/security_context_test.go | 50 ++++++++++++++++++++++++-- 1 file changed, 47 insertions(+), 3 deletions(-) diff --git a/pkg/util/kube/security_context_test.go b/pkg/util/kube/security_context_test.go index f4b9ccb7c..64a9a22cb 100644 --- a/pkg/util/kube/security_context_test.go +++ b/pkg/util/kube/security_context_test.go @@ -67,7 +67,7 @@ capabilities: add: - cap1 - cap2 -sELinuxOptions: +seLinuxOptions: user: userLabel role: roleLabel type: typeLabel @@ -98,6 +98,46 @@ allowPrivilegeEscalation: false`}, AllowPrivilegeEscalation: boolptr.False(), }, }, + { + "valid securityContext with comments only secCtx key check seLinuxOptions is correctly parsed", + args{"", "", "", ` +capabilities: + drop: + - ALL + add: + - cap1 + - cap2 +seLinuxOptions: + user: userLabelFail + role: roleLabel + type: typeLabel + level: levelLabel +# user www-data +runAsUser: 3333 +# group www-data +runAsGroup: 3333 +runAsNonRoot: true +readOnlyRootFilesystem: true +allowPrivilegeEscalation: false`}, + true, + &corev1.SecurityContext{ + RunAsUser: pointInt64(3333), + RunAsGroup: pointInt64(3333), + Capabilities: &corev1.Capabilities{ + Drop: []corev1.Capability{"ALL"}, + Add: []corev1.Capability{"cap1", "cap2"}, + }, + SELinuxOptions: &corev1.SELinuxOptions{ + User: "userLabel", + Role: "roleLabel", + Type: "typeLabel", + Level: "levelLabel", + }, + RunAsNonRoot: boolptr.True(), + ReadOnlyRootFilesystem: boolptr.True(), + AllowPrivilegeEscalation: boolptr.False(), + }, + }, { "valid securityContext with secCtx key override runAsUser runAsGroup and allowPrivilegeEscalation", args{"1001", "999", "true", ` @@ -107,7 +147,7 @@ capabilities: add: - cap1 - cap2 -sELinuxOptions: +seLinuxOptions: user: userLabel role: roleLabel type: typeLabel @@ -216,7 +256,7 @@ allowPrivilegeEscalation: false`}, for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { got, err := ParseSecurityContext(tt.args.runAsUser, tt.args.runAsGroup, tt.args.allowPrivilegeEscalation, tt.args.secCtx) - if tt.wantErr { + if err != nil && tt.wantErr { assert.Error(t, err) return } @@ -226,6 +266,10 @@ allowPrivilegeEscalation: false`}, tt.expected = &corev1.SecurityContext{} } + if tt.wantErr { + assert.NotEqual(t, *tt.expected, got) + return + } assert.Equal(t, *tt.expected, got) }) }