Ldap #1877

phoenix-2021 · 2021-06-16T06:49:47Z

Pull Request Checklist

Login in to dendrite with username and password from ldap.

Signed-off-by: jiafeng zheng <[email protected]>

pull request 20210407

DoM1niC · 2021-06-18T17:42:26Z

#1813 will close

phoenix-2021 · 2021-06-20T04:33:56Z

can someone review the code or guide me what to do next? I'd love to perfect this pr can be merged.

kegsay

Aside from comments, this is a working opt-in LDAP solution. Dendrite will need an LDAP solution inevitably in the future. However, I don't know if we're at that point yet. There's numerous things to consider which this PR doesn't do yet:

Where in the LDAP directory to search: this PR searches everywhere the bind user has permission to.
Support for anonymous clients (that is, no bind user at all)
Any kind of SASL authentication (specifically ones which are more secure and do not send the plaintext password to the server, e.g RFC2222

We would also need some sort of Complement tests for this realistically. Thoughts @neilalexander ?

For comparison, Synapse has LDAP support, with a config that looks like:

password_providers:
#    # Example config for an LDAP auth provider
#    - module: "ldap_auth_provider.LdapAuthProvider"
#      config:
#        enabled: true
#        uri: "ldap://ldap.example.com:389"
#        start_tls: true
#        base: "ou=users,dc=example,dc=com"
#        attributes:
#           uid: "cn"
#           mail: "email"
#           name: "givenName"
#        #bind_dn:
#        #bind_password:
#        #filter: "(objectClass=posixAccount)"

kegsay · 2021-07-19T16:38:35Z

cmd/sytest/sytest.go

@@ -0,0 +1,114 @@
+package main


This binary seems unrelated to this PR.

Yes, it should be removed from here.

kegsay · 2021-07-19T16:40:14Z

clientapi/auth/password.go

+		if err != nil {
+			return nil, &util.JSONResponse{
+				Code: http.StatusUnauthorized,
+				JSON: jsonerror.InvalidUsername(err.Error()),


Unclear why this would be an invalid username. It's a badly configured LDAP address.

kegsay · 2021-07-19T16:40:54Z

clientapi/auth/password.go

+		if t.Config.LDAP.TLS {
+			addr = "ldaps://" + t.Config.LDAP.Host + ":" + t.Config.LDAP.Port
+		} else {
+			addr = "ldap://" + t.Config.LDAP.Host + ":" + t.Config.LDAP.Port


It might be easier to just have t.Config.LDAPAddr which can then mux together TLS/host/port e.g ldaps://localhost:8008

Good idea, it should be changed like this.

kegsay · 2021-07-19T16:41:30Z

clientapi/auth/password.go

+		if e1 != nil {
+			return nil, &util.JSONResponse{
+				Code: http.StatusUnauthorized,
+				JSON: jsonerror.InvalidUsername(err.Error()),


Should be using e1 not err

kegsay · 2021-07-19T16:41:51Z

clientapi/auth/password.go

+				JSON: jsonerror.InvalidUsername(err.Error()),
+			}
+		}
+		filter := fmt.Sprintf("(&%s(%s=%s))", t.Config.LDAP.Filter, "uid", localpart)


"uid" can just be put into the Sprintf?

kegsay · 2021-07-19T16:42:18Z

clientapi/auth/password.go

+			}
+		}
+		filter := fmt.Sprintf("(&%s(%s=%s))", t.Config.LDAP.Filter, "uid", localpart)
+		searchRequest := ldap.NewSearchRequest(t.Config.LDAP.BaseDN, ldap.ScopeWholeSubtree, ldap.NeverDerefAliases, 0, 0, false, filter, []string{"uid"}, nil)


We probably want this to be customisable.

kegsay · 2021-07-19T16:43:49Z

clientapi/auth/password.go

+		if len(sr.Entries) == 0 {
+			_, err = t.GetAccountByPassword(ctx, localpart, r.Password)
+			if err != nil {
+				return nil, &util.JSONResponse{


Rather than duplicating the code on :168-176 it would be better to fork the LDAP code into a separate function and communicate success/failure/fatal failure that way.

neilalexander

Thanks for the contribution! Ultimately this PR seems reasonable but I agree with @kegsay that we need to support, at a minimum:

restricting the search base to specific parts of the directory
restricting authentication to certain LDAP group memberships
avoiding plain-text auth

It's also important that we get the account management flow right here — for example, some questions to consider:

what happens if you delete or disable an LDAP account after creation?
can we convert an LDAP account to a local user or back?
what happens if you have local users and then enable LDAP later?

neilalexander · 2021-07-20T09:10:11Z

clientapi/auth/password.go

+		if err != nil {
+			return nil, &util.JSONResponse{
+				Code: http.StatusUnauthorized,
+				JSON: jsonerror.InvalidUsername(err.Error()),


I suspect this is the wrong return code for an invalid bind DN, since this error code will be returned to the client as if the user had entered a wrong account password?

neilalexander · 2021-07-20T09:12:56Z

clientapi/auth/password.go

@@ -62,6 +70,101 @@ func (t *LoginTypePassword) Login(ctx context.Context, req interface{}) (*Login,
 			JSON: jsonerror.InvalidUsername(err.Error()),
 		}
 	}
+	if len(t.Config.LDAP.Host) > 0 {


Does this mean that we can't have local accounts in addition to LDAP accounts?

neilalexander · 2021-07-20T09:16:38Z

clientapi/auth/password.go

+				err = t.UserAPI.PerformAccountCreation(ctx, &api.PerformAccountCreationRequest{
+					AppServiceID: "",
+					Localpart:    localpart,
+					Password:     r.Password,


This looks like we replicate the user's LDAP password from the point of their first login into the Dendrite account database, which I don't think we want to do since we should probably already be asking LDAP each time the user authenticates with their current password.

phoenix-2021 · 2021-08-02T03:21:53Z

Thanks for the contribution! Ultimately this PR seems reasonable but I agree with @kegsay that we need to support, at a minimum:

restricting the search base to specific parts of the directory

restricting authentication to certain LDAP group memberships

avoiding plain-text auth

It's also important that we get the account management flow right here — for example, some questions to consider:

what happens if you delete or disable an LDAP account after creation?

can we convert an LDAP account to a local user or back?

what happens if you have local users and then enable LDAP later?

Thank you very much for your guidance!
Since I am not familiar with dendrite and matrix, I have no idea how to deal with the relationship between local accounts and ldap, but I will continue to understand.
I have a small question, according to my understanding of LDAP, field LDAP.BaseDN in config can be used to specify the search scope, field LDAP.Filter can be used to filter groups, configured by the administrator here.

neilalexander · 2022-06-10T10:37:50Z

I tried to merge the latest main commits into this branch but it appears the PR doesn't allow maintainer edits. Please check the Allow edits from maintainers box.

neilalexander · 2022-07-26T15:21:34Z

Closing this for now because it is stale. If you get some bandwidth for this then please let us know and we can reopen it.

davidli18 and others added 8 commits April 7, 2021 11:47

Merge pull request #1 from matrix-org/master

19c8af7

pull request 20210407

Merge branch 'matrix-org:master' into master

1ba6606

add sytest cmd

ccbac43

add sytest cmd

fcfaed9

Merge branch 'matrix-org:master' into master

a2aeb7a

Login in to dendrite with username and password from ldap

dc1b1bb

Fix shadows declaration

813205a

Fix duplicated user in ldap

c2d4161

phoenix-2021 closed this Jun 20, 2021

phoenix-2021 reopened this Jun 20, 2021

kegsay reviewed Jul 19, 2021

View reviewed changes

kegsay added the needs discussion label Jul 19, 2021

neilalexander suggested changes Jul 20, 2021

View reviewed changes

Fix something

96d09c8

phoenix-2021 requested a review from a team as a code owner February 4, 2022 17:48

kegsay self-assigned this Feb 4, 2022

doamatto mentioned this pull request Apr 9, 2022

Setup chat service LLEB-ME/gouv.fa#9

Closed

neilalexander closed this Jul 26, 2022

matrixbot mentioned this pull request Nov 1, 2024

Ldap element-hq/dendrite#1877

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ldap #1877

Ldap #1877

phoenix-2021 commented Jun 16, 2021

DoM1niC commented Jun 18, 2021

phoenix-2021 commented Jun 20, 2021

kegsay left a comment •

edited

Loading

kegsay Jul 19, 2021

phoenix-2021 Aug 2, 2021

kegsay Jul 19, 2021

kegsay Jul 19, 2021

phoenix-2021 Aug 2, 2021

kegsay Jul 19, 2021

kegsay Jul 19, 2021

kegsay Jul 19, 2021

kegsay Jul 19, 2021

neilalexander left a comment

neilalexander Jul 20, 2021

neilalexander Jul 20, 2021

neilalexander Jul 20, 2021

phoenix-2021 commented Aug 2, 2021 •

edited

Loading

neilalexander commented Jun 10, 2022

neilalexander commented Jul 26, 2022

Ldap #1877

Ldap #1877

Conversation

phoenix-2021 commented Jun 16, 2021

Pull Request Checklist

DoM1niC commented Jun 18, 2021

phoenix-2021 commented Jun 20, 2021

kegsay left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

neilalexander left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

phoenix-2021 commented Aug 2, 2021 • edited Loading

neilalexander commented Jun 10, 2022

neilalexander commented Jul 26, 2022

kegsay left a comment •

edited

Loading

phoenix-2021 commented Aug 2, 2021 •

edited

Loading