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.

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

Jump to bottom

[TACACS] Improve TACACS per-command authorization UT coverage #8115

Merged
merged 6 commits into from
Jun 15, 2023
Merged

[TACACS] Improve TACACS per-command authorization UT coverage #8115

merged 6 commits into from
Jun 15, 2023

Conversation

liuh-80
Copy link
Contributor

@liuh-80 liuh-80 commented Apr 23, 2023

[TACACS] Improve TACACS per-command authorization UT coverage

Description of PR

Improve TACACS per-command authorization UT coverage

Summary:
Fixes # (issue)

Type of change

  • Bug fix
  • Testbed and Framework(new/improvement)
  • Test case(new/improvement)

Back port request

  • 201911
  • 202012
  • 202205

Approach

What is the motivation for this PR?

Improve TACACS per-command authorization UT coverage.

How did you do it?

Add new UT to cover 'run command with wildcard' scenario.
Improve exist UT to cover more commands.

How did you verify/test it?

Manually test with latest master branch image and 202205/202211 branch image.

Any platform specific information?

Supported testbed topology if it's a new test case?

Documentation

@liuh-80
Copy link
Contributor Author

liuh-80 commented Apr 23, 2023

This UT depends on sonic-net/sonic-buildimage#14787

@liuh-80 liuh-80 mentioned this pull request Apr 23, 2023
Merged
10 tasks
@mssonicbld
Copy link
Collaborator

The pre-commit check detected issues in the files touched by this pull request.
The pre-commit check is a mandatory check, please fix detected issues.

Detailed pre-commit check results:
trim trailing whitespace.................................................Passed
fix end of files.........................................................Failed
- hook id: end-of-file-fixer
- exit code: 1
- files were modified by this hook

Fixing tests/tacacs/test_authorization.py
Fixing tests/tacacs/test_rw_user.py

check yaml...........................................(no files to check)Skipped
check for added large files..............................................Passed
check python ast.........................................................Passed
flake8...................................................................Failed
- hook id: flake8
- exit code: 1

tests/tacacs/test_authorization.py:82:1: E302 expected 2 blank lines, found 1
tests/tacacs/test_authorization.py:93:1: E302 expected 2 blank lines, found 1

To run the pre-commit checks locally, you can follow below steps:

  1. Ensure that default python is python3. In sonic-mgmt docker container, default python is python2. You can run
    the check by activating the python3 virtual environment in sonic-mgmt docker container or outside of sonic-mgmt
    docker container.
  2. Ensure that the pre-commit package is installed:
sudo pip install pre-commit
  1. Go to repository root folder
  2. Install the pre-commit hooks:
pre-commit install
  1. Use pre-commit to check staged file:
pre-commit
  1. Alternatively, you can check committed files using:
pre-commit run --from-ref <commit_id> --to-ref <commit_id>

liuh-80
liuh-80 commented Apr 23, 2023
View reviewed changes
tests/tacacs/utils.py
@@ -244,3 +245,37 @@ def remove_all_tacacs_server(duthost):
tacacs_server = tacacs_server.rstrip()
if tacacs_server:
duthost.shell("sudo config tacacs delete %s" % tacacs_server)


def check_server_received(ptfhost, data):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

No any code change, for fix the circle import issue, move code from test_accounting.py.

qiluo-msft
qiluo-msft previously approved these changes Apr 23, 2023
View reviewed changes
qiluo-msft pushed a commit to sonic-net/sonic-buildimage that referenced this pull request Apr 24, 2023
@liuh-80
Fix per-command authorization failed issue when a command with wildca…
b444a46 
…rd match more than hundred files. (#14787)

Fix per-command authorization failed issue when a command with wildcard match more than hundred files.


#### Why I did it
When user enable TACACS per-command authorization, and run a command with wildcard , if the command match more than hundreds of files, the per-command authorization will failed with following message:
  *** authorize failed by TACACS+ with given arguments, not executing

The root cause of this issue is because bash will match files with wildcard and replace with wildcard args with matched files. when there are too many files, TACACS plugin will generate a big authorization request, which will be reject by server side. 

##### Work item tracking
- Microsoft ADO **(number only)**: 18074861

#### How I did it
Fix bash patch file, use original user inputs as authorization parameters.

#### How to verify it
Pass all UT.
Create new UT to validate the TACACS authorization request are using original command arguments.
UT PR: sonic-net/sonic-mgmt#8115

#### Which release branch to backport (provide reason below if selected)

- [ ] 201811
- [ ] 201911
- [ ] 202006
- [ ] 202012
- [ ] 202106
- [ ] 202111
- [X] 202205
- [X] 202211

#### Tested branch (Please provide the tested image version)

- [x] 202205.258490-412b83d0f
- [x] 202211.71966120-1b971c54b5


#### Description for the changelog
Fix per-command authorization failed issue when a command with wildcard match more than hundred files.
mssonicbld pushed a commit to mssonicbld/sonic-buildimage that referenced this pull request May 11, 2023
@liuh-80 @mssonicbld
Fix per-command authorization failed issue when a command with wildca…
a0e3882 
…rd match more than hundred files. (sonic-net#14787)

Fix per-command authorization failed issue when a command with wildcard match more than hundred files.


#### Why I did it
When user enable TACACS per-command authorization, and run a command with wildcard , if the command match more than hundreds of files, the per-command authorization will failed with following message:
  *** authorize failed by TACACS+ with given arguments, not executing

The root cause of this issue is because bash will match files with wildcard and replace with wildcard args with matched files. when there are too many files, TACACS plugin will generate a big authorization request, which will be reject by server side. 

##### Work item tracking
- Microsoft ADO **(number only)**: 18074861

#### How I did it
Fix bash patch file, use original user inputs as authorization parameters.

#### How to verify it
Pass all UT.
Create new UT to validate the TACACS authorization request are using original command arguments.
UT PR: sonic-net/sonic-mgmt#8115

#### Which release branch to backport (provide reason below if selected)

- [ ] 201811
- [ ] 201911
- [ ] 202006
- [ ] 202012
- [ ] 202106
- [ ] 202111
- [X] 202205
- [X] 202211

#### Tested branch (Please provide the tested image version)

- [x] 202205.258490-412b83d0f
- [x] 202211.71966120-1b971c54b5


#### Description for the changelog
Fix per-command authorization failed issue when a command with wildcard match more than hundred files.
mssonicbld pushed a commit to mssonicbld/sonic-buildimage that referenced this pull request May 11, 2023
@liuh-80 @mssonicbld
Fix per-command authorization failed issue when a command with wildca…
6f7a3b5 
…rd match more than hundred files. (sonic-net#14787)

Fix per-command authorization failed issue when a command with wildcard match more than hundred files.


#### Why I did it
When user enable TACACS per-command authorization, and run a command with wildcard , if the command match more than hundreds of files, the per-command authorization will failed with following message:
  *** authorize failed by TACACS+ with given arguments, not executing

The root cause of this issue is because bash will match files with wildcard and replace with wildcard args with matched files. when there are too many files, TACACS plugin will generate a big authorization request, which will be reject by server side. 

##### Work item tracking
- Microsoft ADO **(number only)**: 18074861

#### How I did it
Fix bash patch file, use original user inputs as authorization parameters.

#### How to verify it
Pass all UT.
Create new UT to validate the TACACS authorization request are using original command arguments.
UT PR: sonic-net/sonic-mgmt#8115

#### Which release branch to backport (provide reason below if selected)

- [ ] 201811
- [ ] 201911
- [ ] 202006
- [ ] 202012
- [ ] 202106
- [ ] 202111
- [X] 202205
- [X] 202211

#### Tested branch (Please provide the tested image version)

- [x] 202205.258490-412b83d0f
- [x] 202211.71966120-1b971c54b5


#### Description for the changelog
Fix per-command authorization failed issue when a command with wildcard match more than hundred files.
mssonicbld pushed a commit to sonic-net/sonic-buildimage that referenced this pull request May 15, 2023
@liuh-80 @mssonicbld
Fix per-command authorization failed issue when a command with wildca…
50705e9 
…rd match more than hundred files. (#14787)

Fix per-command authorization failed issue when a command with wildcard match more than hundred files.


#### Why I did it
When user enable TACACS per-command authorization, and run a command with wildcard , if the command match more than hundreds of files, the per-command authorization will failed with following message:
  *** authorize failed by TACACS+ with given arguments, not executing

The root cause of this issue is because bash will match files with wildcard and replace with wildcard args with matched files. when there are too many files, TACACS plugin will generate a big authorization request, which will be reject by server side. 

##### Work item tracking
- Microsoft ADO **(number only)**: 18074861

#### How I did it
Fix bash patch file, use original user inputs as authorization parameters.

#### How to verify it
Pass all UT.
Create new UT to validate the TACACS authorization request are using original command arguments.
UT PR: sonic-net/sonic-mgmt#8115

#### Which release branch to backport (provide reason below if selected)

- [ ] 201811
- [ ] 201911
- [ ] 202006
- [ ] 202012
- [ ] 202106
- [ ] 202111
- [X] 202205
- [X] 202211

#### Tested branch (Please provide the tested image version)

- [x] 202205.258490-412b83d0f
- [x] 202211.71966120-1b971c54b5


#### Description for the changelog
Fix per-command authorization failed issue when a command with wildcard match more than hundred files.
mssonicbld pushed a commit to sonic-net/sonic-buildimage that referenced this pull request May 16, 2023
@liuh-80 @mssonicbld
Fix per-command authorization failed issue when a command with wildca…
0537a48 
…rd match more than hundred files. (#14787)

Fix per-command authorization failed issue when a command with wildcard match more than hundred files.


#### Why I did it
When user enable TACACS per-command authorization, and run a command with wildcard , if the command match more than hundreds of files, the per-command authorization will failed with following message:
  *** authorize failed by TACACS+ with given arguments, not executing

The root cause of this issue is because bash will match files with wildcard and replace with wildcard args with matched files. when there are too many files, TACACS plugin will generate a big authorization request, which will be reject by server side. 

##### Work item tracking
- Microsoft ADO **(number only)**: 18074861

#### How I did it
Fix bash patch file, use original user inputs as authorization parameters.

#### How to verify it
Pass all UT.
Create new UT to validate the TACACS authorization request are using original command arguments.
UT PR: sonic-net/sonic-mgmt#8115

#### Which release branch to backport (provide reason below if selected)

- [ ] 201811
- [ ] 201911
- [ ] 202006
- [ ] 202012
- [ ] 202106
- [ ] 202111
- [X] 202205
- [X] 202211

#### Tested branch (Please provide the tested image version)

- [x] 202205.258490-412b83d0f
- [x] 202211.71966120-1b971c54b5


#### Description for the changelog
Fix per-command authorization failed issue when a command with wildcard match more than hundred files.
@liuh-80
Copy link
Contributor Author

liuh-80 commented May 18, 2023

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@mssonicbld
Copy link
Collaborator

The pre-commit check detected issues in the files touched by this pull request.
The pre-commit check is a mandatory check, please fix detected issues.

Detailed pre-commit check results:
trim trailing whitespace.................................................Passed
fix end of files.........................................................Passed
check yaml...........................................(no files to check)Skipped
check for added large files..............................................Passed
check python ast.........................................................Passed
flake8...................................................................Failed
- hook id: flake8
- exit code: 1

tests/tacacs/test_authorization.py:175:1: E303 too many blank lines (3)

check conditional mark sort..........................(no files to check)Skipped

To run the pre-commit checks locally, you can follow below steps:

  1. Ensure that default python is python3. In sonic-mgmt docker container, default python is python2. You can run
    the check by activating the python3 virtual environment in sonic-mgmt docker container or outside of sonic-mgmt
    docker container.
  2. Ensure that the pre-commit package is installed:
sudo pip install pre-commit
  1. Go to repository root folder
  2. Install the pre-commit hooks:
pre-commit install
  1. Use pre-commit to check staged file:
pre-commit
  1. Alternatively, you can check committed files using:
pre-commit run --from-ref <commit_id> --to-ref <commit_id>

@liuh-80 liuh-80 marked this pull request as ready for review May 26, 2023 09:11
qiluo-msft
qiluo-msft previously approved these changes Jun 7, 2023
View reviewed changes
@qiluo-msft
Copy link
Contributor

Please resolve the conflicts.

@liuh-80 liuh-80 merged commit c637cfd into sonic-net:master Jun 15, 2023
mrkcmo pushed a commit to Azarack/sonic-mgmt that referenced this pull request Oct 3, 2023
@liuh-80 @parmarkj
[TACACS] Improve TACACS per-command authorization UT coverage (sonic-…
f94a91c 
…net#8115)

[TACACS] Improve TACACS per-command  authorization UT coverage

### Description of PR
Improve TACACS per-command  authorization UT coverage

Summary:
Fixes # (issue)

### Type of change

<!--
- Fill x for your type of change.
- e.g.
- [x] Bug fix
-->

- [ ] Bug fix
- [ ] Testbed and Framework(new/improvement)
- [X] Test case(new/improvement)


### Back port request
- [ ] 201911
- [ ] 202012
- [x] 202205

### Approach
#### What is the motivation for this PR?
Improve TACACS per-command  authorization UT coverage.

#### How did you do it?
Add new UT to cover 'run command with wildcard' scenario.
Improve exist UT to cover more commands.

#### How did you verify/test it?
Manually test with latest master branch image and 202205/202211 branch image.

#### Any platform specific information?

#### Supported testbed topology if it's a new test case?

### Documentation
<!--
(If it's a new feature, new test case)
Did you update documentation/Wiki relevant to your implementation?
Link to the wiki page?
-->
AharonMalkin pushed a commit to AharonMalkin/sonic-mgmt that referenced this pull request Jan 25, 2024
@liuh-80 @AharonMalkin
[TACACS] Improve TACACS per-command authorization UT coverage (sonic-…
444cb29 
…net#8115)

[TACACS] Improve TACACS per-command  authorization UT coverage

### Description of PR
Improve TACACS per-command  authorization UT coverage

Summary:
Fixes # (issue)

### Type of change

<!--
- Fill x for your type of change.
- e.g.
- [x] Bug fix
-->

- [ ] Bug fix
- [ ] Testbed and Framework(new/improvement)
- [X] Test case(new/improvement)


### Back port request
- [ ] 201911
- [ ] 202012
- [x] 202205

### Approach
#### What is the motivation for this PR?
Improve TACACS per-command  authorization UT coverage.

#### How did you do it?
Add new UT to cover 'run command with wildcard' scenario.
Improve exist UT to cover more commands.

#### How did you verify/test it?
Manually test with latest master branch image and 202205/202211 branch image.

#### Any platform specific information?

#### Supported testbed topology if it's a new test case?

### Documentation
<!--
(If it's a new feature, new test case)
Did you update documentation/Wiki relevant to your implementation?
Link to the wiki page?
-->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@qiluo-msft qiluo-msft qiluo-msft approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@liuh-80 @mssonicbld @qiluo-msft