Proxy support patch

[Pál Dorogi]

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>