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/fprintd: option to inhibit PAM rule creation #107320

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

kneitinger
Copy link
Contributor

@kneitinger kneitinger commented Dec 21, 2020

Motivation for this change

Currently, security.pam.services.<name>.fprintAuth's default value is the value of services.fprintd.enable.

Since many fingerprint readers are located on laptops in such a way that they may be inaccessible when the laptop lid is closed, it is tedious to undo this default for commonly used services such as sudo, while keeping occasional services like login or screen lockers.

This commit adds the services.fprintd.global (default: true), that prevents fprintd PAM rules from being created for every service. By having a default value of true, every user's current configs will behave identically to before if they choose to not set this option.

Example:

# fprintd is installed, and *can* be used, but isn't by default
$ grep fprint /etc/nixos/configuration.nix
services.fprintd.enable = true;
services.fprintd.global = false;
$ sudo ls
[sudo] password for kyle:

# fprintd is installed, disabled for services by default, but enabled for sudo
$ grep fprint /etc/nixos/configuration.nix
services.fprintd.enable = true;
services.fprintd.global = false;
security.pam.services.sudo.fprintAuth = true;
$ sudo ls
<fingerprint prompt>

# current configuration behavior is unaffected by this change
$ grep fprint /etc/nixos/configuration.nix
services.fprintd.enable = true;
$ sudo ls
<fingerprint prompt>
Things done
  • Added global (for lack of a more creative name...suggestions?) option to services.fprint
  • Cleaned up formatting of services/fprintd.nix
  • Corrected bug with package override only being honored in one reference.

  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

Currently, `security.pam.services.<name>.fprintAuth`'s default value
is the value of `services.fprintd.enable`. Since many fingerprint
readers are located on laptops in such a way that they may be
inaccessible when the laptop lid is closed, it is tedious to undo this
default for commonly used services such as `sudo`, while keeping
occasional services like login or screen lockers. This commit adds the
`services.fprintd.global` (default: `true`), that prevents fprintd
PAM rules from being created for every service.  By having a default
value of `true`, every user's current configs will behave identically
to before if they choose to not set this option.
@ofborg ofborg 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/` 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 10.rebuild-linux: 1 labels Dec 21, 2020
@doronbehar
Copy link
Contributor

Related: #105319 .

@SuperSandro2000 SuperSandro2000 added the 2.status: merge conflict This PR has merge conflicts with the target branch label Jan 18, 2021
@stale
Copy link

stale bot commented Jul 21, 2021

I marked this as stale due to inactivity. → More info

@stale stale bot added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jul 21, 2021
@ngasull
Copy link

ngasull commented Jan 19, 2023

This PR is a great initiative. There can be many cases where fprintd could be installed without enabling it globally. One may be only interested in installing the drivers without enabling auth for every service.

For instance in my case :

  • I enabled fprintd for Webauthn (even though it didn't happen to work :D)
  • I faced weird bugs on login with SDDM (requires password + fingerprint but displays no message about fingerprint, thus freezing the UI)
  • sudo asked me for fingerprint, which was surprising but I didn't really ask.

Please note that the mentioned related PR has been closed because too complex, whereas this one is very simple, selective, and backwards compatible (assuming fixed conflicts). Any chance to give it a look? :)

@stale stale bot removed the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jan 19, 2023
@wegank wegank added the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Sep 7, 2023
@wegank wegank added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Mar 19, 2024
@wegank wegank marked this pull request as draft March 20, 2024 15:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2.status: merge conflict This PR has merge conflicts with the target branch 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md 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 10.rebuild-linux: 1 12.approvals: 1 This PR was reviewed and approved by one reputable person
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants