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

SSH global config #1075

Merged
merged 23 commits into from
May 17, 2023
Merged

SSH global config #1075

merged 23 commits into from
May 17, 2023

Conversation

ycoheNvidia
Copy link
Contributor

@ycoheNvidia ycoheNvidia commented Aug 31, 2022

```
SSH_GLOBAL:{
policies:{
"login-attempts": {{num}}
Copy link
Contributor

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

done

| 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 |
Copy link
Contributor

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

Copy link
Contributor

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.

}
```
#### 1.10.2. ConfigDB schemas
```
Copy link
Contributor

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this required?

|----------------|---------------------------------------------------------------------------|-------------------------|------------------|
| 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 |
Copy link
Contributor

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

done

Copy link
Contributor Author

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 Show resolved Hide resolved

#### 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):
Copy link
Contributor

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.

Copy link
Contributor

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?

doc/ssh_config.md Outdated Show resolved Hide resolved
Copy link
Contributor

@davidpil2002 davidpil2002 left a 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

@liat-grozovik
Copy link
Collaborator

@qiluo-msft @dgsudharsan any further comments to this HLD?

doc/ssh_config/ssh_config.md Show resolved Hide resolved
doc/ssh_config/ssh_config.md Show resolved Hide resolved
doc/ssh_config/ssh_config.md Outdated Show resolved Hide resolved
doc/ssh_config/ssh_config.md Outdated Show resolved Hide resolved
doc/ssh_config/ssh_config.md Outdated Show resolved Hide resolved
Removed fields that will not be supported in the first phase, added clarification for ssh ports - we want to allow multiple ports configuration
Copy link
Contributor

@dgsudharsan dgsudharsan left a 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?

@Yarden-Z
Copy link
Contributor

@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.

@ycoheNvidia
Copy link
Contributor Author

ycoheNvidia commented Jan 10, 2023

@liat-grozovik
Copy link
Collaborator

@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.

@liat-grozovik
Copy link
Collaborator

@ycoheNvidia could you please check the last PR status indication in the PR description? seems not accurate/missing

@ycoheNvidia
Copy link
Contributor Author

@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

@liat-grozovik
Copy link
Collaborator

@dgsudharsan @qiluo-msft kindly provide your approval/comments.
this HLD and the code PRs are available for quite some time and we wish to move forward and have them merged to master.

@dgsudharsan
Copy link
Contributor

@dgsudharsan @qiluo-msft kindly provide your approval/comments. this HLD and the code PRs are available for quite some time and we wish to move forward and have them merged to master.

Still one comment needs to be addressed
#1075 (comment)

Copy link
Contributor

@qiluo-msft qiluo-msft left a comment

Choose a reason for hiding this comment

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

Updated comments.

@liat-grozovik
Copy link
Collaborator

@qiluo-msft could you please check if your comments were published? I am not sure I see them.

removed [ ] from ports in ConfigDb json
@liat-grozovik
Copy link
Collaborator

@ycoheNvidia please confirm all comments were addressed. if not, please share a plan.

@liat-grozovik
Copy link
Collaborator

@qiluo-msft @dgsudharsan kindly reminder to review the changes done following review feedback.
Code PRs are available as well.

dgsudharsan
dgsudharsan previously approved these changes Mar 8, 2023
@ycoheNvidia
Copy link
Contributor Author

As comments

this was already resolved

updated ports regular expression to allow only ports numbers from 1 to 65536
@liat-grozovik
Copy link
Collaborator

@qiluo-msft kindly reminder to review/approve the HLD first.

@liat-grozovik liat-grozovik merged commit 17dcf76 into sonic-net:master May 17, 2023
liat-grozovik pushed a commit to sonic-net/sonic-host-services that referenced this pull request Jun 27, 2023
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
@liat-grozovik
Copy link
Collaborator

@ycoheNvidia all the PRs related to this feature should be placed in the table of the PR description.
Please add it for better tracking.

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.

@liat-grozovik
Copy link
Collaborator

@ycoheNvidia all the PRs related to this feature should be placed in the table of the PR description. Please add it for better tracking.

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.
As of the second one i dont find why it is shown as not avaialble as i do see it sonic-net/sonic-host-services#32
@zhangyanzhao do you have any idea how to sort it out?

@skg-net
Copy link
Member

skg-net commented Feb 6, 2024

@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
Thanks

@ycoheNvidia
Copy link
Contributor Author

@skg-net this feature was GA level when approved

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants