Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

XFCC header support #4797

Merged
merged 1 commit into from
Oct 21, 2022
Merged

XFCC header support #4797

merged 1 commit into from
Oct 21, 2022

Conversation

gautierdelorme
Copy link
Contributor

This is required to let apps use details from client certificates (e.g. Subject, SAN...). Since the certificate (or the certificate chain) could exceed the web server header size limit, we give the ability to select what specific part of the certificate to expose in the x-forwarded-client-cert header.

Fixes #2885 (I've read through this issue and since that feature is only usable when ClientValidation is used (since that's required for the server to request certs) I think having ForwardClientCertificate under ClientValidation makes more sense)

Signed-off-by: Gautier Delorme [email protected]

@gautierdelorme gautierdelorme requested a review from a team as a code owner October 17, 2022 13:21
@gautierdelorme gautierdelorme requested review from stevesloka and skriss and removed request for a team October 17, 2022 13:21
@codecov
Copy link

codecov bot commented Oct 17, 2022

Codecov Report

Merging #4797 (b7e8dab) into main (ece8a24) will increase coverage by 0.05%.
The diff coverage is 100.00%.

❗ Current head b7e8dab differs from pull request most recent head 16bbf9c. Consider uploading reports for the commit 16bbf9c to get more accurate results

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #4797      +/-   ##
==========================================
+ Coverage   76.10%   76.15%   +0.05%     
==========================================
  Files         140      140              
  Lines       16896    16932      +36     
==========================================
+ Hits        12858    12894      +36     
  Misses       3786     3786              
  Partials      252      252              
Impacted Files Coverage Δ
internal/dag/dag.go 96.62% <ø> (ø)
internal/dag/httpproxy_processor.go 93.19% <100.00%> (+0.05%) ⬆️
internal/envoy/v3/listener.go 98.69% <100.00%> (+0.02%) ⬆️
internal/featuretests/v3/envoy.go 98.98% <100.00%> (+0.02%) ⬆️
internal/xdscache/v3/listener.go 91.30% <100.00%> (+0.16%) ⬆️

@sunjayBhatia sunjayBhatia added the release-note/minor A minor change that needs about a paragraph of explanation in the release notes. label Oct 17, 2022
@gautierdelorme
Copy link
Contributor Author

Do you think this change could make it to the 1.23 release?

@skriss
Copy link
Member

skriss commented Oct 19, 2022

Do you think this change could make it to the 1.23 release?

Unfortunately it's too late to get these into 1.23, but we'll pick up with reviews shortly.

Copy link
Member

@tsaarni tsaarni left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking very good 👍 Just one tiny remark from me.

internal/dag/dag.go Outdated Show resolved Hide resolved
Copy link
Member

@tsaarni tsaarni left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needs merging & regenerating after the other PR was merged, but otherwise ready to go from my perspective.

Signed-off-by: Gautier Delorme <[email protected]>
Copy link
Member

@tsaarni tsaarni left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @gautierdelorme 👍
Leaving for others to review as well.

Copy link
Member

@skriss skriss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks @gautierdelorme! Will leave open for a bit in case @sunjayBhatia wants to take a look.

Copy link
Member

@sunjayBhatia sunjayBhatia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks great!

one thing that could be useful to help troubleshoot header size issues would be to enable upstream cluster req/resp statistics: https://www.envoyproxy.io/docs/envoy/latest/configuration/upstream/cluster_manager/cluster_stats#request-response-size-statistics

I can add a new issue for that so we can consider it (if we dont have one already)

@skriss skriss merged commit c5cbcd9 into projectcontour:main Oct 21, 2022
moeyui1 pushed a commit to moeyui1/contour that referenced this pull request Oct 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-note/minor A minor change that needs about a paragraph of explanation in the release notes.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for forwarding of client certificate material to pod (xfcc in Envoy)
4 participants