refactor pam-u2f: add keysPath, verbose, fix docs about u2f_keys path #11886

spinus · 2015-12-22T21:54:24Z

change enableU2F option to U2F.* set
added few pam-u2f options
fix documentation about default u2f_keys locations

mention-bot · 2015-12-22T21:54:26Z

By analyzing the blame information on this pull request, we identified @philandstuff, @edolstra and @obadz to be potential reviewers

copumpkin · 2015-12-22T23:02:21Z

nixos/modules/security/pam.nix

-          <filename>~/.yubico/u2f_keys</filename> are able to log in
-          with the associated U2F key.
-        '';
+      U2F = {


Not a fan of starting the module name with uppercase, but am not sure if we have any official guidelines on that.

I'm big fan of "this-notation" or "this_notation" but I'm not sure we want that here. Some packages are using this convention, but I didn't see that for options.

Here I followed something like, camelCase but if the first letter is big I'm making it smaller just for naming sake. the same I would do if the name would started with API I wouldn't do apiSomething.

But I can use whatever convention is agreed :-)

https://github.com/Yubico/pam-u2f Yubico themselves seem to have no trouble keeping it lowercase

True.
Is that good occasion to move that decision to mailing list or ticket and make some naming convention agreements? I saw one ticket about that but as far as I remember it stucked.

copumpkin · 2015-12-22T23:05:39Z

Why the duplicate module?

spinus · 2015-12-22T23:10:53Z

What do you mean?

copumpkin · 2015-12-23T14:05:27Z

@spinus as far as I can tell, you have two copies of all the U2F options, first in the let block, and then in the final returned attrset. They have identical (as far as I can eyeball) documentation, types, and names.

spinus · 2015-12-23T14:20:53Z

The reason for this is (I guess), the "top" layer works as defaults for all pam services, the other one is per service.

example:

config.security.pam.U2F.enabled = True
config.security.pam.services.<name?>.U2F.enabled=False

So you can enable it globally and disable for particular modules.
For each service there is the same set of options which get defaults from "global" services config.
It was like that before but only one option was passed down the hole and the name was different. My idea is to unify this names to understand that you are changing the same thing (previously the names were enabledU2F and u2fAuth, so not only different name but different naming convention as well).

The problem I have here, I have no idea how to grab description from "outer" scope the same I grabbed the default value so I copy pasted as "good" start :)

spinus · 2016-01-05T21:36:32Z

hey, can we move it a little? What I should do/fix to make this happen?

spinus · 2016-01-11T21:14:32Z

@copumpkin hey, updated the name (U2F -> u2f), is it better now?

spinus · 2016-01-23T03:11:09Z

@philandstuff @edolstra @obadz do you think this is good direction? Any idea how I could use description only once in current situation (I know I could extract it level up as variable, but that would be confusing probably)

jagajaga · 2016-03-11T21:58:32Z

ping @copumpkin

spacekitteh · 2016-11-04T15:49:28Z

@grahamc security :)

fpletz · 2016-11-28T03:56:31Z

nixos/modules/security/pam.nix

-          with the associated U2F key.
-        '';
+      u2f = {
+        enabled = mkOption {


The attribute shoud be called enable like in every other module.

fpletz · 2016-11-28T03:58:28Z

nixos/modules/security/pam.nix

+          <literal>cue</literal> option is added to <literal>pam-u2f</literal>
+          module and reminder message will be displayed.
+        '';
+      };


Both u2f options should be generated by a function instead auf copy-paste.

@fpletz now, one year after I pushed that, I know lot more about nix :-) I try to refactor.

Thank you for your review.

fpletz · 2016-11-28T03:58:47Z

nixos/modules/security/pam.nix

+            If you sen this option to <literal>true</literal>,
+               <literal>cue</literal> option is added to <literal>pam-u2f</literal>
+                 module and reminder message will be displayed.
+                 '';


Wrong indention.

jb55 · 2017-01-16T20:39:18Z

status of this?

spinus · 2017-01-17T00:08:24Z

@jb55 waits for me to refactor it, my current estimation is I'll take a look in next 2 weeks.

makefu · 2017-03-24T12:19:55Z

any update on this? would love to see this in master 👍

spinus · 2017-03-25T20:45:03Z

Yeah, I started working on it again in January, but during that year stuff has changed a little and I started doing it again rather then rebasing or anything. In the meantime I stuck somewhere there (I had a problem with nix expression evaluation).

I was also wondering if the name should be changed to yubico (from u2f). Yubico has pam module named u2f but it looks like yubikey is supported but I'm not sure if that is universal or not. If it's universal, it's kine of unfortunate that default key files are named yubikey. What do you think?

jb55 · 2017-03-25T21:24:15Z

Tomasz Czyż <notifications@github.com> writes:

I was also wondering if the name should be changed to yubico (from u2f). Yubico has pam module named u2f but it looks like yubikey is supported but I'm not sure if that is universal or not. If it's universal, it's kine of unfortunate that default key files are named yubikey. What do you think?

It is universal, I have a trezor which works with pam-u2f.

spinus · 2017-03-25T21:27:57Z

@jb55 thanks for info. In that case I'll just continue to refactor it as u2f. If I won't finish it to the end of mid-April, I'll post what I have until then and maybe somebody can continue.

spinus · 2017-03-25T21:35:49Z

btw, I was thinking about writing a test for that module but I have no idea how to test that in the vm without the key :-) Any idea how we can test that module?

* change enableU2F option to u2f.* set * add few u2f options (not all) to customize pam-u2f module * document default u2f_keys locations

spinus · 2017-04-18T01:43:33Z

OK, I understood a little better the structure in pam.nix and tried to extend it rather than changing convention like in my previous PR. So security.pam.u2f defines the options and they are set for all applications that use pam. Particular pam applications can use of not u2f by setting u2fAuth attribute (looks like that's how all other modules are set).

I added few (but not all) pam-u2f options.
I added really bad test, probably would be nice to have "software" device and do proper test like in oath module, but I'm not that familiar with u2f and don't have time to dive into it now. If anyone could provide any input I'm happy to implement proper test.

Please review :)

jb55 · 2017-05-03T00:35:42Z

Tomasz Czyż <notifications@github.com> writes:

Please review :)

I got around to testing this today. It seems to work for the initial login and password prompts, but for some reason it breaks systemd user services...

spinus · 2017-05-08T18:31:04Z

@jb55 could you describe a little more the use case? I'll try to test that and fix.

jb55 · 2017-05-08T19:24:27Z

Sure, if I remember correctly all I did was set it to `required` and tried logging in. That worked fine but I noticed redshift wasn't running. So I tried `systemctl start --user redshift` and I got an error. I didn't have time to look into it further. We could merge this and open a separate issue, as it seemed to work fine other than that small hiccup.

spinus · 2017-05-08T21:12:24Z

ah no, I would like not to break anything what is working, give me some time, I'll try to test it or even write test case or try to look if it's possible to limit u2f module to affect only "human" users (if it's affecting system users or whatever)

ixmatus · 2017-05-23T16:02:19Z

I wanted to give my encouragement for this PR and that I'm willing to help if needed and where my spare time permits.

Ekleog · 2018-10-22T00:06:39Z

(triage) @spinus, do you still plan to rebase this PR and have a look at whether the issue with systemd user services still happens? :)

spinus · 2018-10-22T03:20:28Z

@fpletz please take a look

spinus · 2018-10-22T03:23:22Z

@fpletz please take a look

kalbasit · 2019-01-22T02:27:25Z

@spinus can you please rebase?

kalbasit · 2019-01-22T02:37:25Z

nixos/modules/security/pam.nix

@@ -37,12 +37,11 @@ let
      };

      u2fAuth = mkOption {
-        default = config.security.pam.enableU2F;
+        default = config.security.pam.u2f.enable;


can you please update https://github.com/NixOS/nixpkgs/blob/master/nixos/modules/rename.nix to reflect this rename?

kalbasit · 2019-01-22T02:39:08Z

nixos/modules/security/pam.nix

+          <literal>https://developers.yubico.com/pam-u2f/</literal>.
+        '';
+      };
+      authfile = mkOption {


can you add a new line between options? See above (same file) for examples.

kalbasit · 2019-01-22T17:32:07Z

nixos/modules/security/pam.nix

+        '';
+      };
+      control = mkOption {
+        default = "required";


It's currently set to sufficient, setting it to required might unexpectedly change the behavior for everyone. Please set this back to sufficient

kalbasit · 2019-01-22T17:35:56Z

nixos/tests/pam-u2f.nix

+        interactive=true;
+        control="sufficient";
+        cue=true;
+        debug=true;


please leave a space around the = signs

kalbasit · 2019-01-22T17:36:11Z

nixos/tests/pam-u2f.nix

+    ''
+      $machine->waitForUnit('multi-user.target');
+      $machine->succeed('egrep "auth sufficient .*u2f.*" /etc/pam.d/ -R ');
+


nitpick: remove newline.

kalbasit · 2019-01-28T03:25:51Z

closing this PR in favor of #54756.

copumpkin reviewed Dec 22, 2015
View reviewed changes

spinus force-pushed the u2f branch from 175e725 to bb82964 Compare January 11, 2016 20:59

spinus mentioned this pull request Mar 11, 2016

Yubico U2F #10382

Closed

spinus mentioned this pull request Jun 11, 2016

2nd factor authentication turns off 1st factor authentication #14997

Closed

fpletz requested changes Nov 28, 2016

View reviewed changes

joachifm added the 2.status: merge conflict label Mar 12, 2017

pam-u2f: refactor, docs about u2f_keys path

b5fbc05

* change enableU2F option to u2f.* set * add few u2f options (not all) to customize pam-u2f module * document default u2f_keys locations

spinus force-pushed the u2f branch from bb82964 to b5fbc05 Compare April 18, 2017 01:38

oxij mentioned this pull request Jun 27, 2018

wiki: Luks-based FDE with Yubikey PBA and btrfs on UEFI NixOS #13275

Closed

kalbasit self-requested a review January 22, 2019 02:27

kalbasit requested changes Jan 22, 2019

View reviewed changes

This was referenced Jan 25, 2019

nixos/pam: Include the "cue" option to the pam_u2f.so invocation. #45905

Closed

nixos: security.pam: Rename enableU2F and add opts #40455

Closed

kalbasit closed this Jan 28, 2019

kalbasit mentioned this pull request Jan 28, 2019

nixos/pam: refactor U2F, docs about u2f_keys path #54756

Merged

10 tasks

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

refactor pam-u2f: add keysPath, verbose, fix docs about u2f_keys path #11886

refactor pam-u2f: add keysPath, verbose, fix docs about u2f_keys path #11886

Conversation

spinus commented Dec 22, 2015 • edited Loading

mention-bot commented Dec 22, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

copumpkin commented Dec 22, 2015

spinus commented Dec 22, 2015

copumpkin commented Dec 23, 2015

spinus commented Dec 23, 2015

spinus commented Jan 5, 2016

spinus commented Jan 11, 2016

spinus commented Jan 23, 2016

jagajaga commented Mar 11, 2016

spacekitteh commented Nov 4, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jb55 commented Jan 16, 2017

spinus commented Jan 17, 2017

makefu commented Mar 24, 2017

spinus commented Mar 25, 2017

jb55 commented Mar 25, 2017 via email

spinus commented Mar 25, 2017

spinus commented Mar 25, 2017

spinus commented Apr 18, 2017

jb55 commented May 3, 2017 via email

spinus commented May 8, 2017

jb55 commented May 8, 2017 via email

spinus commented May 8, 2017

ixmatus commented May 23, 2017

Ekleog commented Oct 22, 2018

spinus commented Oct 22, 2018

spinus commented Oct 22, 2018

kalbasit commented Jan 22, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kalbasit commented Jan 28, 2019

spinus commented Dec 22, 2015 •

edited

Loading

kalbasit commented Jan 22, 2019 •

edited

Loading