Merge lp:~alecu/ubuntuone-client/proxy-tunnel-auth into lp:ubuntuone-client

[Alejandro J. Cura]

- Use proxy credentials from the keyring (LP: #929207)

- Use proxy credentials from the keyring (LP: #929207)

[lp:~alecu/ubuntuone-client/proxy-tunnel-auth]

1238. By Alejandro J. Cura on 2012-03-15

merged from trunk

[Manuel de la Peña (mandel)]

Everything looks good, here are some small comments:

return QNetworkProxy(QNetworkProxy.NoProxy)

As far as I understand this, the tunnel will use no proxy ignoring the application level proxy. I think it might be more appropriate to return QNetworkProxy(QNetworkProxy.DefaultProxy). Returning default proxy will make the code use a higher alternative, for example the application-level proxy settings. I know that we are using build_proxy to set the application level proxy, but I want to be 100% saure that we do the right thing, also when the proxy is the application level proxy DefaultProxy and NoProxy have the same meaning therefore in our current code we will be using the same.

This is just a save guard, so is certainly not blocking the code from landing.

Maybe we could ensure that self.proxy_domain is always a python string and not a QString in memory, it would be as simple as changing:

340 + self.proxy_domain = proxy.hostName()

for

340 + self.proxy_domain = str(proxy.hostName())

I think it would be less error prone, what is you opinion?

I'll approve even with the above comments.

review: Approve

[Alejandro J. Cura (alecu)]

> Everything looks good, here are some small comments:
>
> return QNetworkProxy(QNetworkProxy.NoProxy)
>
> As far as I understand this, the tunnel will use no proxy ignoring the
> application level proxy. I think it might be more appropriate to return
> QNetworkProxy(QNetworkProxy.DefaultProxy). Returning default proxy will make
> the code use a higher alternative, for example the application-level proxy
> settings. I know that we are using build_proxy to set the application level
> proxy, but I want to be 100% saure that we do the right thing, also when the
> proxy is the application level proxy DefaultProxy and NoProxy have the same
> meaning therefore in our current code we will be using the same.

I've fixed this, thanks for the suggestion.

> Maybe we could ensure that self.proxy_domain is always a python string and not
> a QString in memory, it would be as simple as changing:
>
> 340 + self.proxy_domain = proxy.hostName()
>
> for
>
> 340 + self.proxy_domain = str(proxy.hostName())
>
> I think it would be less error prone, what is you opinion?

I don't think that's needed, since most uses of self.proxy_domain are for comparing again proxy.hostName(). In the only place where it's sent from the containing object to the keyring.get_credentials method it is already being transformed with str.

I'm now pushing the branch with the first fix requested.

Thanks for the review!

[lp:~alecu/ubuntuone-client/proxy-tunnel-auth]

1239. By Alejandro J. Cura on 2012-03-16

Change fallback from NoProxy to DefaultProxy.

[Diego Sarmentero (diegosarmentero)]

I have this failure running the tests:

[ERROR]
Traceback (most recent call last):
  File "/usr/lib/python2.7/dist-packages/twisted/internet/defer.py", line 1039, in _inlineCallbacks
    result = g.send(result)
  File "/media/gato/proyectos/canonical/ubuntuone-client-alecu-last/ubuntuone/syncdaemon/action_queue.py", line 839, in get_webclient
    oauth_sign_plain=True)
exceptions.TypeError: __init__() got an unexpected keyword argument 'connector'

tests.syncdaemon.test_action_queue.BasicTests.test_get_webclient

review: Needs Fixing

[Alejandro J. Cura (alecu)]

> I have this failure running the tests:
>
> [ERROR]
> Traceback (most recent call last):
> File "/usr/lib/python2.7/dist-packages/twisted/internet/defer.py", line
> 1039, in _inlineCallbacks
> result = g.send(result)
> File "/media/gato/proyectos/canonical/ubuntuone-client-alecu-
> last/ubuntuone/syncdaemon/action_queue.py", line 839, in get_webclient
> oauth_sign_plain=True)
> exceptions.TypeError: __init__() got an unexpected keyword argument
> 'connector'
>
> tests.syncdaemon.test_action_queue.BasicTests.test_get_webclient

Looks like the ussoc you are using is not trunk.
Try updating nightlies or configuring this branch using "--with-sso=/path/to/sso/trunk"

[Diego Sarmentero (diegosarmentero)]

+1

review: Approve

=== modified file 'tests/proxy/test_tunnel_server.py'
--- tests/proxy/test_tunnel_server.py	2012-03-14 17:38:31 +0000
+++ tests/proxy/test_tunnel_server.py	2012-03-16 13:44:19 +0000
@@ -21,6 +21,7 @@
 from twisted.internet import defer, protocol, reactor
 from twisted.trial.unittest import TestCase
 from PyQt4.QtCore import QCoreApplication
+from PyQt4.QtNetwork import QAuthenticator
 from ubuntuone.devtools.testcases.squid import SquidTestCase
 
 from tests.proxy import (
@@ -51,6 +52,11 @@
 SAMPLE_HOST = "samplehost.com"
 SAMPLE_PORT = 443
 
+FAKE_CREDS = {
+    "username": "rhea",
+    "password": "caracolcaracola",
+}
+
 
 class DisconnectingProtocol(protocol.Protocol):
     """A protocol that just disconnects."""
@@ -154,9 +160,16 @@
 
     protocol = None
     connection_result = defer.succeed(True)
+    credentials = None
+    check_credentials = False
+    proxy_domain = None
 
     def connect(self, hostport):
         """Establish a connection with the other end."""
+        if (self.check_credentials and
+            self.protocol.proxy_credentials != FAKE_CREDS):
+            self.proxy_domain = "fake domain"
+            return defer.fail(tunnel_server.ProxyAuthenticationError())
         return self.connection_result
 
     def write(self, data):
@@ -167,6 +180,10 @@
     def stop(self):
         """Stop this fake client."""
 
+    def close(self):
+        """Reset this client."""
+
+
 
 class ServerTunnelProtocolTestCase(SquidTestCase):
     """Tests for the ServerTunnelProtocol."""
@@ -243,6 +260,17 @@
         self.proto.header_line("key: host:port")
         self.assertIn("key", dict(self.proto.received_headers))
 
+    @defer.inlineCallbacks
+    def test_keyring_credentials_are_retried(self):
+        """Wrong credentials are retried with values from keyring."""
+        self.fake_client.check_credentials = True
+        self.patch(self.proto, "error_response",
+                   lambda code, desc: self.fail(desc))
+        self.proto.proxy_domain = "xxx"
+        self.patch(tunnel_server.Keyring, "get_credentials",
+                   lambda _, domain: defer.succeed(FAKE_CREDS))
+        yield self.proto.headers_done()
+
 
 class FakeServerTunnelProtocol(object):
     """A fake ServerTunnelProtocol."""
@@ -250,6 +278,7 @@
     def __init__(self):
         """Initialize this fake tunnel."""
         self.response_received = defer.Deferred()
+        self.proxy_credentials = None
 
     def response_data_received(self, data):
         """Fire the response deferred."""
@@ -259,6 +288,55 @@
     def remote_disconnected(self):
         """The remote server disconnected."""
 
+    def proxy_auth_required(self, proxy, authenticator):
+        """Proxy credentials are needed."""
+        if self.proxy_credentials:
+            authenticator.setUser(self.proxy_credentials["username"])
+            authenticator.setPassword(self.proxy_credentials["password"])
+
+
+class BuildProxyTestCase(TestCase):
+    """Tests for the build_proxy function."""
+
+    def test_socks_is_preferred(self):
+        """Socks overrides all protocols."""
+        settings = {
+            "http": {"host": "httphost", "port": 3128},
+            "https": {"host": "httpshost", "port": 3129},
+            "socks": {"host": "sockshost", "port": 1080},
+        }
+        proxy = tunnel_server.build_proxy(settings)
+        self.assertEqual(proxy.type(), proxy.Socks5Proxy)
+        self.assertEqual(proxy.hostName(), "sockshost")
+        self.assertEqual(proxy.port(), 1080)
+
+    def test_https_beats_http(self):
+        """HTTPS wins over HTTP, since all of SD traffic is https."""
+        settings = {
+            "http": {"host": "httphost", "port": 3128},
+            "https": {"host": "httpshost", "port": 3129},
+        }
+        proxy = tunnel_server.build_proxy(settings)
+        self.assertEqual(proxy.type(), proxy.HttpProxy)
+        self.assertEqual(proxy.hostName(), "httpshost")
+        self.assertEqual(proxy.port(), 3129)
+
+    def test_http_if_no_other_choice(self):
+        """Finally, we use the host configured for HTTP."""
+        settings = {
+            "http": {"host": "httphost", "port": 3128},
+        }
+        proxy = tunnel_server.build_proxy(settings)
+        self.assertEqual(proxy.type(), proxy.HttpProxy)
+        self.assertEqual(proxy.hostName(), "httphost")
+        self.assertEqual(proxy.port(), 3128)
+
+    def test_use_noproxy_as_fallback(self):
+        """If nothing useful, revert to no proxy."""
+        settings = {}
+        proxy = tunnel_server.build_proxy(settings)
+        self.assertEqual(proxy.type(), proxy.DefaultProxy)
+
 
 class RemoteSocketTestCase(SquidTestCase):
     """Tests for the client that connects to the other side."""
@@ -276,8 +354,9 @@
 
         self.addCleanup(tunnel_server.QNetworkProxy.setApplicationProxy,
                         tunnel_server.QNetworkProxy.applicationProxy())
-        tunnel_server.QNetworkProxy.setApplicationProxy(
-                        tunnel_server.build_proxy(self.get_proxy_settings()))
+        settings = {"http": self.get_proxy_settings()}
+        proxy = tunnel_server.build_proxy(settings)
+        tunnel_server.QNetworkProxy.setApplicationProxy(proxy)
 
     def test_invalid_port(self):
         """A request with an invalid port fails with a 400."""
@@ -365,6 +444,52 @@
 
     get_proxy_settings = RemoteSocketTestCase.get_auth_proxy_settings
 
+    @defer.inlineCallbacks
+    def test_proxy_authentication_error(self):
+        """The proxy credentials were wrong on purpose."""
+        settings = {"http": self.get_proxy_settings()}
+        settings["http"]["password"] = "wrong password!!!"
+        proxy = tunnel_server.build_proxy(settings)
+        tunnel_server.QNetworkProxy.setApplicationProxy(proxy)
+        fake_protocol = FakeServerTunnelProtocol()
+        client = tunnel_server.RemoteSocket(fake_protocol)
+        self.addCleanup(client.stop)
+        url = urlparse(self.dest_url)
+        yield self.assertFailure(client.connect(url.netloc),
+                                 tunnel_server.ProxyAuthenticationError)
+
+    @defer.inlineCallbacks
+    def test_proxy_nobody_listens(self):
+        """The proxy settings point to a proxy that's unreachable."""
+        settings = dict(http={
+            "host": "127.0.0.1",
+            "port": 83,  # unused port according to /etc/services
+        })
+        proxy = tunnel_server.build_proxy(settings)
+        tunnel_server.QNetworkProxy.setApplicationProxy(proxy)
+        fake_protocol = FakeServerTunnelProtocol()
+        client = tunnel_server.RemoteSocket(fake_protocol)
+        self.addCleanup(client.stop)
+        url = urlparse(self.dest_url)
+        yield self.assertFailure(client.connect(url.netloc),
+                                 tunnel_server.ConnectionError)
+
+    def test_use_credentials(self):
+        """The credentials are used if present."""
+        fake_protocol = FakeServerTunnelProtocol()
+        client = tunnel_server.RemoteSocket(fake_protocol)
+        proxy = tunnel_server.build_proxy(FAKE_SETTINGS)
+        authenticator = QAuthenticator()
+
+        client.proxyAuthenticationRequired.emit(proxy, authenticator)
+        self.assertEqual(proxy.user(), "")
+        self.assertEqual(proxy.password(), "")
+        fake_protocol.proxy_credentials = FAKE_CREDS
+
+        client.proxyAuthenticationRequired.emit(proxy, authenticator)
+        self.assertEqual(authenticator.user(), FAKE_CREDS["username"])
+        self.assertEqual(authenticator.password(), FAKE_CREDS["password"])
+
 
 class FakeNetworkProxyFactoryClass(object):
     """A fake QNetworkProxyFactory."""
@@ -375,7 +500,7 @@
         if enabled:
             self.proxy_type = tunnel_server.QNetworkProxy.HttpProxy
         else:
-            self.proxy_type = tunnel_server.QNetworkProxy.NoProxy
+            self.proxy_type = tunnel_server.QNetworkProxy.DefaultProxy
 
     def type(self):
         """Return the proxy type configured."""

=== modified file 'ubuntuone/proxy/common.py'
--- ubuntuone/proxy/common.py	2012-03-09 22:36:14 +0000
+++ ubuntuone/proxy/common.py	2012-03-16 13:44:19 +0000
@@ -56,5 +56,3 @@
     def format_headers(self, headers):
         """Format some headers as a few response lines."""
         return "".join("%s: %s" % item + CRLF for item in headers.items())
-
-

=== modified file 'ubuntuone/proxy/tunnel_server.py'
--- ubuntuone/proxy/tunnel_server.py	2012-03-14 17:38:31 +0000
+++ ubuntuone/proxy/tunnel_server.py	2012-03-16 13:44:19 +0000
@@ -44,6 +44,7 @@
 from twisted.internet import defer, interfaces
 from zope.interface import implements
 
+from ubuntu_sso.keyring import Keyring
 from ubuntu_sso.utils.webclient import gsettings
 from ubuntuone.proxy.common import BaseTunnelProtocol, CRLF, TUNNEL_PORT_LABEL
 from ubuntuone.proxy.logger import logger
@@ -64,17 +65,25 @@
     """Credentials mismatch going thru a proxy."""
 
 
-def build_proxy(settings):
+def build_proxy(settings_groups):
     """Create a QNetworkProxy from these settings."""
-    if "host" not in settings or "port" not in settings:
-        return QNetworkProxy(QNetworkProxy.NoProxy)
-    else:
-        # TODO: add authentication here, to replace the empty user/pass
-        return QNetworkProxy(QNetworkProxy.HttpProxy,
-                             hostName=settings.get("host", ""),
-                             port=settings.get("port", 0),
-                             user=settings.get("username", ""),
-                             password=settings.get("password", ""))
+    proxy_groups = [
+        ("socks", QNetworkProxy.Socks5Proxy),
+        ("https", QNetworkProxy.HttpProxy),
+        ("http", QNetworkProxy.HttpProxy),
+    ]
+    for group, proxy_type in proxy_groups:
+        if group not in settings_groups:
+            continue
+        settings = settings_groups[group]
+        if "host" in settings and "port" in settings:
+            return QNetworkProxy(proxy_type,
+                                 hostName=settings.get("host", ""),
+                                 port=settings.get("port", 0),
+                                 user=settings.get("username", ""),
+                                 password=settings.get("password", ""))
+    logger.error("No proxy correctly configured.")
+    return QNetworkProxy(QNetworkProxy.DefaultProxy)
 
 
 class RemoteSocket(QTcpSocket):
@@ -84,13 +93,14 @@
         """Initialize this object."""
         super(RemoteSocket, self).__init__()
         self.protocol = tunnel_protocol
-        self.disconnected.connect(self.handle_disconnected)
         self.connected_d = defer.Deferred()
         self.connected.connect(self.handle_connected)
+        self.proxyAuthenticationRequired.connect(self.handle_auth_required)
         self.buffered_data = []
 
     def handle_connected(self):
         """When connected, send all pending data."""
+        self.disconnected.connect(self.handle_disconnected)
         self.connected_d.callback(None)
         for d in self.buffered_data:
             logger.debug("writing remote: %d bytes", len(d))
@@ -120,13 +130,29 @@
             raise ConnectionError(400, "Destination port must be an integer.")
 
         self.readyRead.connect(self.handle_ready_read)
-        # TODO: handle the following signals in an upcoming branch
-        #self.error.connect(...)
-        #self.proxyAuthenticationRequired.connect(...)
+        self.error.connect(self.handle_error)
         self.connectToHost(host, port)
 
         return self.connected_d
 
+    def handle_auth_required(self, proxy, authenticator):
+        """Handle the proxyAuthenticationRequired signal."""
+        self.protocol.proxy_auth_required(proxy, authenticator)
+
+    def handle_error(self, socket_error):
+        """Some error happened while connecting."""
+        error_description = "%s (%d)" % (self.errorString(), socket_error)
+        logger.error("connection error: %s", error_description)
+        if self.connected_d.called:
+            return
+
+        if socket_error == self.ProxyAuthenticationRequiredError:
+            error = ProxyAuthenticationError(407, error_description)
+        else:
+            error = ConnectionError(500, error_description)
+
+        self.connected_d.errback(error)
+
     def handle_ready_read(self):
         """Forward data from the remote end to the parent protocol."""
         data = self.readAll()
@@ -149,19 +175,36 @@
         """Initialize this protocol."""
         BaseTunnelProtocol.__init__(self)
         self.hostport = ""
-        self.client = client_class(self)
+        self.client = None
+        self.client_class = client_class
+        self.proxy_credentials = None
+        self.proxy_domain = None
 
     def error_response(self, code, description):
         """Write a response with an error, and disconnect."""
         self.write_transport("HTTP/1.0 %d %s" % (code, description) + CRLF * 2)
         self.transport.loseConnection()
-        self.client.stop()
+        if self.client:
+            self.client.stop()
         self.clearLineBuffer()
 
     def write_transport(self, data):
         """Write a response in the transport."""
         self.transport.write(data)
 
+    def proxy_auth_required(self, proxy, authenticator):
+        """Proxy authentication is required."""
+        logger.info("auth_required %r, %r, %r", self.proxy_credentials,
+                    proxy.hostName(), self.proxy_domain)
+        if self.proxy_credentials and proxy.hostName() == self.proxy_domain:
+            logger.info("Credentials added to authenticator: %r.",
+                        self.proxy_credentials)
+            authenticator.setUser(self.proxy_credentials["username"])
+            authenticator.setPassword(self.proxy_credentials["password"])
+        else:
+            logger.info("Credentials needed, but none available.")
+            self.proxy_domain = proxy.hostName()
+
     def handle_first_line(self, line):
         """Special handling for the first line received."""
         try:
@@ -180,7 +223,24 @@
     def headers_done(self):
         """An empty line was received, start connecting and switch mode."""
         try:
-            yield self.client.connect(self.hostport)
+            try:
+                logger.info("Connecting once")
+                self.client = self.client_class(self)
+                yield self.client.connect(self.hostport)
+            except ProxyAuthenticationError:
+                if not self.proxy_domain:
+                    logger.info("No proxy domain defined")
+                    raise
+
+                credentials = yield Keyring().get_credentials(
+                                                str(self.proxy_domain))
+                if "username" in credentials:
+                    self.proxy_credentials = credentials
+                logger.info("Connecting again with keyring credentials")
+                self.client = self.client_class(self)
+                yield self.client.connect(self.hostport)
+                logger.info("Connected with keyring credentials")
+
             response_headers = {
                 "Server": "Ubuntu One proxy tunnel",
             }
@@ -188,7 +248,10 @@
                                  CRLF + self.format_headers(response_headers) +
                                  CRLF)
         except ConnectionError as e:
+            logger.exception("Connection error")
             self.error_response(e.code, e.description)
+        except Exception:
+            logger.exception("Unhandled problem while connecting")
 
     def rawDataReceived(self, data):
         """Tunnel all raw data straight to the other side."""
@@ -269,7 +332,7 @@
         settings = gsettings.get_proxy_settings()
         enabled = len(settings) > 0
         if enabled:
-            proxy = build_proxy(settings["http"])
+            proxy = build_proxy(settings)
             QNetworkProxy.setApplicationProxy(proxy)
         else:
             logger.info("Proxy is disabled.")
@@ -277,7 +340,7 @@
     else:
         query = QNetworkProxyQuery(host, port)
         proxies = QNetworkProxyFactory.systemProxyForQuery(query)
-        return len(proxies) and proxies[0].type() != QNetworkProxy.NoProxy
+        return len(proxies) and proxies[0].type() != QNetworkProxy.DefaultProxy
 
 
 def main(argv):
@@ -286,6 +349,8 @@
         sys.stdout.write("Proxy not enabled.")
         sys.stdout.flush()
     else:
+        from dbus.mainloop.qt import DBusQtMainLoop
+        DBusQtMainLoop(set_as_default=True)
         app = QCoreApplication(argv)
         tunnel_server = TunnelServer()
         sys.stdout.write("%s: %d\n" % (TUNNEL_PORT_LABEL, tunnel_server.port))