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

contour: adds global ext auth support to fallback certificates #6558

Merged
merged 1 commit into from
Jul 25, 2024

Conversation

erikflores7
Copy link
Contributor

When using IP to connect, no SNI is sent so the Fallback certificate has to be used in these cases. But when using Fallback certificate, there is currently no support for auth. This change adds the auth filter to add support for the fallback certificate.

Fixes #6512

Signed-off-by: Erik Flores eflores@anduril.com

@erikflores7 erikflores7 requested a review from a team as a code owner July 16, 2024 18:59
@erikflores7 erikflores7 requested review from skriss and sunjayBhatia and removed request for a team July 16, 2024 18:59
@sunjayBhatia sunjayBhatia requested review from a team, izturn and clayton-gonsalves and removed request for a team July 16, 2024 18:59
Copy link

Hi @erikflores7! Welcome to our community and thank you for opening your first Pull Request. Someone will review it soon. Thank you for committing to making Contour better. You can also join us on our mailing list and in our channel in the Kubernetes Slack Workspace

Copy link

codecov bot commented Jul 16, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 81.67%. Comparing base (260151f) to head (d819dfe).
Report is 7 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #6558      +/-   ##
==========================================
+ Coverage   81.64%   81.67%   +0.03%     
==========================================
  Files         133      133              
  Lines       15873    15901      +28     
==========================================
+ Hits        12959    12987      +28     
  Misses       2620     2620              
  Partials      294      294              
Files Coverage Δ
internal/featuretests/v3/envoy.go 99.28% <100.00%> (+0.04%) ⬆️
internal/xdscache/v3/listener.go 92.12% <100.00%> (+0.10%) ⬆️

... and 2 files with indirect coverage changes

@skriss skriss added the release-note/minor A minor change that needs about a paragraph of explanation in the release notes. label Jul 16, 2024
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.

Thanks for the PR! Code change LGTM, just need to add a test case. You'll also need to make sure all of your commits have DCO signoff.

internal/xdscache/v3/listener.go Show resolved Hide resolved
@erikflores7 erikflores7 force-pushed the fallback-global-auth branch from d5700ca to 22c99fe Compare July 25, 2024 10:26
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.

Thanks @erikflores7, LGTM!

…ing Fallback certificates, the global auth was previously ignored. This is needed when using IP routing with no SNI. \n Fixes projectcontour#6512

Signed-off-by: Erik Flores <eflores@anduril.com>
@erikflores7 erikflores7 force-pushed the fallback-global-auth branch from bcbb92f to d819dfe Compare July 25, 2024 17:01
@erikflores7
Copy link
Contributor Author

Thank you for the quick reviews!

By the way, would you happen to know what the lift is to get this added at the HTTPProxy level? There's a comment about the different connection managers: https://github.com/projectcontour/contour/blob/main/internal/dag/httpproxy_processor.go#L280-L290

I have to look into this more, but I believe when using the Global Ext Auth that the auth check fires before an HTTP -> HTTPS redirect happens where as when setting auth at proxy level that doesn't seem to be an issue

@skriss
Copy link
Member

skriss commented Jul 25, 2024

By the way, would you happen to know what the lift is to get this added at the HTTPProxy level? There's a comment about the different connection managers: https://github.com/projectcontour/contour/blob/main/internal/dag/httpproxy_processor.go#L280-L290

I have to look into this more, but I believe when using the Global Ext Auth that the auth check fires before an HTTP -> HTTPS redirect happens where as when setting auth at proxy level that doesn't seem to be an issue

Just to clarify, the use case that isn't working for you is when global ExtAuthz is enabled, and you have TLS-enabled HTTPProxies, and insecure HTTP requests being made to them? Because I would expect global ext auth to already work correctly for both (a) HTTPProxies without TLS (this was what the feature was originally implemented for), and (b) HTTPProxies with TLS, and requests made over HTTPS/TLS.

@skriss skriss merged commit 19626e9 into projectcontour:main Jul 25, 2024
26 checks passed
geomacy pushed a commit to chaosbox/contour that referenced this pull request Aug 22, 2024
Enables the ExtAuthz filter on the fallback cert
filter chain when configuring global external auth,
to support external auth for requests without SNI.

Fixes projectcontour#6512.

Signed-off-by: Erik Flores <eflores@anduril.com>
Signed-off-by: Geoff Macartney <geoff.macartney@sky.uk>
SamMHD pushed a commit to SamMHD/contour that referenced this pull request Sep 8, 2024
Enables the ExtAuthz filter on the fallback cert
filter chain when configuring global external auth,
to support external auth for requests without SNI.

Fixes projectcontour#6512.

Signed-off-by: Erik Flores <eflores@anduril.com>
Signed-off-by: Saman Mahdanian <saman@mahdanian.xyz>
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.

Allow External Auth when Fallback Certificate is used
2 participants