feat: support forward client cert config XFCC header #3202

zufardhiyaulhaq · 2024-04-15T14:21:26Z

What type of PR is this?
configure envoy gateway to propagate XFCC header

What this PR does / why we need it:
this can be used to authorize client based on certificate from XFCC header

Which issue(s) this PR fixes:
#2599

Signed-off-by: zufardhiyaulhaq <zufardhiyaulhaq@gmail.com>

api/v1alpha1/clienttrafficpolicy_types.go

Signed-off-by: zufardhiyaulhaq <zufardhiyaulhaq@gmail.com>

internal/xds/translator/listener.go

api/v1alpha1/clienttrafficpolicy_types.go

Signed-off-by: zufardhiyaulhaq <zufardhiyaulhaq@gmail.com>

…struct Signed-off-by: zufardhiyaulhaq <zufardhiyaulhaq@gmail.com>

zufardhiyaulhaq · 2024-04-20T11:58:31Z

@zhaohuabing @arkodg @guydc moving XFCC configuration to tls.clientvalidation and put mode & XFCC data selection when forwarding into 1 struct.

please help review

Signed-off-by: zufardhiyaulhaq <zufardhiyaulhaq@gmail.com>

api/v1alpha1/tls_types.go

arkodg · 2024-05-06T22:50:29Z

hey @zufardhiyaulhaq a few comments have been added, with those addressed, this PR should be good be be merged

zufardhiyaulhaq · 2024-05-11T11:54:19Z

Hi @arkodg sorry been of for a while, will work on this.
Thank you!

Signed-off-by: zufardhiyaulhaq <zufardhiyaulhaq@gmail.com>

arkodg

LGTM thanks !

api/v1alpha1/clienttrafficpolicy_types.go

Signed-off-by: zufardhiyaulhaq <zufardhiyaulhaq@gmail.com>

zufardhiyaulhaq · 2024-05-19T04:28:18Z

@arkodg please help review,
thank you!

shawnh2

Adding some comments, basically LGTM

internal/gatewayapi/clienttrafficpolicy.go

api/v1alpha1/clienttrafficpolicy_types.go

shawnh2 · 2024-05-20T03:11:56Z

internal/xds/translator/listener.go

+	if in != nil {
+		if in.ForwardClientCert != nil {


can we merge these conditions? just to simplify the code

I don't think we can merge this, there is possibility that headers is nil.

shawnh2 · 2024-05-20T03:13:35Z

internal/xds/translator/listener.go

+	if in == nil {
+		return nil
+	}
+
+	if in.ForwardClientCert == nil {
+		return nil
+	}
+
+	if len(in.ForwardClientCert.CertDetailsToAdd) == 0 {
+		return nil
+	}


I don't think we can merge this, there is possibility that headers & forwardclientcert is is nil.

shawnh2 · 2024-05-20T03:17:47Z

api/v1alpha1/clienttrafficpolicy_types.go

+	// Specifies the fields in the client certificate to be forwarded on the x-forwarded-client-cert (XFCC) HTTP header
+	// +kubebuilder:validation:MaxItems=5
+	// +optional
+	CertDetailsToAdd []ClientCertData `json:"certDetailsToAdd,omitempty"`


should we check the uniqueness of its element?

kubebuilder is not supporting checking unique of this elements

Signed-off-by: zufardhiyaulhaq <zufardhiyaulhaq@gmail.com>

arkodg

LGTM thanks for addressing the changes !

arkodg · 2024-05-20T16:08:56Z

@zufardhiyaulhaq can you run make generate again, see some failures in CI

codecov · 2024-05-20T16:10:05Z

Codecov Report

Attention: Patch coverage is 91.48936% with 4 lines in your changes are missing coverage. Please review.

Project coverage is 67.36%. Comparing base (711d545) to head (812d2c8).
Report is 2 commits behind head on main.

Files	Patch %	Lines
internal/xds/translator/listener.go	89.74%	2 Missing and 2 partials ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #3202   +/-   ##
=======================================
  Coverage   67.36%   67.36%           
=======================================
  Files         166      166           
  Lines       19213    19293   +80     
=======================================
+ Hits        12942    12996   +54     
- Misses       5342     5365   +23     
- Partials      929      932    +3

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Signed-off-by: zufardhiyaulhaq <zufardhiyaulhaq@gmail.com>

zhaohuabing · 2024-05-20T16:57:10Z

api/v1alpha1/clienttrafficpolicy_types.go

+
+// Envoy Proxy mode how to handle the x-forwarded-client-cert (XFCC) HTTP header.
+// +kubebuilder:validation:Enum=Sanitize;ForwardOnly;AppendForward;SanitizeSet;AlwaysForwardOnly
+type ForwardMode string


ForwardMode might be a bit vague here since it's a package-wide type. Perhaps adding a XFCC prefix could enhance clarity.

Suggested change

type ForwardMode string

type XFCCForwardMode string

zhaohuabing · 2024-05-20T16:58:21Z

api/v1alpha1/clienttrafficpolicy_types.go

+
+const (
+	// Do not send the XFCC header to the next hop. This is the default value.
+	ForwardModeSanitize ForwardMode = "Sanitize"


Same here, and the rest of modes.

Suggested change

ForwardModeSanitize ForwardMode = "Sanitize"

XFCCForwardModeSanitize XFCCForwardMode = "Sanitize"

zhaohuabing · 2024-05-20T16:59:04Z

api/v1alpha1/clienttrafficpolicy_types.go

+// Specifies the fields in the client certificate to be forwarded on the x-forwarded-client-cert (XFCC) HTTP header
+// By default, x-forwarded-client-cert (XFCC) will always include By and Hash data
+// +kubebuilder:validation:Enum=Subject;Cert;Chain;Dns;Uri
+type ClientCertData string


Same here.

Suggested change

type ClientCertData string

type XFCCCertData string

zhaohuabing · 2024-05-20T16:59:36Z

api/v1alpha1/clienttrafficpolicy_types.go

+
+const (
+	// Whether to forward the subject of the client cert.
+	ClientCertDataSubject ClientCertData = "Subject"


Same here, and the rest of constants.

Suggested change

ClientCertDataSubject ClientCertData = "Subject"

XFCCCertDataSubject XFCCCertData = "Subject"

zhaohuabing · 2024-05-20T17:16:05Z

api/v1alpha1/clienttrafficpolicy_types.go

+)
+
+// Specifies the fields in the client certificate to be forwarded on the x-forwarded-client-cert (XFCC) HTTP header
+// By default, x-forwarded-client-cert (XFCC) will always include By and Hash data


It might be useful to clarify what is Hash and By in the comment.

zhaohuabing

Let's get this in and address the comments in a follow-up PR.

feat: support forward client cert config XFCC headers

8b806a1

Signed-off-by: zufardhiyaulhaq <zufardhiyaulhaq@gmail.com>

zufardhiyaulhaq requested a review from a team as a code owner April 15, 2024 14:21

arkodg reviewed Apr 15, 2024

View reviewed changes

api/v1alpha1/clienttrafficpolicy_types.go Outdated Show resolved Hide resolved

feat: add clientCertDetailsConfiguration to configure xfcc header

6d4824c

Signed-off-by: zufardhiyaulhaq <zufardhiyaulhaq@gmail.com>

zufardhiyaulhaq requested a review from arkodg April 16, 2024 16:09

guydc reviewed Apr 17, 2024

View reviewed changes

internal/xds/translator/listener.go Show resolved Hide resolved

arkodg reviewed Apr 18, 2024

View reviewed changes

api/v1alpha1/clienttrafficpolicy_types.go Outdated Show resolved Hide resolved

zufardhiyaulhaq added 4 commits April 20, 2024 13:20

Merge branch 'main' into support-forward-client-details-XFCC

6d14974

feat: fix sites

819f9cf

Signed-off-by: zufardhiyaulhaq <zufardhiyaulhaq@gmail.com>

feat: fix unit tests

cc85e7b

Signed-off-by: zufardhiyaulhaq <zufardhiyaulhaq@gmail.com>

feat: move xfcc configuration to tls.clientvalidation & group into 1 …

ca813a4

…struct Signed-off-by: zufardhiyaulhaq <zufardhiyaulhaq@gmail.com>

zufardhiyaulhaq requested review from zhaohuabing, arkodg and guydc April 20, 2024 11:58

zufardhiyaulhaq added 3 commits April 21, 2024 00:47

Merge branch 'envoyproxy:main' into support-forward-client-details-XFCC

a380bc7

feat: fix gen-check

c19d46b

Signed-off-by: zufardhiyaulhaq <zufardhiyaulhaq@gmail.com>

feat: fix API removing uniqueItems & trailing space on test data

dc63165

Signed-off-by: zufardhiyaulhaq <zufardhiyaulhaq@gmail.com>

arkodg reviewed Apr 23, 2024

View reviewed changes

api/v1alpha1/tls_types.go Outdated Show resolved Hide resolved

arkodg reviewed Apr 23, 2024

View reviewed changes

api/v1alpha1/tls_types.go Outdated Show resolved Hide resolved

arkodg reviewed Apr 23, 2024

View reviewed changes

api/v1alpha1/tls_types.go Outdated Show resolved Hide resolved

arkodg reviewed Apr 23, 2024

View reviewed changes

api/v1alpha1/tls_types.go Outdated Show resolved Hide resolved

zufardhiyaulhaq mentioned this pull request May 11, 2024

Support TLS.FrontendValidation within the Gateway spec #3316

Open

zufardhiyaulhaq added 2 commits May 12, 2024 21:46

Merge branch 'main' into support-forward-client-details-XFCC

ecb05dd

fix: API & implementation moving ForwardClientSet to Headers

f73d19b

Signed-off-by: zufardhiyaulhaq <zufardhiyaulhaq@gmail.com>

arkodg previously approved these changes May 13, 2024

View reviewed changes

arkodg requested review from a team May 13, 2024 20:18

arkodg reviewed May 13, 2024

View reviewed changes

api/v1alpha1/clienttrafficpolicy_types.go Outdated Show resolved Hide resolved

arkodg self-requested a review May 13, 2024 20:21

zufardhiyaulhaq added 2 commits May 18, 2024 14:54

Merge branch 'main' into support-forward-client-details-XFCC

e32525b

feat: move unit tests to headers & fix logic on headers

162c4a0

Signed-off-by: zufardhiyaulhaq <zufardhiyaulhaq@gmail.com>

zufardhiyaulhaq dismissed arkodg’s stale review via 162c4a0 May 18, 2024 09:06

zufardhiyaulhaq added 4 commits May 18, 2024 17:04

feat: fix logic on xds listener & unit tests

0a20704

Signed-off-by: zufardhiyaulhaq <zufardhiyaulhaq@gmail.com>

feat: fix gen-check

c611348

Signed-off-by: zufardhiyaulhaq <zufardhiyaulhaq@gmail.com>

feat: fix gen-check

4bba6ca

Signed-off-by: zufardhiyaulhaq <zufardhiyaulhaq@gmail.com>

feat: fix lint

562a6b3

Signed-off-by: zufardhiyaulhaq <zufardhiyaulhaq@gmail.com>

zufardhiyaulhaq requested a review from cnvergence May 19, 2024 08:23

shawnh2 reviewed May 20, 2024

View reviewed changes

zufardhiyaulhaq added 2 commits May 20, 2024 22:47

feat: change forwardClientCert to xForwardedClientCert

a82c4a8

Signed-off-by: zufardhiyaulhaq <zufardhiyaulhaq@gmail.com>

Merge branch 'main' into support-forward-client-details-XFCC

de03f58

arkodg previously approved these changes May 20, 2024

View reviewed changes

arkodg self-requested a review May 20, 2024 16:08

fix gen-check job

812d2c8

Signed-off-by: zufardhiyaulhaq <zufardhiyaulhaq@gmail.com>

zufardhiyaulhaq dismissed arkodg’s stale review via 812d2c8 May 20, 2024 16:52

zhaohuabing reviewed May 20, 2024

View reviewed changes

arkodg approved these changes May 20, 2024

View reviewed changes

zhaohuabing approved these changes May 20, 2024

View reviewed changes

zhaohuabing merged commit b99888a into envoyproxy:main May 20, 2024
26 checks passed

zhaohuabing mentioned this pull request May 20, 2024

API: Follow up on XFCC header #3443

Merged

arkodg mentioned this pull request May 21, 2024

Propagate XFCC header to the backend #2599

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: support forward client cert config XFCC header #3202

feat: support forward client cert config XFCC header #3202

zufardhiyaulhaq commented Apr 15, 2024

zufardhiyaulhaq commented Apr 20, 2024

arkodg commented May 6, 2024

zufardhiyaulhaq commented May 11, 2024

arkodg left a comment

zufardhiyaulhaq commented May 19, 2024

shawnh2 left a comment

shawnh2 May 20, 2024

zufardhiyaulhaq May 20, 2024

shawnh2 May 20, 2024

zufardhiyaulhaq May 20, 2024

shawnh2 May 20, 2024

zufardhiyaulhaq May 20, 2024

arkodg left a comment

arkodg commented May 20, 2024

codecov bot commented May 20, 2024 •

edited

Loading

zhaohuabing May 20, 2024 •

edited

Loading

zhaohuabing May 20, 2024 •

edited

Loading

zhaohuabing May 20, 2024 •

edited

Loading

zhaohuabing May 20, 2024 •

edited

Loading

zhaohuabing May 20, 2024 •

edited

Loading

zhaohuabing left a comment

	ForwardModeSanitize ForwardMode = "Sanitize"
	XFCCForwardModeSanitize XFCCForwardMode = "Sanitize"

	ClientCertDataSubject ClientCertData = "Subject"
	XFCCCertDataSubject XFCCCertData = "Subject"

feat: support forward client cert config XFCC header #3202

feat: support forward client cert config XFCC header #3202

Conversation

zufardhiyaulhaq commented Apr 15, 2024

zufardhiyaulhaq commented Apr 20, 2024

arkodg commented May 6, 2024

zufardhiyaulhaq commented May 11, 2024

arkodg left a comment

Choose a reason for hiding this comment

zufardhiyaulhaq commented May 19, 2024

shawnh2 left a comment

Choose a reason for hiding this comment

shawnh2 May 20, 2024

Choose a reason for hiding this comment

zufardhiyaulhaq May 20, 2024

Choose a reason for hiding this comment

shawnh2 May 20, 2024

Choose a reason for hiding this comment

zufardhiyaulhaq May 20, 2024

Choose a reason for hiding this comment

shawnh2 May 20, 2024

Choose a reason for hiding this comment

zufardhiyaulhaq May 20, 2024

Choose a reason for hiding this comment

arkodg left a comment

Choose a reason for hiding this comment

arkodg commented May 20, 2024

codecov bot commented May 20, 2024 • edited Loading

Codecov Report

zhaohuabing May 20, 2024 • edited Loading

Choose a reason for hiding this comment

zhaohuabing May 20, 2024 • edited Loading

Choose a reason for hiding this comment

zhaohuabing May 20, 2024 • edited Loading

Choose a reason for hiding this comment

zhaohuabing May 20, 2024 • edited Loading

Choose a reason for hiding this comment

zhaohuabing May 20, 2024 • edited Loading

Choose a reason for hiding this comment

zhaohuabing left a comment

Choose a reason for hiding this comment

codecov bot commented May 20, 2024 •

edited

Loading

zhaohuabing May 20, 2024 •

edited

Loading

zhaohuabing May 20, 2024 •

edited

Loading

zhaohuabing May 20, 2024 •

edited

Loading

zhaohuabing May 20, 2024 •

edited

Loading

zhaohuabing May 20, 2024 •

edited

Loading