Enhance L7 NetworkPolicy to support TLS protocol #4932

hongliangl · 2023-05-04T13:09:22Z

No description provided.

build/charts/antrea/crds/networkpolicy.yaml

luolanzone · 2023-05-12T08:21:47Z

@hongliangl I didn't see any doc update in this PR, do you plan to create a separate PR to update L7 NP doc for tls protocol?

hongliangl · 2023-05-12T08:23:33Z

@hongliangl I didn't see any doc update in this PR, do you plan to create a separate PR to update L7 NP doc for tls protocol?

Will add a doc later. I'm not sure if this PR could be merge in v1.12. If we plan to merge this PR, I can add a doc quickly.

luolanzone

Please resolve conflicts and update docs for L7 TLS support.

luolanzone · 2023-07-07T06:16:49Z

pkg/agent/controller/networkpolicy/l7engine/reconciler.go

+	if tls.SNI != "" {
+		keywords = append(keywords, fmt.Sprintf(`tls.sni; content:"%s"; nocase; pcre:"/%s$/;"`, tls.SNI, tls.SNI))
+	}
+	return strings.Join(keywords, " ")


Do you need a slice keywords here? seems there will be only one string.

We could add more key words in the future.

tnqn

LGTM overall, can we have an e2e test to cover it? And please update antrea-l7-network-policy.md.

pkg/agent/controller/networkpolicy/l7engine/reconciler.go

pkg/apis/crd/v1alpha1/types.go

hongliangl · 2023-07-17T06:48:23Z

LGTM overall, can we have an e2e test to cover it? And please update antrea-l7-network-policy.md.

Thanks for reviewing this PR, will address the comments and update antrea-l7-network-policy.md later

jianjuns · 2023-07-07T19:01:39Z

pkg/apis/controlplane/v1beta2/types.go

@@ -320,6 +321,14 @@ type HTTPProtocol struct {
 	Path string `json:"path,omitempty" protobuf:"bytes,3,opt,name=path"`
 }

+// TLSProtocol matches TLS requests with specific SNI. If the field is not provided, this


What does "TLS requests" refer to?

the specific SNI

I follow the style of HTTP like the following in existing code:

// HTTPProtocol matches HTTP requests with specific host, method, and path. All // fields could be used alone or together. If all fields are not provided, this // matches all HTTP requests. type HTTPProtocol struct { // Host represents the hostname present in the URI or the HTTP Host header to match. // It does not contain the port associated with the host. Host string // Method represents the HTTP method to match. // It could be GET, POST, PUT, HEAD, DELETE, TRACE, OPTIONS, CONNECT and PATCH. Method string // Path represents the URI path to match (Ex. "/index.html", "/admin"). Path string }

Is the comment "TLS requests" confused in some degree? If so, could you give some suggestions? Thanks a lot @jianjuns

Perhaps say TLS handshake?

I think TLS handshake is okay for me.

Is it "TLS handshake requests"?

I searched for the words TLS handshake requests but found no usage of TLS handshake and requests together. How about just TLS handshake?

Then let us use "handshake packets". "handshake" is a process that includes rounds of message exchanges.

tnqn · 2023-07-19T08:33:03Z

docs/antrea-l7-network-policy.md

@@ -7,7 +7,9 @@
 - [Prerequisites](#prerequisites)
 - [Usage](#usage)
  - [HTTP](#http)
-    - [More examples](#more-examples)
+    - [More examples - HTTP](#more-examples-http)


This is inconsistent with the actual section name. TOC check should be failing.
And I feel redundant to append HTTP when its parent section is already HTTP. If you are trying to avoid section name conflict, the second anchor of the second section will have "-1", and toc generator tool will do it automatically.

I didn't know that we could generate TOC by calling make toc. I have updated and checked.

docs/antrea-l7-network-policy.md

tnqn · 2023-07-19T08:34:51Z

docs/antrea-l7-network-policy.md

+apiVersion: crd.antrea.io/v1alpha1
+kind: NetworkPolicy
+metadata:
+  name: ingress-allow-tls-request-to-api-v2


The name doesn't reflect what the policy really does

Updated to ingress-allow-tls-hanshake

docs/antrea-l7-network-policy.md

tnqn · 2023-07-19T08:44:54Z

docs/antrea-l7-network-policy.md

+apiVersion: crd.antrea.io/v1alpha1
+kind: NetworkPolicy
+metadata:
+  name: allow-http-only


Updated to allow-tls-only

tnqn · 2023-07-19T08:46:15Z

docs/antrea-l7-network-policy.md

+apiVersion: crd.antrea.io/v1alpha1
+kind: ClusterNetworkPolicy
+metadata:
+  name: allow-web-tls-access-to-internal-domain


web should not be part of it. this rule doesn't indicate web at all.

Updated to allow-tls-handshake-to-internal

tnqn · 2023-07-19T08:51:10Z

multicluster/build/yamls/antrea-multicluster-leader-namespaced.yml

@@ -356,7 +356,7 @@ spec:
  template:
    metadata:
      annotations:
-        checksum/config: 7eb0f1e65f7eb3e35b0739d6064b92b7621af0f4e41813c35bfdee71ceaefbe2
+        checksum/config: 


Is it correct to use empty?

No. I found that in MACOS, sha256sum used in multicluster/hack/update-checksum.sh is not available. I have regenerated the manifest files with alternative shasum -a 256.

tnqn · 2023-07-19T08:54:04Z

pkg/apis/controlplane/types.go

+	// SNI indicates the domain name in the TLS hello message, allowing the server to match it with the correct digital
+	// certificate for that domain.


"allowing the server to match it with the correct digital certificate for that domain."
I don't think we have this functionality? We just check if the actual SNI matches the one set in the policy.

Removed allowing the server to match it with the correct digital certificate for that domain.

tnqn · 2023-07-19T08:57:33Z

test/e2e/l7networkpolicy_test.go

+	time.Sleep(networkPolicyDelay)
+


Check www.google.com is not reachable now.

Changed to apis.google.com.

jianjuns · 2023-07-19T17:07:10Z

docs/antrea-l7-network-policy.md

@@ -201,6 +203,92 @@ spec:
        - http: {}       # automatically dropped, and subsequent rules will not be considered.
 ```

+### TLS
+
+A typical layer 7 NetworkPolicy for TLS protocol is as below:


A typical -> An example

the TLS protocol

jianjuns · 2023-07-19T17:08:23Z

docs/antrea-l7-network-policy.md

+        matchLabels:
+          app: web
+  ingress:
+    - name: allow-tls    # Allow inbound TLS/SSL handshakes to server name "foo.bar.com" from Pods with app=client label.


"handshake requests" or "handshake packets"

with label "app=client"

jianjuns · 2023-07-19T17:09:43Z

docs/antrea-l7-network-policy.md

+      action: Drop
+```
+
+**sni**: The `sni` field matches the TLS/SSL Server Name Indication (SNI) field included in the TLS/SSL handshake process.


Remove "included"?

jianjuns · 2023-07-19T17:11:02Z

docs/antrea-l7-network-policy.md

+          port: 53
+    - name: allow-tls-only      # Allow outbound SSL/TLS handshakes towards "*.bar.com". 
+      action: Allow             # As the rule's "to" and "ports" are empty, which means it selects traffic to any network
+      l7Protocols:              # peer's any port using any transport protocol, all outbound SSL/TLS handshakes towards other


using -> of

jianjuns · 2023-07-19T17:11:37Z

docs/antrea-l7-network-policy.md

+            sni: "*.bar.com"    # will not be considered.
+```
+
+The following NetworkPolicy blocks network traffic using an unauthorized application protocol regardless of port used.


port -> the port

jianjuns · 2023-07-19T17:12:07Z

docs/antrea-l7-network-policy.md

+        matchLabels:
+          app: web
+  ingress:
+    - name: tls-only     # Allow inbound SSL/TLS requests only.


handshake requests

jianjuns · 2023-07-19T17:12:57Z

pkg/apis/controlplane/types.go

@@ -330,6 +331,13 @@ type HTTPProtocol struct {
 	Path string
 }

+// TLSProtocol matches TLS handshakes with specific SNI. If the field is not provided, this


handshake requests or packets

Signed-off-by: Hongliang Liu <lhongliang@vmware.com>

tnqn

LGTM, but defer to @jianjuns for final approval

jianjuns

A nit.

jianjuns · 2023-07-20T17:03:00Z

docs/antrea-l7-network-policy.md

@@ -75,7 +77,7 @@ welcome feature requests for protocols that you are interested in.

 ### HTTP

-A typical layer 7 NetworkPolicy for HTTP protocol is as below:
+An example of layer 7 NetworkPolicy for the HTTP protocol is like below:


Remove "of"

Done, thanks for reviewing this PRs.

Signed-off-by: Hongliang Liu <lhongliang@vmware.com>

tnqn

LGTM

tnqn · 2023-07-21T01:57:22Z

/test-all

hongliangl requested review from tnqn and qiyueyao May 4, 2023 13:09

hongliangl force-pushed the 20230312-l7-tls branch 2 times, most recently from fab937c to ac943e8 Compare May 5, 2023 00:38

hongliangl added the action/release-note Indicates a PR that should be included in release notes. label May 5, 2023

hongliangl added this to the Antrea v1.13 release milestone May 5, 2023

qiyueyao reviewed May 5, 2023

View reviewed changes

build/charts/antrea/crds/networkpolicy.yaml Show resolved Hide resolved

build/charts/antrea/crds/networkpolicy.yaml Outdated Show resolved Hide resolved

hongliangl force-pushed the 20230312-l7-tls branch 2 times, most recently from 50ba5ea to 4366978 Compare May 6, 2023 02:37

luolanzone modified the milestone: Antrea v1.13 release Jul 6, 2023

luolanzone reviewed Jul 7, 2023

View reviewed changes

tnqn reviewed Jul 17, 2023

View reviewed changes

pkg/agent/controller/networkpolicy/l7engine/reconciler.go Outdated Show resolved Hide resolved

pkg/apis/crd/v1alpha1/types.go Outdated Show resolved Hide resolved

jianjuns reviewed Jul 17, 2023

View reviewed changes

hongliangl force-pushed the 20230312-l7-tls branch 4 times, most recently from fc32b93 to 1fd3087 Compare July 19, 2023 08:49

tnqn reviewed Jul 19, 2023

View reviewed changes

hongliangl force-pushed the 20230312-l7-tls branch 6 times, most recently from 403767b to ace16ad Compare July 19, 2023 13:46

jianjuns reviewed Jul 19, 2023

View reviewed changes

Enhance L7 NetworkPolicy to support TLS protocol

1dc0d11

Signed-off-by: Hongliang Liu <lhongliang@vmware.com>

hongliangl force-pushed the 20230312-l7-tls branch 2 times, most recently from 0f2aa1d to 1b4d153 Compare July 20, 2023 06:50

tnqn reviewed Jul 20, 2023

View reviewed changes

jianjuns previously approved these changes Jul 20, 2023

View reviewed changes

Enhance L7 NetworkPolicy to support TLS protocol

2063059

Signed-off-by: Hongliang Liu <lhongliang@vmware.com>

hongliangl dismissed jianjuns’s stale review via 2063059 July 21, 2023 01:11

hongliangl force-pushed the 20230312-l7-tls branch from 1b4d153 to 2063059 Compare July 21, 2023 01:11

hongliangl requested review from jianjuns and tnqn July 21, 2023 01:12

tnqn approved these changes Jul 21, 2023

View reviewed changes

tnqn merged commit 9c09f7c into antrea-io:main Jul 21, 2023
41 of 43 checks passed

hongliangl deleted the 20230312-l7-tls branch July 21, 2023 08:46

		// SNI indicates the domain name in the TLS hello message, allowing the server to match it with the correct digital
		// certificate for that domain.

Enhance L7 NetworkPolicy to support TLS protocol #4932

Enhance L7 NetworkPolicy to support TLS protocol #4932

Conversation

hongliangl commented May 4, 2023

luolanzone commented May 12, 2023

hongliangl commented May 12, 2023

luolanzone left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hongliangl Jul 19, 2023 • edited Loading

Choose a reason for hiding this comment

tnqn left a comment

Choose a reason for hiding this comment

hongliangl commented Jul 17, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hongliangl Jul 19, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tnqn left a comment

Choose a reason for hiding this comment

jianjuns left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tnqn left a comment

Choose a reason for hiding this comment

tnqn commented Jul 21, 2023

hongliangl Jul 19, 2023 •

edited

Loading

hongliangl Jul 19, 2023 •

edited

Loading