Add support for fingerprint login #654

JAicewizard · 2024-07-17T15:18:24Z

This is a port of https://github.com/kercre123/ly, hence he is added as co-author.
This PR adds support for fingerprint login, and fixes #227.
I tested this in a single-user setup, by patching this into the arch package. logging in as a non-default user was not tested.

JAicewizard · 2024-07-17T18:29:07Z

There turned out to be a couple of issues, one was a fast blinking cursor, which has not been fixed, and one issue where after logging in using fingerprint auth, then logging out, logging back in results in an inability to start sway. I tried reproducing the normal auth flow (not pushed) but nothing really fixes. I might have some time to fix this, but some help would be appreciated!

JAicewizard · 2024-07-18T14:02:05Z

I fixed the issue! I also split the normal auth flow in 2, just like the fingerprint auth flow. This removes a lot of duplicate code, and keeps the 2 flows very much similar.

AnErrupTion

The more I look at this PR, the more it looks like just an automatic login feature to me. Unless I missed something, nothing seems to be fingerprint-specific and is just trying to first login at startup (which is fine), but also constantly login afterwards (which is less fine). I believe the latter will burn CPU cycles for not much gain, since the user could simply press Enter after touching their fingerprint reader, or so I assume.

To wrap this up, I have a question: if we were to only apply the changes made to the PAM configuration file, would we be able to still login using a fingerprint reader? I'm not trying to downplay your code additions of course, I'm simply trying to understand if the rest you've added could simply be defined as "convenience features", if you see what I mean.
And, if that's the case, then I think this PR should better be rebranded as adding support for automatic login instead of just fingerprint login.

res/pam.d/ly

src/auth.zig

src/main.zig

AnErrupTion · 2024-07-27T15:36:48Z

src/main.zig

+                        const shouldFingerprint = switch (event.key) {
+                            termbox.TB_KEY_DELETE => true,
+                            termbox.TB_KEY_BACKSPACE, termbox.TB_KEY_BACKSPACE2 => true,
+                            else => event.ch > 31 and event.ch < 127,
+                        };
+
+                        if (shouldFingerprint) {
+                            try startFingerPrintLogin(allocator, config, login);
+                        }


This is a bad idea. If anything, it's better to first attempt fingerprint-based login when pressing Enter before attempting to do regular password-based login. But then this wouldn't do much given that the user could simply authenticate normally without a password and it'd do the same thing (at least, as I understand).

This is a bad idea.

Why is this a bad idea? I understand that spawning a lot of threads would be less than ideal, would it be better to do this when the user moved away from the username into the password field? I think I currently like that better than what I implemented now.

It's a bad idea because the user might not expect Ly to try logging in at each keystroke. Probably here, spawning a single thread that'd try logging in using the fingerprint is a better idea.

Yup got it, agree

I dont think we can get all the way there. The problem is that when an authentication is already happening, there is no way to cancel it. So if the user switches username for example, the old auth is still running with no way to cancel it, and thus that thread cannot be used to start a new authentication, and we need to start a new thread. However, I did make it so that it only starts a new authentication when you go to the password field, such that very few threads will be spawned, likely just one. I suspect that only a malicious user would create any significant amount of threads.

src/main.zig

AnErrupTion · 2024-07-27T15:48:51Z

src/main.zig

+                    try info_line.setText(lang.logout);
+                }
+                asyncPamHandle = null;
+                try startFingerPrintLogin(allocator, config, login);


I don't understand. Is it trying to authenticate again after logging out?

Yes it is, if someone logged in with their fingerprint, and logged out, they should be able to login with their fingerprint. This might be a problem for something that is permanently plugged into a computer. We could wait for first user-input after logout? This would avoid causing a loop. Optionally we could even make sure the user currently has their cursor on the password field, to allow users to change configuration without being put back into their environment.

But why is it needed in the first place? Can't they just log back in using their fingerprint like they would've done on their first login?

Because we need to start the authentication flow again, after a successful authentication the flow stopped and we need to restart it. But I will first rework this code, maybe its good to look at it again after thatn.

Reworked the code so it looks nicer, and I made is such that on logout it only restarts when entering the password field.

JAicewizard · 2024-08-13T09:42:02Z

I'm sorry, github does not seem to be sending me emails about anything anymore, im subscribed to this thread but I just didn't get any emails!! (I even searched all my emails, nothing).

JAicewizard

I believe the latter will burn CPU cycles for not much gain, since the user could simply press Enter after touching their fingerprint reader, or so I assume.

This is true, but I think that restarting a PAM login every few minutes is not so bad. We could also increase the timeout to an hour so that there should be basically no wasted cycles (without removing the feature) (https://man.archlinux.org/man/extra/fprintd/pam_fprintd.8.en#timeout=). We are also not doing any expensive computations, the most power-hungry part is probably activating the fingerprint reader, but that is simply the cost of the user enabling it, not something we can do anything about.

To wrap this up, I have a question: if we were to only apply the changes made to the PAM configuration file, would we be able to still login using a fingerprint reader? I'm not trying to downplay your code additions of course, I'm simply trying to understand if the rest you've added could simply be defined as "convenience features", if you see what I mean.

Yes, it would still "work", but without the "convenience features". The idea is that someone should be able to boot their laptop, press the fingerprint-reader, and login, without the user knowing about the PAM hack of filling in an empty password. Users shouldn't have to figure out how to use it, it should "just work".

PS
I have been working on another project in zig with some synchronization primitives, and I think I can clean up the absolute mess that is the timeout code, it is truly horrifying. I don't feel like putting in the effort if you do not want the feature, but if you have the intention of merging this if it gets into a better state I will clean it up.

src/auth.zig

src/main.zig

JAicewizard · 2024-08-13T10:10:42Z

src/main.zig

+                    try info_line.setText(lang.logout);
+                }
+                asyncPamHandle = null;
+                try startFingerPrintLogin(allocator, config, login);


Yes it is, if someone logged in with their fingerprint, and logged out, they should be able to login with their fingerprint. This might be a problem for something that is permanently plugged into a computer. We could wait for first user-input after logout? This would avoid causing a loop. Optionally we could even make sure the user currently has their cursor on the password field, to allow users to change configuration without being put back into their environment.

src/main.zig

JAicewizard · 2024-08-13T10:20:41Z

src/main.zig

+                        const shouldFingerprint = switch (event.key) {
+                            termbox.TB_KEY_DELETE => true,
+                            termbox.TB_KEY_BACKSPACE, termbox.TB_KEY_BACKSPACE2 => true,
+                            else => event.ch > 31 and event.ch < 127,
+                        };
+
+                        if (shouldFingerprint) {
+                            try startFingerPrintLogin(allocator, config, login);
+                        }


This is a bad idea.

Why is this a bad idea? I understand that spawning a lot of threads would be less than ideal, would it be better to do this when the user moved away from the username into the password field? I think I currently like that better than what I implemented now.

AnErrupTion · 2024-08-13T21:11:04Z

@JAicewizard

This is true, but I think that restarting a PAM login every few minutes is not so bad. We could also increase the timeout to an hour so that there should be basically no wasted cycles (without removing the feature)

I'm not a huge fan of adding such a long artificial timeout, plus, with the link you provided, wouldn't this only apply to the pam_fprintd.so module?

We are also not doing any expensive computations, the most power-hungry part is probably activating the fingerprint reader, but that is simply the cost of the user enabling it, not something we can do anything about

Yes, but think about a laptop for example: even if it's not doing anything expensive, if even just one core is pegged at 100%, the fans may spin really fast, which makes you wonder what Ly is even doing in the background... and it's not great.

I don't feel like putting in the effort if you do not want the feature, but if you have the intention of merging this if it gets into a better state I will clean it up.

I do, in fact, want to merge this PR! So if you can improve this PR, then do it! :D

JAicewizard · 2024-08-14T12:02:20Z

Yes, but think about a laptop for example: even if it's not doing anything expensive, if even just one core is pegged at 100%, the fans may spin really fast, which makes you wonder what Ly is even doing in the background... and it's not great.

I understand the concern, but while in PAM land the CPU should be idle. pam_fprintd.so offloads to fprintd which waits for a finger to be detected and only then starts to actually perform any computations. This waiting is done using some kind of low-level communication with the reader, fprind shows 0% usage when waiting for a finger for me, I hope that alleviates these concerns.

I do, in fact, want to merge this PR! So if you can improve this PR, then do it! :D

Then I will start working on them when I have time :) I will also do a rebase, just so you know

Many thanks to Kerigan for providing the original C code. This commit mainly posts his code to zig. Additional features provided by me are: - automatically checking every 100ms - request new login info for new usernames Co-authored-by: Kerigan <kerigancreighton@gmail.com>

This also involves a major split of the authentication code, which allows the fingerprint and password logins to use the same code.

JAicewizard · 2024-08-24T10:53:35Z

I am not sure why github is saying there are merge conflicts, this branch should be fully up-to-date with upstream except one commit which is just about translations, but I think I resolved most issues you pointed out.

hossamdash · 2024-09-12T20:56:07Z

@AnErrupTion , would be great if work on this can continue, I can volunteer as tester, I think this will make Ly the best login manager for most linux and specifically wayland users right now.

JAicewizard force-pushed the fingerprint branch from 3f43e14 to 13a33e9 Compare July 18, 2024 14:02

JAicewizard changed the title ~~Add support for fingerprint login support~~ Add support for fingerprint login Jul 19, 2024

AnErrupTion requested changes Jul 27, 2024

View reviewed changes

JAicewizard commented Aug 13, 2024

View reviewed changes

JAicewizard and others added 4 commits August 20, 2024 17:33

Add fprintd to the pam file

cc15ab3

Fix fast blinking cursor when no timeout is set

2db48c5

Fix processing wrt fingerprint login

c42cf12

This also involves a major split of the authentication code, which allows the fingerprint and password logins to use the same code.

JAicewizard force-pushed the fingerprint branch 2 times, most recently from 3e086a3 to a53b904 Compare August 24, 2024 10:35

Reimplement most of the asynchronous handling

4bf4de8

JAicewizard force-pushed the fingerprint branch from a53b904 to 4bf4de8 Compare August 24, 2024 10:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for fingerprint login #654

Add support for fingerprint login #654

JAicewizard commented Jul 17, 2024

JAicewizard commented Jul 17, 2024

JAicewizard commented Jul 18, 2024

AnErrupTion left a comment

AnErrupTion Jul 27, 2024

JAicewizard Aug 13, 2024

AnErrupTion Aug 13, 2024

JAicewizard Aug 14, 2024

JAicewizard Aug 24, 2024

AnErrupTion Jul 27, 2024

JAicewizard Aug 13, 2024

AnErrupTion Aug 13, 2024

JAicewizard Aug 14, 2024

JAicewizard Aug 24, 2024

JAicewizard commented Aug 13, 2024

JAicewizard left a comment

JAicewizard Aug 13, 2024

JAicewizard Aug 13, 2024

AnErrupTion commented Aug 13, 2024

JAicewizard commented Aug 14, 2024

JAicewizard commented Aug 24, 2024

hossamdash commented Sep 12, 2024

Add support for fingerprint login #654

Are you sure you want to change the base?

Add support for fingerprint login #654

Conversation

JAicewizard commented Jul 17, 2024

JAicewizard commented Jul 17, 2024

JAicewizard commented Jul 18, 2024

AnErrupTion left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JAicewizard commented Aug 13, 2024

JAicewizard left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AnErrupTion commented Aug 13, 2024

JAicewizard commented Aug 14, 2024

JAicewizard commented Aug 24, 2024

hossamdash commented Sep 12, 2024