[0.12.0] AppArmor deny messages for lsb-release on upgrade

[emkll]

Description

There are AppArmor deny messages when a user navigates to the journalist interface when upgrading to 0.12.0-rc2. from 0.11.1

Steps to Reproduce

1. Upgrade to 0.12.0-rc2 from 0.11.1
2. Navigate to the journalist interface
3. Observe AppArmor denies in /var/log/kern.log:

Feb 20 16:50:31 app-prod kernel: [ 7236.252571] audit: type=1400 audit(1550681431.003:49): apparmor="DENIED" operation="open" profile="/usr/sbin/apache2" name="/etc/" pid=5359 comm="apache2" requested_mask="r" denied_mask="r" fsuid=33 ouid=0

Expected Behavior

AppArmor profile should allow Apache to read the current release

Actual Behavior

AppArmor profile denies read access to the file

Comments

It appears that securedrop-app-code package does not overwrite the existing apache2 configuration, as there are two configs in /etc/apparmor.d/:

vagrant@app-prod:~$ ls -lah /etc/apparmor.d/usr.sbin.apache2*
-rw-r--r-- 1 root root 14K Feb 20 14:48 /etc/apparmor.d/usr.sbin.apache2
-rw-r--r-- 1 root root 15K Feb 18 19:35 /etc/apparmor.d/usr.sbin.apache2.dpkg-dist

This is likely due to Dpkg::Options::=--force-confold in /etc/cron-apt/action.d/5-security

[redshiftzero]

The most conservative choice here seems to be to move the .dpkg-dist Apparmor profile into place in postinst of securedrop-app-code (only when needed), what do you think?

[emkll]

Had a brief conversation with @conorsch about why this happened now (since we've historically updated AppArmor profiles successfully this way in the past, using this same package).
The reason this happened just now is an unintended side effect introduced by #4080, where the AppArmor profiles in the securedrop-app-code packages are now labelled conffiles by the deb package/apt

0.12.0-rc2:

$apt-cache show securedrop-app-code=0.12.0-rc2
Package: securedrop-app-code
Status: install ok installed
Priority: optional
Section: web
Installed-Size: 17579
Maintainer: SecureDrop Team <securedrop@freedom.press>
Architecture: amd64
Version: 0.12.0~rc2+trusty
Depends: python-pip, apparmor-utils, gnupg2, haveged, python, secure-delete, sqlite3, apache2-mpm-worker, libapache2-mod-wsgi, libapache2-mod-xsendfile, redis-server, supervisor, securedrop-keyring, securedrop-config, libpython2.7-dev
Conffiles:
 /var/www/securedrop/static/i/logo.png 92443946d5c9e05020a090f97b62d027
 /etc/apparmor.d/usr.sbin.apache2 0e02a6a567f5292ecc2de6ddff9c4c2b
 /etc/apparmor.d/usr.sbin.tor 050bf8cbecb221efd9bbda75548d9909
Description: Packages the SecureDrop application code pip dependencies and apparmor profiles. This package will put the apparmor profiles in enforce mode. This package does use pip to install the pip wheelhouse
Description-md5: 868eba2e50020fe77d1646ef5d461b4e
Homepage: https://securedrop.org

0.11.1:

$ apt-cache show securedrop-app-code=0.11.1
Package: securedrop-app-code
Priority: optional
Section: web
Maintainer: SecureDrop Team <securedrop@freedom.press>
Architecture: amd64
Source: securedrop
Version: 0.11.1
Depends: python-pip, apparmor-utils, gnupg2, haveged, python, secure-delete, sqlite3, apache2-mpm-worker, libapache2-mod-wsgi, libapache2-mod-xsendfile, redis-server, supervisor, securedrop-keyring, securedrop-config, libpython2.7-dev
Filename: pool/main/s/securedrop/securedrop-app-code-0.11.1-amd64.deb
Size: 13574164
MD5sum: 195b7667f0b02e1399457ab5bfad0583
SHA1: 2c3e4a7172c637e3e2848834d144f44bc56c409a
SHA256: 95f54be7670b64f66534c5056485414f4b326850f7f547a398f5f5855f00aac5
SHA512: be080b206a8261962033960cc0c81d2f068366efdd8de5a0392d255b7a53821f27c9afc42ebed0e5bb26fff0f105aae8770ecc83c8b062cefd4a33f4adebce89
Description: Packages the SecureDrop application code pip dependencies and apparmor profiles. This package will put the apparmor profiles in enforce mode. This package does use pip to install the pip wheelhouse
Description-md5: 868eba2e50020fe77d1646ef5d461b4e
Homepage: https://securedrop.org

Since this is a regression/change in behavior introduced by packaging logic, it might be more prudent to modify the build logic to exclude AppArmor profiles from being marked as conffiles so that we are sure that they are overwritten automatically at each update.

[emkll]

This is because we are using compat version > 3 in debhelper (see https://github.com/freedomofpress/securedrop/pull/4080/files#diff-8c5798708e1b04b0140c0e5c2c9a2030R1):

In v3 compatibility mode and higher, all files in the etc/ directory in a package will automatically be flagged as conffiles by this program, so there is no need to list them manually here. [0]

We could perhaps change the compatibility mode or override this behavior in the debian/rules file.
[0] : https://manpages.debian.org/stretch/debhelper/dh_installdeb.1.en.html

[conorsch]

We could perhaps change the compatibility mode

We can't drop below 5, lest we lose the ubstvars support which is the whole reason we migrated to debhelper.

override this behavior in the debian/rules file

That's a possibility, see example approach here (search for "purge the automatically generated DEBIAN/conffiles"). To avoid a regression, we want to ensure that we do not mark any files as conffiles in the securedrop-app-code package, so more tests are certainly warranted here. Given the discrepancy posted above, looks like we may have side-effects on custom logos, as well, so we should be sure to test with custom logos during QA.

Seems like the most straightforward path is to write to a non-etc location like /usr/share/securedrop/ and cp in postinst. Then we're sure never to trigger automagic regarding conffile classification, and the regression tests will provide coverage going forward.

[kushaldas]

The path @conorsch took seems to be one possible path, though I think this is a straight packaging problem.
That way, I like the proposed path @emkll showed in the https://github.com/freedomofpress/securedrop/compare/4161-override-conffiles branch is easier. And, in future when we will have JSON based config files, those will only come from code or from Ansible.

[conorsch]

Ah, yes! @emkll's solution is quite concise, and notably doesn't require yet-more postinst madness. In that respect, it should prove more maintainable over time. As long as we're careful to include regression tests, I'm happy with to go with that solution.