Remove the "fail_delay()" function
AbandonedPublic

Authored by jfranklin on Sep 5 2019, 1:34 PM.

Details

Reviewers
zzag
Group Reviewers
Plasma
KWin
Summary

I'm a new contributor to kscreenlocker, so I thought I'd start with something very simple to see how the process goes. I hope I'm in line with standard procedures for proposing a change.

I've been spending a lot of time working with the kscreenlocker code in an attempt to debug its conversation with Poldi, which is a PAM module for allowing authentication with GnuPG smart cards. In this time, I've built up some familiarity with the code, and I'd like to submit a few patches over the next couple of weeks to improve things.

This change is pretty simple, and it makes a great deal of sense to me. I'd be very curious to know if there is a reason for having a fail_delay() function that performs no action. Just let me know!

Otherwise, the relevant explanation is in the commit message itself. This appears to have been removed, so I'll add it below:

Remove the "fail_delay()" function

It appears that the custom "fail_delay()" function was defined to
avoid a delay when a call to "pam_authenticate()" fails.  This would
be the effect since the custom function performs no actions.

The Linux-PAM system has a more concise method for disabling the
delay on authentication failure.  By explicitly setting the
"PAM_FAIL_DELAY" item to NULL, we disable any delay and render the
custom "fail_delay" function redundant.

The official Linux-PAM documentation explains this behavior.  As of
this commit, "man pam_fail_delay" explains the following:

 "Note, if PAM_FAIL_DELAY item is unset (or set to NULL), then no
  delay will be performed."

Removing this function seems like a good idea in the interest of
code cleanliness.

I hope the above is not redundant, but I don't see where Phabricator preserved the commit message.

Test Plan

The current tests don't seem to cover the delay from a failure to pam_authenticate(). If one needs to be added, I'd like to help! I've spent a lot of time learning how this project works, and I'd be happy if one of the maintainers here could help me get the test suite up and running.

This patch should change no functionality and, thus, will need no changes in the test code.

Diff Detail

Repository
R133 KScreenLocker
Lint
Lint Skipped
Unit
Unit Tests Skipped
jfranklin created this revision.Sep 5 2019, 1:34 PM
Restricted Application added a project: Plasma. · View Herald TranscriptSep 5 2019, 1:34 PM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
jfranklin requested review of this revision.Sep 5 2019, 1:34 PM
jfranklin edited the summary of this revision. (Show Details)Sep 5 2019, 1:39 PM
ngraham added a reviewer: KWin.Sep 5 2019, 1:42 PM
jfranklin edited the summary of this revision. (Show Details)Sep 5 2019, 1:42 PM
zzag added a subscriber: zzag.Sep 5 2019, 5:05 PM

I'd be very curious to know if there is a reason for having a fail_delay() function that performs no action. Just let me know!

We want to notify the user as soon as possible about authentication failure.

jfranklin added a comment.EditedSep 5 2019, 5:08 PM
In D23734#526274, @zzag wrote:

We want to notify the user as soon as possible about authentication failure.

This makes complete sense! You definitely want the user to be notified immediately. However, the same effect happens with the call pam_set_item(pamh, PAM_FAIL_DELAY, NULL). The Linux-PAM library documentation makes it clear that, when this is set to NULL, no delay will happen.

See this link: http://www.linux-pam.org/Linux-PAM-html/adg-interface-by-app-expected.html#adg-pam_fail_delay

EDIT: Therefore this special fail_delay() function is not necessary.

zzag requested changes to this revision.Sep 5 2019, 5:25 PM

Hmm, "Unlocking failed" doesn't show up immediately if I type wrong password.

This revision now requires changes to proceed.Sep 5 2019, 5:25 PM
In D23734#526285, @zzag wrote:

Hmm, "Unlocking failed" doesn't show up immediately if I type wrong password.

"Unlocking failed" shows up immediately when I type in the wrong password with the change in place. What PAM module is authenticating you?

zzag added a comment.Sep 6 2019, 8:39 AM

Um, pam_unix I suppose...

jfranklin added a comment.EditedSep 6 2019, 6:04 PM

I investigated this further in the linux-pam repository, and I ended up creating this issue. The developers there acknowledged that the documentation was not correct and have asked for a PR to fix it.

Thus, please close/resolve this issue at your convenience.

Edit: Thanks!

zzag added a comment.Sep 6 2019, 6:55 PM

Thank you for looking further into this issue. Just abandon the revision.

jfranklin abandoned this revision.Sep 6 2019, 6:58 PM

Problem exists in linux-pam. I will fix the issue there. Abandoning revision.