Bug 23640

Anyone planning to implement this?

Ah, this is for gabble 0.9?

For SOCKS5/4a/4 it's all in GIO now (>= 2.25.15). HTTP Connect proxy is being proposed in wocky (see bug 23646). To actually get this to work well, we need the notion of fallback-servers as Gabble Connection parameter. This way if SRV fails, we fallback to element of the list (an 'as' or the form host[:port[:old_ssl?]]. When old_ssl is enabled (port 443 in the case of Google) the HTTPS proxy setting will taken (if set) enable HTTPS proxy.

The implementation:
http://git.collabora.co.uk/?p=user/nicolas/telepathy-gabble.git;a=commitdiff;h=b820f676740b567c066010ba002b01074c8bba1f;hp=b3a7fb3f8f04fc1aa89048241e0fb04c85949bb3

I'm sad that we're parsing strings from a list, when we have a perfectly good structured type system, but I can see why you did this (we don't have a sane serialization for anything else in a keyfile) and I can't think of anything better we could do.

> +  guint fallback_server_idx;

..._index - please avoid uncommon abbreviations.

It would be nice to have a concrete example of what we would use for Google Talk, either in the code or in the commit message. I infer that it would be something like this:

    ["talk.l.google.com",
    "talk.l.google.com,443,1"]

The commit message says the syntax has colons and the implementation says it has commas: please fix one or the other.

I assume that the reason for using commas was that colons conflict with IPv6 literals' use of colons, but using commas is weird and unconventional too. Possibilities:

* use commas anyway
* use colons, support URI-style IPv6 literals like [::1]:5222 (hard, pointless?)
* use colons, don't support IPv6 literals, assume that anyone with working IPv6 will also have working DNS

Given its purpose, it's not at all clear to me that we'd ever want IP literals in the fallback server list at all, let alone IPv6 literals.

> +        old_ssl = strcmp (split[2],"1") == 0;

The part after " = " should have parentheses for clarity, or (better) be implemented in terms of tp_strdiff().

I don't much like this syntax for old-SSL anyway, though; if nothing else, ",oldssl" is more mnemonic than ",1".

Are we ever likely to want to fall back to old-SSL on a port that is neither 5223 nor 443, or normal XMPP on port 5223 or 443? If not, we could perhaps decide whether to use old-SSL based on the port number?

(That question basically means: are we ever likely to use this for a service that isn't Google Talk, and if so, are we ever likely to use it for a service that is very strangely set up?)

> +next_fallback_server (GabbleConnection *self, const GError *error)

Style: new line for the second argument please

> + * FALSE is they all have been tried.

Typo: "FALSE if they"

First thanks for this review. I've done the fixes in separate patches that I'll sqash later, this way it's easier for you to recheck that I have not done any mistakes.

(In reply to comment #4)
> I'm sad that we're parsing strings from a list, when we have a perfectly good
> structured type system, but I can see why you did this (we don't have a sane
> serialization for anything else in a keyfile) and I can't think of anything
> better we could do.
> 
> > +  guint fallback_server_idx;
> 
> ..._index - please avoid uncommon abbreviations.

http://git.collabora.co.uk/?p=user/nicolas/telepathy-gabble.git;a=commitdiff;h=9d75699f633a1388437717a5096ab6c697969b90

> 
> It would be nice to have a concrete example of what we would use for Google
> Talk, either in the code or in the commit message. I infer that it would be
> something like this:
> 
>     ["talk.l.google.com",
>     "talk.l.google.com,443,1"]
> 
> The commit message says the syntax has colons and the implementation says it
> has commas: please fix one or the other.

Improved commit message: http://git.collabora.co.uk/?p=user/nicolas/telepathy-gabble.git;a=commit;h=d87799deb33c10940b7c99c6104d07ccd57280ed

> 
> I assume that the reason for using commas was that colons conflict with IPv6
> literals' use of colons, but using commas is weird and unconventional too.
> Possibilities:
> 
> * use commas anyway
> * use colons, support URI-style IPv6 literals like [::1]:5222 (hard,
> pointless?)
> * use colons, don't support IPv6 literals, assume that anyone with working IPv6
> will also have working DNS

I had to switch to comma because : clash with IPV6 has you mentioned and ; clash with Mission Control serialization of 'as'. The main goal was to ensure that a simple strplit would make the job.

> 
> Given its purpose, it's not at all clear to me that we'd ever want IP literals
> in the fallback server list at all, let alone IPv6 literals.
> 
> > +        old_ssl = strcmp (split[2],"1") == 0;
> 
> The part after " = " should have parentheses for clarity, or (better) be
> implemented in terms of tp_strdiff().
> 
> I don't much like this syntax for old-SSL anyway, though; if nothing else,
> ",oldssl" is more mnemonic than ",1".

http://git.collabora.co.uk/?p=user/nicolas/telepathy-gabble.git;a=commitdiff;h=497aabcccda38d397a17a1e3bcb330d599fcdc91

> 
> Are we ever likely to want to fall back to old-SSL on a port that is neither
> 5223 nor 443, or normal XMPP on port 5223 or 443? If not, we could perhaps
> decide whether to use old-SSL based on the port number?
> 
> (That question basically means: are we ever likely to use this for a service
> that isn't Google Talk, and if so, are we ever likely to use it for a service
> that is very strangely set up?)

In my opinion, the cost of being flexible is very low, so why not ?  Also, port number is very weak heuristic for old-SSL. Like any server, you are free to run it on the port you want. XMPP might be used as intranet communication, in which case we may want to use different port for technical reason.

> 
> > +next_fallback_server (GabbleConnection *self, const GError *error)
> 
> Style: new line for the second argument please

http://git.collabora.co.uk/?p=user/nicolas/telepathy-gabble.git;a=commitdiff;h=f9b7e137c0c5b914fdcbc4a8bee455172e22e2e0

> 
> > + * FALSE is they all have been tried.
> 
> Typo: "FALSE if they"

http://git.collabora.co.uk/?p=user/nicolas/telepathy-gabble.git;a=commitdiff;h=84264b4c918ffff782ad8d864281d181d5969a7f

(In reply to comment #5)
> First thanks for this review. I've done the fixes in separate patches that I'll
> sqash later, this way it's easier for you to recheck that I have not done any
> mistakes.

Thanks, that's ideal. To be honest, it's not really necessary to squash them later, but do that if you like.

> Improved commit message:
> http://git.collabora.co.uk/?p=user/nicolas/telepathy-gabble.git;a=commit;h=d87799deb33c10940b7c99c6104d07ccd57280ed

Thanks, that's much better.

> > Possibilities:
> > 
> > * use commas anyway
> > * use colons, support URI-style IPv6 literals like [::1]:5222 (hard,
> > pointless?)
> > * use colons, don't support IPv6 literals, assume that anyone with working IPv6
> > will also have working DNS
> 
> I had to switch to comma because : clash with IPV6 has you mentioned and ;
> clash with Mission Control serialization of 'as'. The main goal was to ensure
> that a simple strplit would make the job.

I can see your reasoning, but the fact that we're inventing our own unconventional serialization does concern me, particularly if we plan to generalize fallback-servers to other protocols and put it in telepathy-spec.

Given that the purpose of the fallback list is to hard-code a list of fallbacks into application data (a .profile or similar) for particular providers (GTalk and friends) for the benefit of networks where SRV doesn't work, I don't see why we'd ever want IP literals, either v4 or v6. It'd be considered rather rude to hard-code IP addresses into a Google Talk profile shipped with either Empathy or the N900, for instance. Instead, in practice we'll always be embedding DNS names that can be looked up with A or AAAA queries.

The only case I can think of where it might be acceptable (or necessary) to use IP literals is to connect to a server on an intranet, or another server controlled by the client's organization; in either case, we'd be using the 'server' parameter (which is the only one that should appear in a UI, IMO), rather than fallback-servers.

(For what it's worth, MC can deal with semicolons in 'as' members - I'm pretty sure it can escape and unescape them correctly according to the .desktop spec, and if it doesn't then that's a bug. Semicolons wouldn't really be any better than commas in this case, though.)

> > Are we ever likely to want to fall back to old-SSL on a port that is neither
> > 5223 nor 443, or normal XMPP on port 5223 or 443? If not, we could perhaps
> > decide whether to use old-SSL based on the port number?
> 
> In my opinion, the cost of being flexible is very low, so why not ?  Also, port
> number is very weak heuristic for old-SSL. Like any server, you are free to run
> it on the port you want.

The cost of being flexible, in this case, is that we go beyond a common convention into our own ad-hoc mini-language, which isn't ideal, but if it's necessary I can live with it.

> XMPP might be used as intranet communication, in which
> case we may want to use different port for technical reason.

Empathy/Maemo/whatever isn't going to ship a .profile for a particular intranet, and if the intranet's owner provides a .profile they can just set a default 'server' if they need to, so fallback-servers doesn't really seem relevant in this case?

http://git.collabora.co.uk/?p=user/nicolas/telepathy-gabble.git;a=commitdiff;h=497aabcccda38d397a17a1e3bcb330d599fcdc91
> +        old_ssl = (tp_strdiff (split[2],"oldssl") == FALSE);

We don't use explicit comparison for things that already return boolean, and we do put whitespace after commas:

    old_ssl = !tp_strdiff (split[2], "oldssl");

(In reply to comment #7)
> http://git.collabora.co.uk/?p=user/nicolas/telepathy-gabble.git;a=commitdiff;h=497aabcccda38d397a17a1e3bcb330d599fcdc91
> > +        old_ssl = (tp_strdiff (split[2],"oldssl") == FALSE);
> 
> We don't use explicit comparison for things that already return boolean, and we
> do put whitespace after commas:
> 
>     old_ssl = !tp_strdiff (split[2], "oldssl");

Base on IRC discussion, I move to a syntax that let me use g_network_address_parse(), host[:port][,oldssl] and fixed that at the same time:

http://git.collabora.co.uk/?p=user/nicolas/telepathy-gabble.git;a=commitdiff;h=a08775fc69534b4efc31dccaba58377476a7346a;hp=84264b4c918ffff782ad8d864281d181d5969a7f

(In reply to comment #8) 
> > we do put whitespace after commas:

Oops, missing that one, this use this commit instead:

http://git.collabora.co.uk/?p=user/nicolas/telepathy-gabble.git;a=commitdiff;h=bb011941d087c4573aeb633f8c47822555f4dd37;hp=84264b4c918ffff782ad8d864281d181d5969a7f

This looks good, thanks. r+

Here's a spec micro-proposal:

fallback-servers: 'as'
    A list of fallback servers to use if the server given by
    'account', 'server' or an auto-discovery mechanism cannot
    be reached.

    Each server in the list is a hostname as if for 'server', or
    a hostname:port pair, or one of those formats followed by a
    comma and implementation-specific extra parameters.

Code pushed, will be in 0.9.18.

Just to be clear, only socks works, right? The most commonly used proxy, http, still doesn't work.

I guess strictly speaking "proxy support" is implemented.

(In reply to comment #12)
> Just to be clear, only socks works, right? The most commonly used proxy, http,
> still doesn't work.
Most commonly used ? I don't think many XMPP client implementation have implemented BOSH, and HTTP Connect over non 443 port rarely works. So let's remember that HTTP proxy is mostly for Google Talk which provide direct TLS on port 443 (which is non-standard hack to simulate what would happen when doing HTTPS connection).

Anyway, let's not confuse our users more. SOCKSv5/4/4a is upstream GLib 2.26 (requires glib-networking 2.26 to be installed too) and HTTP Connect support has been merged into Wocky (the XMPP library used by Gabble) this week.

This means, the next version of Gabble 0.10.X will support SOCKS and HTTP Connect proxy. Special code has been added in Google Talk account profiles to enable fallback to port 443 and use of HTTPS proxy settings, which should improve connectivity in HTTP proxied network.

(In reply to comment #13)
> (In reply to comment #12)
> > Just to be clear, only socks works, right? The most commonly used proxy, http,
> > still doesn't work.
> Most commonly used ? I don't think many XMPP client implementation have
> implemented BOSH, and HTTP Connect over non 443 port rarely works. So let's
> remember that HTTP proxy is mostly for Google Talk which provide direct TLS on
> port 443 (which is non-standard hack to simulate what would happen when doing
> HTTPS connection).

Huh? I thought HTTP connect was pretty straight-forward. So straight-forward that you can even use socat for the transmission and the application would never know it's being proxied.

> Anyway, let's not confuse our users more. SOCKSv5/4/4a is upstream GLib 2.26
> (requires glib-networking 2.26 to be installed too) and HTTP Connect support
> has been merged into Wocky (the XMPP library used by Gabble) this week.
> 
> This means, the next version of Gabble 0.10.X will support SOCKS and HTTP
> Connect proxy. Special code has been added in Google Talk account profiles to
> enable fallback to port 443 and use of HTTPS proxy settings, which should
> improve connectivity in HTTP proxied network.

All right, I didn't know about the http support in wocky. I thought the sane way to do this would be to implement HTTP connect support directly on GLib, so other apps could re-use that.

Anyway, I'll give it a try. Thanks!

(In reply to comment #14)
> All right, I didn't know about the http support in wocky. I thought the sane
> way to do this would be to implement HTTP connect support directly on GLib, so
> other apps could re-use that.

HTTP Connect proxy has not been accepted by GLib maintainers. Argument was that it most likely will reduce connectivity as most sysadmin configure their proxy to only accept proxied port to be 443. The code in Wocky is the code that was supposed to be in GLib (very few changes aside namespace). This might be reconsidered later.

(In reply to comment #15)
> (In reply to comment #14)
> > All right, I didn't know about the http support in wocky. I thought the sane
> > way to do this would be to implement HTTP connect support directly on GLib, so
> > other apps could re-use that.
> 
> HTTP Connect proxy has not been accepted by GLib maintainers. Argument was that
> it most likely will reduce connectivity as most sysadmin configure their proxy
> to only accept proxied port to be 443.

Pfft. That's nonsense. GNOME network properties has an option to use the same proxy for all protocols doesn't it? Thus GLib should work for that option.

> The code in Wocky is the code that was
> supposed to be in GLib (very few changes aside namespace). This might be
> reconsidered later.

Excellent, I think that would already help a lot.