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.

Sign up for GitHub

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

Jump to bottom

Add LDAP feature support #16969

Merged
merged 4 commits into from May 7, 2024
Merged

Add LDAP feature support #16969

merged 4 commits into from May 7, 2024
+219 −4

Conversation

davidpil2002
Copy link
Contributor

@davidpil2002 davidpil2002 commented Oct 23, 2023 •
edited

Why I did it

To support LDAP feature

Work item tracking
  • Microsoft ADO (number only):

How I did it

Similar to Radius/Tacacs authentication methods, the SONiC device is the LDAP client.
Installed the Debian LDAP packages related to making SONiC able to function as an LDAP client.

More description in the following HLD:
sonic-net/SONiC#1487

How to verify it

Do LDAP configuration according to the HLD, then connect to the SONiC switch by using a user that exists in your LDAP server.

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

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

Tested branch (Please provide the tested image version)

Description for the changelog

Link to config_db schema for YANG module changes

YANG Schema can be found attached in the HLD as well.

A picture of a cute animal (not mandatory but encouraged)

@davidpil2002 davidpil2002 mentioned this pull request Oct 23, 2023
Merged
@davidpil2002 davidpil2002 mentioned this pull request Nov 7, 2023
Merged
@davidpil2002 davidpil2002 force-pushed the dev-add-ldap-support branch 3 times, most recently from e34d037 to d3ac900 Compare December 3, 2023 10:22
@liat-grozovik
Copy link
Collaborator

@lguohan who should review and provide feedback?

@qiluo-msft
Copy link
Collaborator

@a-barboza Could you also help review this PR?

@shdasari
Copy link
Contributor

yang-model tests are missing for LDAP, please add the same.

@davidpil2002 davidpil2002 force-pushed the dev-add-ldap-support branch 2 times, most recently from e4d1f05 to 553ac32 Compare December 20, 2023 09:01
@davidpil2002 davidpil2002 reopened this Dec 20, 2023
@davidpil2002 davidpil2002 force-pushed the dev-add-ldap-support branch 4 times, most recently from 4343811 to 4ea585d Compare December 25, 2023 14:20
@davidpil2002
Copy link
Contributor Author

error look not related to the new code.
rerunning:
/azpw run Azure.sonic-buildimage

@davidpil2002
Copy link
Contributor Author

/azpw run Azure.sonic-buildimage

@mssonicbld
Copy link
Collaborator

/AzurePipelines run Azure.sonic-buildimage

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@davidpil2002
Copy link
Contributor Author

yang-model tests are missing for LDAP, please add the same.

DONE

@davidpil2002
Copy link
Contributor Author

All comments was answered,
pls can you re-review/approve?
@shdasari @asha-behera @a-barboza @madhupalu

@fastiuk
Copy link
Contributor

fastiuk commented Jan 29, 2024

@a-barboza , could you please review? I saw you did HLD review as well

@davidpil2002 davidpil2002 force-pushed the dev-add-ldap-support branch 2 times, most recently from 9485c3b to 31c62b6 Compare February 29, 2024 08:01
qiluo-msft
qiluo-msft reviewed Mar 23, 2024
View reviewed changes
files/build_templates/sonic_debian_extension.j2
@@ -272,6 +272,19 @@ sudo dpkg --root=$FILESYSTEM_ROOT -i $debs_path/sonic-device-data_*.deb || \
# package for supporting password hardening
sudo LANG=C DEBIAN_FRONTEND=noninteractive chroot $FILESYSTEM_ROOT apt-get -y install libpam-pwquality

# Install pam-ldap, nss-ldap, ldap-utils
Copy link
Collaborator

Choose a reason for hiding this comment

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

Install

Suggest to add build time config, so deployers not using LDAP could reduce image size, and reduce the security concerns.

Copy link

Choose a reason for hiding this comment

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

Why should this be different than radius/tacacs? Why show different approaches for a feature in the same domain?

@liat-grozovik
Copy link
Collaborator

/azpw run Azure.sonic-buildimage

@liat-grozovik liat-grozovik reopened this May 6, 2024
liat-grozovik
liat-grozovik approved these changes May 7, 2024
View reviewed changes
Copy link
Collaborator

@liat-grozovik liat-grozovik left a comment

Choose a reason for hiding this comment

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

offline discussion confirms PR has no additional comments and thus can be merged.
if more comments will be provided after the merge it will be discussed and considered if change is required.

@liat-grozovik liat-grozovik merged commit afdc5d0 into sonic-net:master May 7, 2024
21 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@qiluo-msft qiluo-msft qiluo-msft left review comments

@a-barboza a-barboza a-barboza requested changes

@madhupalu madhupalu madhupalu left review comments

@asha-behera asha-behera asha-behera left review comments

@Yarden-Z Yarden-Z Yarden-Z left review comments

@liat-grozovik liat-grozovik liat-grozovik approved these changes

@lguohan lguohan Awaiting requested review from lguohan lguohan is a code owner

@liuh-80 liuh-80 Awaiting requested review from liuh-80

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
10 participants
@davidpil2002 @liat-grozovik @qiluo-msft @shdasari @mssonicbld @fastiuk @a-barboza @madhupalu @asha-behera @Yarden-Z