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

Egress firewall don't block host network #2786

Conversation

JacobTanenbaum
Copy link
Contributor

this commit allows host networked pods to not be blocked by egressFirewalls in both shared and local gateway modes. An address set is created with all the node IPs in it and in shared gateway mode an additional ACL is created to allow traffic to the nodes. Local gateway mode is more problematic, since the acls are on the nodes switch we need to use negation to allow the traffic to host network pods similar to how clusterCIDR traffic is ignored.

- What this PR does and why is it needed

- Special notes for reviewers

- How to verify it

- Description for the changelog

…atching pods

This is a legacy commit using ovn-nbctl for the implementation. This is
done as to be able to have the commit backported

Egress firewall matching pods didn't currently have any connectivity to
the host network when a "deny 0.0.0.0" rule is created. That type of a
rule blocked access to all cluster nodes. This was not in-line with the
expectation of how the feature was working for people coming from
openshift-sdn. They thus needed to explicitly allow all node IPs if they
required pods to access the Kube API service, for example. That is not
optimal though as if nodes leave / join the cluster, the rules need to
be updated by the admin. We now have documentation specifically
mentioning that we don't support this, so I am not sure we want this:
openshift/openshift-docs#35311 but this patch
was pretty straightforward to code, so we can discuss it on the PR.
Note: anyone using ovn-kubernetes before this patch (and setting up
specific allow rules for the nodes) should not be impacted negatively by
this patch...but they will have a duplicate ACL for every node they've
explicitly allowed

Signed-off-by: Alexander Constantinescu <aconstan@redhat.com>
@coveralls
Copy link

coveralls commented Feb 1, 2022

Coverage Status

Coverage decreased (-0.05%) to 50.686% when pulling 183b132 on JacobTanenbaum:egress-firewall-host-network into b2bf0fe on ovn-org:master.

Copy link
Contributor

@trozet trozet left a comment

Choose a reason for hiding this comment

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

the local gw mode support should also be in the first commit with "legacy" support for backporting. Other than that and another comment lgtm

@@ -80,6 +82,66 @@ func newEgressFirewallRule(rawEgressFirewallRule egressfirewallapi.EgressFirewal
return efr, nil
}

func (oc *Controller) addNodeForEgressFirewall(node *v1.Node) error {
v4Addr, v6Addr := getNodeInternalAddrs(node)
Copy link
Contributor

Choose a reason for hiding this comment

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

// getNodeInternalAddrs returns the first IPv4 and/or IPv6 InternalIP defined
// for the node. On certain cloud providers (AWS) the egress IP will be added to
// the list of node IPs as an InternalIP address, we don't want to create the
// default allow logical router policies for that IP. Node IPs are ordered,
// meaning the egress IP will never be first in this list.
func getNodeInternalAddrs(node *v1.Node) (net.IP, net.IP) {
I wonder if we should get all ip addresses on the node, using the the k8s.ovn.org/host-addresses annotation? That would allow access to any IP that lives on a node.

@girishmg
Copy link
Member

@trozet @JacobTanenbaum this would cause a security issue isn't it?

We have an EgressFirewall rule where-in all the Pods from a namespace X can't access 10.0.0.0/8. On 10/8 is all of our K8s Node IPs (both control plane and worker) reside. These Pods don't need access to K8s API Server and in some instances don't have ServiceAccount token mounted as well. This is all done so that the untrusted pods can't DDos the services behind K8s Node IP or try to access the K8s Node itself.

@trozet
Copy link
Contributor

trozet commented Feb 22, 2022

I originally thought the premise of this was based on the original behavior of openshift-sdn (where egress firewall was originally conceived). However, @danwinship told me by default openshift-sdn doesn't allow access to node IPs either, and they must be explicitly allowed. Therefore, I think there is no real need for this change and the behavior is expected.

@trozet trozet closed this Feb 22, 2022
@trozet trozet reopened this Feb 22, 2022
@trozet
Copy link
Contributor

trozet commented Feb 22, 2022

OK after some clarification the expected behavior is pod -> service should be allowed to access host network endpoints, not pod -> any host network pod should work through egress firewall. We need a fix for the former, which I'm not sure how to solve since the DNAT happens on the node switch and egress firewall is enforced at the join switch. @dceara @numansiddique @girishmg @JacobTanenbaum any ideas? maybe we could match on CT or something to see if the packet was DNAT'ed by a service.

@trozet
Copy link
Contributor

trozet commented Mar 22, 2022

So after more consideration @danwinship pointed out that the pods can always access any k8s node anyway because it can reach any mgmt port ip. So there really is no point in blocking the node IP. I think this PR is acceptable.

@girishmg
Copy link
Member

@trozet @danwinship deployers can choose to protect the access to node through ovn-k8s-mp0

# firewall-cmd --list-all --zone ovn 
ovn (active)
  target: default
  icmp-block-inversion: yes
  interfaces: ovn-k8s-mp0
  sources: 
  services: 
  ports: 9410/tcp 10255/tcp 10250/tcp 4194/tcp 49123/tcp
  protocols: 
  masquerade: no
  forward-ports: 
  source-ports: 
  icmp-blocks: 
  rich rules: 

On every node we open up ports for K8s endpoints that are backed by HostNetwork Pods in the ovn zone.

Also, if we want to support this: pod -> service should be allowed to access host network endpoints, can't we just add an egressfirewall rule to allow all access to K8s Nodes CIDR

JacobTanenbaum and others added 3 commits March 22, 2022 13:11
KEYWORD

keyword
…pods

Egress firewall matching pods didn't currently have any connectivity to
the host network when a "deny 0.0.0.0" rule is created. That type of a
rule blocked access to all cluster nodes. This was not in-line with the
expectation of how the feature was working for people coming from
openshift-sdn. They thus needed to explicitly allow all node IPs if they
required pods to access the Kube API service, for example. That is not
optimal though as if nodes leave / join the cluster, the rules need to
be updated by the admin. We now have documentation specifically
mentioning that we don't support this, so I am not sure we want this:
openshift/openshift-docs#35311 but this patch
was pretty straightforward to code, so we can discuss it on the PR.
Note: anyone using ovn-kubernetes before this patch (and setting up
specific allow rules for the nodes) should not be impacted negatively by
this patch...but they will have a duplicate ACL for every node they've
explicitly allowed

Signed-off-by: Jacob Tanenbaum <jtanenba@redhat.com>
@trozet
Copy link
Contributor

trozet commented Mar 23, 2022

@girishmg so you are using something outside of ovn-kube to manage firewalling the mp0 port? Shouldn't that be done via OVNK to restrict access to mp0 with egress firewall?

This disadvantage to making a egress firewall rule is:

  1. If you make a rule to allow access to all nodes in the node cidr, there may be some servers on your node network which you dont want pods to be able to access.
  2. If 1 is true, then you have to add per ip rules. The downside to this is every time a new node is added you have to manually go add a new rule (or automate it).

Is your opinion that we shouldn't proceed with trying to allow access to the k8s nodes the default behavior? If so, is it your opinion that we also shouldn't try to allow access to node ips backed by services if blocked by egress firewall?

@girishmg
Copy link
Member

girishmg commented Mar 28, 2022

@trozet preventing access to K8s Node through ovn-k8s-mp0 is a different issue, perhaps we need to add bunch of OVN K8s Managed OVN ACLs on the ovn-k8s-mp0 management port.

The issue is this:
(1) Cluster Admin defines an EgressFirewall rule to drop all the packets to Node IP CIDR except for K8s API Server

spec:
  egress:
  - ports:
    - port: 6443
      protocol: TCP
    to:
      cidrSelector: 10.0.0.170/32
    type: Allow
  - ports:
    - port: 6443
      protocol: TCP
    to:
      cidrSelector: 10.0.0.171/32
    type: Allow
  - ports:
    - port: 6443
      protocol: TCP
    to:
      cidrSelector: 10.0.0.172/32
    type: Allow
  - to:
      cidrSelector: 10.0.0.0/8
    type: Deny
status:
  status: EgressFirewall Rules applied

(2)in order to allow a Pod access a Service backed by HostNetwork Pod, this PR adds an OVN ACL to allow all packets to Node IPs CIDR.

@trozet you asked, so IMHO

Is your opinion that we shouldn't proceed with trying to allow access to the k8s nodes the default behavior?

yes

If so, is it your opinion that we also shouldn't try to allow access to node ips backed by services if blocked by egress firewall?

yes, otherwise it will contradict the user authored egress firewall, isn't it?

In other words, should we allow implicitly adding rules that contradicts what deployer wants? Perhaps, we could add a lower priority OVN ACL to allow access to services backed by Node IPs? That way, if there are an explicit drops, then it will get matched by the deployer defined EgressFirewall?

@trozet
Copy link
Contributor

trozet commented Sep 23, 2022

Closing this PR. I have another PR in progress to allow node selector which will make easier to create dynamic EF rules. We should still cover blocking access to the host via mp0 inside ovnk8s. I expect @npinaeva refactor of egress firewall onto the worker switches to help accommodate this for both gateway modes.

@trozet trozet closed this Sep 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants