Drive interaction from the authentication backend (PAM) #389
Conversation
This helps clarify the purpose
Note that write_comm_auth_result_from_backend could return boolean (like comm_reply_did), but using ssize_t for consistency with the other comm functions.
This gets rid of the static pw_buf.
mgunyho
changed the title
mgunyho
force-pushed
the
pam-driven-conversation
branch
from
b7cab7f to
b37d16f
Compare
|
I have added a simple UI to show the messages from PAM, here is a quick video showing how it interacts with the typing indicator and what happens when the fingerprint validation fails. screen-recording-2025-01-06-124722.mp4 |
mgunyho
force-pushed
the
pam-driven-conversation
branch
from
b37d16f to
eead45d
Compare
|
One issue with the UI currently is that if PAM is waiting for the fingerprint but you submit a password, the indicator says "verifying" which is misleading |
comm.c
Outdated
| @@ -9,41 +9,87 @@ | |||
|
|
|||
| static int comm[2][2] = {{-1, -1}, {-1, -1}}; | |||
|
|
|||
| ssize_t read_comm_request(char **buf_ptr) { | |||
| ssize_t write_string(int fd, const char * const *string, size_t len) { | |||
There was a problem hiding this comment.
I don't have the time and setup to comment on the UI right now, but can advise on matters of C.
A double pointer is unnecessary when writing strings; this function and the functions which call it should use a plain const char *string instead.
(Using unnecessary pointers is also risky: the current write(fd, string[offs], len - offs); compiles fine, but actually has a bug because string[offs] indexes the outer pointer and not the inner one; with a const char *string argument, one has write(fd, &string[offs], len - offs); instead.)
There was a problem hiding this comment.
Oh, you are right. I guess my thinking was that "char * is a string, and a string should be passed by reference instead of value". But of course it's already a reference because it's a pointer. I fixed this now in bc18cc2 (I can clean up the commit history at the end.)
I also see that string[offs] is wrong, but how come it works? ... Oh is it because offs always happens to be zero?
pam.c
Outdated
| swaylock_log(LOG_ERROR, "Failed to read prompt response"); | ||
| return PAM_ABORT; | ||
| } | ||
| //TODO: what to do if size == 0? in fbc5a8136187, if read_comm_prompt_response (there called read_comm_request) returned 0, it would break the while loop (i.e. auth successfully); we can't do that here |
There was a problem hiding this comment.
If size==0, then the pipe connecting to the main swaylock process has disconnected because e.g. swaylock has exited (e.g. unlocked by signal, or crashed). I think the correct thing to do here would be to return PAM_ABORT, so that pam_authenticate returns PAM_ABORT; the current authentication attempt cannot complete.
If you furthermore want run_pw_backend_child() to not warn about this specific exit path, you could provide it with additional information by passing a pointer to a variable declared in run_pw_backend_child as .appdata_ptr in struct pam_conv conv, and having handle_conversation modify the variable depending on the reason for exit.
There was a problem hiding this comment.
Yeah, it makes sense to return PAM_ABORT. I think I was confused because in the current version, size == 0 breaks the while loop and exits with EXIT_SUCCESS, so I thought it indicates success, but indeed the subprocess exit status doesn't necessarily say anything about success on the main process side.
Fixed in 60943f8.
This is related to #61, #213, #244, #251, possibly #270, #283 (though the parallel fingerprint/password case is not implemented).
Instead of blocking until the password is submitted, the backend process goes directly to
pam_authenticate. The messages and prompts displayed to the user and reading the input are based on the PAM conversation. The main change is in e0775f4.Currently the UI part is completely missing, but hopefully with this code it's easier to test various ideas. It's pretty easy to mess up the UI state, by e.g. entering incorrect fingerprints and then an incorrect password.I added a simple UI showing the last few PAM messages in eead45d.Some testing is also needed with more advanced scenarios like Yubikeys, I only have a fingerprint reader available.
I'm not very experienced with C and I had some trouble with casting the void pointers, I hope I didn't mess up. Please be gentle :)
The first two commits are just some additions to logging that help with debugging, let me know if I should remove them or create separate PRs.