Bug 23646

Last week I've pushed proxy support into GLib, it will be available on September 10th with the venue of GLib 2.26. This will transparently enable SOCKSv5, 4a and 4 into Gabble base on system configuration.

Since Google server supports connecting on port 443 with XMPP old way of doing SSL, we can go 1 step further and push the use of HTTP Connect. The HTTP Connect GLib plugin has not been accepted into GLib but can easily be included in Gabble. As the overall goal is connectivity, here's the proposed approach to connecting to Google XMPP server assuming a default server/port configuration:

Attempt | DNS             |PORT |OLD SSL? | Proxy URI
-------------------------------------------------------
  1.      SRV               -     no        xmpp-client://...
  2.      talk.google.com   80    no        xmpp-client://talk.google.com:80
  3.      talk.google.com   443   yes       https://talk.google.com:80

In the third attempt, if an HTTPS proxy server is configured, GLib will handshake with the HTTP proxy server (using CONNECT method). This should work for people that access the Internet (included HTTPS pages) through proxy server. The third attempt without such a proxy configured would still be relevant for network using caching HTTP proxy and where port 5222 is being blocked.

Another change that could improve are connectivity is to enable https:// proxy URI whenever old SSL is being enabled. The point is that there is no detectable difference between an HTTPS connection and an old ssl connection aside the port. So in the case ports are not filtered, we would have one more chance of success on proxied network. A retry disabling proxy would be required here though, otherwise direct connection would never be tried since proxy configuration are authoritative.

After some test, I've also discovered that the Win32 Google Talk software uses talkx.l.google.com instead of talk.google.com. The main difference is that talkx DNS will return a list just like the SRV request do. Using it would most likely improve our connectivity in the case one of the the Google server is down.

Created attachment 38179 [details] [review]
Add HTTP Connect proxy support

This patch enable HTTP Connect proxy support when legacy SSL is being used. GIO Proxy support is upstream but not yet in a stable release, thus the patch will only activate if version of GIO is sufficient.

The rest of my previous note should be done in Gabble.

Any reviewer? This blocks bug 23640.

Review of attachment 38179 [details] [review]:

This looks functionally pretty good to me. There are a couple of outright bugs — the Makefile.am omission, and the crash I suspect — plus a bunch of nitpicks.

::: wocky/Makefile.am
@@ +140,3 @@
+  handwritten_sources += $(PROXY_SRC)
+endif
+

You should have an 'else' branch which adds PROXY_SRC to EXTRA_DIST.

::: wocky/wocky-connector.c
@@ +807,3 @@
+      host, port, NULL, tcp_host_connected, connector);
+#endif
+}

Huh, connecting to xmpp-client://jabber.org does the right thing?

::: wocky/wocky-http-proxy.c
@@ +54,3 @@
+  /* must chain up */
+  G_OBJECT_CLASS (wocky_http_proxy_parent_class)->finalize (object);
+}

Can't you just not override ->finalize?

@@ +82,3 @@
+  const gchar *ptr = buffer + 7;
+
+  if (strncmp (buffer, "HTTP/1.", 7)

I'd prefer 'strncmp (...) != 0' for clarity

@@ +91,3 @@
+
+  ptr++;
+  while (*ptr == ' ') ptr++;

Won't this crash if the buffer is shorter than we expect?

@@ +109,3 @@
+
+      ptr = strchr (msg_start, '\r');
+      if (!ptr)

Our coding style asks for 'ptr == NULL'

@@ +120,3 @@
+        g_set_error (error, G_IO_ERROR, G_IO_ERROR_PROXY_FAILED,
+            "HTTP proxy connection failed: %s",
+            msg);

Might be nice to include the error code in the message, too.

@@ +138,3 @@
+  GInputStream *in;
+  GOutputStream *out;
+  GDataInputStream *datain;

data_in would be a clearer name. I thought this was a typo for 'detain' or something at first reading.

@@ +149,3 @@
+      "destination-port", &port,
+      "username", &username,
+      "password", &password,

You don't ever actually use 'username' and 'password'.

@@ +170,3 @@
+  username = NULL;
+  g_free (password);
+  password = NULL;

You could free 'host' right after you use it, rather than needing to free it here and in 'error:'.

@@ +178,3 @@
+  datain = NULL;
+
+  if (!buffer)

== NULL

@@ +189,3 @@
+
+error:
+  if (datain)

!= NULL

@@ +198,3 @@
+}
+
+

Could you get away just running the synchronous version in a thread, rather than reimplementing it? ;-)

@@ +293,3 @@
+      "destination-port", &data->port,
+      "username", &data->username,
+      "password", &data->password,

You don't actually use the username and password here either.

@@ +298,3 @@
+  in = g_io_stream_get_input_stream (io_stream);
+
+  data->data = g_data_input_stream_new (in);

data as a choice of name for both the variable and the field here is a bit unfortunate.

::: wocky/wocky-http-proxy.h
@@ +21,3 @@
+
+#ifndef __WOCKY_HTTP_PROXY_H__
+#define __WOCKY_HTTP_PROXY_H__

You shouldn't use double-underscores here. They're reserved for the compiler. Since this is an internal header, _WOCKY_HTTP_PROXY_H_ seems appropriate.

(Yeah, I know all the other headers violate this.)

See bug URL for the git branch. Note that I decided to add HTTP proxy authentication and unit tests, will come back soon.

(In reply to comment #5)
> Review of attachment 38179 [details] [review]:
> 
> This looks functionally pretty good to me. There are a couple of outright bugs
> — the Makefile.am omission, and the crash I suspect — plus a bunch of nitpicks.
> 
> ::: wocky/Makefile.am
> @@ +140,3 @@
> +  handwritten_sources += $(PROXY_SRC)
> +endif
> +
> 
> You should have an 'else' branch which adds PROXY_SRC to EXTRA_DIST.

Done: bbd06c9c7816f4f73dc58f130ac20c5fc0c4e929

> 
> ::: wocky/wocky-connector.c
> @@ +807,3 @@
> +      host, port, NULL, tcp_host_connected, connector);
> +#endif
> +}
> 
> Huh, connecting to xmpp-client://jabber.org does the right thing?

Yes, that's one of the new feature in GLib, you can create a socket from a network URI (basically an URI with :// and a non-empty hostname). The URI parser for that will be public in 2.28 though.

Now the rational for using this in wocky is to let the proxy resolver know what protocol we are trying to connect to. This way, if the system configuration contains a setting specific for xmpp-client, it can be choosen. By default the scheme is set to none (e.g. none://hostname:1234). Note that the Gnome system proxy settings will be redesign for 3.0 to allow more protocols to be configured.

> 
> ::: wocky/wocky-http-proxy.c
> @@ +54,3 @@
> +  /* must chain up */
> +  G_OBJECT_CLASS (wocky_http_proxy_parent_class)->finalize (object);
> +}
> 
> Can't you just not override ->finalize?

Right: 21fb9862d177ba4f8af7f52589a44cfdb74c41d7

> 
> @@ +82,3 @@
> +  const gchar *ptr = buffer + 7;
> +
> +  if (strncmp (buffer, "HTTP/1.", 7)
> 
> I'd prefer 'strncmp (...) != 0' for clarity

Sorry, porting from GLib coding style: 0e82a3d7ff4aec5db837ec7ffe4bcb0d42ce352d

> 
> @@ +91,3 @@
> +
> +  ptr++;
> +  while (*ptr == ' ') ptr++;
> 
> Won't this crash if the buffer is shorter than we expect?
No, buffer is NULL terminated and is at minimum "HTTP/1.X", in which case ptr++ will set ptr to the NULL character and *ptr == ' ' will be '\0' == ' ' which is false. Then atoi() will return 0, which will lead to msg being "".

But I'll impose myself the task of writing unit tests for the http proxy. This way I can demonstrate those tricky cases.

> 
> @@ +109,3 @@
> +
> +      ptr = strchr (msg_start, '\r');
> +      if (!ptr)
> 
> Our coding style asks for 'ptr == NULL'

Fixed: 0e82a3d7ff4aec5db837ec7ffe4bcb0d42ce352d

> 
> @@ +120,3 @@
> +        g_set_error (error, G_IO_ERROR, G_IO_ERROR_PROXY_FAILED,
> +            "HTTP proxy connection failed: %s",
> +            msg);
> 
> Might be nice to include the error code in the message, too.

Fixed: 37ecb8ec12a9f52989272e643d2ca800e3c3babd

> 
> @@ +138,3 @@
> +  GInputStream *in;
> +  GOutputStream *out;
> +  GDataInputStream *datain;
> 
> data_in would be a clearer name. I thought this was a typo for 'detain' or
> something at first reading.
> 
> @@ +149,3 @@
> +      "destination-port", &port,
> +      "username", &username,
> +      "password", &password,
> 
> You don't ever actually use 'username' and 'password'.

But I think I should, https://bugs.freedesktop.org/show_bug.cgi?id=16034. I'll implement that and will come back.

> 
> @@ +170,3 @@
> +  username = NULL;
> +  g_free (password);
> +  password = NULL;
> 
> You could free 'host' right after you use it, rather than needing to free it
> here and in 'error:'.

Fixed: 338ef705da73de3d655dbd5a9b19f74f11bd1cd0

> 
> @@ +178,3 @@
> +  datain = NULL;
> +
> +  if (!buffer)
> 
> == NULL

Fixed: 0e82a3d7ff4aec5db837ec7ffe4bcb0d42ce352d

> 
> @@ +189,3 @@
> +
> +error:
> +  if (datain)
> 
> != NULL

Fixed: 0e82a3d7ff4aec5db837ec7ffe4bcb0d42ce352d

> 
> @@ +198,3 @@
> +}
> +
> +
> 
> Could you get away just running the synchronous version in a thread, rather
> than reimplementing it? ;-)

It's not a full reimplementation though, but it could have worked. I think one or the other is fine.

> 
> @@ +293,3 @@
> +      "destination-port", &data->port,
> +      "username", &data->username,
> +      "password", &data->password,
> 
> You don't actually use the username and password here either.
> 
> @@ +298,3 @@
> +  in = g_io_stream_get_input_stream (io_stream);
> +
> +  data->data = g_data_input_stream_new (in);
> 
> data as a choice of name for both the variable and the field here is a bit
> unfortunate.

Renave data->data_in (which match the previous name for the manipulation of GDataInputStream). 0e82a3d7ff4aec5db837ec7ffe4bcb0d42ce352d

> 
> ::: wocky/wocky-http-proxy.h
> @@ +21,3 @@
> +
> +#ifndef __WOCKY_HTTP_PROXY_H__
> +#define __WOCKY_HTTP_PROXY_H__
> 
> You shouldn't use double-underscores here. They're reserved for the compiler.
> Since this is an internal header, _WOCKY_HTTP_PROXY_H_ seems appropriate.
> 
> (Yeah, I know all the other headers violate this.)

Lol, ok, I wonder which funny compiler would declare __WOCKY_HTTP_PROXY_H__ ;):

Fixed: f9ff6bd697e76daff802059e3e7c766790d4be25

I've improve my branch with bug fix (found while writing unit tests) and added basic proxy authentication. Please refer to the last four commits in my branch:

http://git.collabora.co.uk/?p=user/nicolas/wocky.git;a=log;h=refs/heads/http-proxy

- Remove HTTP error code for known errors
- Handle EOS gracefully by manually generating error
- Implement HTTP basic proxy authentication
- Unit tests for HTTP Proxy support

Nice. Just nitpicks on the test, actually:

+  for (i = 0; i < sizeof (test_cases) / sizeof (test_cases[0]); i++)

Glib has a nice macro for this: G_N_ELEMENTS().

+tearup (const HttpTestCase *test_case)

I love the idea of ‘tearup’ being the opposite of ‘teardown’.

+          g_assert (!wocky_strdiff ("Host: to:443", buffer));

g_assert_cmpstr ("Host: to:443", ==, buffer);

+  g_assert (has_host == 1);
+  g_assert (has_user_agent == 1);

I assume the point of these being counters rather than booleans is to catch the same header being included twice? In any case, g_assert_cmpuint (has_host, ==, 1); will give better output if it fails.

Ditto a bunch of other assertions.

I don't like the magic numbers in the comparisons like:

   g_ascii_strncasecmp ("Proxy-Authorization:", buffer, 20)

Maybe you could add str_has_prefix_case (const gchar *str, const gchar *prefix), by analogy to g_str_has_prefix(), which calls g_ascii_strncasecmp (str, prefix, strlen (prefix)); ?

(In reply to comment #8)
> Nice. Just nitpicks on the test, actually:
> 
> +  for (i = 0; i < sizeof (test_cases) / sizeof (test_cases[0]); i++)
> 
> Glib has a nice macro for this: G_N_ELEMENTS().

Nice ! (fixed)

> 
> +tearup (const HttpTestCase *test_case)
> 
> I love the idea of ‘tearup’ being the opposite of ‘teardown’.

I think I've taken that from some JUnit tests a long time ago ...

> 
> +          g_assert (!wocky_strdiff ("Host: to:443", buffer));
> 
> g_assert_cmpstr ("Host: to:443", ==, buffer);

Right, that's much better and it handles NULL properly. (fixed)

> 
> +  g_assert (has_host == 1);
> +  g_assert (has_user_agent == 1);
> 
> I assume the point of these being counters rather than booleans is to catch the
> same header being included twice? In any case, g_assert_cmpuint (has_host, ==,
> 1); will give better output if it fails.

Ok (fixed)

> 
> Ditto a bunch of other assertions.

I went across them all and applied same logic.

> 
> I don't like the magic numbers in the comparisons like:
> 
>    g_ascii_strncasecmp ("Proxy-Authorization:", buffer, 20)
> 
> Maybe you could add str_has_prefix_case (const gchar *str, const gchar
> *prefix), by analogy to g_str_has_prefix(), which calls g_ascii_strncasecmp
> (str, prefix, strlen (prefix)); ?

That's better, thanks. (fixed)

Pushed to wocky-0.10 branch, and master.