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

Move user password verification after checking his groups on ldap auth #19587

Merged
merged 2 commits into from
May 3, 2022
Merged

Conversation

3l0w
Copy link
Contributor

@3l0w 3l0w commented May 2, 2022

This PR move the user password verifications after groups checking.

In my LDAP setup i noticed that groups check always failed because the bind dn changed to be the user one, or the user don't have the permission to see his roles

Signed-off-by: Gwilherm Folliot <gwilherm55fo@gmail.com>
@codecov-commenter

This comment was marked as off-topic.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label May 2, 2022
@Gusted Gusted added this to the 1.17.0 milestone May 2, 2022
@Gusted
Copy link
Contributor

Gusted commented May 2, 2022

In my LDAP setup i noticed that groups check always failed because the bind dn changed to be the user one, or the user don't have the permission to see his roles

This feels like possible misuse of bindUser, should a comment be added to this function and "warn" about the behavior?

@3l0w
Copy link
Contributor Author

3l0w commented May 2, 2022

I don't think this is a misuse because since the checkbox attributes in bind is checked the bindUser should be used to retreive all the informations including groups of the user. Without this modification the user check himself his groups even if the attributes in bind is used.
I don't think a comment will useful

@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels May 2, 2022
@wxiaoguang
Copy link
Contributor

I can not understand why this patch works.

If bindUser fails, the SearchEntry still returns a nil, the same behavior as before. What's the difference?

@3l0w
Copy link
Contributor Author

3l0w commented May 3, 2022

The difference is when the group is fetched it call the method search who use the bindUser to authenticate in the ldap server. But if the bindUser is not anymore the admin (or something with the permission to read inside the ldap), the search will fail since he will not have the permission.

Here are the openldap logs without the patch, at the send it said that it bindint the user DevTest and then fetching the groups but there is a error 32 which mean LDAP_NO_SUCH_OBJECT

6270f18d conn=3952 fd=48 ACCEPT from IP=***.***.***.***:53914 (IP=0.0.0.0:389)
6270f18d conn=3952 op=0 BIND dn="cn=admin,dc=mycompagny,dc=com" method=128
6270f18d conn=3952 op=0 BIND dn="cn=admin,dc=mycompagny,dc=com" mech=SIMPLE ssf=0
6270f18d conn=3952 op=0 RESULT tag=97 err=0 text=
6270f18d conn=3952 op=1 SRCH base="ou=users,dc=mycompagny,dc=com" scope=2 deref=0 filter="(&(objectClass=inetOrgPerson)(uid=devtest)(memberOf=cn=developpeur,ou=groups,dc=mycompagny,dc=com))"
6270f18d conn=3952 op=1 SEARCH RESULT tag=101 err=0 nentries=1 text=
6270f18d conn=3952 op=2 SRCH base="cn=DevTest,ou=users,dc=mycompagny,dc=com" scope=2 deref=0 filter="(&(objectClass=inetOrgPerson)(uid=devtest)(memberOf=cn=developpeur,ou=groups,dc=mycompagny,dc=com))"
6270f18d conn=3952 op=2 SRCH attr=uid 1.1 1.1 mail dn
6270f18d conn=3952 op=2 SEARCH RESULT tag=101 err=0 nentries=1 text=
6270f18d conn=3952 op=3 SRCH base="ou=groups,dc=mycompagny,dc=com" scope=2 deref=0 filter="(|(objectClass=groupOfNames)(objectClass=groupOfUniqueNames)(objectClass=posixGroup))"
6270f18d conn=3952 op=3 SRCH attr=uniqueMember
6270f18d conn=3952 op=3 SEARCH RESULT tag=101 err=0 nentries=15 text=
6270f18d conn=3952 op=4 SRCH base="cn=DevTest,ou=users,dc=mycompagny,dc=com" scope=2 deref=0 filter="(|(memberOf=cn=admin,ou=groups,dc=mycompagny,dc=com)(memberOf=cn=gitea_admin,ou=groups,dc=mycompagny,dc=com))"
6270f18d conn=3952 op=4 SRCH attr=1.1
6270f18d conn=3952 op=4 SEARCH RESULT tag=101 err=0 nentries=0 text=
6270f18d conn=3952 op=5 BIND anonymous mech=implicit ssf=0
6270f18d conn=3952 op=5 BIND dn="cn=DevTest,ou=users,dc=mycompagny,dc=com" method=128
6270f18d conn=3952 op=5 BIND dn="cn=DevTest,ou=users,dc=mycompagny,dc=com" mech=SIMPLE ssf=0
6270f18d conn=3952 op=5 RESULT tag=97 err=0 text=
6270f18d conn=3952 op=6 SRCH base="ou=groups,dc=mycompagny,dc=com" scope=2 deref=0 filter="(uniqueMember=cn=devtest,ou=users,dc=mycompagny,dc=com)"
6270f18d conn=3952 op=6 SEARCH RESULT tag=101 err=32 nentries=0 text=
6270f18d conn=3952 fd=48 closed (connection lost)

But with my patch the group search is done before the user bind and there is not anymore error 32

6270f3b7 conn=3953 fd=48 ACCEPT from IP=***.***.***.***:53918 (IP=0.0.0.0:389)
6270f3b7 conn=3953 op=0 BIND dn="cn=admin,dc=mycompagny,dc=com" method=128
6270f3b7 conn=3953 op=0 BIND dn="cn=admin,dc=mycompagny,dc=com" mech=SIMPLE ssf=0
6270f3b7 conn=3953 op=0 RESULT tag=97 err=0 text=
6270f3b7 conn=3953 op=1 SRCH base="ou=users,dc=mycompagny,dc=com" scope=2 deref=0 filter="(&(objectClass=inetOrgPerson)(uid=devtest)(memberOf=cn=developpeur,ou=groups,dc=mycompagny,dc=com))"
6270f3b7 conn=3953 op=1 SEARCH RESULT tag=101 err=0 nentries=1 text=
6270f3b7 conn=3953 op=2 SRCH base="cn=DevTest,ou=users,dc=mycompagny,dc=com" scope=2 deref=0 filter="(&(objectClass=inetOrgPerson)(uid=devtest)(memberOf=cn=developpeur,ou=groups,dc=mycompagny,dc=com))"
6270f3b7 conn=3953 op=2 SRCH attr=uid 1.1 1.1 mail dn
6270f3b7 conn=3953 op=2 SEARCH RESULT tag=101 err=0 nentries=1 text=
6270f3b7 conn=3953 op=3 SRCH base="ou=groups,dc=mycompagny,dc=com" scope=2 deref=0 filter="(|(objectClass=groupOfNames)(objectClass=groupOfUniqueNames)(objectClass=posixGroup))"
6270f3b7 conn=3953 op=3 SRCH attr=uniqueMember
6270f3b7 conn=3953 op=3 SEARCH RESULT tag=101 err=0 nentries=15 text=
6270f3b7 conn=3953 op=4 SRCH base="cn=DevTest,ou=users,dc=mycompagny,dc=com" scope=2 deref=0 filter="(|(memberOf=cn=admin,ou=groups,dc=mycompagny,dc=com)(memberOf=cn=gitea_admin,ou=groups,dc=mycompagny,dc=com))"
6270f3b7 conn=3953 op=4 SRCH attr=1.1
6270f3b7 conn=3953 op=4 SEARCH RESULT tag=101 err=0 nentries=0 text=
6270f3b7 conn=3953 op=5 SRCH base="ou=groups,dc=mycompagny,dc=com" scope=2 deref=0 filter="(uniqueMember=cn=devtest,ou=users,dc=mycompagny,dc=com)"
6270f3b7 <= mdb_equality_candidates: (uniqueMember) not indexed
6270f3b7 conn=3953 op=5 SEARCH RESULT tag=101 err=0 nentries=1 text=
6270f3b7 conn=3953 op=6 BIND anonymous mech=implicit ssf=0
6270f3b7 conn=3953 op=6 BIND dn="cn=DevTest,ou=users,dc=mycompagny,dc=com" method=128
6270f3b7 conn=3953 op=6 BIND dn="cn=DevTest,ou=users,dc=mycompagny,dc=com" mech=SIMPLE ssf=0
6270f3b7 conn=3953 op=6 RESULT tag=97 err=0 text=
6270f3b8 conn=3953 fd=48 closed (connection lost)

@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels May 3, 2022
@wxiaoguang
Copy link
Contributor

I see the purpose and how it works. LGTM.

However, that's quite strange in your case the a user can not read their own attributes ..........

@wxiaoguang wxiaoguang added skip-changelog This PR is irrelevant for the (next) changelog, for example bug fixes for unreleased features. topic/authentication labels May 3, 2022
@wxiaoguang wxiaoguang merged commit b7abb31 into go-gitea:main May 3, 2022
zjjhot added a commit to zjjhot/gitea that referenced this pull request May 4, 2022
* giteaofficial/main:
  Fix broken TR on cherrypick page (go-gitea#19599)
  Use correct context in `routers/web` (go-gitea#19597)
  Use for a repo action one database transaction (go-gitea#19576)
  Only set CanColorStdout / CanColorStderr to true if the stdout/stderr is a terminal (go-gitea#19581)
  Don't fetch Mirror when it's migrating (go-gitea#19588)
  Move user password verification after checking his groups on ldap auth (go-gitea#19587)
AbdulrhmnGhanem pushed a commit to kitspace/gitea that referenced this pull request Aug 24, 2022
go-gitea#19587)

In case the binded user can not access its own attributes.

Signed-off-by: Gwilherm Folliot <gwilherm55fo@gmail.com>

Co-authored-by: wxiaoguang <wxiaoguang@gmail.com>
@go-gitea go-gitea locked and limited conversation to collaborators May 3, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. skip-changelog This PR is irrelevant for the (next) changelog, for example bug fixes for unreleased features. topic/authentication
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants