implement fscrypt for PAM #101604

NeoTheFox · 2020-10-24T22:16:44Z

Adresses #92352

Tested on a running NixOS 20.90

andir

Thanks for the contribution. Only had a brief look at this would like you to take a moment to reformat the changes in this PR.

You seem to be mixing tabs and spaces whereas we are (mostly) only using spaces in this repository. Usually these settings can be automatically applied by your editor (so you can keep on pressing TAB but still emit the correct number of spaces) by telling your to read editorconfig files. There are a lot of plugins available for most of the common editors: https://editorconfig.org/#download

Furthermore you can probably squash all of the changes into one commit that follows our commit style guide.
In your case this is likely something like this:

nixos/security/pam: support fscrypt

This PR adds support for doing XYZ using fscrypt in combination with PAM. This is desireable because reasons ….

nixos/modules/security/pam.nix

This PR enables fscrypt pam module from fscrypt package. It allows the end users to use their passphrase to unlock encrypted directories on supported filesystems while logging in.

kamentomov

@NeoTheFox I would like to add some changes to this Pull Request (already tested). Alternateively I can comment here about them. If you prefer the former could you please grant me access to your repo? You need to check the box in the pull request that says “Allow edits from maintainers.”

NeoTheFox · 2021-02-11T09:37:01Z

The box was checked! I'm ok with any changes as long as fscrypt gets implemented

kamentomov · 2021-02-11T13:08:01Z

The box was checked! I'm ok with any changes as long as fscrypt gets implemented

I continue not to have rights. Could you please allow access to contributors in your fork so that I try to commit in NeoTheFox:master?

primeos · 2021-02-11T13:48:02Z

I continue not to have rights.

@kamentomov “Allow edits from maintainers.” won't work for you because you need commit access to Nixpkgs. I'd probably be better to suggest changes via the GH UI or link to your commit / post a patch so that @NeoTheFox could cherry-pick/apply it.

But in any case I'm not sure if this PR can get merged anytime soon (I'd recommend to find/ping some more appropriate reviewers).
The thing's I'm a bit concerned about:

fscrypt-experimental is named that way because there isn't a stable release yet: https://github.com/google/fscrypt#note-about-stability
There are different options to enable the PAM module and they have different trade-offs: https://github.com/google/fscrypt#enabling-the-pam-module-on-other-linux-distros
- I.e. we'd need to decide what's the best default for Nixpkgs.
I'm not sure if this belongs to nixos/modules/security/pam.nix (there's already a comment that "Most of these should be moved to specific modules.")

kamentomov · 2021-02-11T14:30:52Z

@primeos Thanks for commenting. This looked so dead.

@kamentomov “Allow edits from maintainers.” won't work for you because you need commit access to Nixpkgs. I'd probably be better to suggest changes via the GH UI or link to your commit / post a patch so that @NeoTheFox could cherry-pick/apply it.

I pushed to @NeoTheFox's repo and that worked. @NeoTheFox thanks for graning me access.

But in any case I'm not sure if this PR can get merged anytime soon (I'd recommend to find/ping some more appropriate reviewers).

Could you please help us with this? I don't know who is qualified to review this.

The thing's I'm a bit concerned about:

fscrypt-experimental is named that way because there isn't a stable release yet: https://github.com/google/fscrypt#note-about-stability

Stragely the package is not experimental in other OSes. Nevertheless, experimental in the name seems good enough for me. I mean I don't think we shall do something additional except keeping the name as it is until they reach a non-experimental version. Please tell us if you have any furhter concerns?

There are different options to enable the PAM module and they have different trade-offs: https://github.com/google/fscrypt#enabling-the-pam-module-on-other-linux-distros

The currently used approach to enable fscrypt seems OK to me. It fits with the rest in pam.nix.

I.e. we'd need to decide what's the best default for Nixpkgs.

I'm not sure if this belongs to nixos/modules/security/pam.nix (there's already a comment that "Most of these should be moved to specific modules.")

Moving to specific modules is a larger task so shouldn't we fit to the current approach until there is a volunteer to do a refactoring?

NeoTheFox · 2021-02-11T14:47:39Z

I also think that this is not disruptive/overreaching. Since fscrypt-experimental exists in the repos and provides the required PAM module not having a way to enable it without resorting to hacks seems incomplete to me. After all this only adds a few lines of text to the config just to enable the functionality that's already there.

primeos · 2021-02-11T18:15:16Z

Could you please help us with this? I don't know who is qualified to review this.

In general I'd try to ping the users that contributed the most (or recently) to the file you're changing (listed here or via git shortlog -sne nixos/modules/security/pam.nix) and other active Nixpkgs reviewers (e.g. ping 1-4 potential reviewers and if there's no response withing a few days repeat that proces).
(Unfortunately the details are a bit more complicated... :o I guess we really should better document this process somewhere.)

I can try to review and help with the fscrypt stuff but I'm not comfortable merging this without another reviewer approving it.

Stragely the package is not experimental in other OSes.

Yeah, the name is pretty non-standard (this is my mistake because IIRC I packaged it pretty early and at that time there where some rough edges and I didn't want anyone to accidentally loose their data without knowing the risks).

The currently used approach to enable fscrypt seems OK to me. It fits with the rest in pam.nix.

Ok, I'll have a closer look at it.

Moving to specific modules is a larger task so shouldn't we fit to the current approach until there is a volunteer to do a refactoring?

This PR would of course only have to "move" the fscrypt part into a specific module (if that's possible without much effort). But maybe it's also fine in pam.nix.
(I'll try to give this a proper review but it might have to wait until tomorrow or even the weekend. Thanks for your fast response though!)

primeos

Sorry for the delay...
I added a few style nitpicks/improvements but overall the diff LGTM (though unfortunately I also realized how unfamiliar I'm with PAM and our NixOS module for it).

Edit: https://github.com/google/fscrypt/releases/tag/v0.3.0 is also relevant now.

primeos · 2021-02-17T16:31:42Z

nixos/modules/security/pam.nix

@@ -207,6 +207,14 @@ let
        '';
      };

+      enableFscrypt = mkOption {
+        default = config.security.pam.enableFscrypt;


It isn't clear to me why we need two options for this (e.g. config.security.pam.enableOTPW does the same but I don't immediately see an obvious reason for it / understand that part of the module).

Update: After looking at this a bit closer it seems like this part (pamOpts function, etc.) does the actual work to set/apply the config per PAM program/service and config.security.pam.enableFscrypt is used as a convenience option to easily enable it for all programs/services - in that case it might make sense to have both but security.pam.services.<name>.enableFscrypt is basically only useful to disable/blacklist it for some PAM programs/services or for "whitelisting" but then pkgs.fscrypt-experimental won't be installed (though it shouldn't be required anyway, only for the setup?)).

Update2: E.g. for eCryptfs only security.pam.enableEcryptfs is used, not the second per program/service option.
(And I'm also not sure if the NixOS approach of setting this for every PAM program/service by default makes that much sense - e.g. in the Arch Wiki it's only set for system-login and passwd: https://wiki.archlinux.org/index.php/Fscrypt#PAM_module)

Update3: It seems to me like the best approach to implement this would be default = false and set security.pam.services.$name.enableFscrypt = config.security.pam.enableFscrypt (the actual implementation would be a bit different) for the relevant services. But I'm not sure how realistic that approach currently is for NixOS.

primeos · 2021-02-17T16:33:32Z

nixos/modules/security/pam.nix

+          description = ''
+          Unlocks fscrypt-encrypted filesystems on login via PAM
+          '';


Suggested change

description = ''

Unlocks fscrypt-encrypted filesystems on login via PAM

'';

description = ''

Unlocks fscrypt-encrypted filesystems on login via PAM.

'';

Maybe we should also link to e.g. https://github.com/google/fscrypt/blob/master/README.md via the description?

primeos · 2021-02-17T16:39:32Z

nixos/modules/security/pam.nix

              ${optionalString cfg.pamMount
                "auth optional ${pkgs.pam_mount}/lib/security/pam_mount.so"}
+              ${optionalString cfg.enableFscrypt
+                "auth optional ${pkgs.fscrypt-experimental}/lib/security/pam_fscrypt.so"}


Suggested change

${optionalString cfg.pamMount

"auth optional ${pkgs.pam_mount}/lib/security/pam_mount.so"}

${optionalString cfg.enableFscrypt

"auth optional ${pkgs.fscrypt-experimental}/lib/security/pam_fscrypt.so"}

${optionalString cfg.enableFscrypt

"auth optional ${pkgs.fscrypt-experimental}/lib/security/pam_fscrypt.so"}

${optionalString cfg.pamMount

"auth optional ${pkgs.pam_mount}/lib/security/pam_mount.so"}

Nitpick but let's keep the order consistent with above.

primeos · 2021-02-17T16:44:26Z

nixos/modules/security/pam.nix

@@ -622,6 +637,8 @@ in

    security.pam.enableOTPW = mkEnableOption "the OTPW (one-time password) PAM module";

+    security.pam.enableFscrypt = mkEnableOption "fscrypt filesystem encryption PAM module";


Suggested change

security.pam.enableFscrypt = mkEnableOption "fscrypt filesystem encryption PAM module";

security.pam.enableFscrypt = mkEnableOption "the fscrypt filesystem encryption PAM module";

primeos · 2021-02-17T16:45:18Z

nixos/modules/security/pam.nix

@@ -851,6 +868,7 @@ in
      ++ optional config.services.sssd.enable pkgs.sssd
      ++ optionals config.krb5.enable [pam_krb5 pam_ccreds]
      ++ optionals config.security.pam.enableOTPW [ pkgs.otpw ]
+      ++ optionals config.security.pam.enableFscrypt [pkgs.fscrypt-experimental]


Suggested change

++ optionals config.security.pam.enableFscrypt [pkgs.fscrypt-experimental]

++ optionals config.security.pam.enableFscrypt [ pkgs.fscrypt-experimental ]

primeos · 2021-02-17T17:13:08Z

nixos/modules/security/pam.nix

@@ -886,6 +904,7 @@ in
        vlock = {};
        xlock = {};
        xscreensaver = {};
+        fscrypt = {};


Is that part actually required / what is the use-case here?
Update: https://github.com/google/fscrypt#allowing-fscrypt-to-check-your-login-passphrase explains it and we seem to have a restrictive /etc/pam.d/other. But I'm starting to get the feeling that our NixOS module for PAM could need a rewrite and uses a bit of a lazy approach (setting options of every PAM module/service instead of only the ones that need it).

primeos · 2021-02-17T17:46:30Z

nixos/modules/security/pam.nix

@@ -886,6 +904,7 @@ in
        vlock = {};
        xlock = {};
        xscreensaver = {};
+        fscrypt = {};


Suggested change

fscrypt = {};

fscrypt.text = "auth required pam_unix.so";

Maybe this would be better (https://github.com/google/fscrypt#allowing-fscrypt-to-check-your-login-passphrase)?

lorenz · 2021-09-23T00:45:03Z

@NeoTheFox Do you want to continue working on this? I'd like to see this merged and could take this over.

NeoTheFox · 2021-09-23T10:45:27Z

@lorenz feel free to take over, it's been a long time and I don't have a system to test it on anymore

stale · 2022-04-16T04:15:52Z

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

mrvik · 2022-11-03T06:08:39Z

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

Still important. Any updates on this?

lorenz · 2023-03-13T21:26:59Z

This has been superseded by #199587 which has since been merged and is available as of 22.11 as security.pam.enableFscrypt.

ofborg bot added 6.topic: nixos 8.has: module (update) 10.rebuild-darwin: 0 10.rebuild-linux: 1-10 labels Oct 24, 2020

andir requested changes Oct 25, 2020

View reviewed changes

nixos/modules/security/pam.nix Outdated Show resolved Hide resolved

NeoTheFox requested a review from andir October 25, 2020 15:11

nixos/security/pam: support fscrypt

e8a13c0

This PR enables fscrypt pam module from fscrypt package. It allows the end users to use their passphrase to unlock encrypted directories on supported filesystems while logging in.

NeoTheFox force-pushed the master branch from 2e50a13 to e8a13c0 Compare October 26, 2020 18:27

Fscrypt configuration as per the tool requirements

36a3b9d

kamentomov reviewed Feb 11, 2021

View reviewed changes

primeos requested changes Feb 17, 2021

View reviewed changes

primeos reviewed Feb 17, 2021

View reviewed changes

stale bot added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Apr 16, 2022

lorenz closed this Mar 13, 2023

Janik-Haag added the 12. first-time contribution label Jun 12, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

implement fscrypt for PAM #101604

implement fscrypt for PAM #101604

NeoTheFox commented Oct 24, 2020

andir left a comment

kamentomov left a comment •

edited

NeoTheFox commented Feb 11, 2021

kamentomov commented Feb 11, 2021

primeos commented Feb 11, 2021

kamentomov commented Feb 11, 2021

NeoTheFox commented Feb 11, 2021

primeos commented Feb 11, 2021

primeos left a comment •

edited

primeos Feb 17, 2021 •

edited

primeos Feb 17, 2021

primeos Feb 17, 2021

primeos Feb 17, 2021

primeos Feb 17, 2021

primeos Feb 17, 2021

primeos Feb 17, 2021

primeos Feb 17, 2021 •

edited

primeos Feb 17, 2021

lorenz commented Sep 23, 2021

NeoTheFox commented Sep 23, 2021

stale bot commented Apr 16, 2022

mrvik commented Nov 3, 2022

lorenz commented Mar 13, 2023

		@@ -622,6 +637,8 @@ in

		security.pam.enableOTPW = mkEnableOption "the OTPW (one-time password) PAM module";

		security.pam.enableFscrypt = mkEnableOption "fscrypt filesystem encryption PAM module";

	++ optionals config.security.pam.enableFscrypt [pkgs.fscrypt-experimental]
	++ optionals config.security.pam.enableFscrypt [ pkgs.fscrypt-experimental ]

implement fscrypt for PAM #101604

implement fscrypt for PAM #101604

Conversation

NeoTheFox commented Oct 24, 2020

andir left a comment

Choose a reason for hiding this comment

kamentomov left a comment • edited

Choose a reason for hiding this comment

NeoTheFox commented Feb 11, 2021

kamentomov commented Feb 11, 2021

primeos commented Feb 11, 2021

kamentomov commented Feb 11, 2021

NeoTheFox commented Feb 11, 2021

primeos commented Feb 11, 2021

primeos left a comment • edited

Choose a reason for hiding this comment

primeos Feb 17, 2021 • edited

Choose a reason for hiding this comment

primeos Feb 17, 2021

Choose a reason for hiding this comment

primeos Feb 17, 2021

Choose a reason for hiding this comment

primeos Feb 17, 2021

Choose a reason for hiding this comment

primeos Feb 17, 2021

Choose a reason for hiding this comment

primeos Feb 17, 2021

Choose a reason for hiding this comment

primeos Feb 17, 2021

Choose a reason for hiding this comment

primeos Feb 17, 2021 • edited

Choose a reason for hiding this comment

primeos Feb 17, 2021

Choose a reason for hiding this comment

lorenz commented Sep 23, 2021

NeoTheFox commented Sep 23, 2021

stale bot commented Apr 16, 2022

mrvik commented Nov 3, 2022

lorenz commented Mar 13, 2023

kamentomov left a comment •

edited

primeos left a comment •

edited

primeos Feb 17, 2021 •

edited

primeos Feb 17, 2021 •

edited