sdap: handle missing rootDSE gracefully

[pbrezina]

If ldap_read_rootdse = never then srv_opts which is unexpected. It can
also happen on other path in the connection code, because
sdap_cli_use_rootdse() is called only when the rootDSE is successfully
fetch. This patch makes sure that srv_opts are always set.

:fixes: SSSD no longer crashes if ldap_read_rootdse = never and
enumarete = true

[gemini-code-assist]

Code Review

This pull request ensures that srv_opts is always initialized in sdap_cli_connect_done to prevent potential NULL pointer issues when rootDSE is unavailable or access is disabled. It also adds a talloc_zfree call in sdap_cli_use_rootdse to prevent memory leaks when re-initializing these options. I have no feedback to provide.

[alexey-tikhonov]

If `ldap_read_rootdse = never` then srv_opts which is unexpected.

-- feels like some words are missing...

[sssd-bot]

Review done using Claude Code / claude-opus-4-6

Functional Issues

1. USN map names may be prematurely set on the first (NULL rootdse) call, preventing proper autodetection on the second call.

   sdap_get_server_opts_from_rootdse has side effects on the shared opts structure: when called with a NULL rootdse and no configured USN attributes (the generic LDAP case where gen_map[SDAP_AT_ENTRY_USN].name and gen_map[SDAP_AT_LAST_USN].name default to NULL), it sets gen_map[SDAP_AT_ENTRY_USN].name to "modifyTimestamp" and propagates that fallback into user_map, group_map, service_map, sudorule_map, iphost_map, and ipnetwork_map (see sdap.c:1519-1562).

   When sdap_cli_use_rootdse later calls the function again with real rootdse data, the autodetect loop (sdap.c:1449-1473) may discover IPA or AD USN attributes and update gen_map, but the per-object-type maps (user_map, group_map, etc.) are skipped because they are already non-NULL from the first call. This leaves the per-object maps using "modifyTimestamp" while gen_map uses the proper USN attribute.

   In practice this only affects generic LDAP providers (not IPA or AD, which pre-set USN defaults in their attribute maps), so the impact is limited. But it is a behavioral regression compared to pre-patch code where the function was called exactly once with the actual rootdse data.

   Consider either (a) clearing the per-object-type USN names in sdap_cli_use_rootdse before the second call, or (b) moving the per-object-type map population out of sdap_get_server_opts_from_rootdse and into sdap_cli_use_rootdse so it only runs once with final data.

2. Commit message body has a broken sentence. "If ldap_read_rootdse = never then srv_opts which is unexpected" is missing a verb — likely should be "then srv_opts is NULL, which is unexpected". Also "the rootDSE is successfully fetch" should be "fetched". While not a code bug, this will appear in git log permanently and makes the rationale hard to follow.

Nits & Non-functional Issues

1. Unmatched parenthesis in comment (src/providers/ldap/sdap_async_connection.c:1658-1659): The comment opens (this can happen if but never closes the parenthesis before */.

2. Typo in :fixes: tag: "enumarete" should be "enumerate".

3. No test coverage. The crash described in the :fixes: tag (ldap_read_rootdse = never with enumerate = true) is a concrete scenario that could be covered by a unit or integration test configuring those options and verifying the connection completes without crashing.

Review of Existing Review Comments

The only review is from gemini-code-assist[bot], which summarized the change and stated "I have no feedback to provide." No actionable items were raised.

[alexey-tikhonov]

1. sdap_get_server_opts_from_rootdse has side effects on the shared opts structure

This seems valid.

[thalman]

Code looks good, please fix the commit message, "If ldap_read_rootdse = never then srv_opts which is unexpected." does not make sense.

[thalman]

CI failures are not connected with the change.

[pbrezina]

1. sdap_get_server_opts_from_rootdse has side effects on the shared opts structure

This seems valid.

It wasn't, because there was talloc_zfree(state->srv_opts); in sdap_cli_use_rootdse() and in we create new srv_opts in sdap_get_server_opts_from_rootdse().

But I refactored it and only create srv_opts with NULL rootdse as a falback in _recv.

[pbrezina]

Ready for new review.

[alexey-tikhonov]

1. sdap_get_server_opts_from_rootdse has side effects on the shared opts structure

This seems valid.

It wasn't, because there was talloc_zfree(state->srv_opts); in sdap_cli_use_rootdse() and in we create new srv_opts in sdap_get_server_opts_from_rootdse().

But I refactored it and only create srv_opts with NULL rootdse as a falback in _recv.

What if server didn't return rootDSE upon first connection (and map names are populated with defaults), but - out of a sudden - returns a rootDSE after re-connection?

I understand this is quite artificial scenario, but I guess this is what was meant.

[pbrezina]

1. sdap_get_server_opts_from_rootdse has side effects on the shared opts structure

This seems valid.

It wasn't, because there was talloc_zfree(state->srv_opts); in sdap_cli_use_rootdse() and in we create new srv_opts in sdap_get_server_opts_from_rootdse().
But I refactored it and only create srv_opts with NULL rootdse as a falback in _recv.

What if server didn't return rootDSE upon first connection (and map names are populated with defaults), but - out of a sudden - returns a rootDSE after re-connection?

I understand this is quite artificial scenario, but I guess this is what was meant.

New srv_opts is returned and used.

[sssd-bot]

@pbrezina You're right that srv_opts itself is freshly allocated each time — no issue there.

The concern is narrower: sdap_get_server_opts_from_rootdse also writes to the shared opts->*_map structures as a side-effect, and those writes are guarded by if (!opts->*_map[...].name) (one-shot). Concretely, in sdap.c around line 1530:

if (!opts->user_map[SDAP_AT_USER_USN].name) {
    opts->user_map[SDAP_AT_USER_USN].name =
                talloc_strdup(opts->user_map,
                              opts->gen_map[SDAP_AT_ENTRY_USN].name);
}
/* same for group_map, service_map, sudorule_map, ... */

In the edge case @alexey-tikhonov described (rootDSE unavailable on first connect, available on reconnect):

1. First call with rootdse=NULL: opts->gen_map[SDAP_AT_ENTRY_USN] → "modifyTimestamp", per-object maps → "modifyTimestamp"
2. Second call with real rootdse: autodetect updates opts->gen_map[SDAP_AT_ENTRY_USN] to the native attribute (e.g. entryUSN), but the per-object maps are already set so the if (!...) guards skip them

The result is srv_opts->supports_usn = true but opts->user_map[SDAP_AT_USER_USN].name still says "modifyTimestamp" while opts->gen_map says the native USN attribute — a mismatch.

That said, this is a pre-existing design limitation of sdap_get_server_opts_from_rootdse's guard-once pattern, not something this PR introduces. And the triggering scenario (transient rootDSE failure followed by recovery on the same SSSD process) is quite unlikely. I don't think it should block this PR — just wanted to clarify what the concern was pointing at since "New srv_opts is returned and used" addresses the struct but not the map side-effects.