2009/12/18 David Woodhouse <dwmw2 at infradead.org> > 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. > > 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.:) > 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. > Yep, you must be right. > > 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' > Yep, that's possible but I prefer just the full hostname based url format instead, coz the bunch of other features handling could make the source too complex. But, the protocol://hostname:port based would be acceptable too. Of course I meant this above for proxy and not for the target host. > + /* else the default one (1080) which is already set will be used */ > Er, 8080? This isn't SOCKS support. > Yep, It's 8080 as it's set in the header file. > > Shouldn't process_http_proxy() return zero for success, not 1? > 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.:) But the "... < 0" and the "return 0;" would have better choice.:) > 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. > I don't think it's necessary as I think the SOCKS proxy is not commonly used nowadays, but I might be false. > > 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 > > OK, I'll clean my patch a bit and add the getenv support too. Cheers, Pal -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.infradead.org/pipermail/openconnect-devel/attachments/20091219/db6f5929/attachment.htm>
