> Ah, I see. Thanks for that context, Derek! I totally missed the > discussions. Anyway, I glanced over them a bit, but didn’t seem to > find anything specific about how this’d affect caching. Makes me > wonder if this would result in further more inconsistent situations > (example, caching the followed object vs 403 vs 3xx) than missing out > CC headers of the intermediate responses.
Normally, if it is a 4xx, it is not cached I think, but it is possible to do this "negative caching". Where it gets really interesting to me is that the right thing to do may depend on the architecture in which ATS is deployed (forward proxy? reverse proxy?), and so the correct design choice isn't necessarily something we are equipped to decide as ATS devs. Probably there are a lot of facets to handling redirects efficiently that could be discussed. I think probably that is out-of-scope for this PR though. It is already heavy enough. > Also, I’m unclear on how common/practical, it’d be an use case for an > Origin server to return a redirect location with a private address. > Imagine, if ATS does not follow redirects (or you’ve a proxy that > doesn’t even support following redirects like ATS), how’d that work > from the client’s perspective? It feels like it is something that > needs to be fixed in the Origin server in the first place than add > defense on the proxy following the redirects? You are right about it being a strange use case. In the documentation for this change there is some warning text that explains the danger of the old default behavior. It's not that it is common or practical behavior to benefit a client, but that it is a vulnerability to an abuse case from a malicious origin. Even returning a 5xx versus a 4xx can leak details about which ports are listening on the system running ATS, for example. -- Derek On Sat, Aug 10, 2019 at 09:26:33PM -0700, Sudheer Vinukonda wrote: > > Ah, I see. Thanks for that context, Derek! I totally missed the discussions. > Anyway, I glanced over them a bit, but didn’t seem to find anything specific > about how this’d affect caching. Makes me wonder if this would result in > further more inconsistent situations (example, caching the followed object vs > 403 vs 3xx) than missing out CC headers of the intermediate responses. > > Also, I’m unclear on how common/practical, it’d be an use case for an Origin > server to return a redirect location with a private address. Imagine, if ATS > does not follow redirects (or you’ve a proxy that doesn’t even support > following redirects like ATS), how’d that work from the client’s perspective? > It feels like it is something that needs to be fixed in the Origin server in > the first place than add defense on the proxy following the redirects? > > > Thanks, > > Sudheer > > > On Aug 10, 2019, at 8:27 PM, Derek Dagit <da...@apache.org> wrote: > > > > Hi Sudheer, > > > >> I may have missed if there was some discussion on this in the past, > >> but IMHO, this seems to be a little too specific and custom to be made > >> part of the core. > > > > This has been merged to master awhile back, and there is some discussion > > linked from the PR: https://github.com/apache/trafficserver/pull/4145 > > > > This change was done in core because the problem it tries to address is > > in core itself and not in a plugin. I think perhaps if we want to move > > this kind of feature into a plugin, then that would necessitate removing > > the feature it configures from ATS into a plugin. > > > > > >> Not to mention, the interaction between the existing redirect follow > >> and cache isn’t super clean already and this new overload of that > >> config seems like it has the potential to make it a lot worse. > > > > Yeah, the interaction between cache and following redirects is not only > > a mess in configuration, but also it is a mess in concept and therefore > > also in implementation. Specifically, following redirects is by itself > > can be tricky, and it is not always clear in theory what functionality > > is most desirable: https://github.com/apache/trafficserver/issues/2742 > > > > As another example—I am not sure if there is any existing issue—each > > time we follow a redirect, we cache only the eventual result and do not > > cache the headers for any of the intermediate redirect responses. This > > has an unintended effect of neglecting valid Cache-Control information, > > such that if cache expires earlier for an intermediate redirect, I > > believe ATS continues to serve the eventual result while it is available > > from cache. > > > > All this to say, I concur it is a messy situation. This specific change > > is an incremental approach to harden what we have. I expect most users > > will accept the 9.x default (follow only globally routable addresses), > > and should 8.1.x users want to use the safer behavior without disabling > > the feature, this gives more than enough granularity to do that. At > > least, that was my thought in offering to back-port it. > > > > -- > > Derek > > > >> On Sat, Aug 10, 2019 at 07:50:31PM -0700, Sudheer Vinukonda wrote: > >> > >> Sorry, I don’t have an answer to your question on 8.1.x specifically, but > >> looking at your PR, I’m thinking if this should be done as a plugin and > >> not in the core. I may have missed if there was some discussion on this in > >> the past, but IMHO, this seems to be a little too specific and custom to > >> be made part of the core. Not to mention, the interaction between the > >> existing redirect follow and cache isn’t super clean already and this new > >> overload of that config seems like it has the potential to make it a lot > >> worse. > >> > >> Thanks, > >> > >> Sudheer > >> > >>> On Aug 10, 2019, at 12:32 PM, Derek Dagit <da...@apache.org> wrote: > >>> > >>> I want to port the following PR to the 8.x line, but as it is changing > >>> configs and adding a feature, it should target a minor release. > >>> > >>> https://github.com/apache/trafficserver/pull/4145 > >>> > >>> Earlier I fetched an 8.1.x branch, but now that I have a branch ready > >>> for a pull request it appears that branch is gone. > >>> > >>> This is the proposed PR: > >>> https://github.com/apache/trafficserver/compare/8.1.x_failed...d2r:handle-redirect-to-loopback-8.1.x > >>> > >>> What should I do? > >>> > >>> -- > >>> Derek >
