PodSecurity message/check/fixture cleanups #103558

liggitt · 2021-07-07T21:31:33Z

What type of PR is this?

/kind cleanup
/kind feature

Which issue(s) this PR fixes:

Fixes #103373

Best reviewed commit-by-commit. Alternate commits are updating generated fixtures.

Address the following:

Consistency of check IDs
Make reason consistent (and consider exporting as a constant)
Make details consistent
- e.g. [PodSecurity] hostProcess baseline check #103382 (comment)
Ensure fixtures balance count/coverage (especially with variants covering initContainers)
- Reduced from 3966 to 2553
Comment blocks capturing the current policy (take it or leave it, but should be consistent)
Usage of sets over slices (sets shuffle the order, and are less efficient when de-duplication isn't needed)

Review pass 1 (#103558 (review)):

checks:

Unreviewed:

checks:

cleanup: drop field path from container visitor

limit warning evaluation/surfacing to requests that are not rejected by an enforce policy

reformat the warning message to read more naturally when admitting pods or pod-controllers

Follow-up:

Avoid duplicate messages from overlapping checks (tracked in [PodSecurity] Improve error UX #103561)
- restricted volumes complains about hostPath volumes already caught by baseline hostPath check
- restricted capabilities complains about added non-default capabilities already caught by baseline capabilities check
- restricted seccomp complains about unconfined seccomp profile already caught by baseline seccomp check

Does this PR introduce a user-facing change?

NONE

/assign @tallclair

k8s-ci-robot · 2021-07-07T21:31:35Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

tallclair

lgtm so far

staging/src/k8s.io/pod-security-admission/policy/check_capabilities_restricted.go

staging/src/k8s.io/pod-security-admission/policy/check_apparmor.go

Make forbidden details more compact Add unit test exercising forbidden message/details Consolidate fixtures

Rename to windowsHostProcess Format reason/details Add unit tests

Updated forbidden reason/details Added unit test to exercise all volume types Consolidated fixtures

Improve message and details Add unit tests Consolidate integration test fixtures

Switch from field paths to container names in messages Add unit tests for messages Consolidate integration test fixtures

Make messages consistent Add unit tests for messages Consolidate integration test fixtures Rename to seccompProfile_baseline

Add documentation and unit tests for messages

rename to seLinuxOptions make message consistent add unit tests for message consolidate integration test fixtures

…ning message

tallclair

/lgtm

tallclair · 2021-07-08T16:18:33Z

staging/src/k8s.io/pod-security-admission/policy/check_restrictedVolumes.go

+				badVolumeTypes.Insert("storageos")
+			default:
+				badVolumeTypes.Insert("unknown")
+			}


I suppose we shouldn't depend on PSP code? We might want to move

kubernetes/pkg/security/podsecuritypolicy/util/util.go

Line 81 in 24a614c

func GetVolumeFSType(v api.Volume) (policy.FSType, error) {

to a common location.

I don't want to add any new dependencies on that code... looking forward to deleting it in 1.25

k8s-ci-robot · 2021-07-08T16:36:26Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: liggitt, tallclair

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~staging/src/k8s.io/pod-security-admission/OWNERS~~ [liggitt,tallclair]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

k8s-ci-robot added the release-note-none Denotes a PR that doesn't merit a release note. label Jul 7, 2021

k8s-ci-robot assigned tallclair Jul 7, 2021

k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 7, 2021

liggitt added this to the v1.22 milestone Jul 7, 2021

k8s-ci-robot requested review from deads2k and krmayankk July 7, 2021 21:32

k8s-ci-robot added sig/auth Categorizes an issue or PR as relevant to SIG Auth. approved Indicates a PR has been approved by an approver from all required OWNERS files. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Jul 7, 2021

liggitt force-pushed the podsecurity-check-cleanup branch 3 times, most recently from 7dd2693 to 0c4ad8d Compare July 7, 2021 23:27

tallclair reviewed Jul 8, 2021

View reviewed changes

staging/src/k8s.io/pod-security-admission/policy/check_capabilities_restricted.go Outdated Show resolved Hide resolved

staging/src/k8s.io/pod-security-admission/policy/check_apparmor.go Outdated Show resolved Hide resolved

liggitt force-pushed the podsecurity-check-cleanup branch 3 times, most recently from 5b6522c to 8678f6f Compare July 8, 2021 04:45

liggitt marked this pull request as ready for review July 8, 2021 05:25

liggitt changed the title ~~WIP - podsecurity check cleanups~~ PodSecurity check/fixture cleanups Jul 8, 2021

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 8, 2021

liggitt added 3 commits July 8, 2021 02:19

PodSecurity: add message helper

648b970

PodSecurity: allowPrivilegeEscalation: cleanup

1e28863

Make forbidden details more compact Add unit test exercising forbidden message/details Consolidate fixtures

PodSecurity: allowPrivilegeEscalation: regenerate files

8291f84

liggitt added 17 commits July 8, 2021 02:19

PodSecurity: windowsHostProcess: cleanup

9dce1d6

Rename to windowsHostProcess Format reason/details Add unit tests

PodSecurity: windowsHostProcess: regenerate files

7c70467

PodSecurity: privileged: cleanup

f9b8dfd

PodSecurity: procMount: cleanup

4a69c57

PodSecurity: restrictedVolumes: cleanup

676240a

Updated forbidden reason/details Added unit test to exercise all volume types Consolidated fixtures

PodSecurity: restrictedVolumes: regenerate files

edb7cdb

PodSecurity: runAsNonRoot: cleanup

5fc0659

Improve message and details Add unit tests Consolidate integration test fixtures

PodSecurity: runAsNonRoot: regenerate files

43146d4

PodSecurity: seccompProfile_restricted: cleanup

88a1241

Switch from field paths to container names in messages Add unit tests for messages Consolidate integration test fixtures

PodSecurity: seccompProfile_restricted: regenerate files

2af08d1

PodSecurity: seccompProfile_baseline: cleanup

bebf612

Make messages consistent Add unit tests for messages Consolidate integration test fixtures Rename to seccompProfile_baseline

PodSecurity: seccompProfile_baseline: regenerate files

e178695

PodSecurity: sysctls: cleanup

19c8ab2

Add documentation and unit tests for messages

PodSecurity: seLinuxOptions: cleanup

d541970

rename to seLinuxOptions make message consistent add unit tests for message consolidate integration test fixtures

PodSecurity: seLinuxOptions: regenerate files

7895399

PodSecurity: Drop field path from container visitor

36907db

limit warnings to requests that would otherwise succeed, reformat war…

7f9d2ed

…ning message

liggitt force-pushed the podsecurity-check-cleanup branch from 960a7b2 to 7f9d2ed Compare July 8, 2021 06:19

liggitt changed the title ~~PodSecurity check/fixture cleanups~~ PodSecurity message/check/fixture cleanups Jul 8, 2021

tallclair approved these changes Jul 8, 2021

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 8, 2021

liggitt added priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. triage/accepted Indicates an issue or PR is ready to be actively worked on. labels Jul 8, 2021

k8s-ci-robot removed needs-priority Indicates a PR lacks a `priority/foo` label and requires one. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Jul 8, 2021

k8s-ci-robot merged commit 72283f1 into kubernetes:master Jul 8, 2021

liggitt mentioned this pull request Jul 8, 2021

[test-only][PodSecurity] Add test coverage for pod-template-containing objects #103452

Merged

liggitt deleted the podsecurity-check-cleanup branch July 8, 2021 22:46

liggitt mentioned this pull request Jul 10, 2021

PodSecurity admission (PodSecurityPolicy replacement) kubernetes/enhancements#2579

Closed

12 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PodSecurity message/check/fixture cleanups #103558

PodSecurity message/check/fixture cleanups #103558

liggitt commented Jul 7, 2021 •

edited

Loading

k8s-ci-robot commented Jul 7, 2021

tallclair left a comment

tallclair left a comment

tallclair Jul 8, 2021

liggitt Jul 8, 2021

k8s-ci-robot commented Jul 8, 2021

PodSecurity message/check/fixture cleanups #103558

PodSecurity message/check/fixture cleanups #103558

Conversation

liggitt commented Jul 7, 2021 • edited Loading

What type of PR is this?

Which issue(s) this PR fixes:

Does this PR introduce a user-facing change?

k8s-ci-robot commented Jul 7, 2021

tallclair left a comment

Choose a reason for hiding this comment

tallclair left a comment

Choose a reason for hiding this comment

tallclair Jul 8, 2021

Choose a reason for hiding this comment

liggitt Jul 8, 2021

Choose a reason for hiding this comment

k8s-ci-robot commented Jul 8, 2021

liggitt commented Jul 7, 2021 •

edited

Loading