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

[TACACS] Improve per-command authorization performance by read passwd entry with getpwent #16460

Merged
merged 5 commits into from
Oct 14, 2023

Conversation

liuh-80
Copy link
Contributor

@liuh-80 liuh-80 commented Sep 6, 2023

Improve per-command authorization performance by read passwd entry with getpwent.

Why I did it

Currently per-command authorization will check if user is remote user with getpwnam API, which will trigger tacplus-nss for authentication with TACACS server.
But this is not necessary because when user login the user information already add to local passwd file.
Use getpwent API can directly read from passwd file, this will improve per-command authorization performance.

Work item tracking
  • Microsoft ADO: 25104723

How I did it

Improve per-command authorization performance by read passwd entry with getpwent.

How to verify it

Pass all UT.

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

  • 201811
  • 201911
  • 202006
  • 202012
  • 202106
  • 202111
  • 202205
  • 202211
  • 202305

Tested branch (Please provide the tested image version)

  • SONiC.master-16460.356317-6c3424111
  • SONiC.202205-16659.368899-1c78e9b70

Description for the changelog

Improve per-command authorization performance by read passwd entry with getpwent.

Link to config_db schema for YANG module changes

A picture of a cute animal (not mandatory but encouraged)

@liuh-80 liuh-80 changed the title [POC] Improve per-command authorization performance by check user info from… Improve per-command authorization performance by read passwd entry with getpwent Sep 8, 2023
@liuh-80 liuh-80 marked this pull request as ready for review September 8, 2023 05:14
@liuh-80 liuh-80 requested a review from qiluo-msft September 8, 2023 05:14
@qiluo-msft
Copy link
Collaborator

qiluo-msft commented Sep 19, 2023

In "Tested branch (Please provide the tested image version)", please provide a version to indicate you have tested on earliest backport branch. #Closed

@qiluo-msft
Copy link
Collaborator

qiluo-msft commented Sep 19, 2023

int is_local_user(char *user)

Suggest extensively unit test. #Closed


Refers to: src/tacacs/bash_tacplus/bash_tacplus.c:345 in ad1647d. [](commit_id = ad1647d, deletion_comment = False)

@liuh-80
Copy link
Contributor Author

liuh-80 commented Sep 21, 2023

Suggest extensively unit test.

Fixed, add UT to cover new codes.

@liuh-80 liuh-80 changed the title Improve per-command authorization performance by read passwd entry with getpwent [TACACS] Improve per-command authorization performance by read passwd entry with getpwent Sep 22, 2023
@liuh-80
Copy link
Contributor Author

liuh-80 commented Sep 22, 2023

Fixed, PR description updated.

Change can't be clean cherry-pick to 202205, create a manually cherry-pick PR: #16659

@liuh-80
Copy link
Contributor Author

liuh-80 commented Sep 22, 2023

Close and open to trigger re-build.

@liuh-80 liuh-80 closed this Sep 22, 2023
@liuh-80 liuh-80 reopened this Sep 22, 2023
qiluo-msft
qiluo-msft previously approved these changes Sep 22, 2023
@lguohan lguohan added the auth label Sep 23, 2023
@liuh-80
Copy link
Contributor Author

liuh-80 commented Sep 25, 2023

TACACS UT keeps failed on T0 device, every time failed on different place. test on local test bed all UT passed.

After check test failed log, seems UT check TACPLUS server log before server updates it.
Before this change, the per-command authorization need take longer time, so there is enough time to make tacplus server write data to log.

create a new UT to fix this issue.:

sonic-net/sonic-mgmt#10110

@sonic-net sonic-net deleted a comment from azure-pipelines bot Oct 11, 2023
@liuh-80
Copy link
Contributor Author

liuh-80 commented Oct 12, 2023

UT fix merged, close and re-open to trigger validation.

@liuh-80 liuh-80 closed this Oct 12, 2023
@liuh-80 liuh-80 reopened this Oct 12, 2023
@liuh-80
Copy link
Contributor Author

liuh-80 commented Oct 12, 2023

Master branch build image failed, waiting fix merge first: #16859

@liuh-80
Copy link
Contributor Author

liuh-80 commented Oct 12, 2023

Cherry-pick to 202205 will have conflict, here is manually cherry-pick PR for 202205: #16659

@liuh-80
Copy link
Contributor Author

liuh-80 commented Oct 12, 2023

UT still failed, seems found a corner case, will fix and update later.

@liuh-80
Copy link
Contributor Author

liuh-80 commented Oct 13, 2023

Close and reopen to trigger rebuild and validation.

@liuh-80 liuh-80 closed this Oct 13, 2023
@liuh-80 liuh-80 reopened this Oct 13, 2023
@qiluo-msft qiluo-msft merged commit f0d88f3 into sonic-net:master Oct 14, 2023
yxieca pushed a commit that referenced this pull request Oct 14, 2023
…ad passwd entry with getpwent (#16659)

Improve per-command authorization performance by read passwd entry with getpwent.
This is manually cherry-pick PR for #16460

Why I did it
Currently per-command authorization will check if user is remote user with getpwnam API, which will trigger tacplus-nss for authentication with TACACS server.
But this is not necessary because when user login the user information already add to local passwd file.
Use getpwent API can directly read from passwd file, this will improve per-command authorization performance.
return ERROR_CHECK_LOCAL_USER;
}
if (strcmp(ppwd->pw_name, user) != 0) {
continue;
Copy link
Collaborator

Choose a reason for hiding this comment

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

continue

This will lead to a loop, is it possible that the code flow always like this and keep burning CPU in an infinite loop?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This will not be an infinity loop, the getpwent_r API designed to return next entry in passwd, the loop will end when there is no next entry:

https://www.man7.org/linux/man-pages/man3/getpwent_r.3.html

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants