Merge lp:~free.ekanayaka/landscape-client/exchange-token into lp:~landscape/landscape-client/trunk

[Free Ekanayaka]

This branch implements the client-side part of the changes needed to detect cloned computers. The idea is that at each exchange the server will give the client a token that the client will be required to give back at the following exchange to prove that it's still the same client. If two clients with the same secure ID try to perform an exchange using the same token, only the first one that completes the exchange will succeed, because the second will not be authenticated since the token has changed in the meantime (after the first exchange completed).

This branch implements the client-side part of the changes needed to detect cloned computers. The idea is that at each exchange the server will give the client a token that the client will be required to give back at the following exchange to prove that it's still the same client. If two clients with the same secure ID try to perform an exchange using the same token, only the first one that completes the exchange will succeed, because the second will not be authenticated since the token has changed in the meantime (after the first exchange completed).

[lp:~free.ekanayaka/landscape-client/exchange-token]

655. By Free Ekanayaka on 2013-04-10

Better comments

656. By Free Ekanayaka on 2013-04-10

Rephrase

657. By Free Ekanayaka on 2013-04-10

Extract exchange-token logic into a separate method

658. By Free Ekanayaka on 2013-04-10

Move private method at the bottom

659. By Free Ekanayaka on 2013-04-11

Change the computer title for cloned clients

[Alberto Donato (ack)]

Looks good! +1

#1:
Not really related to your changes, but I see MessageStore hs a lot of getter/setters for parameters that are just passed to the persist. Maybe we could reduce duplication using __getattr__/__seattr__, perhaps with defaults for some parameters. Just an idea, not something for this branch anyway.

review: Approve

[lp:~free.ekanayaka/landscape-client/exchange-token]

660. By Free Ekanayaka on 2013-04-11

Nicer title

[Chris Glass (tribaal)]

+1 Nice!

review: Approve

[Free Ekanayaka (free.ekanayaka)]

#1 Yeah maybe it'd be an interesting cleanup for a follow-up branch.

=== modified file 'landscape/__init__.py'
--- landscape/__init__.py	2013-03-12 21:47:27 +0000
+++ landscape/__init__.py	2013-04-11 13:21:20 +0000
@@ -35,8 +35,11 @@
 # 3.4:
 #  * Add "hold" field to "change-packages"
 #  * Add "remove-hold" field to "change-packages"
+#
+# 3.5:
+#  * Support per-exchange authentication tokens
 
-CLIENT_API = "3.4"
+CLIENT_API = "3.5"
 
 from twisted.python import util
 

=== modified file 'landscape/broker/exchange.py'
--- landscape/broker/exchange.py	2013-04-10 09:54:13 +0000
+++ landscape/broker/exchange.py	2013-04-11 13:21:20 +0000
@@ -395,6 +395,7 @@
         self._reactor.call_in_thread(handle_result, None,
                                      self._transport.exchange, payload,
                                      self._registration_info.secure_id,
+                                     self._get_exchange_token(),
                                      payload.get("server-api"))
         return deferred
 
@@ -443,6 +444,26 @@
             self._exchange_id = self._reactor.call_later(interval,
                                                          self.exchange)
 
+    def _get_exchange_token(self):
+        """Get the token given us by the server at the last exchange.
+
+        It will be C{None} if we are not fully registered yet or if something
+        bad happened during the last exchange and we could not get the token
+        that the server had given us.
+        """
+        exchange_token = self._message_store.get_exchange_token()
+
+        # Before starting the exchange set the saved token to None. This will
+        # prevent us from locking ourselves out if the exchange fails or if we
+        # crash badly, while the server has saved a new token that we couldn't
+        # receive or persist (this works because if the token is None the
+        # server will be forgiving and will authenticate us based only on the
+        # secure ID we provide).
+        self._message_store.set_exchange_token(None)
+        self._message_store.commit()
+
+        return exchange_token
+
     def _notify_impending_exchange(self):
         self._reactor.fire("impending-exchange")
 
@@ -485,8 +506,7 @@
                    "accepted-types": accepted_types_digest,
                    "messages": messages,
                    "total-messages": total_messages,
-                   "next-expected-sequence": store.get_server_sequence(),
-                  }
+                   "next-expected-sequence": store.get_server_sequence()}
         accepted_client_types = self.get_client_accepted_message_types()
         accepted_client_types_hash = self._hash_types(accepted_client_types)
         if accepted_client_types_hash != self._client_accepted_types_hash:
@@ -524,7 +544,6 @@
             next_expected += len(payload["messages"])
 
         message_store_state = got_next_expected(message_store, next_expected)
-        message_store.commit()
         if message_store_state == ANCIENT:
             # The server has probably lost some data we sent it. The
             # slate has been wiped clean (by got_next_expected), now
@@ -536,6 +555,11 @@
             self.send({"type": "resynchronize"})
             self._reactor.fire("resynchronize-clients")
 
+        # Save the exchange token that the server has sent us. We will provide
+        # it at the next exchange to prove that we're still the same client.
+        # See also landscape.broker.transport.
+        message_store.set_exchange_token(result.get("next-exchange-token"))
+
         old_uuid = message_store.get_server_uuid()
         new_uuid = result.get("server-uuid")
         if new_uuid != old_uuid:
@@ -544,6 +568,8 @@
             self._reactor.fire("server-uuid-changed", old_uuid, new_uuid)
             message_store.set_server_uuid(new_uuid)
 
+        message_store.commit()
+
         sequence = message_store.get_server_sequence()
         for message in result.get("messages", ()):
             self.handle_message(message)

=== modified file 'landscape/broker/registration.py'
--- landscape/broker/registration.py	2013-04-09 12:18:50 +0000
+++ landscape/broker/registration.py	2013-04-11 13:21:20 +0000
@@ -337,8 +337,22 @@
 
     def _handle_unknown_id(self, message):
         id = self._identity
-        logging.info("Client has unknown secure-id for account %s."
-                     % id.account_name)
+        clone = message.get("clone-of")
+        if clone is None:
+            logging.info("Client has unknown secure-id for account %s."
+                         % id.account_name)
+        else:
+            logging.info("Client is clone of computer %s" % clone)
+            # Set a new computer title so when a registration request will be
+            # made, the pending computer UI will indicate that this is a clone
+            # of another computer. There's no need to persist the changes since
+            # a new registration will be requested immediately.
+            if clone == self._config.computer_title:
+                title = "%s (clone)" % self._config.computer_title
+            else:
+                title = "%s (clone of %s)" % (self._config.computer_title,
+                                              clone)
+            self._config.computer_title = title
         id.secure_id = None
         id.insecure_id = None
 

=== modified file 'landscape/broker/store.py'
--- landscape/broker/store.py	2013-04-03 08:57:30 +0000
+++ landscape/broker/store.py	2013-04-11 13:21:20 +0000
@@ -200,6 +200,14 @@
         """Change the known UUID from the server we're communicating to."""
         self._persist.set("server_uuid", uuid)
 
+    def get_exchange_token(self):
+        """Get the authentication token to use for the next exchange."""
+        return self._persist.get("exchange_token")
+
+    def set_exchange_token(self, token):
+        """Set the authentication token to use for the next exchange."""
+        self._persist.set("exchange_token", token)
+
     def get_pending_offset(self):
         """Get the current pending offset."""
         return self._persist.get("pending_offset", 0)

=== modified file 'landscape/broker/tests/test_exchange.py'
--- landscape/broker/tests/test_exchange.py	2013-03-31 22:44:46 +0000
+++ landscape/broker/tests/test_exchange.py	2013-04-11 13:21:20 +0000
@@ -242,7 +242,7 @@
         self.mstore.add({"type": "data", "data": 3})
 
         # next one, server will respond with 1!
-        def desynched_send_data(payload, computer_id=None, message_api=None):
+        def desynched_send_data(*args, **kwargs):
             self.transport.next_expected_sequence = 1
             return {"next-expected-sequence": 1}
 
@@ -381,7 +381,7 @@
         self.mstore.set_accepted_types(["data"])
         self.mstore.add({"type": "data", "data": 0})
 
-        def desynched_send_data(payload, computer_id=None, message_api=None):
+        def desynched_send_data(*args, **kwargs):
             self.transport.next_expected_sequence = 0
             return {"next-expected-sequence": 0}
 
@@ -476,6 +476,33 @@
         self.assertEqual(payload.get("server-api"), "2.0")
         self.assertEqual(self.transport.message_api, "2.0")
 
+    def test_exchange_token(self):
+        """
+        When sending messages to the server, the exchanger provides the
+        token that the server itself gave it during the former exchange.
+        """
+        self.exchanger.exchange()
+        self.assertIs(None, self.transport.exchange_token)
+        exchange_token = self.mstore.get_exchange_token()
+        self.assertIsNot(None, exchange_token)
+        self.exchanger.exchange()
+        self.assertEqual(exchange_token, self.transport.exchange_token)
+
+    def test_reset_exchange_token_on_failure(self):
+        """
+        If an exchange fails we set the value of the next exchange token to
+        C{None}, so we can authenticate ourselves even if we couldn't receive
+        a valid token.
+        """
+        self.mstore.set_exchange_token("abcd-efgh")
+        self.mstore.commit()
+        self.transport.exchange = lambda *args, **kwargs: None
+        self.exchanger.exchange()
+        # Check that the change was persisted
+        persist = Persist(filename=self.persist_filename)
+        store = MessageStore(persist, self.config.message_store_path)
+        self.assertIs(None, store.get_exchange_token())
+
     def test_include_total_messages_none(self):
         """
         The payload includes the total number of messages that the client has
@@ -611,7 +638,7 @@
         exchanger raises an exception.
         """
 
-        def failed_send_data(payload, computer_id=None, message_api=None):
+        def failed_send_data(*args, **kwargs):
             return None
 
         self.transport.exchange = failed_send_data

=== modified file 'landscape/broker/tests/test_registration.py'
--- landscape/broker/tests/test_registration.py	2013-01-17 08:52:48 +0000
+++ landscape/broker/tests/test_registration.py	2013-04-11 13:21:20 +0000
@@ -113,6 +113,18 @@
         self.assertEqual(self.identity.secure_id, None)
         self.assertEqual(self.identity.insecure_id, None)
 
+    def test_unknown_id_with_clone(self):
+        """
+        If the server reports us that we are a clone of another computer, then
+        set our computer's title accordingly.
+        """
+        self.config.computer_title = "Wu"
+        self.mstore.set_accepted_types(["register"])
+        self.exchanger.handle_message({"type": "unknown-id", "clone-of": "Wu"})
+        self.assertEqual("Wu (clone)", self.config.computer_title)
+        self.assertIn("Client is clone of computer Wu",
+                      self.logfile.getvalue())
+
     def test_should_register(self):
         self.mstore.set_accepted_types(["register"])
         self.config.computer_title = "Computer Title"

=== modified file 'landscape/broker/tests/test_store.py'
--- landscape/broker/tests/test_store.py	2012-03-07 08:28:31 +0000
+++ landscape/broker/tests/test_store.py	2013-04-11 13:21:20 +0000
@@ -71,6 +71,19 @@
         store = self.create_store()
         self.assertEqual(store.get_server_uuid(), "abcd-efgh")
 
+    def test_get_set_exchange_token(self):
+        """
+        The next-exchange-token value can be persisted and retrieved.
+        """
+        self.assertEqual(self.store.get_exchange_token(), None)
+        self.store.set_exchange_token("abcd-efgh")
+        self.assertEqual(self.store.get_exchange_token(), "abcd-efgh")
+
+        # Ensure it's actually saved.
+        self.store.commit()
+        store = self.create_store()
+        self.assertEqual(store.get_exchange_token(), "abcd-efgh")
+
     def test_get_pending_offset(self):
         self.assertEqual(self.store.get_pending_offset(), 0)
         self.store.set_pending_offset(3)

=== modified file 'landscape/broker/tests/test_transport.py'
--- landscape/broker/tests/test_transport.py	2012-03-07 08:25:39 +0000
+++ landscape/broker/tests/test_transport.py	2013-04-11 13:21:20 +0000
@@ -79,11 +79,13 @@
         transport = HTTPTransport(
             None, "http://localhost:%d/" % (port.getHost().port,))
         result = deferToThread(transport.exchange, "HI", computer_id="34",
-                               message_api="X.Y")
+                               exchange_token="abcd-efgh", message_api="X.Y")
 
         def got_result(ignored):
             self.assertEqual(r.request.received_headers["x-computer-id"],
                              "34")
+            self.assertEqual(r.request.received_headers["x-exchange-token"],
+                             "abcd-efgh")
             self.assertEqual(r.request.received_headers["user-agent"],
                              "landscape-client/%s" % (VERSION,))
             self.assertEqual(r.request.received_headers["x-message-api"],

=== modified file 'landscape/broker/transport.py'
--- landscape/broker/transport.py	2012-03-07 08:25:39 +0000
+++ landscape/broker/transport.py	2013-04-11 13:21:20 +0000
@@ -2,6 +2,7 @@
 import time
 import logging
 import pprint
+import uuid
 
 import pycurl
 
@@ -31,22 +32,28 @@
         """Set the URL of the remote message system."""
         self._url = url
 
-    def _curl(self, payload, computer_id, message_api):
+    def _curl(self, payload, computer_id, exchange_token, message_api):
         headers = {"X-Message-API": message_api,
                    "User-Agent": "landscape-client/%s" % VERSION,
                    "Content-Type": "application/octet-stream"}
         if computer_id:
             headers["X-Computer-ID"] = computer_id
+        if exchange_token:
+            headers["X-Exchange-Token"] = str(exchange_token)
         curl = pycurl.Curl()
         return (curl, fetch(self._url, post=True, data=payload,
                             headers=headers, cainfo=self._pubkey, curl=curl))
 
-    def exchange(self, payload, computer_id=None, message_api=SERVER_API):
+    def exchange(self, payload, computer_id=None, exchange_token=None,
+                 message_api=SERVER_API):
         """Exchange message data with the server.
 
         @param payload: The object to send, it must be L{bpickle}-compatible.
         @param computer_id: The computer ID to send the message as (see
             also L{Identity}).
+        @param exchange_token: The token that the server has given us at the
+            last exchange. It's used to prove that we are still the same
+            client.
 
         @type: C{dict}
         @return: The server's response to sent message or C{None} in case
@@ -60,7 +67,8 @@
             start_time = time.time()
             if logging.getLogger().getEffectiveLevel() <= logging.DEBUG:
                 logging.debug("Sending payload:\n%s", pprint.pformat(payload))
-            curly, data = self._curl(spayload, computer_id, message_api)
+            curly, data = self._curl(spayload, computer_id, exchange_token,
+                                     message_api)
             logging.info("Sent %d bytes and received %d bytes in %s.",
                          len(spayload), len(data),
                          format_delta(time.time() - start_time))
@@ -95,6 +103,7 @@
         self._current_response = 0
         self.next_expected_sequence = 0
         self.computer_id = None
+        self.exchange_token = None
         self.message_api = None
         self.extra = {}
         self._url = url
@@ -106,9 +115,11 @@
     def set_url(self, url):
         self._url = url
 
-    def exchange(self, payload, computer_id=None, message_api=SERVER_API):
+    def exchange(self, payload, computer_id=None, exchange_token=None,
+                 message_api=SERVER_API):
         self.payloads.append(payload)
         self.computer_id = computer_id
+        self.exchange_token = exchange_token
         self.message_api = message_api
         self.next_expected_sequence += len(payload.get("messages", []))
 
@@ -119,6 +130,7 @@
             response = []
 
         result = {"next-expected-sequence": self.next_expected_sequence,
+                  "next-exchange-token": unicode(uuid.uuid4()),
                   "messages": response}
         result.update(self.extra)
         return result