From 5585979e836fab2b9fa4c4e1c785cc52478eb274 Mon Sep 17 00:00:00 2001 From: Conor Schaefer Date: Wed, 20 Feb 2019 17:10:51 -0800 Subject: [PATCH 1/4] Adds regression test for conffiles in app-code 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. --- .../tests/test_securedrop_deb_package.py | 28 +++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/molecule/builder-trusty/tests/test_securedrop_deb_package.py b/molecule/builder-trusty/tests/test_securedrop_deb_package.py index 0e026d8a67..fbb99e6e0e 100644 --- a/molecule/builder-trusty/tests/test_securedrop_deb_package.py +++ b/molecule/builder-trusty/tests/test_securedrop_deb_package.py @@ -1,6 +1,7 @@ import pytest import os import re +import tempfile SECUREDROP_TARGET_PLATFORM = os.environ.get("SECUREDROP_TARGET_PLATFORM", "trusty") @@ -228,6 +229,33 @@ def test_deb_package_contains_no_generated_assets(host, deb): assert not re.search("^.*css.map$", c.stdout, re.M) +@pytest.mark.parametrize("deb", deb_packages) +def test_deb_package_contains_no_generated_assets(host, deb): + """ + Ensures the `securedrop-app-code` package declares only whitelisted + `conffiles`. Several files in `/etc/` would automatically be marked + conffiles, which would break unattended updates to critical package + functionality such as AppArmor profiles. This test validates overrides + in the build logic to unset those conffiles. + """ + deb_package = host.file(deb.format( + securedrop_test_vars.securedrop_version)) + + # Only relevant for the securedrop-app-code package: + if "securedrop-app-code" in deb_package.path: + tmpdir = tempfile.mkdtemp() + # The `--raw-extract` flag includes `DEBIAN/` dir with control files + host.run("dpkg-deb --raw-extract {} {}".format(deb, tmpdir)) + conffiles_path = os.path.join(tmpdir, "DEBIAN", "conffiles") + f = host.file(conffiles_path) + + assert f.is_file + # Ensure that the entirety of the file lists only the logo as conffile; + # effectively ensures e.g. AppArmor profiles are not conffiles. + conffiles = f.content_string.rstrip() + assert conffiles == "/var/www/securedrop/static/i/logo.png" + + @pytest.mark.parametrize("deb", deb_packages) def test_deb_package_contains_css(host, deb): """ From 1a04ad8d9262ac69790a6d13405114882ac07507 Mon Sep 17 00:00:00 2001 From: Conor Schaefer Date: Wed, 20 Feb 2019 17:46:08 -0800 Subject: [PATCH 2/4] Removes aa-complain logic from Ansible 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. --- .../roles/app-test/defaults/main.yml | 5 -- .../app-test/tasks/apparmor_complain.yml | 49 ------------------- .../roles/app-test/tasks/main.yml | 5 -- 3 files changed, 59 deletions(-) delete mode 100644 install_files/ansible-base/roles/app-test/tasks/apparmor_complain.yml diff --git a/install_files/ansible-base/roles/app-test/defaults/main.yml b/install_files/ansible-base/roles/app-test/defaults/main.yml index 8c4f26ca05..6063562478 100644 --- a/install_files/ansible-base/roles/app-test/defaults/main.yml +++ b/install_files/ansible-base/roles/app-test/defaults/main.yml @@ -1,9 +1,4 @@ --- -# Whether to set the Apache AppArmor profiles to "complain". -# Can aid in debugging the AppArmor profiles, but also permits -# misconfigurations, and so should be used with caution. -securedrop_app_test_apparmor_complain: False - # Username for Apache service, used to set permissions on the # Source Interface config to enable logging in the staging environment. apache_user: www-data diff --git a/install_files/ansible-base/roles/app-test/tasks/apparmor_complain.yml b/install_files/ansible-base/roles/app-test/tasks/apparmor_complain.yml deleted file mode 100644 index b597e7e919..0000000000 --- a/install_files/ansible-base/roles/app-test/tasks/apparmor_complain.yml +++ /dev/null @@ -1,49 +0,0 @@ ---- -# The `aa-status` command does not support machine-readable output, -# so we must parse it with sed. This is a read-only task that will -# display only the profiles listed in "complain" mode. The results -# of this check are used to conditionally set the profiles to complain -# mode only if necessary, to ensure idempotence. - -# Unfortunately the `aa-complain` command does not yield useful output -# sufficient for determining changed state, but rather announces -# "Setting to complain mode" every time, which is inane. - -- name: Check for complaining AppArmor profiles. - # You're probably thinking that no one deserves to read a sed regex like this. - # You're probably correct. Here's a blow-by-blow, to ease the pain: - # - # 1. Call `aa-status` and pipe to sed (thus the use of `shell`) - # 2. Suppress automatic printing of the stream while editing via "--quiet", - # since we'll explictly print desired lines via the `p` command. - # 3. Declare START and STOP patterns, to restrict the scope of the matching. - # Sed calls these patterns "addresses"; see `info sed`. - # 4. Declare a command group via '{}' that applies to matches bounded by - # the addresses previously declared. Reiterate the same patterns and use - # the branch command `b` with no label to stop processing the current cycle. - # 5. Remove all leading whitespace, since `aa-status` indents profile names. - # 6. Finally, print the line and terminate the command group. - shell: > - aa-status | sed --quiet - '/profiles are in complain mode/,/processes have profiles defined/ - {/profiles are in complain mode/b;/processes have profiles defined/b; - s/^\s*//;p}' - register: apparmor_complaining_profiles_result - # Read-only task, so don't report changed. - changed_when: false - # If the filepath to the AppArmor profile does not exist, `aa-complain` will inanely - # exit 0 and spit a message to STDOUT: - # /etc/apparmor.d/usr.sbin.apache2 does not exist, please double-check the path. - # So let's check both the exit code and STDOUT to fail appropriately. - failed_when: apparmor_complaining_profiles_result.rc != 0 or - 'does not exist' in apparmor_complaining_profiles_result.stdout - -- name: Set AppArmor profiles to complain mode. - command: aa-complain /etc/apparmor.d/{{ item }} - # Using multiline YAML syntax ('>') to avoid excessive escaping of quotes in the conditional. - # Using `regex_replace` to substitute newlines because the AppArmor profile names are - # dot-separated, but `aa-status` the executable path in place of the filename. - # Doing it this way keeps the config DRY and will work with future profiles as well. - when: > - "/{{ item | regex_replace('\\.', '/') }}" not in apparmor_complaining_profiles_result.stdout_lines - with_items: "{{ apparmor_profiles }}" diff --git a/install_files/ansible-base/roles/app-test/tasks/main.yml b/install_files/ansible-base/roles/app-test/tasks/main.yml index 2ffb07b95e..a504413372 100644 --- a/install_files/ansible-base/roles/app-test/tasks/main.yml +++ b/install_files/ansible-base/roles/app-test/tasks/main.yml @@ -4,11 +4,6 @@ tags: - apache -- include: apparmor_complain.yml - when: securedrop_app_test_apparmor_complain - tags: - - aa-complain - - include: dev_setup_xvfb_for_functional_tests.yml - include: setup_firefox_for_selenium.yml From dd6b14d3f5e20eb0ba921ccc51da5f520d7dc517 Mon Sep 17 00:00:00 2001 From: mickael e Date: Thu, 21 Feb 2019 08:59:01 -0500 Subject: [PATCH 3/4] Use only explicitly declared conffiles 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. --- install_files/securedrop-app-code/debian/rules | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/install_files/securedrop-app-code/debian/rules b/install_files/securedrop-app-code/debian/rules index 3ef32adac5..20268c26f4 100755 --- a/install_files/securedrop-app-code/debian/rules +++ b/install_files/securedrop-app-code/debian/rules @@ -21,3 +21,9 @@ endif override_dh_gencontrol: dh_gencontrol -- $(SUBSTVARS) + +# Move the conffile in version control to squash the autogenerated one by debhelper, as files in /etc/ are automatically marked as conffiles. +# We are shipping AppArmor profiles via this package, and want them to be correctly updated with each update. +override_dh_installdeb: + dh_installdeb + cp ${CURDIR}/debian/conffiles ${CURDIR}/debian/securedrop-app-code/DEBIAN/ From 5e7e3c68e46a09863f78007cadd65c7f7a334594 Mon Sep 17 00:00:00 2001 From: mickael e Date: Fri, 22 Feb 2019 09:38:27 -0500 Subject: [PATCH 4/4] Fix test name collision to pass linting --- molecule/builder-trusty/tests/test_securedrop_deb_package.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/molecule/builder-trusty/tests/test_securedrop_deb_package.py b/molecule/builder-trusty/tests/test_securedrop_deb_package.py index fbb99e6e0e..472f90e458 100644 --- a/molecule/builder-trusty/tests/test_securedrop_deb_package.py +++ b/molecule/builder-trusty/tests/test_securedrop_deb_package.py @@ -230,7 +230,7 @@ def test_deb_package_contains_no_generated_assets(host, deb): @pytest.mark.parametrize("deb", deb_packages) -def test_deb_package_contains_no_generated_assets(host, deb): +def test_deb_package_contains_expected_conffiles(host, deb): """ Ensures the `securedrop-app-code` package declares only whitelisted `conffiles`. Several files in `/etc/` would automatically be marked