Re-enables unattended upgrades to AppArmor profiles #4167
Conversation
There was a problem hiding this comment.
Went through the test plan, functional upgrade testing LGTM
- Build debs on this branch
- Run
make upgrade-startto provision 0.11.1 VMs - Configure user accounts and set a custom logo
- Perform Trusty 0.11.1 -> 0.12.0~rc upgrade via
make upgrade-test-local - Confirm AppArmor profile is updated correctly
- Confirm
/etc/apparmor.d/usr.sbin.apache2.dpkg-distdoes not exist - Confirm Xenial upgrade message displays on Journalist Interface
- Confirm custom logo is still present
There is also another way to resolve this issue, in the packaging logic itself rather than via postinst, see https://github.com/freedomofpress/securedrop/compare/4161-override-conffiles
Let's discuss this in the engineering meeting today, to ensure we are using the most simple and maintainable solution.
conorsch
force-pushed
the
4161-force-apparmor-profiles-in-app-code-deb-package
branch
from
ec6779f
to
c4b679f
Compare
|
Updated the implementation with the far simpler approach advocated by @emkll, as discussed in engineering meeting earlier today. The diff is far more concise now, and the regression tests should prevent similar problems in the future. Given that @emkll and I have already implicitly signed off on these changes, requesting full review from @kushaldas, according to test plan above. |
There was a problem hiding this comment.
Test result
- Build debs on this branch
- Run
make upgrade-startto provision 0.11.1 VMs - Configure user accounts and set a custom logo
- Perform Trusty 0.11.1 -> 0.12.0~rc upgrade via
make upgrade-test-local - Confirm AppArmor profile is updated correctly
- Confirm
/etc/apparmor.d/usr.sbin.apache2.dpkg-distdoes not exist - Confirm Xenial upgrade message displays on Journalist Interface
- Confirm custom logo is still present
Works as intended. Approved 🎉 🏊♀️ 🏊♂️
We will need someone with enough CI experience to tell what is wrong below.
The recent migration to debhelper caused the AppArmor profiles to be considered conffiles, which breaks our ability to ship unattended updates to those files. We want the AppArmor profiles to be exactly as we configure them, including any updates over time, so we cannot permit the conffiles classification. This test ensures that the `securedrop-app-code` deb package contains only the explicitly whitelisted conffile of the default logo location. Any additional conffiles, automatically classified via debhelper or otherwise, will cause the test to fail, to guard against regressions.
The aa-complain logic paths were off by default, and are left over from the days when we did not enforce AppArmor on staging VMs. We've since switched to enforcing AppArmor profiles by default in staging, to catch problems earlier in the development and testing cycle.
At compat level>=3, debhelper will automatically add any file in /etc/ in a package as a conffile. Since the securedrop-app-code package ships AppArmor profiles in /etc/ that we want to squash at every install, they must not be specified as conffiles. We can override dh_installdeb to substitute the automatically generated conffile with the one we create.
emkll
force-pushed
the
4161-force-apparmor-profiles-in-app-code-deb-package
branch
from
c4b679f
to
dd6b14d
Compare
|
It seems like a branch was forced pushed to this branch and is not found by circle: I've rebased on lastest develop and the code seems to be checking out as expected. You review was dismissed as a result, @kushaldas . Could you please re-review/restamp? |
emkll
deleted the
4161-force-apparmor-profiles-in-app-code-deb-package
branch
Status
Ready for review.
Description of Changes
Fixes #4161.
Changes proposed in this pull request:
conffilesclassification of AppArmor profiles insecuredrop-app-codedeb packageaa-complainlogic from AnsibleTesting
make upgrade-startto provision 0.11.1 VMsmake upgrade-test-local/etc/apparmor.d/usr.sbin.apache2.dpkg-distdoes not existNote that I have not actually tested these changes end-to-end myself yet, so feel free to append fixes as part of review.
Deployment
Certainly! This change is designed to ensure proper updates, given the recent packaging logic changes in #4080.
Checklist
If you made changes to the server application code:
make ci-lint) and tests (make -C securedrop test) pass in the development containerIf you made changes to
securedrop-admin:make -C admin test) pass in the admin development containerIf you made changes to the system configuration:
If you made non-trivial code changes:
If you made changes to documentation:
make docs-lint) passed locally