-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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+]: Add TACACS+ Authentication #746
Conversation
Summary: * Add pam-tacplus module * Add nss-tacplus module * Add TACACS+ make rules and install script * Add TACACS+ build dependence packages * Add remote_user and remote_user_su * Add AAA configuration file
.gitmodules
Outdated
@@ -66,3 +66,9 @@ | |||
[submodule "platform/broadcom/sonic-platform-modules-accton"] | |||
path = platform/broadcom/sonic-platform-modules-accton | |||
url = https://github.com/edge-core/sonic-platform-modules-accton.git | |||
[submodule "src/tacacs/sonic-pam-tacplus"] | |||
path = src/tacacs/sonic-pam-tacplus | |||
url = https://github.com/liuqu/sonic-pam-tacplus.git |
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 can see only one patch is made. It is better maintian it as a patch, like what we did for libteam.
Can you follow that as an example?
https://github.com/Azure/sonic-buildimage/tree/master/src/libteam
ttps://github.com/liuqu/sonic-pam-tacplus/commit/6aab5b6cafb65763e297f99e889418677528cd35
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.
ok, I will change it.
.gitmodules
Outdated
url = https://github.com/liuqu/sonic-pam-tacplus.git | ||
[submodule "src/tacacs/sonic-nss-tacplus"] | ||
path = src/tacacs/sonic-nss-tacplus | ||
url = https://github.com/liuqu/sonic-nss-tacplus.git |
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.
can you maintain this as a patch?
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 is a NSS plugin for TACACS+. Do you mean to maintain it as a patch for pam-tacplus?
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 will think about how to change it as a patch to make.
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.
Take https://github.com/Azure/sonic-buildimage/tree/master/src/initramfs-tools as an example. The patching method is recommended if your changes is small.
build_debian.sh
Outdated
@@ -158,6 +158,13 @@ sudo LANG=C chroot $FILESYSTEM_ROOT useradd -G sudo,docker $USERNAME -c "$DEFAUL | |||
## Create password for the default user | |||
echo $USERNAME:$PASSWORD_ENCRYPTED | sudo LANG=C chroot $FILESYSTEM_ROOT chpasswd -e | |||
|
|||
## Create remote user |
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.
can you explain why we need this remote user?
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 used for user role map. If an authenticated user only exists in TACACS+ server database, not exists in local, it can't get passwd info without user role map. So I create remote user for TACACS+ user.
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 is confusing for all other users not using TACACS+.
- Do we need to install rbash on SONiC host?
- The username, uid, gid are hard-coded. Are they wellknown in 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.
Thanks for your review.
- You're right. I will integrate the users' creation for TACACS+ Authentication into TACACS+ command. When the user enable TACACS+ by command, the remote users will be created.
- We don't need to install rbash. It's used for limit the TACACS+ user who only have show privilege. We will replace it with the CLI shell.
- The nss plugin for TACACS+ provides the getpwnam_r() entry point. It parses the privilege level of TACACS+ user by connecting with TACACS+ server, and maps to the pre-added user. If a use's priv-level is 15, the nss plugin will return the passwd info of remote_user_su in /etc/passwd.
@@ -89,6 +89,16 @@ sudo cp -f $IMAGE_CONFIGS/bash/bash.bashrc $FILESYSTEM_ROOT/etc/ | |||
sudo dpkg --root=$FILESYSTEM_ROOT -i target/debs/sonic-device-data_*.deb || \ | |||
sudo LANG=C DEBIAN_FRONTEND=noninteractive chroot $FILESYSTEM_ROOT apt-get -y install -f | |||
|
|||
# Install pam-tacplus |
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 it possible to capsulate the TACACS+ functionality into a docker container like docker-vas? It is not an easy job but technically feasible.
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 pam_tacplus is only a dynamic library for Linux PAM module, and nss-tacplus is a dynamic library for Linux NSS module. I think there's no need to capsulate them into a docker container.
* Delete sonic-nss-tacplus and sonic-pam-tacplus submodule * The nss-tacplus plugin is applied as patch for pam-tacplus * Create remote user in aaa_config when TACACS+ enable, not default in build image * Remove TACACS+ default config file, load default config by aaa config
* Modify the default user mapping policy, create local user for each TACACS+ user. * Add configuration to change the default gid, group and shell of the local user to be created.
0203d7f
to
c5765f9
Compare
* Delete build dependence 'autoconf-archive'
[vs] Skip MACsec clean up if /sbin/ip is not accessible (sonic-net#750) Configure enable -Wcast-align=strict when supported by compiler (sonic-net#749) [syncd] Translate depreacated attr enum values to new ones (sonic-net#746) [sairedis]vs SAI support for voq neighbor (sonic-net#725) Signed-off-by: Sabareesh Kumar Anandan <[email protected]>
[vs] Add workaround for clean up macsec ports (#752) [logfile]: Add handling of Sairedis rec filename (#747) Update README.md [meta] Fix stat_mode enums to sai_bulk_op_error_mode_t (#753) [syncd][tests] Add syncd deprecated attribute value test (#751) [vs] Skip MACsec clean up if /sbin/ip is not accessible (#750) Configure enable -Wcast-align=strict when supported by compiler (#749) [syncd] Translate depreacated attr enum values to new ones (#746) [sairedis]vs SAI support for voq neighbor (#725) [syncd] Translate removed RIDs in fdb notification (#734) [syncd] Move syncd classes to syncd namespace (#742) [vs] Use /sbin/ip absolute path for ip command in MACsecManager (#744) [saidiscovery] Update saidiscovery to use VendorSai object and metadata (#736) Remove Winline warning since it depends on external headers (#741) [meta] Enable strict cast-align warning (#738) [vs] Use meta class instead info when using unittests (#740) [vs] Support flush entry type all on virtual switch (#735) [vslib]: Add MACsec state to state base (#722) [README.md] Update installation steps (#730) Switch Capability support (#728) [vs] Fail switch create when warm boot requested and no warm boot state (#739) Dynamic Port breakout fix the crash, port down event processing after<80> (#727) Code clean (#721) Signed-off-by: Sabareesh Kumar Anandan <[email protected]>
…#746) If attribute is enum, and we are loading some older SAI values it may happen that we get deprecated/ignored value string and we need to update it to current one to not cause attribute compare confusion since they are compared by string value. This fixes issues when warm shutdown on 201811 branch, and warm boot on master branch.
Signed-off-by: Liuqu [email protected]