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

ExternalIP allows access to node part 2 #1209

Closed
0xHazard opened this issue Dec 7, 2021 · 11 comments
Closed

ExternalIP allows access to node part 2 #1209

0xHazard opened this issue Dec 7, 2021 · 11 comments
Labels

Comments

@0xHazard
Copy link
Contributor

0xHazard commented Dec 7, 2021

#618 follow up.
This solution is designed exclusively for ECMP setup and doesn't work for floating addresses (aka ARP mode) because it relies on kube-router-local-ips set, which is filled up by "local" IP addresses by iterating over all interfaces available on the host and retrieving its IP addresses

func getAllLocalIPs() ([]netlink.Addr, error) {
links, err := netlink.LinkList()
if err != nil {
return nil, errors.New("Could not load list of net interfaces: " + err.Error())
}
addrs := make([]netlink.Addr, 0)
for _, link := range links {
// do not include IPs for any interface that calls itself "dummy"
// or any of the docker# interfaces
if strings.Contains(link.Attrs().Name, "dummy") ||
strings.Contains(link.Attrs().Name, "kube") ||
strings.Contains(link.Attrs().Name, "docker") {
continue
}
linkAddrs, err := netlink.AddrList(link, netlink.FAMILY_V4)
if err != nil {
return nil, errors.New("Failed to get IPs for interface: " + err.Error())
}
addrs = append(addrs, linkAddrs...)
}
return addrs, nil
}

so that rule has no effect

// traffic to local addresses if any NodePort service exists.
comment = "reject all unexpected traffic to service IPs"
args = []string{"-m", "comment", "--comment", comment,
"-m", "set", "!", "--match-set", localIPsIPSetName, "dst",
"-j", "REJECT", "--reject-with", "icmp-port-unreachable"}

What if we add an extra argument that would accept a list of tracked interfaces, so we can iterate only over these interfaces?

@aauren
Copy link
Collaborator

aauren commented Dec 8, 2021

I think that the intention is that for a NodePort service it should be controlled by the --nodeport-bindon-all-ip flag. If you have enabled that, then you should be able to receive traffic on all of the interfaces that kube-router does not specifically control which we assume to be real interfaces. From there, if the user desires further filtering, then it would be up to the user to define appropriate NetworkPolicy's which are much more flexible.

If you have not enabled that option, then in

if nsc.nodeportBindOnAllIP {
your service will only be bound on the nodeIP that is set on the Kubernetes node definition for your node (i.e. InternalIP or fall through to ExternalIP) in which case it doesn't matter if iptables lets it through or not because there won't be anything to receive the traffic.

I could see potentially changing

to only add localhost and the nodeIP similar to what we do in service_endpoints_sync when the user hasn't enabled --nodeport-bindon-all-ip, and possibly also filtering on the dst port range defined by --service-node-port-range but otherwise, I think that the current logic is working as intended.

@0xHazard
Copy link
Contributor Author

0xHazard commented Dec 8, 2021

I'm not sure if we're talking about the same thing. Imagine a machine with 2 interfaces eth for NodeIP and eth.666 for VIPs aka ExternalIPs. Both respond to ARP requests. Now, since getAllLocalIPs() added our VIPs to kube-router-local-ips, our rules

-A KUBE-ROUTER-SERVICES -m comment --comment "allow input traffic to ipvs services" -m set --match-set kube-router-ipvs-services dst,dst -j ACCEPT
-A KUBE-ROUTER-SERVICES -p icmp -m comment --comment "allow icmp echo requests to service IPs" -m icmp --icmp-type 8 -j ACCEPT
-A KUBE-ROUTER-SERVICES -m comment --comment "reject all unexpected traffic to service IPs" -m set ! --match-set kube-router-local-ips dst -j REJECT --reject-with icmp-port-unreachable

are no longer effective and allow access to any service listening 0.0.0.0 within default namespace. Is this expected behavior?

@aauren
Copy link
Collaborator

aauren commented Dec 9, 2021

Maybe we aren't talking about the same thing...

Are you talking about potential exposure of services that are not containerized or are containerized, but are running in the host network namespace?

Because kube-router doesn't comment on host based services, only kubernetes based containers and services running within the container's network namespace. The rule that you mention doesn't result in an ACCEPT, but rather the traffic is passed along to future chains and it is left up to the administrator to filter those services appropriately in subsequent chains.

@0xHazard
Copy link
Contributor Author

0xHazard commented Dec 9, 2021

I want access to only exposed ports e.g. if 10.10.10.111 tcp/80 is exposed through a Service, then I don't want let in traffic to 10.10.10.111 tcp/22, which is the case now. The rule I pointed to was made to resolve this exact issue. However, that works only with ECMP setup, because in that case all the service IPs, including ExternalIPs, are assigned to kube-dummy-if, which is excluded from tracking in getAllLocalIPs(), so all unexpected traffic to the service IPs gets rejected.

@aauren
Copy link
Collaborator

aauren commented Dec 9, 2021

I think that you're missing my point though. There is nothing in kube-router that would specifically comment on accepting traffic on 10.10.10.111:22. If it is a service that kube-router recognizes, then it will accept the traffic specifically. Otherwise the intention is that the traffic continues to process through the rest of the chain so that users have the choice for what they want to do with the traffic.

The only thing that the rule you're talking about is doing is not rejecting the traffic. This is by design. In this way it allows the user to comment on what they want to do with local services in future chains. If you want to block traffic on port 22 because you have SSH setup there, all you would need to do is something like:

iptables -A input_provides -d <ip_you_want_ssh_on>/32 -i <allowed_ssh_interface> -p tcp -m tcp --dport 22 -j ACCEPT
iptables -P INPUT DROP

@0xHazard
Copy link
Contributor Author

0xHazard commented Dec 9, 2021

I see what you mean, but still, how's the case I described above any different from #282?

@aauren
Copy link
Collaborator

aauren commented Dec 9, 2021

That's a very old issue. Since that issue, and specifically in the last year, we've made significant changes to kube-router's network policy handling.

Previously, we would ACCEPT all traffic once we matched it. In cases like yours, where we matched it incorrectly rather than just "not rejecting" the traffic, we would actually accept the traffic. This meant that in order to allow users to secure their nodes we had to specifically account for non-kubernetes traffic because we were causing the problem by making judgements about non-kubernetes traffic. This added a lot of complexity to kube-router and eventually it became impossible to satisfy all use-cases.

Over the last year, and at the request of several users, we have changed our posture. We have refactored much of our network policy implementation, to instead only comment on traffic that kube-router should control and leaving other host based traffic types to fall through to future, user-defined iptables chains so that system administrators have choice in how they deal with non-kubernetes traffic that hits their nodes.

@0xHazard
Copy link
Contributor Author

0xHazard commented Dec 9, 2021

Define "non-kubernetes" traffic. Is packets destined to ExternalIP non-kubernetes traffic? Then why are packets coming to unexposed ports being rejected by the rule and not flowing further down the chains as you said? My point is that Kube-router currently treats the same traffic differently, relying only upon a network interface name.

@aauren
Copy link
Collaborator

aauren commented Dec 9, 2021

ExternalIP services are consider Kubernetes traffic.

@0xHazard
Copy link
Contributor Author

So, back to the question. Why if an interface name doesn't contain 'Kube', 'Docker' or 'Dummy' in it unexpected traffic to ExternalIPs is not rejected, and if it does, unexpected traffic to ExternalIPs is rejected? Can we make this part flexible for users rather than hardcoded?

@aauren
Copy link
Collaborator

aauren commented Dec 10, 2021

Because those are the interfaces that kube-router manages, so it makes sense for us to only consider those ones. If users add additional interfaces that kube-router doesn't know about, that isn't the domain of kube-router. I don't see us adding a command-line option or exposing an annotation or some such to allow users to configure interfaces as that brings too much complexity into kube-router to manage.

  1. The traffic enters KUBE-ROUTER-SERVICES if it is destined to a potential service providing IP address, this includes cluster IPs, external IPs, and the Node IP (for node port based services)
  2. If the traffic destination matches both a configured service IP AND a service port, then it is accepted (later filtering based on network policy will be done in the FORWARD table).
  3. If the traffic is some form of ICMP request that we allow, the traffic is accepted
  4. Otherwise, if the traffic is destined towards an interface that kube-router owns (i.e. the interface has the name kube, docker, or dummy), and it didn't match any of the other above rules, then the traffic is rejected. This is because kube-router can safely assume at this point, that the traffic pattern is invalid.
  5. All other traffic is left to traverse the rest of the chain so that the user can decide which flows are valid or invalid.

We cannot just start rejecting traffic from interfaces that we don't know about because it would include potentially valid flows to the node's IP address.

kube-router doesn't manage or create arp based floating address interfaces, so it shouldn't attempt to manage them. If the user creates them, then the user should expect to manage them via the method I described above.

@aauren aauren closed this as completed Dec 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants