After installing clicks to /custom/click, /usr/share/click/preinstalled version are still preferred

This is quite an interesting problem. I think I've tracked it down to Hook.install_link, which calls DB.get_path to work out where a package lives so that it can set the symlink to point to the right place, and that walks the database from the bottom up. On one level this is clearly wrong; but we have to be careful here, particularly in the area of multi-user support. Consider this situation:

  Initial state:
    /usr/share/click/preinstalled: org.example.foo 1.0, --all-users
    /opt/click.ubuntu.com: org.example.foo 1.1, --user=cjwatson

    This is clear: user cjwatson has upgraded that package from the store, but we are to leave the package at version 1.0 for all other users. There's no conflict as we can simply run system hooks for both org.example.foo 1.0 and org.example.foo 1.1, creating AppArmor profiles for both.

  After system image upgrade:
    /usr/share/click/preinstalled: org.example.foo 1.1, --all-users
    /opt/click.ubuntu.com: org.example.foo 1.1, --user=cjwatson

    Now what do we do? If user cjwatson runs org.example.foo then they will get the version in /opt/click.ubuntu.com, but user cwayne will get the version in /usr/share/click/preinstalled. It's not possible to simultaneously generate both AppArmor profiles; we have to pick one.

I think the proper answer here is probably to resolve versions before running system hooks. In general only the topmost database layer is available for per-user registrations, so it should always be possible to find a topmost path that is common to all users. We can then automatically unregister any conflicting versions above that and use the topmost path for system hooks.

I've made some progress on this. It is all however excruciatingly delicate, and this is not the first time that I have had to fix bugs of this general nature. I'm beginning to wonder if I should be taking a different approach. The whole problem here is that system hooks may only be attached to by a single instance of any given package version; this leads to the requirement that the AppArmor profile must point to just the right instance, and we often run into trouble (of the "launching application results in blank screen" kind) when trying to move packages around between databases in various ways. Even with regression tests, I'm worried that if I whack this mole it's still going to pop up somewhere else. It's not clear that all the requirements are truly soluble given the current design.

A more comprehensive answer to this category of problems, then, would involve a slight tweak to the design of system hooks, though fortunately an entirely backward-compatible one:

 * We add a new substitution to the hook Pattern field that expresses which database a package is in (the practicalities seem to suggest that a mangled version of its path would be easiest, so ${db-path} expanding to "usr_share_click_preinstalled" etc.).
 * When this substitution is present, "click hook" would start running system hooks for all instances of every package version.
 * click-apparmor could then add that to its .hook file once it's ready, perhaps becoming "Pattern: /var/lib/apparmor/clicks/${db-path}/${id}.json", or a flat structure if that's easier. It would need to adjust its handling of /var/lib/apparmor/{clicks,profiles}/ to cope with this, of course, and the first system image upgrade with the new version of click might have to rebuild all profiles on startup unless it's very clever, but that's comparatively minor.
 * Now that profiles depend on the database path, aa-exec-click would need to change its lookup to depend on the click database containing the requested executable. Some care would be needed to make sure this doesn't slow down application startup and make Ricardo cry; ubuntu-app-launch's desktop hook could perhaps help with this, although that may not be necessary.

The result of this would be much easier to deal with in click, and would be free of corner cases where there isn't obviously a single correct answer.

This is a long reply, but stick with me until the end-- it should be worth it. :)

This would complicate things a bit in the following ways (which may reinforce some of your points but listing them for the alternate perspective if nothing else). This bit I am most concerned about is that we have multiple profiles for a particular app version since each profile will have a different CLICK_DIR. Eg, from comment #2, cjwatson may end up with version 1.1 in /opt/click.ubuntu.com and everyone else 1.1 in /usr/share/click/preinstalled. This means:

1. we have extra profiles to compile which will take more time when compiles are required. On the one hand, we can expect this number to be relatively low, but I've heard custom tarballs having as many as 30 or more clicks. Compiling profiles takes time, and while there are some additional gains to be made, we are likely going to focus on precompiling profiles in the short-medium term (see below)

2. these extra profiles need to be loaded into the kernel. With the numbers we are talking about, we probably won't be over 3M for these extra profiles unless the device has an extreme number of preinstalled/custom clicks, but I thought the point worth making (profiles are typically (far) under 50K each). It also takes time to load profiles into the kernel, but most of this pain should be addressed by recent changes to profile loading (we can load ~40 cached profiles per second on mako, for example). With the numbers we are talking about, this shouldn't be more than 2 seconds.

3. precompiling profiles is affected, but not appreciably more complicated. Right now we precompile profiles for preinstalled apps and custom apps. Preinstalled apps have the cache files in the rootfs and custom in the custom tarball. Scripts and boot jobs would have to be updated for both, but there is a 1 to 1 mapping (ie, custom always deals with custom and preinstalled with rootfs) so this change should be easy enough

4. app launch can be complicated since it would have to figure out which profile to use. This could probably be done without startup impact

5. developers are familiar with current filenames and the APP_ID concept. Adding complexity based on db path could confuse existing users and increase the learning curve for new users.

6. Most importantly, the APP_ID concept becomes considerably more complicated. Right now, the APP_ID flows though click, apparmor, lifecycle, and trusted helpers. Click currently works with click-apparmor to generate profile names that are the APP_ID. AppArmor doesn't care about the filenames of the profiles, and uses the APP_ID as the profile name. App launch (UAL and aa-exec-click, the latter not used in unity8) uses the APP_ID to change profile (application lifecycle also keeps track of the APP_ID for tracking processes of course, though I don't think it looks at the label of the process to do this). Trusted helpers will look at the AppArmor profile name to make decisions (eg trust-store lookups, online accounts access, etc). Right now, the APP_ID is a thread that all of these disparate parts can trust. The current proposal would necessitate the profile name to change to differentiate between pro...

OK. I'm convinced by your arguments about app IDs; I didn't realise
that the profile name needed to be known outside just aa-exec-click
(which does have the application path in hand as well as the app ID).

I like your proposal of putting all the possible database names in
CLICK_DIR. Would it be reasonable to just add all the valid click
databases there, regardless of whether a given package happens to be
unpacked there at the moment? In that case, the necessary API for that
already exists, albeit a little obscurely:

  >>> from gi.repository import Click
  >>> db = Click.DB()
  >>> db.read()
  >>> [db.get(i).props.root for i in range(db.props.size)]
  ['/usr/share/click/preinstalled', '/custom/click', '/opt/click.ubuntu.com']

It might then be unnecessary to change click at all, which is obviously
fantastic from my point of view. ;-) But this does genuinely seem like
a better technical solution, and would let us stop playing whack-a-mole
with the precise ordering of various database operations. Given that
click-apparmor is the only system-level hook in the phone stack right
now, we could mandate this kind of thing as required behaviour for
future system-level hooks.

This will need an apparmor upstart job adjustment as will to regenerate policy on click-apparmor upgrades.

This bug was fixed in the package click-apparmor - 0.2.10

---------------
click-apparmor (0.2.10) utopic; urgency=medium

  * also remove cache files on uninstall (LP: #1375938)
  * obtain all available click database names from click and use all of them
    in CLICK_DIR. This ensures that the profile can be used regardless of
    where the click app installation directory is (LP: #1371574)
  * debian/postinst: update the cached .md5sums file on upgrade to avoid
    running on install and then again on first boot after upgrade. This change
    only affects apt upgrades and not system-image upgrades since system-image
    upgrades always use the existing .md5sums if they exist (see
    /etc/system-image/writable-paths).
 -- Jamie Strandboge <email address hidden> Mon, 06 Oct 2014 16:01:26 -0500

This bug was fixed in the package click-apparmor - 0.2.11.1

---------------
click-apparmor (0.2.11.1) utopic; urgency=medium

  * aa-clickhook: don't remove the lock file so we can properly handle 3 or
    more processes contending for the lock

click-apparmor (0.2.11) utopic; urgency=medium

  * apparmor/click.py: be more careful with out_fn assignment in
    output_policy()
  * aa-clickhook: implement blocking lockfile to make sure this script does
    not run concurrently with itself (LP: #1377338)
 -- Jamie Strandboge <email address hidden> Tue, 07 Oct 2014 09:32:53 -0500

This bug was fixed in the package apparmor - 2.8.96~2652-0ubuntu5.1

---------------
apparmor (2.8.96~2652-0ubuntu5.1) 14.09; urgency=medium

  * debian/apparmor.{upstart,init}: check if click-apparmor md5sums changed so
    we regenerate the policy if it changes too (LP: #1371574)
  * debian/lib/apparmor/functions: fall back to using -n1 if the parser failed
    to load a profile set. This should be removed when the parser properly
    handles profile sets with corrupted profiles (LP: #1377338).
 -- Jamie Strandboge <email address hidden> Tue, 07 Oct 2014 09:24:45 -0500

Reducing the utopic apparmor task to 'Medium' since it makes it show up on the rtm report even though it is fixed in rtm. It will be fixed in the next utopic upload (this week)

This bug was fixed in the package apparmor - 2.8.98-0ubuntu2

---------------
apparmor (2.8.98-0ubuntu2) utopic; urgency=medium

  * Updated to apparmor 2.9.beta4 (aka apparmor 2.8.98)
    - fix logparsing memory leak (LP: #1340927)
    - incorporate fixes to regression testsuite to compensate for
      af_unix mediation, as well as extend test coverage
      (LP: #1375403, LP: #1375516)
    - fix libapparmor's log parsing code to accept additional rejection
      types (LP: #1375413)
    - fix X abstraction for changed lightdm xauthority file locations
      (LP: #1339727)
    - parser: disable downgrade and not enforced rule messages
      by default
    - fix error when using regex profile names in IPC rules
      (LP: #1373085)
    - updates and fixes to the python utilities
    - translation updates

  [ Steve Beattie ]
  * Removed upstreamed patches:
    drop-peer_addr-with-local-addr-in-base.patch,
    update_socketpair_tests_for_af_unix.patch,
    fix_socketpair_tests.patch, sanitized-helpers-updates.patch,
    01-tests-unix_socket_lists.patch,
    02-tests-accept_unix_rules_in_mkprofile.patch,
    03-tests-unix_sockets_v7_pathnames.patch,
    04-tests-migrate_from_poll_to_sockio_timeout.patch,
    05-tests-add_abstract_socket_tests.patch,
    06-tests-use_socketpair_and_none.patch,
    07-parser-fix_local_perms.patch,
    08-phpsysinfo-policy-updates.patch,
    09-apache2-policy-instructions.patch,
    10-lp1371771.patch, 11-lp1371765.patch,
    lp1169881.patch
  * refreshed etc-writable.patch and libapparmor-layout-deb.patch
  * debian/control: add breaks on python3-apparmor against older
    apparmor-utils that used to be where python bits lived
    (LP: #1373259)
  * debian/apport/source_apparmor.py:
   - fixes the apparmor apport hook so it does not raise an exception if
     a non-unicode character is found in /var/log/kern.log or in
     /var/log/syslog. This should work under python3 or python2.7
     (LP: #1304447)
   - adjusts the add_info() function to take the expected additional ui
     argument, though it has no need for it.
   - converts the log parsing code to use with statements so as not to
     leak open file descriptors
   - updates the set of packages to query to see if installed and if so,
     report the version of.
   - adjust import to make pyflakes job easier
   - minor pep8 cleanups

  [ Jamie Strandboge ]
  * add-chromium-browser.patch: don't allow writing to the oom score and
    adjust files since this allows chromium to change the values for any
    process matching our UID
  * debian/apparmor.upstart: check if click-apparmor md5sums changed so we
    regenerate the policy if it changes too (LP: #1371574)
  * debian/apparmor.init: make corresponding upstart change to initscript
  * debian/lib/apparmor/functions: fall back to using -n1 if the parser failed
    to load a profile set. This should be removed when the parser properly
    handles profile sets with corrupted profiles (LP: 1377338)
  * debian/control: fix typo (LP: #1187447)
 -- Steve Beattie <email address hidden> Thu, 09 Oct 2014 22:39:32 -0700