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.

Sign up for GitHub

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

Jump to bottom

nixos/pam/u2f: implement RFC0042 #276106

Merged
merged 1 commit into from
Jul 5, 2024
Merged

nixos/pam/u2f: implement RFC0042 #276106

merged 1 commit into from
Jul 5, 2024

Conversation

9ary
Copy link
Contributor

@9ary 9ary commented Dec 22, 2023 •
edited
Loading

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

  • 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.05 Release Notes (or backporting 23.05 and 23.11 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.

2xsaiko
2xsaiko suggested changes Dec 22, 2023
View reviewed changes
Copy link
Contributor

@2xsaiko 2xsaiko left a comment

Choose a reason for hiding this comment

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

Nice.

nixos/modules/security/pam.nix Outdated Show resolved Hide resolved
@2xsaiko
Copy link
Contributor

2xsaiko commented Dec 22, 2023

You'll have to update the test, use this to run it: nix-build nixos/tests/pam/pam-u2f.nix

Looks like that's the only usage of this module in nixos though, so the rest should be good.

@9ary
Copy link
Contributor Author

9ary commented Dec 23, 2023

Test is fixed.

Side note: first time running one, it's really cool.

2xsaiko
2xsaiko approved these changes Dec 23, 2023
View reviewed changes
Copy link
Contributor

@2xsaiko 2xsaiko left a 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.

@2xsaiko 2xsaiko requested a review from Majiir December 23, 2023 00:51
Majiir
Majiir reviewed Dec 23, 2023
View reviewed changes
Copy link
Contributor

@Majiir Majiir left a 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.

nixos/doc/manual/release-notes/rl-2405.section.md Outdated Show resolved Hide resolved
nixos/modules/security/pam.nix Outdated Show resolved Hide resolved
nixos/modules/security/pam.nix Outdated Show resolved Hide resolved
@nixos-discourse
Copy link

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

@wegank
Copy link
Member

wegank commented Feb 4, 2024

Merge conflict, please rebase.

@9ary
Copy link
Contributor Author

9ary commented Feb 11, 2024

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.

@9ary
Copy link
Contributor Author

9ary commented Mar 6, 2024

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.

@9ary
nixos/pam/u2f: implement RFC0042
fcccfb9 
This module has a lot of options, so it's a good candidate for freeform
settings.
@9ary
Copy link
Contributor Author

9ary commented Jul 5, 2024

Rebased to fix merge conflict on changelog. Confirmed working.

@9ary
Copy link
Contributor Author

9ary commented Jul 5, 2024

@wegank is anything else needed to get this merged?

@wegank
Copy link
Member

wegank commented Jul 5, 2024

@ofborg test pam-u2f

@wegank wegank merged commit 93244d1 into NixOS:master Jul 5, 2024
27 checks passed
@9ary
Copy link
Contributor Author

9ary commented Jul 5, 2024

Thanks!

jtojnar added a commit to jtojnar/nixfiles that referenced this pull request Jul 9, 2024
@jtojnar
flake.lock: Update
c6e93ca 
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)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Majiir Majiir Majiir approved these changes

@2xsaiko 2xsaiko 2xsaiko approved these changes

@wegank wegank wegank approved these changes

Assignees
No one assigned
Labels
6.topic: nixos 8.has: changelog 8.has: documentation 8.has: module (update) 10.rebuild-darwin: 0 10.rebuild-linux: 1-10
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@9ary @2xsaiko @nixos-discourse @wegank @Majiir @delroth