Add deny policies #12716

aanm · 2020-07-29T21:17:35Z

Deny rules will be configured with ingressDeny and egressDeny.

Policy example:

apiVersion: "cilium.io/v2"
kind: CiliumNetworkPolicy
metadata:
  name: "guestbook-web"
spec:
  endpointSelector:
    matchLabels:
      k8s-app.guestbook: web
  ingress:
  - fromEndpoints:
    - matchLabels:
        k8s-app.guestbook: redis
  ingressDeny:
  - fromEndpoints:
    - matchLabels:
        k8s-app.guestbook: redis

Since in the datapath the denies take priority over the allow rules, this rule will drop all packets from any pod running with the label k8s-app.guestbook=redis in the default namespace that tries to reach any pod running with the label k8s-app.guestbook=web.

The ingressDeny differs from the ingress as the L7 rules are not available in the L4 section. For example:
ingress:

...
  ingress:
  - fromEndpoints:
    - matchLabels:
        env: prod
    toPorts:
    - ports:
      - port: "80"
        protocol: TCP
      rules:
        http:
        - method: "GET"
          path: "/public"
...

vs
ingressDeny:

...
  ingressDeny:
  - fromEndpoints:
    - matchLabels:
        env: prod
    toPorts:
    - ports:
      - port: "80"
        protocol: TCP
...

And in egressDeny vs egress the toFQDNs are not available as well as, similar to the ingressDeny, the L7 Rules.

egress:

...
  egress:
  - toEndpoints:
    - matchLabels:
       "k8s:io.kubernetes.pod.namespace": kube-system
       "k8s:k8s-app": kube-dns
    toPorts:
      - ports:
         - port: "53"
           protocol: ANY
        rules:
          dns:
            - matchPattern: "*"
  - toFQDNs:
      - matchName: "cilium.io"
      - matchName: "sub.cilium.io"
      - matchPattern: "*.sub.cilium.io"
...

egressDeny:

...
  egressDeny:
  - toEndpoints:
    - matchLabels:
       "k8s:io.kubernetes.pod.namespace": kube-system
       "k8s:k8s-app": kube-dns
    toPorts:
      - ports:
         - port: "53"
           protocol: ANY
...

TODO:

Fix tests
Add tests
Documentation
Change the slightly the exposed API to the users since deny policies won't have L7 policies initially.
This was done in the last commit if we don't like it we can always revert it.

Fixes #7111

aanm · 2020-07-29T21:17:56Z

test-me-please

aanm · 2020-07-29T21:21:56Z

test-me-please

jrajahalme

Looking good, some minor comments only so far. Overall one would hope that most of the code duplication for the deny case could be avoided by refactoring code.

bpf/lib/policy.h

pkg/endpoint/bpf.go

jrajahalme

Some questions for clarification. Also, it's a pity that the last commit makes code less readable :-(

pkg/endpoint/endpoint_status.go

jrajahalme · 2020-08-04T00:51:15Z

pkg/policy/repository.go

+				}
+			}
+		} else if len(ingressPolicy) > 0 {
+			verdict = ingressPolicy.IngressCoversContext(ctx)


Normal policy should still be considered if verdict after checking deny policies is not api.Denied, so this can not be in an else block.

nice catch!

pkg/policy/repository.go

pkg/policy/resolve.go

jrajahalme · 2020-08-04T01:01:42Z

pkg/policy/rule.go

+			found += cnt
+		}
+	}
+


I wonder if SetDeny(true) needs to be called before this block as well?

jrajahalme · 2020-08-04T01:01:49Z

pkg/policy/rule.go

+			found += cnt
+		}
+	}
+


I wonder if SetDeny(true) needs to be called before this block as well?

aanm · 2020-08-04T01:29:23Z

test-me-please

coveralls · 2020-08-04T02:18:49Z

Coverage increased (+0.04%) to 37.173% when pulling 94b466831133a88a11499dd369be30de3170fa28 on aanm:pr/denypolicies into bd89e83 on cilium:master.

aanm · 2020-08-05T19:57:45Z

test-me-please

pchaigno

LGTM overall. My only concern is around handling of AlwaysAllowLocalhost when deny policy rules are loaded, in particular with L4-only deny rules (see comment below). I think all other comments can be addressed as follow ups.

Since L7 rules are not supported in deny rules, should we reject them during validation?

That last commit is really hard to review. Couldn't it be broken up in several commits?

bpf/lib/maps.h

bpf/lib/policy.h

pkg/k8s/apis/cilium.io/v2/register.go

pkg/policy/mapstate.go

pkg/policy/repository.go

aanm · 2020-08-06T17:08:41Z

@pchaigno thanks for the review

Since L7 rules are not supported in deny rules, should we reject them during validation?

How? I mean, there isn't any L7 field in deny rules, so how could even be validated (or where)?

That last commit is really hard to review. Couldn't it be broken up in several commits?

Unfortunately it couldn't, the unit tests would have been broken if that would be the case.

aanm · 2020-10-02T17:26:10Z

test-me-please

christarazi

🎉 Mainly looked over CI code changes this time around.

aanm · 2020-10-02T19:00:45Z

test-focus K8sPolicyTest*

aanm · 2020-10-02T19:56:14Z

test-focus K8sPolicyTest*

joestringer

I did another brief pass particularly looking at the docs, I think there's some improvements we can make before release but I'm OK to push those out to a subsequent PR.

Given the level of review we've done between Jarno, you and I across the core logic + eyes on the overall k8s API behaviour from a broader set of folks, I think we have nailed down the desired behaviour. Furthermore we've caught a bunch of issues and added testing for several areas that were previously ambiguous so I'm getting towards diminishing returns on spending more time staring at the code in this PR. Given the beta tag I think we're safe to get this in, and push it out for broader exposure, testing, and iteration on top of this base.

Are there any remaining areas you feel could do with more eyes/attention?

Documentation/policy/language.rst

joestringer · 2020-10-02T19:40:06Z

Documentation/policy/language.rst

+The current known limitation is a deny policy with ``toEntities`` "world" for
+which a ``toFQDNs`` can bypass it.


Maybe we should file an issue for this and reference it here so if users are concerned about it, they can 👍 and/or investigate?

Documentation/policy/language.rst

pkg/maps/policymap/policymap.go

joestringer · 2020-10-02T19:49:22Z

pkg/maps/policymap/policymap.go

+	Flags     uint8  `align:"deny"`
+	Pad0      uint8  `align:"pad0"`


My comment was not about the naming, it was about the content of the values we populate. I think if we declare the PolicyEntry in Go, it will auto-zero the fields like Pad0 so we should be good.

examples/policies/l3/entities/from_world_deny.yaml

This will allow to reuse functions. Signed-off-by: André Martins <andre@cilium.io>

This function can be refactored out of the pkg/endpoint. With this change it is also possible to unit test this function. Signed-off-by: André Martins <andre@cilium.io>

Create function to de-duplicate code and have the ability to re-use code. Signed-off-by: André Martins <andre@cilium.io>

Signed-off-by: André Martins <andre@cilium.io>

With these changes users will have the ability to dump deny policy maps using 'cilium bpf policy map'. Signed-off-by: André Martins <andre@cilium.io>

This change adds the deny policy enforcement in the datapath. When checking the entry of the policy map, we will check the dedicated bit for policy denies. If that bit is set, which we will consider unlikely to avoid performance penalties for the allow case, the traffic will be dropped. Signed-off-by: André Martins <andre@cilium.io>

These changes only add the fields into the API for both Cilium and Kubernetes interactions. Signed-off-by: André Martins <andre@cilium.io>

Signed-off-by: André Martins <andre@cilium.io>

Since not all functionalities will be available in the IngressDeny and EgressDeny fields of Rule, it was necessary to create a new structure, IngressDeny and EgressDeny. These 2 structures share common fields with the Ingress and Egress except the implementation of L7 Policies, which is part of the PortRule but not of the PortDenyRules. This requirement requires the introduction of some interfaces to avoid code duplication in the policy calculation. Signed-off-by: André Martins <andre@cilium.io>

These changes introduce automatic capability of performing unit tests by adding an algorithm used to derive map states and test cases. This commit keeps the same behavior used in the manually unit tests to check if the new algorithm used to derive map states breaks the old expectations. In follow up changes the manual tests will be removed. Signed-off-by: André Martins <andre@cilium.io>

Signed-off-by: André Martins <andre@cilium.io>

Add documentation and examples for policy denies. Signed-off-by: André Martins <andre@cilium.io>

aanm · 2020-10-02T22:40:06Z

test-me-please

aanm added release-note/major This PR introduces major new functionality to Cilium. dont-merge/preview-only Only for preview or testing, don't merge it. labels Jul 29, 2020

aanm requested a review from jrajahalme July 29, 2020 21:17

aanm requested a review from a team as a code owner July 29, 2020 21:17

aanm requested a review from a team July 29, 2020 21:17

aanm requested review from a team as code owners July 29, 2020 21:17

aanm requested review from a team July 29, 2020 21:17

aanm requested a review from a team as a code owner July 29, 2020 21:17

aanm force-pushed the pr/denypolicies branch from c81e2e8 to ce2d451 Compare July 29, 2020 21:21

jrajahalme reviewed Jul 30, 2020

View reviewed changes

aanm force-pushed the pr/denypolicies branch from ce2d451 to 01a7374 Compare August 3, 2020 20:34

aanm requested a review from a team August 3, 2020 20:34

aanm force-pushed the pr/denypolicies branch from 01a7374 to e6441ca Compare August 3, 2020 20:38

aanm requested a review from jrajahalme August 3, 2020 20:40

jrajahalme reviewed Aug 4, 2020

View reviewed changes

aanm force-pushed the pr/denypolicies branch from e6441ca to 041834a Compare August 4, 2020 01:29

aanm force-pushed the pr/denypolicies branch from 041834a to 65f448e Compare August 5, 2020 19:57

pchaigno requested changes Aug 6, 2020

View reviewed changes

aanm force-pushed the pr/denypolicies branch from 65f448e to a20c16d Compare August 6, 2020 17:25

aanm force-pushed the pr/denypolicies branch 2 times, most recently from 288cfb7 to 65e3463 Compare October 2, 2020 17:22

christarazi approved these changes Oct 2, 2020

View reviewed changes

aanm force-pushed the pr/denypolicies branch from 65e3463 to c54d8f1 Compare October 2, 2020 19:00

aanm force-pushed the pr/denypolicies branch from c54d8f1 to ade5689 Compare October 2, 2020 19:56

joestringer approved these changes Oct 2, 2020

View reviewed changes

aanm added 14 commits October 3, 2020 00:13

k8s/apis: refactor parseToCiliumRule functions

518f005

This will allow to reuse functions. Signed-off-by: André Martins <andre@cilium.io>

refactor desiredPolicyAllowsIdentity out of pkg/endpoint

5b46e9e

This function can be refactored out of the pkg/endpoint. With this change it is also possible to unit test this function. Signed-off-by: André Martins <andre@cilium.io>

pkg/policy: add function to retrieve identities of policyMapState

e542363

Create function to de-duplicate code and have the ability to re-use code. Signed-off-by: André Martins <andre@cilium.io>

pkg/endpoint: remove unused functions

1101789

Signed-off-by: André Martins <andre@cilium.io>

api/v1: add policy deny specific fields

192513a

Signed-off-by: André Martins <andre@cilium.io>

cilium/cmd: dump policy deny maps

9c13098

With these changes users will have the ability to dump deny policy maps using 'cilium bpf policy map'. Signed-off-by: André Martins <andre@cilium.io>

policy/api: add IngressDeny and EgressDeny fields to API

d08314c

These changes only add the fields into the API for both Cilium and Kubernetes interactions. Signed-off-by: André Martins <andre@cilium.io>

pkg/{endpoint,policy}: add policy deny implementation

6f8d073

Signed-off-by: André Martins <andre@cilium.io>

pkg/policy: add unit tests for deny policies

b641a67

Signed-off-by: André Martins <andre@cilium.io>

test: add some e2e coverage for deny policies

6c2b245

Signed-off-by: André Martins <andre@cilium.io>

docs: add deny policies documentation

3dca900

Add documentation and examples for policy denies. Signed-off-by: André Martins <andre@cilium.io>

aanm force-pushed the pr/denypolicies branch from ade5689 to 3dca900 Compare October 2, 2020 22:40

aanm merged commit 6083b06 into cilium:master Oct 3, 2020

aanm deleted the pr/denypolicies branch October 3, 2020 16:00

pchaigno mentioned this pull request Oct 8, 2020

Implement cgroup-bpf filtering kinvolk/egress-filtering-benchmark#10

Merged

joestringer mentioned this pull request Oct 12, 2020

Prepare for release v1.9.0-rc1 #13518

Merged

jrajahalme mentioned this pull request Jun 19, 2023

endpoint: Do not override deny entries with proxy redirects #26344

Merged

		The current known limitation is a deny policy with ``toEntities`` "world" for
		which a ``toFQDNs`` can bypass it.

Add deny policies #12716

Add deny policies #12716

Uh oh!

Conversation

aanm commented Jul 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aanm commented Jul 29, 2020

Uh oh!

aanm commented Jul 29, 2020

Uh oh!

jrajahalme left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jrajahalme left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jrajahalme Aug 4, 2020

Choose a reason for hiding this comment

Uh oh!

aanm Aug 4, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jrajahalme Aug 4, 2020

Choose a reason for hiding this comment

Uh oh!

jrajahalme Aug 4, 2020

Choose a reason for hiding this comment

Uh oh!

aanm commented Aug 4, 2020

Uh oh!

coveralls commented Aug 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aanm commented Aug 5, 2020

Uh oh!

pchaigno left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

aanm commented Aug 6, 2020

Uh oh!

aanm commented Oct 2, 2020

Uh oh!

christarazi left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aanm commented Oct 2, 2020

Uh oh!

aanm commented Oct 2, 2020

Uh oh!

joestringer left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

joestringer Oct 2, 2020

Choose a reason for hiding this comment

aanm commented Jul 29, 2020 •

edited

Loading

coveralls commented Aug 4, 2020 •

edited

Loading

pchaigno left a comment •

edited

Loading

christarazi left a comment •

edited

Loading

joestringer left a comment •

edited

Loading