-
-
Notifications
You must be signed in to change notification settings - Fork 13.4k
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/pam/u2f: implement RFC0042 #276106
nixos/pam/u2f: implement RFC0042 #276106
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice.
You'll have to update the test, use this to run it: Looks like that's the only usage of this module in nixos though, so the rest should be good. |
Test is fixed. Side note: first time running one, it's really cool. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They are!
Ran the test, looks good.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, thank you for doing this! Just a few nitpicks.
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/prs-already-reviewed/2617/1336 |
Merge conflict, please rebase. |
Sorry for the delay, rebase is done but I'm unable to properly test it at the moment. Diff looks fine though. If someone else can verify that it works that'd be helpful. |
Rebased again. I still can't test against a real config since I run stable, but the test passes. The diff looks pretty much identical to the pre-rebase version (and not much has changed in pam.nix anyway) so I have no reason to believe anything's wrong. |
This module has a lot of options, so it's a good candidate for freeform settings.
Rebased to fix merge conflict on changelog. Confirmed working. |
@wegank is anything else needed to get this merged? |
@ofborg test pam-u2f |
Thanks! |
Flake lock file updates: • Updated input 'agenix': 'github:ryantm/agenix/3a56735779db467538fb2e577eda28a9daacaca6' (2024-06-14) → 'github:ryantm/agenix/de96bd907d5fbc3b14fc33ad37d1b9a3cb15edc6' (2024-07-09) • Updated input 'home-manager': 'github:nix-community/home-manager/59ce796b2563e19821361abbe2067c3bb4143a7d' (2024-07-01) → 'github:nix-community/home-manager/2fb5c1e0a17bc6059fa09dc411a43d75f35bb192' (2024-07-08) • Updated input 'nixpkgs': 'github:NixOS/nixpkgs/7f993cdf26ccef564eabf31fdb40d140821e12bc' (2024-07-01) → 'github:nixos/nixpkgs/655a58a72a6601292512670343087c2d75d859c1' (2024-07-08) - large `gnome` extraction (NixOS/nixpkgs#319659) - PAM module switched `u2f` to RFC42 settings (NixOS/nixpkgs#276106) - Proper GDM fingerprint support (NixOS/nixpkgs#324347)
This change lays a trap for users migrating their configurations. It's easy not to notice that when NixOS will silently ignore this configuration error. However, the next time the user attempts to log in, they will find that they can't use U2F. I would suggest adding an error case specific to cc @9ary @NixOS/nixos-release-managers |
Accepting freeform configuration with little to no validation (in nix code) is kind of the point of RFC 42. Let's see if it's possible to do a runtime check during the build, otherwise a simple assertion for this particular case should be enough. |
Description of changes
All options specific to this module were moved to
.settings
, which now accepts freeform attributes.I needed options that weren't previously exposed, and since the man page lists a lot of options I figured this would be the best approach.
Unsure if tests need to be updated, I'll have a look.
Things done
nix.conf
? (See Nix manual)sandbox = relaxed
sandbox = true
nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD"
. Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/
)Add a 👍 reaction to pull requests you find important.