-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
SSH global config #1075
SSH global config #1075
Conversation
doc/ssh_config.md
Outdated
``` | ||
SSH_GLOBAL:{ | ||
policies:{ | ||
"login-attempts": {{num}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe it should be underscore instead of hyphen in logic-attempts. Can you please correct this in other variables too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
doc/ssh_config.md
Outdated
| Policy | Action | Param values | Default OS value | | ||
|----------------|---------------------------------------------------------------------------|-------------------------|------------------| | ||
| login attempts | Number of attempts to try to log in before rejecting the session | 3-100 | 6 | | ||
| login timeout | SSH session timeout | 1-600 (secs) | 120 | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should there be an option to disable the timeout? I believe it maps to LoginGraceTime which when specified as 0 disables timeout
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want to disable this? Since this is a security item and we should retain it.
doc/ssh_config.md
Outdated
} | ||
``` | ||
#### 1.10.2. ConfigDB schemas | ||
``` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we have another table which maps the config_db field to the ssh config option?
e.g. TCP_FORWARDING - AllowTcpForwarding
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this required?
doc/ssh_config.md
Outdated
|----------------|---------------------------------------------------------------------------|-------------------------|------------------| | ||
| login attempts | Number of attempts to try to log in before rejecting the session | 3-100 | 6 | | ||
| login timeout | SSH session timeout | 1-600 (secs) | 120 | | ||
| ports | Port number for SSH | 1-65535 | 22 | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be only 'port' instead of 'ports' since we can have only one ssh port?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we want to support multiple ports
doc/ssh_config.md
Outdated
|
||
#### 1.7.2 <a name='SSH global policies'></a>SSH global policies | ||
|
||
We want to enable configuring the following policies, with default values are taken from OS (Debian): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please mention if the current SONiC defaults match with the defaults that you are proposing. This will highlight that new design is backward compatible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Current design should match debian standards, if these are in conflict - do we still want to take SONiC basic defaults?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fix typo in HLD file according comments
@qiluo-msft @dgsudharsan any further comments to this HLD? |
Removed fields that will not be supported in the first phase, added clarification for ssh ports - we want to allow multiple ports configuration
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ycoheNvidia What tcp_forwarding and x11_forwarding were removed?
Due to security concerns from SONiC in adding these commands to the redis db - these options were removed from the commitment. |
@dgsudharsan , @qiluo-msft as you were previously commenting on this feature, could you please review and if no additional feedback appreciate if you can review so we can merge. |
@ycoheNvidia could you please check the last PR status indication in the PR description? seems not accurate/missing |
It doesn't seem to be working for this specific repository |
@dgsudharsan @qiluo-msft kindly provide your approval/comments. |
Still one comment needs to be addressed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated comments.
@qiluo-msft could you please check if your comments were published? I am not sure I see them. |
removed [ ] from ports in ConfigDb json
@ycoheNvidia please confirm all comments were addressed. if not, please share a plan. |
@qiluo-msft @dgsudharsan kindly reminder to review the changes done following review feedback. |
this was already resolved |
updated ports regular expression to allow only ports numbers from 1 to 65536
@qiluo-msft kindly reminder to review/approve the HLD first. |
HLD in sonic-net/SONiC#1075 - Why I did it Implemented ssh configurations - How I did it Added ssh config table in configDB, once changed - hostcfgd will change the relevant OS files (sshd_config) - How to verify it Tests in added in this PR. User can change relevant configs in configDB such as ports, and see sshd port was modified - Link to config_db schema for YANG module changes https://github.com/ycoheNvidia/SONiC/blob/ssh_config/doc/ssh_config/ssh_config.md
@ycoheNvidia all the PRs related to this feature should be placed in the table of the PR description. As for asking to have the feature as planned for 202305, once all the PRs of the features are merged, please go one by one and add the label for 202305. |
FYI, i update the title of the first PR to align with the PR name. |
@ycoheNvidia Can you please update the Quality Metric (Alpha/Beta/GA) for the feature either in this PR comments or in HLD itself based on https://github.com/sonic-net/SONiC/blob/master/doc/SONiC%20feature%20quality%20definition.md |
@skg-net this feature was GA level when approved |
Added HLD of ssh global config