Skip to content

Commit 62b04a0

Browse files
nader-ziadanader-ziada
authored
Change SecurePodDefaults default from Disabled to Enabled (knative#16042)
* create a new default value for secure-pod-defaults: AllowRootBounded - default is still disabled - when AllowRootBounded, defaults SeccompProfile and Capabilities if nil - when enabled sets RunAsNonRoot to true if not already specified * clean up to address review comments * use slices.Contains for checking flag value
1 parent b2f3ee3 commit 62b04a0

File tree

10 files changed

+493
-19
lines changed

10 files changed

+493
-19
lines changed

‎config/core/configmaps/features.yaml‎

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,7 @@ metadata:
2222
app.kubernetes.io/component: controller
2323
app.kubernetes.io/version: devel
2424
annotations:
25-
knative.dev/example-checksum: "e6ec6806"
25+
knative.dev/example-checksum: "9553535b"
2626
data:
2727
_example: |-
2828
################################
@@ -43,8 +43,11 @@ data:
4343
# Default SecurityContext settings to secure-by-default values
4444
# if unset.
4545
#
46-
# This value will default to "enabled" in a future release,
47-
# probably Knative 1.10
46+
# Disabled - do nothing; no security options are applied
47+
# AllowRootBounded - Applies secure defaults without enforcing strict policies; sets seccompProfile
48+
# to RuntimeDefault and drops all capabilities
49+
# Enabled - Enforces security defaults; sets seccompProfile to RuntimeDefault, drops all capabilities,
50+
# and sets runAsNonRoot to true if not already specified.
4851
secure-pod-defaults: "disabled"
4952
5053
# Indicates whether multi container support is enabled

‎pkg/apis/config/features.go‎

Lines changed: 21 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -37,6 +37,9 @@ const (
3737
// Allowed neither explicitly disables or enables a behavior.
3838
// eg. allow a client to control behavior with an annotation or allow a new value through validation.
3939
Allowed Flag = "Allowed"
40+
// AllowRootBounded is used by secure-pod-defaults to apply secure defaults without enforcing strict policies;
41+
// sets RunAsNonRoot to true if not already specified
42+
AllowRootBounded Flag = "AllowRootBounded"
4043
)
4144

4245
// service annotations under features.knative.dev/*
@@ -124,7 +127,7 @@ func NewFeaturesConfigFromMap(data map[string]string) (*Features, error) {
124127
asFlag("multi-container-probing", &nc.MultiContainerProbing),
125128
asFlag("queueproxy.mount-podinfo", &nc.QueueProxyMountPodInfo),
126129
asFlag("queueproxy.resource-defaults", &nc.QueueProxyResourceDefaults),
127-
asFlag("secure-pod-defaults", &nc.SecurePodDefaults),
130+
asSecurePodDefaultsFlag("secure-pod-defaults", &nc.SecurePodDefaults),
128131
asFlag("tag-header-based-routing", &nc.TagHeaderBasedRouting),
129132
asFlag(FeatureContainerSpecAddCapabilities, &nc.ContainerSpecAddCapabilities),
130133
asFlag(FeaturePodSpecAffinity, &nc.PodSpecAffinity),
@@ -198,6 +201,7 @@ type Features struct {
198201
}
199202

200203
// asFlag parses the value at key as a Flag into the target, if it exists.
204+
// Only accepts Enabled, Disabled, and Allowed values.
201205
func asFlag(key string, target *Flag) cm.ParseFunc {
202206
return func(data map[string]string) error {
203207
if raw, ok := data[key]; ok {
@@ -211,3 +215,19 @@ func asFlag(key string, target *Flag) cm.ParseFunc {
211215
return nil
212216
}
213217
}
218+
219+
// asSecurePodDefaultsFlag parses the value at key as a Flag into the target, if it exists.
220+
// Accepts Enabled, Disabled, Allowed, and SecureDefaultsOverridable values.
221+
func asSecurePodDefaultsFlag(key string, target *Flag) cm.ParseFunc {
222+
return func(data map[string]string) error {
223+
if raw, ok := data[key]; ok {
224+
for _, flag := range []Flag{Disabled, AllowRootBounded, Enabled} {
225+
if strings.EqualFold(raw, string(flag)) {
226+
*target = flag
227+
return nil
228+
}
229+
}
230+
}
231+
return nil
232+
}
233+
}

‎pkg/apis/config/features_test.go‎

Lines changed: 45 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -714,6 +714,51 @@ func TestFeaturesConfiguration(t *testing.T) {
714714
data: map[string]string{
715715
"kubernetes.podspec-hostnetwork": "Disabled",
716716
},
717+
}, {
718+
name: "secure-pod-defaults Disabled",
719+
wantErr: false,
720+
wantFeatures: defaultWith(&Features{
721+
SecurePodDefaults: Disabled,
722+
}),
723+
data: map[string]string{
724+
"secure-pod-defaults": "Disabled",
725+
},
726+
}, {
727+
name: "invalid secure-pod-defaults value",
728+
wantErr: false,
729+
wantFeatures: defaultWith(&Features{
730+
SecurePodDefaults: Disabled,
731+
}),
732+
data: map[string]string{
733+
"secure-pod-defaults": "InvalidValue",
734+
},
735+
}, {
736+
name: "secure-pod-defaults AllowRootBounded",
737+
wantErr: false,
738+
wantFeatures: defaultWith(&Features{
739+
SecurePodDefaults: AllowRootBounded,
740+
}),
741+
data: map[string]string{
742+
"secure-pod-defaults": "AllowRootBounded",
743+
},
744+
}, {
745+
name: "PodSpecAffinity cannot be set to Restricted",
746+
wantErr: false,
747+
wantFeatures: defaultWith(&Features{
748+
PodSpecAffinity: Disabled, // Should remain default since Restricted is not valid
749+
}),
750+
data: map[string]string{
751+
"kubernetes.podspec-affinity": "Restricted",
752+
},
753+
}, {
754+
name: "tag-header-based-routing cannot be set to Restricted",
755+
wantErr: false,
756+
wantFeatures: defaultWith(&Features{
757+
TagHeaderBasedRouting: Disabled, // Should remain default since Restricted is not valid
758+
}),
759+
data: map[string]string{
760+
"tag-header-based-routing": "Restricted",
761+
},
717762
}}
718763

719764
for _, tt := range configTests {

‎pkg/apis/serving/fieldmask.go‎

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@ package serving
2121

2222
import (
2323
"context"
24+
"slices"
2425

2526
corev1 "k8s.io/api/core/v1"
2627
"knative.dev/serving/pkg/apis/config"
@@ -664,7 +665,7 @@ func PodSecurityContextMask(ctx context.Context, in *corev1.PodSecurityContext)
664665

665666
out := new(corev1.PodSecurityContext)
666667

667-
if config.FromContextOrDefaults(ctx).Features.SecurePodDefaults == config.Enabled {
668+
if slices.Contains([]config.Flag{config.Enabled, config.AllowRootBounded}, config.FromContextOrDefaults(ctx).Features.SecurePodDefaults) {
668669
// Allow to opt out of more-secure defaults if SecurePodDefaults is enabled.
669670
// This aligns with defaultSecurityContext in revision_defaults.go.
670671
if in.SeccompProfile != nil {
@@ -749,8 +750,8 @@ func CapabilitiesMask(ctx context.Context, in *corev1.Capabilities) *corev1.Capa
749750

750751
if config.FromContextOrDefaults(ctx).Features.ContainerSpecAddCapabilities == config.Enabled {
751752
out.Add = in.Add
752-
} else if config.FromContextOrDefaults(ctx).Features.SecurePodDefaults == config.Enabled {
753-
if len(in.Add) == 1 && in.Add[0] == "NET_BIND_SERVICE" {
753+
} else if slices.Contains([]config.Flag{config.Enabled, config.AllowRootBounded}, config.FromContextOrDefaults(ctx).Features.SecurePodDefaults) {
754+
if slices.Equal(in.Add, []corev1.Capability{"NET_BIND_SERVICE"}) {
754755
out.Add = in.Add
755756
} else {
756757
out.Add = nil

‎pkg/apis/serving/fieldmask_test.go‎

Lines changed: 119 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -855,8 +855,20 @@ func TestPodSecurityContextMask(t *testing.T) {
855855
},
856856
}
857857

858-
want := &corev1.PodSecurityContext{}
859-
ctx := context.Background()
858+
want := &corev1.PodSecurityContext{
859+
SeccompProfile: &corev1.SeccompProfile{
860+
Type: corev1.SeccompProfileTypeRuntimeDefault,
861+
LocalhostProfile: nil,
862+
},
863+
}
864+
ctx := config.ToContext(context.Background(),
865+
&config.Config{
866+
Features: &config.Features{
867+
SecurePodDefaults: config.Enabled,
868+
PodSpecSecurityContext: config.Disabled,
869+
},
870+
},
871+
)
860872

861873
got := PodSecurityContextMask(ctx, in)
862874

@@ -967,6 +979,111 @@ func TestPodSecurityContextMask_SecurePodDefaultsEnabled(t *testing.T) {
967979
}
968980
}
969981

982+
func TestPodSecurityContextMask_SecurePodDefaults_AllowRootBounded(t *testing.T) {
983+
// Test that AllowRootBounded works the same as Enabled for masking purposes
984+
want := &corev1.PodSecurityContext{
985+
SeccompProfile: &corev1.SeccompProfile{
986+
Type: corev1.SeccompProfileTypeRuntimeDefault,
987+
},
988+
}
989+
990+
in := &corev1.PodSecurityContext{
991+
SELinuxOptions: &corev1.SELinuxOptions{},
992+
WindowsOptions: &corev1.WindowsSecurityContextOptions{},
993+
SupplementalGroups: []int64{1},
994+
Sysctls: []corev1.Sysctl{},
995+
RunAsUser: ptr.Int64(1),
996+
RunAsGroup: ptr.Int64(1),
997+
RunAsNonRoot: ptr.Bool(true),
998+
FSGroup: ptr.Int64(1),
999+
SeccompProfile: &corev1.SeccompProfile{
1000+
Type: corev1.SeccompProfileTypeRuntimeDefault,
1001+
},
1002+
}
1003+
1004+
ctx := config.ToContext(context.Background(),
1005+
&config.Config{
1006+
Features: &config.Features{
1007+
SecurePodDefaults: config.AllowRootBounded,
1008+
PodSpecSecurityContext: config.Disabled,
1009+
},
1010+
},
1011+
)
1012+
1013+
got := PodSecurityContextMask(ctx, in)
1014+
1015+
if &want == &got {
1016+
t.Error("Input and output share addresses. Want different addresses")
1017+
}
1018+
1019+
if diff, err := kmp.SafeDiff(want, got); err != nil {
1020+
t.Error("Got error comparing output, err =", err)
1021+
} else if diff != "" {
1022+
t.Error("PodSecurityContextMask (-want, +got):", diff)
1023+
}
1024+
}
1025+
1026+
func TestCapabilitiesMask_SecurePodDefaults_AllowRootBounded(t *testing.T) {
1027+
// Ensures users can only add NET_BIND_SERVICE or nil capabilities
1028+
tests := []struct {
1029+
name string
1030+
in *corev1.Capabilities
1031+
want *corev1.Capabilities
1032+
}{{
1033+
name: "empty",
1034+
in: &corev1.Capabilities{
1035+
Add: nil,
1036+
},
1037+
want: &corev1.Capabilities{
1038+
Add: nil,
1039+
},
1040+
}, {
1041+
name: "allows NET_BIND_SERVICE capability",
1042+
in: &corev1.Capabilities{
1043+
Add: []corev1.Capability{"NET_BIND_SERVICE"},
1044+
},
1045+
want: &corev1.Capabilities{
1046+
Add: []corev1.Capability{"NET_BIND_SERVICE"},
1047+
},
1048+
}, {
1049+
name: "prevents restricted fields",
1050+
in: &corev1.Capabilities{
1051+
Add: []corev1.Capability{"CHOWN"},
1052+
},
1053+
want: &corev1.Capabilities{
1054+
Add: nil,
1055+
},
1056+
}}
1057+
1058+
for _, test := range tests {
1059+
ctx := config.ToContext(context.Background(),
1060+
&config.Config{
1061+
Features: &config.Features{
1062+
SecurePodDefaults: config.AllowRootBounded,
1063+
},
1064+
},
1065+
)
1066+
1067+
t.Run(test.name, func(t *testing.T) {
1068+
got := CapabilitiesMask(ctx, test.in)
1069+
1070+
if &test.want == &got {
1071+
t.Error("Input and output share addresses. Want different addresses")
1072+
}
1073+
1074+
if diff, err := kmp.SafeDiff(test.want, got); err != nil {
1075+
t.Error("Got error comparing output, err =", err)
1076+
} else if diff != "" {
1077+
t.Error("CapabilitiesMask (-want, +got):", diff)
1078+
}
1079+
1080+
if got = CapabilitiesMask(ctx, nil); got != nil {
1081+
t.Errorf("CapabilitiesMask = %v, want: nil", got)
1082+
}
1083+
})
1084+
}
1085+
}
1086+
9701087
func TestSecurityContextMask(t *testing.T) {
9711088
mtype := corev1.UnmaskedProcMount
9721089
want := &corev1.SecurityContext{

‎pkg/apis/serving/k8s_validation.go‎

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@ import (
2121
"fmt"
2222
"math"
2323
"path"
24+
"slices"
2425
"strings"
2526

2627
"github.com/google/go-containerregistry/pkg/name"
@@ -985,14 +986,19 @@ func ValidatePodSecurityContext(ctx context.Context, sc *corev1.PodSecurityConte
985986
// Note that this **explicitly** does not warn on dangerous SecurityContext
986987
// settings, the purpose is to avoid accidentally-insecure settings, not to
987988
// block deliberate use of dangerous settings.
988-
func warnDefaultContainerSecurityContext(_ context.Context, psc *corev1.PodSecurityContext, sc *corev1.SecurityContext) *apis.FieldError {
989+
func warnDefaultContainerSecurityContext(ctx context.Context, psc *corev1.PodSecurityContext, sc *corev1.SecurityContext) *apis.FieldError {
989990
if sc == nil {
990991
sc = &corev1.SecurityContext{}
991992
}
992993
if psc == nil {
993994
psc = &corev1.PodSecurityContext{}
994995
}
995996

997+
// if the user has explicitly enabled the feature, we don't need to warn
998+
if slices.Contains([]config.Flag{config.Enabled, config.AllowRootBounded}, config.FromContextOrDefaults(ctx).Features.PodSpecSecurityContext) {
999+
return nil
1000+
}
1001+
9961002
insecureDefault := func(fieldPath string) *apis.FieldError {
9971003
return apis.ErrGeneric("Kubernetes default value is insecure, Knative may default this to secure in a future release", fieldPath).At(apis.WarningLevel)
9981004
}

‎pkg/apis/serving/k8s_validation_test.go‎

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1474,6 +1474,26 @@ func TestPodSpecFeatureValidation(t *testing.T) {
14741474
}
14751475
}
14761476

1477+
func TestPodSpecSecurityContext_FeatureEnabled_AllowsPodSecurityContext(t *testing.T) {
1478+
ctx := context.Background()
1479+
// Ensure the feature is enabled in context
1480+
cfg := config.FromContextOrDefaults(ctx)
1481+
cfg.Features.PodSpecSecurityContext = config.Enabled
1482+
ctx = config.ToContext(ctx, cfg)
1483+
1484+
ps := corev1.PodSpec{
1485+
SecurityContext: &corev1.PodSecurityContext{
1486+
RunAsNonRoot: ptr.Bool(false),
1487+
},
1488+
Containers: []corev1.Container{{
1489+
Image: "busybox",
1490+
}},
1491+
}
1492+
if got := ValidatePodSpec(ctx, ps).Filter(apis.ErrorLevel); got != nil {
1493+
t.Fatalf("ValidatePodSpec() = %v, want nil", got)
1494+
}
1495+
}
1496+
14771497
func TestPodSpecFieldRefValidation(t *testing.T) {
14781498
tests := []struct {
14791499
name string

‎pkg/apis/serving/v1/revision_defaults.go‎

Lines changed: 15 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,9 @@ limitations under the License.
1717
package v1
1818

1919
import (
20+
"cmp"
2021
"context"
22+
"slices"
2123
"strconv"
2224

2325
corev1 "k8s.io/api/core/v1"
@@ -203,10 +205,13 @@ func (*RevisionSpec) applyGRPCProbeDefaults(container *corev1.Container) {
203205

204206
// Upgrade SecurityContext for this container and the Pod definition to use settings
205207
// for the `restricted` profile when the feature flag is enabled.
206-
// This does not currently set `runAsNonRoot` for the restricted profile, because
207-
// that feels harder to default safely.
208+
// when the feature flag is enabled or AllowRootBounded:
209+
// `seccompProfile` is set to `RuntimeDefault` if its empty or nil
210+
// `capabilities` is set to `NET_BIND_SERVICE` if its empty or nil
211+
// when the feature flag is set to Enabled:
212+
// `runAsNonRoot` is set to true only if its empty or nil
208213
func (rs *RevisionSpec) defaultSecurityContext(psc *corev1.PodSecurityContext, container *corev1.Container, cfg *config.Config) {
209-
if cfg.Features.SecurePodDefaults != config.Enabled {
214+
if !slices.Contains([]config.Flag{config.Enabled, config.AllowRootBounded}, cfg.Features.SecurePodDefaults) {
210215
return
211216
}
212217

@@ -224,9 +229,7 @@ func (rs *RevisionSpec) defaultSecurityContext(psc *corev1.PodSecurityContext, c
224229
updatedSC.AllowPrivilegeEscalation = ptr.Bool(false)
225230
}
226231
if psc.SeccompProfile == nil || psc.SeccompProfile.Type == "" {
227-
if updatedSC.SeccompProfile == nil {
228-
updatedSC.SeccompProfile = &corev1.SeccompProfile{}
229-
}
232+
updatedSC.SeccompProfile = cmp.Or(updatedSC.SeccompProfile, &corev1.SeccompProfile{})
230233
if updatedSC.SeccompProfile.Type == "" {
231234
updatedSC.SeccompProfile.Type = corev1.SeccompProfileTypeRuntimeDefault
232235
}
@@ -247,8 +250,12 @@ func (rs *RevisionSpec) defaultSecurityContext(psc *corev1.PodSecurityContext, c
247250
}
248251
}
249252

250-
if psc.RunAsNonRoot == nil {
251-
updatedSC.RunAsNonRoot = ptr.Bool(true)
253+
if cfg.Features.SecurePodDefaults == config.Enabled {
254+
if psc.RunAsNonRoot == nil {
255+
if updatedSC.RunAsNonRoot == nil {
256+
updatedSC.RunAsNonRoot = ptr.Bool(true)
257+
}
258+
}
252259
}
253260

254261
if *updatedSC != (corev1.SecurityContext{}) {

0 commit comments

Comments
 (0)