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

Login through RADIUS is not working due to invalid shell #13141

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

saravanan-i
Copy link

Why I did it

RADIUS presumes that sonic launch shell (/usr/bin/sonic-launch-shell) is available by default to be authenticated users, which is not the case. This leads to failure in RADIUS authentication.

How I did it

Added valid bash shell (/bin/bash) in RADIUS nss code

How to verify it

Verification of remote RADIUS user with valid credentials is authenticated without any failure - PASS

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

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

Description for the changelog

Fixed RADIUS user authentication issue due to invalid shell (sonic-launch-shell)
Resolves

This pull request resolves PR#11352 (#11352) under sonic-buildimage repository.

Root-Cause: RADIUS presumes that sonic launch shell (/usr/bin/sonic-launch-shell) is available by default to be authenticated users, which is not the case. This leads to failure in RADIUS authentication.
What I did: Added valid bash shell (/bin/bash) in RADIUS nss code
@saravanan-i saravanan-i changed the title Issue Description: Login through RADIUS is not working Login through RADIUS is not working due to invalid shell Dec 22, 2022
@saravanan-i saravanan-i marked this pull request as ready for review December 22, 2022 10:22
@saravanan-i saravanan-i changed the title Login through RADIUS is not working due to invalid shell Draft: Login through RADIUS is not working due to invalid shell Dec 23, 2022
@saravanan-i saravanan-i marked this pull request as draft December 23, 2022 08:45
@saravanan-i saravanan-i marked this pull request as ready for review December 23, 2022 13:31
@saravanan-i saravanan-i changed the title Draft: Login through RADIUS is not working due to invalid shell Login through RADIUS is not working due to invalid shell Dec 23, 2022
@DavidZagury
Copy link
Contributor

Looks good to me. However, in the triage meeting where we first discussed this issue, we asked BRCM to check the reason it was first built with that shell.
I don't think we got a answer about that.

@zhangyanzhao did BRCM check that? can we conclude that the shell really is wrong and we can change it? if so, this PR should fix the issue.

Copy link

@dharmaraj-gurusamy dharmaraj-gurusamy left a comment

Choose a reason for hiding this comment

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

Looks good to me. @lguohan / @zhangyanzhao - Please review.

@liat-grozovik
Copy link
Collaborator

@lguohan @qiluo-msft would you please help to approve this fix? it is needed for 202211

@keboliu keboliu requested a review from qiluo-msft April 8, 2023 05:57
@keboliu
Copy link
Collaborator

keboliu commented Apr 8, 2023

@qiluo-msft would you please help to review and approve?

@lguohan
Copy link
Collaborator

lguohan commented Apr 13, 2023

THIS PR also claims fix #11352

#14466

can we get clarification? which pr fix? which one to go?

@keboliu
Copy link
Collaborator

keboliu commented Apr 16, 2023

THIS PR also claims fix #11352

#14466

can we get clarification? which pr fix? which one to go?

@saravanan-i would you please clarify?

@saravanan-i
Copy link
Author

saravanan-i commented Apr 18, 2023

THIS PR also claims fix #11352
#14466
can we get clarification? which pr fix? which one to go?

@saravanan-i would you please clarify?

This PR has corrected the invalid soft link ("sonic-launch-shell") to valid RADIUS login launch shell("/bin/bash"). Other authentication protocols like TACACS use this shell ("/bin/bash") in SoNIC. Link to TACACS code which use the shell ("/bin/bash") TACACS_Link_shell

@zhangyanzhao
Copy link
Collaborator

@adyeung can you please help to review this PR? Thanks.

@@ -159,11 +159,11 @@ static void init_rnm(RADIUS_NSS_CONF_B * conf) {
rnm[0].gid = 999;
rnm[0].groups = "docker";

Choose a reason for hiding this comment

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

To reduce the attack surface, maybe change the default group given here.
MPL = 0 user does not require docker, but maybe it does require the redis group or adm group.

Copy link
Author

@saravanan-i saravanan-i Jul 18, 2023

Choose a reason for hiding this comment

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

Hi @Yarden-Z . They were defined as per the existing RADIUS design document of SoNIC ( RADIUS_Design ). I haven't introduced them as part of this pull request. Can you please confirm if you have to change the same ?

Choose a reason for hiding this comment

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

I understand that these values were introduced in the HLD design.
But given this item - I think we can re-visit this.
From my perspective, a non-privileged user requires redis (potentially) and the default adm group.
If I understood your question

@@ -159,11 +159,11 @@ static void init_rnm(RADIUS_NSS_CONF_B * conf) {
rnm[0].gid = 999;
rnm[0].groups = "docker";
Copy link
Collaborator

@qiluo-msft qiluo-msft Jun 27, 2023

Choose a reason for hiding this comment

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

The readonly user should be in users group, and no docker group membership. #WontFix

Copy link
Author

Choose a reason for hiding this comment

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

Hi @qiluo-msft. User groups were defined based on existing RADIUS design document of SoNiC RADIUS_Design. I haven't introduced them as part of this pull request. Can you please confirm if we have to modify the same ?

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.

None yet

9 participants