From 20bb84b3f1c79958a3c957fd721539b3e2396b6e Mon Sep 17 00:00:00 2001 From: Mengjiao Liu Date: Mon, 6 Dec 2021 16:53:27 +0800 Subject: [PATCH] Pod SecurityContext and PodSecurityPolicy supports slash as sysctl separator --- pkg/api/pod/util.go | 19 ------- pkg/apis/core/validation/validation.go | 32 +++-------- pkg/apis/core/validation/validation_test.go | 53 +++++-------------- pkg/apis/policy/validation/validation.go | 18 ++----- pkg/apis/policy/validation/validation_test.go | 18 +++++-- pkg/kubelet/sysctl/allowlist.go | 2 +- 6 files changed, 40 insertions(+), 102 deletions(-) diff --git a/pkg/api/pod/util.go b/pkg/api/pod/util.go index b617d1d8dfa..70d8a5cdaee 100644 --- a/pkg/api/pod/util.go +++ b/pkg/api/pod/util.go @@ -329,20 +329,6 @@ func usesHugePagesInProjectedEnv(item api.Container) bool { return false } -// hasSysctlsWithSlashNames returns true if the sysctl name contains a slash, otherwise it returns false -func hasSysctlsWithSlashNames(podSpec *api.PodSpec) bool { - if podSpec.SecurityContext == nil { - return false - } - securityContext := podSpec.SecurityContext - for _, s := range securityContext.Sysctls { - if strings.Contains(s.Name, "/") { - return true - } - } - return false -} - func checkContainerUseIndivisibleHugePagesValues(container api.Container) bool { for resourceName, quantity := range container.Resources.Limits { if helper.IsHugePageResourceName(resourceName) { @@ -434,8 +420,6 @@ func GetValidationOptionsFromPodSpecAndMeta(podSpec, oldPodSpec *api.PodSpec, po AllowExpandedDNSConfig: utilfeature.DefaultFeatureGate.Enabled(features.ExpandedDNSConfig) || haveSameExpandedDNSConfig(podSpec, oldPodSpec), // Allow pod spec to use OS field AllowOSField: utilfeature.DefaultFeatureGate.Enabled(features.IdentifyPodOS), - // The default sysctl value does not contain a forward slash, and in 1.24 we intend to relax this to be true by default - AllowSysctlRegexContainSlash: false, } if oldPodSpec != nil { @@ -457,9 +441,6 @@ func GetValidationOptionsFromPodSpecAndMeta(podSpec, oldPodSpec *api.PodSpec, po // if old spec used non-integer multiple of huge page unit size, we must allow it opts.AllowIndivisibleHugePagesValues = usesIndivisibleHugePagesValues(oldPodSpec) - // if old spec used use relaxed validation for Update requests where the existing object's sysctl contains a slash, we must allow it. - opts.AllowSysctlRegexContainSlash = hasSysctlsWithSlashNames(oldPodSpec) - } if oldPodMeta != nil && !opts.AllowInvalidPodDeletionCost { // This is an update, so validate only if the existing object was valid. diff --git a/pkg/apis/core/validation/validation.go b/pkg/apis/core/validation/validation.go index 0e07a8e2a95..a8adf223cb3 100644 --- a/pkg/apis/core/validation/validation.go +++ b/pkg/apis/core/validation/validation.go @@ -3398,8 +3398,6 @@ type PodValidationOptions struct { AllowExpandedDNSConfig bool // Allow OSField to be set in the pod spec AllowOSField bool - // Allow sysctl name to contain a slash - AllowSysctlRegexContainSlash bool } // validatePodMetadataAndSpec tests if required fields in the pod.metadata and pod.spec are set, @@ -4058,9 +4056,6 @@ const ( // a sysctl segment regex, concatenated with dots to form a sysctl name SysctlSegmentFmt string = "[a-z0-9]([-_a-z0-9]*[a-z0-9])?" - // a sysctl name regex - SysctlFmt string = "(" + SysctlSegmentFmt + "\\.)*" + SysctlSegmentFmt - // a sysctl name regex with slash allowed SysctlContainSlashFmt string = "(" + SysctlSegmentFmt + "[\\./])*" + SysctlSegmentFmt @@ -4068,41 +4063,28 @@ const ( SysctlMaxLength int = 253 ) -var sysctlRegexp = regexp.MustCompile("^" + SysctlFmt + "$") - var sysctlContainSlashRegexp = regexp.MustCompile("^" + SysctlContainSlashFmt + "$") // IsValidSysctlName checks that the given string is a valid sysctl name, -// i.e. matches SysctlFmt (or SysctlContainSlashFmt if canContainSlash is true). +// i.e. matches SysctlContainSlashFmt. // More info: // https://man7.org/linux/man-pages/man8/sysctl.8.html // https://man7.org/linux/man-pages/man5/sysctl.d.5.html -func IsValidSysctlName(name string, canContainSlash bool) bool { +func IsValidSysctlName(name string) bool { if len(name) > SysctlMaxLength { return false } - if canContainSlash { - return sysctlContainSlashRegexp.MatchString(name) - } - return sysctlRegexp.MatchString(name) + return sysctlContainSlashRegexp.MatchString(name) } -func getSysctlFmt(canContainSlash bool) string { - if canContainSlash { - // use relaxed validation everywhere in 1.24 - return SysctlContainSlashFmt - } - // Will be removed in 1.24 - return SysctlFmt -} -func validateSysctls(sysctls []core.Sysctl, fldPath *field.Path, allowSysctlRegexContainSlash bool) field.ErrorList { +func validateSysctls(sysctls []core.Sysctl, fldPath *field.Path) field.ErrorList { allErrs := field.ErrorList{} names := make(map[string]struct{}) for i, s := range sysctls { if len(s.Name) == 0 { allErrs = append(allErrs, field.Required(fldPath.Index(i).Child("name"), "")) - } else if !IsValidSysctlName(s.Name, allowSysctlRegexContainSlash) { - allErrs = append(allErrs, field.Invalid(fldPath.Index(i).Child("name"), s.Name, fmt.Sprintf("must have at most %d characters and match regex %s", SysctlMaxLength, getSysctlFmt(allowSysctlRegexContainSlash)))) + } else if !IsValidSysctlName(s.Name) { + allErrs = append(allErrs, field.Invalid(fldPath.Index(i).Child("name"), s.Name, fmt.Sprintf("must have at most %d characters and match regex %s", SysctlMaxLength, sysctlContainSlashRegexp))) } else if _, ok := names[s.Name]; ok { allErrs = append(allErrs, field.Duplicate(fldPath.Index(i).Child("name"), s.Name)) } @@ -4142,7 +4124,7 @@ func ValidatePodSecurityContext(securityContext *core.PodSecurityContext, spec * } if len(securityContext.Sysctls) != 0 { - allErrs = append(allErrs, validateSysctls(securityContext.Sysctls, fldPath.Child("sysctls"), opts.AllowSysctlRegexContainSlash)...) + allErrs = append(allErrs, validateSysctls(securityContext.Sysctls, fldPath.Child("sysctls"))...) } if securityContext.FSGroupChangePolicy != nil { diff --git a/pkg/apis/core/validation/validation_test.go b/pkg/apis/core/validation/validation_test.go index a13468f1e40..d625d9a662c 100644 --- a/pkg/apis/core/validation/validation_test.go +++ b/pkg/apis/core/validation/validation_test.go @@ -18261,6 +18261,8 @@ func TestIsValidSysctlName(t *testing.T) { "a-b", "abc", "abc.def", + "a/b/c/d", + "a/b.c", } invalid := []string{ "", @@ -18285,6 +18287,10 @@ func TestIsValidSysctlName(t *testing.T) { "a.abc*", "a.b.*", "Abc", + "/", + "/a", + "a/abc*", + "a/b/*", func(n int) string { x := make([]byte, n) for i := range x { @@ -18294,34 +18300,13 @@ func TestIsValidSysctlName(t *testing.T) { }(256), } - containSlashesValid := []string{ - "a/b/c/d", - "a/b.c", - } - - containSlashesInvalid := []string{ - "/", - "/a", - "a/abc*", - "a/b/*", - } for _, s := range valid { - if !IsValidSysctlName(s, false) { + if !IsValidSysctlName(s) { t.Errorf("%q expected to be a valid sysctl name", s) } } for _, s := range invalid { - if IsValidSysctlName(s, false) { - t.Errorf("%q expected to be an invalid sysctl name", s) - } - } - for _, s := range containSlashesValid { - if !IsValidSysctlName(s, true) { - t.Errorf("%q expected to be a valid sysctl name", s) - } - } - for _, s := range containSlashesInvalid { - if IsValidSysctlName(s, true) { + if IsValidSysctlName(s) { t.Errorf("%q expected to be an invalid sysctl name", s) } } @@ -18331,6 +18316,8 @@ func TestValidateSysctls(t *testing.T) { valid := []string{ "net.foo.bar", "kernel.shmmax", + "net.ipv4.conf.enp3s0/200.forwarding", + "net/ipv4/conf/enp3s0.200/forwarding", } invalid := []string{ "i..nvalid", @@ -18342,16 +18329,11 @@ func TestValidateSysctls(t *testing.T) { "kernel.shmmax", } - containSlashes := []string{ - "net.ipv4.conf.enp3s0/200.forwarding", - "net/ipv4/conf/enp3s0.200/forwarding", - } - sysctls := make([]core.Sysctl, len(valid)) for i, sysctl := range valid { sysctls[i].Name = sysctl } - errs := validateSysctls(sysctls, field.NewPath("foo"), false) + errs := validateSysctls(sysctls, field.NewPath("foo")) if len(errs) != 0 { t.Errorf("unexpected validation errors: %v", errs) } @@ -18360,7 +18342,7 @@ func TestValidateSysctls(t *testing.T) { for i, sysctl := range invalid { sysctls[i].Name = sysctl } - errs = validateSysctls(sysctls, field.NewPath("foo"), false) + errs = validateSysctls(sysctls, field.NewPath("foo")) if len(errs) != 2 { t.Errorf("expected 2 validation errors. Got: %v", errs) } else { @@ -18376,21 +18358,12 @@ func TestValidateSysctls(t *testing.T) { for i, sysctl := range duplicates { sysctls[i].Name = sysctl } - errs = validateSysctls(sysctls, field.NewPath("foo"), false) + errs = validateSysctls(sysctls, field.NewPath("foo")) if len(errs) != 1 { t.Errorf("unexpected validation errors: %v", errs) } else if errs[0].Type != field.ErrorTypeDuplicate { t.Errorf("expected error type %v, got %v", field.ErrorTypeDuplicate, errs[0].Type) } - - sysctls = make([]core.Sysctl, len(containSlashes)) - for i, sysctl := range containSlashes { - sysctls[i].Name = sysctl - } - errs = validateSysctls(sysctls, field.NewPath("foo"), true) - if len(errs) != 0 { - t.Errorf("unexpected validation errors: %v", errs) - } } func newNodeNameEndpoint(nodeName string) *core.Endpoints { diff --git a/pkg/apis/policy/validation/validation.go b/pkg/apis/policy/validation/validation.go index ae2d931615e..a3888f5df4b 100644 --- a/pkg/apis/policy/validation/validation.go +++ b/pkg/apis/policy/validation/validation.go @@ -400,29 +400,21 @@ func validatePSPAllowedProcMountTypes(fldPath *field.Path, allowedProcMountTypes const sysctlPatternSegmentFmt string = "([a-z0-9][-_a-z0-9]*)?[a-z0-9*]" -// SysctlPatternFmt is a regex used for matching valid sysctl patterns. -const SysctlPatternFmt string = "(" + apivalidation.SysctlSegmentFmt + "\\.)*" + sysctlPatternSegmentFmt - // SysctlContainSlashPatternFmt is a regex that contains a slash used for matching valid sysctl patterns. const SysctlContainSlashPatternFmt string = "(" + apivalidation.SysctlSegmentFmt + "[\\./])*" + sysctlPatternSegmentFmt -var sysctlPatternRegexp = regexp.MustCompile("^" + SysctlPatternFmt + "$") - var sysctlContainSlashPatternRegexp = regexp.MustCompile("^" + SysctlContainSlashPatternFmt + "$") // IsValidSysctlPattern checks if name is a valid sysctl pattern. -// i.e. matches sysctlPatternRegexp (or sysctlContainSlashPatternRegexp if canContainSlash is true). +// i.e. matches sysctlContainSlashPatternRegexp. // More info: // https://man7.org/linux/man-pages/man8/sysctl.8.html // https://man7.org/linux/man-pages/man5/sysctl.d.5.html -func IsValidSysctlPattern(name string, canContainSlash bool) bool { +func IsValidSysctlPattern(name string) bool { if len(name) > apivalidation.SysctlMaxLength { return false } - if canContainSlash { - return sysctlContainSlashPatternRegexp.MatchString(name) - } - return sysctlPatternRegexp.MatchString(name) + return sysctlContainSlashPatternRegexp.MatchString(name) } func validatePodSecurityPolicySysctlListsDoNotOverlap(allowedSysctlsFldPath, forbiddenSysctlsFldPath *field.Path, allowedUnsafeSysctls, forbiddenSysctls []string) field.ErrorList { @@ -478,12 +470,12 @@ func validatePodSecurityPolicySysctls(fldPath *field.Path, sysctls []string) fie for i, s := range sysctls { if len(s) == 0 { allErrs = append(allErrs, field.Invalid(fldPath.Index(i), sysctls[i], "empty sysctl not allowed")) - } else if !IsValidSysctlPattern(string(s), false) { + } else if !IsValidSysctlPattern(string(s)) { allErrs = append( allErrs, field.Invalid(fldPath.Index(i), sysctls[i], fmt.Sprintf("must have at most %d characters and match regex %s", apivalidation.SysctlMaxLength, - SysctlPatternFmt, + SysctlContainSlashPatternFmt, )), ) } else if s[0] == '*' { diff --git a/pkg/apis/policy/validation/validation_test.go b/pkg/apis/policy/validation/validation_test.go index c6d154cb400..546836daed8 100644 --- a/pkg/apis/policy/validation/validation_test.go +++ b/pkg/apis/policy/validation/validation_test.go @@ -524,12 +524,12 @@ func TestValidatePodSecurityPolicy(t *testing.T) { "invalid allowed unsafe sysctl pattern": { psp: invalidAllowedUnsafeSysctlPattern, errorType: field.ErrorTypeInvalid, - errorDetail: fmt.Sprintf("must have at most 253 characters and match regex %s", SysctlPatternFmt), + errorDetail: fmt.Sprintf("must have at most 253 characters and match regex %s", SysctlContainSlashPatternFmt), }, "invalid forbidden sysctl pattern": { psp: invalidForbiddenSysctlPattern, errorType: field.ErrorTypeInvalid, - errorDetail: fmt.Sprintf("must have at most 253 characters and match regex %s", SysctlPatternFmt), + errorDetail: fmt.Sprintf("must have at most 253 characters and match regex %s", SysctlContainSlashPatternFmt), }, "invalid overlapping sysctl pattern": { psp: invalidOverlappingSysctls, @@ -805,6 +805,12 @@ func TestIsValidSysctlPattern(t *testing.T) { "abc*", "a.abc*", "a.b.*", + "a/b/c/d", + "a/*", + "a/b/*", + "a.b/c*", + "a.b/c.d", + "a/b.c/d", } invalid := []string{ "", @@ -823,6 +829,10 @@ func TestIsValidSysctlPattern(t *testing.T) { "a*b", "*a", "Abc", + "/", + "a/", + "/a", + "a*/b", func(n int) string { x := make([]byte, n) for i := range x { @@ -832,12 +842,12 @@ func TestIsValidSysctlPattern(t *testing.T) { }(256), } for _, s := range valid { - if !IsValidSysctlPattern(s, false) { + if !IsValidSysctlPattern(s) { t.Errorf("%q expected to be a valid sysctl pattern", s) } } for _, s := range invalid { - if IsValidSysctlPattern(s, false) { + if IsValidSysctlPattern(s) { t.Errorf("%q expected to be an invalid sysctl pattern", s) } } diff --git a/pkg/kubelet/sysctl/allowlist.go b/pkg/kubelet/sysctl/allowlist.go index 022da01dcee..beb1de3ba4e 100644 --- a/pkg/kubelet/sysctl/allowlist.go +++ b/pkg/kubelet/sysctl/allowlist.go @@ -48,7 +48,7 @@ func NewAllowlist(patterns []string) (*patternAllowlist, error) { } for _, s := range patterns { - if !policyvalidation.IsValidSysctlPattern(s, true) { + if !policyvalidation.IsValidSysctlPattern(s) { return nil, fmt.Errorf("sysctl %q must have at most %d characters and match regex %s", s, validation.SysctlMaxLength,