Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

auth: implement a full pam conversation #205

Merged
merged 17 commits into from
Apr 10, 2024

Conversation

PaideiaDilemma
Copy link
Collaborator

@PaideiaDilemma PaideiaDilemma commented Mar 18, 2024

Hi!

Currently, on paper, we don't handle the pam conversation fully like it is intended by pam. (#107)
This is an implementation where we actually would handle it properly. At least I think so from what I gathered.

BUT, this kinda has diminishing returns, since you would only need this if you use a weird custom pam config or 2FA and I don't know if there are people who want a 2FA lockscreen.
I think fingerprint and yubikeys should work fine with the current solution (pls correct me if I am wrong).

Also having a fallback pam module is not really feasible this way. (To be fair personally I am not a fan of the su fallback, since it makes me have to wait longer when I fat finger my password)

What it gives you is the pam prompt and the ability to input multiple things in order to authenticate with pam.

Implementation Notes
Thought about using a socket like @Mikilio tried in #200.
But it seemed a bit overkill and I don't really want to send raw passwords over a socket if I don't have to.
This just uses a CV to wait for input.

I put this out to discuss. Still needs double checking. Also sorry for the single fat commit.

Question
I kinda think it would be cool to support a full conversation like this. But Idk if anyone would use it. Do we want it or no?

And sorry to @Mikilio for just overtaking this. I hope you don't mind ❤️

Closes #170

@PaideiaDilemma PaideiaDilemma marked this pull request as draft March 18, 2024 23:07
@vaxerski
Copy link
Member

some rebasing fails, I sense

@PaideiaDilemma PaideiaDilemma force-pushed the full-pam-conversation branch from 0d916e2 to c8b2caf Compare March 19, 2024 11:44
@PaideiaDilemma PaideiaDilemma force-pushed the full-pam-conversation branch from 7413e5a to 07e9baf Compare March 19, 2024 19:00
@bvr-yr
Copy link
Contributor

bvr-yr commented Mar 20, 2024

what about handling this messages?

image_2024-03-20_15-05-01

that's if i set pam_module = hyprlock, default one shipped
mine only differs by auth include system-auth because of autologin

currently input is blocked (which is okay) but it is stuck at checkWaiting i guess?

150510-area- 26 639-541x161

if i do

faillock --user bvr --reset

from another tty it does nothing, only killing with USR1
probably even separate color could be dedicated to faillock later

@PaideiaDilemma
Copy link
Collaborator Author

PaideiaDilemma commented Mar 21, 2024

Displaying if the account gets locked would be nice I think. But not in this PR probably.

I assume that previously doing faillock --user bvr --reset worked fine right? I will try to repro and fix that.

Also the default pam_module should of be the one we ship. I just wanted to check if it works on most of the major distros, because this PR removes the fallback.

Thanks for the input and testing!

@PaideiaDilemma
Copy link
Collaborator Author

I was not able to directly reproduce your problem @bvr-yr.

But I fixed some issues that might have caused it beeing stuck at checkWaiting.
On a system I tested it on I was able to input passwords normally when faillock locked the account. And I was also able to reset faillock from a different tty.

The Invalid Key down logs are currently a problem when switching to a different tty.
Then hyprlock looses focus and the key up event is not getting registered leading to those Logs. Unrelated to this PR.

Regarding displaying if the account is locked:
Looked into it a bit and its not that easy to check if your account has been locked via faillock_pam. But you can use a command like.

test "$(faillock --user $my_user | wc -l)" -ge "$max_attempts_plus_two" && echo "Locked"`

@bvr-yr
Copy link
Contributor

bvr-yr commented Mar 22, 2024

yeah can confirm now works correctly, unlocking by sleep 60 && faillock --reset, even after switching to another tty and reset as root I was able to type and unlock, although Invalid key down logged anyway
nice!

@PaideiaDilemma
Copy link
Collaborator Author

closed by accident xD.
Tested some 2fa stuff today. Works fine after fixing some stuff I overlooked.

hyprlock_2fa.mp4

@PaideiaDilemma PaideiaDilemma marked this pull request as ready for review March 22, 2024 12:37
@niksingh710
Copy link

closed by accident xD. Tested some 2fa stuff today. Works fine after fixing some stuff I overlooked.
hyprlock_2fa.mp4

how you configured this 2fA by using pam can you give some insight or direct me to the docs?

@PaideiaDilemma
Copy link
Collaborator Author

This is google-authenticator-libpam.
The repo gives some instructions to set it up.

@PaideiaDilemma PaideiaDilemma force-pushed the full-pam-conversation branch from 690cf4e to 9cec507 Compare March 26, 2024 20:29
@PaideiaDilemma
Copy link
Collaborator Author

Idk the Prompt being empty might be related to a weird pam config. Should not happen normally, since we are not setting a new placeholder when the prompt text is empty.

I think it is ready @vaxerski

Will make a wiki mr later today!

@vaxerski
Copy link
Member

vaxerski commented Apr 8, 2024

alr wiki mr then

@PaideiaDilemma
Copy link
Collaborator Author

@vaxerski done

@PaideiaDilemma
Copy link
Collaborator Author

Nevermind I just reproduced the empty placeholder and noticed I did some dumb stuff. let me check that again. 😅

@vaxerski
Copy link
Member

vaxerski commented Apr 8, 2024

tag me when done then

@PaideiaDilemma
Copy link
Collaborator Author

Alright @vaxerski ready for review

Reason I needed to revision some stuff is that I noticed that not restarting the authentication immediately on a failure was dump. Now m_bDisplayFailText handles how long the error message is displayed and that allows us to just block the input as long the auth thread is not waiting for input. That simplified some stuff that was ugly before.

I tested multiple authentication stages, as well as the weird su pam config on fedora.

@5ysk3y
Copy link

5ysk3y commented Apr 10, 2024

Edge case, but would it also be possible to allow the setting of the path for the PAM module directory that is checked here or does that have to be hardcoded? I noticed from your wiki PR: If the module isn't found in /etc/pam.d, "su" will be used as a fallback - I run on NixOS and it wont store certain PAM modules in that particular directory, e.g. if I want to use u2f auth (yubikey), as its an extra package:

$ find / -name 'pam_u2f.so' 2>/dev/null
/nix/store/83rbp1sxjhpg9gdlzclnjsng5a0n7cq9-system-path/lib/security/pam_u2f.so
/nix/store/azg8kfwr70fw7x4fisidkilazai2naw0-system-path/lib/security/pam_u2f.so
/nix/store/gj8913xflvfms77df6y3cp5fv39q94hp-system-path/lib/security/pam_u2f.so

If I declare pam_module as pam_u2f.so I just get this from Hyprlock because /etc/pam.d doesnt have the module:

[ERR] Pam module "pam_u2f.so" not found! Falling back to "su"

If we can delcare the path then when the nix module is updated the path can also be set more in a more nix-friendly manner through that? Not sure if that would be in scopre for this PR.

@PaideiaDilemma
Copy link
Collaborator Author

When you add a pam configuration via NixOs, it gets added to /etc/pam.d.
pam_u2f.so is a shared lib that can be used within a pam configuration. please look at nixos option search for pam

@5ysk3y
Copy link

5ysk3y commented Apr 10, 2024

When you add a pam configuration via NixOs, it gets added to /etc/pam.d. pam_u2f.so is a shared lib that can be used within a pam configuration. please look at nixos option search for pam

You're better than me ;) Let me take a look, nice one.

@vaxerski vaxerski merged commit 883fbdf into hyprwm:main Apr 10, 2024
1 check passed
@vaxerski
Copy link
Member

top

@vaxerski
Copy link
Member

@PaideiaDilemma can I get a wiki mr for the auth opt

@PaideiaDilemma
Copy link
Collaborator Author

Here @vaxerski hyprwm/hyprland-wiki#582

@DiskyToy
Copy link

DiskyToy commented Apr 11, 2024

I'm not 100% sure if this is the correct place to address this, but i have been following this PR quite some time and am happy to see it is finally happening. Thanks in advance!!

I just updated hyprlock and I am using my own pam-module (#209) and noticed the following...
d9a6229 removes the check if the /etc/pam.d/hyprlock exists and should install it anyways.
It did replace the /etc/pam.d/hyprlock in the previous versions, but with the current one, it installs into /usr/local/etc/pam.d/hyprlock.

I wanted to ask if this is expected behaviour because of this PR and the option to specify your own pam_module in config?
If so, feel free to ignore my concerns.

sudo cmake --install
Please touch the device.
-- Install configuration: "Release"
-- Installing: /usr/local/bin/hyprlock
-- Installing: /usr/local/etc/pam.d/hyprlock

@DiskyToy
Copy link

DiskyToy commented Apr 11, 2024

I also noticed something else because of https://github.com/hyprwm/hyprlock/pull/205/files/43b13bfb71899f73dba80567e5dae8216f21cdb1#r1559789603

I'm not 100% sure if this is the root cause, but it's my best guess...
Since we are starting the authentication on start, there is a possibility to lock the current user, depending on the pam_module/pam config they are using.

I am using the following pam_module:

auth	required	pam_fprintd.so
auth	required	pam_u2f.so cue origin=pam://Kanto appid=pam://Kanto
account	include		system-auth
session	include		system-auth

and when I lock my screen, the fingerprint starts to expect "input" and times out after x seconds. After it times out, it starts to query my yubikey (u2f) if present, which then also times out (in a scenario where I lock my screen and leave the yubikey plugged in).
After this, I have 1 failed authentication, but it immediately starts another cycle.
After 3 unsuccessfull cycles, my user would get locked out for x minutes.

@Mikilio
Copy link

Mikilio commented Apr 11, 2024

Apparently there it is a known issue with pam_fprintd.so that you can't disable the timeout. There is a PR for that.

For now, there should be a mechanism to test user presence before starting auth. Something like press any key to unlock. Something that can easily be added and removed via config.

@PaideiaDilemma PaideiaDilemma deleted the full-pam-conversation branch April 11, 2024 11:56
@bvr-yr
Copy link
Contributor

bvr-yr commented May 2, 2024

this keeps troubling me, I was getting faillocks pretty often cause I use hyprlock with grace when smth is playing.
Personally I just added faillock --reset since it anyway launched through script, but still

I tried to dig a bit and it seems like pam_end never happens on grace end, as well as on kill -USR1, although it looks like it should.
But again, with conv returning PAM_CONV_ERR which will fail anyway I suppose? (Ok for USR1 but not for grace unlock I think)
Can you clarify please, that's a culprit or something wrong with pam on me end?

@PaideiaDilemma
Copy link
Collaborator Author

I do not understand why faillock would be involved at all, when a grace unlock happens.
When grace happens, then the main loop calls g_pAuth->terminate();

Can you open an issue for that?
Sadly I currently don't have a lot of time, but I will try to look into it if I could reproduce it.
I also want to bring to make faillock displayable in hyprlock, like suggested in this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

pam_conversation not being handled properly
8 participants