Skip to content

Migrate AAA table per-command authorization in db_migrator #3296

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.

Sign up for GitHub

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

Merged
merged 14 commits into from
May 15, 2024
Merged

Migrate AAA table per-command authorization in db_migrator #3296

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
50e8581
Enable per-command authorization in db_migrator
liuh-80 Apr 23, 2024
991150c
Move test case to new class
liuh-80 Apr 23, 2024
ceb6613
Fix code error
liuh-80 Apr 23, 2024
aac4f8e
Improve UT
liuh-80 Apr 23, 2024
316a9c6
Improve UT
liuh-80 Apr 23, 2024
316c179
Fix code
liuh-80 Apr 23, 2024
c1c277b
Enable test message to debug
liuh-80 Apr 23, 2024
752ae43
Improve debug information
liuh-80 Apr 23, 2024
5047096
Improve code and test case
liuh-80 Apr 24, 2024
61e9f3a
Add both accoutning and authorization
liuh-80 Apr 29, 2024
b35dc64
Add minrate method
liuh-80 Apr 29, 2024
66bb884
Merge remote-tracking branch 'origin' into dev/liuh/enable-per-comman…
liuh-80 Apr 29, 2024
6633383
Fix code by PR comments
liuh-80 May 14, 2024
f3afd81
Improve code
liuh-80 May 14, 2024
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
16 changes: 16 additions & 0 deletions scripts/db_migrator.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -840,6 +840,22 @@ def migrate_aaa(self):
self.configDB.set_entry("AAA", "accounting", accounting_new)
log.log_info('Migrate AAA accounting: {}'.format(accounting_new))

# setup per-command authorization
tacplus_config = self.configDB.get_entry('TACPLUS', 'global')
if 'passkey' in tacplus_config and '' != tacplus_config.get('passkey'):
authorization = self.configDB.get_entry('AAA', 'authorization')
if not authorization:
Copy link
Contributor

@qiluo-msft qiluo-msft May 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

authorization

Could you provide a table to analyze the old_config, new_config, actions for all the situations?
#Closed

Copy link
Contributor Author

@liuh-80 liuh-80 May 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

According to your comments in #3284:

qiluo-msft 2 weeks ago
The old image's enablement does not matter. As long as the required passkey is there, and intention is to enable, then we should enable.

So, we only check the new config:

New_config Action
Passkey does not exist Not enable
Passkey exist, Authorization enable Enable
Passkey exist, Authorization not enable Not enable

authorization_new = aaa_new.get("authorization")
self.configDB.set_entry("AAA", "authorization", authorization_new)
log.log_info('Migrate AAA authorization: {}'.format(authorization_new))
else:
# If no passkey, setup per-command authorization will block remote user command
Copy link
Contributor

@qiluo-msft qiluo-msft May 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

If no passkey

As the designed in your table, you should disable Authorization. However, the current implement is just log and skip. #Closed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed, disable authorization by delete entry.

log.log_info('TACACS passkey does not exist, disable per-command authorization.')
authorization_key = "AAA|authorization"
keys = self.configDB.keys(self.configDB.CONFIG_DB, authorization_key)
if keys:
self.configDB.delete(self.configDB.CONFIG_DB, authorization_key)

def version_unknown(self):
"""
version_unknown tracks all SONiC versions that doesn't have a version
Expand Down
3 changes: 3 additions & 0 deletions tests/db_migrator_input/config_db/per_command_aaa_enable_expected.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,9 @@
"AAA|authentication": {
"login": "tacacs+"
},
"AAA|authorization": {
"login": "tacacs+"
},
"TACPLUS|global": {
"auth_type": "login",
"passkey": "testpasskey"
Expand Down
3 changes: 3 additions & 0 deletions tests/db_migrator_input/config_db/per_command_aaa_no_authentication_expected.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,9 @@
"AAA|authentication": {
"login": "tacacs+"
},
"AAA|authorization": {
"login": "tacacs+"
},
"TACPLUS|global": {
"auth_type": "login",
"passkey": "testpasskey"
Expand Down
3 changes: 3 additions & 0 deletions tests/db_migrator_input/config_db/per_command_aaa_no_passkey.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,9 @@
"AAA|authentication": {
"login": "tacacs+"
},
"AAA|authorization": {
"login": "tacacs+"
},
"TACPLUS|global": {
"auth_type": "login"
}
Expand Down
3 changes: 3 additions & 0 deletions tests/db_migrator_input/config_db/per_command_aaa_no_tacplus_expected.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,9 @@
"AAA|authentication": {
"login": "tacacs+"
},
"AAA|authorization": {
"login": "tacacs+"
},
"TACPLUS|global": {
"auth_type": "login",
"passkey": "testpasskey"
Expand Down