Fixes #5776 adds iptables-persistent dependency on Focal #5780

kushaldas · 2021-02-09T10:44:34Z

Status

Ready for review.

Description of Changes

On Ubuntu Focal, we can use iptables-persistent package, and also
uses updated rules filepath based on distribution version.

Testing

Install on Focal on Hardware, and test iptables -L in both the servers
Add some new rules, example, accept everything on INPUT chain.
reboot and check ipables -L again. Everything should go back to default.
Install on Xenial on Hardware, and test iptables -L in both the servers
reboot and check ipables -L again.

Deployment

Any special considerations for deployment? Consider both:

Upgrading existing production instances.
New installs.

Checklist

If you made changes to the server application code:

Linting (make lint) and tests (make test) pass in the development container

If you made changes to `securedrop-admin`:

Linting and tests (make -C admin test) pass in the admin development container

If you made changes to the system configuration:

Configuration tests pass

If you added or removed a file deployed with the application:

I have updated AppArmor rules to include the change

If you made non-trivial code changes:

I have written a test plan and validated it for this PR

Choose one of the following:

I have opened a PR in the docs repo for these changes, or will do so later
I would appreciate help with the documentation
These changes do not require documentation

If you added or updated a code dependency:

Choose one of the following:

I have performed a diff review and pasted the contents to the packaging wiki
I would like someone else to do the diff review

conorsch · 2021-02-09T14:37:10Z

CI failure in https://app.circleci.com/pipelines/github/freedomofpress/securedrop/1827/workflows/afc450be-1937-4dae-b6ed-e5b174ef0379/jobs/49991, OSSEC service was not listening. Kicked CI to see if it's a flake.

conorsch

Left some in-line comments about reducing duplication, similar in spirit to what's discussed for dual distro support in #5772 (comment).

At present I'm not comfortable merging these changes until we understand completely what the divergence is between the two distros. I'll keep investigating on root causes and report findings in #5776

conorsch · 2021-02-09T14:42:15Z

install_files/ansible-base/roles/app/handlers/main.yml

+- name: reload iptables rules for focal
+  shell: iptables-restore < /etc/iptables/rules.v4
+  when:
+    - ansible_distribution_release == 'focal'


Since the only change here appears to be the filepath for the rules, let's make that a var and import it based on distro. See

securedrop/install_files/ansible-base/roles/common/tasks/main.yml

Line 2 in 2de09b4

- include_vars: "{{ ansible_distribution }}_{{ ansible_distribution_release }}.yml"

for an example

conorsch · 2021-02-09T14:47:18Z

install_files/ansible-base/roles/restrict-direct-access/files/load_iptables

 else
  echo "Iptables rules file does not exist"
  exit 1
 fi

 if [ -f /etc/network/iptables/rules_v6 ]; then
  ip6tables-restore < /etc/network/iptables/rules_v6
+elif [ -f /etc/iptables/rules.v6 ]; then
+  ip6tables-restore < /etc/iptables/rules.v6


As above, these paths can be interpolated from a var if we use a template. However, I see Focal-only paths in here, whereas the relevant task only copies this file on Xenial hosts. So these elifs appear to be unreachable.

Removed now.

conorsch · 2021-02-09T14:48:34Z

install_files/ansible-base/roles/restrict-direct-access/tasks/iptables.yml

@@ -35,6 +47,8 @@
    owner: root
    group: root
    dest: /etc/network/iptables
+  when:
+    - ansible_distribution_release == 'xenial'


If the load_iptables script is only copied under Xenial, then its content shouldn't change to address Focal support.

rmol · 2021-02-09T16:05:07Z

I think the divergence is explained by the fact that on Focal server, network configuration is handled by a combination of netplan and systemd-networkd (search for "hook scripts"; the document lacks anchors).

I tried simply moving our /etc/network/if-up.d/load_iptables script to /etc/networkd-dispatcher/routable.d/00_load_iptables as suggested by this conversion table and the iptables rules were correctly started when I bounced the interface.

I have not changed the Ansible config and rebuilt staging, or tried it on hardware. There are also other hook scripts installed into /etc/network/if-up.d by other packages (resolvconf, ethtool), which I can't explain, unless they simply weren't updated for the Focal network changes, but ethtool is in the utils section. 🤷‍♂️

conorsch · 2021-02-09T16:31:24Z

I tried simply moving our /etc/network/if-up.d/load_iptables script to /etc/networkd-dispatcher/routable.d/00_load_iptables

That's a great suggestion, and matches with the use-a-path-based-on-vars comments above. We're bumping up against reimplementing iptables-persistent functionality, so let's not rule that out yet. As mentioned in #5776 (comment), tying the restore logic to the ifup event should be the strictest of all possible options. That's based on my assumption that iptables-persistent will always restore whatever rules are present on system shutdown—I'll read up on that now.

kushaldas · 2021-02-09T17:28:34Z

I noticed https://unix.stackexchange.com/questions/125833/why-isnt-the-iptables-persistent-service-saving-my-changes

conorsch · 2021-02-09T23:49:55Z

That's based on my assumption that iptables-persistent will always restore whatever rules are present on system shutdown

Not so. Performed some interactive testing, and it appears that iptables-persistent is behaving the way we'd want: ad-hoc rules added on the command-line are wiped after reboot, and the original ruleset is enforced on subsequent boot. With that concern out of the way, I'm comfortable switching to iptables-persistent for Focal only (cc @emkll for visibility).

@kushaldas Right now, CI is still running on #5712, but I've approved it. Please merge it and rebase these changes on top, then take a look at the in-line comments here and try to refactor. Also, please update the test plan to include verification that ad-hoc rules are removed. In my testing, I tried setting the input policy to ACCEPT, as well as adding a global allow on TCP 443 inbound.

kushaldas · 2021-02-10T13:41:50Z

Made all other changes but #5712 still needs work, so did not merge it. @conorsch

conorsch · 2021-02-10T15:10:39Z

Rebased on top of latest develop, to include changes from #5712

conorsch · 2021-02-10T16:50:30Z

Still seeing the same CI failure:

failing test output

assert False == True
  +False
  -True
host = <testinfra.host.Host ansible://mon-staging>
ossec_service = {'host': '0.0.0.0', 'listening': True, 'port': 1514, 'proto': 'udp'}

    @pytest.mark.skip_in_prod
    @pytest.mark.parametrize('ossec_service', [
        dict(host="0.0.0.0", proto="tcp", port=22, listening=True),
        dict(host="0.0.0.0", proto="udp", port=1514, listening=True),
        dict(host="0.0.0.0", proto="tcp", port=1515, listening=False),
    ])
    def test_listening_ports(host, ossec_service):
        """
        Ensure the OSSEC-related services are listening on the
        expected sockets. Services to check include ossec-remoted
        and ossec-authd. Helper services such as postfix are checked
        separately.
    
        Note that the SSH check will fail if run against a prod host, due
        to the SSH-over-Tor strategy. We can port the parametrized values
        to config test YAML vars at that point.
        """
        socket = "{proto}://{host}:{port}".format(**ossec_service)
        with host.sudo():
>           assert (host.socket(socket).is_listening ==
                    ossec_service['listening'])
E           assert False == True
E             +False
E             -True

Will dig into it more today.

rmol · 2021-02-10T17:38:44Z

This could be a red herring, but host.socket supposedly requires netstat, and at least on my fresh staging mon server, it's not installed. Has this ever worked for any tester or service?

conorsch · 2021-02-10T17:47:47Z

That's helpful, @rmol, but also a bit confusing, because the test is passing locally for me. It also passed in CI as recently as merge of #5712!

conorsch · 2021-02-10T22:15:05Z

The fact that CI is reliably failing here, yet passing on #5783, indicates we clearly have a regression in the firewall logic. I'm optimistic that the changes proposed in #5783 will resolve here, too. Pushed a separate branch now to evaluate behavior in CI: https://github.com/freedomofpress/securedrop/tree/5776-iptables-persistent-for-focal-with-reconnect

conorsch · 2021-02-10T23:17:56Z

I'm optimistic that the changes proposed in #5783 will resolve here, too. Pushed a separate branch now to evaluate behavior in CI

Sure enough, tests are passing: https://app.circleci.com/pipelines/github/freedomofpress/securedrop/1844/workflows/835e3251-afb0-412d-95b3-b25373c5dbc3/jobs/50137 So let's get #5783 reviewed and in, then rebase this one.

kushaldas · 2021-02-11T10:44:44Z

And magic, all tests are passing!!!

conorsch · 2021-02-11T14:55:30Z

Thanks, @kushaldas! Taking another look now.

conorsch · 2021-02-11T15:53:43Z

Changes are solid. To test, I set sudo iptables -P INPUT ACCEPT as an intentional breakage of the rules. I applied this diff:

diff --git a/molecule/qubes-staging-focal/molecule.yml b/molecule/qubes-staging-focal/molecule.yml
index 5bb3873cf..f8f538b29 100644
--- a/molecule/qubes-staging-focal/molecule.yml
+++ b/molecule/qubes-staging-focal/molecule.yml
@@ -55,5 +55,6 @@ verifier:
   options:
     n: auto
     v: 2
+    k: iptables_rules
   env:
     SECUREDROP_TESTINFRA_TARGET_HOST: qubes-staging

to run only the iptables tests, and confirmed failing:

 >           assert iptables_expected == iptables
    E           assert '*filter\n:IN... DROP\nCOMMIT' == '*filter\n:IN... DROP\nCOMMIT'
    E               *filter
    E             - :INPUT ACCEPT
    E             + :INPUT DROP
    E               :FORWARD DROP
    E               :OUTPUT DROP
    E               :LOGNDROP -
    E               -A INPUT -p tcp -m state --state RELATED,ESTABLISHED -m comment --comment "Allow traffic back for tor" -j ACCEPT...
    E             
    E             ...Full output truncated (39 lines hidden), use '-vv' to show
    
    ../testinfra/app/test_app_network.py:54: AssertionError

After rebooting the host, all tests are passing again:

../testinfra/mon/test_mon_network.py::test_mon_iptables_rules[ansible:/mon-staging]
    ../testinfra/app/test_app_network.py::test_app_iptables_rules[ansible:/app-staging]
    [gw0] [ 50%] PASSED ../testinfra/mon/test_mon_network.py::test_mon_iptables_rules[ansible:/mon-staging]
    [gw1] [100%] PASSED ../testinfra/app/test_app_network.py::test_app_iptables_rules[ansible:/app-staging]

In addition to changing the policy items, I added additional rules such as all TCP 443 allowed in, and confirmed that a reboot removed the customizations.

There are a few changes I'd like to make to clean up the formatting, which I'll append now. Very pleased with the results of testing, these changes are solid and Focal-only, as we want.

conorsch · 2021-02-11T15:50:58Z

install_files/ansible-base/roles/restrict-direct-access/tasks/iptables.yml

@@ -59,14 +75,14 @@
 - name: Copy IPv4 iptables rules.
  template:
    src: rules_v4
-    dest: /etc/network/iptables/rules_v4
+    dest: "{{ '/etc/iptables/rules.v4' if ansible_distribution_release == 'focal' else  '/etc/network/iptables/rules_v4' }}"


Path should use the var created above

conorsch · 2021-02-11T15:51:01Z

install_files/ansible-base/roles/restrict-direct-access/tasks/iptables.yml

    owner: root
    mode: "0644"
  notify: drop flag for reboot

 - name: Copy IPv6 iptables rules.
  copy:
    src: iptables_rules_v6
-    dest: /etc/network/iptables/rules_v6
+    dest: "{{ '/etc/iptables/rules.v6' if ansible_distribution_release == 'focal' else  '/etc/network/iptables/rules_v6' }}"


Path should use the var created above

conorsch · 2021-02-11T15:51:28Z

install_files/ansible-base/roles/restrict-direct-access/tasks/iptables.yml

+    pkg: iptables-persistent
+    state: latest
+    update_cache: yes
+    cache_valid_time: 3600


We can avoid another apt run by placing the package requirement in the common role, in the focal vars.

conorsch · 2021-02-11T15:56:46Z

install_files/ansible-base/roles/app/handlers/main.yml

- name: reload iptables rules
-  shell: iptables-restore < /etc/network/iptables/rules_v4
+- name: reload iptables rules for xenial
+  shell: iptables-restore < "{{ iptables_v4_path }}"


This handler isn't actually used anywhere, neither in this PR nor in develop. OK to remove

On Ubuntu Focal, we can use iptables-persistent package, and also uses updated rules filepath based on distribution version.

Focal uses iptables-persistent package for the same.

Based on PR review feedback, we are now using paths defined in the variable vars files (based on the Ubuntu distribution value).

conorsch · 2021-02-11T19:09:53Z

Works as advertised!

conorsch

Re-approving post-rebase, CI is green, merging!

kushaldas requested review from conorsch and emkll as code owners February 9, 2021 10:44

This was referenced Feb 9, 2021

[Focal] iptables rules are not applied on boot #5776

Closed

Bump securedrop-grsec-focal metapackage to 5.4.88 #5772

Merged

conorsch suggested changes Feb 9, 2021

View reviewed changes

emkll added this to In Development in SecureDrop Team Board Feb 9, 2021

kushaldas force-pushed the 5776-iptables-persistent-for-focal branch from 0db6ebc to d4522ee Compare February 10, 2021 06:44

conorsch force-pushed the 5776-iptables-persistent-for-focal branch from d4522ee to 27ccd22 Compare February 10, 2021 15:10

conorsch self-requested a review February 10, 2021 15:10

emkll added the release blocker label Feb 10, 2021

conorsch added the blocked label Feb 10, 2021

kushaldas force-pushed the 5776-iptables-persistent-for-focal branch from 27ccd22 to d1b178d Compare February 11, 2021 09:43

emkll moved this from In Development to Ready for Review in SecureDrop Team Board Feb 11, 2021

conorsch removed the blocked label Feb 11, 2021

emkll assigned conorsch Feb 11, 2021

conorsch reviewed Feb 11, 2021

View reviewed changes

kushaldas and others added 4 commits February 11, 2021 14:07

Fixes #5776 adds iptables-persistent dependency on Focal

ac1ca03

On Ubuntu Focal, we can use iptables-persistent package, and also uses updated rules filepath based on distribution version.

Uses the load_iptables only for Xenial

01cc5af

Focal uses iptables-persistent package for the same.

Uses distribution based variables in app handlers

70c5307

Based on PR review feedback, we are now using paths defined in the variable vars files (based on the Ubuntu distribution value).

Removes unused iptables-restore handler

47853d6

conorsch force-pushed the 5776-iptables-persistent-for-focal branch from d1b178d to da2ae7c Compare February 11, 2021 19:08

conorsch previously approved these changes Feb 11, 2021

View reviewed changes

Adds config tests for iptables-persistent package

8e10aec

conorsch dismissed their stale review via 8e10aec February 11, 2021 20:01

conorsch force-pushed the 5776-iptables-persistent-for-focal branch from da2ae7c to 8e10aec Compare February 11, 2021 20:01

conorsch self-requested a review February 11, 2021 21:24

conorsch approved these changes Feb 11, 2021

View reviewed changes

conorsch merged commit 240a9a9 into develop Feb 11, 2021

SecureDrop Team Board automation moved this from Ready for Review to Done Feb 11, 2021

rmol deleted the 5776-iptables-persistent-for-focal branch June 23, 2021 14:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes #5776 adds iptables-persistent dependency on Focal #5780

Fixes #5776 adds iptables-persistent dependency on Focal #5780

kushaldas commented Feb 9, 2021 •

edited

conorsch commented Feb 9, 2021

conorsch left a comment

conorsch Feb 9, 2021

conorsch Feb 9, 2021

kushaldas Feb 10, 2021

conorsch Feb 9, 2021

kushaldas Feb 10, 2021

rmol commented Feb 9, 2021

conorsch commented Feb 9, 2021

kushaldas commented Feb 9, 2021

conorsch commented Feb 9, 2021 •

edited

kushaldas commented Feb 10, 2021

conorsch commented Feb 10, 2021

conorsch commented Feb 10, 2021 •

edited

rmol commented Feb 10, 2021

conorsch commented Feb 10, 2021

conorsch commented Feb 10, 2021

conorsch commented Feb 10, 2021

kushaldas commented Feb 11, 2021

conorsch commented Feb 11, 2021

conorsch commented Feb 11, 2021

conorsch Feb 11, 2021

conorsch Feb 11, 2021

conorsch Feb 11, 2021

conorsch Feb 11, 2021

conorsch commented Feb 11, 2021

conorsch left a comment

Fixes #5776 adds iptables-persistent dependency on Focal #5780

Fixes #5776 adds iptables-persistent dependency on Focal #5780

Conversation

kushaldas commented Feb 9, 2021 • edited

Status

Description of Changes

Testing

Deployment

Checklist

If you made changes to the server application code:

If you made changes to securedrop-admin:

If you made changes to the system configuration:

If you added or removed a file deployed with the application:

If you made non-trivial code changes:

If you added or updated a code dependency:

conorsch commented Feb 9, 2021

conorsch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rmol commented Feb 9, 2021

conorsch commented Feb 9, 2021

kushaldas commented Feb 9, 2021

conorsch commented Feb 9, 2021 • edited

kushaldas commented Feb 10, 2021

conorsch commented Feb 10, 2021

conorsch commented Feb 10, 2021 • edited

rmol commented Feb 10, 2021

conorsch commented Feb 10, 2021

conorsch commented Feb 10, 2021

conorsch commented Feb 10, 2021

kushaldas commented Feb 11, 2021

conorsch commented Feb 11, 2021

conorsch commented Feb 11, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

conorsch commented Feb 11, 2021

conorsch left a comment

Choose a reason for hiding this comment

kushaldas commented Feb 9, 2021 •

edited

If you made changes to `securedrop-admin`:

conorsch commented Feb 9, 2021 •

edited

conorsch commented Feb 10, 2021 •

edited