ping

I plan to push this on Monday, so a review would be appreciated till then :-)

ping! This is security relevant code. I don't want to push without anybody looking at it!

Since there seem to be not many others I'll try to give you reviews for this and your following patches, but first I've to understand this stuff.

Tell me if the following is right:

Until now the greeter waited for the exit code of the forked process with the pid m_pid. The child process executes the kcheckpass binary via execlp for pam/etcshadow password validation and exits immediately. The parent process now reads the requests and results from kcheckpass via socket m_fd. It gets notified when kcheckpass has written something to the socket thanks to m_notifier.

First kcheckpass's pam/etcshadows part asks for the provided password, i.e. transmits ConvGetNormal / ConvGetHidden, so KCheckPass::handleVerify writes back the provided password to the socket. kcheckpass can apparently without having to wait directly read it from the socket. Pam/etcshadow compares it with the set user password and (until now) exits with the respective success or fail code.

With your change instead of setting an exit code conv_server writes back one more time to the socket with the result (i.e. only the enum signaling it with empty prompt).

EDIT: Follow up question: In the current version at which point in the execution is the reapVerify method called at all? handleVerify is only connected to the activated method of QSocketNotifier and I assume that kcheckpass writes to the socket in the end and handleVerify is called one last time with GRecvInt( &ret ) being false. But I don't see any more writes to the socket in kcheckpass.c and for example checkpass_shadow.c after the initial query for the provided password.

In D4806#96684, @subdiff wrote:

Since there seem to be not many others I'll try to give you reviews for this and your following patches, but first I've to understand this stuff.

Tell me if the following is right:

Until now the greeter waited for the exit code of the forked process with the pid m_pid.

yes, the exit code was the auth result. exit code 0 means auth success, everything else failure.

The child process executes the kcheckpass binary via execlp for pam/etcshadow password validation and exits immediately. The parent process now reads the requests and results from kcheckpass via socket m_fd. It gets notified when kcheckpass has written something to the socket thanks to m_notifier.

yes

First kcheckpass's pam/etcshadows part asks for the provided password, i.e. transmits ConvGetNormal / ConvGetHidden, so KCheckPass::handleVerify writes back the provided password to the socket. kcheckpass can apparently without having to wait directly read it from the socket. Pam/etcshadow compares it with the set user password and (until now) exits with the respective success or fail code.

yes

With your change instead of setting an exit code conv_server writes back one more time to the socket with the result (i.e. only the enum signaling it with empty prompt).

yes

EDIT: Follow up question: In the current version at which point in the execution is the reapVerify method called at all? handleVerify is only connected to the activated method of QSocketNotifier and I assume that kcheckpass writes to the socket in the end and handleVerify is called one last time with GRecvInt( &ret ) being false. But I don't see any more writes to the socket in kcheckpass.c and for example checkpass_shadow.c after the initial query for the provided password.

reapVerify seems to me to be only if the communication horribly breaks. I did not fully understand the code and what it was doing. Part is to wait for the auth result, part is further error checking.
In the long run I want to replace this by a nice QProcess, but one step at a time. This change is actually only a preparation step for the follow up which has the long running kcheckpass.

In D4806#97351, @graesslin wrote:

reapVerify seems to me to be only if the communication horribly breaks.

I don't think that's the case, because otherwise greeter could've never read in the exit code of kcheckpass on current master.

It's more like this: In kcheckpass.c the method conv_server() first sends GSendInt(), which applies to one of the cases in handleVerify(). Then (in the cases ConvGetNormal and ConvGetHidden) conv_server() sends again a string via GSendStr. For checkpass_shadow.c this is the 0 string, which then falls through to reapVerify().

In D4806#97649, @subdiff wrote:

In D4806#97351, @graesslin wrote:

reapVerify seems to me to be only if the communication horribly breaks.

I don't think that's the case, because otherwise greeter could've never read in the exit code of kcheckpass on current master.

I mean in the new code. In the old one: yes it was relevant to handle the authentication.