Allow SSH over local network instead of Tor #2592

msheiny · 2017-11-20T17:14:31Z

Status

Ready for review

Description of Changes

Fixes Allow optional local management of SD servers without tor #2091 - Allows an admin to enable SSH over local network instead of Tor.
Kind of, sorta helps with SecureDrop validate role fails on Tails 3.3 #2591 on fresh install only -- need a fix in a different PR to permanently address

Changes proposed in this pull request:

If an admin toggles this setting (optional, its off by default) though, this PR will:

not install tor on the monitor server
add in iptables rules restricting SSH to the same interface and IP space that the admin is utilizing to install on. For instance, if the admin has multiple interfaces and comes in over a particular IPv4 addr, the iptables rules will restrict SSH to that interface and to the local /cidr network of that interface's ip address.
allow admin to set ssh over local network option under securedrop-admin sdconfig

Testing

How should the reviewer test this PR?

I've tested this locally under a full provisioning (boot to tails, running ./securedrop-admincommands). So I could use eyes on vagrant work-flow . CI now takes advantage of this mode (since essentially it was running staging before anyways).

Deployment

Any special considerations for deployment? Consider both:

Upgrading existing production instances. --- If an admin were to run this against a previously setup pair of instances - this might not close the old Tor ssh exception specifically on the mon server. I just thought of that. I don't have a good story here right now. I'd like to push off to another ticket so this PR doesnt get too crazy (if theres no objections).
New installs. -- No crazy considerations, admin will just need to answer the prompt at sdconfig

Checklist

I made changes to both system config and docs.

If you made changes to the system configuration:

Configuration tests pass

If you made changes to documentation:

Doc linting passed locally

conorsch

Performed thorough readthrough of the docs and requesting changes for clarity. Happy to commit directly to clean up. Have not yet validated the functionality of the options as presented—that'll happen next.

conorsch · 2017-11-27T19:51:26Z

docs/ssh_over_local_net.rst

+
+Most administrators will never need SSH access during the course of running a
+SecureDrop instance so the potential short-falls of having ssh over tor aren't
+usually a big deal. The cons of having SSH over tor can include:


Most administrators will never need SSH access during the course of running a SecureDrop instance

Not true. We ask Admins to run commands against servers on average twice a year. They'll definitely need it at some point. Still, the point about minimizing attack surface is valid. Specifically we're trading the additional factor of a HidServAuth cookie for physical access to the hardware firewall. Given the damage an attacker could do with physical access to the boxes, that's a reasonable trade-off for some orgs.

conorsch · 2017-11-27T19:52:45Z

docs/ssh_over_local_net.rst

+* Provides anonymity to an administrator while logging into the SecureDrop
+  back-end.
+* Can mitigate against an attacker on your local network attempting to exploit
+  vulnerabilities against the SSHd daemon.


Changes to be made throughout:

s/ssh/SSH/ (unless referenced specifically as a command)

s/SSHd daemon/SSH daemon/

s/tor/Tor/ (unless referenced as a command or file)

conorsch · 2017-11-27T19:53:24Z

docs/ssh_over_local_net.rst

+          exposed to unintended users if you did not properly follow our network
+          firewall guide.
+
+The setting that controls ssh local-net access is set during the `sdconfig` step


ssh local-net access

How about: "SSH over LAN".

conorsch · 2017-11-27T19:55:12Z

docs/ssh_over_local_net.rst

+        for more information https://docs.securedrop.org/en/stable"
+        }
+
+    Enable SSH over Tor - alternatively ssh over local net [true]: no


This is a confusing prompt—hard to tell what system state "true" will enforce. How about:

Force SSH over Tor (otherwise over LAN) [true]: no

Even that phrasing makes it tempting for Admins to disable the option.

👍 i really like this wording change

conorsch · 2017-11-27T19:56:31Z

install_files/ansible-base/roles/restrict-direct-access/defaults/main.yml

-# connections are forced over Tor. In staging, this value will be 0.0.0.0
-# to enable direct access for testing during development.
-ssh_listening_address: 127.0.0.1
+enable_ssh_over_tor: true


Default value consistent with current project settings. 👍

conorsch · 2017-11-27T21:46:35Z

install_files/ansible-base/roles/restrict-direct-access/templates/rules_v4

+  {%- if ssh_ip|ipaddr(net_string) -%}
+    {%- set ssh_local_net = net_string|ipaddr('cidr') -%}
+    {%- set ssh_int = interface -%}
+      {%- if enable_ssh_over_tor == true or 'staging' in group_names -%}


The if enable_ssh_over_tor conditional should be the governor of whether the entire "Permit direct SSH access" block is entered. Let's move this to the top of the block and iterate over the interfaces only if required.

The netmask attributes on ifaces will also require additional pip requirements, but I see you already added those to the admin requirements file.

unfortunately -- if you do that logic (only loop over this if enable_ssh_over_tor) you are unable to use these values outside that loop. That bit me hard while debugging and I couldnt find a way to set jinja variables global inside a loop

for reference of similar issue I ran into when trying to add this under a giant conditional at the top previously -- https://stackoverflow.com/questions/17925674/jinja2-local-global-variable

i did just find a post of someone assigning to a dictionary instead though to get around this --> https://stackoverflow.com/questions/9486393/jinja2-change-the-value-of-a-variable-inside-a-loop .

I honestly dont think its a huge deal as-is. can you chip in with more context on why you think the current logic is going to be problematic?

msheiny · 2017-11-27T22:31:10Z

rebased on develop

msheiny · 2017-11-28T15:54:45Z

rebased again to fix an issue with a previous rebase ... :|

conorsch · 2018-02-23T20:05:11Z

I realize this PR has undergone significant rebasing, but CI is failing at the moment, so I am not re-reviewing. Unlikely to make it into 0.6 before merge window closes @msheiny. 😞

Addressed two issues here: * One bug report during QA of the PR where a user had an interface up without an active ipv4 address (so the ipv4 attribute failed) * Added back the ssh rate-limiting logic to local ssh that is present in the ssh over tor logic (for feature parity)

Found a situation when a user previously had ssh_over_tor enabled, they disable it, and run tailsconfig. We want to ensure that the local users sshconfig is setup to connect directly as oppossed to over tor.

This is a rare scenario that I'm addressing and this isnt the smoothest logic in the world, but it does the trick. All of our roles are currently designed for the opposite scenario (transistioning from ssh localnet -> ssh over tor), going the opposite way is tricky. So I added a minimal amount of conditional logic and have it bail early with a friendly user message. I tried to do a message and a meta `end_play` event instead so it wouldn't show as an "error" but I was unsuccessful to get that working as documented

* Noticed a white-space bug in the iptables template :| * Expanded iptables tests for the staging environment

Turns out this wasnt "legacy" and is needed for macosx/vbox users under vagrant 🤷

:smh:

Previously system would not reboot as needed when iptables changes were made after the initial install. With the updated logic when iptables changes, we drop a file on disk indicating a reboot is needed. The reboot task which is wayy further down the line sees that file and reboots as necessary.

Modified the comments a bit. In general the iptables rules are pretty terrible and need a huge refactor. There is a community member attempting to address some of that. Hopefully I can assist with rebase efforts after this.

So during testing against a production hardware instance this came up and smacked me in the face :) During testing locally everything is done on the same /24 but in PROD we recommend having split networks which is what I had -- and things fell down fast :( So this fixes the logic, so it'll calculate the network CIDR of the ADMIN network (derived from commands run on the admin workstation) and allow all from there.

* Fix reference to securedrop-admin sdconfig * Add warning about not using this feature through a NAT

Its important the user specifically updates to a later branch that includes this update. It is also important for the end-user to update their pip dependencies which includes a jinja ipaddr filter used for network string parsing

msheiny · 2018-04-17T16:38:10Z

Rebased on develop to pull in #3272 fix

conorsch · 2018-04-17T20:04:28Z

Having an exceedingly hard time breaking this, which is fantastic. Going to run through yet another install on clean prod VMs, this time validating that opting out of ssh-over-tor from the get-go works smoothly. Very optimistic this'll get in before the current sprint is over.

conorsch · 2018-04-18T00:29:20Z

Repeated runs with both clean-install-without-tor and converted-install worked well. Used prod VMs from virtualized Tails environment exclusively. We'll surely get in more quality time with this puppy during pre-release QA for 0.7, but as it stands, it addresses all the major edges we should accommodate. Fantastic work here, @msheiny.

@emkll

Dismissing blocking review because @emkll commented LGTM in text comment above

msheiny requested a review from conorsch as a code owner November 20, 2017 17:14

msheiny requested a review from a user November 20, 2017 17:14

msheiny requested review from heartsucker and redshiftzero as code owners November 20, 2017 17:14

msheiny force-pushed the SSHoverLocal branch from 462b94c to 01c0093 Compare November 20, 2017 22:20

redshiftzero added this to the 0.5 Stretch milestone Nov 27, 2017

redshiftzero assigned emkll Nov 27, 2017

conorsch suggested changes Nov 27, 2017

View reviewed changes

redshiftzero added the release blocker label Nov 27, 2017

redshiftzero modified the milestones: 0.5 Stretch, 0.5 Nov 27, 2017

redshiftzero mentioned this pull request Nov 27, 2017

Explicit docs to reactivate the virtualenv #2621

Merged

1 task

msheiny force-pushed the SSHoverLocal branch from 31a93f7 to 0efb25a Compare November 27, 2017 22:31

msheiny force-pushed the SSHoverLocal branch from 0efb25a to ca3f8a6 Compare November 27, 2017 22:32

redshiftzero removed the release blocker label Nov 28, 2017

redshiftzero modified the milestones: 0.5, 0.5 Stretch Nov 28, 2017

msheiny force-pushed the SSHoverLocal branch from 9b94c9f to 8162c81 Compare November 28, 2017 15:54

redshiftzero modified the milestones: 0.5 Stretch, 0.5.1 Nov 28, 2017

msheiny mentioned this pull request Dec 22, 2017

Uses apt mirror for Tor via securedrop-config package #2441

Merged

3 tasks

redshiftzero modified the milestones: 0.5.1, 0.6 Jan 4, 2018

emkll assigned msheiny and unassigned emkll Feb 8, 2018

msheiny force-pushed the SSHoverLocal branch from 8162c81 to 7d17c41 Compare February 16, 2018 21:48

redshiftzero removed this from the 0.6 milestone Feb 27, 2018

msheiny added 15 commits April 17, 2018 12:36

Address transistion scenario when disabling ssh_over_tor

7e26a2e

Found a situation when a user previously had ssh_over_tor enabled, they disable it, and run tailsconfig. We want to ensure that the local users sshconfig is setup to connect directly as oppossed to over tor.

Add accompanying admin test for nable_ssh_over_tor

a55dd5a

fix iptables template, expand tests ssh+localnet

51606ed

* Noticed a white-space bug in the iptables template :| * Expanded iptables tests for the staging environment

Ignore visual-studio code local config folder

a5a8a5e

Fix - prod not installw/ ssh/local on fresh inst

b4e62e3

Revert "Remove legacy second interface on staging env"

db0cf42

Turns out this wasnt "legacy" and is needed for macosx/vbox users under vagrant 🤷

Fix var name, ssh over local net for mon

3314884

:smh:

Adjust iptables tor rules to allow traffic for mon

927e54c

Modified the comments a bit. In general the iptables rules are pretty terrible and need a huge refactor. There is a community member attempting to address some of that. Hopefully I can assist with rebase efforts after this.

Update SSH over LAN docs

367827d

* Fix reference to securedrop-admin sdconfig * Add warning about not using this feature through a NAT

Tweak SSH documentation to add reference to env update

4ee572e

Its important the user specifically updates to a later branch that includes this update. It is also important for the end-user to update their pip dependencies which includes a jinja ipaddr filter used for network string parsing

Fix doc goof with reference to admin command

113ccd0

msheiny force-pushed the SSHoverLocal branch from 69f2f43 to 113ccd0 Compare April 17, 2018 16:37

conorsch approved these changes Apr 18, 2018

View reviewed changes

redshiftzero merged commit e0b9adf into develop Apr 18, 2018

redshiftzero deleted the SSHoverLocal branch April 18, 2018 14:40

conorsch mentioned this pull request Apr 18, 2018

[qt-journalist-updater] Add docs for updater #3283

Merged

1 task

emkll mentioned this pull request Apr 25, 2018

Release SecureDrop 0.7.0 #3307

Closed

21 tasks

ghost mentioned this pull request May 5, 2018

Staging environment broken #3318

Closed

conorsch mentioned this pull request Jun 22, 2018

[QA] iptables conditional logic can reference undefined var #3555

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow SSH over local network instead of Tor #2592

Allow SSH over local network instead of Tor #2592

msheiny commented Nov 20, 2017 •

edited

Loading

conorsch left a comment

conorsch Nov 27, 2017

conorsch Nov 27, 2017

conorsch Nov 27, 2017

conorsch Nov 27, 2017

msheiny Nov 27, 2017

conorsch Nov 27, 2017

conorsch Nov 27, 2017

msheiny Nov 27, 2017

msheiny Nov 28, 2017

msheiny commented Nov 27, 2017

msheiny commented Nov 28, 2017

conorsch commented Feb 23, 2018

msheiny commented Apr 17, 2018

conorsch commented Apr 17, 2018

conorsch commented Apr 18, 2018

Allow SSH over local network instead of Tor #2592

Allow SSH over local network instead of Tor #2592

Conversation

msheiny commented Nov 20, 2017 • edited Loading

Status

Description of Changes

Testing

Deployment

Checklist

If you made changes to the system configuration:

If you made changes to documentation:

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

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

msheiny commented Nov 27, 2017

msheiny commented Nov 28, 2017

conorsch commented Feb 23, 2018

msheiny commented Apr 17, 2018

conorsch commented Apr 17, 2018

conorsch commented Apr 18, 2018

msheiny commented Nov 20, 2017 •

edited

Loading