Expose ResultCode and DiagnosticMessage on SearchResult by Pujathacker2210 · Pull Request #626 · go-ldap/ldap

Pujathacker2210 · 2026-09-07T11:28:59Z

Problem

GetLDAPError returns nil when resultCode is 0 (success), discarding
the diagnosticMessage from SearchResultDone. Per RFC 4511 §4.1.9, servers
may set diagnosticMessage even on a successful operation to communicate
additional information such as degraded state.

For example, Red Hat IPA sets diagnosticMessage when the directory is
reinitializing but still returns resultCode 0 with an empty entry list.
Callers of Search() currently have no way to see that message — they only
see zero entries and no error, which is indistinguishable from a genuine
empty result.

Solution

Add ResultCode and DiagnosticMessage fields to SearchResult.
In Search(), populate them from the SearchResultDone packet (case 5) before calling GetLDAPError, so they are preserved even whenGetLDAPError returns nil on success.
Add parseLDAPResult helper that extracts resultCode, matchedDN, and diagnosticMessage from an LDAPResult BER packet without the early return on success that GetLDAPError has.
Copy the new fields in appendTo so paged searches preserve them.

Backward compatibility

GetLDAPError is unchanged.
SearchResult is a struct; adding fields is backward compatible in Go.
Existing callers are unaffected — they never read the new fields.
Callers who need the diagnostic on success can now read result.DiagnosticMessage after Search() returns.

References

RFC 4511 §4.1.9 — LDAPResult definition (resultCode + diagnosticMessage)
RFC 4511 §4.5.2 — SearchResultDone is an LDAPResult

Prior art

Python's python-ldap library already exposes the diagnostic message after successful operations by calling get_option(OPT_DIAGNOSTIC_MESSAGE) on the LDAP handle (source). This PR brings the same capability to go-ldap by surfacing diagnosticMessage on the SearchResult struct regardless of resultCode.

Pujathacker2210 · 2026-09-22T10:29:04Z

@t2y @cpuschma @johnweldon - Request you to review the PR

t2y · 2026-09-23T05:39:26Z

This is my personal opinion based on the changes. Before we proceed with this PR, I'd like others' opinions on two API changes.

Add DiagnosticMessage() string to the Response interface

SearchAsync drops the diagnosticMessage on success just like Search. To support both, I'd add this method and have it work like Err(), i.e. valid after Next returns false. This breaks external implementations of Response (e.g. test fakes), but fixing them only takes one extra method.

Add a DiagnosticMessage field to ldap.Error

The goal is to parse LDAPResult in one place. GetLDAPError and the success path in Search/SearchAsync would share one internal parser, and GetLDAPError would copy the diagnosticMessage into the new field. Callers could then read it as a plain field on both success (SearchResult.DiagnosticMessage) and failure (Error.DiagnosticMessage), instead of pulling it out of Err, which also holds client-side errors.

cpuschma

Please see the comments.

I would prefer not to make this function publicly available yet, until the aforementioned code snippets have been resolved.

For now, please change the function to non-public, so that only the new fields in SearchResult are propagated.

Pujathacker2210 · 2026-09-23T17:11:29Z

@cpuschma Thanks for the review! Pushed the changes:

ParseLDAPResult → parseLDAPResult (unexported)
Simplified to int64 only per your note on TagInteger
All malformed-packet paths now return an error instead of silent zero values

Please take another look when you get a chance. Also, @t2y raised two additional suggestions — adding DiagnosticMessage() to the Response interface and a DiagnosticMessage field to ldap.Error. Both would be breaking changes for external consumers. @cpuschma @johnweldon could you share your thoughts on those? Happy to incorporate them into this PR if there's consensus, or we can handle them as a follow-up.

cpuschma · 2026-09-23T17:16:16Z

 			result.Entries = append(result.Entries, entry)
 		case 5:
+			rc, _, diag, parseErr := parseLDAPResult(packet)
+			if parseErr == nil {


parseErr is not returned, the error is silently dropped here.

cpuschma · 2026-09-23T17:19:16Z

@cpuschma Thanks for the review! Pushed the changes:

ParseLDAPResult → parseLDAPResult (unexported) Simplified to int64 only per your note on TagInteger All malformed-packet paths now return an error instead of silent zero values

Please take another look when you get a chance. Also, @t2y raised two additional suggestions — adding DiagnosticMessage() to the Response interface and a DiagnosticMessage field to ldap.Error. Both would be breaking changes for external consumers. @cpuschma @johnweldon could you share your thoughts on those? Happy to incorporate them into this PR if there's consensus, or we can handle them as a follow-up.

Let's not introduce breaking changes in a minor release. Currently we have a abundance of PRs regarding OOB reads for malformed packets we need to address first before new functions or even breaking changes can be introduced (#632)

johnweldon

Thank you for your contribution!

t2y · 2026-09-24T04:22:55Z

I agree with @cpuschma's direction. Let's not include breaking changes in a minor release. We can revisit changing the interface later if needed.

GetLDAPError returns nil when resultCode is 0, discarding the diagnosticMessage. Some servers set this field on success to signal degraded state (e.g. directory reinitializing). Add ResultCode and DiagnosticMessage to SearchResult, populated before GetLDAPError. Add ParseLDAPResult to read all LDAPResult fields without the early return on success. GetLDAPError is unchanged. Existing callers are unaffected.

Pujathacker2210 · 2026-09-25T04:04:27Z

@cpuschma @johnweldon Pushed the fixes and replied to each comment. Let me know if anything else needs to be addressed.

Pujathacker2210 · 2026-09-25T14:53:56Z

Thank you @cpuschma for merging the PR. Please confirm when will this be released.

Pujathacker2210 · 2026-09-28T17:25:50Z

@t2y @cpuschma @johnweldon Please suggest when will this code be released

cpuschma · 2026-09-28T20:54:36Z

Some issues are still to be adressed before a new release is published. If you're eager to use the newest patches, you can use the current commit instead of the tagged release:

go get github.com/go-ldap/ldap/v3@05f305f2813dc3d18a886d8f927d9ac9ca18a986

Pujathacker2210 mentioned this pull request Sep 8, 2026

Fix: detect untrustworthy LDAP search results during IPA reinit redhat-data-and-ai/usernaut#356

Open

cpuschma requested changes Sep 23, 2026

View reviewed changes

Comment thread v3/error.go Outdated

Comment thread v3/error.go Outdated

Comment thread v3/error.go Outdated

Comment thread v3/error.go Outdated

cpuschma self-assigned this Sep 23, 2026

cpuschma added enhancement go Pull requests that update go code labels Sep 23, 2026

Pujathacker2210 force-pushed the search-result-diagnostic branch from 68eebfc to 8ab56ea Compare September 23, 2026 17:00

Pujathacker2210 requested a review from cpuschma September 23, 2026 17:12

cpuschma requested changes Sep 23, 2026

View reviewed changes

johnweldon requested changes Sep 23, 2026

View reviewed changes

Comment thread v3/search.go

Comment thread v3/error.go Outdated

Comment thread v3/error.go Outdated

Comment thread v3/error.go

Comment thread v3/search.go

Pujathacker2210 force-pushed the search-result-diagnostic branch from 8ab56ea to b540ec6 Compare September 24, 2026 06:59

cpuschma approved these changes Sep 25, 2026

View reviewed changes

cpuschma merged commit ca8bcfa into go-ldap:master Sep 25, 2026
4 checks passed

Conversation

Pujathacker2210 commented Sep 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Problem

Solution

Backward compatibility

References

Prior art

Uh oh!

Pujathacker2210 commented Sep 22, 2026

Uh oh!

t2y commented Sep 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cpuschma left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Pujathacker2210 commented Sep 23, 2026

Uh oh!

cpuschma Sep 23, 2026

Choose a reason for hiding this comment

Uh oh!

Pujathacker2210 Sep 24, 2026

Choose a reason for hiding this comment

Uh oh!

cpuschma commented Sep 23, 2026

Uh oh!

johnweldon left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

t2y commented Sep 24, 2026

Uh oh!

Pujathacker2210 commented Sep 25, 2026

Uh oh!

Uh oh!

Pujathacker2210 commented Sep 25, 2026

Uh oh!

Pujathacker2210 commented Sep 28, 2026

Uh oh!

cpuschma commented Sep 28, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Pujathacker2210 commented Sep 7, 2026 •

edited

Loading

t2y commented Sep 23, 2026 •

edited

Loading