Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
router: Create InternalRedirectPolicy in side RouteAction and extend it with pluggable predicates #10908
router: Create InternalRedirectPolicy in side RouteAction and extend it with pluggable predicates #10908
Changes from all commits
2d1206f
15f1510
81debc8
e3eee18
c89a6b4
0f77ae8
985819a
8199c60
fac46f2
fe9e5c9
179e5a9
1c7ada2
925950c
61e15f7
a806956
5711900
684ba65
ad05c9b
c606a1e
feedb0e
b5a1f54
23521a6
dabc625
d6d1dce
753e046
9fa3ecd
ceb0fa9
70da0b9
68cad35
f7beb49
bc525c4
8cd5974
4e06b6d
8c31bf9
db7a8fe
cac69d6
a24ee60
c05d049
1144d82
9e08926
c111425
5be8361
5c1e199
8138621
8461c5f
56b0188
15a4d3f
948ca63
93cd0e6
c62f9da
4e660fd
416e79e
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's your plan here?
So the default code base today allows serving secure and insecure content over TLS, and insecure content over raw HTTP.
Either you're coding up a behavioral change which changes the default (doable, but why not leave it permissive and add a predicate if you want to not allow?) or you're making it possible to serve TLS content in the clear which I'd really prefer we not do.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok looking at the code this looks intentional, and I'm not sure how we want to document a difference in the new behavior, i.e. calling out to folks as they migrate that if they want to preserve existing behavior, they need to set something new. It's probably worth sorting out what we want to do in general, though you could bypass that conversation if you invert this and have disallow_cross_scheme_redirect to disallow serving http content over an https channel.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I took the route of using disallow_cross_scheme_redirect to be more permissive.
We have an upstream that receives traffic both directly from end user and from the Envoy. The upstream issues redirect scheme based on the incoming scheme. We would like to have the Envoy -> upstream traffic to always be TLS to protect internal only URL, but it's perfectly fine for the end user to talk to the Envoy in plain text otherwise. So cross scheme redirect is really valuable here:
Admittedly we can make the upstream smarter by looking not only to the tls context but also to x-forwarded-proto header, but the upstream then need to know when to look at x-forwarded-proto and when to not. Since it receives internet traffic, anyone can set such header to confuse it.