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: security.pam: Rename enableU2F and add opts #40455

Closed
wants to merge 1 commit into from

Conversation

ArdaXi
Copy link
Contributor

@ArdaXi ArdaXi commented May 13, 2018

Renamed security.pam.enableU2F to security.pam.u2f.enable
Added security.pam.u2f.options

Motivation for this change

The old setup makes it impossible to provide options to the U2F PAM module, even though these options provide a lot of extra possibilities in using the module (to hardcode U2F keys in /etc/, for example).

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option build-use-sandbox in nix.conf on non-NixOS)
  • 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 nox --run "nox-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Fits CONTRIBUTING.md.

Renamed security.pam.enableU2F to security.pam.u2f.enable
Added security.pam.u2f.options
@bjornfor
Copy link
Contributor

Have you considered using type "listOf str"? (It allows merging multiple definitions.)

@ArdaXi
Copy link
Contributor Author

ArdaXi commented May 14, 2018

Merging is something I'd be reluctant to support because PAM is a rather sensitive part of the system and if you override your own options it's hard to tell what will happen and getting an error seems like a good thing to me. I'm open to feedback on this, of course.

Ideally every PAM module would take a set of options instead, but that would be a pretty significant rework and something I would want to get feedback on first.

@@ -250,6 +250,9 @@ with lib;
(mkRenamedOptionModule [ "programs" "info" "enable" ] [ "documentation" "info" "enable" ])
(mkRenamedOptionModule [ "programs" "man" "enable" ] [ "documentation" "man" "enable" ])

# PAM
(mkRenamedOptionModule [ "security" "pam" "enableU2F" ] [ "security" "pam" "u2f" "enable" ])

Copy link
Member

Choose a reason for hiding this comment

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

Isn't this missing the u2fAuth -> u2f.enable change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's defined on every (freeform) service. I don't believe the rename module supports a usecase where something like security.pam.services.<name?>.u2fAuth is renamed.

Copy link
Member

Choose a reason for hiding this comment

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

Ah right, it won't work here probably, but maybe it'll work by adding an imports = [(mkRenamed ...)] at line 12 in pam.nix

@infinisil
Copy link
Member

Ping?

Copy link
Member

@kalbasit kalbasit left a comment

Choose a reason for hiding this comment

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

#11886 has the options detailed, although it's missing the mkRenamedOptionModule. Can we somehow merge both PRs?

@kalbasit
Copy link
Member

@ArdaXi can you port the options from #11886 and add @spinus as a Co-Authored-By in your commit? Thank you!

@kalbasit
Copy link
Member

closing this PR in favor of #54756.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants