Merge lp:~tribaal/landscape-client/more-information-on-ssl-errors into lp:~landscape/landscape-client/trunk

[Chris Glass]

Add a specific error message when the cause of registration failure is an SSL error.

This branch adds a specific error message when the cause of registration failure is an SSL error, since it's a common pitfall for new users, this makes the failure more explicit.

Hopefully, this should put users on the right track when try to report/fix the problem.

It's implemented by simply firing a custom message when the SSL failure is detected, and making the GUI react to this message by printing a more specific error message in this particular case.

How to test:
- Build this branche's packages with "make package", install them on a machine
- Try to register against LDS without specifying an SSL certificate. You should see the new message.
- Try to register against hosted, specifying the LDS's instance certificate. You should see the new message as well.
- Registering using a valid certificate (LDS) or no certificate (hosted) should work.

[Free Ekanayaka (free.ekanayaka)]

Looks good, but I'd suggest to not introduce a new event (see below). +1 with that addressed

review: Approve

[lp:~tribaal/landscape-client/more-information-on-ssl-errors]

809. By Chris Glass on 2015-01-12

Changed code to use a parameter to the exchange-failed message as per code review.

It seems the mocking infrastructure is not able to handle parameters, however,
a test is still failing.

810. By Chris Glass on 2015-01-12

Fixed parameter name. Test still fails though.

[Free Ekanayaka (free.ekanayaka)]

Please also file a bug for this MP.

[Данило Шеган (danilo)]

Free's test fixes from https://pastebin.canonical.com/123160/ do indeed fix tests for me for all but

landscape.broker.tests.test_amp.RemoteBrokerTest.test_listen_events

which fails with https://pastebin.canonical.com/123161/

[Данило Шеган (danilo)]

FWIW, while testing this, landscape-config fails nicely with this error message (yet it still hangs for a while [like 20-30s] after that), yet landscape-client-install-ui just hangs for a while without any information on what's going on and then just exits. I'd lean on that being fixed in a new branch, though.

Otherwise, the code looks good.

review: Approve

[lp:~tribaal/landscape-client/more-information-on-ssl-errors]

811. By Chris Glass on 2015-01-12

Added free's fix for the AMQ handling of kwargs.
Small test fix on top.

[Chris Glass (tribaal)]

Thanks for the help Free!

@Danilo: I'll open a bug for the UI part since non-SSL failures have the same problem, AFAICT.

=== modified file 'landscape/broker/amp.py'
--- landscape/broker/amp.py	2013-07-11 08:40:49 +0000
+++ landscape/broker/amp.py	2015-01-12 12:10:59 +0000
@@ -29,7 +29,8 @@
             callable will be fired.
         """
         result = self.listen_events(handlers.keys())
-        return result.addCallback(lambda event_type: handlers[event_type]())
+        return result.addCallback(
+            lambda (event_type, kwargs): handlers[event_type](**kwargs))
 
 
 class FakeRemoteBroker(object):

=== modified file 'landscape/broker/exchange.py'
--- landscape/broker/exchange.py	2014-10-03 10:51:12 +0000
+++ landscape/broker/exchange.py	2015-01-12 12:10:59 +0000
@@ -348,7 +348,7 @@
 
 from twisted.internet.defer import Deferred, succeed
 
-from landscape.lib.fetch import HTTPCodeError
+from landscape.lib.fetch import HTTPCodeError, PyCurlError
 from landscape.lib.message import got_next_expected, ANCIENT
 from landscape.lib.versioning import is_version_higher, sort_versions
 from landscape.log import format_delta
@@ -573,6 +573,7 @@
 
         def handle_failure(error_class, error, traceback):
             self._exchanging = False
+
             if isinstance(error, HTTPCodeError) and error.http_code == 404:
                 # If we got a 404 HTTP error it could be that we're trying to
                 # speak a server API version that the server does not support,
@@ -583,7 +584,17 @@
                     self._message_store.set_server_api(DEFAULT_SERVER_API)
                     self.exchange()
                     return
-            self._reactor.fire("exchange-failed")
+
+            ssl_error = False
+            if isinstance(error, PyCurlError) and error.error_code == 60:
+                # The error returned is an SSL error, most likely the server
+                # is using a self-signed certificate. Let's fire a special
+                # event so that the GUI can display a nice message.
+                logging.error("Message exchange failed: %s" % error.message)
+                ssl_error = True
+
+            self._reactor.fire("exchange-failed", ssl_error=ssl_error)
+
             self._message_store.record_failure(int(self._reactor.time()))
             logging.info("Message exchange failed.")
             exchange_completed()

=== modified file 'landscape/broker/server.py'
--- landscape/broker/server.py	2014-11-06 07:42:13 +0000
+++ landscape/broker/server.py	2015-01-12 12:10:59 +0000
@@ -312,10 +312,10 @@
 
         def get_handler(event_type):
 
-            def handler():
+            def handler(**kwargs):
                 for call in calls:
                     self._reactor.cancel_call(call)
-                deferred.callback(event_type)
+                deferred.callback((event_type, kwargs))
 
             return handler
 

=== modified file 'landscape/broker/tests/test_amp.py'
--- landscape/broker/tests/test_amp.py	2013-05-07 21:29:44 +0000
+++ landscape/broker/tests/test_amp.py	2015-01-12 12:10:59 +0000
@@ -171,7 +171,7 @@
         self.reactor.call_later(0.05, self.reactor.fire, "event2")
         self.reactor.advance(0.05)
         self.remote._factory.fake_connection.flush()
-        self.assertEqual("event2", self.successResultOf(deferred))
+        self.assertEqual(("event2", {}), self.successResultOf(deferred))
 
     def test_call_on_events(self):
         """

=== modified file 'landscape/broker/tests/test_exchange.py'
--- landscape/broker/tests/test_exchange.py	2014-10-03 10:06:24 +0000
+++ landscape/broker/tests/test_exchange.py	2015-01-12 12:10:59 +0000
@@ -1,6 +1,6 @@
 from landscape import CLIENT_API
 from landscape.lib.persist import Persist
-from landscape.lib.fetch import HTTPCodeError
+from landscape.lib.fetch import HTTPCodeError, PyCurlError
 from landscape.lib.hashlib import md5
 from landscape.schema import Message, Int
 from landscape.broker.config import BrokerConfiguration
@@ -993,7 +993,7 @@
         """
         events = []
 
-        def failed_exchange():
+        def failed_exchange(ssl_error=False):
             events.append(None)
 
         self.reactor.call_on("exchange-failed", failed_exchange)
@@ -1001,6 +1001,43 @@
         self.exchanger.exchange()
         self.assertEqual([None], events)
 
+    def test_SSL_error_exchanging_causes_failed_exchange(self):
+        """
+        If an SSL error occurs when exchanging, the 'exchange-failed'
+        event should be fired with the optional "ssl_error" flag set to True.
+        """
+        self.log_helper.ignore_errors("Message exchange failed: Failed to "
+                                      "communicate.")
+        events = []
+
+        def failed_exchange(ssl_error):
+            if ssl_error:
+                events.append(None)
+
+        self.reactor.call_on("exchange-failed", failed_exchange)
+        self.transport.responses.append(PyCurlError(60,
+                                                    "Failed to communicate."))
+        self.exchanger.exchange()
+        self.assertEqual([None], events)
+
+    def test_pycurl_error_exchanging_causes_failed_exchange(self):
+        """
+        If an undefined PyCurl error is raised during exchange, (not an SSL
+        error), the 'exchange-failed' event should be fired with the ssl_error
+        flag set to False.
+        """
+        events = []
+
+        def failed_exchange(ssl_error):
+            if not ssl_error:
+                events.append(None)
+
+        self.reactor.call_on("exchange-failed", failed_exchange)
+        self.transport.responses.append(PyCurlError(10,  # Not 60
+                                                    "Failed to communicate."))
+        self.exchanger.exchange()
+        self.assertEqual([None], events)
+
     def test_wb_error_exchanging_records_failure_in_message_store(self):
         """
         If a traceback occurs whilst exchanging, the failure is recorded

=== modified file 'landscape/broker/tests/test_server.py'
--- landscape/broker/tests/test_server.py	2014-11-05 13:14:55 +0000
+++ landscape/broker/tests/test_server.py	2015-01-12 12:10:59 +0000
@@ -348,19 +348,33 @@
         The L{BrokerServer.listen_events} method returns a deferred which is
         fired when the first of the given events occurs.
         """
-        result = self.broker.listen_events(["event1", "event2"])
+        deferred = self.broker.listen_events(["event1", "event2"])
         self.reactor.fire("event2")
-        return self.assertSuccess(result, "event2")
+        result = self.successResultOf(deferred)
+        self.assertIsNotNone(result)
+
+    def test_listen_events_with_payload(self):
+        """
+        The L{BrokerServer.listen_events} method returns a deferred which is
+        fired when the first of the given events occurs. The result of the
+        deferred is a 2-tuple with name of the event and any keyword arguments
+        passed when the event was fired.
+        """
+        deferred = self.broker.listen_events(["event1", "event2"])
+        self.reactor.fire("event2", foo=123)
+        result = self.successResultOf(deferred)
+        self.assertEqual(("event2", {"foo": 123}), result)
 
     def test_listen_event_only_once(self):
         """
         The L{BrokerServer.listen_events} listens only to one occurrence of
         the given events.
         """
-        result = self.broker.listen_events(["event"])
+        deferred = self.broker.listen_events(["event"])
         self.assertEqual(self.reactor.fire("event"), [None])
         self.assertEqual(self.reactor.fire("event"), [])
-        return self.assertSuccess(result, "event")
+        result = self.successResultOf(deferred)
+        self.assertEqual("event", result[0])
 
     def test_listen_events_call_cancellation(self):
         """

=== modified file 'landscape/configuration.py'
--- landscape/configuration.py	2014-10-03 10:06:24 +0000
+++ landscape/configuration.py	2015-01-12 12:10:59 +0000
@@ -626,12 +626,20 @@
     def success():
         on_message("System successfully registered.")
 
-    def exchange_failure():
-        on_message("We were unable to contact the server. "
-                   "Your internet connection may be down. "
-                   "The landscape client will continue to try and contact "
-                   "the server periodically.",
-                   error=True)
+    def exchange_failure(ssl_error=False):
+        if ssl_error:
+            message = ("\nThe server's SSL information is incorrect, or fails "
+                       "signature verification!\n"
+                       "If the server is using a self-signed certificate, "
+                       "please ensure you supply it with the --ssl-public-key "
+                       "parameter.")
+        else:
+            message = ("\nWe were unable to contact the server.\n"
+                       "Your internet connection may be down. "
+                       "The landscape client will continue to try and contact "
+                       "the server periodically.")
+
+        on_message(message, error=True)
         return 2
 
     def handle_registration_errors(failure):

=== modified file 'landscape/tests/test_configuration.py'
--- landscape/tests/test_configuration.py	2014-10-03 10:06:24 +0000
+++ landscape/tests/test_configuration.py	2015-01-12 12:10:59 +0000
@@ -1805,7 +1805,7 @@
         self.mocker.call(register_done)
 
         # The deferred errback finally prints out this message.
-        print_text_mock("We were unable to contact the server. "
+        print_text_mock("\nWe were unable to contact the server.\n"
                         "Your internet connection may be down. "
                         "The landscape client will continue to try and "
                         "contact the server periodically.",
@@ -1828,6 +1828,57 @@
         register(self.config, print_text, exit.append, reactor=FakeReactor())
         self.assertEqual([2], exit)
 
+    def test_register_exchange_SSL_failure(self):
+        """
+        When registration fails because the server's SSL certificate could not
+        be validated, a message is printed and the program quits.
+        """
+        service = self.broker_service
+
+        registration_mock = self.mocker.replace(service.registration)
+        print_text_mock = self.mocker.replace(print_text)
+        reactor_mock = self.mocker.patch(FakeReactor)
+
+        # This must necessarily happen in the following order.
+        self.mocker.order()
+
+        # This very informative message is printed out.
+        print_text_mock("Please wait... ", "")
+
+        time_mock = self.mocker.replace("time")
+        time_mock.sleep(ANY)
+        self.mocker.count(1)
+
+        def register_done():
+            service.reactor.fire("exchange-failed", ssl_error=True)
+        registration_mock.register()
+        self.mocker.call(register_done)
+
+        # The deferred errback finally prints out this message.
+        print_text_mock("\nThe server's SSL information is incorrect, or "
+                        "fails signature verification!\n"
+                        "If the server is using a self-signed certificate, "
+                        "please ensure you supply it with the "
+                        "--ssl-public-key parameter.",
+                        error=True)
+
+        reactor_mock.stop()
+
+        # This is actually called after everything else since all deferreds
+        # are synchronous and callbacks will be executed immediately.
+        reactor_mock.run()
+
+        # Nothing else is printed!
+        print_text_mock(ANY)
+        self.mocker.count(0)
+
+        self.mocker.replay()
+
+        # DO IT!
+        exit = []
+        register(self.config, print_text, exit.append, reactor=FakeReactor())
+        self.assertEqual([2], exit)
+
     def test_register_timeout_failure(self):
         service = self.broker_service