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

Include policies for securing sandbox namespace #792

Conversation

CosimoMichelagnoli
Copy link
Contributor

@CosimoMichelagnoli CosimoMichelagnoli commented May 24, 2022

Description

This PR aims to apply some cluster-wide policies based on Kyverno framework. Three main security requirements:

  • Avoid creation of privileged pods
  • Avoid creation of load balancer services (i.e avoid creation of routable IPs) and nodePort
  • Force specific name for ingress hostname

These policies are applied only to sandbox namespaces.

@CosimoMichelagnoli CosimoMichelagnoli requested a review from a team as a code owner May 24, 2022 12:32
@kingmakerbot
Copy link
Collaborator

Hi @CosimoMichelagnoli. Thanks for your PR.

I am @kingmakerbot.
You can interact with me issuing a slash command in the first line of a comment.
Currently, I understand the following commands:

  • /rebase: Rebase this PR onto the master branch
  • /merge: Merge this PR into the master branch
  • /hold: Adds hold label to prevent merging with /merge
  • /unhold: Removes the hold label to allow merging with /merge
  • /deploy-staging: Deploy a staging environment to test this PR (the build-all flag enables user environments building)
  • /undeploy-staging: Manually undeploy the staging environment

Make sure this PR appears in the CrownLabs changelog, adding one of the following labels:

  • kind/breaking: 💥 Breaking Change
  • kind/feature: 🚀 New Feature
  • kind/bug: 🐛 Bug Fix
  • kind/cleanup: 🧹 Code Refactoring
  • kind/docs: 📝 Documentation

@giorio94 giorio94 marked this pull request as draft May 24, 2022 17:45
@CosimoMichelagnoli CosimoMichelagnoli force-pushed the grdcom/sb-kyvernoPolicies branch 2 times, most recently from c178ff0 to ac392c0 Compare June 8, 2022 15:41
@CosimoMichelagnoli CosimoMichelagnoli force-pushed the grdcom/sb-kyvernoPolicies branch 7 times, most recently from c2952b5 to 7778f20 Compare June 14, 2022 07:06
@CosimoMichelagnoli CosimoMichelagnoli marked this pull request as ready for review June 14, 2022 07:11
@CosimoMichelagnoli CosimoMichelagnoli force-pushed the grdcom/sb-kyvernoPolicies branch 2 times, most recently from c5734a1 to 1392acd Compare June 15, 2022 16:03
Copy link
Member

@giorio94 giorio94 left a comment

Choose a reason for hiding this comment

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

A few initial comments inline. Additionally:

  • For the moment, I would personally drop the crownlabs-policies-privilege-escalation and crownlabs-policies-run-as-non-root policies, as they make sense, but require a modification of the deployment manifests (this could create problems when students experiment for the first time).
  • I would add the following policies (from the kyverno repo), as targeting definitely dangerous aspects:
  • I would keep the information about the installation of kyverno in the infrastructure/policies folder, while moving the chart with the policies in a policies folder in the root of the repo, to separate the two things (as we will also need to customize the kyverno values file when installing it on CrownLabs).

infrastructure/policies/Chart.yaml Outdated Show resolved Hide resolved
infrastructure/policies/templates/_helpers.tpl Outdated Show resolved Hide resolved
infrastructure/policies/templates/disallow_empty_host.yaml Outdated Show resolved Hide resolved
infrastructure/policies/README.md Outdated Show resolved Hide resolved
infrastructure/policies/README.md Outdated Show resolved Hide resolved
infrastructure/policies/README.md Outdated Show resolved Hide resolved
infrastructure/policies/README.md Outdated Show resolved Hide resolved
@CosimoMichelagnoli CosimoMichelagnoli force-pushed the grdcom/sb-kyvernoPolicies branch 2 times, most recently from 08ce053 to 1a703df Compare June 22, 2022 17:18
@CosimoMichelagnoli
Copy link
Contributor Author

A few initial comments inline. Additionally:

  • For the moment, I would personally drop the crownlabs-policies-privilege-escalation and crownlabs-policies-run-as-non-root policies, as they make sense, but require a modification of the deployment manifests (this could create problems when students experiment for the first time).

  • I would add the following policies (from the kyverno repo), as targeting definitely dangerous aspects:

  • I would keep the information about the installation of kyverno in the infrastructure/policies folder, while moving the chart with the policies in a policies folder in the root of the repo, to separate the two things (as we will also need to customize the kyverno values file when installing it on CrownLabs).

We just corrected the PR following your comments. Hoping to have done everything right, how should we proceed with regards to the kyverno values ​​to be customized?

@giorio94 giorio94 added the kind/feature New feature or request label Jun 24, 2022
Copy link
Member

@giorio94 giorio94 left a comment

Choose a reason for hiding this comment

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

A couple of additional comments inline. Moreover, please delete the empty values file for the moment (the one in the infrastructure folder).

infrastructure/policies/README.md Outdated Show resolved Hide resolved
policies/README.md Outdated Show resolved Hide resolved
policies/README.md Outdated Show resolved Hide resolved
deploy/crownlabs/values.yaml Outdated Show resolved Hide resolved
policies/templates/disallow-host-namespaces.yaml Outdated Show resolved Hide resolved
policies/README.md Outdated Show resolved Hide resolved
Copy link
Member

@giorio94 giorio94 left a comment

Choose a reason for hiding this comment

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

Looks almost ready to me. Just a few very minor comments inline. Additionally, you should fix the conflicts and double check all files to remove the trailing spaces at the end of the lines.

policies/README.md Outdated Show resolved Hide resolved
policies/README.md Outdated Show resolved Hide resolved
policies/README.md Outdated Show resolved Hide resolved
policies/templates/disallow-hostPath.yaml Outdated Show resolved Hide resolved
policies/templates/restrict-host-ingress.yaml Outdated Show resolved Hide resolved
policies/templates/disallow-hostPorts.yaml Outdated Show resolved Hide resolved
policies/templates/disallow-loadbalancer.yaml Outdated Show resolved Hide resolved
policies/templates/disallow-nodeport.yaml Outdated Show resolved Hide resolved
policies/templates/disallow-privileged-containers.yaml Outdated Show resolved Hide resolved
policies/templates/restrict-host-ingress.yaml Outdated Show resolved Hide resolved
@CosimoMichelagnoli CosimoMichelagnoli force-pushed the grdcom/sb-kyvernoPolicies branch 3 times, most recently from 3f88b1f to 39b87b6 Compare July 8, 2022 10:11
Copy link
Member

@giorio94 giorio94 left a comment

Choose a reason for hiding this comment

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

A couple of final nits inline. Please, generalize also the commit message, then we can go ahead and merge the PR.

policies/templates/disallow-hostPath.yaml Outdated Show resolved Hide resolved
deploy/crownlabs/values.yaml Outdated Show resolved Hide resolved
policies/templates/restrict-host-ingress.yaml Outdated Show resolved Hide resolved
Co-authored-by: Grazia D'Onghia <graziadonghia925@gmail.com>
@giorio94
Copy link
Member

/merge

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/feature New feature or request size/XL
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants