Skip to content
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

Update and add annotations to sshd config for servers #5666

Merged
merged 3 commits into from
Feb 11, 2021

Conversation

emkll
Copy link
Contributor

@emkll emkll commented Dec 9, 2020

Status

Work in progress

Description of Changes

Fixes #5660

  • Update supported algorthms
  • Disable some agent forwarding and tunnelling options
  • Annotate and reorder configuration for readability

Sources:

Testing

  • provided config is good / adheres to best practices
  • provided config is adequately annotated
  • provided config eliminates log message described in Update sshd config for Focal #5660
  • on an existing Xenial install the config is applied properly when an Ansible run occurs (I have tested this)

Deployment

New installs

The sshd configuration will be applied on new installs via Ansible

Existing installs

Existing installs will not be updated unattended, but applied when an Ansible run occurs

Checklist

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

@emkll emkll force-pushed the 5660-sshd-config-focal branch from 57af90a to 2609cce Compare December 10, 2020 20:18
@codecov-io
Copy link

codecov-io commented Dec 10, 2020

Codecov Report

Merging #5666 (3f4f6ac) into develop (b24cbaa) will decrease coverage by 4.05%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #5666      +/-   ##
===========================================
- Coverage    85.46%   81.41%   -4.06%     
===========================================
  Files           51       53       +2     
  Lines         3709     3965     +256     
  Branches       464      496      +32     
===========================================
+ Hits          3170     3228      +58     
- Misses         439      632     +193     
- Partials       100      105       +5     
Impacted Files Coverage Δ
securedrop/models.py 91.66% <0.00%> (-0.55%) ⬇️
securedrop/source_app/api.py 100.00% <0.00%> (ø)
securedrop/source_app/main.py 91.75% <0.00%> (ø)
securedrop/source_app/__init__.py 100.00% <0.00%> (ø)
securedrop/journalist_app/forms.py 100.00% <0.00%> (ø)
.../92fba0be98e9_added_organization_name_field_in_.py 66.66% <0.00%> (ø)
securedrop/loaddata.py 0.00% <0.00%> (ø)
securedrop/journalist_app/__init__.py 93.45% <0.00%> (+0.18%) ⬆️
securedrop/journalist_app/admin.py 92.92% <0.00%> (+0.25%) ⬆️
securedrop/source_app/utils.py 92.20% <0.00%> (+2.59%) ⬆️
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b24cbaa...3f4f6ac. Read the comment docs.

@emkll emkll marked this pull request as ready for review December 11, 2020 13:36
@zenmonkeykstop
Copy link
Contributor

One minor note, would it be worth making an accompanying docs change specifically in https://docs.securedrop.org/en/stable/servers.html?highlight=ssh-keygen to use ed25519 for new installs??

@zenmonkeykstop zenmonkeykstop self-assigned this Dec 14, 2020
@zenmonkeykstop
Copy link
Contributor

Tested on prod VMs (using existing instructions to generate an RSA SSH key on the workstation):

  • installation completes successfully
  • on subsequent playbook runs, or on attempts to connect via ssh <serverAlias> SSH fails with a host key mismatch, as an ECDSA host key was set up initially but an ED25519 key is now present.

Removing the known_hosts entries for app and mon and accepting the new host keys clears the error but it might be good to handle it more transparently.

# Cipher selection

Ciphers chacha20-poly1305@openssh.com,aes256-gcm@openssh.com,aes128-gcm@openssh.com,aes256-ctr,aes128-ctr
HostKeyAlgorithms ssh-ed25519,rsa-sha2-256,rsa-sha2-512
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like this ordering forces a new host key to be generated when the new config is applied. Since ECDSA is being removed altogether I don't see that there's a way around this in Xenial. It might make sense to have tailsconfig remove the old host key entries on the workstation if found, and alert the user that they can expect to see prompts on their first SSH/playbook run.

@emkll
Copy link
Contributor Author

emkll commented Dec 16, 2020

Since ECDSA is being removed altogether I don't see that there's a way around this in Xenial. It might make sense to have tailsconfig remove the old host key entries on the workstation if found, and alert the user that they can expect to see prompts on their first SSH/playbook run.

This might also be an issue in Focal, in a default configuration, the Host key is still ECDSA.

We could preserve the same host key preferences as the previous sshd_config: Given by default ssh is exposed Tor, which provides a second layer of authentication via authenticated onion services, or only exposed through a local network, we could simply preserve ECDSA / distribution defaults for host keys.

I will look into @zenmonkeykstop 's suggestion above, and use Ansible to

  1. Delete the server fingerprint in known_hosts, and have a user re-verify the fingerprints or
  2. Have Ansible edit known_hosts and add server fingerprints there

This might need to be done before the reboot action, so in the main Ansible playbook

@conorsch
Copy link
Contributor

We could preserve the same host key preferences as the previous sshd_config

Preserving as-is seems the most straightforward. Certainly for Xenial, there's no pressing need to change the config. For Focal, ideally we'd have a stronger default, but the gains from v2 -> v3 onions are pretty great already, and v3 is required for Focal. So I don't think we need to block on managing the host keys, as long as you're comfortable backing out on the defaults for now, @emkll.

It does strike me as worthwhile to consider hostkey management outside the context of config updates: on first install, the same hostkey for the IPv4 address must be re-confirmed for the ssh onions, which we needn't prompt admins to configure. Still, I don't think that work needs to go into this PR. We can file a separate issue for it, and address if it proves to be a sticking point during testing the backup-and-restore flow as part of the LTS upgrade path.

- Update supported algorthms
- Disable some agent forwarding and tunnelling options
- Annotate and reorder configuration for readability

Sources:
- https://github.com/dev-sec/ansible-ssh-hardening
- https://github.com/arthepsy/ssh-audit
ECDSA will be used by defaut for the client to authenticate the host.

Tor Onion Services will also provide another layer of authentication,
when using ssh over Tor.
@emkll emkll force-pushed the 5660-sshd-config-focal branch from d4f1643 to 54abdb2 Compare December 17, 2020 15:07
@emkll
Copy link
Contributor Author

emkll commented Jan 5, 2021

This is ready for re-review, however the docs update identified in #5666 (comment) is still pending


# Cipher selection

Ciphers chacha20-poly1305@openssh.com,aes256-gcm@openssh.com,aes128-gcm@openssh.com,aes256-ctr,aes128-ctr
Copy link
Contributor

@kushaldas kushaldas Jan 6, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we really need chacha20-poly1305@openssh.com?
chacha20 is specially being used from the cheap phones which does not have good hardware. And our users will use Tails on proper hardware to ssh.

- As pointed out by @kushaldas, chacha20-poly1305 mostly mobile-specific cipher, and while historically present in the sshd configuration for SecureDrop, is not necessary to support Debian-based ssh clients.

- The UsePrivilegeSeparation option has been deprecated in OpenSSH 7.5 [1]. UsePrivilegeSeparation has defaulted to 'sandbox' since 6.1 [2] and to 'yes' since 3.3 [3].

[1] https://www.openssh.com/txt/release-7.5
[2] https://www.openssh.com/txt/release-6.1
[3] https://www.openssh.com/txt/release-3.3
Copy link
Contributor

@kushaldas kushaldas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The current changes look nice to me, did not test it in a server yet.

Copy link
Contributor

@kushaldas kushaldas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • provided config is good / adheres to best practices
  • provided config is adequately annotated
  • provided config eliminates log message described in Update sshd config for Focal #5660
  • on an existing Xenial install the config is applied properly when an Ansible run occurs

I tested manually with ssh over Tor on both Focal and Xenial. Works as expected. Approving. ~ Will merge after standup.~ @conorsch do you think is it ready for merge?

@eloquence eloquence added this to the 1.8.0 milestone Feb 4, 2021
@conorsch conorsch dismissed zenmonkeykstop’s stale review February 11, 2021 19:19

Concerns raised have been addressed: specifically, the changes to hostkey algos were backed out

@conorsch conorsch merged commit fc6753e into develop Feb 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update sshd config for Focal
6 participants