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.

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

#146 Fixing Container Security Context Logic #149

Merged
merged 8 commits into from
Jun 18, 2019

Conversation

endzyme
Copy link
Contributor

@endzyme endzyme commented Jun 13, 2019

Kubernetes rationalizes Container Security Context in conjuction with the
Pod Spec Security Context. In this scenario you can 'leave out' certain
security context settings and rely on the pod spec definition to still
set these settings for you. The RunAsNonRoot setting originally only checked
to see if the value was set at the container level, vs also checking if it
was enabled at the pod level.

I have attached the container's parent pod spec to the container validate
struct in case any other things like this arise in the future.

I have also refactored the logic for validating bool pointers, since these
can be tricky, if you want to avoid dereferences pointer issues.

Changes:

  • Added parent pod spec of container to validate certain settings which affect container spec
  • Refactored the logic statements for validating bool pointers (used helpers)
  • Added tests for this pod.container.securityContext condition

Kubernetes rationalizes Container Security Context in conjuction with the
Pod Spec Security Context. In this scenario you can 'leave out' certain
security context settings and rely on the pod spec definition to still
set these settings for you. The RunAsNonRoot setting originally only checked
to see if the value was set at the container level, vs also checking if it
was enabled at the pod level.

I have attached the container's parent pod spec to the container validate
struct in case any other things like this arise in the future.

I have also refactored the logic for validating bool pointers, since these
can be tricky, if you want to avoid dereferences pointer issues.

Changes:
- Added parent pod spec of container to validate certain settings which affect container spec
- Refactored the logic statements for validating bool pointers (used helpers)
- Added tests for this pod.container.securityContext condition
@endzyme endzyme added the bug Something isn't working label Jun 13, 2019
@endzyme endzyme requested review from robscott and rbren June 13, 2019 23:39
@endzyme endzyme self-assigned this Jun 13, 2019
@endzyme endzyme requested a review from kimschles June 13, 2019 23:39
@endzyme
Copy link
Contributor Author

endzyme commented Jun 13, 2019

I'm interested if anyone has any other ideas on how to solve this issue of podSpec affecting unset values in containers. Another potential way to go about this is to check each container (while in the pod validation) if the security-context is set to false || nil at the pod level.

@endzyme endzyme mentioned this pull request Jun 13, 2019
Nick Huanca added 2 commits June 13, 2019 18:09
Had to adjust the logic to test more conditions than just if _either_ one was true. A condition came up
where PodSpec was True and Container was explicitly False (which caused a false positive good grade).
@endzyme
Copy link
Contributor Author

endzyme commented Jun 14, 2019

FYI I'm still missing the CHANGELOG additions (when this eventually is ready)

pkg/validator/pod.go Outdated Show resolved Hide resolved
@rbren
Copy link
Contributor

rbren commented Jun 14, 2019

This is awesome, thanks for chasing it down! Tests look solid.

I looked a bit at SecurityContext vs PodSecurityContext, and this seems to be the only variable that's shared between the two.

Re: changelog, I've just been updating at release time (since we don't know the eventual version number). You're welcome to start an "In Progress" section though.

Other than the comment on bool helpers this LGTM.

@endzyme endzyme marked this pull request as ready for review June 18, 2019 15:09
pkg/validator/container.go Show resolved Hide resolved
pkg/validator/container.go Show resolved Hide resolved
Was missing a test case where podspec was set as a good value
but the container spec is explicitly a bad setting. The previous
code would return a 'good' rating, which would be incorrect.
@endzyme endzyme merged commit 4c7429e into master Jun 18, 2019
@endzyme endzyme deleted the nh/adjust-pod-security-validation branch June 18, 2019 17:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants