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

Proposed by Alejandro J. Cura on 2012-03-15
Status: Merged
Approved by: Alejandro J. Cura on 2012-03-16
Approved revision: 1239
Merged at revision: 1211
Proposed branch: lp:~alecu/ubuntuone-client/proxy-tunnel-auth
Merge into: lp:ubuntuone-client
Diff against target: 417 lines (+212/-24)
3 files modified
tests/proxy/test_tunnel_server.py (+128/-3)
ubuntuone/proxy/common.py (+0/-2)
ubuntuone/proxy/tunnel_server.py (+84/-19)
To merge this branch: bzr merge lp:~alecu/ubuntuone-client/proxy-tunnel-auth
Reviewer Review Type Date Requested Status
Diego Sarmentero (community) Approve on 2012-03-16
Manuel de la Peña (community) 2012-03-15 Approve on 2012-03-16
Review via email: mp+97763@code.launchpad.net

Commit message

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

Description of the change

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

To post a comment you must log in.
1238. By Alejandro J. Cura on 2012-03-15

merged from trunk

Manuel de la Peña (mandel) wrote :

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) wrote :

> 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!

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

Change fallback from NoProxy to DefaultProxy.

Diego Sarmentero (diegosarmentero) wrote :

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) wrote :

> 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) wrote :

+1

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'tests/proxy/test_tunnel_server.py'
2--- tests/proxy/test_tunnel_server.py 2012-03-14 17:38:31 +0000
3+++ tests/proxy/test_tunnel_server.py 2012-03-16 13:44:19 +0000
4@@ -21,6 +21,7 @@
5 from twisted.internet import defer, protocol, reactor
6 from twisted.trial.unittest import TestCase
7 from PyQt4.QtCore import QCoreApplication
8+from PyQt4.QtNetwork import QAuthenticator
9 from ubuntuone.devtools.testcases.squid import SquidTestCase
10
11 from tests.proxy import (
12@@ -51,6 +52,11 @@
13 SAMPLE_HOST = "samplehost.com"
14 SAMPLE_PORT = 443
15
16+FAKE_CREDS = {
17+ "username": "rhea",
18+ "password": "caracolcaracola",
19+}
20+
21
22 class DisconnectingProtocol(protocol.Protocol):
23 """A protocol that just disconnects."""
24@@ -154,9 +160,16 @@
25
26 protocol = None
27 connection_result = defer.succeed(True)
28+ credentials = None
29+ check_credentials = False
30+ proxy_domain = None
31
32 def connect(self, hostport):
33 """Establish a connection with the other end."""
34+ if (self.check_credentials and
35+ self.protocol.proxy_credentials != FAKE_CREDS):
36+ self.proxy_domain = "fake domain"
37+ return defer.fail(tunnel_server.ProxyAuthenticationError())
38 return self.connection_result
39
40 def write(self, data):
41@@ -167,6 +180,10 @@
42 def stop(self):
43 """Stop this fake client."""
44
45+ def close(self):
46+ """Reset this client."""
47+
48+
49
50 class ServerTunnelProtocolTestCase(SquidTestCase):
51 """Tests for the ServerTunnelProtocol."""
52@@ -243,6 +260,17 @@
53 self.proto.header_line("key: host:port")
54 self.assertIn("key", dict(self.proto.received_headers))
55
56+ @defer.inlineCallbacks
57+ def test_keyring_credentials_are_retried(self):
58+ """Wrong credentials are retried with values from keyring."""
59+ self.fake_client.check_credentials = True
60+ self.patch(self.proto, "error_response",
61+ lambda code, desc: self.fail(desc))
62+ self.proto.proxy_domain = "xxx"
63+ self.patch(tunnel_server.Keyring, "get_credentials",
64+ lambda _, domain: defer.succeed(FAKE_CREDS))
65+ yield self.proto.headers_done()
66+
67
68 class FakeServerTunnelProtocol(object):
69 """A fake ServerTunnelProtocol."""
70@@ -250,6 +278,7 @@
71 def __init__(self):
72 """Initialize this fake tunnel."""
73 self.response_received = defer.Deferred()
74+ self.proxy_credentials = None
75
76 def response_data_received(self, data):
77 """Fire the response deferred."""
78@@ -259,6 +288,55 @@
79 def remote_disconnected(self):
80 """The remote server disconnected."""
81
82+ def proxy_auth_required(self, proxy, authenticator):
83+ """Proxy credentials are needed."""
84+ if self.proxy_credentials:
85+ authenticator.setUser(self.proxy_credentials["username"])
86+ authenticator.setPassword(self.proxy_credentials["password"])
87+
88+
89+class BuildProxyTestCase(TestCase):
90+ """Tests for the build_proxy function."""
91+
92+ def test_socks_is_preferred(self):
93+ """Socks overrides all protocols."""
94+ settings = {
95+ "http": {"host": "httphost", "port": 3128},
96+ "https": {"host": "httpshost", "port": 3129},
97+ "socks": {"host": "sockshost", "port": 1080},
98+ }
99+ proxy = tunnel_server.build_proxy(settings)
100+ self.assertEqual(proxy.type(), proxy.Socks5Proxy)
101+ self.assertEqual(proxy.hostName(), "sockshost")
102+ self.assertEqual(proxy.port(), 1080)
103+
104+ def test_https_beats_http(self):
105+ """HTTPS wins over HTTP, since all of SD traffic is https."""
106+ settings = {
107+ "http": {"host": "httphost", "port": 3128},
108+ "https": {"host": "httpshost", "port": 3129},
109+ }
110+ proxy = tunnel_server.build_proxy(settings)
111+ self.assertEqual(proxy.type(), proxy.HttpProxy)
112+ self.assertEqual(proxy.hostName(), "httpshost")
113+ self.assertEqual(proxy.port(), 3129)
114+
115+ def test_http_if_no_other_choice(self):
116+ """Finally, we use the host configured for HTTP."""
117+ settings = {
118+ "http": {"host": "httphost", "port": 3128},
119+ }
120+ proxy = tunnel_server.build_proxy(settings)
121+ self.assertEqual(proxy.type(), proxy.HttpProxy)
122+ self.assertEqual(proxy.hostName(), "httphost")
123+ self.assertEqual(proxy.port(), 3128)
124+
125+ def test_use_noproxy_as_fallback(self):
126+ """If nothing useful, revert to no proxy."""
127+ settings = {}
128+ proxy = tunnel_server.build_proxy(settings)
129+ self.assertEqual(proxy.type(), proxy.DefaultProxy)
130+
131
132 class RemoteSocketTestCase(SquidTestCase):
133 """Tests for the client that connects to the other side."""
134@@ -276,8 +354,9 @@
135
136 self.addCleanup(tunnel_server.QNetworkProxy.setApplicationProxy,
137 tunnel_server.QNetworkProxy.applicationProxy())
138- tunnel_server.QNetworkProxy.setApplicationProxy(
139- tunnel_server.build_proxy(self.get_proxy_settings()))
140+ settings = {"http": self.get_proxy_settings()}
141+ proxy = tunnel_server.build_proxy(settings)
142+ tunnel_server.QNetworkProxy.setApplicationProxy(proxy)
143
144 def test_invalid_port(self):
145 """A request with an invalid port fails with a 400."""
146@@ -365,6 +444,52 @@
147
148 get_proxy_settings = RemoteSocketTestCase.get_auth_proxy_settings
149
150+ @defer.inlineCallbacks
151+ def test_proxy_authentication_error(self):
152+ """The proxy credentials were wrong on purpose."""
153+ settings = {"http": self.get_proxy_settings()}
154+ settings["http"]["password"] = "wrong password!!!"
155+ proxy = tunnel_server.build_proxy(settings)
156+ tunnel_server.QNetworkProxy.setApplicationProxy(proxy)
157+ fake_protocol = FakeServerTunnelProtocol()
158+ client = tunnel_server.RemoteSocket(fake_protocol)
159+ self.addCleanup(client.stop)
160+ url = urlparse(self.dest_url)
161+ yield self.assertFailure(client.connect(url.netloc),
162+ tunnel_server.ProxyAuthenticationError)
163+
164+ @defer.inlineCallbacks
165+ def test_proxy_nobody_listens(self):
166+ """The proxy settings point to a proxy that's unreachable."""
167+ settings = dict(http={
168+ "host": "127.0.0.1",
169+ "port": 83, # unused port according to /etc/services
170+ })
171+ proxy = tunnel_server.build_proxy(settings)
172+ tunnel_server.QNetworkProxy.setApplicationProxy(proxy)
173+ fake_protocol = FakeServerTunnelProtocol()
174+ client = tunnel_server.RemoteSocket(fake_protocol)
175+ self.addCleanup(client.stop)
176+ url = urlparse(self.dest_url)
177+ yield self.assertFailure(client.connect(url.netloc),
178+ tunnel_server.ConnectionError)
179+
180+ def test_use_credentials(self):
181+ """The credentials are used if present."""
182+ fake_protocol = FakeServerTunnelProtocol()
183+ client = tunnel_server.RemoteSocket(fake_protocol)
184+ proxy = tunnel_server.build_proxy(FAKE_SETTINGS)
185+ authenticator = QAuthenticator()
186+
187+ client.proxyAuthenticationRequired.emit(proxy, authenticator)
188+ self.assertEqual(proxy.user(), "")
189+ self.assertEqual(proxy.password(), "")
190+ fake_protocol.proxy_credentials = FAKE_CREDS
191+
192+ client.proxyAuthenticationRequired.emit(proxy, authenticator)
193+ self.assertEqual(authenticator.user(), FAKE_CREDS["username"])
194+ self.assertEqual(authenticator.password(), FAKE_CREDS["password"])
195+
196
197 class FakeNetworkProxyFactoryClass(object):
198 """A fake QNetworkProxyFactory."""
199@@ -375,7 +500,7 @@
200 if enabled:
201 self.proxy_type = tunnel_server.QNetworkProxy.HttpProxy
202 else:
203- self.proxy_type = tunnel_server.QNetworkProxy.NoProxy
204+ self.proxy_type = tunnel_server.QNetworkProxy.DefaultProxy
205
206 def type(self):
207 """Return the proxy type configured."""
208
209=== modified file 'ubuntuone/proxy/common.py'
210--- ubuntuone/proxy/common.py 2012-03-09 22:36:14 +0000
211+++ ubuntuone/proxy/common.py 2012-03-16 13:44:19 +0000
212@@ -56,5 +56,3 @@
213 def format_headers(self, headers):
214 """Format some headers as a few response lines."""
215 return "".join("%s: %s" % item + CRLF for item in headers.items())
216-
217-
218
219=== modified file 'ubuntuone/proxy/tunnel_server.py'
220--- ubuntuone/proxy/tunnel_server.py 2012-03-14 17:38:31 +0000
221+++ ubuntuone/proxy/tunnel_server.py 2012-03-16 13:44:19 +0000
222@@ -44,6 +44,7 @@
223 from twisted.internet import defer, interfaces
224 from zope.interface import implements
225
226+from ubuntu_sso.keyring import Keyring
227 from ubuntu_sso.utils.webclient import gsettings
228 from ubuntuone.proxy.common import BaseTunnelProtocol, CRLF, TUNNEL_PORT_LABEL
229 from ubuntuone.proxy.logger import logger
230@@ -64,17 +65,25 @@
231 """Credentials mismatch going thru a proxy."""
232
233
234-def build_proxy(settings):
235+def build_proxy(settings_groups):
236 """Create a QNetworkProxy from these settings."""
237- if "host" not in settings or "port" not in settings:
238- return QNetworkProxy(QNetworkProxy.NoProxy)
239- else:
240- # TODO: add authentication here, to replace the empty user/pass
241- return QNetworkProxy(QNetworkProxy.HttpProxy,
242- hostName=settings.get("host", ""),
243- port=settings.get("port", 0),
244- user=settings.get("username", ""),
245- password=settings.get("password", ""))
246+ proxy_groups = [
247+ ("socks", QNetworkProxy.Socks5Proxy),
248+ ("https", QNetworkProxy.HttpProxy),
249+ ("http", QNetworkProxy.HttpProxy),
250+ ]
251+ for group, proxy_type in proxy_groups:
252+ if group not in settings_groups:
253+ continue
254+ settings = settings_groups[group]
255+ if "host" in settings and "port" in settings:
256+ return QNetworkProxy(proxy_type,
257+ hostName=settings.get("host", ""),
258+ port=settings.get("port", 0),
259+ user=settings.get("username", ""),
260+ password=settings.get("password", ""))
261+ logger.error("No proxy correctly configured.")
262+ return QNetworkProxy(QNetworkProxy.DefaultProxy)
263
264
265 class RemoteSocket(QTcpSocket):
266@@ -84,13 +93,14 @@
267 """Initialize this object."""
268 super(RemoteSocket, self).__init__()
269 self.protocol = tunnel_protocol
270- self.disconnected.connect(self.handle_disconnected)
271 self.connected_d = defer.Deferred()
272 self.connected.connect(self.handle_connected)
273+ self.proxyAuthenticationRequired.connect(self.handle_auth_required)
274 self.buffered_data = []
275
276 def handle_connected(self):
277 """When connected, send all pending data."""
278+ self.disconnected.connect(self.handle_disconnected)
279 self.connected_d.callback(None)
280 for d in self.buffered_data:
281 logger.debug("writing remote: %d bytes", len(d))
282@@ -120,13 +130,29 @@
283 raise ConnectionError(400, "Destination port must be an integer.")
284
285 self.readyRead.connect(self.handle_ready_read)
286- # TODO: handle the following signals in an upcoming branch
287- #self.error.connect(...)
288- #self.proxyAuthenticationRequired.connect(...)
289+ self.error.connect(self.handle_error)
290 self.connectToHost(host, port)
291
292 return self.connected_d
293
294+ def handle_auth_required(self, proxy, authenticator):
295+ """Handle the proxyAuthenticationRequired signal."""
296+ self.protocol.proxy_auth_required(proxy, authenticator)
297+
298+ def handle_error(self, socket_error):
299+ """Some error happened while connecting."""
300+ error_description = "%s (%d)" % (self.errorString(), socket_error)
301+ logger.error("connection error: %s", error_description)
302+ if self.connected_d.called:
303+ return
304+
305+ if socket_error == self.ProxyAuthenticationRequiredError:
306+ error = ProxyAuthenticationError(407, error_description)
307+ else:
308+ error = ConnectionError(500, error_description)
309+
310+ self.connected_d.errback(error)
311+
312 def handle_ready_read(self):
313 """Forward data from the remote end to the parent protocol."""
314 data = self.readAll()
315@@ -149,19 +175,36 @@
316 """Initialize this protocol."""
317 BaseTunnelProtocol.__init__(self)
318 self.hostport = ""
319- self.client = client_class(self)
320+ self.client = None
321+ self.client_class = client_class
322+ self.proxy_credentials = None
323+ self.proxy_domain = None
324
325 def error_response(self, code, description):
326 """Write a response with an error, and disconnect."""
327 self.write_transport("HTTP/1.0 %d %s" % (code, description) + CRLF * 2)
328 self.transport.loseConnection()
329- self.client.stop()
330+ if self.client:
331+ self.client.stop()
332 self.clearLineBuffer()
333
334 def write_transport(self, data):
335 """Write a response in the transport."""
336 self.transport.write(data)
337
338+ def proxy_auth_required(self, proxy, authenticator):
339+ """Proxy authentication is required."""
340+ logger.info("auth_required %r, %r, %r", self.proxy_credentials,
341+ proxy.hostName(), self.proxy_domain)
342+ if self.proxy_credentials and proxy.hostName() == self.proxy_domain:
343+ logger.info("Credentials added to authenticator: %r.",
344+ self.proxy_credentials)
345+ authenticator.setUser(self.proxy_credentials["username"])
346+ authenticator.setPassword(self.proxy_credentials["password"])
347+ else:
348+ logger.info("Credentials needed, but none available.")
349+ self.proxy_domain = proxy.hostName()
350+
351 def handle_first_line(self, line):
352 """Special handling for the first line received."""
353 try:
354@@ -180,7 +223,24 @@
355 def headers_done(self):
356 """An empty line was received, start connecting and switch mode."""
357 try:
358- yield self.client.connect(self.hostport)
359+ try:
360+ logger.info("Connecting once")
361+ self.client = self.client_class(self)
362+ yield self.client.connect(self.hostport)
363+ except ProxyAuthenticationError:
364+ if not self.proxy_domain:
365+ logger.info("No proxy domain defined")
366+ raise
367+
368+ credentials = yield Keyring().get_credentials(
369+ str(self.proxy_domain))
370+ if "username" in credentials:
371+ self.proxy_credentials = credentials
372+ logger.info("Connecting again with keyring credentials")
373+ self.client = self.client_class(self)
374+ yield self.client.connect(self.hostport)
375+ logger.info("Connected with keyring credentials")
376+
377 response_headers = {
378 "Server": "Ubuntu One proxy tunnel",
379 }
380@@ -188,7 +248,10 @@
381 CRLF + self.format_headers(response_headers) +
382 CRLF)
383 except ConnectionError as e:
384+ logger.exception("Connection error")
385 self.error_response(e.code, e.description)
386+ except Exception:
387+ logger.exception("Unhandled problem while connecting")
388
389 def rawDataReceived(self, data):
390 """Tunnel all raw data straight to the other side."""
391@@ -269,7 +332,7 @@
392 settings = gsettings.get_proxy_settings()
393 enabled = len(settings) > 0
394 if enabled:
395- proxy = build_proxy(settings["http"])
396+ proxy = build_proxy(settings)
397 QNetworkProxy.setApplicationProxy(proxy)
398 else:
399 logger.info("Proxy is disabled.")
400@@ -277,7 +340,7 @@
401 else:
402 query = QNetworkProxyQuery(host, port)
403 proxies = QNetworkProxyFactory.systemProxyForQuery(query)
404- return len(proxies) and proxies[0].type() != QNetworkProxy.NoProxy
405+ return len(proxies) and proxies[0].type() != QNetworkProxy.DefaultProxy
406
407
408 def main(argv):
409@@ -286,6 +349,8 @@
410 sys.stdout.write("Proxy not enabled.")
411 sys.stdout.flush()
412 else:
413+ from dbus.mainloop.qt import DBusQtMainLoop
414+ DBusQtMainLoop(set_as_default=True)
415 app = QCoreApplication(argv)
416 tunnel_server = TunnelServer()
417 sys.stdout.write("%s: %d\n" % (TUNNEL_PORT_LABEL, tunnel_server.port))

Subscribers

People subscribed via source and target branches