Repository navigation
CFP: Clarify the intent for policies with default deny specified with no rules #35558
Description
Problem
As of Cilium v1.16, in order to apply a default deny policy, a CNP or CCNP must have at least one rule in an egress, egressDeny, ingress or ingressDeny stanza.
Furthermore, in Cilium v1.16 there is a new enableDefaultDeny field which seems like it should be able to be used to enforce a default deny posture without listing any rules in one of the rule allow/deny fields. If this field is specified and describes that default deny should be enabled, but there are no rules, then the network policy does not enforce a default deny.
Proposal A
This CFP proposes that the following network policy should enable a default deny posture even though it does not contain an egress, egressDeny, ingress or ingressDeny field.
apiVersion: cilium.io/v2
kind: CiliumClusterwideNetworkPolicy
metadata:
name: default-deny
spec:
endpointSelector:
matchExpressions:
- key: "io.kubernetes.pod.namespace"
operator: "NotIn"
values:
- "kube-system"
enableDefaultDeny:
egress: true
ingress: true
EDIT: Note that there are counter-proposals for different behavior in the discussion below.
Activity
joestringer commentedon Oct 25, 2024
I believe that currently this is considered to be an invalid policy as it is not fully formed (does not contain an
egress,egressDeny,ingressoringressDenyfield). Though I will also note that the cilium operator's network policy validator does not explicitly identify the network policy above as invalid, it just does not have any effect.squeed commentedon Oct 28, 2024
My instinct would be to reject empty policies at the apiserver. There should probably be a OneOf directive for Egress / EgressDeny / Ingress / IngressDeny stanzas.
One of the trickier things to understand in the Kubernetes style of network policy is the effect of empty fields. Does it select everything or nothing? Will setting this field change some sort of hidden local variable?
So, I'm generally in favor of preventing "should be a no-op" entry, much how Linux usually rejects unknown flags rather than ignoring them.
joestringer commentedon Oct 29, 2024
That's certainly also an option. Personally when I read the YAML snippet posted in the issue description, the intent seems crystal clear to me.. though admittedly it could bring up some other semantic questions that are less obvious.
For instance, if we allowed there to be no ingress/egress stanzas, would that mean that the following also valid CNP? What does it mean?
I would interpret this one to be a complete no-op, and therefore I think it'd be helpful to reject this policy. Maybe that's speaking a bit to your instinct @squeed about requiring one of the Egress / EgressDeny / Ingress / IngressDeny stanzas.
renyunkang commentedon Nov 8, 2024
If the
enableDefaultDenyfield in a Cilium Network Policy (CNP) is set to{}(an empty object), two solutions are possible:Strict validation with status updates
Treat an empty enableDefaultDeny as an invalid configuration. Reject the policy application, update the CNP status to reflect the validation failure, and provide a clear error message indicating the missing ingress and egress settings.
Defaulting to ingress: false, egress: false.
This means that default-deny behavior is disabled for both ingress and egress traffic. The policy would be applied without error, providing a smoother user experience.
While both are viable, I personally favor Option 2, defaulting to disabling default-deny for both ingress and egress.
joestringer commentedon Nov 8, 2024
@renyunkang are you considering the state of the
enableDefaultDeny: {}field with or without rule statements likeingress,egress, etc?For existing policies written without the
enableDefaultDenyfield, the behavior must not change. This is equivalent to installing the resource without theenableDefaultDenyfield, and then if you retrieve the resource with the new schema, it translates the field to the empty value,enableDefaultDeny: {}. Since the existing policies would enable a default deny when there is at least one rule in aningress,egress, etc stanza, this means that theenableDefaultDeny: {}field must not disable default deny for each direction. In order to disable default deny, you must explicitly setenableDefaultDeny: ingress: false(and/or equivalent egress statement).As a general constraint of the system, if some configuration has no effect, then I think it can improve user experience for the resource to be rejected. For example the resource described here. This helps to communicate when the user has created a configuration that does not do what they intend to do.
renyunkang commentedon Nov 11, 2024
I've reconsidered; please review the following description and let me know if it's accurate. @joestringer
If the daemon has
EnableNonDefaultDenyPoliciesset totrue.One of the following must be configured:
Ingress,IngressDeny,Egress,EgressDeny,EnableDefaultDeny.Ingress, orEnableDefaultDeny.Egress. Otherwise, the policy configuration is considered erroneous.For CNP with
IngressorIngressDenyrules, ifEnableDefaultDeny.Ingressis specified, its value is used; otherwise, it defaults totrue. The same logic applies toEgressandEnableDefaultDeny.Egress.If the daemon has
EnableNonDefaultDenyPoliciesset tofalse, the previous logic is used.Ingress,IngressDeny,Egress, orEgressDenymust be configured; otherwise, the policy configuration is considered erroneous.EnableDefaultDenydefaults to{ingress: true, egress: true}.joestringer commentedon Nov 20, 2024
To align with @squeed's statements above, I think it would be simpler to just require a rule to be specified. Effectively that means there would be a small change from your post above @renyunkang :
If the daemon has EnableNonDefaultDenyPolicies set to true:
EnableDefaultDeny.Ingress, or EnableDefaultDeny.Egress.One of the benefits is that for this simpler expression it should be easier to embed this in the CRD validation so that the kube-apiserver enforces it, rather than having the cilium-agent enforce it after the resource is already configured. Better UX.
renyunkang commentedon Nov 26, 2024
The current description completely contradicts the CFP title. Is this modification consistent with the CFP proposal? Or should we revise the CFP proposal title?
renyunkang commentedon Nov 27, 2024
With the following rule configured, what happens to ingress traffic?
Will setting
enableDefaultDeny.ingresstotrueorfalseallow all ingress traffic in either case? @joestringer @squeedjoestringer commentedon Nov 27, 2024
Sorry, I agree it contradicts the CFP title. I created the proposal to try to gather feedback but I don't hold a strong opinion that my original description of the solution is the right way. Probably if I were to rewrite the issue, I would reframe it to first describe:
enableDefaultDeny.egress/enableDefaultDeny.ingressby itself has no effectYour questions are super helpful to help us reason about what we think the right solution to the problem is. I think that the logical conclusion of Casey's proposal would be that the network policies you proposed in your latest post should both be rejected, because they configure the
ingressdefault deny posture but they do not contain any ingress rules.[-]CFP: Support default deny policy with no rules[/-][+]CFP: Clarify the intent for policies with default deny specified with no rules[/+]renyunkang commentedon Nov 28, 2024
Okay, based on your description, let's elaborate with an example: @joestringer @squeed
The problem:
Before v1.16.0, the following rule allowed all ingress traffic:
After v1.16.0 introduced
enableDefaultDeny, the above configuration, regardless of whetherenableDefaultDeny.ingressis true or false, still allows all ingress traffic. This is confusing for users because the configuration has no effect.Proposed Solutions:
Solution 1: Change the semantics of enableDefaultDeny so that it takes effect as intended.
Solution 2: Declare the policy invalid.
My Understanding:
Solution 1 is easy to understand.
How should Solution 2 be interpreted? Detecting invalid policies works well for policies like this:
However, Solution 2 might not be suitable for the first example. Should we still use Solution 2 for the first example? Since there's no
ingressoringressDenysection,enableDefaultDeny.ingresscannot be configured, right?Furthermore, if setting
enableDefaultDeny.ingressto eithertrueorfalseresults in the rejection of all ingress traffic, will the behavior change for policies without theenableDefaultDenyfield after users upgrade from v1.15.x or lower versions?joestringer commentedon Dec 3, 2024
It's a bit nuanced to avoid severely breaking existing clusters. We can think of the default deny configuration as a tri-state for each direction: It is either not present in the policy (v1.15-compatible), or it is present with a true (deny all by default) or false (do not deny all by default; only apply the rules explicitly listed in the policy) value.
This is how I think that Solution 2 solves these problems:
If there is an
ingressoringressDenystanza with at least one rule:enableDefaultDeny.ingress: trueor noenableDefaultDenysection must place the subject endpoints into default deny, ie reject all ingress traffic that does not match the rules.enableDefaultDeny.ingress: falsemust not place the subject endpoints into default deny. Only the traffic clearly selected by the rules will be impacted by the rule.If there is no
ingressoringressDenystanza:enableDefaultDeny.ingresswith any value: Reject the policy. No change in enforcement, this should ideally occur when the resource is created or at worst case cilium-agent rejects the policy.enableDefaultDeny.ingressorenableDefaultDenystanza: no change.If there is no
enableDefaultDenyfield:Then the equivalent should apply for
egress/egressDenywithenableDefaultDeny.egress.