<br><br><div class="gmail_quote">2009/12/18 David Woodhouse <span dir="ltr"><<a href="mailto:dwmw2@infradead.org" target="_blank">dwmw2@infradead.org</a>></span><br><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">
<div><div></div><div>On Fri, 2009-12-18 at 11:43 +1300, Pál Dorogi wrote:<br>
> Hi,<br>
><br>
> I've created a patch for the openconnect-2.12 to allow connect to the<br>
> target host through a Proxy.<br>
> It's just a simple patch without any Proxy authentication feature.<br>
> More info can be found in the<br>
> <a href="http://ilapstech.blogspot.com/2009/12/add-proxy-support-to-openconnect.html" target="_blank">http://ilapstech.blogspot.com/2009/12/add-proxy-support-to-openconnect.html</a><br>
<br>
</div></div><div><div></div><div>Looks good; thanks. A few comments...<br>
<br>
It doesn't look like you've fixed the NetworkManager authentication<br>
dialog to set the default proxy port, as main.c does. Perhaps it's<br>
better to handle that in the connection code itself<br>
(vpninfo->proxy_port?:"8080") rather than requiring the setup as you<br>
have? But I'll settle for anything that doesn't make the NetworkManager<br>
GUI segfault.<br>
<br></div></div></blockquote><div>I just had a quick look at source and I haven't tried to understand all the logic of all of the codes. I just pick up those things which I needed urgently as this was just a quick solution for my problem.:) To tell the truth, I didn't know that this great stuff is supporting the NetworkManager either.:)<br>
<br></div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;"><div><div>
Don't you want to disable DTLS support when you connect through a proxy?<br>
If direct TCP connections to port 443 don't work, then UDP surely isn't<br>
going to either -- even if the server _would_ accept DTLS connections<br>
from a different IP address to the CSTP connection, which I suspect it<br>
doesn't.<br></div></div></blockquote><div>Yep, you must be right. <br> <br></div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;"><div><div>
<br>
You can't necessarily just use strchr(proxy,':') because that's going to<br>
break parsing of IPv6 addresses. We need to make that work with a string<br>
like '[2001:8b0:10b:1:21d:7dff:fe04:dbe2]:8080'<br></div></div></blockquote><div>Yep, that's possible but I prefer just the full hostname based url format instead, coz<br>the bunch of other features handling could make the source too complex.<br>
But, the protocol://hostname:port based would be acceptable too.<br>Of course I meant this above for proxy and not for the target host.<br></div><div> </div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">
<div><div>
+ /* else the default one (1080) which is already set will be used */<br>
Er, 8080? This isn't SOCKS support.<br></div></div></blockquote><div>Yep, It's 8080 as it's set in the header file.<br> <br></div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">
<div><div>
<br>
Shouldn't process_http_proxy() return zero for success, not 1?<br></div></div></blockquote><div>Probably. I've learnt the C in the old days, where the TRUE was 1 and the FALSE is 0. So, I tried to use the mix of the logic of your codes and my old days things.:)<br>
But the "... < 0" and the "return 0;" would have better choice.:) <br><br></div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">
<div><div><br>
This is going to immediately lead to feature requests / bug reports<br>
about honouring an $http_proxy environment variable, and/or using<br>
something like libproxy to "do the right thing" according to system-wide<br>
configuration, including proxy autoconfig. Using libproxy ought to be<br>
fairly simple, and it's small and simple enough that I think it's<br>
reasonable to declare it an unconditional build dependency. Although<br>
it's simple enough to have a fallback which just uses<br>
getenv("http_proxy") if libproxy isn't available at build time.<br></div></div></blockquote><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">
<div><div>
<br>
It's going to lead to requests for SOCKS support too, but I can probably<br>
do that later, if you want.<br></div></div></blockquote><div>I don't think it's necessary as I think the SOCKS proxy is not commonly used nowadays, but I might be false. <br></div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">
<div><div>
<br>
Oh, and ideally you'd send a patch made from the git repository, which<br>
wouldn't include changes to the version.c file. But just excluding<br>
version.c from the patch works too.<br>
<br>
--<br>
dwmw2<br>
<br></div></div></blockquote><div>OK, I'll clean my patch a bit and add the getenv support too.<br>Cheers,<br><br>Pal <br></div></div><br>