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

Adding a pam configuration file #115

Merged
merged 2 commits into from
Mar 1, 2024
Merged

Adding a pam configuration file #115

merged 2 commits into from
Mar 1, 2024

Conversation

alba4k
Copy link
Contributor

@alba4k alba4k commented Mar 1, 2024

This pull request would make hyprlock use it's own pam configuration file instead of the su one

This allows the user to include random pam services into it without touching stuff required by other programs.

By default, this just includes the /etc/pam.d/login configuration, which contains, by default:

#%PAM-1.0
auth       required     pam_securetty.so
auth       requisite    pam_nologin.so
auth       include      system-local-login
account    include      system-local-login
session    include      system-local-login
password   include      system-local-login

Basically just asks for a password.

Creating a custom pam configuration is exactly what #4 is asking for, and that issue is therefore fixed by this PR

On a sidenote, I'm not really sure about this line in CMakeLists.txt:

# [...]
install(FILES "pam/hyprlock" DESTINATION "/etc/pam.d")

Is this the best way to do that? I'm not sure if using an absolute path is the best approach, but I didn't find anything better. Of course it can still be modified by using DESTDIR

@vaxerski
Copy link
Member

vaxerski commented Mar 1, 2024

all good just need @fufexan to fix nix nix fix pls fix nix nix fix

@alba4k
Copy link
Contributor Author

alba4k commented Mar 1, 2024

Was about to ping but wasn't 100% completely sure if it was just me being me

@vaxerski
Copy link
Member

vaxerski commented Mar 1, 2024

also I think trying su if regular fails would be a nice touch too.

Don't lock the user out :(

@alba4k
Copy link
Contributor Author

alba4k commented Mar 1, 2024

login already includes a ton of stuff, that file not working would mean the user is already locked out of his system anyway (since /bin/login wouldn't work), ergo there would be bigger issues than hyprlock not unlocking.

This is the same approach swaylock and i3lock use.

@alba4k
Copy link
Contributor Author

alba4k commented Mar 1, 2024

On a sidenote, I managed to SIGSEGV hyprlock using my modified login file:

#%PAM-1.0
auth       sufficient   pam_unix.so try_first_pass likeauth nullok
auth       sufficient   pam_fprintd.so
auth       required     pam_securetty.so
auth       requisite    pam_nologin.so
auth       include      system-local-login
account    include      system-local-login
session    include      system-local-login
password   include      system-local-login

To do this I would press enter on an empty password, then the fingerprint auth will start. Once fprintd times out tho, hyprlock just terminates. Swaylock (yeah, sorry, that's the obvious comparison) will just restart the authentication. Should I open a different issue for that?

Again, this is not related to this PR in particular but moreso to using fprintd

CMakeLists.txt Outdated Show resolved Hide resolved
@fufexan
Copy link
Member

fufexan commented Mar 1, 2024

@alba4k what about CMAKE_INSTALL_FULL_SYSCONFDIR?
In any case we can include -DCMAKE_INSTALL_SYSCONFDIR=/etc in the readme build command, but it should already exist either way.

@alba4k
Copy link
Contributor Author

alba4k commented Mar 1, 2024

Sorry for deleting, added it as a comment to the suggestion. CMAKE_INSTALL_FULL_SYSCONFDIR doesn't work either, should I just add it to the README?

Copy link
Member

@fufexan fufexan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@fufexan fufexan merged commit fa2a875 into hyprwm:main Mar 1, 2024
1 check passed
@vaxerski
Copy link
Member

vaxerski commented Mar 2, 2024

login already includes a ton of stuff, that file not working would mean the user is already locked out of his system anyway (since /bin/login wouldn't work), ergo there would be bigger issues than hyprlock not unlocking.

This is the same approach swaylock and i3lock use.

My logic was more like someone forgor 💀 to copy the file.

@alba4k
Copy link
Contributor Author

alba4k commented Mar 2, 2024

oh so in hyprlock, in case the file is not found. That makes sense.

@PaideiaDilemma
Copy link
Collaborator

As a nix user I had to do "security.pam.services.hyprlock = {};" for this to work.
Maybe that should be mentioned in the hm module like swaylocks hm module does.

@alba4k
Copy link
Contributor Author

alba4k commented Mar 2, 2024

@fufexan

@vaxerski
Copy link
Member

vaxerski commented Mar 2, 2024

it's your fault for not fallbacking to su

@alba4k
Copy link
Contributor Author

alba4k commented Mar 2, 2024

also I think trying su if regular fails would be a nice touch too.

@vaxerski looking into that right now. pam_start seems to return 0 regardless of whether the file exists or not, should I just manually check?

I'd just leave everything as-is tbh, as that is what every other program would usually do (again, take swaylock as an example).

@vaxerski
Copy link
Member

vaxerski commented Mar 2, 2024

why not try regardless if any fail

@fufexan
Copy link
Member

fufexan commented Mar 2, 2024

@fufexan

ye?

@alba4k
Copy link
Contributor Author

alba4k commented Mar 2, 2024

ye?

#115 (comment)
Not familiar with nix stuff

@alba4k
Copy link
Contributor Author

alba4k commented Mar 2, 2024

why not try regardless if any

Because I'd have no idea how to implement that in the existing code, having never worked with pam myself :P

@fufexan
Copy link
Member

fufexan commented Mar 2, 2024

@alba4k done in 97548ec.

@alba4k
Copy link
Contributor Author

alba4k commented Mar 2, 2024

ping @PaideiaDilemma not me ahah

@vaxerski
Copy link
Member

vaxerski commented Mar 3, 2024

I swear to god amma revert this, I added the pam file and yet STILL can't login

@vaxerski
Copy link
Member

vaxerski commented Mar 3, 2024

why did you make the pam file 600 ffs

@vaxerski
Copy link
Member

vaxerski commented Mar 3, 2024

added su fallback in f9fe60c

@Jackaed
Copy link

Jackaed commented Apr 24, 2024

Can we make the su fallback optional @vaxerski? I'm trying to configure hyprlock to not try and use fprintd, despite the fact that my su does trigger fprintd auth, but even if I remove it from the hyprlock PAM config, if I put in an incorrect password, it still calls fprintd and then I have to wait for that to timeout before I can put a new attempt in.

I'm not convinced that having su as a fallback by default is a good idea in general, if people need that functionality they can just include their su pam file within the hyprlock pam file. I'm yet to see any similar projects that behave similarly to hyprlock in this regard.

@alba4k
Copy link
Contributor Author

alba4k commented Apr 24, 2024

When you input a wrong password you get prompted for fingerprint but that likely has nothing to do with your su, rather with your hyprlock config

I believe su is only used when hyprlock can not be used, not when it fails to authenticate

@Jackaed
Copy link

Jackaed commented Apr 24, 2024

I believed that to be the case as well, but fprint is not in my PAM config in /etc/pam.d/hyprlock, and removing fprint from my su pam configuration causes the system to no longer unlock with my fingerprint. I get this output from hyprlock when inputting an incorrect password:

[ERR] auth: Authentication failed for hyprlock
[ERR] auth: Authentication failed for su

Neither of these errors occur if I just input the correct password. I haven't looked at the code, but based on this, it seems like it uses su as a fallback if the unlock attempt wasn't successful.

Fortunately I have root user disabled anyways so I can just remove fingerprint auth from su and keep it on sudo, so it's not actually an issue for my use case, but I still think this is a questionable way of doing things.

NOTE: I'm only on version 2, this may work differently on version 3, v3 has only just been put in nixpkgs so I'll be able to test this tomorrow probably.

@PaideiaDilemma
Copy link
Collaborator

Guys the latest git version does not have a su fallback anymore. You can specify the pam module used by hyprlock in the config. Check the wiki :)

@alba4k
Copy link
Contributor Author

alba4k commented Apr 25, 2024

su is still used as fallback btw

@bvr-yr
Copy link
Contributor

bvr-yr commented Apr 26, 2024

su is still used as fallback btw

really? no fallback for me, I'm getting faillocks like intended.
What is not intended, faillock also happens with grace enebled. @PaideiaDilemma that's a bug or smth wrong on my end with pam?

@PaideiaDilemma
Copy link
Collaborator

su is still used as fallback btw

It is, but not at time of authentication, but at startup.
From the wiki:

If the module isn’t found in /etc/pam.d, “su” will be used as a fallback

flexiondotorg added a commit to wimpysworld/nix-config that referenced this pull request Aug 24, 2024
flexiondotorg added a commit to wimpysworld/nix-config that referenced this pull request Aug 24, 2024
flexiondotorg added a commit to wimpysworld/nix-config that referenced this pull request Aug 27, 2024
* fix: switch existing workspaces

* feat: add wlogout

* fix: waybar battery states now have "good"

* feat: enable udiskie service and indicator

* refactor: simplify bluetoothToggle

* refactor: drop gnome-settings from dbus packages

* fix: enable hyprlock pam

hyprwm/hyprlock#115 (comment)

* refactor: improve hyprlock

* feat: replace mako with swaync

* style: make waybar appearance more cohesive

* fix: correct path to swappy config and sane font defaults

* chore: drop gBar

* chore: drop walker

* chore: drop mako

* refactor: put all hyprland components in discrete directories

* feat: enable udevil/devmon automounting

* chore: update flake.lock
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.

6 participants