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

envoyconfig: add virtual host domains for certificates in addition to routes #3593

Merged
merged 4 commits into from Aug 31, 2022

Conversation

calebdoxsey
Copy link
Contributor

Summary

Currently we only add domains to the envoy config based on routes. This means if we have a route from.example.com and a wildcard certificate *.example.com, that certificate will only be served to requests from from.example.com. So unknown.example.com would end up receiving a self-signed certificate. This PR updates the code to also add the *.example.com as a handled domain. This domain will have no routes attached to it, but it will serve the proper certificate, so the user would no longer receive a self-signed certificate.

Unlike route matching envoy matches filter chains based on most specific to least specific. So if there is a filter chain for *.example.com and another one for from.example.com, if a request comes in for from.example.com it will be sent to that filter chain regardless of the ordering of the filter chains. (so it's ok if *.example.com occurs first in the list)

Related issues

Fixes #3577

Checklist

  • reference any related issues
  • updated docs
  • updated unit tests
  • updated UPGRADING.md
  • add appropriate tag (improvement / bug / etc)
  • ready for review

@coveralls
Copy link

coveralls commented Aug 30, 2022

Coverage Status

Coverage decreased (-0.06%) to 66.403% when pulling 5772f13 on cdoxsey/3577-unknown-routes into 8713108 on main.

Copy link
Contributor

@desimone desimone left a comment

Choose a reason for hiding this comment

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

Few suggestions, mostly around unit tests but otherwise LGTM!

pkg/cryptutil/certificates.go Outdated Show resolved Hide resolved
config/envoyconfig/listeners.go Show resolved Hide resolved
pkg/cryptutil/certificates.go Show resolved Hide resolved
pkg/cryptutil/certificates.go Outdated Show resolved Hide resolved
pkg/cryptutil/certificates.go Outdated Show resolved Hide resolved
pkg/cryptutil/tls.go Show resolved Hide resolved
pkg/cryptutil/tls.go Show resolved Hide resolved
config/envoyconfig/listeners.go Show resolved Hide resolved
calebdoxsey and others added 3 commits August 31, 2022 09:19
Co-authored-by: bobby <1544881+desimone@users.noreply.github.com>
Co-authored-by: bobby <1544881+desimone@users.noreply.github.com>
@calebdoxsey calebdoxsey merged commit 33794ff into main Aug 31, 2022
@calebdoxsey calebdoxsey deleted the cdoxsey/3577-unknown-routes branch August 31, 2022 16:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Self-signed certificate surfaced on unknown routes
3 participants