netpol: allowedIngressPorts and interNamespaceAccessLabels config added with defaults retaining 0.9.1 current behavior #1842
Conversation
consideRatio
force-pushed
the
pr/network-policies-across-namespaces
branch
from
2a36f98
to
818605c
Compare
consideRatio
changed the title
|
If you're running multiple JupyterHubs segregated by namespace you wouldn't want cross-namespace traffic. |
You still require a dedicated label on the pods in other namespaces, so, right now it is secure assuming there isn't someone in another namespace that can create pods and actively wants to attack the JupyterHub pods somehow. We would be more locked down if we actively ask users to to three things instead of one:
I think we would add too much complexity for users if we ask them to do all three parts, so they may very well end up just adding a more basic rule, like on allowing the entire namespace. - namespaceSelector:
matchLabels:
my-namespace-label: somethingI'm open to the idea of allowing this to be an opt-out option, so that one can do the 3-step solution instead, but I'd like to avoid introducing additional configuration options if possible. |
|
The labels were originally added to make the definitions of policies between JupyterHub components easier: #534 (comment) I don't think it's as useful to enforce those labels outside of the JupyterHub namespace. In general an application owner should have control over who can access a service, but this effectively hands over control to the owner of the other namespace. This is a potential problem when namespaces are run by people with different privileges, or where it's used to reduce the impact of an exploit in a service. How about if we say the labels are only intended for use within the same namespace, and for cross namespace traffic labels are irrelevant since it doesn't add much security. Then if you want to allow ingress from a namespace If the other app owner wants to limit a pod to only connect to JupyterHub they can do that by defining their own egress network policy. |
|
Do you have any examples of how other Helm charts configure network policies? For instance if all charts follow the Z2JH approach then if you had say 50 applications that need to interact with each other each of those 50 applications would need 49 annotations like What ingress did you have problems with? I've always used nginx-ingress in its own namespace without a specific exemption in the Z2JH network policy- I just assumed ingress had some sort of special access. Maybe something's changed recently? |
Hmmm I mean right now, all traffic is banned from all other namespaces, and this is a mechanism to open up communication, so unless the policy is to allow communication from all other namespaces, it doesn't become much easier other than creating a namespace label that will allow traffic for all pods within the namespace. That would make sense to add as well, because why not. If we don't have this PR merged, then I'd do this to resolve it is... Option 1proxy:
networkPolicy:
ingress:
from:
- namespaceSelector:
matchLabels: {}
podSelector:
matchLabels:
hub.jupyter.org/network-access-proxy-http: "true"Then update the nginx-ingress helm chart, like I'd do otherwise as well. controller:
podLabels:
hub.jupyter.org/network-access-proxy-http: "true"Option 2proxy:
networkPolicy:
ingress:
from:
- namespaceSelector:
matchLabels:
hub.jupyter.org/network-access-proxy-http: "true"Summary of current situationWe need to have a suggestion on how users should configure access to various jupyterhub pods that otherwise would block access from any pod outside the jupyterhub namespace. The concrete case that made me realize this, is that I had a nginx-ingress-controller in another k8s namespace that directed traffic to the proxy-public service / the proxy pod, but that traffic was denied by the network policy rules. It was a surprise to me that a ingress rule without a namespaceSelector rule explicitly part of the traffic to the local namespace. The proposal of this PR is to simply allow the various JupyterHub pods to allow traffic incoming traffic from pods in all namespaces with a dedicated label, such as Another option to this, would be to ask users to add a custom ingress rule they manage themselves, like below. This rule would require both a namespace being labeled correctly, and have a pod within it labelled correctly, to send traffic to a specific port, on on the proxy pod. proxy:
networkPolicy:
ingress:
from:
- namespaceSelector:
matchLabels:
my-specific-namespace-label: my-specific-namespace-label-value
podSelector:
matchLabels:
my-specific-pod-label: my-specific-pod-label-value
ports:
- port: http # or https depending on other configurationTo learn how to specify networkPolicies is quite tricky, I'd say it is the most complicated resource to get right of all the resources we manage in this Helm chart.
I'm not sure, to me it sounds weird that a in-cluster-pod in another namespace should be allowed access on the merit it can be considered functioning as a loadbalancer, because how would it proove it was acting as a loadbalancer to the networkpolicycontroller that enforce the rules? Perhaps I locked down the proxy policy in a recent update to the networkpolicy resources without thinking about it? Hmm... yes...
In the past, we had a rule saying that as long as anyone wants to speak specifically with the proxy pod's HTTP or HTTPS port, that was fine, because specifying only port doesn't block ingress from other namespaces I assume. I didn't realize that I changed the behaviour when I made the change in #1670 - a testiment to networkPolicy resources are complicated to manage in my mind. |
|
My suggestion for allowing ingress from other namespaces is for the Z2JH config to list those other namespaces it's allowing ingress from (which may include all) and not bother checking labels for pods outside the current namespace. i.e. the resulting network ingress policy is the union of:
I think this avoids reducing security when namespaces are used for segregation of users with different privileges, and avoids users having to add labels to their own pods to access jupyterhub which can be tricky as each helm chart may do it differently. |
Ideas of options summarizedAllowlist of namespaces to accept traffic fromAn upside of this is that the configuration to do this option is entirely within our Helm chart's control to document. Another upside is that only a explicitly trusted namespace is allowed to send traffic here. A downside is that it will allow traffic from more pods than just the pods it need to allow traffic from within this namespace. A predefined label on pods to accept traffic from, even if coming from outside the namespaceIt will limit traffic very explicitly to a minimal scope is the main benefit in my mind, so that a virus in some other pod in this namespace won't risk sending malicious traffic without even needing to label itself correctly. But, this it is also a bit cumbersome considering it requires knowledge on how to label the pods which may deployed with various mechanisms and and configuration options. Further, this could allow a separate namespace with maliciously labeled pods to send traffic to the hub/proxy/singleuser servers. I think both of these options are individually viable, and it would also be viable to allow both of these options simultaneously by default, or let them be toggle-able by a configuration flag. @yuvipanda and @minrk who have also edited these files, what do you think? Configuration to choose between single-namespace and inter-namespace access labelsThis PR is currently equivalent of this idea without exposing a configuration to toggle to pods in other namespaces. # allowed pods (hub.jupyter.org/network-access-proxy-http) --> proxy (http/https port)
- ports:
- port: http
{{- if or $manualHTTPS $manualHTTPSwithsecret }}
- port: https
{{- end }}
from:
- podSelector:
matchLabels:
hub.jupyter.org/network-access-proxy-http: "true"
+ {{- if .Values.someconfiguration }}
+ namespaceSelector:
+ matchLabels: {} # allow the label above to be set on a pod in any namespace
+ {{- end }}Addition of a namespace label to provide access for all podsNote that we cannot target a namespace by name, only by labels set on the namespaces. I suggest that we make use of our already meaning infused labels. # allowed pods (hub.jupyter.org/network-access-proxy-http) --> proxy (http/https port)
- ports:
- port: http
{{- if or $manualHTTPS $manualHTTPSwithsecret }}
- port: https
{{- end }}
from:
- podSelector:
matchLabels:
hub.jupyter.org/network-access-proxy-http: "true"
+ - namespaceSelector:
+ matchLabels:
+ hub.jupyter.org/network-access-proxy-http: "true" |
|
This section of the k8s documentation is very relevant details on the rules that we can set in a NetworkPolicy from the k8s docs, distinguishing between: a) podSelector, b) namespaceSelector, c) podSelector AND namespaceSelector, and d) ipblock. |
|
I currently have the following lines to enable network policy and also allow ingress from a set of namespaces: https://github.com/2i2c-org/pilot-hubs/blob/master/hub/values.yaml#L71 I wrote #1380 a while ago to make this possible. I think Option 1 you described is the way to go, @consideRatio. |
|
@yuvipanda thank you for your input! I just updated #1842 (comment) with two concrete examples of: a) making it possible to out of Option 1, and b) adding a new way to allow all pods in namespaces with a certain label. Do you think we should write such configuration, or just go with them by default without exposing configuration to explain and maintain? @yuvipanda as a separate note, in your pilot-hubs configuration, you have a ingress:
- from:
- namespaceSelector:
matchLabels:
name: support
- podSelector:
matchLabels:
hub.jupyter.org/network-access-proxy-http: "true"
ports:
- port: httpWith the two separate sources of traffic that you allow in this rule, either any pod from the support namespace, or from a pod in the same namespace labelled with the access label. The second source is already allowed by default currently. I suspect that you wanted to use a "podSelector AND namespaceSelector" source rather than two separate sources. Related reference documentation are available here. |
consideRatio
changed the title
consideRatio
changed the title
|
@manics @yuvipanda I updated to also allow labelled namespaces and not only labelled pods in other namespaces. In my mind, merge or not now depends on: do we see it as a problem that we expose hub/singleuser-server/proxy for communication from other pods in other namespaces assuming they have the correct label? I would argue that this is not an escalation compared to in 0.9.1 where all http/https traffic is allowed to the proxy pod from other namespaces until #1670 was merged, and the proxy pod expose the singleuser servers and the hub server. I'm open to the idea that we make the inter-namespace ingress for hub/proxy http(s)/proxy api/singleuser-server configurable by a default, which we set to false by default for all pods except proxy pods http(s) ingress. The downside of this though, is it becomes quite a bit more complicated to correctly communicate this to users who may assume adding a label like |
consideRatio
force-pushed
the
pr/network-policies-across-namespaces
branch
from
92c57b7
to
70e2f86
Compare
|
@manics okay so I implemented But... Note that we have an autohttps pod which is not covered by any policy, and will route traffic to the proxy pod, and onwards to the hub pod and user pods. Hmm... In general, if you have no policy targetting a pod, it can ingress/egress like it wants. But if you have default network policy targetting all pods regarding ingress specifically, and no rules allowing traffic, then it implies they are denied all ingress unless another policy allows it. Hmmmm hmmm hmmm... I think we should really cover the autohttps pod with these network policies as well at least... Perhaps not create a default deny rule to influence pods in the namespace we don't know about though. |
|
Cheers! After sleeping on it I wonder if we could simplify the options to just one per component? Your intention was to have the defaults "just work" for the majority of people, my wish was to have an easy way to lock-down the setup with additional ingress done through the hub:
networkPolicy:
interNamespaceLabeledIngress: allow|deny (or enabled|disabled, true|false, allow|block, ...)
ingress: []
proxy:
networkPolicy:
# Covers both HTTP and API
interNamespaceLabeledIngress: allow|deny (or enabled|disabled, true|false)
ingress: []
singleuser:
networkPolicy:
interNamespaceLabeledIngress: allow|deny (or enabled|disabled, true|false)where |
|
@manics thanks for iterating with me on this! I'll go with your suggestion of having a Currently aiming for...
Currently considering...
I added Iteration on names@manics I'm not sure about the naming, for example, block indicates block, but we are not actually explicitly blocking, only saying that access with labels can only be used in the local namespace rather than in all namespaces". I'm also not sure about the |
|
With #1852 it might be better to rename |
|
I'm unhappy about exposing the proxy API port by default, I think that should be locked down. |
|
Good point. Perhaps |
I think this is an excellent idea! |
consideRatio
force-pushed
the
pr/network-policies-across-namespaces
branch
2 times, most recently
from
11d20af
to
aa3b70a
Compare
Co-authored-by: Simon Li <[email protected]>
consideRatio
force-pushed
the
pr/network-policies-across-namespaces
branch
from
f5ffd95
to
16e140a
Compare
|
The other use case that I can see coming up is folks deploying users in their own namespaces. These deployments definitely need cross-namespace access from singleuser pods -> hub API. I wonder if our lack of using the release name in our resources is coming up here again. Would it be simpler or more confusing to have two labels, such as:
or more a descriptive The idea being that rather than having network policies with our own customized configuration options, we have two simpler/more static ingress policies. Is that a good idea, or no? The main downside I can see of this being unconditional is that it provides a way for pods in another namespace who know a release name to grant themselves access. Maybe that's not ideal, and what we have here is the best option. |
| See the [security | ||
| documentation](https://zero-to-jupyterhub.readthedocs.io/en/latest/administrator/security.html#kubernetes-network-policies) | ||
| for more details on this. | ||
|
|
||
| - The Helm chart configuration `proxy.networkPolicy` has been removed, |
There was a problem hiding this comment.
This change is my only concern here. I don't love it because it seems to be baking too much of the implementation into the configuration. When we finally manage to land the traefik proxy implementation (#1162), this configuration structure will change again when nothing relevant to users should really need to. What do users need to know about the separation of autohttps and chp that we couldn't represent without separating them?
There was a problem hiding this comment.
We will still have a dedicated proxy for TLS termination i think, given that traefik doesnt support HA with lets encrypt.
This helm chart has a separate pod for TLS termination atm, and combining the config of two pods into one has caused it to be confusion in general. I'm think im for doing this even if it was a standalone refactoring without any other purpose to reduce that confusion.
If you feel strongly about it, i can revert it back, locating chp netpol config under proxy.netpol again.
There was a problem hiding this comment.
To me, this change seems like it would increase confusion to have two network policies applying to what users should see as one resource (and indeed will be one resource if we manage to land the traefik proxy). I am thinking of the example of
Q: I set proxy.chp.networkPolicy, why is it not having any effect?
A: that's because you have enabled autohttps, which means that proxy.traefik.networkPolicy governs proxy-public, not proxy.chp.networkPolicy. If you change how you configure https, your network policy will do what you expect, or you need to move the configuration to proxy.traefik.networkPolicy
Or is that not an accurate description of the behavior here?
|
Thank you for reviewing this @minrk! Hmmmmm, if we get away with simpler logic in all cases, i'm for simpler logic. My guiding compass feel very strongly about avoiding users needing to add their custom netpol rule injections at this point, because it is just so hard to understand for everybody involved and will become fragile with changes. If we can get away with simpler logic partially relates to if inter-namespace traffic can accept always going through the proxy pod, or if they need direct access without tls termination etc to the hub pod etc.
I had a very long discussion with @manics related to the security implications in a now resolved thread. I think that access to the outer proxy (autohttps, or proxy chp) seems reasonable to allow by active choice for someone powerful to label pods in some namespace. Now we lock down inter namespace communication by default except for the autohttps and proxy pods http/https ports, but leave the proxy api port blocked by default. It is a complicated API to design, and i can make changes as wished but i feel strongly about the following:
|
|
Other than the last discussion about whether to separate CHP and the http pod or to have a single proxy config I think this PR is a good solution. It provides:
(1) and (3) cover @consideRatio's wishes, (2) and (3) cover mine. My preference is to default to more secure |
|
Thanks for your comments, and for thinking so carefully about this! What I was trying to get at is that I think defining network policies separately for traefik and chp means that users who need to touch this configuration need to know more about how our https handling works than they should have to.
That's why I think we should have only a single configuration to set the accessibility of the "external entrypoint", and it should be up to us to make sure that's applied to the right target, not the user. Other than that detail, I think this PR is a good solution, and I would just like to consolidate the two network policy configurations into one (or assign them more practical and less implementation-detail names if there's a reason they need to be separate that I haven't understood such as |
There is a lot of things to say, and when it comes down to it everything is possible, but I find consolidating them to be too complicated to document for users and maintainers to motivate a change. Here is a list of thoughts.
@minrk perhaps we can have a chat about this to arrive at a resolution to go with? I'd like to hash out a concrete action plan and validate that you prefer that to the current PR suggestion. |
|
@minrk thank you for the video chat! I wanted to summarize a relevant point that we can pursue in the future. Idea to followup this PR
|
|
I'll merge this now given agreement that it can be good enough for now with @minrk in a video chat and that @manics seem positive in #1842 (comment) about the current state. With it merged, I plan to update the changelog and cut 0.10.0-beta.1 finally! |
|
👍 thanks for talking me through it! |
PR changes summarized
Breaking change
proxy.networkPolicyis deprecated, andproxy.chp.networkPolicyandproxy.traefik.networkPolicyis introduced.Restores access to
proxypod's http/https ports by default (bugfix)Since #1670 the proxy pod would not accept incoming traffic from pods in other namespaces, due to a bugfix that caused a breaking change. This can be observed in #1863 for example I think.
- ports: - port: http {{- if or $manualHTTPS $manualHTTPSwithsecret }} - port: https {{- end }} - - from: + from: - podSelector: matchLabels: hub.jupyter.org/network-access-proxy-http: "true"The bug fixed was that
hub.jupyter.org/network-access-proxy-http: "true"was entirely ignored and had no meaning, but now that it got meaning, we ended up restricting access too much. As an example, the proxy would reject incoming traffic from a nginx-ingress-controller pod that terminated TLS in another namespace.In this PR I introduce the networkPolicy configuration
allowedIngressPortsas suggested by @manics, and configures it forproxy.chp.networkPolicyandproxy.traefik.networkPolicyto be set to[http, https]by default, while the networkPolicy configuration for hub/singleuser will not allow any of this by default.This provides us with a configuration concept applicable to all networkPolicy resources we deploy, while retaining ability to work with even finer grained control with network access labels.
Conditional use of access labels across namespaces (enhancement)
This PR let's the respective
networkPolicyconfiguration configureinterNamespaceAccessLabels. If this configuration option is set toaccept, the access labels valid for the respective networkPolicy configuration, such ashub.jupyter.org/network-access-proxy-api: "true", will be accepted even on pods in other namespaces, and on entire namespaces, to allow these pods as sources for incoming network traffic.Motivation
Network policies are enabled by default as of #1271, and we need ability to configure access in a way that won't force users and maintainers to debug k8s native network policy configuration which is notoriously hard to understand correctly. Closes #1863.
allowedIngressPorts config
We needed to come up with a solution after #1670, which as described below ended up restricting ingress, for example from a nginx-ingress-controller pod in another namespace, towards the proxy pod. At the same time, we should have a flexible and cohesive configuration of networkPolicies.
interNamespaceAccessLabels config
interNamespaceAccessLabelsis ensuring that users are not forced to add fragile workarounds if they need to access something from another namespace in the future, and by avoiding fragile workarounds, we avoid complicated issue reports following the fragile configurations future failures.Related
NetworkPolicy documentation
This section of the k8s documentation is very relevant details on the rules that we can set in a NetworkPolicy from the k8s docs, distinguishing between: a) podSelector, b) namespaceSelector, c) podSelector AND namespaceSelector, and d) ipblock.