Bug 59468

Modern versions of both Empathy and KDE-Telepathy implement a SASLAuthentication channel handler[1], which means MC never needs to store (or see) your password. Ideally, Sugar should do the same, one day. I realise that's not something that can be implemented instantly, though.

[1] http://telepathy.freedesktop.org/spec/Channel_Interface_SASL_Authentication.html

The Empathy implementation of SASLAuthentication also uses gnome-keyring, and will automatically migrate accounts' passwords from MC's gnome-keyring "schema" to its own. Until recently, a MC bug broke that migration (Bug #42088) which is why MC now has code to recover from that bug.

Eventually, Mission Control will stop storing passwords in gnome-keyring, and just store them in clear-text (if told to store them at all). This is not really a security regression, given that a password stored in MC can be retrieved from it over D-Bus by getting the value of the Parameters property...

However, at the moment its behaviour is inconsistent:

* if asked to store a password (by putting {"password": "s3kr1t"} in Parameters), it will store it in gnome-keyring;
* but on subsequent startups, it will behave as if that password had not been stored in gnome-keyring.

You could try having it only ignore the stored password if Empathy's automatic migration has taken place, like this:

 if (empathy_ok == GNOME_KEYRING_RESULT_OK &&
     empathy_items != NULL)
   {
     KeyringSetData *ksd = g_slice_new0 (KeyringSetData);

     DEBUG ("An Empathy 3.0 password migration wasn't finished "
           "due to fd.o #42088. Finishing it now by deleting the "
            "password for %s", account);

     ksd->account = g_strdup (account);
     ksd->name = g_strdup ("password");
     ksd->set = FALSE;

     gnome_keyring_delete_password (&keyring_schema,
                                    _keyring_set_cb, ksd, NULL,
                                    "account", account,
                                    "param", "password",
                                    NULL);
+
+    /* behave as if it had already been deleted, i.e. we never
+     * actually found it... */
+    param = NULL;
+    value = NULL;
   }

 gnome_keyring_found_list_free (empathy_items);

-/* behave as if it had already been deleted, i.e. we never
- * actually found it... */
-param = NULL;
-value = NULL;

A regression test would also be very welcome.

Created attachment 73150 [details]
default.keyring with secret

This is the default keyring that is stored on the device. It contains the password fine.

(In reply to comment #0)
> I did downgrade to telepathy-gabble-0.16, telepathy-mission-control-5.12,
> telepathy-glib-0.18 and the issue was gone.

You must have been using an outdated version of telepathy-mission-control from the 5.12 branch - 5.12.3 has the same code for this as 5.14.x. I think the same change ought to work on that branch too.

Thanks Simon for the quick reply!

(In reply to comment #3)
> (In reply to comment #0)
> > I did downgrade to telepathy-gabble-0.16, telepathy-mission-control-5.12,
> > telepathy-glib-0.18 and the issue was gone.
> 
> You must have been using an outdated version of telepathy-mission-control
> from the 5.12 branch - 5.12.3 has the same code for this as 5.14.x. I think
> the same change ought to work on that branch too.

Indeed: telepathy-mission-control-5.12.1 We did froze the repo at one point and did not pull in an update it looks like.

I will try your change and report back.

Created attachment 73160 [details] [review]
proposed patch for 5.12.x

Subject: Only ignore passwords in gnome-keyring if Empathy has copied them

When passwords are stored in the Parameters by MC and no
SASLAuthentication handler is used, e.g. under Sugar, we currently
save passwords into gnome-keyring, if enabled at compile time; so, we
want to read them back out of gnome-keyring.

This conflicts slightly with wanting to ignore passwords in gnome-keyring
that are left over from an Empathy 3.0 schema migration that failed
due to fd.o #42088. To avoid failure to authenticate/re-prompt if the
password has been changed on the service side, ignore passwords that
are going to be deleted; but to avoid breaking Sugar, don't ignore
any other passwords we might find in gnome-keyring.

---

This passes regression tests in the 5.12 branch. It should merge nicely into 5.14 and master, but I haven't tried that yet.

Awesome, just tested with 5.14 and things worked out fine for me. Thanks for acting on it so quickly!

Just had another positive feedback with the 5.14 fix from another colleague with a school server.

Do you guys plan a bug fix release soon? Otherwise I could add the patch to the Fedora 18 rpms in the meantime so Sugar is covered there.

Also confirmed to pass tests on 5.14 here; code review required before it can be merged.

Thanks for being responsive to us here. Who can we pester for a code review so that this goes upstream? :)

We have got four independent positive test results. And the code as I can judge it looks good to me. Who can help us with the final review so we can get that fix finally in? Would be great to cross this one from the 'open' list :) Thanks in advance.

Ping, I guess this did not find it's way in yet?

Cc other Telepathy reviewers; any chance of some review here? I'd review it myself, but, er, I wrote it...

Since we've had several positive test results, I'll merge it anyway at some point, if nobody vetoes it.

Comment on attachment 73160 [details] [review]
proposed patch for 5.12.x

Review of attachment 73160 [details] [review]:
-----------------------------------------------------------------

Looks good to me.

Fixed in git for 5.12.4, 5.14.1 and 5.15.0. I'll try to get some releases out later this week; it's about time we had a 5.15.0.