On Fri, 2009-12-18 at 11:43 +1300, P?l Dorogi wrote: > Hi, > > I've created a patch for the openconnect-2.12 to allow connect to the > target host through a Proxy. > It's just a simple patch without any Proxy authentication feature. > More info can be found in the > http://ilapstech.blogspot.com/2009/12/add-proxy-support-to-openconnect.html Looks good; thanks. A few comments... It doesn't look like you've fixed the NetworkManager authentication dialog to set the default proxy port, as main.c does. Perhaps it's better to handle that in the connection code itself (vpninfo->proxy_port?:"8080") rather than requiring the setup as you have? But I'll settle for anything that doesn't make the NetworkManager GUI segfault. Don't you want to disable DTLS support when you connect through a proxy? If direct TCP connections to port 443 don't work, then UDP surely isn't going to either -- even if the server _would_ accept DTLS connections from a different IP address to the CSTP connection, which I suspect it doesn't. You can't necessarily just use strchr(proxy,':') because that's going to break parsing of IPv6 addresses. We need to make that work with a string like '[2001:8b0:10b:1:21d:7dff:fe04:dbe2]:8080' + /* else the default one (1080) which is already set will be used */ Er, 8080? This isn't SOCKS support. Shouldn't process_http_proxy() return zero for success, not 1? This is going to immediately lead to feature requests / bug reports about honouring an $http_proxy environment variable, and/or using something like libproxy to "do the right thing" according to system-wide configuration, including proxy autoconfig. Using libproxy ought to be fairly simple, and it's small and simple enough that I think it's reasonable to declare it an unconditional build dependency. Although it's simple enough to have a fallback which just uses getenv("http_proxy") if libproxy isn't available at build time. It's going to lead to requests for SOCKS support too, but I can probably do that later, if you want. Oh, and ideally you'd send a patch made from the git repository, which wouldn't include changes to the version.c file. But just excluding version.c from the patch works too. -- dwmw2
