SSH hardening configuration options #238
Conversation
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
The SSH configuration does not contain many of the hardening requirements by the various standards bodies. This adds support for: * password_authentication - ability to disable password auth * permit_root_login - ability to prevent root logins * ciphers - ability to specify available ciphers * kex_algorithms - ability to specify key exchange algorithms * macs - ability to specify macs Signed-off-by: Brad House <bhouse@nexthop.ai>
bhouse-nexthop
force-pushed
the
bhouse-nexthop/ssh-config
branch
from
19a049a to
84f7678
Compare
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
There was a problem hiding this comment.
Pull Request Overview
This PR enhances the SSH configuration options to support hardening requirements by adding new configuration vectors for password authentication, root login, ciphers, key exchange algorithms, and MACs. Key changes include:
- Updates to test vectors and sample outputs for various SSH hardening options.
- Addition of new test cases in hostcfgd tests to validate the updated SSH configuration.
- Modifications in the SshServer code to support the new configuration keys and proper value conversion.
Reviewed Changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| tests/hostcfgd/test_ssh_server_vectors.py | Added new test vectors for password_authentication, permit_root_login, ciphers, kex_algorithms, and macs. |
| tests/hostcfgd/sample_output/*/sshd_config | Updated sample SSHD configuration outputs to reflect the new hardening options. |
| tests/hostcfgd/hostcfgd_ssh_server_test.py | Added new test cases corresponding to each new SSH hardening option. |
| scripts/hostcfgd | Modified the SSH configuration processing to support additional options and value conversions. |
|
|
||
| if key in SSH_INT_VALUES and (int(value) < SSH_MIN_VALUES.get(key, 65535) or | ||
| SSH_MAX_VALUES.get(key, -1) < int(value)): | ||
| syslog.syslog(syslog.LOG_ERR, "Ssh {} {} out of range".format(key, value)) |
There was a problem hiding this comment.
The out-of-range check logs an error but does not stop further processing. Consider adding an early return or skipping further processing to prevent invalid configuration values from being applied.
| syslog.syslog(syslog.LOG_ERR, "Ssh {} {} out of range".format(key, value)) | |
| syslog.syslog(syslog.LOG_ERR, "Ssh {} {} out of range".format(key, value)) | |
| continue |
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
May I know the reject reason and also update to code comments?
If out of range is not an error, maybe write warning log here.
#Closed
There was a problem hiding this comment.
Actually, may be i did miss the 'continue' clause addition here after looking at the code not the diff.
| #IgnoreRhosts yes | ||
|
|
||
| # To disable tunneled clear text passwords, change to no here! | ||
| PasswordAuthentication yes |
There was a problem hiding this comment.
Is this new feature backward-compatible? If the new config is not used, the default value should ensure sshd_config same as old behavior, so it does not break existing production usage easily. We can keep this file unchanged to test backward-compatible. If you need, you can add extra testcases (test output files).
There was a problem hiding this comment.
This is 100% backwards compatible. Not a single default is changed. The file you are commenting on is the result file for a test case that is changing every one of the configuration knobs. Previously password authentication couldn't be toggled, but now it can, so you see it being toggled by this test case.
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
@bhouse-nexthop it looks like there is a test failure. Could you pls check? =========================== short test summary info ============================ |
|
looks like the input file was changed by a different PR, I'll update to match |
7 participants
The SSH configuration does not contain many of the hardening requirements by the various standards bodies. This adds support for:
Depends on sonic-net/sonic-buildimage#22308
Fixes sonic-net/sonic-buildimage#22309