-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Add SONiC design doc: TACACS+ improvement #813
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
Conversation
doc/aaa/TACACS+ Requirement.md
Outdated
- Different privilege level have different permission to run these command. | ||
- All commands in sudoers will add to the whitelist. and sudoers config file still need for RO users, this is because when remote TACACS server not avaliable, we need use local group permission for failover. | ||
|
||
- Disable user behavior in shell: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why rbash is a must for command authorization?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reason is because for command authorization, we create a symbol link for every command we want enable authorization.
Without rbash user can run any command, include those command we not create symbol link.
Then they can bypass the authorization very easy.
doc/aaa/TACACS+ Requirement.md
Outdated
- Specifying command names containing slashes. | ||
- Importing function definitions from the shell environment at startup. | ||
- Parsing the value of SHELLOPTS from the shell environment at startup. | ||
- Redirecting output using the ‘>’, ‘>|’, ‘<>’, ‘>&’, ‘&>’, and ‘>>’ redirection operators. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By patch bash, we can allow user use redirect, will remove this from document.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you have a link to the patch? I am wondering if redirect could be configurable. For example, RW users could use redirect as normal, but RO users are banned to use redirect.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a very simple code change, remove following code in redir.c line:836
#if defined (RESTRICTED_SHELL)
if (restricted && (WRITE_REDIRECT (ri)))
{
free (redirectee_word);
return (RESTRICTED_REDIRECT);
}
#endif /* RESTRICTED_SHELL */
So for RW allow and RO disable, we can:
-
create a variable isReadonlyUser
-
When bash startup, check current user group and set this variable.
-
If any behavior we want to disable for RO only we can change code like this:
#if defined (RESTRICTED_SHELL)
if (restricted && isReadonlyUser && (WRITE_REDIRECT (ri)))
{
free (redirectee_word);
return (RESTRICTED_REDIRECT);
}
#endif /* RESTRICTED_SHELL */
doc/aaa/TACACS+ Requirement.md
Outdated
- Different privilege level have different permission to run these command. | ||
- All commands in sudoers will add to the whitelist. and sudoers config file still need for RO users, this is because when remote TACACS server not avaliable, we need use local group permission for failover. | ||
|
||
- Disable user behavior in shell: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In that case, we need consider some other solution. I will check other solution in POC and update this document.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I check some other solution and they are based on shell function hook, those solution have a common limitation: user can bypass restriction by ingest new shell hook.
Then after check the rbash restriction, I think we have 2 solution:
-
Patch bash to allow redirection in restrict mode, and accept most other restriction because those are necessary for use tacplus-auth: those restriction we accept will make both RO/RW user can't access commands not in our white list, and make user can't switch to other shell.
-
Patch bash, so bash can do Authorization before execute any user command.
With this solution, there is no any restriction in router machine side, in TACACS server side we just need disable user switch to other shell.
I will try a POC for solution 2, it's seems will be a minor change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The POC for solution 2 is ready, which will authorize any user 'disk' command, so if solution 2 is acceptable, will update the document.
For 'disk' command, in bash, if a command is a file stored in disk, it's a disk command. built-in command and bash function are not disk command, executable file and script are disk command.
doc/aaa/TACACS+ Requirement.md
Outdated
- Counter command | ||
``` | ||
show tacacs counter | ||
clear tacacs counter |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have a similar command sonic-clear
. ref: https://github.com/Azure/sonic-utilities/blob/888701b67fd4f1cc5b9da534a360048f93f263f4/setup.py#L157
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed with sonic-clear command, will push update later.
doc/aaa/TACACS+ Requirement.md
Outdated
|
||
## 2.4 System log | ||
- Generate system log when Authentication/Authorization/Accounting. | ||
- When remote TACACS server not avaliable, use system log for accounting. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's not necessary.
If tacacs server is accessible, we only put trace log to syslog for metering and debugging.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess your system log
== syslog
.
Then if the networking is not stable, you will have some random log lines locally, does it help anything?
For login authentication, the log messages always go to local syslog, no matter remote tacacs accessible or not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Document updated.
What I mean here is when the remote TACACS service not accessible, for example TACACS server down after user login, but networking still healthy, then the Authorization and Accounting information will write to syslog. Then the syslog will upload via network later, and those information will not lost.
doc/aaa/TACACS+ Design.md
Outdated
## 3.2 Server count | ||
- Max TACACS server count was hardcoded, default count is 8. | ||
|
||
## 3.1 Local authorization |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
doc/aaa/TACACS+ Design.md
Outdated
- Max TACACS server count was hardcoded, default count is 8. | ||
|
||
## 3.1 Local authorization | ||
- Operation system limitation: SONiC based on linux system, so permission to execute local command are managed by Linux file permission control. This means TACACS+ authorization can't config to disable 'local' authorization, and local authorization must be last authorization in authorization method list. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My point is that you have 2 options
- tacacs + local
- tacacs only, no local
And this is not a "Operation system limitation".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here is a example of this issue:
user name: domainuser
When user login, tacacs return privilege level 0 so in SONiC this user is a RO user, then this user on sonic only have Linux permission to run RO commands.
But in tacacs server side, the 'config' command was config as allow user run.
Then when user trying to run the 'config' command, tacacs authorization succeeded, but user still can't run this command because user not have Linux permission.
In EOS system, the behavior is different:
We add domainuser as local RO user.
In tacacs server config this user can run RW command.
Then we user run a RW command, user can run it.
The reason is the EOS system can bypass the local permission check, but SONiC is a Linux system, and currently we using Linux permission for 'local authorization', so tacacs authorization can't bypass the Linux permission check.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see your point!
Actually I am considering another config "tacacs only, no fallback to local", which means tacacs server unreachable, will stop the already-login user to run anything except logout
or exit
. Then the user need to re-login with local account, local authentication and local authorization.
doc/aaa/TACACS+ Design.md
Outdated
|
||
## 1.4 Docker support | ||
- Docker exec command will be covered by Authorization and Accounting. | ||
- Any command run inside docker container will not covered by Authorization and Accounting. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should move/add this to the limitations section.
A user can run commands inside a container to evade TACACS authorization and accounting.
It may be good to add a recommendation to administrators to disable access to 'docker exec' for most users.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
|
||
## 1.5 Multiple TACACS server | ||
- Support config multiple TACACS server. | ||
- When a server not accessible, will try next server as backup. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a way to specify which server is primary in the list of servers?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is no special flag to specify primary server, the first server in the TACACS+ config will will be the primary server.
To expand on my comments during the review meeting, here is a list of my concerns, but I don't know how you implemented this patch in bash so some of these concerns may be invalid, so please excuse/ignore them if you have already covered these cases. Concerns with tacacs+ command authorization: Cannot limit built-in commands:
I am not certain how you limit what bash can execute, is it only the top level command, or all sub-commands that a command might run?
Commands that can run other commands:
LD_LIBRARY_PATH LD_PRELOAD:
example:
Prefixes/Quoting: Example:
If you are going to go this route, it might be safer/easier to patch glibc execve directly to check user authorization... |
I wonder if SELinux would be better suited to restrict user commands? Here is some examples: https://access.redhat.com/solutions/65822 |
@seiferteric, Thanks for your comments, about your concern:
I verify following cases with my POC code, and command can be blocked with TACACS+ server side setting:
For the LD_PRELOAD issue, current solution can't block it because this not a command parameter. However this not be a issue because: |
doc/aaa/TACACS+ Design.md
Outdated
## 2.1 SONiC CLI | ||
- Enable/Disable TACACS Authorization/Accounting command | ||
``` | ||
config aaa authorization {local | tacacs+} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will change to config aaa authorization {local | tacacs+ | local tacacs+}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
Verify TACACS+ user can't run command not in server side whitelist. | ||
``` | ||
|
||
- config AAA authorization with TACACS+ only: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This a very practical work flow. Normally users found tacacs user could run nothing, they will logout and login with local account, and run command as local authorization, and even tacacs server recovered then, it does not impact local user command authorization. #Closed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
config AAA authorization with TACACS+ and local, but server not accessible:
Add this step: when remote tacacs accessible, local user still can run command. doesn't impact local user authorization.
Check if we have PR cover this: when tacacs accessible, local can't login.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Discuss offline, and you will add some step in another test case below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
doc/aaa/TACACS+ Design.md
Outdated
Verify TACACS+ user can run command not in server side whitelist but have permission in local. | ||
Verify TACACS+ user can't run command in server side whitelist but not have permission in local. | ||
Verify Local user can login, and run command with local permission. | ||
Verify after Local user login, then server accessible, Local user still can run command with local permission. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed. also update AAA table schema according to current SONiC yang model.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only minor word issue.
Please link the code PR by referring to this example #806 |
Uh oh!
There was an error while loading. Please reload this page.