Re: [sugar] [PATCH] services/presence/: identify Buddies by "key ID"	(pubkey hash), not whole key. FWIW, looks good to me.

Morgan

Simon McVittie wrote:
> I thought I should send this one in for dcbw's approval, since it's a
> behaviour change...
> 
> """
> services/presence/: identify Buddies by "key ID" (pubkey hash), not whole key.
> 
> This allows us to create Buddy objects as soon as we see a contact on the
> server. For contacts not on trusted servers, or seen in anonymous MUCs, we
> create a Buddy identified by JID instead (so we have some way to talk
> about the anonymous contact within the Sugar API).
> 
> The concept of "trusted server" means a server which we trust to validate that
> users with a keyID as the username part of their JID do in fact have that key.
> Currently we just pretend that olpc.collabora.co.uk does this - in future, the
> school servers will do this validation by using key rather than password
> authentication.
> 
> Also create Buddy object paths based on the keyID or JID (for easier debugging).
> """
> 
> This will allow me to improve the Sugar-visible API by mapping from
> Buddies to handles (and back) in a synchronous way, so we can track
> buddies entering and leaving activities in a sane way, for instance. Also, we
> can interoperate with non-OLPC servers correctly (with a Buddy object
> present even for non-OLPC users).
> 
> I'm assuming here that the key-ID is "sufficiently unique" across all
> trusted servers. It's a SHA-1 of the public key, so basically the
> same strength as GnuPG key fingerprints and git object hashes.
> (Actually, for hysterical raisins, it's a SHA-1 of Base64(public_key) -
> we should probably change this before we ship.)
> 
> The actual function used for the key-ID can be changed (if it
> is, old and new versions of Sugar will be incompatible, but that's not
> really a problem yet) so if any crypto gurus want to specify something
> different, now would be a really good time. As currently implemented, its
> output must be short enough to put in the JID (for which a hex SHA-1 is
> somewhat long already).
> 
> ---
>  services/presence/buddy.py           |   49 +++++++++-------
>  services/presence/presenceservice.py |   48 +++++++++------
>  services/presence/pstest.py          |   10 ++-
>  services/presence/server_plugin.py   |  108 ++++++++++++++++++++++++++++++++--
>  4 files changed, 166 insertions(+), 49 deletions(-)
> 
> diff --git a/services/presence/buddy.py b/services/presence/buddy.py
> index 1b45fd5..b858b41 100644
> --- a/services/presence/buddy.py
> +++ b/services/presence/buddy.py
> @@ -37,6 +37,7 @@ _PROP_CURACT = "current-activity"
>  _PROP_COLOR = "color"
>  _PROP_OWNER = "owner"
>  _PROP_VALID = "valid"
> +_PROP_OBJID = 'objid'
>  
>  # Will go away soon
>  _PROP_IP4_ADDRESS = "ip4-address"
> @@ -90,15 +91,14 @@ class Buddy(ExportedGObject):
>      }
>  
>      __gproperties__ = {
> -        _PROP_KEY          : (str, None, None, None,
> -                              gobject.PARAM_READWRITE |
> -                              gobject.PARAM_CONSTRUCT_ONLY),
> +        _PROP_KEY          : (str, None, None, None, gobject.PARAM_READWRITE),
>          _PROP_ICON         : (object, None, None, gobject.PARAM_READWRITE),
>          _PROP_NICK         : (str, None, None, None, gobject.PARAM_READWRITE),
>          _PROP_COLOR        : (str, None, None, None, gobject.PARAM_READWRITE),
>          _PROP_CURACT       : (str, None, None, None, gobject.PARAM_READWRITE),
>          _PROP_VALID        : (bool, None, None, False, gobject.PARAM_READABLE),
>          _PROP_OWNER        : (bool, None, None, False, gobject.PARAM_READABLE),
> +        _PROP_OBJID        : (str, None, None, None, gobject.PARAM_READABLE),
>          _PROP_IP4_ADDRESS  : (str, None, None, None, gobject.PARAM_READWRITE)
>      }
>  
> @@ -106,16 +106,16 @@ class Buddy(ExportedGObject):
>          """Initialize the Buddy object
>  
>          bus -- connection to the D-Bus session bus
> -        object_id -- the activity's unique identifier
> +        object_id -- the buddy's unique identifier, either based on their
> +            key-ID or JID
>          kwargs -- used to initialize the object's properties
>  
>          constructs a DBUS "object path" from the _BUDDY_PATH
>          and object_id
>          """
> -        if not object_id or not isinstance(object_id, int):
> -            raise ValueError("object id must be a valid number")
>  
> -        self._object_path = _BUDDY_PATH + str(object_id)
> +        self._object_id = object_id
> +        self._object_path = dbus.ObjectPath(_BUDDY_PATH + object_id)
>  
>          self._activities = {}   # Activity ID -> Activity
>          self._activity_sigids = {}
> @@ -130,9 +130,6 @@ class Buddy(ExportedGObject):
>          self._color = None
>          self._ip4_address = None
>  
> -        if not kwargs.get(_PROP_KEY):
> -            raise ValueError("key required")
> -
>          _ALLOWED_INIT_PROPS = [_PROP_NICK, _PROP_KEY, _PROP_ICON,
>                                 _PROP_CURACT, _PROP_COLOR, _PROP_IP4_ADDRESS]
>          for (key, value) in kwargs.items():
> @@ -158,7 +155,9 @@ class Buddy(ExportedGObject):
>  
>          pspec -- property specifier with a "name" attribute
>          """
> -        if pspec.name == _PROP_KEY:
> +        if pspec.name == _PROP_OBJID:
> +            return self._object_id
> +        elif pspec.name == _PROP_KEY:
>              return self._key
>          elif pspec.name == _PROP_ICON:
>              return self._icon
> @@ -422,32 +421,40 @@ class Buddy(ExportedGObject):
>          """
>          changed = False
>          changed_props = {}
> -        if _PROP_NICK in properties.keys():
> +        if _PROP_NICK in properties:
>              nick = properties[_PROP_NICK]
>              if nick != self._nick:
>                  self._nick = nick
>                  changed_props[_PROP_NICK] = nick
>                  changed = True
> -        if _PROP_COLOR in properties.keys():
> +        if _PROP_COLOR in properties:
>              color = properties[_PROP_COLOR]
>              if color != self._color:
>                  self._color = color
>                  changed_props[_PROP_COLOR] = color
>                  changed = True
> -        if _PROP_CURACT in properties.keys():
> +        if _PROP_CURACT in properties:
>              curact = properties[_PROP_CURACT]
>              if curact != self._current_activity:
>                  self._current_activity = curact
>                  changed_props[_PROP_CURACT] = curact
>                  changed = True
> -        if _PROP_IP4_ADDRESS in properties.keys():
> +        if _PROP_IP4_ADDRESS in properties:
>              ip4addr = properties[_PROP_IP4_ADDRESS]
>              if ip4addr != self._ip4_address:
>                  self._ip4_address = ip4addr
>                  changed_props[_PROP_IP4_ADDRESS] = ip4addr
>                  changed = True
> -
> -        if not changed or not len(changed_props.keys()):
> +        if _PROP_KEY in properties:
> +            # don't allow key to be set more than once
> +            if self._key is None:
> +                key = properties[_PROP_KEY]
> +                if key is not None:
> +                    self._key = key
> +                    changed_props[_PROP_KEY] = key
> +                    changed = True
> +
> +        if not changed or not changed_props:
>              return
>  
>          # Try emitting PropertyChanged before updating validity
> @@ -558,13 +565,11 @@ class ShellOwner(GenericOwner):
>      _SHELL_OWNER_INTERFACE = "org.laptop.Shell.Owner"
>      _SHELL_PATH = "/org/laptop/Shell"
>  
> -    def __init__(self, ps, bus, object_id, test=False):
> +    def __init__(self, ps, bus):
>          """Initialize the ShellOwner instance
>  
>          ps -- presenceservice.PresenceService object
>          bus -- a connection to the D-Bus session bus
> -        object_id -- the activity's unique identifier
> -        test -- ignored
>  
>          Retrieves initial property values from the profile
>          module.  Loads the buddy icon from file as well.
> @@ -584,8 +589,8 @@ class ShellOwner(GenericOwner):
>          icon = f.read()
>          f.close()
>  
> -        GenericOwner.__init__(self, ps, bus, object_id, key=key,
> -                nick=nick, color=color, icon=icon, server=server,
> +        GenericOwner.__init__(self, ps, bus, psutils.pubkey_to_keyid(key),
> +                key=key, nick=nick, color=color, icon=icon, server=server,
>                  key_hash=key_hash, registered=registered)
>  
>          # Ask to get notifications on Owner object property changes in the
> diff --git a/services/presence/presenceservice.py b/services/presence/presenceservice.py
> index bf058d3..4f84a3b 100644
> --- a/services/presence/presenceservice.py
> +++ b/services/presence/presenceservice.py
> @@ -1,4 +1,5 @@
>  # Copyright (C) 2007, Red Hat, Inc.
> +# Copyright (C) 2007 Collabora Ltd. 
>  #
>  # This program is free software; you can redistribute it and/or modify
>  # it under the terms of the GNU General Public License as published by
> @@ -33,6 +34,7 @@ from sugar import util
>  
>  from buddy import Buddy, ShellOwner
>  from activity import Activity
> +from psutils import pubkey_to_keyid
>  
>  _PRESENCE_SERVICE = "org.laptop.Sugar.Presence"
>  _PRESENCE_INTERFACE = "org.laptop.Sugar.Presence"
> @@ -57,15 +59,17 @@ class PresenceService(ExportedGObject):
>  
>      def _create_owner(self):
>          # Overridden by TestPresenceService
> -        return ShellOwner(self, self._session_bus, self._get_next_object_id())
> +        return ShellOwner(self, self._session_bus)
>  
>      def __init__(self):
>          self._next_object_id = 0
>          self._connected = False
>  
> -        self._buddies = {}      # key -> Buddy
> +        self._buddies = {}              # identifier -> Buddy
> +        self._buddies_by_pubkey = {}    # base64 public key -> Buddy
>          self._handles_buddies = {}      # tp client -> (handle -> Buddy)
> -        self._activities = {}   # activity id -> Activity
> +
> +        self._activities = {}           # activity id -> Activity
>  
>          self._session_bus = dbus.SessionBus()
>          self._session_bus.add_signal_receiver(self._connection_disconnected_cb,
> @@ -74,7 +78,10 @@ class PresenceService(ExportedGObject):
>  
>          # Create the Owner object
>          self._owner = self._create_owner()
> -        self._buddies[self._owner.props.key] = self._owner
> +        key = self._owner.props.key
> +        keyid = pubkey_to_keyid(key)
> +        self._buddies['keyid/' + keyid] = self._owner
> +        self._buddies_by_pubkey[key] = self._owner
>  
>          self._registry = ManagerRegistry()
>          self._registry.LoadManagers()
> @@ -133,31 +140,35 @@ class PresenceService(ExportedGObject):
>          if self._connected != old_status:
>              self.emit('connection-status', self._connected)
>  
> -    def _contact_online(self, tp, handle, props):
> -        new_buddy = False
> -        key = props["key"]
> -        buddy = self._buddies.get(key)
> -        if not buddy:
> +    def get_buddy(self, objid):
> +        buddy = self._buddies.get(objid)
> +        if buddy is None:
> +            _logger.debug('Creating new buddy at .../%s', objid)
>              # we don't know yet this buddy
> -            objid = self._get_next_object_id()
> -            buddy = Buddy(self._session_bus, objid, key=key)
> +            buddy = Buddy(self._session_bus, objid)
>              buddy.connect("validity-changed", self._buddy_validity_changed_cb)
>              buddy.connect("disappeared", self._buddy_disappeared_cb)
> -            self._buddies[key] = buddy
> +            self._buddies[objid] = buddy
> +        return buddy
> +
> +    def _contact_online(self, tp, objid, handle, props):
> +        _logger.debug('Handle %u, .../%s is now online', handle, objid)
> +        buddy = self.get_buddy(objid)
>  
>          self._handles_buddies[tp][handle] = buddy
>          # store the handle of the buddy for this CM
>          buddy.add_telepathy_handle(tp, handle)
> -
>          buddy.set_properties(props)
>  
>      def _buddy_validity_changed_cb(self, buddy, valid):
>          if valid:
>              self.BuddyAppeared(buddy.object_path())
> +            self._buddies_by_pubkey[buddy.props.key] = buddy
>              _logger.debug("New Buddy: %s (%s)", buddy.props.nick,
>                            buddy.props.color)
>          else:
>              self.BuddyDisappeared(buddy.object_path())
> +            self._buddies_by_pubkey.pop(buddy.props.key, None)
>              _logger.debug("Buddy left: %s (%s)", buddy.props.nick,
>                            buddy.props.color)
>  
> @@ -166,16 +177,17 @@ class PresenceService(ExportedGObject):
>              self.BuddyDisappeared(buddy.object_path())
>              _logger.debug('Buddy left: %s (%s)', buddy.props.nick,
>                            buddy.props.color)
> -        self._buddies.pop(buddy.props.key)
> +            self._buddies_by_pubkey.pop(buddy.props.key, None)
> +        self._buddies.pop(buddy.props.objid, None)
>  
>      def _contact_offline(self, tp, handle):
>          if not self._handles_buddies[tp].has_key(handle):
>              return
>  
>          buddy = self._handles_buddies[tp].pop(handle)
> -        key = buddy.props.key
> -
>          # the handle of the buddy for this CM is not valid anymore
> +        # (this might trigger _buddy_disappeared_cb if they are not visible
> +        # via any CM)
>          buddy.remove_telepathy_handle(tp, handle)
>  
>      def _get_next_object_id(self):
> @@ -326,8 +338,8 @@ class PresenceService(ExportedGObject):
>                           in_signature="ay", out_signature="o",
>                           byte_arrays=True)
>      def GetBuddyByPublicKey(self, key):
> -        if self._buddies.has_key(key):
> -            buddy = self._buddies[key]
> +        buddy = self._buddies_by_pubkey.get(key)
> +        if buddy is not None:
>              if buddy.props.valid:
>                  return buddy.object_path()
>          raise NotFoundError("The buddy was not found.")
> diff --git a/services/presence/pstest.py b/services/presence/pstest.py
> index 1900993..3054e48 100644
> --- a/services/presence/pstest.py
> +++ b/services/presence/pstest.py
> @@ -26,6 +26,7 @@ from sugar import env, util
>  
>  from buddy import GenericOwner, _PROP_NICK, _PROP_CURACT, _PROP_COLOR
>  from presenceservice import PresenceService
> +from psutils import pubkey_to_keyid
>  
>  
>  _logger = logging.getLogger('s-p-s.pstest')
> @@ -37,7 +38,7 @@ class TestOwner(GenericOwner):
>  
>      __gtype_name__ = "TestOwner"
>  
> -    def __init__(self, ps, bus, object_id, test_num, randomize):
> +    def __init__(self, ps, bus, test_num, randomize):
>          self._cp = ConfigParser()
>          self._section = "Info"
>          self._test_activities = []
> @@ -62,8 +63,9 @@ class TestOwner(GenericOwner):
>          icon = _get_random_image()
>  
>          _logger.debug("pubkey is %s" % pubkey)
> -        GenericOwner.__init__(self, ps, bus, object_id, key=pubkey, nick=nick,
> -                color=color, icon=icon, registered=registered, key_hash=privkey_hash)
> +        GenericOwner.__init__(self, ps, bus, pubkey_to_keyid(pubkey),
> +                key=pubkey, nick=nick, color=color, icon=icon,
> +                registered=registered, key_hash=privkey_hash)
>  
>          # Only do the random stuff if randomize is true
>          if randomize:
> @@ -169,7 +171,7 @@ class TestPresenceService(PresenceService):
>          PresenceService.__init__(self)
>  
>      def _create_owner(self):
> -        return TestOwner(self, self._session_bus, self._get_next_object_id(),
> +        return TestOwner(self, self._session_bus,
>                           self.__test_num, self.__randomize)
>  
>      def internal_get_activity(self, actid):
> diff --git a/services/presence/server_plugin.py b/services/presence/server_plugin.py
> index 26adba9..b020286 100644
> --- a/services/presence/server_plugin.py
> +++ b/services/presence/server_plugin.py
> @@ -20,6 +20,7 @@
>  import logging
>  import os
>  import sys
> +from string import hexdigits
>  try:
>      # Python >= 2.5
>      from hashlib import md5
> @@ -42,6 +43,7 @@ from telepathy.constants import (HANDLE_TYPE_CONTACT,
>      CONNECTION_STATUS_CONNECTING,
>      CONNECTION_STATUS_REASON_AUTHENTICATION_FAILED,
>      CONNECTION_STATUS_REASON_NONE_SPECIFIED,
> +    CHANNEL_GROUP_FLAG_CHANNEL_SPECIFIC_HANDLES,
>      PROPERTY_FLAG_WRITE)
>  from sugar import util
>  
> @@ -105,8 +107,11 @@ class ServerPlugin(gobject.GObject):
>          'contact-online':
>              # Contact has come online and we've discovered all their buddy
>              # properties.
> -            # args: contact handle: int; dict {name: str => property: object}
> -            (gobject.SIGNAL_RUN_FIRST, None, [object, object]),
> +            # args:
> +            #   contact identification (based on key ID or JID): str
> +            #   contact handle: int or long
> +            #   dict {name: str => property: object}
> +            (gobject.SIGNAL_RUN_FIRST, None, [str, object, object]),
>          'contact-offline':
>              # Contact has gone offline.
>              # args: contact handle
> @@ -263,7 +268,7 @@ class ServerPlugin(gobject.GObject):
>  
>          account_info['server'] = self._owner.get_server()
>  
> -        khash = util.printable_hash(util._sha_data(self._owner.props.key))
> +        khash = psutils.pubkey_to_keyid(self._owner.props.key)
>          account_info['account'] = "%s@%s" % (khash, account_info['server'])
>  
>          account_info['password'] = self._owner.get_key_hash()
> @@ -770,10 +775,13 @@ class ServerPlugin(gobject.GObject):
>              return
>  
>          props['nick'] = aliases[0]
> +
>          jid = self._conn[CONN_INTERFACE].InspectHandles(HANDLE_TYPE_CONTACT,
>                                                          [handle])[0]
>          self._online_contacts[handle] = jid
> -        self.emit("contact-online", handle, props)
> +        objid = self.identify_contacts(None, [handle])[handle]
> +
> +        self.emit("contact-online", objid, handle, props)
>  
>          self._conn[CONN_INTERFACE_BUDDY_INFO].GetActivities(handle,
>              reply_handler=lambda *args: self._contact_online_activities_cb(
> @@ -841,7 +849,7 @@ class ServerPlugin(gobject.GObject):
>                  handle not in self._subscribe_local_pending and
>                  handle not in self._subscribe_remote_pending):
>              # it's probably a channel-specific handle - can't create a Buddy
> -            # object
> +            # object for those yet
>              return
>  
>          self._online_contacts[handle] = None
> @@ -1063,3 +1071,93 @@ class ServerPlugin(gobject.GObject):
>              if room == act_handle:
>                  self.emit("activity-properties-changed", act_id, properties)
>                  return
> +
> +    def _server_is_trusted(self, hostname):
> +        """Return True if the server with the given hostname is trusted to
> +        verify public-key ownership correctly, and only allows users to
> +        register JIDs whose username part is either a public key fingerprint,
> +        or of the wrong form to be a public key fingerprint (to allow for
> +        ejabberd's admin@example.com address).
> +
> +        If we trust the server, we can skip verifying the key ourselves,
> +        which leads to simplifications. In the current implementation we
> +        never verify that people actually own the key they claim to, so
> +        we will always give contacts on untrusted servers a JID- rather than
> +        key-based identity.
> +
> +        For the moment we assume that the test server, olpc.collabora.co.uk,
> +        does this verification.
> +        """
> +        return (hostname == 'olpc.collabora.co.uk')
> +
> +    def identify_contacts(self, tp_chan, handles):
> +        """Work out the "best" unique identifier we can for the given handles,
> +        in the context of the given channel (which may be None), using only
> +        'fast' connection manager API (that does not involve network
> +        round-trips).
> +
> +        For the XMPP server case, we proceed as follows:
> +
> +        * Find the owners of the given handles, if the channel has
> +          channel-specific handles
> +        * If the owner (globally-valid JID) is on a trusted server, return
> +          'keyid/' plus the 'key fingerprint' (the user part of their JID,
> +          currently implemented as the SHA-1 of the Base64 blob in
> +          owner.key.pub)
> +        * If the owner (globally-valid JID) cannot be found or is on an
> +          untrusted server, return 'xmpp/' plus an escaped form of the JID
> +
> +        The idea is that we identify buddies by key-ID (i.e. by key, assuming
> +        no collisions) if we can find it without making network round-trips,
> +        but if that's not possible we just use their JIDs.
> +
> +        :Parameters:
> +            `tp_chan` : telepathy.client.Channel or None
> +                The channel in which the handles were found, or None if they
> +                are known to be channel-specific handles
> +            `handles` : iterable over (int or long)
> +                The contacts' handles in that channel
> +        :Returns:
> +            A dict mapping the provided handles to the best available
> +            unique identifier, which is a string that could be used as a
> +            suffix to an object path
> +        """
> +        # we need to be able to index into handles, so force them to
> +        # be a sequence
> +        if not isinstance(handles, (tuple, list)):
> +            handles = tuple(handles)
> +
> +        owners = handles
> +
> +        if tp_chan is not None and CHANNEL_INTERFACE_GROUP in tp_chan:
> +
> +            group = tp_chan[CHANNEL_INTERFACE_GROUP]
> +            if group.GetFlags() & CHANNEL_GROUP_FLAG_CHANNEL_SPECIFIC_HANDLES:
> +
> +                owners = group.GetHandleOwners(handles)
> +                for i, owner in enumerate(owners):
> +                    if owner == 0:
> +                        owners[i] = handles[i]
> +
> +        jids = self._conn[CONN_INTERFACE].InspectHandles(HANDLE_TYPE_CONTACT,
> +                                                         owners)
> +
> +        ret = {}
> +        for handle, jid in zip(handles, jids):
> +            if '/' in jid:
> +                # the contact is unidentifiable (in an anonymous MUC) - create
> +                # a temporary identity for them, based on their room-JID
> +                ret[handle] = 'xmpp/' + psutils.escape_identifier(jid)
> +            else:
> +                user, host = jid.split('@', 1)
> +                if (self._server_is_trusted(host) and len(user) == 40 and
> +                    user.strip(hexdigits) == ''):
> +                    # they're on a trusted server and their username looks
> +                    # like a key-ID
> +                    ret[handle] = 'keyid/' + user.lower()
> +                else:
> +                    # untrusted server, or not the right format to be a
> +                    # key-ID - identify the contact by their JID
> +                    ret[handle] = 'xmpp/' + psutils.escape_identifier(jid)
> +
> +        return ret
_______________________________________________
Sugar mailing list
Sugar@lists.laptop.org
http://lists.laptop.org/listinfo/sugar

