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

Remove unused sshd config options #3980

Merged
merged 2 commits into from Dec 10, 2018

Conversation

Projects
None yet
3 participants
@emkll
Copy link
Contributor

emkll commented Dec 10, 2018

Status

Ready for review

Description of Changes

Fixes #3979

Disables ChallengeResponseAuthentication, UsePam, and PasswordAuthentication in sshd config.
Updates the builder hash (security updates were required for passing tests).

Testing

Clean install scenario

  • Ensure sshd configuration is as described (see tests)
  • Ensure you can log in via ssh, through either tor or local network
  • Ensure ssh access via Ansible works after reboot/bouncing sshd service (e.g. ./securedrop-admin backup)

Upgrade install scenario

Use upgrade testing guide https://docs.securedrop.org/en/release-0.10.0/development/upgrade_testing.html or manually perform a clean install on 0.10.0 and install debs build from this branch. Confirm the following:

  • Debs install successfully
  • Ensure sshd configuration is as described (see tests, perform additional visual review of /etc/ssh/sshd_config to ensure is is identical to the one provisioned in clean installs of this branch.
  • Ensure you can log in via ssh, through either tor or local network
  • Ensure ssh access via Ansible works after reboot/bouncing sshd service (e.g. ./securedrop-admin backup)

Deployment

Any special considerations for deployment? Consider both:

  1. Upgrading existing production instances.

Existing production will see their sshd configuration change by way of the securedrop-config package postinst script as part of the next SecureDrop release.

  1. New installs.

Sshd config for new installs will be provisioned at install-time via Ansible.

Checklist

If you made changes to the system configuration:

If you made non-trivial code changes:

  • I have written a test plan and validated it for this PR
Disable password authentication for sshd
UsePAM yes will override PasswordAuthentication set to no, so UsePAM
must be set to no.
For the new install case, this is handled by the Ansible logic. For
existing installs, the config is modified via securedrop-config postinst
hook.

@emkll emkll requested review from conorsch and msheiny as code owners Dec 10, 2018

@emkll emkll referenced this pull request Dec 10, 2018

Merged

[0.11.0] Remove unused sshd config options #3981

3 of 5 tasks complete

@emkll emkll added this to the 0.11.0 milestone Dec 10, 2018

@kushaldas
Copy link
Contributor

kushaldas left a comment

On manual upgrade testing:

  • Ensure sshd configuration is as described (see tests, perform additional visual review of /etc/ssh/sshd_config to ensure is is identical to the one provisioned in clean installs of this branch.
  • Ensure you can log in via ssh, through local network
  • Ensure ssh access via Ansible works after reboot/bouncing sshd service (e.g. ./securedrop-admin backup)
grep -qF "PasswordAuthentication no" /etc/ssh/sshd_config || echo "PasswordAuthentication no" >> /etc/ssh/sshd_config
sed -i "/^UsePAM\ /s/\ .*/\ no/" /etc/ssh/sshd_config
sed -i "/^ChallengeResponseAuthentication\ /s/\ .*/\ no/" /etc/ssh/sshd_config
service ssh restart

This comment has been minimized.

@redshiftzero

redshiftzero Dec 10, 2018

Member

sorry, just noticed this - is using service ssh restart in postinst the right practice here? https://www.debian.org/doc/debian-policy/ch-opersys.html#running-initscripts says we should be using invoke-rc.d

(note this did work for me as expected during testing, just wondering if we should change it to the recommendation)

This comment has been minimized.

@emkll

emkll Dec 10, 2018

Contributor

Good point @redshiftzero , thanks for the link. From my understanding of that document, the intent of the guideline is for packagers to be mindful of other services and dependencies of these services on the host, to ensure all services change state graciously. Given that the state of services on SecureDrop instances is known (all SD instances should have the same services running), this seems less important in this context, but happy to make the change. Note that we do already use this method for apache: https://github.com/freedomofpress/securedrop/blob/develop/install_files/securedrop-app-code/DEBIAN/postinst#L119

This comment has been minimized.

@redshiftzero

redshiftzero Dec 10, 2018

Member

ok thanks for the explanation, since we (~)know the state of running SecureDrops, let's leave as is

@redshiftzero
Copy link
Member

redshiftzero left a comment

Fresh install testing:

SSH config is as expected
can log in via SSH
cannot log in via SSH with password only

But: I do get a local test failure re: upgrades being needed when doing make build-debs

Update builder image hash
Builder image needed updates
@emkll

This comment has been minimized.

Copy link
Contributor

emkll commented Dec 10, 2018

Confirmed, tests were failing for me locally as well (they were passing only a few hours ago). I've pushed an updated to the container image (and changed the hash in this PR). Deb build tests should be passing now.

@redshiftzero
Copy link
Member

redshiftzero left a comment

confirmed all tests passing now on make build-debs

@emkll emkll merged commit ab339a5 into develop Dec 10, 2018

5 checks passed

ci/circleci: admin-tests Your tests passed on CircleCI!
Details
ci/circleci: lint Your tests passed on CircleCI!
Details
ci/circleci: staging-test-with-rebase Your tests passed on CircleCI!
Details
ci/circleci: tests Your tests passed on CircleCI!
Details
ci/circleci: updater-gui-tests Your tests passed on CircleCI!
Details

@emkll emkll deleted the sshd-config branch Dec 10, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment