-
Notifications
You must be signed in to change notification settings - Fork 687
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Get testinfra working on Qubes #5712
Conversation
@rocodes has offered to take a first pass through this PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pretty rad work, @rmol. I still encountered a few failing tests, all of them related to the hostname problems you flagged. Even after adjusting the entries in /etc/hosts
, there are other places, like /etc/hostname
, that would need to be updated as well. Simply following along in the docs and substituting the new hostname info would be reasonable, but I haven't done that yet.
Before I do, I ask you: is it worth substituting all of them, and updating the docs to match? See suggestion in-line about making the hostname targeting for the tests fuzzier, in a way that should Just Work™ for existing setups. Supporting existing setups isn't critical here, but I like that we could shelve the docs updates for now if you agree. If you'd prefer to stick with the implementation as proposed, then I'd ask for a docs PR that I can review in tandem with this one.
-A INPUT -s {{ mon_ip }}/32 -p udp -m udp --sport 1514 -m state --state RELATED,ESTABLISHED -m comment --comment "OSSEC server agent" -j ACCEPT | ||
-A INPUT -s {{ mon_ip }}/32 -p tcp -m tcp --dport 22 -m comment --comment "Block explicitly SSH from the adjacent SD component" -j DROP | ||
-A INPUT -s {{ ssh_ip }}/32 -i {{ default_interface }} -p tcp -m tcp --dport 22 -m state --state NEW -m limit --limit 3/min --limit-burst 3 -m comment --comment "Rate limit incoming ssh traffic" -j ACCEPT | ||
-A INPUT -s {{ ssh_ip }}/32 -i {{ default_interface }} -p tcp -m tcp --dport 22 -m state --state NEW,RELATED,ESTABLISHED -j ACCEPT |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The firewall rules template is duplicated in its entirety for just L18 & L19, which differ slightly from the existing staging rules:
$ diff molecule/testinfra/app/iptables-app-staging.j2 molecule/testinfra/app/iptables-app-qubes-staging.j2
18,19c18,19
< -A INPUT -i eth0 -p tcp -m tcp --dport 22 -m state --state NEW -m limit --limit 3/min --limit-burst 3 -m comment --comment "Rate limit incoming ssh traffic" -j ACCEPT
< -A INPUT -i eth0 -p tcp -m tcp --dport 22 -m state --state NEW,RELATED,ESTABLISHED -j ACCEPT
---
> -A INPUT -s {{ ssh_ip }}/32 -i {{ default_interface }} -p tcp -m tcp --dport 22 -m state --state NEW -m limit --limit 3/min --limit-burst 3 -m comment --comment "Rate limit incoming ssh traffic" -j ACCEPT
> -A INPUT -s {{ ssh_ip }}/32 -i {{ default_interface }} -p tcp -m tcp --dport 22 -m state --state NEW,RELATED,ESTABLISHED -j ACCEPT
Can you explain that difference? Inspecting the logic in the restrict-direct-access role, it's not immediately obvious to me why the qubes env would differ in the fw rules that land on the host.
admin_user: sdadmin | ||
|
||
app_hostname: app-staging | ||
monitor_hostname: mon-staging |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The hostnames can be customized here. The testinfra files themselves typically have a line like testinfra_hosts = [sdvars.app_hostname]
to restrict targeting, but I don't see why we can't—or shouldn't—update that to a more general testinfra_hosts = ["sd-app", "app-staging"]
etc. If you agree, @rmol, then that'd save the trouble of munging the docs, I think.
# Build a dict of variables to pass to jinja for iptables comparison | ||
kwargs = dict( | ||
ssh_ip=local.interface("eth0").addresses[0], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As above, it's not clear to me why this is suddenly required for Qubes, but not for other staging envs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change appears to be breaking CI, investigating.
- 8.8.8.8 | ||
- 8.8.4.4 | ||
mon_ip: 10.137.0.51 | ||
app_ip: 10.137.0.50 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In practice, do you reuse these IPv4 addresses between Xenial & Focal envs? I'm still figuring out what works for me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Poking around in the IP-related logic specific to the Qubes staging env, found some dead code that should be removed:
- https://github.com/freedomofpress/securedrop/blob/develop/molecule/qubes-staging-xenial/qubes-vars.yml#L10-L24
- https://github.com/freedomofpress/securedrop/blob/develop/molecule/qubes-staging-xenial/create.yml#L42-L49
If you agree, now's a good time to snip them out—and in the corresponding Focal locations, too.
5e1e524
to
fb6f4f1
Compare
Documenting changes presented in freedomofpress/securedrop#5712
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Works well, thank you for taking the time, @rmol. I took the liberty of rebasing on top of latest develop, and while there are still a few failing tests, they're tracked in discrete tickets such as #5776 & #5782.
I also submitted a very small docs update to match these changes, here: freedomofpress/securedrop-docs#150
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Focal
CI is failing with the following error:
AssertionError: Unexpected exit code 1 for CommandResult(command=b'/usr/bin/ip addr show eth0', exit_status=1, stdout=None, stderr=b'Device "eth0" does not exist.\n')
assert 1 == 0
+1
-0
host = <testinfra.host.Host ansible://app-staging>
@pytest.mark.skip_in_prod
def test_app_iptables_rules(host):
local = host.get_host("local://")
# Build a dict of variables to pass to jinja for iptables comparison
kwargs = dict(
> ssh_ip=local.interface("eth0").addresses[0],
mon_ip=os.environ.get('MON_IP', securedrop_test_vars.mon_ip),
default_interface=host.check_output("ip r | head -n 1 | "
"awk '{ print $5 }'"),
tor_user_id=host.check_output("id -u debian-tor"),
securedrop_user_id=host.check_output("id -u www-data"),
ssh_group_gid=host.check_output("getent group ssh | cut -d: -f3"),
dns_server=securedrop_test_vars.dns_server)
../testinfra/app/test_app_network.py:21:
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
self = <interface eth0>
@property
def addresses(self):
> stdout = self.check_output("%s addr show %s", self._ip, self.name)
E AssertionError: Unexpected exit code 1 for CommandResult(command=b'/usr/bin/ip addr show eth0', exit_status=1, stdout=None, stderr=b'Device "eth0" does not exist.\n')
E assert 1 == 0
E +1
E -0
../../.venv/lib/python3.7/site-packages/testinfra/modules/interface.py:70: AssertionError
I think the probable cause for this failure is the naming scheme of the network interfaces on Focal
, on my prod
hardware installation it is enp2s0. I wonder if that is same in the Qubes environment.
During testing in staging VMs for Focal, frequently encountered this error: TASK [common : Install base apt depedencies] *********************************** fatal: [app-staging]: FAILED! => {"changed": false, "msg": "No package matching 'aptitude' is available"} fatal: [mon-staging]: FAILED! => {"changed": false, "msg": "No package matching 'aptitude' is available"} which task is immediately after we update the /etc/apt/sources.list file. Let's run `apt-get update` after modifying that file to ensure our lists are up to date.
Tries to make it conditional, based on platform. This should work under Qubes already, let's try it in CI.
fb6f4f1
to
3c3d0e2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All tests are passing.
Requested changes have been made
Status
Ready for review
Description of Changes
Fixes #5377.
Adds qubes-specific configuration to get testinfra tests passing with our Qubes staging environment.
The hostnames (e.g.
app-staging
versussd-staging-app
) and network addresses were the culprit in a lot of failures.Testing
make build-debs
While that's running, adjust the hostnames in your
sd-staging-(app|mon)-base-xenial
VMs:sd-staging-(app|mon)
to(app|mon)-staging
in/etc/hostname
and/etc/hosts
Once the debs are built:
molecule destroy -s qubes-staging-xenial
make staging
molecule verify -s qubes-staging-xenial
ormake testinfra
The staging qubes should be created and configured without error, and all tests should pass.
Deployment
This is dev/test only.
Checklist
If you made non-trivial code changes:
Choose one of the following:
qubes_staging.rst
)