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

Add SONiC design doc: TACACS+ improvement #813

Merged
merged 19 commits into from
Oct 21, 2021
Merged

Conversation

@liuh-80 liuh-80 requested review from qiluo-msft and lguohan July 9, 2021 04:49
- 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:
Copy link
Contributor

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?

Copy link
Contributor Author

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.

- 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.
Copy link
Contributor

@qiluo-msft qiluo-msft Jul 9, 2021

Choose a reason for hiding this comment

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

Redirecting

What are the alternatives for user to achieve similar goal? #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.

By patch bash, we can allow user use redirect, will remove this from document.

Copy link
Contributor

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.

Copy link
Contributor Author

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:

  1. create a variable isReadonlyUser

  2. When bash startup, check current user group and set this variable.

  3. 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 */

- 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:
Copy link
Contributor

@qiluo-msft qiluo-msft Jul 9, 2021

Choose a reason for hiding this comment

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

Disable

Disable for RO users are understandable. Is it possible to keep RW users capability at the same time of AAA? #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.

In that case, we need consider some other solution. I will check other solution in POC and update this document.

Copy link
Contributor Author

@liuh-80 liuh-80 Jul 15, 2021

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:

  1. 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.

  2. 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.

Copy link
Contributor Author

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.

- Counter command
```
show tacacs counter
clear tacacs counter
Copy link
Contributor

@qiluo-msft qiluo-msft Jul 9, 2021

Choose a reason for hiding this comment

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

clear

clear is part of ncurses-bin. We'd better not to override. #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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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 with sonic-clear command, will push update later.


## 2.4 System log
- Generate system log when Authentication/Authorization/Accounting.
- When remote TACACS server not avaliable, use system log for accounting.
Copy link
Contributor

@qiluo-msft qiluo-msft Jul 9, 2021

Choose a reason for hiding this comment

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

When remote TACACS server not avaliable

When tacacs server is accessible, do we also use local syslog to account? #WontFix

Copy link
Contributor Author

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.

Copy link
Contributor

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.

Copy link
Contributor Author

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.

@liuh-80 liuh-80 changed the title Add TACACS+ protocol requirement document. Add TACACS+ protocol design document. Jul 29, 2021
## 3.2 Server count
- Max TACACS server count was hardcoded, default count is 8.

## 3.1 Local authorization
Copy link
Contributor

@qiluo-msft qiluo-msft Jul 29, 2021

Choose a reason for hiding this comment

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

3.1

Section numbers are wrong. #Closed

Choose a reason for hiding this comment

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

Fixed.

- 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.
Copy link
Contributor

@qiluo-msft qiluo-msft Jul 29, 2021

Choose a reason for hiding this comment

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

can't config to disable 'local' authorization

If tacacs server is not reachable during authorization, we have option to fail the authorization, so local auth is disabled. #Closed

Choose a reason for hiding this comment

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

Fixed.

Copy link
Contributor

@qiluo-msft qiluo-msft Aug 4, 2021

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

  1. tacacs + local
  2. tacacs only, no local
    And this is not a "Operation system limitation".

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.

Copy link
Contributor

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.

@seiferteric
Copy link

I wonder if SELinux would be better suited to restrict user commands? Here is some examples: https://access.redhat.com/solutions/65822

@liuh-80
Copy link
Contributor Author

liuh-80 commented Sep 22, 2021

@seiferteric, Thanks for your comments, about your concern:

  1. The reason we doesn't limit built-in commands: in bash, when user run any command stored in disk, the code will go to following method:
    execute_cmd.c

                    /* Call execve (), handling interpreting shell scripts, and handling
              exec failures. */
           int
           shell_execve (command, args, env)
                char *command;
                char **args, **env;
    

    And our patch will work inside this method to check TACACS+ authorization before command been executed.
    Then user cannot use built-in command/function/variable to bypass TACACS+ authorization. So following way to bypass authorization will not work:
    1. Use built-in command to bypass authorization.
    2. Use shell function to bypass authorization.
    3. Use shell variable to bypass authorization.

  2. For the bash script authorization, we will only check the script name, and will not check the sub commands inside the script, this may cause potential security issue, so for RO user, administrator should disable user from run any user defined script. and for Admin user, because they already have permission to do anything, so it's not necessary to block them from create/run their scripts.

  3. Start another harmless process and run command from it will be a security risk, so administrator should disable RO user from run following commands:
    1. Run another shell.
    2. Run another interpreter command.
    3. Run loader.
    4. Any command can run other commands, for example: find, VI.

I verify following cases with my POC code, and command can be blocked with TACACS+ server side setting:

  1. Run script with sh/python or other interpreter.
  2. Run find with -exec parameter
  3. Run loader
  4. Use Prefixes/Quoting to run command.

For the LD_PRELOAD issue, current solution can't block it because this not a command parameter. However this not be a issue because:
For RO user, we tested this trick work with RO user, this have risk but not related with tacacs per-command authorization feature.
Also sonic sudoers file already disable user set LD_PRELOAD environment with sudo command, so this trick not have big impact.
For RW user, they already have permission to do almost all operation, for example create their own script and run it. so this will not be a risk.

## 2.1 SONiC CLI
- Enable/Disable TACACS Authorization/Accounting command
```
config aaa authorization {local | tacacs+}
Copy link
Contributor

@qiluo-msft qiluo-msft Sep 22, 2021

Choose a reason for hiding this comment

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

local | tacacs

In below sectin, you support combined local | tacacs, which is different from this syntax. #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.

Will change to config aaa authorization {local | tacacs+ | local tacacs+}

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.

Verify TACACS+ user can't run command not in server side whitelist.
```

- config AAA authorization with TACACS+ only:
Copy link
Contributor

@qiluo-msft qiluo-msft Sep 22, 2021

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+ only

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

Copy link
Contributor Author

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.

Copy link
Contributor

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.

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.

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.
Copy link
Contributor

@qiluo-msft qiluo-msft Oct 12, 2021

Choose a reason for hiding this comment

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

server accessible

Do you mean "server becomes accessible" #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. also update AAA table schema according to current SONiC yang model.

qiluo-msft
qiluo-msft previously approved these changes Oct 12, 2021
Copy link
Contributor

@qiluo-msft qiluo-msft left a 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.

@zhangyanzhao
Copy link
Collaborator

Please link the code PR by referring to this example #806

@liuh-80 liuh-80 merged commit 11cf67a into sonic-net:master Oct 21, 2021
@zhangyanzhao
Copy link
Collaborator

@liuh-80 please update this PR to add the code PRs with example of #806. Thanks.

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

Successfully merging this pull request may close these issues.

8 participants