pam: apply SIDs from PAC to authentication indicators

[sumit-bose]

This patch reads the PAC of a Kerberos ticket while evaluating the
authentication indicators of the Kerberos ticket during a pam_sss_gss
request. Based on the value of the pam_gssapi_indicators_apply option
the found SIDs might add additional authentication indicators to the
evaluation.

The primary use case is to handle SIDs added by Active Directory's
Authentication Mechanism Assurance (AMA).

[gemini-code-assist]

Code Review

This pull request introduces the pam_gssapi_indicators_apply configuration option, which allows SSSD to assign additional authentication indicators based on information extracted from Kerberos tickets, specifically SIDs from the PAC. The implementation involves updating the configuration schema, documentation, and the PAM responder's GSSAPI logic to handle PAC data parsing and SID-to-indicator mapping. Review feedback identifies a bug where an error code is not properly set upon memory allocation failure and suggests improving the robustness of GSSAPI attribute matching to avoid partial string matches.

[gemini-code-assist]

The check for AUTH_INDICATORS_TAG uses strncmp without checking the length of the attribute name. This could lead to a partial match if another attribute name starts with "auth-indicators". For consistency and correctness, it's better to perform an exact match, similar to how MSPAC_TAG is checked.

        if (attrs->elements[i].length == sizeof(AUTH_INDICATORS_TAG) - 1 &&
            strncmp(AUTH_INDICATORS_TAG, (char *)attrs->elements[i].value,
                    sizeof(AUTH_INDICATORS_TAG) - 1) == 0) {

[pbrezina]

Hi, code looks good, but it deserves a release note.

[sumit-bose]

Hi, code looks good, but it deserves a release note.

Hi,

thanks for the review, added.

bye,
Sumit

[thalman]

Thanks for the work, ACK

[sssd-bot]

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

Functional Issues

1. Missing NULL check after talloc_asprintf_append in handle_pac() (src/responder/pam/pamsrv_gssapi.c:594): In the SID matching loop, talloc_asprintf_append can return NULL on allocation failure, but the return value is not checked. If it fails, exported becomes NULL, and on the next loop iteration talloc_asprintf_append(NULL, ...) has undefined behavior. After the loop, *exported at line 600 would be a NULL pointer dereference. The existing pattern in gssapi_get_indicators() (line 732) correctly checks for NULL after each talloc_asprintf_append — the same pattern should be applied here.

   // Current code (line 594):
   exported = talloc_asprintf_append(exported, "%s ",
                           state->indicators_apply_sid[d] + l + 1);
   // Missing: if (exported == NULL) { ret = ENOMEM; goto done; }

2. Pre-existing bug in gssapi_get_state() — wrong variable in NULL check (src/responder/pam/pamsrv_gssapi.c:846): After state->username = talloc_strdup(state, username), the code checks if (state == NULL) instead of if (state->username == NULL). This means a talloc_strdup failure is never detected, and the code proceeds with a NULL username pointer. While this bug pre-dates this PR, the function was modified in this PR (new pam_ctx parameter and SID list initialization), so it's worth fixing now.

   // Current (line 845-848):
   state->username = talloc_strdup(state, username);
   if (state == NULL) {   // <-- should be: state->username == NULL
       talloc_free(state);
       return NULL;
   }

Nits & Non-functional Issues

1. Typo in comment (src/confdb/confdb.h:451): "addition" should be "additional":

   /* List of addition Kerberos ticket data ... */
   //         ^^^^^^^^^^ should be "additional"

2. Inconsistent attribute name matching (src/responder/pam/pamsrv_gssapi.c:663 vs :667): The AUTH_INDICATORS_TAG check uses a prefix match (strncmp without length check), while the MSPAC_TAG check correctly verifies exact length first. This means any attribute starting with "auth-indicators" would match. In practice this is not a problem since GSSAPI attribute names are well-defined, and this pattern is pre-existing in the original code. For consistency, consider adding a length check for AUTH_INDICATORS_TAG as well.

3. Misleading comment (src/responder/pam/pamsrv_gssapi.c:712): The comment /* exported_from_pac will have a trailing whitespace */ is not always true — when no SIDs match in handle_pac(), exported_from_pac is an empty string "" with no trailing whitespace.

4. Extra blank line (src/responder/pam/pamsrv_gssapi.c:523): There is a double blank line between idmap_free() and handle_pac().

5. Inconsistent indentation in gssapi_get_indicators() (src/responder/pam/pamsrv_gssapi.c:719-728): The is_auth_indicator block uses 8-space indentation for its body, while surrounding code uses 4-space indentation:

   if (is_auth_indicator) {
           DEBUG(SSSDBG_TRACE_FUNC,     // 8-space indent
                   "attribute's...          // 8-space indent

Confirmed Issues from Existing Review Comments

1. Gemini bot — missing ret = ENOMEM on talloc_strdup failure: This was raised for src/responder/pam/pamsrv_gssapi.c:574. Confirmed fixed — ret = ENOMEM is now set at line 578 before goto done.

2. thalman — entering condition even when handle_pac returns error: This was raised for the exported_from_pac handling block. Confirmed fixed — the code now uses else if (line 711) so the append is only reached when handle_pac returns EOK.

3. pbrezina — missing release note: Confirmed fixed — a :relnote: tag is included in the top commit message.

(testing comment update)

[sumit-bose]

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

Functional Issues

1. **Missing NULL check after `talloc_asprintf_append` in `handle_pac()` (`src/responder/pam/pamsrv_gssapi.c:594`)**: In the SID matching loop, `talloc_asprintf_append` can return NULL on allocation failure, but the return value is not checked. If it fails, `exported` becomes NULL, and on the next loop iteration `talloc_asprintf_append(NULL, ...)` has undefined behavior. After the loop, `*exported` at line 600 would be a NULL pointer dereference. The existing pattern in `gssapi_get_indicators()` (line 732) correctly checks for NULL after each `talloc_asprintf_append` — the same pattern should be applied here.
   ```c
   // Current code (line 594):
   exported = talloc_asprintf_append(exported, "%s ",
                           state->indicators_apply_sid[d] + l + 1);
   // Missing: if (exported == NULL) { ret = ENOMEM; goto done; }
   ```

fixed

2. **Pre-existing bug in `gssapi_get_state()` — wrong variable in NULL check (`src/responder/pam/pamsrv_gssapi.c:846`)**: After `state->username = talloc_strdup(state, username)`, the code checks `if (state == NULL)` instead of `if (state->username == NULL)`. This means a `talloc_strdup` failure is never detected, and the code proceeds with a NULL `username` pointer. While this bug pre-dates this PR, the function was modified in this PR (new `pam_ctx` parameter and SID list initialization), so it's worth fixing now.
   ```c
   // Current (line 845-848):
   state->username = talloc_strdup(state, username);
   if (state == NULL) {   // <-- should be: state->username == NULL
       talloc_free(state);
       return NULL;
   }
   ```

fixed

Nits & Non-functional Issues

1. **Typo in comment (`src/confdb/confdb.h:451`)**: "addition" should be "additional":
   ```c
   /* List of addition Kerberos ticket data ... */
   //         ^^^^^^^^^^ should be "additional"
   ```

fixed

2. **Inconsistent attribute name matching (`src/responder/pam/pamsrv_gssapi.c:663` vs `:667`)**: The `AUTH_INDICATORS_TAG` check uses a prefix match (`strncmp` without length check), while the `MSPAC_TAG` check correctly verifies exact length first. This means any attribute starting with `"auth-indicators"` would match. In practice this is not a problem since GSSAPI attribute names are well-defined, and this pattern is pre-existing in the original code. For consistency, consider adding a length check for `AUTH_INDICATORS_TAG` as well.

I guess this is valid but since it is from existing code, I'd prefer if this can be fixed in a separate PR.

3. **Misleading comment (`src/responder/pam/pamsrv_gssapi.c:712`)**: The comment `/* exported_from_pac will have a trailing whitespace */` is not always true — when no SIDs match in `handle_pac()`, `exported_from_pac` is an empty string `""` with no trailing whitespace.

Since a trailing whitespace is only important if exported_from_pac is not empty I ignored this comment.

4. **Extra blank line (`src/responder/pam/pamsrv_gssapi.c:523`)**: There is a double blank line between `idmap_free()` and `handle_pac()`.

fixed

5. **Inconsistent indentation in `gssapi_get_indicators()` (`src/responder/pam/pamsrv_gssapi.c:719-728`)**: The `is_auth_indicator` block uses 8-space indentation for its body, while surrounding code uses 4-space indentation:
   ```c
   if (is_auth_indicator) {
           DEBUG(SSSDBG_TRACE_FUNC,     // 8-space indent
                   "attribute's...          // 8-space indent
   ```

This is exisintg code, so I prefer to keep it as it is.

Confirmed Issues from Existing Review Comments

1. **Gemini bot — missing `ret = ENOMEM` on `talloc_strdup` failure**: This was raised for `src/responder/pam/pamsrv_gssapi.c:574`. **Confirmed fixed** — `ret = ENOMEM` is now set at line 578 before `goto done`.

2. **thalman — entering condition even when `handle_pac` returns error**: This was raised for the `exported_from_pac` handling block. **Confirmed fixed** — the code now uses `else if` (line 711) so the append is only reached when `handle_pac` returns `EOK`.

3. **pbrezina — missing release note**: **Confirmed fixed** — a `:relnote:` tag is included in the top commit message.

(testing comment update)

[alexey-tikhonov]

test_kcm__tgt_renewal_updates_ticket_as_configured is a known flaky test.

[sssd-bot]

The pull request was accepted by @alexey-tikhonov with the following PR CI status:

---

🟢 CodeQL (success)
🟢 osh-diff-scan:fedora-rawhide-x86_64:upstream (success)
🟢 rpm-build:centos-stream-10-x86_64:upstream (success)
🟢 rpm-build:fedora-42-x86_64:upstream (success)
🟢 rpm-build:fedora-43-x86_64:upstream (success)
🟢 rpm-build:fedora-44-x86_64:upstream (success)
🟢 rpm-build:fedora-rawhide-x86_64:upstream (success)
🟢 Analyze (target) / cppcheck (success)
🟢 Build / freebsd (success)
🟢 Build / make-distcheck (success)
🟢 ci / intgcheck (centos-10) (success)
🟢 ci / intgcheck (fedora-42) (success)
🟢 ci / intgcheck (fedora-43) (success)
🟢 ci / intgcheck (fedora-44) (success)
🟢 ci / intgcheck (fedora-45) (success)
🟢 ci / prepare (success)
🟢 ci / system (centos-10) (success)
🟢 ci / system (fedora-42) (success)
🟢 ci / system (fedora-43) (success)
🟢 ci / system (fedora-44) (success)
🔴 ci / system (fedora-45) (failure)
➖ Coverity scan / coverity (skipped)
🟢 Static code analysis / codeql (success)
🟢 Static code analysis / pre-commit (success)
🟢 Static code analysis / python-system-tests (success)

---

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