pam: handle protected authentication path

[sumit-bose]

If a Smartcard reader has a built-in keypad or keyboard the flag
CKF_PROTECTED_AUTHENTICATION_PATH is set in the token info data. To
properly tell the user that the pin must be given at the reader directly
and not at the computer this information must be propagated to the
pam_sss module.

Resolves: #5371

[gemini-code-assist]

Code Review

This pull request implements support for PKCS#11 tokens that utilize a protected authentication path, such as an external keypad. The changes span the p11_child process, the PAM responder, and the PAM client, ensuring that a flag indicating the presence of a protected path is propagated through the system. This allows the PAM client to correctly prompt the user to use an external device and bypass standard PIN collection. Additionally, the PR refactors authentication token utility functions to handle keypad-based authentication more generically and updates the test suite to reflect the modified communication protocol. One review comment identifies a logic issue in the responder's response parsing where a prefix of the string "true" could be incorrectly interpreted as a match.

[sssd-bot]

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

Functional Issues

1. Backward compatibility: new pam_sss client with old SSSD server
   src/sss_client/pam_sss.c:1046-1049

   When a new pam_sss.so module parses a cert info response from an older sssd_pam responder that does not include the has_protected_authentication_path field, the offset >= len check at line 1046 will fail with EINVAL, breaking Smartcard authentication entirely. This can happen when the SSSD package is upgraded but the daemon has not yet been restarted.

   The old server sends pam_cert_user as the last field; after advancing offset past it, offset == len, triggering the >= check. Instead of returning EINVAL, this should default has_protected_authentication_path to false and continue:

   offset += strlen(cai->pam_cert_user) + 1;
   if (offset < len) {
       if (strcmp((char *) &buf[*p + offset], "true") == 0) {
           cai->has_protected_authentication_path = true;
       } else {
           cai->has_protected_authentication_path = false;
       }
   } else {
       cai->has_protected_authentication_path = false;
   }

2. sss_authtok_set_sc_from_blob type parameter not validated
   src/util/authtok.c:928-930

   The new type parameter accepts any sss_authtok_type value, but only SSS_AUTHTOK_TYPE_SC_PIN and SSS_AUTHTOK_TYPE_SC_KEYPAD are valid for this function. Currently, callers pass valid values, but the function's contract (documented in the header) says the type "may be SSS_AUTHTOK_TYPE_SC_PIN or SSS_AUTHTOK_TYPE_SC_KEYPAD" — adding a validation check would be consistent with other setter functions in this file that validate their type parameter.

Nits & Non-functional Issues

1. Double space before SSS_AUTHTOK_TYPE_SC_PIN
src/responder/pam/pamsrv_cmd.c:2907

   Extra space: : SSS_AUTHTOK_TYPE_SC_PIN (two spaces after the colon in the ternary).

2. Extra blank line
   src/sss_client/pam_sss.c:1057-1058

   Two consecutive blank lines between the has_protected_authentication_path assignment and the debug log statement. Should be one.

3. New field not included in debug output
   src/sss_client/pam_sss.c:1059-1062

   The debug log after parsing all cert info fields does not include the newly parsed has_protected_authentication_path value. Adding it would aid troubleshooting:

   D(("cert user: [%s] token name: [%s] module: [%s] key id: [%s] "
      "prompt: [%s] pam cert user: [%s] protected auth path: [%s]",
      cai->cert_user, cai->token_name, cai->module_name,
      cai->key_id, cai->prompt_str, cai->pam_cert_user,
      cai->has_protected_authentication_path ? "true" : "false"));

4. No test coverage for the "true" protected authentication path case

   All test check arrays in src/tests/cmocka/test_pam_srv.c (lines 1154, 1165, 1261, 1366, 1383, 1400) use "false" for the has_protected_authentication_path field. There are no tests verifying correct behavior when a token with CKF_PROTECTED_AUTHENTICATION_PATH is present — e.g., that SSS_AUTHTOK_TYPE_SC_KEYPAD is correctly set in the preauth response, and that pack_cert_data emits "true".

5. SC_KEYPAD_FMT prompt string could be clearer
   src/sss_client/pam_sss.c:2129

   The format "Use external keypad for %s" does not explicitly tell the user to enter the PIN on the reader. A message like "Enter PIN on the keypad of %s" or "PIN for %s must be entered at the card reader" would be more instructive for end users. (This is a UX suggestion, not a bug.)

Confirmed Issues from Existing Review Comments

1. strncmp prefix matching issue (gemini-code-assist) — Fixed.
   The bot flagged that strncmp((char *) p, "true", pn - p) would incorrectly match prefixes of "true". The latest revision at src/responder/pam/pamsrv_p11.c:676 now correctly checks (pn - p) == 4 && strncmp((char *) p, "true", 4) == 0, resolving this issue.

[alexey-tikhonov]

Note: Covscan is green.

[alexey-tikhonov]

/gemini review

[gemini-code-assist]

Code Review

This pull request introduces support for the CKF_PROTECTED_AUTHENTICATION_PATH flag, allowing SSSD to handle PKCS#11 tokens that use a protected authentication path (e.g., an external keypad). The changes span the p11_child helper, the PAM responder, and the SSS client to ensure the flag is correctly detected, transmitted, and acted upon by adjusting the PAM conversation style and prompts. Feedback indicates a potential memory leak in the do_pam_conversation function when it is called with the PAM_TEXT_INFO style, which is now utilized when a protected authentication path is detected.

[alexey-tikhonov]

Does

response = GDM_PAM_EXTENSION_REPLY_TO_CUSTOM_JSON_RESPONSE(reply);

allocate a new memory or points to reply internals?

[sumit-bose]

Points to internals.

[alexey-tikhonov]

Isn't it a double-free then?

[sumit-bose]

Hi,

imo not, there is reply as struct pam_response and then the char * resp member which is set to response. Both should be freed individually.

But I realized that the struct returned by GDM_PAM_EXTENSION_REPLY_TO_CUSTOM_JSON_RESPONSE() has a member which might be freed as well. I asked Joan about it and will update the patch when he replies.

bye,
Sumit

[sumit-bose]

Hi,

GDM upstream will add free macros in https://gitlab.gnome.org/GNOME/gdm/-/merge_requests/363 . In the latest version I added an SSSD version of them if they are not already present and use them.

bye,
Sumit

[alexey-tikhonov]

to -> do

[sumit-bose]

fixed

[alexey-tikhonov]

Is it?
There is still "number of bytes to not match"...
Probably a wrong string was edited: "... We just need do notify that the"

[alexey-tikhonov]

Review round completed. Only few minor comments.