[MIR] online-accounts and friends

libaccounts-glib needs work:
* Should enable tests. Add dbus-test-runner to Build-Depends and get rid of the override_dh_auto_test in debian/rules. I tested this and it seemed to work fine.
* Should use dh_python3 instead of dh_python2 (doesn't seem like it needs anything 2-only anyway).

Bummer about using dbus-glib instead of gdbus, but not a blocker.
Pleased to see a symbols files.

signon needs work:
* Tests are disabled and fail when enabled due to "Could not access Signon Database". Can they be made to run?
* /usr/include/signon-plugins/* should be installed as part of signond-dev.
* What's the story with requiring g++-4.6? I tried dropping that Build-Depend and it still built fine.

Would be nice to use debhelper compat 9.
Starts a daemon, but it is a user-daemon, so shouldn't be a system-wide security concern there. But none-the-less, I'd like a security team member to check this out because it is very sensitive for the user.

Which of these has the dependency on qtwebkit and the bits doing https? Please assign to me.

libsignon-glib needs work:
* Should enable tests. Getting rid of the override_dh_auto_test in debian/rules, gave me 6 failures out of 17. Do we know why those are failing?
* Should use dh_python3 instead of dh_python2 (doesn't seem like it needs anything 2-only anyway).

Bummer about using dbus-glib instead of gdbus, but not a blocker.
Pleased to see a symbols files.
Would be nice to use debhelper compat 9. (same for libaccounts-glib, but I forgot to note it.)

signon:

 * Some of the headers in /usr/include/signon-plugins/ are installed in the signon-plugins-dev package, the ones that aren't shouldn't really be installed at all.
 * I'll work on making the tests work in pbuilder, drop gcc-4.6 and use debhelper 9

libsignon-glib is now fine. Tests, dh_python3, and dh9 were all fixed. Thanks, Ken!

Same for libaccounts-glib.

Reviewed signon-keyring-extension from NEW.

Blockers:
* Tests are disabled but Ken tells me some work is going into mocking gnome-keyring in order to enable the tests, which sounds good.

Nits:
Would be nice to use debhelper compat 9.
Package installs /usr/bin unnecessarily (debian/rule can just rm -rf .../usr/bin instead of rm -f .../usr/bin/keyring-test).
I would prefer to see this using libsecret, but since that barely got uploaded to the archive itself, I understand. But note that libsecret is preferred over libgnome-keyring in GNOME now.
This will likely need a rebuild after signon uses multiarch to put the plugin in the right directory, just FYI.

Reviewed gnome-control-center-signon from NEW.

Blockers:
* Tests are disabled.

Nits:
Would be nice to use debhelper compat 9.
Love the symbols and the --fail-missing.
Bummer about using libdbus-glib, but not a blocker.

signon-keyring-extension NEWed, adding it to the list of packages on the bug

libaccounts-qt is fine once tests are enabled.

signon-ui has a dep on qtwebkit, so assigning to jdstrand.

account-plugins needs some work.

Blockers:
* The python build-dep and dependencies seem like they can be switched to python3? tools/account-console would need to be ported, but that looks like a small job.

Questions:
What's the deal with the twitter secret in debian/rules and configure.ac? I assume that's safe...

Nits:
Should have a watch file.
Debhelper 9 would be nice.
No test suite, but I guess that makes sense. It would all be complicated mocks for the services.

For signon-plugin-oauth2, it looks mostly fine. Love the tests! No blockers, but I do have some questions.

Questions:
Why is libqtwebkit-dev a Build-Dependency? It didn't look like it was used.
Why is there a tests package in debian/control? Seems like that's not something users would want to install, vs just being a part of the build.

Nits:
Maintainer should be "Ubuntu Developers <email address hidden>"
Debhelper 9 would be nice.

Passing off to security team for a quick review of the oauth1 and oauth2 implementations. Not sure if it's needed, but better safe than sorry.

libaccounts-qt tests enabled, should be good now.

signon-plugin-oauth2:

The signon-plugin-oauth2-tests package doesn't seem to be used anywhere yet, but it is something that could be useful for other plugin developers to use in their test suites.

Good catch on the libqtwebkit-dev build depends, that is in fact no longer needed, I removed it.

Updated maintainer, and debhelper 9 as well.

account-plugins:
 * debian/watch
    - added
  * debian/patches/py3.patch
    - ported account-console to python3
  * debian/control, debian/rules
    - python3
  * debian/compat
    - debhelper 9

The twitter secret has to be separated from the source, so the one in the source is just for development.

With latest upload, signon is fine from a packaging point of view. Still assigned to security team for quick pass though.

signon-keyring-extension bug 1035037 to mock keyring access so we can enable the tests

gnome-control-center-signon bug 1035039 to mock keyring and signon-ui access so we can enable the tests

FYI, I just filed bug #1037169 against accounts-plugins. Please get this updated before promoting.

In reference to bug #1037169 those services require http, so marking that bug as invalid.

Ok, ken-vandine is uploading a new accounts-plugins to have flickr use the secure endpoint. account-plugin-sina and account-plugin-sohu are flawed delivering web content over http. These need to be disabled or left in universe.

signon-ui:
 * No CVEs (new project), no initscript/upstart jobs, dbus services, setuid, fscaps usage, sudo/su/pkexec or cron jobs. Hardening options are present.
 * Uses standard C++ 'new' and no uses of dangerous C-style strings/memory operations.
 * has a testsuite with no errors in the build

A very high-level review shows signon-ui is suitable for main except for its use of QtWebKit. While QtWebKit is currently in main, no supported applications in main actively use it and Ubuntu Security is discouraging its use because we cannot properly support two separate webkit libraries in Ubuntu. I have talked to the UOA team via email with options for moving forward.

Can someone from the UOA team comment on how signon-ui's use of QtWebKit will be supported in stable releases of Ubuntu?

Thanks!

Oops! There is a dbus service (mis-pasted that line) which is started on the session bus.

Marking gnome-control-center-signon as Fix Committed, since the tests require a little work and a separate milestoned bug was filed.

Same for signon-keyring-extension.

Review:
 * No CVEs,
 * Hardening options are present. Would be nice to enable PIE.
 * No initscript/upstart jobs, dbus system services, setuid, fscaps usage, sudo/su/pkexec or cron jobs. Has two session services.
 * does use malloc with strcpy, but these are safe. Spot-checking other operations seems fine and it seems to be generally well coded using typical C++ style.
 * This was an interesting find in the code:
./lib/signond/signond.pc:prefix=/home/mardy/tmp/signond
./lib/signond/signond.pc:libdir=/home/mardy/tmp/signond/lib64
 * no significant compiler warnings
 * lintian warnings:
libsignon-plugins-doc_8.41-0ubuntu3_all.deb:
W: libsignon-plugins-doc: embedded-javascript-library usr/share/doc/signon-plugins/html/jquery.js
libsignon-qt-doc_8.41-0ubuntu3_all.deb:
W: libsignon-qt-doc: embedded-javascript-library usr/share/doc/libsignon-qt/html/jquery.js
signond_8.41-0ubuntu3_amd64.deb:
W: signond: binary-without-manpage usr/bin/signonclient
W: signond: binary-without-manpage usr/bin/signond
W: signond: binary-without-manpage usr/bin/signonpluginprocess
signond-doc_8.41-0ubuntu3_all.deb:
W: signond-doc: embedded-javascript-library usr/share/doc/signon/html/jquery.js
signon-plugin-ssotest_8.41-0ubuntu3_amd64.deb:
E: signon-plugin-ssotest: description-synopsis-is-duplicated

signond is running in the same context as the user. This means that any application can request credentials. While I didn't look to see what kind of protections are in place for signond, it doesn't matter-- an attacker able to execute arbitrary code could execute a hacked signond in the same user context and access the data, etc. Since signond contains sensitive information, this should be moved to a separate user with its db in a protected directory or encryption (maemo mentions this: "Server stores credentials in optionally encrypted database. It is running under different user id than applications.") Ideally, signond would also run under apparmor confinement.

The ~/.signon directory is created with curious permissions:
$ ls -ld ~/.signon/
drwxrwx--x 2 jamie jamie 4096 Aug 15 15:55 /home/jamie/.signon/

While the signon.db is created safely:
$ ls -l ~/.signon/
-rw-r----- 1 jamie jamie 26624 Aug 15 15:29 signon.db

For defense in depth, it makes sense to tighten the directory permissions to 0700.

I'm going to give this an ACK for now with the understanding that some changes might have to be performed. This will be discussed outside of this bug.

Review:
 * No CVEs. Hardening options are enabled, but it would be nice to have this compiled with PIE. No initscripts/upstart jobs, dbus services, setuid, use of fscaps, use of sudoe/su/pkexec, or cron jobs
 * coded in typical C++ and looks to have appropriate SSL handling. Spot checking code looks fine.
 * Has testsuite and it is enabled in the build
 * libqtwebkit-dev dependency is thankfully dropped
 * no errors or important warnings in the build
 * is not lintian clean:
signon-plugin-oauth2-tests_0.11-0ubuntu2_amd64.deb:
W: signon-plugin-oauth2-tests: binary-without-manpage usr/bin/oauthclient
W: signon-plugin-oauth2-tests: binary-without-manpage usr/bin/signon-oauth2plugin-tests

ACK

Jamie, about:

> * This was an interesting find in the code:
> ./lib/signond/signond.pc:prefix=/home/mardy/tmp/signond
> ./lib/signond/signond.pc:libdir=/home/mardy/tmp/signond/lib64

this is probably a bug in "make dist"; that file should not be shipped; it's recreated during the build (from the *.pc.in file), so it shouldn't affect the final package. I'll fix that anyways, thanks for spotting it.

About the QtWebkit issue with signon-ui: that's a very much essential dependency. During the last UDS we were informed that there could be a problem with that (not because of security issues, but just because of space in the CD), but then we were reassured that that wouldn't be a problem and a solution would be found. Anyway, we concluded that rewriting the whole application was not feasible at that time -- and even more so now.

Just a quick summary of what was decided about signon-ui's use of QtWebkit: signon-ui will refuse to load any non "https://" URLs; and those URLs which allow the user to navigate away from the OAuth authentication pages will be opened on the default browser.

We currently have just two plugins using "http", Sina and Sohu. Sina also supports "https", so we'll fix that plugin (https://bugs.launchpad.net/bugs/1038871); Sohu doesn't, so unfortunately the plugin will be removed until QtWebkit will be supported by our security team (or Sohu switches to https).

ACK, but please file bugs, tageted at 12.10, for each of:
 * requiring https-only URLs
 * navigating away from the authentication pages causes the url to open in the default browser

Bugs filed, assigned and milestones set:

bug 1039084 Refuse non-https URLs
bug 1039085 navigating away from the authentication pages should open the url in the default browser

Approve signon-ui based on jdstrand's comments.

gnome-control-center-signon promoted

libsignon-glib promoted to main

signon promoted to main

signon-ui promoted to main

signon-keyring-extension promoted to main

libaccounts-qt promoted to main

libaccounts-glib promoted to main

since security and MIR team gave an ack I promoted the component showing up on component mismatch after the empathy upload, thanks everyone

Packaging-wise and maintaining-wise, it's fine. But since it embeds a copy of oauth2, I'm going to pass on to security team for a quick audit.

Blockers:
* Needs to be python3, not python2. Why are people still writing stuff for main in python2?

Nits:
* What's the story with "source='njpatel-UnityLensGDocs-0.1'" in unity-scope-gdocs?
* What's the story with <desktop-entry>unity-2d-shell.desktop</desktop-entry> unity-scope-gdocs.application?
* Should be a bug subscriber

Oh whoops, I didn't see that 0.2 had been uploaded. That resolves the python3 issue. What of the questions? unity-2d-shell and njpatel seem like typos.

The desktop file is harmless, but I fixed it in unity-scope-gdocs=0.2-0ubuntu2

OK, njpatel string is gone in 0.2. The other nit is harmless, but fixed. Seems fine.

unity-scope-gdocs promoted in quantal

I am almost done with my review, but won't finish until tomorrow. In the interest of time, I thought I would comment on what I have so far:
Security review:
 * No CVE history in unity-lens-photos (new) or the embedded oauth2 module. The upstream for python-oauth2 doesn't seem particularly active with no commits since December. That said, python-oauth2 has a comprehensive testsuite that was not embedded in the unity-lens-photos (though, it is not enabled in the build and there is a failing test)
 * no compiled code
 * embeds oauth2.py with looks like a python3 port of python-oauth2. I would much prefer python-oauth2 be updated and promoted so that other projects could utilize this.
 * no privileged commands (sudo/su/pkexec), no /tmp files, no initscripts/upstart jobs, no dbus system services, no setuid, fscaps or use of sudo. no cron jobs
 * no build errors or warnings
 * facebook is using https (good)
 * flickr: should be adjusted to use the secure api like in bug #1037169 for account plugins.
 * these are using python3-httplib2 (good) which should be doing SSL verification by default (see bug #882027)

I can say that things look ok but that I have two conditions so far:
 * flickr is updated to use the secure api
 * use system python-oauth2 instead of embedding. python-oauth2 will need packaging updates for python3, but presumably there are going to be many lenses that build off of the online-accounts work and thus will use oauth2. Having one python library with a testsuite that all of them can use and that the security can support is the best solution.

Regarding embedding oauth2... python-oauth2 is python2 only right now and in universe. The recommeded oauth module is apparently python-oauthlib in main.

I didn't want to go down the route of promoting python-oauth2 and having competing oauth recommendations for other packages. But it wasn't trivial to use python-oauthlib either (python2 only, its oauth2 support is young, and the API is different).

From a security perspective, do you prefer -oauthlib or -oauth2?

python-oauthlib is currently in main, but it is only shipping as py2. It wasn't clear to me that it supported oauth 2, which is of course a requirement for many of the providers. If it can do what we want, then that is preferred (the server team's 'glance' pulls it into main, and that is unlikely to change).

FWIW, I did a diff between __init__.py from python-oauth2 and src/oauth2.py and the differences were not big (only 25 hunks, most changing only one line). I figured this diff could be applied to python-oauth2, adjusting it to make it bi-lingual, then adjust the packaging to have it provide both py2 and py3. It would be good to adjust the tests and run them with both py2 and py3 in the build. All this should of course be sent upstream so we don't have to maintain the delta long term.

IMO, the best solution is to use python-oauthlib (especially if it doesn't need porting to py3). An acceptable solution is to make python-oauth2 bi-lingual and pull it into main. I'd much rather have this one additional package in main than have to support multiple embedded python-oauth2s sprinkled throughout the archive as we build out our online-accounts.

I poked at unity-lens-photos some more:
* Packet analysis showed that clicking on a friend's photo opened a browser window with an http:// url (facebook). This error should be rendered via http. I'm guessing this didn't work right because I didn't have the browser addon.
* A quick look at the DBus session interfaces and things seemed ok

So at this point, no new conditions for the security review. Please address the issues in comment #49.

Jamie and others. I've opened bug 1044447 to continue the MIR (and now, FFe) discussion surrounding unity-lens-photos. The photo lens is a distinct feature from the rest of this, which has already landed.

For now, as I mention in bug 1044447, I'm going down the path of investigating using oauthlib.

@jdstrand

Please see bug 1049946