Search with filter doesn't return correct results

[WhileLoop]

I have encountered a situation where using the search filter(&(objectClass=inetOrgPerson)(uid=user01)) in go-ldap returns no results but when I search with (objectClass=*) I can see that the object exists.

Using ldapsearch with (&(objectClass=inetOrgPerson)(uid=user01)) returns the expected result.

My LDAP server is Bitnami OpenLDAP docker image. To reproduce:

docker-compose.yml

version: '3.4'
services:
  openldap:
    image: docker.io/bitnami/openldap:2.5
    ports:
      - '389:1389'
    environment:
      - LDAP_ADMIN_USERNAME=admin
      - LDAP_ADMIN_PASSWORD=adminpassword
      - LDAP_USERS=user01
      - LDAP_PASSWORDS=password1
    volumes:
      - 'openldap_data:/bitnami/openldap'
volumes:
  openldap_data:
    driver: local

program.go

package main

import (
	"log"
	"fmt"
	"github.com/go-ldap/ldap/v3"
)

func main() {
	if err := findUser(); err != nil {
		log.Fatal(err)
	}
}

func findUser() error {
	ldapURL := "ldap://0.0.0.0:389"
	adminusername := "cn=admin,dc=example,dc=org"
	adminpassword := "adminpassword"
	baseDN := "dc=example,dc=org"

	l, err := ldap.DialURL(ldapURL)
	if err != nil {
		return err
	}
	l.Start()

	err = l.Bind(adminusername, adminpassword)
	if err != nil {
		return err
	}

	searchAll := &ldap.SearchRequest{
		BaseDN: baseDN,
		Scope:  ldap.ScopeWholeSubtree,
		Filter: "(objectClass=*)",
	}

	fmt.Println("Search: (objectClass=*)")
	sr, err := l.Search(searchAll)
	if err != nil {
		return err
	}

	printResult(sr.Entries)

	fmt.Println("")
	fmt.Println("(&(objectClass=inetOrgPerson)(uid=user01))")
	searchSpecificUser := &ldap.SearchRequest{
		BaseDN: baseDN,
		Scope:  ldap.ScopeWholeSubtree,
		Filter: "(&(objectClass=inetOrgPerson)(uid=user01))",
	}

	sr, err = l.Search(searchSpecificUser)
	if err != nil {
		return err
	}

	if len(sr.Entries) != 1 {
		fmt.Printf("%+v\n", sr)
		log.Fatal("User does not exist or too many entries returned")
	} else {
		fmt.Printf("****** FOUND! ******")
	}
	return nil
}

func printResult(entries []*ldap.Entry) {
	for _, entry := range entries {
		fmt.Println("DN:", entry.DN)
		for _, attr := range entry.Attributes {
			for i := 0; i < len(attr.Values); i++ {
				fmt.Printf("%s: %s\n", attr.Name, attr.Values[i])
			}
		}
		fmt.Println()
	}
}

program output:

$ go run ldap-find-user.go 
Search: (objectClass=*)
DN: ou=users,dc=example,dc=org
objectClass: organizationalUnit
ou: users

DN: dc=example,dc=org
objectClass: dcObject
objectClass: organization
dc: example
o: example

DN: cn=user01,ou=users,dc=example,dc=org
cn: User1
cn: user01
sn: Bar1
objectClass: inetOrgPerson
objectClass: posixAccount
objectClass: shadowAccount
userPassword: password1
uid: user01
uidNumber: 1000
gidNumber: 1000
homeDirectory: /home/user01

DN: cn=readers,ou=users,dc=example,dc=org
cn: readers
objectClass: groupOfNames
member: cn=user01,ou=users,dc=example,dc=org


(&(objectClass=inetOrgPerson)(uid=user01))
&{Entries:[] Referrals:[] Controls:[]}
2022/01/23 19:23:04 User does not exist or too many entries returned
exit status 1

ldapsearch test:

$ ldapsearch -x -b "dc=example,dc=org" -H ldap://0.0.0.0 -D cn=admin,dc=example,dc=org -w adminpassword "(&(objectclass=inetOrgPerson)(uid=user01))"
# extended LDIF
#
# LDAPv3
# base <dc=example,dc=org> with scope subtree
# filter: (&(objectclass=inetOrgPerson)(uid=user01))
# requesting: ALL
#

# user01, users, example.org
dn: cn=user01,ou=users,dc=example,dc=org
cn: User1
cn: user01
sn: Bar1
objectClass: inetOrgPerson
objectClass: posixAccount
objectClass: shadowAccount
userPassword:: cGFzc3dvcmQx
uid: user01
uidNumber: 1000
gidNumber: 1000
homeDirectory: /home/user01

# search result
search: 2
result: 0 Success

# numResponses: 2
# numEntries: 1

Full repo: https://github.com/WhileLoop/go-ldap-test

[vetinari]

I wonder if you have a similar issue as #363 by using Start()

[cpuschma]

I can reproduce this. I created a an OpenLDAP instance in verbose mode and both search requests were submitted successfully + looking at the package dump in Wireshark, the directory server answered with all the result we're looking for.

It really looks like that spawning a new reader and processMessage through conn.Start() seems to trigger weird behaivour and sometimes even race conditions. I ran the same code 1000 times and the results were almost always different..

I'll take a look at this once I'm home

[cpuschma]

The problem seems to come from those two lines. One goroutine unregisters the *messageContext from the map messageContexts, while the other has still responses in it's pipeline. After unregistering, the pending results are discarded in line 510 & 511. It's basically a race condition that sometimes works, if the goroutine with the response messages is faster -- which isn't possible as soon as the directory server returns a bunch of search results for the library to process..

The question is, why is conn.Start() exposed in the first place? 😅 Should we perhaps mark it as deprecated and make it private in future versions? I don't see an "easy" way to fix this, since no goroutine can know/guarantee that no packets or responses are pending. If we can't make it private, one way might be to explicitly assign the messageContext to a goroutine so that only one routine ever handles the message for a messageContext.

@vetinari Any thoughts? ^^

[james-d-elliott]

I think it's reasonable to either deprecate it and remove it within 12 months (regardless of a major bump or not) or to just remove it. Unless there is something documented that it's intended to be used, or there is a use case linked to its inclusion it should be managed by the library.

[gustavoluvizotto]

hi! Looking at the issue, I can see:

        l, err := ldap.DialURL(ldapURL)
	if err != nil {
		return err
	}
	l.Start()

however, the "DialURL" function already calls "Start". So, I think you're starting twice the go routines to read replies and process messages, which could cause race condition.

I would not deprecate Connection.Start because some cases you may want to re-use a previously open TCP or TLS connection with this ldap library by "ldap.NewConn" and then you need the "Connection.Start". I like this flexibility. Maybe worth just add a piece of text to the Start and Dial documentation for the users.

[cpuschma]

I'm going to revert the commit as mentioned in #507 but add a warning to the comment in the meantime. We'll have to either extend our currently available DialOpts or refactor message handling, to bind a messageID to a certain worker to prevent said race conditions