Adding the HLD for SONiC reset local users' passwords #1577

azmy98 · 2024-01-04T10:41:59Z

This document provides general information about reset local users' passwords during init implementation in SONiC

The scope of this document is to cover definition, design and implementation of SONiC reset local users' passwords during init feature.

Submodule	PR Title	Status
sonic-buildimage	Feature YANG Model & SystemD service
sonic-utilities	CLI commands
sonic-swss-common	Config DB Schema
sonic-platform-common	Feature Base Class Implementation

doc/SONiC_long_reset_button_press_hld.md

dgsudharsan · 2024-01-04T22:52:59Z

doc/SONiC_long_reset_button_press_hld.md

+    LongRebootPress().start()
+```
+
+The feature is added to the init flow as long as the we set the ```ENABLE_LONG_RESET_BUTTON_PRESS``` in ```rules/config```


If we set ENABLE_LONG_RESET_BUTTON_PRESS = n during compilation, will it avoid adding the long-boot service to SONiC?

yes, same as we do for:
CHANGE_DEFAULT_PASSWORD ?= y

dgsudharsan · 2024-01-04T22:54:48Z

doc/SONiC_long_reset_button_press_hld.md

+
+# Warmboot and Fastboot Design Impact
+
+The feature will not have any impact on fast reboot or warmboot.


There needs to be explicit check in the script not to start on warmboot/fastboot. Please refer to https://github.com/sonic-net/sonic-host-services/blob/722b7962f95e8d602a9a60e77356826e9f68891c/scripts/hostcfgd#L1506

This is interesting. The system should be able to determine if this reboot is either a physical press on the switch or a warm/fast reboot by calling the get_reboot_cause function that we are use in the script. So, I don't think we need it. @dgsudharsan, can you confirm?

As part of the meeting discussion, I added the changes to support warmboot/fastboot.

zhangyanzhao · 2024-01-09T16:17:36Z

community review recording https://zoom.us/rec/share/Mmpq0viHcC2J7QQnHzszlZX4iPR7VqYnXj4u3r7Rzavs4nPG1SImgBb2FK-fL--K.12p00bKsBDn_RCO9

venkatmahalingam · 2024-01-09T16:48:54Z

doc/SONiC_long_reset_button_press_hld.md

+
+            description "LONG_RESET_BUTTON part of config_db.json";
+
+            container STATE {


Suggest to avoid STATE name for the CONFIG_DB data as it looks like a read-only data, please use STATUS.

@venkatmahalingam , just to be aligned, do we need to change the leaf as well? or just the container name?
because I see in other places 'state' as a command.

venkatmahalingam · 2024-01-09T16:52:49Z

doc/SONiC_long_reset_button_press_hld.md

+
+The long reset button press feature implementation shall support the following:
+
+    1. On long reboot press, the OS will restore the local users' configurations, by deleting non-default users and restoring default passwords for default users and expiring  these passwords due to security conecerns.


Is the use-case mainly to reset the password of the default user? if no, why cant we login to the switch with the default user to delete the non-default users explicitly?

yes, I changed the feature name accordingly. the fearture will be reset local users for the default users and in the default implementation this will be triggered by a long reset button

venkatmahalingam · 2024-01-09T16:54:51Z

doc/SONiC_long_reset_button_press_hld.md

+    4. On short reset button press, the current init flow should not be affected and should boot normally.
+    5. Feature should be enabled or diabled by setting compliation flag to true or false.
+    6. Feature can be disabled and enabled using CLI command.
+    7. The feature is enabled by default.


Not sure above statement is true, I think, it's disabled by default, please clarify.

changed to disabled :)

dgsudharsan · 2024-02-10T00:12:28Z

doc/SONiC_local_users_password_reset_hld.md

+
+This feature will be a built-in SONiC feature. There will be three main files to consider in this feature:
+
+1. ```src/sonic-platform-common/sonic_platform_base/reset_local_users_passwords_base.py``` - The default behavior implementation will reside in this file, including when to trigger the feature and how to reset the local users' configurations.


Why do we want to name the plugin reset_local_users_passwords? If the trigger of this flow is going to be only long press, I recommend renaming it reset_button.py or something similar.

dgsudharsan · 2024-02-10T00:13:30Z

doc/SONiC_local_users_password_reset_hld.md

+This feature will be a built-in SONiC feature. There will be three main files to consider in this feature:
+
+1. ```src/sonic-platform-common/sonic_platform_base/reset_local_users_passwords_base.py``` - The default behavior implementation will reside in this file, including when to trigger the feature and how to reset the local users' configurations.
+2. ```platform/<vendor-path>/sonic_platform/reset_local_users_passwords.py``` - The vendor specifc implementation of the feature, each vendor can decide what is the trigger cause to start the functionality and how it is implemented.


Same here. If the trigger is going to pressing reset button alone, lets rename it accordingly.

dgsudharsan · 2024-02-10T00:17:05Z

doc/SONiC_local_users_password_reset_hld.md

+2. ```platform/<vendor-path>/sonic_platform/reset_local_users_passwords.py``` - The vendor specifc implementation of the feature, each vendor can decide what is the trigger cause to start the functionality and how it is implemented.
+3. ```src/sonic-host-services/scripts/reset-local-users-passwords``` - The python file that will be called on service start during init that imports the vendor's specific implementation.
+
+The default behavior will delete non-default users and restore the original passwords of the default users and expire them based on the content of the file of ```/etc/sonic/default_users.json``` on long reboot press.


Do we address the security concern raised in the sonic community meeting? This behavior will reset the passwords alone and not data or configuration and thus its a security concern if someone resets and logs in with default password and obtains all the data. Should we consider resetting configuration and other important files?

This is a recovery mechanism, if users would like to use this in order to recover their own passwords (which they forgot), without deleting the data on top of the switch - this is the way to do so.
Customers that are using AAA methods will not be affected by this - so let's reduce this to local users only.
In case of a local user, if we have a PW reset, then the default users will be available and might pose a security risk. This is why customers that do not want a recovery mechanism should disable this functionality.
There is indeed a tradeoff between functionality and user experience to security in this feature, but it is manageable.
Also, in order to execute this attack and attack will have to have physical access to the switch, and only after a reboot (which can be tracked since the system will experience downtime)

I think the concern was raised by @lguohan . I think another scenario is when a switch is RMA, the switch's data could be easily accessed if the long reset was enabled. This might be the motivation for the security concern. @lguohan Can you please see if Yarden's comment addresses your concern?

dgsudharsan · 2024-02-10T00:20:06Z

doc/SONiC_local_users_password_reset_hld.md

+
+# Warmboot and Fastboot Design Impact
+
+The feature will not have any impact on fast reboot or warmboot.


This daemon itself shouldn't run when the boot type is warmboot or fastboot as the reset button press results only in cold boot. Warmboot and fastboot have strict timing requirements and since this flow will not happen during warm or fastreboot, lets make sure not to run the daemon.
Please update the same in HLD as well

I will update it also in the requirements

dgsudharsan · 2024-02-10T00:22:16Z

doc/SONiC_local_users_password_reset_hld.md

+
+
+```
+class LocalUsersConfigurationReset:


I would recommend not to add code into the HLD. the HLD should contain design and flow diagrams. The code is subject to change and will lead to confusion in the future.

dgsudharsan · 2024-02-10T00:24:28Z

doc/SONiC_local_users_password_reset_hld.md

+### Show command
+**The following command display long-reset-button-press  configuration:**
+```bash
+root@sonic:/home/admin# show local-users-passwords-reset


I would prefer the show command to show the last reboot reset button state, rather than just showing the configuration. Can you please publish the last reboot reset button state to state_db and use it to display in show, so that user will be aware if a long press is detected after the current boot?

I think the user can know that from the reboot-cause, why do we need to have double commands for it.

If show reboot-cause can cover it, I am fine with it

qiluo-msft · 2024-04-09T17:59:07Z

@liuh-80 Could you help review?

doc/SONiC_local_users_password_reset_hld.md

liat-grozovik · 2024-04-17T10:13:00Z

@azmy98 when do you expect to share the code PRs?

liat-grozovik · 2024-04-25T15:29:46Z

it was decided to remove this HLD for SONiC. PR is closed.

liat-grozovik · 2024-05-22T07:00:17Z

I see no further comments and all seems to be addressed.
@liuh-80 @venkatmahalingam FYI the PR is going to be merged soon.

Appreciate if you can also review the code PRs and provide your feedback.

liat-grozovik · 2024-05-22T07:02:34Z

@venkatmahalingam would you mind to provide code PR review feedback as well?

zhangyanzhao · 2024-05-22T16:09:45Z

YANG PR will be reviewed in the YANG workgroup meeting. Other PR review is on-going.

zhangyanzhao · 2024-05-22T16:11:22Z

@liat-grozovik will bring this to TSC to get guidance how to handle such PR which needs 2nd reviewers.

fastiuk · 2024-08-13T22:59:50Z

@liat-grozovik / @zhangyanzhao can we merge this PR?

fastiuk · 2024-10-08T15:11:59Z

@liuh-80 could you please merge it?

fastiuk · 2024-10-14T12:10:21Z

@liuh-80 / @zhangyanzhao / @liat-grozovik could you please merge it? It was approved a month ago

zhangyanzhao · 2024-12-10T05:21:34Z

code PRs are not merged, move to backlog

azmy98 force-pushed the dev-long-reset-button-hld branch 15 times, most recently from a166dfa to d98771f Compare January 4, 2024 12:47

dgsudharsan suggested changes Jan 4, 2024

View reviewed changes

azmy98 force-pushed the dev-long-reset-button-hld branch 10 times, most recently from 44d3aac to caa4ddb Compare January 9, 2024 13:59

venkatmahalingam reviewed Jan 9, 2024

View reviewed changes

azmy98 force-pushed the dev-long-reset-button-hld branch from 5ae9a60 to 939c65e Compare February 7, 2024 11:10

dgsudharsan suggested changes Feb 10, 2024

View reviewed changes

Adding the HLD for SONiC long reset button press

f57585c

azmy98 force-pushed the dev-long-reset-button-hld branch from 939c65e to f57585c Compare February 18, 2024 14:37

dgsudharsan approved these changes Feb 28, 2024

View reviewed changes

qiluo-msft requested review from liuh-80 and maipbui March 18, 2024 07:06

liuh-80 reviewed Apr 16, 2024

View reviewed changes

doc/SONiC_local_users_password_reset_hld.md Show resolved Hide resolved

liat-grozovik assigned orenreiss Apr 17, 2024

liat-grozovik assigned azmy98 Apr 17, 2024

liat-grozovik closed this Apr 25, 2024

This was referenced May 2, 2024

Added CLI commands to configure Local Users' Passwords Reset feature sonic-net/sonic-utilities#3303

Closed

Added LOCAL_USERS_PASSWORDS_RESET table to ConfigDB sonic-net/sonic-swss-common#874

Open

liat-grozovik reopened this May 2, 2024

liuh-80 approved these changes Sep 9, 2024

View reviewed changes

liat-grozovik merged commit e05fc6f into sonic-net:master Oct 14, 2024
1 check passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding the HLD for SONiC reset local users' passwords #1577

Adding the HLD for SONiC reset local users' passwords #1577

azmy98 commented Jan 4, 2024 •

edited

Loading

dgsudharsan Jan 4, 2024

azmy98 Jan 9, 2024

dgsudharsan Jan 4, 2024

azmy98 Jan 9, 2024

azmy98 Feb 7, 2024

zhangyanzhao commented Jan 9, 2024

venkatmahalingam Jan 9, 2024

azmy98 Feb 7, 2024 •

edited

Loading

venkatmahalingam Jan 9, 2024

azmy98 Feb 7, 2024

venkatmahalingam Jan 9, 2024

azmy98 Feb 7, 2024

dgsudharsan Feb 10, 2024

dgsudharsan Feb 10, 2024

dgsudharsan Feb 10, 2024

Yarden-Z Feb 18, 2024

dgsudharsan Feb 28, 2024

dgsudharsan Feb 10, 2024

azmy98 Feb 18, 2024

dgsudharsan Feb 10, 2024

dgsudharsan Feb 10, 2024

azmy98 Feb 18, 2024

dgsudharsan Feb 28, 2024

qiluo-msft commented Apr 9, 2024

liat-grozovik commented Apr 17, 2024

liat-grozovik commented Apr 25, 2024

liat-grozovik commented May 22, 2024

liat-grozovik commented May 22, 2024

zhangyanzhao commented May 22, 2024

zhangyanzhao commented May 22, 2024

fastiuk commented Aug 13, 2024

fastiuk commented Oct 8, 2024

fastiuk commented Oct 14, 2024

zhangyanzhao commented Dec 10, 2024


		# Warmboot and Fastboot Design Impact

		The feature will not have any impact on fast reboot or warmboot.


		description "LONG_RESET_BUTTON part of config_db.json";

		container STATE {


		The long reset button press feature implementation shall support the following:

		1. On long reboot press, the OS will restore the local users' configurations, by deleting non-default users and restoring default passwords for default users and expiring these passwords due to security conecerns.


		This feature will be a built-in SONiC feature. There will be three main files to consider in this feature:

		1. ```src/sonic-platform-common/sonic_platform_base/reset_local_users_passwords_base.py``` - The default behavior implementation will reside in this file, including when to trigger the feature and how to reset the local users' configurations.



		```
		class LocalUsersConfigurationReset:

Adding the HLD for SONiC reset local users' passwords #1577

Adding the HLD for SONiC reset local users' passwords #1577

Conversation

azmy98 commented Jan 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhangyanzhao commented Jan 9, 2024

Choose a reason for hiding this comment

azmy98 Feb 7, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qiluo-msft commented Apr 9, 2024

liat-grozovik commented Apr 17, 2024

liat-grozovik commented Apr 25, 2024

liat-grozovik commented May 22, 2024

liat-grozovik commented May 22, 2024

zhangyanzhao commented May 22, 2024

zhangyanzhao commented May 22, 2024

fastiuk commented Aug 13, 2024

fastiuk commented Oct 8, 2024

fastiuk commented Oct 14, 2024

zhangyanzhao commented Dec 10, 2024

azmy98 commented Jan 4, 2024 •

edited

Loading

azmy98 Feb 7, 2024 •

edited

Loading