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

[fix] Standardize sshd config #518

Merged
merged 33 commits into from Dec 9, 2018

Conversation

Projects
None yet
5 participants
@zamentur
Copy link
Contributor

zamentur commented Aug 26, 2018

The problem

Some instance don't use the sshd conf of YunoHost.

Solution

  • Remove the from_script file
  • make a migration to add a include instruction and ensure the ssh conf is managed
  • make a manual migration to ask user what he want do if the conf is different
  • implement in postinstall a way to keep the old sshd conf if the user ask for it during the install_script

PR Status

Ready
Related PR YunoHost/install_script#50

How to test

Run migrations in different case:

  • With /etc/yunohost/from_script
  • With different port or permitrootlogin at yes
  • ?

Validation

  • Principle agreement 0/2 :
  • Quick review 0/1 :
  • Simple test 0/1 :
  • Deep review 0/1 :

@zamentur zamentur referenced this pull request Aug 26, 2018

Merged

[fix] Ask user for keeping or not sshd config #50

0 of 4 tasks complete

@zamentur zamentur changed the title Fix standardize sshd config [fix] Standardize sshd config Aug 26, 2018

@zamentur zamentur removed the work needed label Aug 26, 2018

@zamentur zamentur referenced this pull request Aug 26, 2018

Closed

[enh] Improve ssh security #516

0 of 4 tasks complete

@zamentur zamentur requested a review from Psycojoker Aug 30, 2018

@zamentur zamentur referenced this pull request Aug 30, 2018

Merged

Synchronize root and admin password #527

0 of 4 tasks complete

@zamentur zamentur referenced this pull request Sep 20, 2018

Open

PasswordAuthentication yes in the upgrade script #21

0 of 5 tasks complete
ports = []
root_login = []
port_rgx = r'^[ \t]*Port[ \t]+(\d+)[ \t]*(?:#.*)?$'
root_rgx = r'^[ \t]*PermitRootLogin[ \t]([^# \t]*)[ \t]*(?:#.*)?$'

This comment has been minimized.

@zamentur

zamentur Sep 20, 2018

Author Contributor

This regex conflict with YunoHost-Apps/my_webapp_ynh#21

@alexAubin
Copy link
Member

alexAubin left a comment

Started reviewing, but not yet done ;D Just committing those comments for now

Show resolved Hide resolved data/hooks/conf_regen/03-ssh Outdated
Show resolved Hide resolved src/yunohost/tools.py Outdated

@alexAubin alexAubin added this to the 3.x milestone Oct 25, 2018

@alexAubin

This comment has been minimized.

Copy link
Member

alexAubin commented Nov 4, 2018

To be investigated : some people report to not be able to connect with the admin account (even with the UsePAM thing in sshd_config which imho is what's related to LDAP users?). Someone ended up installing libpam-ldapd on and OVH VPS.

https://forum.yunohost.org/t/impossible-connection-sur-le-compte-admin-en-ssh-ssh-could-not-resolve-hostname-service/6129/12

Edit: adressing this in #587

@alexAubin alexAubin modified the milestones: 3.x, 3.4.x Nov 27, 2018

@Psycojoker

This comment has been minimized.

Copy link
Member

Psycojoker commented Nov 28, 2018

I'm not 100% opposed to simply keeping root access allowed in any cases, we should just be aware that it definitely increases the attack surface so it's a trade off between security and comfort in debugging even in the case where LDAP is down :/

Indeed, but with the new password policy we should be a little bit safe no? At least this mitigate this quite a lot (and also "admin" is a very common user name that is directly tried so removing root is a bit limited on that regards :/)

I like the "local network only" idea but I'm not sure on how much setup it would be realistic.

@Josue-T

This comment has been minimized.

Copy link
Contributor

Josue-T commented Nov 28, 2018

My point of view that we should allow root login only with public key.

@alexAubin

This comment has been minimized.

Copy link
Member

alexAubin commented Nov 28, 2018

Hmokay but then we have a new problem which is to define an SSH key, which the admin should then keep once it'll be needed, and that needs to be generated somehow. Some people don't even have Linux to run a keygen on :/...

@Psycojoker

This comment has been minimized.

Copy link
Member

Psycojoker commented Nov 28, 2018

Indeed, that looks like a really complicated side-solution :x

@alexAubin

This comment has been minimized.

Copy link
Member

alexAubin commented Nov 28, 2018

So 90e542a re-enable root login on local network ... (192.168.. and 10.0.., dunno if that's enough)

Show resolved Hide resolved data/templates/ssh/sshd_config Outdated
@zamentur

This comment has been minimized.

Copy link
Contributor Author

zamentur commented Nov 30, 2018

A nice to have could ba a settings to specify which ip can connect as root, but i think it's not completely necessary for this PR.

@zamentur zamentur removed the work needed label Nov 30, 2018

@zamentur
Copy link
Contributor Author

zamentur left a comment

LGTM

alexAubin added some commits Dec 3, 2018

[enh] Clean + harden sshd config using Mozilla recommendation (#590)
* Clean sshd_config + harden using Mozilla recommendation
* Order of keys matter, ed25519 is recommended
@alexAubin
Copy link
Member

alexAubin left a comment

So on my side I'm done reviewing and testing this. I fixed quite a few bugs I found and it should be ready to deploy in production I think (or at least deployed in a testing release to be fully validated in real life conditions).

@alexAubin alexAubin force-pushed the fix-standardize-sshd-config branch from 0e5c83b to 3949333 Dec 3, 2018

@alexAubin

This comment has been minimized.

Copy link
Member

alexAubin commented Dec 4, 2018

Following today's meeting, we plan to merge this as is (still open to review tho)

Follow-up items to be adressed in other PR :

  • have a look at how the migration system behaves with several migrations (e.g. 2 migrations with disclaimer, what happens if you want to accept the first but not the second one ?)
  • possibly implement settings to tweak SSH conf easily

@alexAubin alexAubin merged commit cd7477a into stretch-unstable Dec 9, 2018

0 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
continuous-integration/travis-ci/push The Travis CI build is in progress
Details

@alexAubin alexAubin deleted the fix-standardize-sshd-config branch Dec 9, 2018

@nqb nqb referenced this pull request Dec 11, 2018

Merged

[enh] Improve security docs #876

@SylvainCecchetto

This comment has been minimized.

Copy link

SylvainCecchetto commented Feb 7, 2019

Hi, I don't if the SSH migration from YHN 3.4 come from this merge but anyway.
The migration 8 ssh_conf_managed_by_yunohost_step2 want to modify the SSH port to 22 if the user (like me) use another custom port.
I have no problem with that but I have a question: Is this migration script also check that the fail2ban SSHD config is also configuring to check on the 22 port.
Because the guide https://yunohost.org/#/security_fr says to modify the fail2ban configuration if we modify the SSH port.

Thank you ;-)

@alexAubin

This comment has been minimized.

Copy link
Member

alexAubin commented Feb 7, 2019

Nope it doesn't .. if you changed the SSH port, then right now that's up to you to reconfigure things properly :/

@SylvainCecchetto

This comment has been minimized.

Copy link

SylvainCecchetto commented Feb 7, 2019

Ok, thank you for the confirmation ;-)

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