-
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
Login through RADIUS is not working due to invalid shell #13141
base: master
Are you sure you want to change the base?
Login through RADIUS is not working due to invalid shell #13141
Conversation
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
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. @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. |
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.
Looks good to me. @lguohan / @zhangyanzhao - Please review.
@lguohan @qiluo-msft would you please help to approve this fix? it is needed for 202211 |
@qiluo-msft would you please help to review and approve? |
@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 |
@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"; |
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.
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.
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.
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 ?
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 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"; |
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 readonly user should be in users
group, and no docker
group membership. #WontFix
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.
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 ?
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)
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.