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

Sure I will fix it

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

Sure

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.

Sure

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

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.

Is this field mandatory?

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

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

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.

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.

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.

Sure

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

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.

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

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"
}

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

Do we need a "expires" parameter?

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

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

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.

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.

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")

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

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

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?

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