[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...