Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

PodSecurity message/check/fixture cleanups #103558

Merged
merged 29 commits into from Jul 8, 2021
Merged

PodSecurity message/check/fixture cleanups #103558

merged 29 commits into from Jul 8, 2021

Conversation

liggitt
Copy link
Member

@liggitt liggitt commented Jul 7, 2021 •
edited

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
  • 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:

  • allowPrivilegeEscalation
  • appArmorProfile
  • addCapabilities → capabilities_baseline
  • dropCapabilities → capabilities_restricted
  • hostNamespaces
  • hostPath → hostPathVolumes
  • hostPorts

Unreviewed:

checks:

  • hostProcess → windowsHostProcess
  • privileged
  • procMount
  • restrictedVolumes
  • runAsNonRoot
  • seccomp_restricted → seccompProfile_restricted
  • seccomp_baseline → seccompProfile_baseline
  • sysctls
  • selinux → seLinuxOptions

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 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 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
@k8s-ci-robot
Copy link
Contributor

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

@k8s-ci-robot k8s-ci-robot added kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. kind/feature Categorizes issue or PR as related to a new feature. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jul 7, 2021
@liggitt liggitt added this to In Review in SIG-Auth: PodSecurity via automation Jul 7, 2021
@liggitt liggitt added this to the v1.22 milestone Jul 7, 2021
@k8s-ci-robot 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 liggitt force-pushed the podsecurity-check-cleanup branch 3 times, most recently from 7dd2693 to 0c4ad8d Compare July 7, 2021 23:27
tallclair
tallclair reviewed Jul 8, 2021
View reviewed changes
Copy link
Member

@tallclair tallclair left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm so far

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 liggitt force-pushed the podsecurity-check-cleanup branch 3 times, most recently from 5b6522c to 8678f6f Compare July 8, 2021 04:45
@liggitt liggitt marked this pull request as ready for review July 8, 2021 05:25
@liggitt liggitt changed the title WIP - podsecurity check cleanups PodSecurity check/fixture cleanups Jul 8, 2021
@k8s-ci-robot 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
PodSecurity: add message helper
648b970
@liggitt
PodSecurity: allowPrivilegeEscalation: cleanup
1e28863 
Make forbidden details more compact
Add unit test exercising forbidden message/details
Consolidate fixtures
liggitt added 16 commits July 8, 2021 02:19
@liggitt
PodSecurity: windowsHostProcess: regenerate files
7c70467
@liggitt
PodSecurity: privileged: cleanup
f9b8dfd
@liggitt
PodSecurity: procMount: cleanup
4a69c57
@liggitt
PodSecurity: restrictedVolumes: cleanup
676240a 
Updated forbidden reason/details
Added unit test to exercise all volume types
Consolidated fixtures
@liggitt
PodSecurity: restrictedVolumes: regenerate files
edb7cdb
@liggitt
PodSecurity: runAsNonRoot: cleanup
5fc0659 
Improve message and details
Add unit tests
Consolidate integration test fixtures
@liggitt
PodSecurity: runAsNonRoot: regenerate files
43146d4
@liggitt
PodSecurity: seccompProfile_restricted: cleanup
88a1241 
Switch from field paths to container names in messages
Add unit tests for messages
Consolidate integration test fixtures
@liggitt
PodSecurity: seccompProfile_restricted: regenerate files
2af08d1
@liggitt
PodSecurity: seccompProfile_baseline: cleanup
bebf612 
Make messages consistent
Add unit tests for messages
Consolidate integration test fixtures
Rename to seccompProfile_baseline
@liggitt
PodSecurity: seccompProfile_baseline: regenerate files
e178695
@liggitt
PodSecurity: sysctls: cleanup
19c8ab2 
Add documentation and unit tests for messages
@liggitt
PodSecurity: seLinuxOptions: cleanup
d541970 
rename to seLinuxOptions
make message consistent
add unit tests for message
consolidate integration test fixtures
@liggitt
PodSecurity: seLinuxOptions: regenerate files
7895399
@liggitt
PodSecurity: Drop field path from container visitor
36907db
@liggitt
limit warnings to requests that would otherwise succeed, reformat war…
7f9d2ed 
…ning message
@liggitt liggitt force-pushed the podsecurity-check-cleanup branch from 960a7b2 to 7f9d2ed Compare July 8, 2021 06:19
@liggitt liggitt changed the title PodSecurity check/fixture cleanups PodSecurity message/check/fixture cleanups Jul 8, 2021
tallclair
tallclair approved these changes Jul 8, 2021
View reviewed changes
Copy link
Member

@tallclair tallclair left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

staging/src/k8s.io/pod-security-admission/policy/check_restrictedVolumes.go
badVolumeTypes.Insert("storageos")
default:
badVolumeTypes.Insert("unknown")
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

[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:

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

@liggitt 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 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
@liggitt liggitt moved this from In Review to Done in SIG-Auth: PodSecurity Jul 8, 2021
@k8s-ci-robot k8s-ci-robot merged commit 72283f1 into kubernetes:master Jul 8, 2021
@liggitt liggitt mentioned this pull request Jul 8, 2021
Merged
@liggitt liggitt deleted the podsecurity-check-cleanup branch July 8, 2021 22:46
@liggitt liggitt mentioned this pull request Jul 10, 2021
Closed
12 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tallclair tallclair tallclair approved these changes

@deads2k deads2k Awaiting requested review from deads2k

@krmayankk krmayankk Awaiting requested review from krmayankk

Assignees

@tallclair tallclair

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. kind/feature Categorizes issue or PR as related to a new feature. lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. release-note-none Denotes a PR that doesn't merit a release note. sig/auth Categorizes an issue or PR as relevant to SIG Auth. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
SIG-Auth: PodSecurity
Done (1.22, Alpha)
Milestone
v1.22
Development

Successfully merging this pull request may close these issues.

[PodSecurity] make name/reason/details messages consistent across checks
3 participants
@liggitt @k8s-ci-robot @tallclair