-
Notifications
You must be signed in to change notification settings - Fork 686
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
Removes uid-owner restriction from apt iptables rules #3978
Conversation
2b2a3f0
to
e39249c
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.
This PR is working as it should be, the only problem is that sometimes after installing the new debian package, the system is loosing the DNS ability. Not sure, why :(
# Find already configured apt allow line, targeting root uid, and | ||
# subsitute entire line, dropping the uid targeting entirely, | ||
# so both Trusty and Xenial continue to work well when invoking apt. | ||
perl -npi -e \ |
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 works :)
Good flag, let's dig into that. What commands did you use to test? For example, |
Ah, now I can see that |
|
Pushed up another change here, attempting to resolve the DNS behavior post-upgrade. @kushaldas take this change for a spin and let me know how it treats you? |
@conorsch The latest push works. Though it helped to find a different problem, we do need a FPF tor-apt for xenial ready.
|
Performed functional testing based on the test plan as follows: Trusty -> Xenial (i.e. Trusty clean install)I have tested the 'clean' install under Trusty, then doing an interactive upgrade to Xenial. The changes here seem to work as expected, both in Trusty and Xenial: apt traffic goes through the firewall, all tests pass. Xenial clean installThe provisioning process in xenial from a clean install works as expected, apt traffic can go through the firewall, all test pass. Trusty -> Trusty (this branch) -> XenialApt traffic goes through the firewall. |
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.
Given passwordless sudo, I think this a reasonable stopgap to ensure smooth migration to Xenial: having uid-owner set to root during the upgrade will break the upgrade process. Default action for OUTPUT chain appears to be DROP, not LOGNDROP as stated in commit message.
We should strongly consider revisiting these rules (e.g. set -m owner --uid-owner _apt
) after we migrate to Xenial, since we also lock down iptables rules per user for tor, ntp and ssh (perhaps as part of the upgrade playbook?)
I'll let @kushaldas complete his review, to ensure this PR gets the careful review it needs, and resolve postinst conflicts, otherwise 👍 from me
Under Trusty, apt calls run as the root user. Under Xenial, however, apt calls run as the `_apt` user. We must ensure that the firewall rules permit apt updates on both systems, before and after the upgrade from Trusty to Xenial. We cannot add references to `_apt` under Trusty, since the user doesn't exist in the passwd file, causing the iptables-restore action to fail. Let's simply remove the uid-owner restriction on the apt-specific iptables rule. By this point in the iptables logic, non-whitelisted traffic from both the `debian-tor` and `www-data` users hsa been dropped (via jump to the LOGNDROP chain). As such, it's fine to loosen the specificity of the iptables rules somewhat, in order to ensure a smooth transition.
Simply removing the apt-specific user-based whitelisting was insufficient to unbreak DNS after a Trusty to Xenial upgrade. Also removing the restrictions on DNS resolution to allow e.g. `apt update` to function.
48be673
to
b5e9756
Compare
Rebased on latest develop (66e3607) to resolve conflicts. @kushaldas , please review and let me know if you find any problems. |
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.
I tried the PR manually, a full scale upgrade to xenial failed due to other reasons.
This PR works as intended.
Thanks for re-review, @kushaldas! |
Status
Ready for review.
Description of Changes
Fixes #3952.
Under Trusty, apt calls run as the root user. Under Xenial, however, apt
calls run as the
_apt
user. We must ensure that the firewall rulespermit apt updates on both systems, before and after the upgrade from
Trusty to Xenial. We cannot add references to
_apt
under Trusty, sincethe user doesn't exist in the passwd file, causing the iptables-restore
action to fail.
Let's simply remove the uid-owner restriction on the apt-specific
iptables rule. By this point in the iptables logic, non-whitelisted traffic
from both the
debian-tor
andwww-data
users hsa been dropped (viajump to the LOGNDROP chain). As such, it's fine to loosen the
specificity of the iptables rules somewhat, in order to ensure a smooth
transition.
Testing
We need to cover a few different scenarios here for full coverage:
In all cases, the following should be true:
sudo apt update
works on both hostsNote that
sudo host apt.freedom.press
can still pass whereassudo apt update
would be broken, so it's important to use the apt-update command during testing.I haven't tested all these scenarios yet myself, but opening PR to invite more feedback and increase the changes we find something to fix early.
Deployment
Yes, we must make sure this change lands in prod prior to the Xenial upgrade, otherwise we run the risk of breaking future automatic updates.
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