Re: [PATCH] no_proxy domain matching

[Tim Rühsen]

Thank you, Thomas, patches applied and pushed.

Regards, Tim

On 10.07.20 15:34, Tomas Hozza wrote:
>
>
> On 27. 11. 2019 10:50, Tim Rühsen wrote:
>> On 11/26/19 4:00 PM, Tomas Hozza wrote:
>>>
>>>
>>> On 20. 11. 2019 18:47, Tim Rühsen wrote:
>>>> On 20.11.19 12:41, Tomas Hozza wrote:
>>>>> On 7. 11. 2019 21:30, Tim Rühsen wrote:
>>>>>> On 07.11.19 15:21, Tomas Hozza wrote:
>>>>>>> Hi.
>>>>>>>
>>>>>>> In RHEL-8, we ship a wget version that suffers from bug fixed by [1]. 
>>>>>>> The fix resolved issue with matching subdomains when no_proxy domain 
>>>>>>> definition was prefixed with dot, e.q. "no_prefix=.mit.edu". As part of 
>>>>>>> backporting the fix to RHEL, I wanted to create an upstream test for 
>>>>>>> no_prefix functionality. However I found that there is still one corner 
>>>>>>> case, which is not handled by the current upstream code and honestly 
>>>>>>> I'm not sure what is the intended domain matching behavior in that 
>>>>>>> case. Man page is also not very specific in this regard.
>>>>>>>
>>>>>>> The corner case is as follows:
>>>>>>> - no_proxy=.mit.edu
>>>>>>> - download URL is e.g. "http://mit.edu/file1";
>>>>>>>
>>>>>>> In this case the proxy settings are used, because domains don't match 
>>>>>>> due to the leftmost dot in no_proxy domain definition. This is either 
>>>>>>> intended or corner case that was not considered. One could argue, that 
>>>>>>> if the no_proxy is set to ".mit.edu", then leftmost dot means that the 
>>>>>>> proxy settings should not apply only to subdomains of "mit.edu", but 
>>>>>>> proxy settings should still apply to "mit.edu" domain itself. From my 
>>>>>>> point of view, after reading wget man page, I don't think that the 
>>>>>>> leftmost dost in no_proxy definition has any special meaning.
>>>>>>
>>>>>> Hello Tomas,
>>>>>>
>>>>>> hard to decide how to handle this. I personally would like to see a
>>>>>> match with curl's behavior (see 
>>>>>> https://github.com/curl/curl/issues/1208).
>>>>>>
>>>>>> Given the docs from GNU emacs, you are right. "no_proxy=.mit.edu" means
>>>>>> "mit.edu and subdomains" are excluded from proxy settings.
>>>>>> (see 
>>>>>> https://www.gnu.org/software/emacs/manual/html_node/url/Proxies.html)
>>>>>>
>>>>>> The caveat with emacs' behavior is that you cannot exclude just all
>>>>>> subdomains of mit.edu without mit.edu itself. Effectively, that creates
>>>>>> a corner case that can't be handled at all. (but if curl also does it
>>>>>> that way, let's go for it).
>>>>>>
>>>>>> Maybe you can find out about the current no_proxy behavior of typical
>>>>>> and wide-spread tools (regarding leftmost dot) !? Once we have that
>>>>>> information, we can make a confident decision.
>>>>>>
>>>>>> Regards, Tim
>>>>>
>>>>> Hi Tim.
>>>>>
>>>>> It took me some time to go through the current situation and to be 
>>>>> honest, it is kind of a mess. While each tool handles the no_proxy env a 
>>>>> little bit differently, there are some similarities. Nevertheless I was 
>>>>> not able to find any standard.
>>>>>
>>>>> curl's behavior:
>>>>> - "no_proxy=.mit.edu"
>>>>>   - will match the domain and subdomains e.g. "www.mit.edu" or 
>>>>> "www.subdomain.mit.edu"
>>>>>   - will match the host "mit.edu"
>>>>> - "no_proxy=mit.edu"
>>>>>   - will match the domain and subdomains e.g. "www.mit.edu" or 
>>>>> "www.subdomain.mit.edu"
>>>>>   - will match the host "mit.edu"
>>>>> - downside: can not match only the host; can not match only the domain 
>>>>> and subdomains
>>>>>
>>>>> current wget's behavior:
>>>>> - "no_proxy=.mit.edu"
>>>>>   - will match the domain and subdomains e.g. "www.mit.edu" or 
>>>>> "www.subdomain.mit.edu"
>>>>>   - will NOT match the host "mit.edu"
>>>>> - "no_proxy=mit.edu"
>>>>>   - will match the domain and subdomains e.g. "www.mit.edu" or 
>>>>> "www.subdomain.mit.edu"
>>>>>   - will match the host "mit.edu"
>>>>> - downside: can not match only the host
>>>>>
>>>>> wget's behavior with proposed patch:
>>>>> - "no_proxy=.mit.edu"
>>>>>   - will match the domain and subdomains e.g. "www.mit.edu" or 
>>>>> "www.subdomain.mit.edu"
>>>>>   - will match the host "mit.edu"
>>>>> - "no_proxy=mit.edu"
>>>>>   - will match the domain and subdomains e.g. "www.mit.edu" or 
>>>>> "www.subdomain.mit.edu"
>>>>>   - will match the host "mit.edu"
>>>>> - downside: can not match only the host; can not match only the domain 
>>>>> and subdomains
>>>>> - it would be consistent with curl's behavior
>>>>>
>>>>> emacs's behavior:
>>>>> - "no_proxy=.mit.edu"
>>>>>   - will match the domain and subdomains e.g. "www.mit.edu" or 
>>>>> "www.subdomain.mit.edu"
>>>>>   - will match the host "mit.edu"
>>>>> - "no_proxy=mit.edu"
>>>>>   - will NOT match the domain and subdomains e.g. "www.mit.edu" or 
>>>>> "www.subdomain.mit.edu"
>>>>>   - will match the host "mit.edu"
>>>>> - downside: can not match only subdomains
>>>>>
>>>>> python httplib2's behavior:
>>>>> - "no_proxy=.mit.edu"
>>>>>   - will match the domain and subdomains e.g. "www.mit.edu" or 
>>>>> "www.subdomain.mit.edu"
>>>>>   - will match the host "mit.edu"
>>>>> - "no_proxy=mit.edu"
>>>>>   - will NOT match the domain and subdomains e.g. "www.mit.edu" or 
>>>>> "www.subdomain.mit.edu"
>>>>>   - will match the host "mit.edu"
>>>>> - downside: can not match only subdomains
>>>>>
>>>>> To sum it up. Each approach has some downsides. Given the change that I 
>>>>> provided, wget's behavior would be consistent with curl's behavior. 
>>>>> However it will have more downsides that it currently has, specifically 
>>>>> it will loose the ability to not to match the host, but only domain and 
>>>>> subdomains. Emacs's behavior is similar to Python's httplib2 behavior 
>>>>> regarding the leftmost dot.
>>>>>
>>>>> Honestly I have a soft preference for keeping the current wget's 
>>>>> behavior. But I admit that making the behavior consistent with curl's 
>>>>> behavior makes sense. Please let me know how you would like to proceed.
>>>>>
>>>>> To make the behavior consistent with curl, the previously attached 
>>>>> changes should be OK. If you find those new conditions too complicated, I 
>>>>> can try to rethink it, but I already tried to make it as little 
>>>>> complicated as possible and at the same time trying to not completely 
>>>>> rewrite the function.
>>>>>
>>>>> If you'll decide to keep the current behavior, I'll modify the test that 
>>>>> I added to cope with the behavior.
>>>>
>>>> Great work, Tomas !
>>>>
>>>> Wow, didn't think it's so messed up :-(
>>>>
>>>> We should definitely document your results, e.g. in the wget manual.
>>>>
>>>> If we keep the current behavior, we could adjust it with a new option or
>>>> a new env variable 'WGET_NO_PROXY_MODE'. Which could take well-defined
>>>> values like 'curl', 'emacs', 'wget' (the default), and maybe a new one
>>>> ('strict') with none of the detected downsides.
>>>>
>>>> Looks a bit over-engineered, but it means that wget can easily adopt to
>>>> existing environments. And the code seems pretty straight forward.
>>>>
>>>> Let's see if some more opinions come in.
>>>>
>>>> Regards, Tim
>>>
>>> Yes, 'WGET_NO_PROXY_MODE' is probably the safest option with regard to 
>>> backward compatibility. And if needed, the default could later change. One 
>>> downside of allowing values like 'curl' or 'emacs' is that these would 
>>> probably require also handling of asterisk "*" in hostnames, as those tools 
>>> do.
>>>
>>> Nevertheless for now, I at least modified the new test to cover current 
>>> wget behavior. There were also minor changes to the test framework in order 
>>> to make the test possible. Patches are attached. Please let me know if they 
>>> need any changes.
>
> Hi Tim.
>
> I finally got to finishing the requested changes. Modified patches are 
> attached.
>
>> Please add your test to testenv/Makefile.am (best directly before adding
>> $(METALINK_TESTS)). The test currently doesn't work for me, since host
>> `working1.localhost` can't be resolved.
>
> I added the test to testenv/Makefile.am.
>
>> Maybe you can add `testenv/certs/wgethosts`, similar as
>> `tests/certs/wgethosts`, but with working1.localhost and
>> working2.localhost included. You have to set env variable HOSTALIASES to
>> that file name (glibc feature).
>
> I was surprised by this, since I was not able to reproduce this issue at 
> first. However later I found out that since Fedora uses by default systemd's 
> nss plugin "myhostname", which translates any localhost subdomain to 
> localhost address, it magically works.
>
> I tried your suggestion, however based on the documentation of HOSTALIASES 
> and my experiments, the HOSTALIASES works only for hostnames without dot. So 
> in this case, when I need to check even subdomains of working1.localhost 
> (like www.working1.localhost), it does not work. It does not even work for 
> working1.localhost. Honestly I didn't find any easy way how to make this work 
> on a system which does not support translating any localhost subdomains to 
> localhost address.
>
> After some thinking, I came up with at least a workaround, to skip this test 
> if the system can not translate necessary localhost subdomains. This is 
> checked at the beginning of the test and if the resolution fails, the test is 
> skipped. However on systems that support it (like Fedora, RHEL), the test is 
> run and passes.
>
> If you have some additional ideas about how to make this work everywhere, 
> I'll be more than happy to change the test. Unfortunately I'm out of ideas 
> and skipping the test in some cases makes it at least possible to include in 
> the upstream repository (so that it does not make unnecessarily the test 
> suite to fail).
>
>> In patch 0001 there is a typo 'retuns'.
>
> Fixed.
>
>> Please don't make your lines in the commit messages longer than 79 chars.
>
> Fixed.
>
>> Not sure if and when I implement WGET_NO_PROXY_MODE. We try to make no
>> more changes to wget 1.x except bug fixes. All new stuff should only go
>> into wget2.
>>
>> Regards, Tim
>>
> Regards,
> Tomas