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

Promote networkpolicies out of /contrib #2385

Closed
juliusvonkohout opened this issue Feb 9, 2023 · 11 comments · Fixed by #2457
Closed

Promote networkpolicies out of /contrib #2385

juliusvonkohout opened this issue Feb 9, 2023 · 11 comments · Fixed by #2457

Comments

@juliusvonkohout
Copy link
Member

Follow up of #2311 (comment)

@kimwnasptd @annajung @TobiasGoerke

Yes, we should promote it even for 1.7. It is a second security barrier and best practice. It prevented some of the latest security flaws that we had in Kubeflow with missing sidecars or before that the unprotected KFAM.

@kimwnasptd common/networkpolicies is the right place and we should include it in the example installation file https://github.com/kubeflow/manifests/blob/master/example/kustomization.yaml

It would also make sense to add me to the root owner file then, since istio-cni is the next topic.in /common

@kimwnasptd
Copy link
Member

Thanks for creating the issue!

Before deciding on promoting them to be added as an extra layer in Kubeflow I'd like to understand if they offer something that we currently can't achieve with Istio. In both the above cases for the missing sidecar and the unprotected KFAM the issue was with an incorrect misconfiguration in Istio's auth mechanism (AuthorizationPolicies and sidecars).

Personally for Notebooks WG I'd like to avoid adding another layer of complexity in the KF stack, which will add a small burden on debugging and maintaining this logic, if this can already be achieved with Istio.

In my mind securing who can talk to who is the responsibility of the service mesh, that's why I keep on bringing Istio. Is there a scenario that we currently can't secure with Istio and for which we need explicitly NetworkPolicies?

let's also bring this to the attention of the rest of the WG leads, since such a change would affect other WGs as well
cc @andreyvelich @johnugeorge @zijianjoy

@juliusvonkohout
Copy link
Member Author

@kimwnasptd i do not think that all components support istio yet and it is also useful for isolating user namespaces where sometimes not everything uses istio. If we get to a perfect world in 2 years and really do not need it anymore we can always think about deprecation. Also for it administrators that have to specify network flows and satisfy enterprise guidelines it comes in quite handy. Furthermore i do not see debugging problems and it is used by some other companies as well. I can maintain it for the next years, although there is not much to do.

@juliusvonkohout
Copy link
Member Author

juliusvonkohout commented Apr 12, 2023

And again something that would have been prevented by networkpolicies kubeflow/kubeflow#7032. In the long term i also want to protect the usernamespaces with networkpolicies as done in another PR.

@rawc0der
Copy link
Member

Hello! 👋
Do you think @kimwnasptd that migrating the networkpolicies into /common might be in scope for #2456?

@kimwnasptd
Copy link
Member

@rawc0der I think there are still missing details we need to define before merging #2456

There are 2 questions that I think are not clear regarding this:

  1. Why do we need an extra component when we can achieve the same functionality with Istio?
  2. Moving this in /common means that then it's up to the Manifests WG to maintain an extra layer that should be added on top of all WGs. Shouldn't this change be pushed in WGs instead?

@juliusvonkohout
Copy link
Member Author

For 1. we could list the exploits so far that have been or would have been prevented by networkpolicies in the kubeflow and or user namespaces.
Furthermore there are some workloads which do not support istio yet. It also provides a proper network layer description in one place. So far i know of 2 critical and 2 high CVEs

For 2. I think the security related stuff has to be mostly in one place. This is also valid for podsecuritypolicies / standards. It would take years to get this into all WGs anyway. But if this is your long term goal we should have it in manifests and migrate it step by step in the WGs.

@rawc0der
Copy link
Member

Hi @kimwnasptd!

  1. I want to point out that Istio although a dependency to Kubeflow is not Kubernetes native and adding some native guardrails is not actually a bad thing.
    Imagine a structure of security-in-depth components that all add up to the entire security posture. You could consider the following analogy (Kernel & OS, where Istio AuthPol secure only the os layer and the networkpolicies secure the kernel).

  2. I tend to echo with @juliusvonkohout here. What happens to all security related resources could be better managed under one place in /common in order to have a better overview of all the layers and relationships between components.
    Things change quickly PSP get deprecated, PSS become stable, then what about adding some OPA polcies for further enforcing runtime conditions.. how would you keep track of it all with the complexity and uncertainties in individual WGs?

@kimwnasptd
Copy link
Member

Hi @rawc0der. Regarding the first point I see the following two problems:

  • Having 2 different components for achieving the same functionality seems like additional maintenance (or are corner cases that Istio doesn't handle? Can we list them as I asked above?)
  • I'm not sure I understand what you mean by Kubernetes native. What I'd say though is that for me the decision is either we (Kubeflow project) decide that we depend on Istio or not. If we do then let's use what it provides for securing the mesh, which Istio is there to configure in the first place

@kimwnasptd
Copy link
Member

Regarding the second point, that @juliusvonkohout is also talking about:

Right now the Manifests repo is just a place for gathering the components maintained by the KF WGs and providing some third-party ones for folks to deploy a whole solution. Adding NetworkPolicies to /common means that Manifests WG will have overlay logic that affects all the components. Who is then the entity responsible for this functionality? Is it the Manifests WG? Is it the WGs that are affected themselves? Is it the Security Team/WG?

We currently don't have such an agreed process right now (but, by all means let's continue the discussion to establish one). And for me this is how it should look like, for efforts need to affect all components:

  1. WGs need to approve the effort
  2. WGs will need to own and maintain the part that corresponds to them, in their repos
    • In this case, for NetworkPolicies, for Manifests WG we'll need to maintain NPs for Istio, Cert Manager, Dex etc
  3. For efforts that for implementation reasons all files need to be in one place there still needs to be a clear distinction of which entity owns what
    • Which to me again it should be each WG owning their part, not Manifests owning the corresponding part for each WG
    • What is the expectations also from the Security Team/WG for such efforts

Lastly, to @juliusvonkohout's point

It would take years to get this into all WGs anyway. But if this is your long term goal we should have it in manifests and migrate it step by step in the WGs.

I wouldn't want the manifests repo to be a place where functionality gets added because the WGs don't accept a change or discussions take too long. The bug in this case is how features are agreed and discussed in WGs. But the solution to this bug is not to have a "backdoor" for pushing efforts in a central place in order to avoid interactions with WGs and impose decisions on them.

@rawc0der
Copy link
Member

rawc0der commented Jun 28, 2023

Hi @kimwnasptd,

Thanks for raising up these points and for trying to describe the decision process from a WG Lead perspective. Decision making is tough and comes with some risk, but the community needs to figure out how to move forward and try to come to some conclusions.
Hope I can shed some light on some of the proposals, so here's my two cents:

I'm not sure I understand what you mean by Kubernetes native.

Istio authn/z enforcement is different from the Kubernetes networkpolicies in the sense that netpols are part of kuberntes in-tree api services ("native") and one should expect Stable behaviour as they are part of --> networking.k8s.io/v1 apigroup, while Istio Authpols are enabled by extension and the behaviour can change since they are part of beta apigroups --> security.istio.io/v1beta1 .
Istio security enforcement is acting on Layer 7 (HTTP requests) in envoy proxies securing traffic from a client-server perspective, while Kubernetes Netpols act as true network firewall on Layer 3/4 (IP and ports).

(or are corner cases that Istio doesn't handle? Can we list them as I asked above?)

This is something I can't answer, but because of different conditions whre security is enforced, you can have edge cases yes. This is what keeps some people up at night :D.

Adding NetworkPolicies to /common means that Manifests WG will have overlay logic that affects all the components. Who is then the entity responsible for this functionality? Is it the Manifests WG? Is it the WGs that are affected themselves? Is it the Security Team/WG?

Well, I believe since the functionality added by NP is security related the responsibility and ownership should fall under Security WG Team (once process, official channels and core members are established). Until then seems that Manifest WG responsibilities are a bit all-over the place, stretching from /contrib to /common and all corners of this repo.
Moving ownership of security related assets (NetPols, Istio Authpols, PSS, Admission Controls etc) to the Security WG Team and having a consensus of who is responsible for what should improve collaboration between WGs and also take some pressure off the Manifests WG concerns.

For efforts that for implementation reasons all files need to be in one place there still needs to be a clear distinction of which entity owns what
Which to me again it should be each WG owning their part, not Manifests owning the corresponding part for each WG
What is the expectations also from the Security Team/WG for such efforts

I strongly believe that multiple WG should commit to maintain clean in structure the same repository, having all files (kustomize manifests) in one place makes sense and corresponding WG should maintain the parts they are concerned with.

Obviously there needs to be some synergy between Security/Manifests WG - so let's keep the discussion alive to bring these ideas to life.

@juliusvonkohout
Copy link
Member Author

@andreyvelich @johnugeorge @jbottum this is probably a decision for the KSC or TOC

google-oss-prow bot pushed a commit that referenced this issue Apr 16, 2024
* Move networkpolicies out of /contrib into /common (#2385)

* extend OWNERS file

* disable seldon NP

* enable seldon NP

* add rawc0der as Reviewer

* Remove rawc0der reviewer

* remove the duplicate resource
doncorsean pushed a commit to doncorsean/kubeflow-manifests that referenced this issue Jul 18, 2024
…beflow#2457)

* Move networkpolicies out of /contrib into /common (kubeflow#2385)

* extend OWNERS file

* disable seldon NP

* enable seldon NP

* add rawc0der as Reviewer

* Remove rawc0der reviewer

* remove the duplicate resource
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants