OK I think we can converge on the PR now: https://github.com/apache/trafficserver/pull/4145#issuecomment-415647049
I expect more iterations. On Thu, Aug 23, 2018 at 8:52 PM, Derek Dagit <der...@oath.com> wrote: > Thanks, working on it. > > On Thu, Aug 23, 2018 at 6:19 PM, Bryan Call <bc...@apache.org> wrote: > >> Inline >> >> -Bryan >> >> > On Aug 22, 2018, at 11:37 AM, Derek Dagit <der...@oath.com.INVALID> >> wrote: >> > >> > Bryan, some clarification questions: >> > >> > >> >> 2. 403 >> > >> > What should we do on the N+1st redirect? Do we try to resolve the target >> > host and 403 that too, or just return it to the client regardless of >> how it >> > resolves? >> > >> > This is why I went with "Return" by default rather than "Reject". >> > >> >> Sounds good, that is what we are doing now with n+1. >> >> > >> > >> >> 3. I like Lief’s proposal. >> > >> > OK I can renumber the config values if necessary. >> > >> > >> > >> >> You might want to add another option to add non-routable IPs to the >> list >> > too. >> > >> > We do prevent connections to the ANY address 0.0.0.0 globally in ATS >> > already (redirect or not). I can look at adding additional checks for >> > non-routable addresses. >> > >> > >> >> I would create a configuration option called >> proxy.config.http.redirect_enabled >> > and start with 0 as off and set proxy.config.http.number_of_redirections >> to >> > 1 as the default. >> > >> > We have recently removed this config, seeing as it is redundant with >> > number_of_redirections. Are you proposing we bring this config back >> with a >> > new meaning and use it to configure the Follow/Reject/Return behavior? >> >> Yes, if it is being enabled for different modes I would think it would be >> best to have it under and enabled option, This is how some other >> configuration options are done (e.g. proxy.config.log.logging_enabled). >> >> > >> > Setting it to 0 according to Leif's proposal would default to the >> current >> > behavior, which we are saying is not recommended. Is this what you >> suggest >> > or were you thinking Reject or Return should be the default? >> > >> > >> >> When checking for localhost you should resolve the ip and check to see >> if >> > it matches any of the IPs on the server. >> > >> > OK will look at this too. I am not sure there is a good solution to >> handle >> > other servers or VIPs where the VIP is not mapped to a loop-back >> interface. >> > (If the VIP is mapped to the loopback, then this is covered by checks >> for >> > loopback. If it is not mapped to loopback, then I am not sure how we can >> > know to treat it specially aside from an additional config—and that >> starts >> > to sound like maybe a plugin should be handling such things.) >> > >> > >> > >> > Thanks for the input. I'm fine with changing it. >> > >> > The current Pull Request is up, but keeping the discussion here until we >> > settle on what we want to do. >> > https://github.com/apache/trafficserver/pull/4145 >> > >> > >> > >> > On Wed, Aug 22, 2018 at 1:04 PM, Bryan Call <bc...@apache.org> wrote: >> > >> >> 1. Yes >> >> 2. 403 >> >> 3. I like Lief’s proposal. You might want to add another option to add >> >> non-routable IPs to the list too. I would create a configuration >> option >> >> called proxy.config.http.redirect_enabled and start with 0 as off and >> set >> >> proxy.config.http.number_of_redirections to 1 as the default. >> >> >> >> When checking for localhost you should resolve the ip and check to see >> if >> >> it matches any of the IPs on the server. That could be a long list if >> >> there servers has many IP aliases. This still doesn’t block IPs used >> by >> >> other servers in the same network. >> >> >> >> -Bryan >> >> >> >> >> >> >> >>> On Aug 6, 2018, at 5:25 PM, Derek Dagit <der...@oath.com.INVALID> >> wrote: >> >>> >> >>> 1. Yep >> >>> >> >>> 2. I see multiple possible answers as well... >> >>> >> >>> 3. ... and the 0,1,2-style config having either default sounds OK. >> >>> >> >>> Regarding 1)2), Could we resolve and then check if the address is a >> >>> loopback address? >> >>> 3) Hmm, not sure about the use of *cast addresses either. >> >>> 4) The VIP deployment I'm used to will actually set up the host such >> >>> that the VIP name resolves to the loopback address, so the above >> works. >> >>> That might not be good enough for all deployments though. >> >>> >> >>> On Mon, Aug 6, 2018 at 6:16 PM, Leif Hedstrom <zw...@apache.org> >> wrote: >> >>> >> >>>> >> >>>> >> >>>>> On Aug 6, 2018, at 4:50 PM, Alan Carroll <solidwallofc...@oath.com. >> >> INVALID> >> >>>> wrote: >> >>>>> >> >>>>> 1. Yes. >> >>>> >> >>>> Agreed. >> >>>> >> >>>>> >> >>>>> 2. I think a 403 >> >>>> >> >>>> I can go either way. The HTTP way would be to just return the >> Location >> >> as >> >>>> is (i.e. retain the redirect), following redirects is a little >> >> unorthodox. >> >>>> Maybe since we are adding new configuration(s), maybe make it such >> that >> >> the >> >>>> behavior can be configurable either way? >> >>>> >> >>>> Depending on if we add another configuration, or add on to the >> existing >> >>>> one, I’m thinking something in the line of >> >>>> >> >>>> 0 - Always allow follow redirect >> >>>> 1 - Allow follow redirect, but return the normal redirect if >> it’s >> >>>> for localhost >> >>>> 2 - Allow follow redirect, but give a 403 if it's to localhost >> >>>> >> >>>> >> >>>> My vote would be for 1) to be the default, but I can live with 2) as >> >> well. >> >>>> >> >>>> Question: The notion of “localhost” is a little vague here… I think >> >>>> there’s a number of other ways to reach “localhost”, rather than just >> >>>> localhost/127.0.0.1: >> >>>> >> >>>> 1) FQDNs >> >>>> 2) One of the possibly many IPs that are local to the box, >> >>>> including IPv6 link-locals >> >>>> 3) Possibly some broadcast or multicast addresses?? >> >>>> 4) The upstream VIP IP that might point back to the box(es) >> >>>> >> >>>> I think there are many ways that someone could make a follow redirect >> >> loop >> >>>> back to itself, or a peering proxy. >> >>>> >> >>>> Cheers, >> >>>> >> >>>> — Leif >> >>>> >> >>>> >> >>> >> >>> >> >>> -- >> >>> Derek >> >> >> >> >> > >> > >> > -- >> > Derek >> >> > > > -- > Derek > -- Derek
