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

[enh] Improve SSHd config / ciphers #436

Closed
wants to merge 3 commits into from

Conversation

kitoy30
Copy link
Contributor

@kitoy30 kitoy30 commented Feb 15, 2018

Recomendation from this link:
https://stribika.github.io/2015/01/04/secure-secure-shell.html
Use this tool for test:
https://github.com/arthepsy/ssh-audit

The problem

...

Solution

...

PR Status

...

How to test

...

Validation

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

@@ -9,13 +9,20 @@ ListenAddress 0.0.0.0
Protocol 2
# HostKeys for protocol version 2
HostKey /etc/ssh/ssh_host_rsa_key
HostKey /etc/ssh/ssh_host_dsa_key
HostKey /etc/ssh/ssh_host_ed25519_key
#HostKey /etc/ssh/ssh_host_dsa_key
Copy link
Member

Choose a reason for hiding this comment

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

isn't this modification going to trigger a warning like "the key of this server has changed"?

Copy link
Member

Choose a reason for hiding this comment

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

+1 ... we really need to test this carefully

Copy link
Member

Choose a reason for hiding this comment

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

Could break some people using external automated tools to connect to their server (borg, nagios?, may be personnal cron)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was a bad idea to comments this lines. It's true sorry 👍 . This will generate a warning for an existing configuration.

@Psycojoker
Copy link
Member

Thanks for this PR ❤️

@alexAubin alexAubin changed the title update sshd_config [enh] Improve SSHd config / ciphers May 10, 2018
Copy link
Member

@alexAubin alexAubin left a comment

Choose a reason for hiding this comment

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

Those are probably good improvements, but this is a really touchy topic. I expect that, for the majority of instances, SSH is the only way to "really" access the server and be able to edit files. So if somehow the configuration is messed up or clients have incompatibilities with this, users might get locked out (though they might still have access to the webadmin but that's no help here). So we really need some extensive testing on this 😕 ...

Also we should really consider taking care of https://dev.yunohost.org/issues/1110 , otherwise updating this file will only affect people who installed from the x86 ISO ...

# Algorimthm limitation
KexAlgorithms curve25519-sha256@libssh.org,diffie-hellman-group-exchange-sha256
Ciphers chacha20-poly1305@openssh.com,aes256-gcm@openssh.com,aes128-gcm@openssh.com,aes256-ctr,aes192-ctr,aes128-ctr
MACs hmac-sha2-512-etm@openssh.com,hmac-sha2-256-etm@openssh.com,umac-128-etm@openssh.com,hmac-sha2-512,hmac-sha2-256,umac-128@openssh.com
Copy link
Member

Choose a reason for hiding this comment

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

Please add a comment to specify the recommendation where those are coming from

Copy link
Member

Choose a reason for hiding this comment

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

I suggest to put nothing to follow ciphers from debian / Openssh-server default conf, we can't be sure we will think to update this ciphers list

#Privilege Separation is turned on for security
UsePrivilegeSeparation yes

# Lifetime and size of ephemeral version 1 server key
KeyRegenerationInterval 3600
ServerKeyBits 768
ServerKeyBits 1024
Copy link
Member

Choose a reason for hiding this comment

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

Is this going to trigger a key regeneration ?

Copy link
Member

Choose a reason for hiding this comment

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

In Protocol 2 this instruction is ignored

@@ -9,13 +9,20 @@ ListenAddress 0.0.0.0
Protocol 2
# HostKeys for protocol version 2
HostKey /etc/ssh/ssh_host_rsa_key
HostKey /etc/ssh/ssh_host_dsa_key
HostKey /etc/ssh/ssh_host_ed25519_key
#HostKey /etc/ssh/ssh_host_dsa_key
Copy link
Member

Choose a reason for hiding this comment

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

+1 ... we really need to test this carefully

Copy link
Member

@frju365 frju365 left a comment

Choose a reason for hiding this comment

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

Everything is ok for me. For the documentation/source of the cyphers and the conf, it's surely here : https://cipherli.st/
Thanks for the PR.

@frju365
Copy link
Member

frju365 commented May 10, 2018

For info : "The option ServerKeyBits specifies how many bits to use in the server key. These bits are used when the daemon starts to generate its RSA key."
cf. http://tldp.org/LDP/solrhe/Securing-Optimizing-Linux-RH-Edition-v1.3/chap15sec122.html

@alexAubin alexAubin added this to the 3.x milestone Jun 13, 2018
@alexAubin alexAubin changed the base branch from unstable to stretch-unstable June 17, 2018 02:10
@@ -28,7 +35,7 @@ StrictModes yes

RSAAuthentication yes
PubkeyAuthentication yes
#AuthorizedKeysFile %h/.ssh/authorized_keys
AuthorizedKeysFile %h/.ssh/authorized_keys
Copy link
Member

Choose a reason for hiding this comment

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

%h/.ssh/authorized_keys is the default value, so this line is not needed

@@ -9,13 +9,20 @@ ListenAddress 0.0.0.0
Protocol 2
# HostKeys for protocol version 2
HostKey /etc/ssh/ssh_host_rsa_key
HostKey /etc/ssh/ssh_host_ed25519_key
Copy link
Member

Choose a reason for hiding this comment

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

OK

@alexAubin
Copy link
Member

Sorry, closing this
It's superseeded by #590 and more importantly by #518 to ensure a smooth migration

@alexAubin alexAubin closed this Nov 28, 2018
@alexAubin alexAubin removed this from the 3.x milestone Nov 28, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants