[BUG] AdminNetworkPolicy Core can't express allow/deny all

[fasaxc]

Various use cases of ANP require a "deny all" following a set of Pass/Allow rules but it looks like that's hard to express in the current structure. We require at least one ANPXXXPeer in a rule and then a peer can't be empty. I think "deny all" requires the Networks extension to be present so that you can write 0.0.0.0/0 and ::/0 into the list of CIDRs

	// From is the list of sources whose traffic this rule applies to.
	// If any AdminNetworkPolicyIngressPeer matches the source of incoming
	// traffic then the specified action is applied.
	// This field must be defined and contain at least one item.
	//
	// Support: Core
	//
	// +kubebuilder:validation:MinItems=1
	// +kubebuilder:validation:MaxItems=100
	From []AdminNetworkPolicyIngressPeer `json:"from"`

I suppose this is a consequence of the requirement to fail closed if the policy is "empty" but it struck me as awkward to need to think of something to put in a Peer when I want to create a "blanket" rule. (It'd be easy to forget the IPv6 CIDR, for example, resulting in only blocking IPv4 traffic.)

[tssurya]

Right I acknowledge this issue its something that sets apart ANP from NetPol

1. firstly ingress deny all is not possible to express, at max one can say ingress deny from all pods/namespaces in your cluster - we knew this while designing and was intentionally done is my understanding since we didn't have a use case for other stories
2. for egress yes the only way to achieve ultimate deny is using 0.0.0.0 and ::/0;

All in all this explicit requirement in the API AFAIK was intentionally done, without expressing what you want to deny all to we didn't want the denyAll to be an assumption of implicit things

Like I think its by design and not a bug per say. Let's see what others thing in our next meeting

[fasaxc]

I do think that not being able to express "deny all", in particular, is a significant limitation that prevents the admin from imposing "guard rails" around the cluster. I can live with "deny all" requiring something (for example a CIDR) to be specified given that it was done for extensibility reasons.

It's a very common pattern amongst our tiered policy users to use the "admin" tiers for something like this:

• Allow to kube-system/etc
• Allow from monitoring components to everything
• Pass prod-to-prod
• Pass staging-to-staging
• Pass dev-to-dev
• Deny everything else

I'm not sure that everyone in the group has internalised this limitation. For example, the docs for DomainNames assumes you can "allow some and then deny all" but that's only true if the dataplane also implements the Networks extension.

	// DomainNames is only supported for ALLOW rules. In order to control
	// access, DomainNames ALLOW rules should be used with a lower priority
	// egress deny -- this allows the admin to maintain an explicit "allowlist"
	// of reachable domains.

Admittedly, that is an egress-only feature, where CIDRs are available (as an extension).

[fasaxc]

One potential "fix" for this would be to have a field in the Peer that is explicitly for matching all:

AllTraffic bool 

Then we have a specific non-zero field to mean "match all".

[fasaxc]

From @danwinship : let's not use a bool, may want to use this for protocols or other scopes: "AllButICMP"

[tssurya]

one question I have is denyAll would mean what? Would it mean same as "0.0.0.0/0" or is there more to it? from use case perspective

[fasaxc]

I think ANP is entirely focused on L3/4 and only IP protocols, so, with that caveat, I think it means "all IP traffic" which is pretty much the same as 0.0.0.0/0 and ::/0. How you do that match is implementation dependent though, in iptables, you'd say

-A some-chain -j DROP

Rather than needing to say

-A some-chain -s 0.0.0.0/0 -j DROP

Note that, at least in Calico's implementation, we always allow "necessary" ICMP traffic for IPv6 bootstrap (and we don't use L2), so, I suppose our interpretation is "all user traffic".

[danwinship]

one question I have is denyAll would mean what? Would it mean same as "0.0.0.0/0" or is there more to it? from use case perspective

Right, this is what I was getting at with maybe not just using a bool.

Things "deny all" might mean:

1. Deny everything that it is possible to deny using the fully-standard/supported selectors (ie, deny all pod to pod traffic but nothing else).
2. Deny everything that it is possible to deny using all selectors that this implementation of ANP supports.
3. Deny TCP, UDP, and SCTP to all unicast IPv4 and IPv6 addresses.
4. Deny TCP, UDP, and SCTP to all unicast, multicast, anycast, and broadcast IPv4 and IPv6 addresses.
5. Deny all IP and IPv6 traffic (including ICMP).
6. Deny all L3 traffic, and all L2 traffic that isn't needed to make L3 traffic work.

1 and 2 change over time (and between clusters).

3 and 4 have the problem that they may not deny everything that the admin wants to deny.

5 and 6 have the problem that they deny traffic that ANP itself does not currently have the ability to re-allow.

So we can pick one of these and say "that's what 'match all' means", but I feel like there are possibly use cases for both 3/4 and 5/6...

[mikemorris]

Right, this is what I was getting at with maybe not just using a bool.

If we have an enum for this, we could also specify that some of these well-known options MUST be supported by all implementations, and others may be optional but if implemented should conform to expected behavior.

[danwinship]

oh I was thinking "not a boolean" as in a struct. like

spec:
  egress:
    - action: Deny
      to:
        - all: {}

vs

      to:
        - all:
            allL3: true
            allL2: true

(but I didn't really think about this deeply)

struct is much more future-proof though...

[k8s-triage-robot]

The Kubernetes project currently lacks enough contributors to adequately respond to all issues.

This bot triages un-triaged issues according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

• Mark this issue as fresh with /remove-lifecycle stale
• Close this issue with /close
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

[danwinship]

/remove-lifecycle stale

5 and 6 have the problem that they deny traffic that ANP itself does not currently have the ability to re-allow.

Thinking about this some more, this is wrong: if we have any sort of All selector that you can use in a Deny rule, then presumably you can also use that selector in an Allow rule too. So the problem isn't "you can deny traffic that you can't re-allow", it's just "there is a certain category of traffic that you can allow or deny broadly, but you can't allow or deny narrowly". (eg, you'd be able to deny all ICMP, but not some ICMP). But I guess that's strictly better than the current situation, where you can't allow/deny it broadly or narrowly.

So maybe we should just implement 6: "all" means "all L2 or L3 traffic that isn't required to make other allowed traffic work". (We would need to be very specific about what that means, and whether there's any implementation-defined behavior in there.)

I still think the struct version is better than the boolean version. Among other things, if we use a boolean, then you can say:

egress:
- action: Deny
  to:
  - all: false

which is silly. Better to have:

egress:
- action: Deny
  to:
  - all: {}

and then we can potentially extend it later