search

kubernetes-sigs / kube-network-policies

Kubernetes network policies
Apache License 2.0
34 stars 11 forks source link

allow unknown protocols #49

Closed aojea closed 2 months ago

aojea commented 3 months ago

Unknown protocols are not subject to Kubernetes policies, hence they must be allowed

If the receiving pod is local, then ipvs sends packet via the output hook (not forward). So both are needed

Fixes https://github.com/kubernetes-sigs/kube-network-policies/issues/46

Co-Authored-By: Lars Ekman uablrek@gmail.com @uablrek

k8s-ci-robot commented 3 months ago

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: aojea

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files: - ~~[OWNERS](https://github.com/kubernetes-sigs/kube-network-policies/blob/main/OWNERS)~~ [aojea] Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment
aojea commented 3 months ago

Also check if the IPv6 failure in https://github.com/kubernetes-sigs/kube-network-policies/pull/47 are related to the output chain is something environmental

cc: @uablrek

aojea commented 3 months ago

ok, the test passes here, so is the output rule

/hold

I don't have clear what is the expected behavior https://github.com/kubernetes-sigs/network-policy-api/issues/187#issuecomment-2202722531 regarding protocol

/assign @danwinship

aojea commented 3 months ago

/hold cancel

@uablrek I've added you as co author and incorporated your commits with some amendments

aojea commented 3 months ago

So, thinking as an user, I think the best behavior will be that if I don't define any protocol in a network policy I apply the existing policy rules ... we can do this just not returning an error during packet processing , it will return an empty string for the protocol field and zero for src and destination port, and it will fail to match if there is a rule specifying a protocol

aojea commented 2 months ago

So, it seems there are two AI:

  1. figure out why ipv6 fails https://github.com/kubernetes-sigs/kube-network-policies/pull/49#discussion_r1669094712
  2. define the behavior on unrecognized packets https://github.com/kubernetes-sigs/kube-network-policies/pull/49#discussion_r1670034038
danwinship commented 2 months ago

is there any part of this or #47 that can be merged as-is?

uablrek commented 2 months ago

is there any part of this or https://github.com/kubernetes-sigs/kube-network-policies/pull/47 that can be merged as-is?

~IMO https://github.com/kubernetes-sigs/kube-network-policies/pull/47 can wait. We know the consequences (proxy-mode=ipvs doesn't work). I plan to return the IPPROTO number and reject some protocols, like packets with IPv6 routing headers, so I am unsure about this PR also.~

uablrek commented 2 months ago

I think accepting too much (all unknown) may be a security issue in the IPv6 extension header case. For instance if we miss some combination of EH's that are valid TCP, we may accept TCP traffic that should be blocked.

Better be restrictive, and for instance reject packets with EH's in a non-recommended order.

uablrek commented 2 months ago

Scratch https://github.com/kubernetes-sigs/kube-network-policies/pull/49#issuecomment-2217717047.

Please merge this PR as-is an close #47. I'll rebase #51 if needed, then that PR should be next in line IMO. I have prepared a commit on top of #51 to handle EH's out-of-order, and routing-EH (https://github.com/kubernetes-sigs/kube-network-policies/pull/49#discussion_r1671898983)

danwinship commented 2 months ago

OK, so thinking about this some more, if a pod is unprivileged and does not have CAP_NET_ADMIN or CAP_NET_RAW, then it can't send anything except IP. Given that, and the fact that we want to avoid accidentally blocking ICMP reply packets (eg destination unreachable), I think we should not make any effort to ever block ICMP.

So, I'd say, we should keep the meta l4proto != { tcp, udp, sctp } accept rule and just not worry about anything else.

So that solves the "unrecognized protocol" problem and leaves:

Right?

aojea commented 2 months ago

OK, so thinking about this some more, if a pod is unprivileged and does not have CAP_NET_ADMIN or CAP_NET_RAW, then it can't send anything except IP. Given that, and the fact that we want to avoid accidentally blocking ICMP reply packets (eg destination unreachable), I think we should not make any effort to ever block ICMP.

So, I'd say, we should keep the meta l4proto != { tcp, udp, sctp } accept rule and just not worry about anything else.

So that solves the "unrecognized protocol" problem and leaves:

  • fragments, which I think we think isn't a problem if we have the meta l4proto rule? We should figure out how to test this...
  • parse errors, which probably shouldn't happen, but I think we should treat as "deny", and log errors about, and see if they ever actually happen
  • IPv6 extensions, which I believe we are intentionally punting to Parse IPv6 extension headers #51 and not worrying about in this patch

Right?

ICMP packets can be evaluated by network policies if no Port fields are set https://github.com/kubernetes-sigs/kube-network-policies/pull/51#discussion_r1681040192

I'm going to close this to avoid more noise, once we have the parser done, I want to debug the OUTPUT chain problem separetly