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 LDAP HLD #1487

Merged
merged 9 commits into from
Apr 14, 2024
Merged

Add LDAP HLD #1487

merged 9 commits into from
Apr 14, 2024

Conversation

davidpil2002
Copy link
Contributor

@davidpil2002 davidpil2002 commented Sep 27, 2023

This LDAP HLD doc describes the requirements, architecture and configuration details of LDAP feature in SONiC.

PR title state context
[sonic-buildimage] Added support for LDAP GitHub issue/pull request detail GitHub pull request check contexts
[sonic-utilities] Added support for LDAP GitHub issue/pull request detail GitHub pull request check contexts
[sonic-host-services]Added support for LDAP GitHub issue/pull request detail GitHub pull request check contexts

@davidpil2002 davidpil2002 force-pushed the dev-ldap-hld branch 3 times, most recently from 5d8e620 to c411de6 Compare October 2, 2023 08:49
@zhangyanzhao
Copy link
Collaborator

- Configure version

#### System Test cases

Copy link
Contributor

Choose a reason for hiding this comment

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

Is the LDAP test cases integrated with sonic-mgmt?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, the LDAP functional test should be integrated to sonic-mgmt

LDAP Main flow – As mentioned in the LDAP authentication desc, the LDAP supports authentication by authenticating users via a remote server instead of locally (in the switch device).
In high level the connection flow is the following:
User will connect to a switch using ssh/login, the switch is an LDAP client (configured with LDAP feature - description of the configuration flow below), the client switch will “referred” the authentication of the user to the LDAP server binded, then the LDAP server will approve the authentication if the user & password match the LDAP server DB.
And finally, the user will get approved and will be connected to the Switch.
Copy link
Contributor

Choose a reason for hiding this comment

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

How does the design handle LDAP client handles w.r.t LDAP server users lifecycle (add/del/update) and any stale entries?

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 our design, LDAP clients don't handle the deletion/addition/modification of users.
For now, we decided to remain aligned to the original design of the LDAP package, which means, that only the LDAP server is allowed to add/remove users and configure the permissions (mainly by setting to which group each user belongs.)

Note:
LDAP package has a tool named ldapmodify that can be used to modify the configuration in the server after passing LDAP bind authentication by remote, but we are not going to support this flow in the CLI.
If a user likes to use this application it will be his responsibility.

Authentication failed – user will not be able to connect like regular authentication fail.

### Restrictions/Limitations

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 support multiple LDAP servers with various priorities?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, can be seen in the schema

Note: no restart is required when modifying PAM configuration. Only required to restart the NSLCD service after any modification in nslcd.conf file

#### LDAP NSS
LDAP can be used as an option in the Name Services Switch(NSS) configuration. The NSS configuration enables various programming APIs to use other sources than the default files (e.g., Use LDAP directory information instead of /etc/passwd for user and group information). User information includes uid, gid, and home directory.
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the rationale behind using NSL CD instead of SSSD for ldap?
There is a concern was the NSLCD does not support cache so it affects performance for scaling w.r.t many users. Is there any reason to go with NSLCD?
Can you explore SSSD as well.

Copy link
Contributor Author

@davidpil2002 davidpil2002 Oct 12, 2023

Choose a reason for hiding this comment

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

NSLCD working smoothly, until today we haven't issues related to performances.
SSSD it's more complex to configure and use, so, for now, we go to the NSLCD direction.
The PR will be share soon

But, it can be a future item to review.
should be review:
if the performance really enhances in our use cases and also if sssd makes the configuration even more complex to the customer from the UX (User experience).

@davidpil2002
Copy link
Contributor Author

@madhupalu

answered all the comments, pls can you approve/review?

Specifies the credentials with which to bind. This option is only applicable when used with binddn above. If you set this option you should consider changing the permissions of the nslcd.conf file to only grant access to the root user.

So, this file will contain all the LDAP configurations besides the login configuration that was described in the functional section above.
Note: no restart is required when modifying PAM configuration. Only required to restart the NSLCD service after any modification in nslcd.conf file
Copy link
Collaborator

Choose a reason for hiding this comment

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

Are any changes being made to nslcd sources ? i.e. will the nslcd be source patched in SONiC ? It appears many of the CONFIG_DB schema (key) names for LDAP (or the corresponding sonic yang) names are not matching the corresponding nslcd.conf file configuration directives. One advantage of keeping the names similar (wherever possible) would be the ease of mapping the names in the hostcfg/jinja template to the corresponding nslcd.conf options. (Eg: binddn as opposed to bind_dn, bindpw as opposed to bind_password). Some options are missing from the nslcd.conf (Eg: hostname_check). Are those changes made to nslcd ? Would this be in the code PR shared?

Copy link
Contributor Author

@davidpil2002 davidpil2002 Oct 22, 2023

Choose a reason for hiding this comment

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

nslcd will be installed, and is part of those packages: libnss-ldapd libpam-ldapd - no patches are required to be supported in SONiC.
about the naming, we will try to be aligned, but sometimes the name has no good human-readable name.

hostname_check - this is not part of the code, we will remove it from HLD.

I will share the PR soon, hope this week, due to the situation in IL, could be delayed a little bit.

in conclusion - at first, we will support the minimum requirement to use the remote authentication of LDAP.
(already tested internally in SONiC)

In the future, if required more LDAP features can be added on top of our code that will support easy extension in LDAP class

@davidpil2002 davidpil2002 force-pushed the dev-ldap-hld branch 2 times, most recently from 0e68b6d to 094b958 Compare October 22, 2023 13:04
…hat supported and tested. (more feature can be added in the future if required)
@davidpil2002
Copy link
Contributor Author

davidpil2002 commented Oct 23, 2023

Share the PRs related to the feature support. Pls see in the HLD description

WC to merge/comment the HLD and start the PRs review

@liat-grozovik
Copy link
Collaborator

@zhangyanzhao all comments and answers were handled. HLD should be merged. Can you please assist?

@qiluo-msft
Copy link
Contributor

@a-barboza @madhupalu Do you have more concern? Could you explicitly approve PR if you are satisfying?

@Yarden-Z
Copy link
Contributor

@a-barboza can you please approve this HLD?

@a-barboza
Copy link
Collaborator

@a-barboza can you please approve this HLD?

@Yarden-Z : Can you please check this submission? There are some issues which were deferred from the HLD to the code PR for details..

@davidpil2002
Copy link
Contributor Author

davidpil2002 commented Feb 1, 2024

@a-barboza can you please approve this HLD?

@Yarden-Z : Can you please check this submission? There are some issues which were deferred from the HLD to the code PR for details..

We approved some name suggestions and some not.
Added a commit that aligns the name fixes done in the yang model to the yang model appended in the HLD.
no more name fixes are planned to be done.

@a-barboza
pls, can you approve this HLD PR?

@zhangyanzhao
Copy link
Collaborator

@a-barboza @madhupalu can you please help to review this PR and approve if no more concern? Thanks.

Hostcfgd – listen to changes in CONFIG_DB in the LDAP table, and when the table has a new modification/or init happens it will trigger a callback in hostcfgd handle in AAA class to modify the PAM & NSS configuration files in Linux.

#### SSH/Local login
After enabling the LDAP configuration the ssh/local login to the switch will be authenticated by the LDAP server.
Copy link

@nmoray nmoray Feb 6, 2024

Choose a reason for hiding this comment

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

I have one query related to the authentication. Is there a passkey used to locally authenticate an user against the centralised identity management system? Similar to TACACS and RADIUS?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes - similat to tacacs/radius, you have a passkey to bind to the AAA server, and then, a user need to authenticate via the AAA server authentication defined in the AAA server.

@liat-grozovik
Copy link
Collaborator

@zhangyanzhao i think we need to close this HLD PR
that was review long time ago and comments were addressed. additional review feedback can be provided but should not hold the PR review nor the HLD approval flow. please help to close the loop

@liat-grozovik
Copy link
Collaborator

As not further comments or feedback , this PR is getting merge. Code PR should be reviewed and this feature should be included in 202405.

@liat-grozovik
Copy link
Collaborator

@davidpil2002 please go over the PRs, ensure they are aligned with the HLD and ask for review comments.
we have few more weeks to close 202405 and since this is under review for quite some time we dont want to miss 202405.

@liat-grozovik liat-grozovik merged commit e888d97 into sonic-net:master Apr 14, 2024
1 check passed
@liat-grozovik liat-grozovik mentioned this pull request Apr 25, 2024
liat-grozovik pushed a commit to sonic-net/sonic-buildimage that referenced this pull request May 7, 2024
Why I did it
To support LDAP feature

- 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.
liat-grozovik pushed a commit to sonic-net/sonic-utilities that referenced this pull request May 15, 2024
- What I did
Add LDAP CLI

- How I did it
created the CLI by using YANG model generator, the YANG model can be found in the LDAP HLD:
sonic-net/SONiC#1487

- How to verify it
Manually:
you can use configurations command like"config ldap global " or
"show ldap global" (more examples in the HLD.)
Auto:
1.There are unitest of each policy including good & bad flow in this commit, that should pass.
arfeigin pushed a commit to arfeigin/sonic-utilities that referenced this pull request Jun 16, 2024
- What I did
Add LDAP CLI

- How I did it
created the CLI by using YANG model generator, the YANG model can be found in the LDAP HLD:
sonic-net/SONiC#1487

- How to verify it
Manually:
you can use configurations command like"config ldap global " or
"show ldap global" (more examples in the HLD.)
Auto:
1.There are unitest of each policy including good & bad flow in this commit, that should pass.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

9 participants