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

Fix ControlServerSideSortingResult handling for OpenLDAP #546

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

EgorKo25
Copy link

Description

This pull request fixes an issue in the ControlServerSideSortingResult function where an empty pkt.Children caused an error in OpenLDAP. According to OpenLDAP behavior, the presence of Children is not always required, so returning an empty ControlServerSideSortingResult instead of an error ensures better compatibility.

Changes

  1. Replaced struct instantiation with new(...) for consistency.
  2. Modified NewControlServerSideSortingResult to return an empty ControlServerSideSortingResult instead of an error when pkt.Children is missing.
  3. Added an inline comment explaining the fix.

Testing

  1. Manually tested with OpenLDAP to confirm that it no longer fails on missing Children.
  2. Existing tests pass successfully.

Additional Notes

  1. This fix improves OpenLDAP compatibility without affecting other LDAP implementations.
  2. No breaking changes introduced.

@cpuschma
Copy link
Member

cpuschma commented Feb 3, 2025

Hi @EgorKo25 ,

thank you for your PR. RFC 2891 Section 1.2 (https://datatracker.ietf.org/doc/html/rfc2891#section-1.2) only names the second attribute as optional, but not the first. Therefore it should never happen that there are no children at controlValue:

SortResult ::= SEQUENCE {
         sortResult  ENUMERATED {
             success                   (0), -- results are sorted
             operationsError           (1), -- server internal failure
             timeLimitExceeded         (3), -- timelimit reached before
                                            -- sorting was completed
             strongAuthRequired        (8), -- refused to return sorted
                                            -- results via insecure
                                            -- protocol
             adminLimitExceeded       (11), -- too many matching entries
                                            -- for the server to sort
             noSuchAttribute          (16), -- unrecognized attribute
                                            -- type in sort key
             inappropriateMatching    (18), -- unrecognized or
                                            -- inappropriate matching
                                            -- rule in sort key
             insufficientAccessRights (50), -- refused to return sorted
                                            -- results to this client
             busy                     (51), -- too busy to process
             unwillingToPerform       (53), -- unable to sort
             other                    (80)
             },
       attributeType [0] AttributeDescription OPTIONAL }

Maybe I'm missing something here, but usually we don't derivate from the RFCs.

@andrewlord607
Copy link

Hello,
I believe this is a known bug — #506.

I tried to reproduce the issue myself. Here is the test program:

package main

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

func main() {
	l, err := ldap.DialURL("ldap://localhost:389")
	if err != nil {
		log.Fatal(err)
	}
	defer l.Close()

	pageControl := ldap.NewControlPaging(1)
	serverSideSortingControl := ldap.NewControlServerSideSortingWithSortKeys(
		[]*ldap.SortKey{
			{
				Reverse:       false,
				AttributeType: "cn",
				MatchingRule:  "caseIgnoreOrderingMatch",
			},
		})

	searchRequest := ldap.NewSearchRequest(
		"dc=globus,dc=ru", // The base dn to search
		ldap.ScopeWholeSubtree, ldap.NeverDerefAliases, 0, 0, false,
		"(&(objectClass=organizationalPerson))", // The filter to apply
		[]string{"dn", "cn"},                    // A list attributes to retrieve
		[]ldap.Control{pageControl, serverSideSortingControl},
	)

	sr, err := l.Search(searchRequest)
	if err != nil {
		log.Fatal(err)
	}

	for _, entry := range sr.Entries {
		fmt.Printf("%s: %v\n", entry.DN, entry.GetAttributeValue("cn"))
	}
}

With OpenLDAP (I tried versions 2.5.18 and 2.6.7), the program always stops with the error "failed to decode child control: bad packet" after the Search call.

In the debug output Children is indeed empty.

image

I also created a dump using tcpdump to check what LDAP returns in response, and Wireshark decodes the sortingResult as "success".
sss_ldap.zip

I have an idea that during decoding, we should use Value instead of Children when evaluating ControlServerSideSortingCode, but I'm not sure how this aligns with the RFC.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants