Skip to content

sdap: defer libldap global options setup to first connection#8709

Open
alexey-tikhonov wants to merge 1 commit into
SSSD:masterfrom
alexey-tikhonov:openldap-init
Open

sdap: defer libldap global options setup to first connection#8709
alexey-tikhonov wants to merge 1 commit into
SSSD:masterfrom
alexey-tikhonov:openldap-init

Conversation

@alexey-tikhonov
Copy link
Copy Markdown
Member

During initialization LDAP/AD/IPA backends unconditionally call setup_tls_config() and setup_ldap_debug() that call ldap_set_option(). This triggers ldap_int_initialize() -> getaddrinfo(local_hostname). If DNS is unresponsive, this blocks and the backend doesn't complete initialization in time, so that 'monitor' terminates the entire SSSD.

Move these calls out of the module init path into a new sdap_setup_libldap_global_options() wrapper guarded by a static bool. Call it from sdap_connect_send() just before sss_ldap_init_send(), which is the single entry point for all LDAP connections.

:fixes:Fixed an issue where SSSD fails to start when DNS is unresponsive.

Assisted-By: Claude Code (Opus 4.6)

@alexey-tikhonov alexey-tikhonov added no-backport This should go to target branch only. coverity Trigger a coverity scan labels May 15, 2026
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request refactors the initialization of global LDAP options by consolidating calls to setup_ldap_debug and setup_tls_config into a single function, sdap_setup_libldap_global_options, which is now called once during the connection process. This change affects the AD, IPA, and LDAP providers. Feedback suggests adding a NULL check for the basic_opts parameter in the new function to improve robustness and prevent potential null pointer dereferences.

Comment thread src/providers/ldap/sdap.c
During initialization LDAP/AD/IPA backends unconditionally call
`setup_tls_config()` and `setup_ldap_debug()` that call
`ldap_set_option()`. This triggers `ldap_int_initialize()` ->
`getaddrinfo(local_hostname)`. If DNS is unresponsive, this blocks
and the backend doesn't complete initialization in time, so that
'monitor' terminates the entire SSSD.

Move these calls out of the module init path into a new
`sdap_setup_libldap_global_options()` wrapper guarded by a static bool.
Call it from `sdap_connect_send()` just before `sss_ldap_init_send()`,
which is the single entry point for all LDAP connections.

:fixes:Fixed an issue where SSSD fails to start when DNS is unresponsive.

Assisted-By: Claude Code (Opus 4.6)
@alexey-tikhonov alexey-tikhonov added coverity Trigger a coverity scan and removed coverity Trigger a coverity scan labels May 15, 2026
@alexey-tikhonov alexey-tikhonov marked this pull request as ready for review May 15, 2026 16:08
@sumit-bose
Copy link
Copy Markdown
Contributor

Hi,

thank you for the change. Why did you set no-backport, do you think this is a too large change for older version? Should this patch be added to #8566 as well?

bye,
Sumit

@alexey-tikhonov alexey-tikhonov added backport-to-sssd-2-13 and removed no-backport This should go to target branch only. labels May 18, 2026
@alexey-tikhonov
Copy link
Copy Markdown
Member Author

Why did you set no-backport, do you think this is a too large change for older version?

I missed sssd-2-13 by a mistake, thank you.

Should this patch be added to #8566 as well?

This will happen naturally during https://github.com/SSSD/sssd/tree/failover rebase

Copy link
Copy Markdown

@danlavu danlavu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LG2M

@alexey-tikhonov alexey-tikhonov requested a review from danlavu May 21, 2026 13:07
Copy link
Copy Markdown
Contributor

@sumit-bose sumit-bose left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi,

thank you for the patch, I have no further comments, ACK.

bye,
Sumit

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