Correct pam_sss behavior with inherited 2fa_single authtok #7916

mstone232 · 2025-04-09T17:46:07Z

After the change in "krb5_child: do not try passwords with OTP #7462" pam_sss broke PAM configurations like this:
auth sufficient pam_unix.so nullok
auth sufficient pam_sss.so forward_pass
What happens in this case is that the combined password+otp is entered at the prompt generated by pam_unix, then forwarded to pam_sss. But in pam_sss any forwarded authtok is unconditionally set to SSS_AUTHTOK_TYPE_PASSWORD. This causes tokeninfo_matches to bail with the message "Unsupported authtok type 1" in krb5_child.log because tokeninfo_matches no longer handles SSS_AUTHTOK_TYPE_PASSWORD.

This patch adds the logic used elsewhere in pam_sss to determine whether a pam_authtok string should be treated as SSS_AUTHTOK_TYPE_PASSWORD or SSS_AUTHTOK_TYPE_2FA_SINGLE. Given the duplication it may be useful to refactor that logic into a separate function, but I tried to keep the change minimal.

alexey-tikhonov · 2025-04-10T10:35:41Z

Hi.

Is this an update for a #7456 fix?

mstone232 · 2025-04-10T12:28:25Z

I'm not quite sure what you're asking. #7462 is meant to address #7456, but in relaxing enforcement of 2fa when that is meant to be optional (honestly, a configuration I don't even understand) it made it impossible to authenticate at all with some pam configurations when 2fa is required.

alexey-tikhonov · 2025-04-10T12:36:43Z

I mean you claim a deficiency in a fix for #7456

mstone232 · 2025-04-10T12:40:22Z

yes, the fix for #7456 caused a new bug

alexey-tikhonov · 2025-04-12T11:39:08Z

@justin-stephenson, @thalman, could you please take a look since you reviewed original fix?

ikerexxe · 2025-04-16T07:43:30Z

This patch adds the logic used elsewhere in pam_sss to determine whether a pam_authtok string should be treated as SSS_AUTHTOK_TYPE_PASSWORD or SSS_AUTHTOK_TYPE_2FA_SINGLE. Given the duplication it may be useful to refactor that logic into a separate function, but I tried to keep the change minimal.

I'm fine with the changes, but are you willing to implement the refactor you are mentioning? This will clean up the code a bit and you can do it in a different commit to keep things separate.

sumit-bose · 2025-04-16T08:34:31Z

Hi,

it is a general problem to determine how to handle credentials forwarded by other PAM modules. That's why we recommend to use pam_localuser.so, pam_usertype.so or or pam_succeed_if.so to call pam_unix.so only for local users and pam_sss.so for the remaining. This becomes even more important since we are working on offering the user more choices of authentication when logging in, i.e. SSSD will offer a selection of methods the user can use to authenticate. As a result of this I would like to ask you the change your patch at least in the sense that flags & PAM_CLI_FLAGS_USE_2FA should be always set, i.e. replace the || with &&. The reason is that in older versions the SSSD PAM responder made a decision which authentication type should be offered to the user and as a result typically only one type was sent to pam_sss.so. So if e.g. pi->otp_token_id was set it was safe to assume that only two-factor authentication credentials were expected. Currently, it might be possible that although pi->otp_token_id is set, that there might be smartcard or passkey authentication available as well and the credentials forwarded by pam_unix.so might be a PIN for one of those.

Since there already is the unfortunately undocumented option use_2fa, this can be used to indicate that the forwarded credentials are for two-factor authentication, with the side effect the now all SSSD users have to use two-factor authentication, not sure if this fits your use-case. Imo a more general and better approach would be to add a new option for pam_sss.so, e.g. forwarded_authtok_type with options like password, 2fa, smartcard, passkey.

bye,
Sumit

mstone232 · 2025-04-16T10:56:08Z

it is a general problem to determine how to handle credentials forwarded by other PAM modules. That's why we recommend to use pam_localuser.so, pam_usertype.so or or pam_succeed_if.so to call pam_unix.so only for local users and pam_sss.so for the remaining. This becomes even more important since we are working on offering the user more choices of authentication when logging in, i.e. SSSD will offer a selection of methods the user can use to authenticate. As a result of this I would like to ask you the change your patch at least in the sense that flags & PAM_CLI_FLAGS_USE_2FA should be always set, i.e. replace the || with &&. The reason is that in older versions the SSSD PAM responder made a decision which authentication type should be offered to the user and as a result typically only one type was sent to pam_sss.so. So if e.g. pi->otp_token_id was set it was safe to assume that only two-factor authentication credentials were expected. Currently, it might be possible that although pi->otp_token_id is set, that there might be smartcard or passkey authentication available as well and the credentials forwarded by pam_unix.so might be a PIN for one of those.

I think you are underweighting the fact that a configuration that was previously working will stop working when sss is upgraded. If you want to require additional options to enable new functionality that's fine, but as things exist now a person is unable to sudo to change the configuration after installing the new sss. I think maintaining compatibility with the prior release is essential. It would be valid to deprecate this pam configuration syntax entirely and not support any new functionality until the pam configuration is changed, but people need to be able to authenticate as they did before in order to make that change. Making continued administration of the system dependent on an undocumented configuration flag makes no sense, IMO.

mstone232 · 2025-04-16T10:58:31Z

I'm fine with the changes, but are you willing to implement the refactor you are mentioning? This will clean up the code a bit and you can do it in a different commit to keep things separate.

given @sumit-bose comment I don't know what a refactor should look like

sumit-bose · 2025-04-16T11:41:22Z

it is a general problem to determine how to handle credentials forwarded by other PAM modules. That's why we recommend to use pam_localuser.so, pam_usertype.so or or pam_succeed_if.so to call pam_unix.so only for local users and pam_sss.so for the remaining. This becomes even more important since we are working on offering the user more choices of authentication when logging in, i.e. SSSD will offer a selection of methods the user can use to authenticate. As a result of this I would like to ask you the change your patch at least in the sense that flags & PAM_CLI_FLAGS_USE_2FA should be always set, i.e. replace the || with &&. The reason is that in older versions the SSSD PAM responder made a decision which authentication type should be offered to the user and as a result typically only one type was sent to pam_sss.so. So if e.g. pi->otp_token_id was set it was safe to assume that only two-factor authentication credentials were expected. Currently, it might be possible that although pi->otp_token_id is set, that there might be smartcard or passkey authentication available as well and the credentials forwarded by pam_unix.so might be a PIN for one of those.

I think you are underweighting the fact that a configuration that was previously working will stop working when sss is upgraded. If you want to require additional options to enable new functionality that's fine, but as things exist now a person is unable to sudo to change the configuration after installing the new sss. I think maintaining compatibility with the prior release is essential. It would be valid to deprecate this pam configuration syntax entirely and not support any new functionality until the pam configuration is changed, but people need to be able to authenticate as they did before in order to make that change. Making continued administration of the system dependent on an undocumented configuration flag makes no sense, IMO.

Hi,

thank you for your comment. I completely agree that they recent changes in SSSD are a regression with respect to your PAM configuration. However, it was not expected to work as I tried to explain since pam_sss.so so far didn't expect anything else than passwords to be forwarded by other PAM modules.

Since, as far as I can see your patch won't break any other use case and, as mentioned, using the use_2fa option has other effects as well, I'm fine including it as is or with a refactoring by e.g. putting the condition for the 2FA credentials into a #define. I will write a patch with the new option for pam_sss.so which will make sure your use-case will keep working without setting the new option, but will deprecate this use-case with a note that a future major version of SSSD will remove the support for it. Will this work for you?

bye,
Sumit

mstone232 · 2025-04-16T11:50:58Z

thank you for your comment. I completely agree that they recent changes in SSSD are a regression with respect to your PAM configuration. However, it was not expected to work as I tried to explain since pam_sss.so so far didn't expect anything else than passwords to be forwarded by other PAM modules.

Yes pam_sss didn't expect anything but passwords, but until #7462 passwords and 2fa_single had identical semantics when 2fa was being used.

Since, as far as I can see your patch won't break any other use case and, as mentioned, using the use_2fa option has other effects as well, I'm fine including it as is or with a refactoring by e.g. putting the condition for the 2FA credentials into a #define. I will write a patch with the new option for pam_sss.so which will make sure your use-case will keep working without setting the new option, but will deprecate this use-case with a note that a future major version of SSSD will remove the support for it. Will this work for you?

Sounds good. Maybe a runtime warning if the implicit behavior is in use (which would be eliminated by setting explicit parameter)?

ikerexxe · 2025-04-30T07:51:41Z

@mstone232 If you don't mind, I'd like to see this refactored to make the condition easier to understand.

This reverts commit 4a435eb.

Makes explicit the fact that a credential obtained by use_first_pass from the pam stack is neither an SSS_AUTHTOK_TYPE_PASSWORD nor an SSS_AUTHTOK_TYPE_2FA_SINGLE, but rather a Schrodinger's cat whose status can't be determined until it is used.

mstone232 · 2025-05-07T13:47:00Z

@sumit-bose @ikerexxe based on the comments and rethinking the problem a bit, I've taken the patch in a different direction. The problem I see is that the current flow (and my previous change) are trying to cram the use_first_pass credential into either an SSS_AUTHTOK_TYPE_PASSWORD or an SSS_AUTHTOK_TYPE_2FA_SINGLE, but it's not really either of those at the point the decision is made. What I've done is add a new type, SSS_AUTHTOK_TYPE_PAM_STACKED specifically for the use_first_pass case where a credential is passed from a prior pam module and there's no chance to be explicit about its type. That lets us defer the decision about how to use that credential until we're actually asked for either a password or an otp. This restores the original semantics for use_first_pass, and I've tested that it works with IPA in either password or 2fa mode without having to fiddle with use_2fa.

sumit-bose · 2025-05-08T14:42:53Z

Hi,

thank you, I like the idea, especially moving the decision away from the PAM module. So far I see no issue with the approach but I have to think a bit more about it. In the meantime I wonder if you can fix the issue ci /build complains about

 ../src/util/authtok.c:512:56: error: suggest parentheses around '&&' within '||' [-Werror=parentheses]
  512 |     if (!tok || tok->type != SSS_AUTHTOK_TYPE_PASSWORD &&
      |                 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~
  513 |                 tok->type != SSS_AUTHTOK_TYPE_PAM_STACKED) {
      |                 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

While technically correct it would be much easier to read/understand if there are parentheses.

bye,
Sumit

mstone232 · 2025-05-08T15:56:13Z

In the meantime I wonder if you can fix the issue ci /build complains about

done

Correct pam_sss behavior with inherited 2fa_single authtok

4a435eb

alexey-tikhonov added the Waiting for review label Apr 10, 2025

alexey-tikhonov added the branch: sssd-2-9 label Apr 10, 2025

andreboscatto requested review from ikerexxe and sumit-bose April 15, 2025 12:41

andreboscatto assigned ikerexxe and sumit-bose Apr 15, 2025

mstone232 and others added 5 commits May 6, 2025 15:07

Merge branch 'SSSD:master' into master

18d8117

Revert "Correct pam_sss behavior with inherited 2fa_single authtok"

19c0a3f

This reverts commit 4a435eb.

return here so MINOR_FAILURE isn't auto-promoted to FATAL_FAILURE

61efc7b

make log line match preceeding function name

a555009

add SSS_AUTHTOK_TYPE_PAM_STACKED

8eecb11

Makes explicit the fact that a credential obtained by use_first_pass from the pam stack is neither an SSS_AUTHTOK_TYPE_PASSWORD nor an SSS_AUTHTOK_TYPE_2FA_SINGLE, but rather a Schrodinger's cat whose status can't be determined until it is used.

address paren warning

fe52629

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Correct pam_sss behavior with inherited 2fa_single authtok #7916

Correct pam_sss behavior with inherited 2fa_single authtok #7916

Correct pam_sss behavior with inherited 2fa_single authtok #7916

Are you sure you want to change the base?

Correct pam_sss behavior with inherited 2fa_single authtok #7916

Conversation