pam: gate PAC indicator code on BUILD_SAMBA by padelsbach · Pull Request #8620 · SSSD/sssd

padelsbach · 2026-04-20T23:51:05Z

Commit 1f680ed added ad_pac_common.c and $(NDR_KRB5PAC_LIBS) to sssd_pam unconditionally. So when building --without-samba, sssd_pam fails to link with undefined references to ndr_pull_init_blob and ndr_pull_PAC_DATA.

This change qualifies those additions with BUILD_SAMBA so the PAC indicator feature is compiled in only when samba support is enabled.

gemini-code-assist

Code Review

This pull request makes the Samba-related PAC handling and idmapping dependencies optional for the PAM responder and its associated tests. It introduces conditional build logic in Makefile.am and preprocessor guards in src/responder/pam/pamsrv_gssapi.c using the BUILD_SAMBA flag. I have no feedback to provide.

sssd-bot

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

Functional Issues

No functional issues found. The BUILD_SAMBA guards in pamsrv_gssapi.c correctly wrap all code that depends on Samba-provided symbols (ad_pac.h, handle_pac, idmap_talloc/idmap_free, exported_from_pac variable), and the Makefile.am changes correctly gate ad_pac_common.c, $(NDR_KRB5PAC_CFLAGS), and $(NDR_KRB5PAC_LIBS) behind BUILD_SAMBA for all three targets (sssd_pam, pam_srv_tests, test_pamsrv_json). The libsss_idmap.la linkage was only gated for sssd_pam (where it was added by commit 1f680ed), while correctly left unconditional for the test targets (where it pre-existed).

Nits & Non-functional Issues

Unnecessary MSPAC attribute processing without Samba (src/responder/pam/pamsrv_gssapi.c:679-693): When BUILD_SAMBA is not defined, the MSPAC tag detection code (lines 679–683) still identifies MSPAC attributes and enters the while (more != 0) loop (line 697), calling gss_get_name_attribute() to extract PAC data that is then silently discarded (the PAC handling block on line 714 is gated, and is_auth_indicator is false). This is functionally harmless but wastes GSSAPI calls. Consider gating the MSPAC detection itself:
```
#ifndef BUILD_SAMBA
        } else {
#else
        } else if (attrs->elements[i].length == (sizeof(MSPAC_TAG) - 1)
                        && strncmp(MSPAC_TAG, attrs->elements[i].value,
                                   sizeof(MSPAC_TAG) - 1) == 0) {
            is_mspac = true;
            is_auth_indicator = false;
        } else {
#endif
```
Or more simply, add a continue after the MSPAC detection when BUILD_SAMBA is not defined.
Indentation inconsistency in Makefile.am: Within the same sssd_pam target, SOURCES additions inside if BUILD_SAMBA blocks are indented with 4 spaces (e.g., Makefile.am:1530), but CFLAGS and LDADD additions are not indented (e.g., Makefile.am:1537, Makefile.am:1551). The same inconsistency appears in the pam_srv_tests and test_pamsrv_json targets. The BUILD_PASSKEY conditional just above (line 1527) uses indented content, so the indented style appears to be the local convention:
```
# Line 1536-1538: not indented
if BUILD_SAMBA
sssd_pam_CFLAGS += $(NDR_KRB5PAC_CFLAGS)
endif

# vs Line 1529-1531: indented (matches BUILD_PASSKEY above)
if BUILD_SAMBA
    sssd_pam_SOURCES += src/providers/ad/ad_pac_common.c
endif
```
Unconditional gssapi_get_apply_sid_list call (src/responder/pam/pamsrv_gssapi.c:870): The function at line 787 and its call at line 870 are not gated behind BUILD_SAMBA. Without Samba, state->indicators_apply_sid is populated but never consumed (the only consumer at line 716 is inside the BUILD_SAMBA guard). This wastes a small amount of memory and config parsing. Low priority since it compiles and runs correctly in all configurations.

Confirmed Issues from Existing Review Comments

No inline review comments were submitted on this PR.

sumit-bose · 2026-04-21T10:39:50Z

Hi,

thank you for the fixes, I have to admit that I didn't thought of the non-Samba use-case when writing the offending patches.

I agree with our changes but I wonder if you can add the following changes as well:

diff --git a/src/man/Makefile.am b/src/man/Makefile.am
index 5a15a014a..deb50317f 100644
--- a/src/man/Makefile.am
+++ b/src/man/Makefile.am
@@ -64,9 +64,12 @@ endif
 if HAVE_GDM_CUSTOM_JSON_PAM_EXTENSION
 JSON_PAM_CONDS = ;build_json_pam
 endif
+if BUILD_SAMBA
+SAMBA_CONDS = ;with_samba
+endif
 
 
diff --git a/src/man/Makefile.am b/src/man/Makefile.am
index 5a15a014a..deb50317f 100644
--- a/src/man/Makefile.am
+++ b/src/man/Makefile.am
@@ -64,9 +64,12 @@ endif
 if HAVE_GDM_CUSTOM_JSON_PAM_EXTENSION
 JSON_PAM_CONDS = ;build_json_pam
 endif
+if BUILD_SAMBA
+SAMBA_CONDS = ;with_samba
+endif
 
 
-CONDS = with_false$(SUDO_CONDS)$(AUTOFS_CONDS)$(SSH_CONDS)$(PAC_RESPONDER_CONDS)$(GPO_CONDS)$(SYSTEMD_CONDS)$(KCM_CONDS)$(STAP_CONDS)$(KCM_RENEWAL_CONDS)$(LOCKFREE_CLIENT_CONDS)$(HAVE_INOTIFY_CONDS)$(SUBID_CONDS)$(PASSKEY_CONDS)$(SSSD_NON_ROOT_USER_CONDS)$(LIBNL_CONDS)$(IDP_CONDS)$(JSON_PAM_CONDS)
+CONDS = with_false$(SUDO_CONDS)$(AUTOFS_CONDS)$(SSH_CONDS)$(PAC_RESPONDER_CONDS)$(GPO_CONDS)$(SYSTEMD_CONDS)$(KCM_CONDS)$(STAP_CONDS)$(KCM_RENEWAL_CONDS)$(LOCKFREE_CLIENT_CONDS)$(HAVE_INOTIFY_CONDS)$(SUBID_CONDS)$(PASSKEY_CONDS)$(SSSD_NON_ROOT_USER_CONDS)$(LIBNL_CONDS)$(IDP_CONDS)$(JSON_PAM_CONDS)$(SAMBA_CONDS)
 
 
 #Special Rules:
diff --git a/src/man/sssd.conf.5.xml b/src/man/sssd.conf.5.xml
index eee31997d..952389e41 100644
--- a/src/man/sssd.conf.5.xml
+++ b/src/man/sssd.conf.5.xml
@@ -1981,7 +1981,7 @@ pam_gssapi_indicators_map = sudo:pkinit, sudo-i:pkinit
                         </para>
                     </listitem>
                 </varlistentry>
-                <varlistentry>
+                <varlistentry condition="with_samba">
                     <term>pam_gssapi_indicators_apply</term>
                     <listitem>
                         <para>

which removes the related option from the man page and

diff --git a/src/responder/pam/pamsrv.c b/src/responder/pam/pamsrv.c
index be95d7251..e43d54c51 100644
--- a/src/responder/pam/pamsrv.c
+++ b/src/responder/pam/pamsrv.c
@@ -415,6 +415,7 @@ static int pam_process_init(TALLOC_CTX *mem_ctx,
                                  CONFDB_PAM_GSSAPI_INDICATORS_APPLY);
 
     if (tmpstr != NULL) {
+#ifdef BUILD_SAMBA
         ret = split_on_separator(pctx, tmpstr, ',', true, true,
                                  &pctx->gssapi_indicators_apply, NULL);
         if (ret != EOK) {
@@ -423,6 +424,13 @@ static int pam_process_init(TALLOC_CTX *mem_ctx,
                   sss_strerror(ret));
             goto done;
         }
+#else
+        DEBUG(SSSDBG_CRIT_FAILURE,
+              "This build does not support the [%s] option.\n",
+              CONFDB_PAM_GSSAPI_INDICATORS_APPLY);
+        ret = ENOTSUP;
+        goto done;
+#endif
     }
 
     /* Check if JSON authentication selection method is enabled for any PAM

which will error out during startup if the option is used.

I think this will make the build without Samba more consistent.

bye,
Sumit

padelsbach · 2026-04-21T16:33:09Z

@sumit-bose, I've applied the diffs you requested.

sumit-bose

Hi,

thank you for including the additional patches, I have no further comments, ACK.

bye,
Sumit

thalman

LGTM

Commit 1f680ed added ad_pac_common.c and $(NDR_KRB5PAC_LIBS) to sssd_pam unconditionally. So when building --without-samba, sssd_pam fails to link with undefined references to ndr_pull_init_blob and ndr_pull_PAC_DATA. This change qualifies those additions with `BUILD_SAMBA` so the PAC indicator feature is compiled in only when samba support is enabled. Reviewed-by: Sumit Bose <sbose@redhat.com> Reviewed-by: Tomáš Halman <thalman@redhat.com>

sssd-bot · 2026-04-22T08:32:28Z

The pull request was accepted by @thalman with the following PR CI status:

There are unsuccessful or unfinished checks. Make sure that the failures are not related to this pull request before merging.

gemini-code-assist Bot reviewed Apr 20, 2026

View reviewed changes

padelsbach marked this pull request as ready for review April 21, 2026 02:03

alexey-tikhonov added the no-backport This should go to target branch only. label Apr 21, 2026

alexey-tikhonov requested a review from sumit-bose April 21, 2026 06:28

alexey-tikhonov assigned alexey-tikhonov and unassigned alexey-tikhonov Apr 21, 2026

alexey-tikhonov requested review from pbrezina and thalman April 21, 2026 06:30

sssd-bot reviewed Apr 21, 2026

View reviewed changes

alexey-tikhonov added the Changes requested label Apr 21, 2026

alexey-tikhonov assigned sumit-bose Apr 21, 2026

padelsbach force-pushed the fix-without-samba-build branch from f7a8c13 to 3f3d92f Compare April 21, 2026 16:32

alexey-tikhonov added Waiting for review and removed Changes requested labels Apr 21, 2026

alexey-tikhonov mentioned this pull request Apr 22, 2026

[autobackport: sssd-2-9] pam: apply SIDs from PAC to authentication indicators #8618

Merged

alexey-tikhonov added backport-to-sssd-2-9 and removed no-backport This should go to target branch only. labels Apr 22, 2026

sumit-bose approved these changes Apr 22, 2026

View reviewed changes

thalman approved these changes Apr 22, 2026

View reviewed changes

alexey-tikhonov removed the request for review from pbrezina April 22, 2026 08:31

alexey-tikhonov assigned thalman Apr 22, 2026

alexey-tikhonov added the coverity Trigger a coverity scan label Apr 22, 2026

thalman added Accepted and removed Waiting for review labels Apr 22, 2026

sssd-bot force-pushed the fix-without-samba-build branch from 3f3d92f to 4cdea3a Compare April 22, 2026 08:32

alexey-tikhonov removed the coverity Trigger a coverity scan label Apr 22, 2026

alexey-tikhonov added coverity Trigger a coverity scan and removed coverity Trigger a coverity scan labels Apr 22, 2026

alexey-tikhonov merged commit d0becea into SSSD:master Apr 22, 2026
14 of 16 checks passed

sssd-bot mentioned this pull request Apr 22, 2026

[autobackport: sssd-2-9] pam: gate PAC indicator code on BUILD_SAMBA #8628

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

pam: gate PAC indicator code on BUILD_SAMBA#8620

pam: gate PAC indicator code on BUILD_SAMBA#8620
alexey-tikhonov merged 1 commit intoSSSD:masterfrom
padelsbach:fix-without-samba-build

padelsbach commented Apr 20, 2026

Uh oh!

gemini-code-assist Bot left a comment

Uh oh!

sssd-bot left a comment

Uh oh!

sumit-bose commented Apr 21, 2026

Uh oh!

padelsbach commented Apr 21, 2026

Uh oh!

sumit-bose left a comment

Uh oh!

thalman left a comment

Uh oh!

sssd-bot commented Apr 22, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Conversation

padelsbach commented Apr 20, 2026

Uh oh!

gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

sssd-bot left a comment

Choose a reason for hiding this comment

Functional Issues

Nits & Non-functional Issues

Confirmed Issues from Existing Review Comments

Uh oh!

sumit-bose commented Apr 21, 2026

Uh oh!

padelsbach commented Apr 21, 2026

Uh oh!

sumit-bose left a comment

Choose a reason for hiding this comment

Uh oh!

thalman left a comment

Choose a reason for hiding this comment

Uh oh!

sssd-bot commented Apr 22, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants