User Management HLD #1048

Mohammedz93 · 2022-08-07T13:22:47Z

This PR provides high level design for the username management and configuration in Sonic.
It will define the default role (capabilities), default users and describe the supported configurations for users.

linux-foundation-easycla · 2022-08-07T13:22:50Z

The committers listed above are authorized under a signed CLA.

✅ login: Mohammedz93 (289da1d, a8922fb, 8fb5e6b, d04b91c)

Added password expiration flow and password prompt

Fix spaces

Change password flow diagram and fix spaces

zhangyanzhao · 2022-12-13T16:20:16Z

doc/user_mgmt/user_mgmt_hld.md

+
+module sonic-user-mgmt {
+    yang-version 1.1;
+    namespace "http://github.com/Azure/sonic-user-mgmt";


namespace should be http://github.com/sonic-net/sonic-user-mgmt ? SONiC does not belong to Azure github org.

Sure I will fix it

zhangyanzhao · 2022-12-13T16:20:41Z

doc/user_mgmt/user_mgmt_hld.md

+//filename:  sonic-role-mgmt.yang
+module sonic-role-mgmt {
+    yang-version 1.1;
+    namespace "http://github.com/Azure/sonic-role-mgmt";


namespace should be http://github.com/sonic-net/sonic-role-mgmt ? SONiC does not belong to Azure github org.

prvattem · 2022-12-13T16:38:07Z

doc/user_mgmt/user_mgmt_hld.md

+                leaf role {
+                    default "admin";
+                    type string {
+                        pattern "admin|monitor";


Instead of fixing the user roles to admin/monitor, please use leafref to /ROLE_TABLE/ROLE_TABLE_LIST/name.
In the future, if we have additional users, we dont need to change this yang to accommodate new users.

prvattem · 2022-12-13T16:38:53Z

doc/user_mgmt/user_mgmt_hld.md

+
+Options:
+  -?, -h, --help  Show this message and exit.
+```


Please consider providing configuration cli for new role additions as well.

I will add the role commands to the HLD
but I think that we won't implement it them at phase 1 of this feature

lguohan · 2022-12-13T17:00:59Z

doc/user_mgmt/user_mgmt_hld.md

+		* 3.2. [ConfigDB schemas](#ConfigDBschemas)
+		* 3.3. [CLI/YANG model](#CLIYANGmodel)
+* 4. [Build](#Build)
+		* 4.1. [Compilation](#Compilation)


suggest to have runtime configuration only, not build time. enablement can be controlled by config db, it would be easier for user to consume this feature.

+1 -- can you also share the specific reasons why this is starting off as disabled by default? Are we concerned that this design may not be accepted by all users -- if so, can we update the design in such a way that it is?

We have received a request from several vendors to add a compilation flag as they are using remote authentication only.
If we change it to be controlled in runtime, it will necessitate usercfd to run always, which will undoubtedly have an impact on the initialization time even when the feature is disabled.

venkatmahalingam · 2022-12-13T17:02:17Z

doc/user_mgmt/user_mgmt_hld.md

+                }
+
+                leaf secondary_groups {
+                    mandatory true;


Is this field mandatory?

In a second thought, no.
I will delete it.

venkatmahalingam · 2022-12-13T17:02:38Z

doc/user_mgmt/user_mgmt_hld.md

+                    type string;
+                }
+
+                leaf hashed-password {


Use underscore in the field name.

I have seen both formats in other yang models
I couldn't find any conventions document. If you are aware of one, can you please share it?

https://github.com/sonic-net/SONiC/blob/master/doc/mgmt/SONiC_YANG_Model_Guidelines.md

venkatmahalingam · 2022-12-13T17:03:24Z

doc/user_mgmt/user_mgmt_hld.md

+                  	description "List of hashed passwords seperated by comma."; 
+                }
+
+                leaf passwd_date_of_last_change {


Should we use the yang-types:date-and-time instead of uint32?

I don't think so
This field is actually the Unix epoch time of the last password change
It will be used to preserve ageing after upgrade, and it won't be exposed to user.
so it is better to keep it in the original format.

venkatmahalingam · 2022-12-13T17:04:45Z

doc/user_mgmt/user_mgmt_hld.md

+    }
+   container sonic-role-mgmt {
+
+        container ROLE_TABLE {


I believe, this table will populated for admin and monitor roles when there are no entries in the config_db from usercfgd, right?

This table will be added to init_cfgd with admin and monitor roles only.
so we always expect to find these roles there.
If for some reason usercfgd fails to fetch this info, then it will report an error.

venkatmahalingam · 2022-12-13T17:08:26Z

doc/user_mgmt/user_mgmt_hld.md

+
+```
+
+##### Config CLI


Hope we'll use auto Click command generator utility based on YANG model for these commands, please mention the commands that cant be auto-generated here.

venkatmahalingam · 2022-12-13T17:11:09Z

doc/user_mgmt/user_mgmt_hld.md

+    }
+   container sonic-user-mgmt {
+
+        container USER_TABLE {


Can we have the container name as "USER" instead of USER_TABLE? generally. We use _TABLE suffix in the APP DB tables.

I have seen examples in CONFIG DB for tables with "_TABLE" Suffix: ACL_TABLE, FLEX_COUNTER_TABLE, PBH_TABLE...
Is there a guideline document ?

I understand few exceptions today, IMO we should not keep that has a practice for new tables.

https://github.com/sonic-net/SONiC/blob/master/doc/mgmt/SONiC_YANG_Model_Guidelines.md

venkatmahalingam · 2022-12-13T17:12:35Z

doc/user_mgmt/user_mgmt_hld.md

+                  description "User name.";
+                }                
+
+                leaf state {


Generally state name is used for the read-only field, can we use "status" or some other appropriate name?

it is already used in many other yang models
for example: sonic-feature.yang

It's a suggestion to change the field name but I leave it upto you.

jeffhtgt · 2022-12-13T19:12:03Z

doc/user_mgmt/user_mgmt_hld.md

+
+In addition, we will define two default users:
+* **admin**: User role is admin.<br/>
+	The only change from the current admin in Sonic is the additional group: adm


Should this be "users"? Having adm and admin could lead to confusion.

If you are referring to the group name "adm", so it a is basic Linux group that we can't change.
Please see:
https://wiki.debian.org/SystemGroups

jeffhtgt · 2022-12-13T19:14:11Z

doc/user_mgmt/user_mgmt_hld.md

+
+##  3 <a name='Configurationandmanagement'></a>Configuration and management
+
+###  3.1 <a name='ConfigDBTables'></a>ConfigDB Tables


Using CONFIG_DB seems like a security issue. CONFIG_DB is exported into "show techsupport". Superuser/su is not required for "show techsupport". This will allow non-superuser to export hashed passwords. Wouldn't it be better to utilize a new keyspace and add support for exporting/importing like config save/import?

I agree.
I am working with the team on a new design for storing users passwords.
Can you please elaborate about the keyspace ? where I can learn more about it ?
I would appreciate if you could provide an example

See /var/run/redis/sonic-db/database_config.json on any running switch (keyspace = DB). Maybe an unused keyspace/DB could be used and not exported during a "show techsupport". Here is a .json from a switch running 202111 SONiC:
{
"INSTANCES": {
"redis": {
"hostname": "127.0.0.1",
"port": 6379,
"unix_socket_path": "/var/run/redis/redis.sock",
"persistence_for_warm_boot": "yes"
}
},
"DATABASES": {
"APPL_DB": {
"id": 0,
"separator": ":",
"instance": "redis"
},
"ASIC_DB": {
"id": 1,
"separator": ":",
"instance": "redis"
},
"COUNTERS_DB": {
"id": 2,
"separator": ":",
"instance": "redis"
},
"LOGLEVEL_DB": {
"id": 3,
"separator": ":",
"instance": "redis"
},
"CONFIG_DB": {
"id": 4,
"separator": "|",
"instance": "redis"
},
"PFC_WD_DB": {
"id": 5,
"separator": ":",
"instance": "redis"
},
"FLEX_COUNTER_DB": {
"id": 5,
"separator": ":",
"instance": "redis"
},
"STATE_DB": {
"id": 6,
"separator": "|",
"instance": "redis"
},
"SNMP_OVERLAY_DB": {
"id": 7,
"separator": "|",
"instance": "redis"
},
"RESTAPI_DB": {
"id": 8,
"separator": "|",
"instance": "redis"
},
"GB_ASIC_DB": {
"id": 9,
"separator": ":",
"instance": "redis"
},
"GB_COUNTERS_DB": {
"id": 10,
"separator": ":",
"instance": "redis"
},
"GB_FLEX_COUNTER_DB": {
"id": 11,
"separator": ":",
"instance": "redis"
},
"APPL_STATE_DB": {
"id": 14,
"separator": ":",
"instance": "redis"
}
},
"VERSION": "1.0"
}

jeffhtgt · 2022-12-13T19:14:42Z

doc/user_mgmt/user_mgmt_hld.md

+    "hashed-password": {{string}}
+    "hashed-password_history": {{string}}
+    "role": {{admin/monitor}}
+    "passwd_date_of_last_change": {{uint32}}


Do we need a parameter for "expires" (e.g. user expires in 30 days)?

"passwd_date_of_last_change" is the actually the third field in shadow password
(Please see: understanding-etcshadow-file)
Linux forces the password ageing by comparing the current epoch with this field.
We need this field in DB to preserve the expiration date after upgrade.
it also will allow us to apply the California-SB237 law feature easily.
California-SB237 feature

jeffhtgt · 2022-12-13T19:15:12Z

doc/user_mgmt/user_mgmt_hld.md

+STATE                                 = "enabled" / "disabled"      ; user enable/disable
+FULL-NAME                             = STRING                      ; Full-name/Description of user
+HASHED-PASSWORD                       = STRING                      ; Hashed password
+HASHED-PASSWORD_HISTORY               = STRING                      ; List of old passwords (0-99)


Do we need a "expires" parameter?

See the comment above: https://github.com/sonic-net/SONiC/pull/1048/files#r1073802100

jeffhtgt · 2022-12-13T19:16:22Z

doc/user_mgmt/user_mgmt_hld.md

+
+##### Config CLI
+
+```


All of these command should require superuser/su. If this is the case, please explicitly state this in the HLD so appropriate test cases are run.

Sure I will add it

jeffhtgt · 2022-12-13T19:17:21Z

doc/user_mgmt/user_mgmt_hld.md

+
+
+###  7 <a name='WarmbootandFastbootDesignImpact'></a>Warmboot and Fastboot Design Impact
+Not relevant.


If the migration logic is always run, wouldn't this impact timings?

I will test the warm-boot and fast-boot again and update the results in the HLD.

jeffhtgt · 2022-12-13T19:19:43Z

doc/user_mgmt/user_mgmt_hld.md

+  - Configure full-name of user
+
+###### Configuration - Negative flow
+  - Change default users role


I think the following tests would also be appropriate:

Add user and attributes but do not perform config save. Perform reboot and verify expected behavior (this would be applicable for attribute changes and user deletion)

Add new user with new attributes, config save, then downgrade to older SONiC version and verify expected results

I will add these tests to the list.

jeffhtgt · 2022-12-13T19:29:34Z

doc/user_mgmt/user_mgmt_hld.md

+
+Options:
+  -?, -h, --help  Show this message and exit.
+```


Should "show" commands also be added (and only available to su)? e.g. "show users" could display the list of users/attributes but not the hashed passwords

If you are referring to "show" command for users, then it is already in the HLD, "show username"
We can't use "show users" because it already in CLI and it shows that current connected users.
I don't think it should be available to su only since in Linux any user can see the list of the current users on the system (e.g. "lslogins")

jeff-yin · 2022-12-14T23:38:15Z

doc/user_mgmt/user_mgmt_hld.md

+		* 3.2. [ConfigDB schemas](#ConfigDBschemas)
+		* 3.3. [CLI/YANG model](#CLIYANGmodel)
+* 4. [Build](#Build)
+		* 4.1. [Compilation](#Compilation)


+1 -- can you also share the specific reasons why this is starting off as disabled by default? Are we concerned that this design may not be accepted by all users -- if so, can we update the design in such a way that it is?

jeff-yin · 2022-12-14T23:41:53Z

doc/user_mgmt/user_mgmt_hld.md

+
+###  1.5. <a name='Requirements'></a>Requirements
+####  1.5.1. <a name='Definedefaults'></a>Define defaults
+* Default roles: admin and monitor<br/>


Propose updating the roles to admin and operator (instead of monitor). I think the operator term is more widely used for the least-privileged / read-only role in other RBAC implementations, but I might be biased by my own experience at Dell.

operators usually have limited write permissions. our monitor users will have read-only permissions
Please see this RBAC document: RBAC
Monitor – a read-only role. Cannot modify any resource.
Operator – Monitor permissions, plus can modify runtime state, but cannot modify anything that ends up in the persistent configuration. Could, for example, restart a server.

I found a similar description of operator in Dell website:
Dell users

jeff-yin · 2022-12-14T23:50:11Z

doc/user_mgmt/user_mgmt_hld.md

+
+###  1.2. <a name='Scope'></a>Scope
+
+This HLD document described the requirements, architecture and configuration details of User Management feature in switches Sonic OS based.


Does the scope of this document cover local user accounts only?

I feel that we should try to make sure that this design makes space for, or at least avoids conflicting with, remote authentication servers and the management of remotely authenticated users.

There was a "AAA Improvements" HLD proposed and merged a few years ago (https://github.com/sonic-net/SONiC/blob/master/doc/aaa/AAA%20Improvements/AAA%20Improvements.md) but the code PRs didn't make it in (sonic-net/sonic-buildimage#5553).
I'm not saying this needs to be revived necessarily, but it is indicative of the complexity that may be involved.

I have reviewed the implementation of remote authentication servers and I think that there is no conflict.
I still need to do some manual tests then I will add a new section for AAA integration and elaborate about it.
Regarding "AAA Improvements" HLD, I wasn't aware of it.
Is there still an intention to merge it?
it is obviously conflicts with this HLD

I am seeking to revive the effort to contribute the AAA improvements code to SONiC, as the original contributor is no longer assigned to the SONiC project. It would be ideal if we modified both proposals to be compatible with each other so that we could allow for the eventual contribution of AAA enhancements.
What are some of the conflicts you see? Is it just that the proposed DB schemas are different?

jeff-yin · 2022-12-14T23:54:20Z

doc/user_mgmt/user_mgmt_hld.md

+  "<username>":{
+    "state": {{enabled/disabled}}
+    "full-name": {{string}}
+    "hashed-password": {{string}}


+1 agree with @jeffhtgt 's comment above. The hashed user pw is already stored in /etc/shadow, which is permission-restricted in the Linux file system. Putting the hashed pw in CONFIG_DB will circumvent those permission restrictions.

I agree.
I am working with the team on a new design for storing users passwords.

liat-grozovik · 2023-02-27T09:23:16Z

@Mohammedz93 can you please share a plan when the HLD should be updated following the comments provided?

Mohammedz93 added 3 commits August 7, 2022 16:20

user mgmt folder

289da1d

Rename user_mgmt_hld to user_mgmt_hld.md

a8922fb

add diagrams

8fb5e6b

Mohammedz93 added 2 commits August 7, 2022 16:26

Update user_mgmt_hld.md

d04b91c

Merge branch 'master' into user_mgmt_hld

ffdd71a

Mohammedz93 changed the title ~~User mgmt hld~~ User Management HLD Nov 13, 2022

Mohammedz93 added 7 commits December 13, 2022 10:05

Update HLD with new content

4291ecc

Added password expiration flow and password prompt

Merge branch 'master' into user_mgmt_hld

0d0b3bd

Create password_prompt_flow.png

0fc929b

Update user_mgmt_hld.md

f830a22

Fix spaces

Update user_mgmt_hld.md

ffb9302

Change password flow diagram and fix spaces

Typo fixes

dac7d3b

Add California Law feature to defatuls

ba44e49

zhangyanzhao reviewed Dec 13, 2022

View reviewed changes

prvattem reviewed Dec 13, 2022

View reviewed changes

lguohan reviewed Dec 13, 2022

View reviewed changes

venkatmahalingam reviewed Dec 13, 2022

View reviewed changes

jeffhtgt reviewed Dec 13, 2022

View reviewed changes

jeff-yin reviewed Dec 14, 2022

View reviewed changes

Mohammedz93 mentioned this pull request Jan 26, 2023

Reset factory hld #1231

Merged

Mohammedz93 closed this Mar 16, 2023


		## 3 <a name='Configurationandmanagement'></a>Configuration and management

		### 3.1 <a name='ConfigDBTables'></a>ConfigDB Tables



		### 7 <a name='WarmbootandFastbootDesignImpact'></a>Warmboot and Fastboot Design Impact
		Not relevant.


		### 1.2. <a name='Scope'></a>Scope

		This HLD document described the requirements, architecture and configuration details of User Management feature in switches Sonic OS based.


		```

		##### Config CLI


		##### Config CLI

		```

User Management HLD #1048

User Management HLD #1048

Conversation

Mohammedz93 commented Aug 7, 2022 • edited Loading

linux-foundation-easycla bot commented Aug 7, 2022 • edited Loading

zhangyanzhao Dec 13, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhangyanzhao Dec 13, 2022 • 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

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

venkatmahalingam Dec 13, 2022 • 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

Mohammedz93 Jan 18, 2023 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liat-grozovik commented Feb 27, 2023

Mohammedz93 commented Aug 7, 2022 •

edited

Loading

linux-foundation-easycla bot commented Aug 7, 2022 •

edited

Loading

zhangyanzhao Dec 13, 2022 •

edited

Loading

zhangyanzhao Dec 13, 2022 •

edited

Loading

venkatmahalingam Dec 13, 2022 •

edited

Loading

Mohammedz93 Jan 18, 2023 •

edited

Loading