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]: skip accessing tacacs servers for local non-tacacs users #2843

Merged
merged 2 commits into from
May 9, 2019

Conversation

renukamanavalan
Copy link
Contributor

This helps use the legacy passwd file for user info and go to tacacs only if not found.
This means, we never contact tacacs for local users like "admin".
This isolates local users from any issues with tacacs servers.
W/o this fix, the sudo commands by local users could take * seconds, if the tacacs servers are unreachable.

- What I did
Switched order of user lookup to "compat" first, followed by "tacplus".

- How I did it

- How to verify it
Configure properly for tacacs+ login, with exception of server-IP, where you put an unreachable/non-existing.
login as admin and try "time sudo ls"

This would take roughly * <tacacs timeout -- which defaults to 5> seconds.

With this fix, the "sudo" will work fine

- Description for the changelog

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

This helps use the legacy passwd file for user info and go to tacacs only if not found.
This means, we never contact tacacs for local users like "admin".
This isolates local users from any issues with tacacs servers.
W/o this fix, the sudo commands by local users could take <count of servers> * <tacacs timeout> seconds, if the tacacs servers are unreachable.
@lguohan
Copy link
Collaborator

lguohan commented May 1, 2019

looks like #2163 changed this deliberately.

@lguohan
Copy link
Collaborator

lguohan commented May 1, 2019

without #2163, if user is already in passwd file nss will not even communicate with tacacs. This is to address the scenario of user permission change

@renukamanavalan
Copy link
Contributor Author

There does not seems to be a detailed explanation of the bug in the PR #2163.

In short:
When a TACACS user login, indeed a passwd entry gets created, but with a locked passwd. This implies that when the same TACACS user logs in, he can't get in w/o reaching tacacs server.

But say a TACACS user is logged in, and then his credentials are removed from TACACS servers, it would not affect his current session. But he can't create anymore new sessions.

So this restricts the nss access to only getting user info and nothing more.

Hence compat first and TACACS next, should not affect the level of security in any way,

Later when we move on to controlling authorizations, I believe this, still would not impact as its impact is restricted to getpwnam, getpwent and related functions, with source of info from passwd file only.

@lguohan
Copy link
Collaborator

lguohan commented May 1, 2019

the scenario is that user change from ro to rw, if we put compat first, will the user privilege gets updated after he changed from ro to rw on the tacacs side.

Revert the order of 'compat tacplus' to original 'tacplus compat' as tacplus
access is required for all tacacs users, who also get created locally.
@lguohan lguohan changed the title Switch the nss look up order as "compat" followed by "tacplus". [tacacs]: skip accessing tacacs servers for local non-tacacs users May 8, 2019
@lguohan
Copy link
Collaborator

lguohan commented May 8, 2019

I have this question, why do you choose to compare the gecos? why not compare the group name of the tacacs user? it is going to be either remote_user or remote_user_su.

Copy link
Collaborator

@lguohan lguohan left a comment

Choose a reason for hiding this comment

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

one more comment

@lguohan lguohan self-requested a review May 8, 2019 08:07
@renukamanavalan
Copy link
Contributor Author

renukamanavalan commented May 9, 2019

The strings "remote_user" & "remote_user_su" are not names of groups, but used to set pw_gecos (Full name) of the user.

The read-only users (privilege level = 1) get the gid as "100", which is users:x:100:user_ro and rw users (privilege level = 15) gets gid=1000 which is admin:x:1000: as primary group and "sudo,docker" as secondary groups.

@lguohan lguohan merged commit a357693 into sonic-net:master May 9, 2019
lguohan pushed a commit that referenced this pull request May 17, 2019
…2843)

* Switch the nss look up order as "compat" followed by "tacplus".
This helps use the legacy passwd file for user info and go to tacacs only if not found.
This means, we never contact tacacs for local users like "admin".
This isolates local users from any issues with tacacs servers.
W/o this fix, the sudo commands by local users could take <count of servers> * <tacacs timeout> seconds, if the tacacs servers are unreachable.

* Skip tacacs server access for local non-tacacs users.
Revert the order of 'compat tacplus' to original 'tacplus compat' as tacplus
access is required for all tacacs users, who also get created locally.
yxieca pushed a commit that referenced this pull request May 20, 2019
…2843)

* Switch the nss look up order as "compat" followed by "tacplus".
This helps use the legacy passwd file for user info and go to tacacs only if not found.
This means, we never contact tacacs for local users like "admin".
This isolates local users from any issues with tacacs servers.
W/o this fix, the sudo commands by local users could take <count of servers> * <tacacs timeout> seconds, if the tacacs servers are unreachable.

* Skip tacacs server access for local non-tacacs users.
Revert the order of 'compat tacplus' to original 'tacplus compat' as tacplus
access is required for all tacacs users, who also get created locally.
MichelMoriniaux pushed a commit to criteo-forks/sonic-buildimage that referenced this pull request May 28, 2019
…onic-net#2843)

* Switch the nss look up order as "compat" followed by "tacplus".
This helps use the legacy passwd file for user info and go to tacacs only if not found.
This means, we never contact tacacs for local users like "admin".
This isolates local users from any issues with tacacs servers.
W/o this fix, the sudo commands by local users could take <count of servers> * <tacacs timeout> seconds, if the tacacs servers are unreachable.

* Skip tacacs server access for local non-tacacs users.
Revert the order of 'compat tacplus' to original 'tacplus compat' as tacplus
access is required for all tacacs users, who also get created locally.
@renukamanavalan renukamanavalan deleted the hostcfgd branch July 9, 2019 23:33
dprital added a commit to dprital/sonic-buildimage that referenced this pull request Jun 20, 2023
Update sonic-utilities submodule pointer to include the following:
* 0b629ba1 Revert [chassis][voq] Clear fabric counters queue/port (2789) ([sonic-net#2882](sonic-net/sonic-utilities#2882))
* 3ba8241a [db_migtrator] Add migration of FLEX_COUNTER_DELAY_STATUS during 1911->master upgrade + fast-reboot. Add UT. ([sonic-net#2839](sonic-net/sonic-utilities#2839))
* fceef2ed [chassis][voq] Clear fabric counters queue/port ([sonic-net#2789](sonic-net/sonic-utilities#2789))
* 659ba24b [syslog] Adjust runningconfiguration syslog command ([sonic-net#2843](sonic-net/sonic-utilities#2843))
* 46fba26f [db_migrator] add required protocol field in ROUTE_TABLE ([sonic-net#2766](sonic-net/sonic-utilities#2766))
* f186376e Fix issue: show interfaces transceiver eeprom -d should display same entry for CMIS cable ([sonic-net#2864](sonic-net/sonic-utilities#2864))
* de491798 fix precedence in portstat CLI ([sonic-net#2874](sonic-net/sonic-utilities#2874))

Signed-off-by: dprital <[email protected]>
dprital added a commit to dprital/sonic-buildimage that referenced this pull request Jun 21, 2023
Update sonic-utilities submodule pointer to include the following:
* 0b629ba1 Revert [chassis][voq] Clear fabric counters queue/port (2789) ([sonic-net#2882](sonic-net/sonic-utilities#2882))
* 3ba8241a [db_migtrator] Add migration of FLEX_COUNTER_DELAY_STATUS during 1911->master upgrade + fast-reboot. Add UT. ([sonic-net#2839](sonic-net/sonic-utilities#2839))
* fceef2ed [chassis][voq] Clear fabric counters queue/port ([sonic-net#2789](sonic-net/sonic-utilities#2789))
* 659ba24b [syslog] Adjust runningconfiguration syslog command ([sonic-net#2843](sonic-net/sonic-utilities#2843))
* 46fba26f [db_migrator] add required protocol field in ROUTE_TABLE ([sonic-net#2766](sonic-net/sonic-utilities#2766))
* f186376e Fix issue: show interfaces transceiver eeprom -d should display same entry for CMIS cable ([sonic-net#2864](sonic-net/sonic-utilities#2864))
* de491798 fix precedence in portstat CLI ([sonic-net#2874](sonic-net/sonic-utilities#2874))

Signed-off-by: dprital <[email protected]>
mssonicbld added a commit that referenced this pull request Jul 13, 2023
…lly (#15808)

#### Why I did it
src/sonic-swss
```
* eff2b751 - (HEAD -> 202211, origin/202211) [202211][CodeQL]: Use dependencies with relevant versions in azp template. (#2843) (6 hours ago) [Nazarii Hnydyn]
```
#### How I did it
#### How to verify it
#### Description for the changelog
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