PAM: user feedback when login fails due to blocked PIN

[alexey-tikhonov]

Resolves: #6153

[alexey-tikhonov]

Tested manually:

$ su test2
PIN for test2: 
PIN locked
su: Authentication failure

[alexey-tikhonov]

@sumit-bose, does this message need a localization?

[alexey-tikhonov]

Another thing is, probably it's not worth providing this string from sssd_pam to sss_client at all. It could be hard-coded in sss_client...

[sumit-bose]

Hi,

sorry for the delay. Yes, I think it is a good idea to have a localization for this string and as a consequence it would be better to just send the type and let pam_sss generate the message, in the locale of the calling environment, similar to SSS_PAM_USER_INFO_OTP_CHPASS and user_info_otp_chpass().

bye,
Sumit

[alexey-tikhonov]

Done.

[justin-stephenson]

Changes LGTM.

[sumit-bose]

Hi,

thank you for the updates, you accidentally added translation files to the commit.

Additionally I wonder if it wouldn't be easier to call pam_add_response() not in pam_reply() but directly after parse_p11_child_response() is called and has returned ERR_P11_PIN_LOCKED. The pam_data struct is already passed to the pam_check_cert_send() request and just has to be added to the state struct. This would keep everything Smartcard related into pamsrv_p11.c and avoid the switching between different return values and does not block PAM_MAXTRIES from other usage.

bye,
Sumit

[alexey-tikhonov]

thank you for the updates, you accidentally added translation files to the commit.

Hm, but I did this intentionally: since patch adds new _() string, I rebuild po-files (to make translation team aware of new string). Is this wrong?

Additionally I wonder if it wouldn't be easier to call pam_add_response() not in pam_reply() but directly after parse_p11_child_response() is called and has returned ERR_P11_PIN_LOCKED. The pam_data struct is already passed to the pam_check_cert_send() request and just has to be added to the state struct. This would keep everything Smartcard related into pamsrv_p11.c and avoid the switching between different return values and does not block PAM_MAXTRIES from other usage.

That's better, right. I didn't know it was possible. Thanks.

[alexey-tikhonov]

thank you for the updates, you accidentally added translation files to the commit.

Hm, but I did this intentionally: since patch adds new _() string, I rebuild po-files (to make translation team aware of new string). Is this wrong?

Or do I need to commit only po/sssd.pot?

[sumit-bose]

thank you for the updates, you accidentally added translation files to the commit.

Hm, but I did this intentionally: since patch adds new _() string, I rebuild po-files (to make translation team aware of new string). Is this wrong?

Or do I need to commit only po/sssd.pot?

Hi,

I think this should be sufficient but I'm currently not sure how translations are handled. I remember some long time ago we had some string-freeze, say 4 weeks before a release, where the pot file was updates and pushed upstream so that translators could add there contributions which were then pulled in before the release was done. But currently it look like the pot file is updated directly before a release. @pbrezina, do you have any details here?

bye,
Sumit

[alexey-tikhonov]

I remember some long time ago we had some string-freeze, say 4 weeks before a release, where the pot file was updates and pushed upstream so that translators could add there contributions which were then pulled in before the release was done.

This is exactly my intention, because this is last planned patch that touches strings in 2.7.z series.

I don't know if sssd.pot would be enough, but I think rebuilding all po-files won't hurt?

[alexey-tikhonov]

Additionally I wonder if it wouldn't be easier to call pam_add_response() not in pam_reply() but directly after parse_p11_child_response() is called and has returned ERR_P11_PIN_LOCKED. The pam_data struct is already passed to the pam_check_cert_send() request and just has to be added to the state struct. This would keep everything Smartcard related into pamsrv_p11.c and avoid the switching between different return values and does not block PAM_MAXTRIES from other usage.

That's better, right. I didn't know it was possible. Thanks.

Done in the latest version.

I also dropped all po-files updates per agreement with @pbrezina.

[alexey-tikhonov]

Rebased.

[alexey-tikhonov]

Covscan complains:

Error: :Error: CHECKED_RETURN (CWE-252):
sssd-pr6162/src/responder/pam/pamsrv_p11.c:1021: check_return: Calling "pam_add_response" without checking return value (as is done elsewhere 37 out of 42 times).

But I don't think it's worth addressing. There is no action to take in case pam_add_response() fails.

[alexey-tikhonov]

(Requesting re-review by @justin-stephenson because patch was changed.)

[justin-stephenson]

functionality works as expected.

[justin@agalloch yubikey]$ yubico-piv-tool -a verify -P 000000
Pin verification failed, 2 tries left before pin is blocked.
[justin@agalloch yubikey]$ yubico-piv-tool -a verify -P 000000
Pin verification failed, 1 tries left before pin is blocked.
[justin@agalloch yubikey]$ yubico-piv-tool -a verify -P 000000
Pin code blocked, use unblock-pin action to unblock.
[justin@agalloch yubikey]$ su - test
PIN for test: 
PIN locked
su: Authentication failure

[sumit-bose]

Hi,

code looks good and my test were successful as well, ACK.

bye,
Sumit

[pbrezina]

Pushed PR: #6162

• master
  • 5433961 - PAM: user feedback when login fails due to blocked PIN
  • f119522 - PAM P11: fixed minor mem-leak
  • 1ed59fb - PAM P11: fixed mistype in a log message
• sssd-2-7
  • f0609d8 - PAM: user feedback when login fails due to blocked PIN
  • aec9733 - PAM P11: fixed minor mem-leak
  • abc2ae5 - PAM P11: fixed mistype in a log message