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

nixos/gdm: allow logging in via fingerprint #321591

Merged
merged 1 commit into from
Jun 26, 2024

Conversation

JohnRTitor
Copy link
Contributor

@JohnRTitor JohnRTitor commented Jun 21, 2024

Description of changes

Lets GDM/GNOME users to login via fingerprint if fprintd service is enabled.

Cherry-picked from #306338. Have not tested but should work fine. Originally borrowed from https://gitlab.gnome.org/GNOME/gdm/-/blob/main/data/pam-arch/gdm-fingerprint.pam

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 24.11 Release Notes (or backporting 23.11 and 24.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

@github-actions github-actions bot added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/` labels Jun 21, 2024
@JohnRTitor JohnRTitor force-pushed the gdm-fingerprint-auth branch from b3e5496 to 9d41fe6 Compare June 21, 2024 20:13
@JohnRTitor JohnRTitor requested a review from a team June 21, 2024 20:13
@JohnRTitor
Copy link
Contributor Author

Please have a look as well @LeSuisse, changes are mostly same as amaxine's PR

I removed manual addition of gnome-keyring.so so to go with security.pam.services.gdm-fingerprint.enableGnomeKeyring = true;

@JohnRTitor JohnRTitor added the 6.topic: GNOME GNOME desktop environment and its underlying platform label Jun 21, 2024
@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 labels Jun 21, 2024
@JohnRTitor JohnRTitor changed the title nixos/gdm: add fingerprint pam rules nixos/gdm: allow logging in via fingerprint Jun 22, 2024
@github-actions github-actions bot removed the 6.topic: GNOME GNOME desktop environment and its underlying platform label Jun 22, 2024
Copy link
Contributor

@Pandapip1 Pandapip1 left a comment

Choose a reason for hiding this comment

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

Diff LGTM. Have not tested myself.

@Pandapip1 Pandapip1 added the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Jun 23, 2024
@JohnRTitor
Copy link
Contributor Author

Thanks, I will keep it open for a while for others to provide feedback on it.

@JohnRTitor JohnRTitor requested review from ElvishJerricco and removed request for dawidd6 June 24, 2024 19:15
@JohnRTitor JohnRTitor merged commit 44f79e5 into NixOS:master Jun 26, 2024
29 checks passed
@JohnRTitor JohnRTitor deleted the gdm-fingerprint-auth branch June 26, 2024 17:38
@schuelermine
Copy link
Contributor

Wanted to note that fingerprint login was already possible before, it just didn’t work well (fingerprint would immediately fail, then reactivate and block all password login attempts until it failed again)

@schuelermine
Copy link
Contributor

I’m using a Framework Laptop 16 and following this patch fingerprint login has become more inconsistent than before.

@JohnRTitor
Copy link
Contributor Author

Could be related to

login.fprintAuth = mkIf config.services.fprintd.enable false;
, could you mkForce it to true and tell us the result?

Fingerprint support on NixOS needs wider testing. Would love if you could help.

@schuelermine
Copy link
Contributor

doing it now

@schuelermine
Copy link
Contributor

schuelermine commented Jul 2, 2024

doing this prevents me from logging in via password, and also GDM repeatedly shakes the password box and displays an error about fingerprint login not working (but when I touch the finger I can log in) (note I did not reboot)

@schuelermine
Copy link
Contributor

note: that was the behavior on the lock screen
it was also the behavior before this force (but after this PR) but it would refuse fingerprint and accept password

@schuelermine
Copy link
Contributor

I will test a fresh login later

auth required ${pkgs.fprintd}/lib/security/pam_fprintd.so
auth optional pam_permit.so
auth required pam_env.so
auth [success=ok default=1] ${pkgs.gnome.gdm}/lib/security/pam_gdm.so
Copy link
Member

@jtojnar jtojnar Jul 2, 2024

Choose a reason for hiding this comment

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

I am seeing the following in the journal:

gdm-fingerprint][1033624]: PAM bad jump in stack

Probably caused by the default=1, which should skip one auth instruction when pam_gdm gives anything but success, there are no further authinstructions.

Copy link
Member

Choose a reason for hiding this comment

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

security.pam.services.gdm-fingerprint.enableGnomeKeyring = true; does not appear to do anything for me.

But I am now pretty sure invoking it manually as in b3e5496 would be fine.

If we want to make it conditional, we would need to replace the one after the jump with the pam_permit.so line when disabled.

auth requisite pam_nologin.so
auth requisite pam_faillock.so preauth
auth required ${pkgs.fprintd}/lib/security/pam_fprintd.so
auth optional pam_permit.so
Copy link
Member

Choose a reason for hiding this comment

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

I started looking into pam two weeks ago and according to my current understanding, this line does not do anything.

@jtojnar
Copy link
Member

jtojnar commented Jul 2, 2024

Could be related to

login.fprintAuth = mkIf config.services.fprintd.enable false;

, could you mkForce it to true and tell us the result?

I do not think that is desirable.

Not exactly sure what would happen but at best, I would expect it to lead to the same behaviour as before (user is not prompted for password until fingerprint auth fails). And at worst, two pam modules would ask for the fingerprint at the same time, which sounds like a fun way to get new bugs.

@jtojnar
Copy link
Member

jtojnar commented Jul 3, 2024

Opened #324347 with a fix, also adding gnome-keyring.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/` 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 12.approvals: 1 This PR was reviewed and approved by one reputable person
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

5 participants