From 0574a8181de2438affef15e9dc7c42a49b10198c Mon Sep 17 00:00:00 2001 From: Tim Allclair Date: Wed, 5 Oct 2022 17:17:47 -0700 Subject: [PATCH 1/2] [PSA] default warn to enforce level --- .../admission/admission_test.go | 4 +- .../pod-security-admission/api/helpers.go | 15 ++ .../api/helpers_test.go | 148 ++++++++++++++++++ 3 files changed, 165 insertions(+), 2 deletions(-) diff --git a/staging/src/k8s.io/pod-security-admission/admission/admission_test.go b/staging/src/k8s.io/pod-security-admission/admission/admission_test.go index 6d20569b07c..c0412b6d930 100644 --- a/staging/src/k8s.io/pod-security-admission/admission/admission_test.go +++ b/staging/src/k8s.io/pod-security-admission/admission/admission_test.go @@ -278,7 +278,7 @@ func TestValidateNamespace(t *testing.T) { expectAllowed: true, expectListPods: false, expectWarnings: []string{ - `namespace "test" is exempt from Pod Security, and the policy (enforce=restricted:latest) will be ignored`, + `namespace "test" is exempt from Pod Security, and the policy (enforce=restricted:latest, warn=restricted:latest) will be ignored`, }, }, { @@ -1180,7 +1180,7 @@ func TestExemptNamespaceWarning(t *testing.T) { api.EnforceLevelLabel: string(api.LevelBaseline), }, expectWarning: true, - expectWarningContains: "(enforce=baseline:latest)", + expectWarningContains: "(enforce=baseline:latest, warn=baseline:latest)", }, { name: "warn-on-warn", labels: map[string]string{ diff --git a/staging/src/k8s.io/pod-security-admission/api/helpers.go b/staging/src/k8s.io/pod-security-admission/api/helpers.go index 67aa83e5f0f..200a82cdab0 100644 --- a/staging/src/k8s.io/pod-security-admission/api/helpers.go +++ b/staging/src/k8s.io/pod-security-admission/api/helpers.go @@ -213,12 +213,16 @@ func PolicyToEvaluate(labels map[string]string, defaults Policy) (Policy, field. errs field.ErrorList p = defaults + + hasEnforceLevel bool + hasWarnLevel, hasWarnVersion bool ) if len(labels) == 0 { return p, nil } if level, ok := labels[EnforceLevelLabel]; ok { p.Enforce.Level, err = ParseLevel(level) + hasEnforceLevel = (err == nil) // Don't default warn in case of error errs = appendErr(errs, err, EnforceLevelLabel, level) } if version, ok := labels[EnforceVersionLabel]; ok { @@ -237,6 +241,7 @@ func PolicyToEvaluate(labels map[string]string, defaults Policy) (Policy, field. errs = appendErr(errs, err, AuditVersionLabel, version) } if level, ok := labels[WarnLevelLabel]; ok { + hasWarnLevel = true p.Warn.Level, err = ParseLevel(level) errs = appendErr(errs, err, WarnLevelLabel, level) if err != nil { @@ -244,9 +249,19 @@ func PolicyToEvaluate(labels map[string]string, defaults Policy) (Policy, field. } } if version, ok := labels[WarnVersionLabel]; ok { + hasWarnVersion = true p.Warn.Version, err = ParseVersion(version) errs = appendErr(errs, err, WarnVersionLabel, version) } + + // Default warn to the enforce level when explicitly set to a more restrictive level. + if !hasWarnLevel && hasEnforceLevel && CompareLevels(p.Enforce.Level, p.Warn.Level) > 0 { + p.Warn.Level = p.Enforce.Level + if !hasWarnVersion { + p.Warn.Version = p.Enforce.Version + } + } + return p, errs } diff --git a/staging/src/k8s.io/pod-security-admission/api/helpers_test.go b/staging/src/k8s.io/pod-security-admission/api/helpers_test.go index 09a0b2a9028..7c22d2438bf 100644 --- a/staging/src/k8s.io/pod-security-admission/api/helpers_test.go +++ b/staging/src/k8s.io/pod-security-admission/api/helpers_test.go @@ -117,3 +117,151 @@ func TestPolicyEquals(t *testing.T) { assert.True(t, baseline.Equivalent(&baseline), "baseline policy equals itself") assert.False(t, privileged.Equivalent(&baseline), "privileged != baseline") } + +func TestPolicyToEvaluate(t *testing.T) { + privilegedLV := LevelVersion{ + Level: LevelPrivileged, + Version: LatestVersion(), + } + privilegedPolicy := Policy{ + Enforce: privilegedLV, + Warn: privilegedLV, + Audit: privilegedLV, + } + + type testcase struct { + desc string + labels map[string]string + defaults Policy + expect Policy + expectErr bool + } + + tests := []testcase{{ + desc: "simple enforce", + labels: makeLabels("enforce", "baseline"), + expect: Policy{ + Enforce: LevelVersion{LevelBaseline, LatestVersion()}, + Warn: LevelVersion{LevelBaseline, LatestVersion()}, + Audit: privilegedLV, + }, + }, { + desc: "simple warn", + labels: makeLabels("warn", "restricted"), + expect: Policy{ + Enforce: privilegedLV, + Warn: LevelVersion{LevelRestricted, LatestVersion()}, + Audit: privilegedLV, + }, + }, { + desc: "simple audit", + labels: makeLabels("audit", "baseline"), + expect: Policy{ + Enforce: privilegedLV, + Warn: privilegedLV, + Audit: LevelVersion{LevelBaseline, LatestVersion()}, + }, + }, { + desc: "enforce & warn", + labels: makeLabels("enforce", "baseline", "warn", "restricted"), + expect: Policy{ + Enforce: LevelVersion{LevelBaseline, LatestVersion()}, + Warn: LevelVersion{LevelRestricted, LatestVersion()}, + Audit: privilegedLV, + }, + }, { + desc: "enforce version", + labels: makeLabels("enforce", "baseline", "enforce-version", "v1.22"), + expect: Policy{ + Enforce: LevelVersion{LevelBaseline, MajorMinorVersion(1, 22)}, + Warn: LevelVersion{LevelBaseline, MajorMinorVersion(1, 22)}, + Audit: privilegedLV, + }, + }, { + desc: "enforce version & warn-version", + labels: makeLabels("enforce", "baseline", "enforce-version", "v1.22", "warn-version", "latest"), + expect: Policy{ + Enforce: LevelVersion{LevelBaseline, MajorMinorVersion(1, 22)}, + Warn: LevelVersion{LevelBaseline, LatestVersion()}, + Audit: privilegedLV, + }, + }, { + desc: "enforce & warn-version", + labels: makeLabels("enforce", "baseline", "warn-version", "v1.23"), + expect: Policy{ + Enforce: LevelVersion{LevelBaseline, LatestVersion()}, + Warn: LevelVersion{LevelBaseline, MajorMinorVersion(1, 23)}, + Audit: privilegedLV, + }, + }, { + desc: "fully specd", + labels: makeLabels( + "enforce", "baseline", "enforce-version", "v1.20", + "warn", "restricted", "warn-version", "v1.21", + "audit", "restricted", "audit-version", "v1.22"), + expect: Policy{ + Enforce: LevelVersion{LevelBaseline, MajorMinorVersion(1, 20)}, + Warn: LevelVersion{LevelRestricted, MajorMinorVersion(1, 21)}, + Audit: LevelVersion{LevelRestricted, MajorMinorVersion(1, 22)}, + }, + }, { + desc: "enforce no warn", + labels: makeLabels("enforce", "baseline", "warn", "privileged"), + expect: Policy{ + Enforce: LevelVersion{LevelBaseline, LatestVersion()}, + Warn: privilegedLV, + Audit: privilegedLV, + }, + }, { + desc: "enforce warn error", + labels: makeLabels("enforce", "baseline", "warn", "foo"), + expect: Policy{ + Enforce: LevelVersion{LevelBaseline, LatestVersion()}, + Warn: privilegedLV, + Audit: privilegedLV, + }, + expectErr: true, + }, { + desc: "enforce error", + labels: makeLabels("enforce", "foo"), + expect: Policy{ + Enforce: LevelVersion{LevelRestricted, LatestVersion()}, + Warn: privilegedLV, + Audit: privilegedLV, + }, + expectErr: true, + }} + + for _, test := range tests { + t.Run(test.desc, func(t *testing.T) { + if test.defaults == (Policy{}) { + test.defaults = privilegedPolicy + } + + actual, errs := PolicyToEvaluate(test.labels, test.defaults) + if test.expectErr { + assert.Error(t, errs.ToAggregate()) + } else { + assert.NoError(t, errs.ToAggregate()) + } + + assert.Equal(t, test.expect, actual) + }) + } +} + +// makeLabels turns the kev-value pairs into a labels map[string]string. +func makeLabels(kvs ...string) map[string]string { + if len(kvs)%2 != 0 { + panic("makeLabels called with mismatched key-values") + } + labels := map[string]string{ + "other-label": "foo-bar", + } + for i := 0; i < len(kvs); i += 2 { + key, value := kvs[i], kvs[i+1] + key = labelPrefix + key + labels[key] = value + } + return labels +} From 153bc2ef1a1a8d0982494d380661896ec64b154e Mon Sep 17 00:00:00 2001 From: Tim Allclair Date: Wed, 2 Nov 2022 09:27:56 -0700 Subject: [PATCH 2/2] [PodSecurity] only include explicitly set modes in exempt namespace warning --- .../admission/admission.go | 37 +++++++++++++++++-- .../admission/admission_test.go | 8 ++-- .../pod-security-admission/api/helpers.go | 29 --------------- 3 files changed, 37 insertions(+), 37 deletions(-) diff --git a/staging/src/k8s.io/pod-security-admission/admission/admission.go b/staging/src/k8s.io/pod-security-admission/admission/admission.go index d6450a429f4..fc36f0530c7 100644 --- a/staging/src/k8s.io/pod-security-admission/admission/admission.go +++ b/staging/src/k8s.io/pod-security-admission/admission/admission.go @@ -21,6 +21,7 @@ import ( "fmt" "reflect" "sort" + "strings" "time" "k8s.io/klog/v2" @@ -250,7 +251,7 @@ func (a *Admission) ValidateNamespace(ctx context.Context, attrs api.Attributes) return invalidResponse(attrs, newErrs) } if a.exemptNamespace(attrs.GetNamespace()) { - if warning := a.exemptNamespaceWarning(namespace.Name, newPolicy); warning != "" { + if warning := a.exemptNamespaceWarning(namespace.Name, newPolicy, namespace.Labels); warning != "" { response := allowedResponse() response.Warnings = append(response.Warnings, warning) return response @@ -293,7 +294,7 @@ func (a *Admission) ValidateNamespace(ctx context.Context, attrs api.Attributes) return sharedAllowedResponse } if a.exemptNamespace(attrs.GetNamespace()) { - if warning := a.exemptNamespaceWarning(namespace.Name, newPolicy); warning != "" { + if warning := a.exemptNamespaceWarning(namespace.Name, newPolicy, namespace.Labels); warning != "" { response := allowedResponse() response.Warnings = append(response.Warnings, warning) return response @@ -736,11 +737,39 @@ func containsString(needle string, haystack []string) bool { // exemptNamespaceWarning returns a non-empty warning message if the exempt namespace has a // non-privileged policy and sets pod security labels. -func (a *Admission) exemptNamespaceWarning(exemptNamespace string, policy api.Policy) string { +func (a *Admission) exemptNamespaceWarning(exemptNamespace string, policy api.Policy, nsLabels map[string]string) string { if policy.FullyPrivileged() || policy.Equivalent(&a.defaultPolicy) { return "" } + // Build a compact representation of the policy, only printing non-privileged modes that have + // been explicitly set. + sb := strings.Builder{} + _, hasEnforceLevel := nsLabels[api.EnforceLevelLabel] + _, hasEnforceVersion := nsLabels[api.EnforceVersionLabel] + if policy.Enforce.Level != api.LevelPrivileged && (hasEnforceLevel || hasEnforceVersion) { + sb.WriteString("enforce=") + sb.WriteString(policy.Enforce.String()) + } + _, hasAuditLevel := nsLabels[api.AuditLevelLabel] + _, hasAuditVersion := nsLabels[api.AuditVersionLabel] + if policy.Audit.Level != api.LevelPrivileged && (hasAuditLevel || hasAuditVersion) { + if sb.Len() > 0 { + sb.WriteString(", ") + } + sb.WriteString("audit=") + sb.WriteString(policy.Audit.String()) + } + _, hasWarnLevel := nsLabels[api.WarnLevelLabel] + _, hasWarnVersion := nsLabels[api.WarnVersionLabel] + if policy.Warn.Level != api.LevelPrivileged && (hasWarnLevel || hasWarnVersion) { + if sb.Len() > 0 { + sb.WriteString(", ") + } + sb.WriteString("warn=") + sb.WriteString(policy.Warn.String()) + } + return fmt.Sprintf("namespace %q is exempt from Pod Security, and the policy (%s) will be ignored", - exemptNamespace, policy.CompactString()) + exemptNamespace, sb.String()) } diff --git a/staging/src/k8s.io/pod-security-admission/admission/admission_test.go b/staging/src/k8s.io/pod-security-admission/admission/admission_test.go index c0412b6d930..26bf4f3ab01 100644 --- a/staging/src/k8s.io/pod-security-admission/admission/admission_test.go +++ b/staging/src/k8s.io/pod-security-admission/admission/admission_test.go @@ -278,7 +278,7 @@ func TestValidateNamespace(t *testing.T) { expectAllowed: true, expectListPods: false, expectWarnings: []string{ - `namespace "test" is exempt from Pod Security, and the policy (enforce=restricted:latest, warn=restricted:latest) will be ignored`, + `namespace "test" is exempt from Pod Security, and the policy (enforce=restricted:latest) will be ignored`, }, }, { @@ -1180,7 +1180,7 @@ func TestExemptNamespaceWarning(t *testing.T) { api.EnforceLevelLabel: string(api.LevelBaseline), }, expectWarning: true, - expectWarningContains: "(enforce=baseline:latest, warn=baseline:latest)", + expectWarningContains: "(enforce=baseline:latest)", }, { name: "warn-on-warn", labels: map[string]string{ @@ -1206,7 +1206,7 @@ func TestExemptNamespaceWarning(t *testing.T) { }, defaultPolicy: baselinePolicy, expectWarning: true, - expectWarningContains: "(enforce=baseline:v1.23, audit=baseline:v1.23, warn=baseline:latest)", + expectWarningContains: "(warn=baseline:latest)", }} const ( @@ -1228,7 +1228,7 @@ func TestExemptNamespaceWarning(t *testing.T) { policy, err := api.PolicyToEvaluate(labels, defaultPolicy) require.NoError(t, err.ToAggregate()) - warning := a.exemptNamespaceWarning(test.name, policy) + warning := a.exemptNamespaceWarning(test.name, policy, labels) if !test.expectWarning { assert.Empty(t, warning) return diff --git a/staging/src/k8s.io/pod-security-admission/api/helpers.go b/staging/src/k8s.io/pod-security-admission/api/helpers.go index 200a82cdab0..706b6b36b20 100644 --- a/staging/src/k8s.io/pod-security-admission/api/helpers.go +++ b/staging/src/k8s.io/pod-security-admission/api/helpers.go @@ -161,35 +161,6 @@ func (p *Policy) String() string { return fmt.Sprintf("enforce=%#v, audit=%#v, warn=%#v", p.Enforce, p.Audit, p.Warn) } -// CompactString prints a minimalist representation of the policy that excludes any privileged -// levels. -func (p *Policy) CompactString() string { - sb := strings.Builder{} - if p.Enforce.Level != LevelPrivileged { - sb.WriteString("enforce=") - sb.WriteString(p.Enforce.String()) - } - if p.Audit.Level != LevelPrivileged { - if sb.Len() > 0 { - sb.WriteString(", ") - } - sb.WriteString("audit=") - sb.WriteString(p.Audit.String()) - } - if p.Warn.Level != LevelPrivileged { - if sb.Len() > 0 { - sb.WriteString(", ") - } - sb.WriteString("warn=") - sb.WriteString(p.Warn.String()) - } - if sb.Len() == 0 { - // All modes were privileged, just output "privileged". - return string(LevelPrivileged) - } - return sb.String() -} - // Equivalent determines whether two policies are functionally equivalent. Policies are considered // equivalent if all 3 modes are considered equivalent. func (p *Policy) Equivalent(other *Policy) bool {