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

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

Merged
merged 1 commit into from
Oct 14, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
285 changes: 285 additions & 0 deletions doc/SONiC_local_users_password_reset_hld.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,285 @@
# Reset local users passwords during init - HLD

## Table of contents

- [Revision](#)
- [Scope](#scope)
- [Definitions/Abbreviations](#definitionsabbreviations)
- [Requirements](#requirements-in-this-feature-we-want-to-be-able-to)
- [Architecture design](#architecture-design)
- [High level design](#high-level-design)
- [Performance](#performance)
- [Flows](#flows)
- [Configuration and management](#configuration-and-management)
- [CLI](#cli)
- [YANG Model](#yang-model)
- [Warmboot and Fastboot Design Impact](#warmboot-and-fastboot-design-impact)
- [Restrictions/Limitations](#restrictionslimitations)
- [Testing Requirements/Design](#testing-requirementsdesign)
- [Unit Test cases](#unit-test-cases)
- [System Test cases](#system-test-cases)

#


# Revision

| **Rev** | **Date** | **Author** | **Change Description** |
|:---------:|:------------:|:-------------------------------:|:------------------------|
| 1.0 | 04/01/2024 | Azmy Ali (Nvidia SONiC Team) | Initial public version |
| 2.0 | 23/01/2024 | Azmy Ali (Nvidia SONiC Team) | Revised version |
| 2.1 | 07/02/2024 | Azmy Ali (Nvidia SONiC Team) | Revised version |

###

# Scope

This document describes the design details for resetting local users' passwords during **init** on a SONiC switch. Within this document, we shall outline the default trigger that initiates the reset of passwords for local users, allowing each vendor to incorporate their unique implementation.

# Definitions/Abbreviations

| **Term** | **Description** |
|-----------------------------|--------------------------------------------------------------------------------------|
| SONiC | Software for Open Networking in the Cloud |
| Long reset button press | Pressing the physical reset button on the switch not less than 15 seconds |
| short reset button press | Pressing the physical reset button on the switch less than 15 seconds |
| CLI | Сommand-line Interface |
| YANG | Yet Another Next Generation |

###

# Requirements Overview

The feature implementation shall support the following:

1. 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 concerns during **init**.
2. The default behavior to trigger this functionality is done by long reset button press, but each vendor can add his desired implementation to when to trigger this feature.
3. Feature should be enabled or disabled by setting compilation flag to true or false.
4. Feature can be disabled and enabled using CLI command.
5. The feature is disabled by default.
6. The service introduced as part of this feature will run if the platform has capability of long reset support.
7. The feature will not affect warm and fast reboot


###

# Architecture Design

In this feature, we want to add new systemd service ```reset-local-users-passwords.service``` to the init flow. This new service is expected to run on the system during boot and before any connection is open to the switch which includes SSH and serial connection as well and perform restoring local users' configurations, by deleting non-default users and restoring default passwords for default users and expiring these passwords due to security conecerns.

![](./reset-local-users-passwords-init.png)

The service is dependent on the ```database.service``` for reading the configured feature state, so it needs to run after it during boot. As well as it needs to run before any connection is open and this includes all the following services: `sshd.service`, `getty.target`, `systemd-logind.service` and `[email protected]`.

###

# High-Level Design

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

Choose a reason for hiding this comment

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

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.

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

Choose a reason for hiding this comment

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

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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)

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

liuh-80 marked this conversation as resolved.
Show resolved Hide resolved


The ```reset_local_users_passwords_base.py``` will contain the following class:

```
class LocalUsersConfigurationResetBase(object):
"""
Base class for resetting local users' configuration on the switch
"""
def should_trigger(self):
'''
define the condition to trigger
'''
# the condition to trigger start() method, the default implementation will be by checking if a long reboot press was detected.

def start(self):
'''
define the functionality
'''
# the implementation of deleting non-default users and restoring original passwords for default users and expiring them
```


Each vendor should inherent this class and add the implementation that he desires in this class.

In the ```src/sonic-host-services/scripts/reset-local-users-passwords``` script, that it is being called on ```reset-local-users-passwords.service``` start, will do import of the inherented class and calls the ```start()``` method of the class as we can see in the following code snippet:


```
def get_platform_local_users_passwords_reset():
try:
from sonic_platform.reset_local_users_passwords import LocalUsersConfigurationResetPlatform
local_users_password_reset_class = LocalUsersConfigurationResetPlatform()
except ImportError:
syslog.syslog(syslog.LOG_WARNING, "LocalUsersPasswordsReset: sonic_platform package not installed. Unable to find platform local users passwords reset implementation")
raise Exception('Local users passwords reset implementation is not defined')

return local_users_password_reset_class

```


The ```LocalUsersConfigurationResetPlatform``` class residing in ```platform/<vendor-path>/sonic_platform/reset_local_users_passwords.py``` that each vendor implements should look like this:


```
class LongRebootPressPlatform(LongRebootPressBase):

def should_trigger(self):
'''
Vendor's specific condition to trigger
'''
# can inherent the base class implementation

def start(self):
'''
Vendor specific implementation.
'''
# can inherent the base class implementation
```


The new serivce will call the python script ```src/sonic-host-services/scripts/reset-local-users-passwords``` which in return it will read the feature state from the ```LOCAL_USERS_PASSWORDS_RESET``` table in ```CONFIG_DB``` database. If feature is enabled and a long reset button press detected, the service will perfom the implementation in the ```start()``` in the vendor's specifc implementation as we can see in the following code snippet of ```src/sonic-host-services/scripts/reset-local-users-passwords```:


```
class LocalUsersConfigurationReset:
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

def __init__(self):
# Wait if the Warm/Fast boot is in progress

def get_feature_state(self):
'''
check if the feature is enabled by reading the redis tables
'''

def start(self):
'''
if the feature is enabled then reset the password's using the platform
specific implementation
'''


def main():
LocalUsersConfigurationReset().start()
```

The feature is added to the init flow as long as we set the new ```ENABLE_LOCAL_USERS_PASSWORDS_RESET``` global variable and the exsiting ```CHANGE_DEFAULT_PASSWORD``` global variable in ```rules/config``` to ```y```.

```
# ENABLE_LOCAL_USERS_PASSWORDS_RESET - enable local users' passwords reset during init on switch
ENABLE_LOCAL_USERS_PASSWORDS_RESET ?= y
```

###

# Performance

The ```reset-local-users-passwords.service``` is required to run as fast as possible, and it is expected to finish running in milliseconds **(150-300 milliseconds)** on long reset detected, and not delay the init flow more than that.

# Flows

###### Local users' password reset during init flow
![](./reset-local-users-passwords-init-flow.png "Local users' password reset during init")

The defualt behavior is on long reset button press, the system will delete non default users and restore original passwords for default users and expire them, all done using linux commands

# Configuration and management

## CLI

<!-- omit in toc -->
## Command structure

**User interface**:
```
config
\-- local-users-passwords-reset
|-- state <enabled|disabled>

show
\-- local-users-passwords-reset
```

### Config command group
**The following command set local-users-passwords-reset feature state:**
```bash
config local-users-passwords-reset state <enabled|disabled>
```

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

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

state
-------
enabled
```

## YANG model
New YANG model `local-users-passwords-reset.yang` will be added to provide support for configuring llocal-users-passwords-reset state.

**Skeleton code:**
```
module sonic-local-users-passwords-reset {

yang-version 1.1;

namespace "http://github.com/sonic-net/local-users-passwords-reset";

description "LONG_RESET_BUTTON YANG Module for SONiC-based OS";

revision 2024-01-04 {
description "First Revision";
}

container sonic-local-users-passwords-reset {

container LOCAL_USERS_PASSWORDS_RESET {

description "LOCAL_USERS_PASSWORDS_RESET part of config_db.json";

container STATE {
leaf state {
type string {
pattern "enabled|disabled";
}
description "Local users' password reset during init feature state";
default disabled;
}

} /* end of container STATE */
}
/* end of container LOCAL_USERS_PASSWORDS_RESET */
}
/* end of top level container */
}
/* end of module sonic-local-users-passwords-reset */
```


# Warmboot and Fastboot Design Impact

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

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will update it also in the requirements


# Restrictions/Limitations

No limitations and restrictions

# Testing Requirements/Design

## Unit Test cases

In unit testing we want to see that the feature control commands change the state in the DB.

## System Test cases

We want to test the following flows:

1. On long reset button press and the feature state is enabled, non-default users are supposed to be deleted and default users are restored to original passwords and expired.
2. On long reset button press and the feature state is disabled, no change the current init flow.
3. short reboot/warm/fast reboot does not affect the current init flow.
Binary file added doc/reset-local-users-passwords-init-flow.png
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file added doc/reset-local-users-passwords-init.png
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.