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

Open
wants to merge 1 commit into
from

Conversation

Projects
None yet
10 participants
@spinus
Contributor

spinus commented Dec 22, 2015

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

This comment has been minimized.

Show comment
Hide comment
@mention-bot

mention-bot Dec 22, 2015

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

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

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

This comment has been minimized.

@copumpkin

copumpkin Dec 22, 2015

Member

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

@copumpkin

copumpkin Dec 22, 2015

Member

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

This comment has been minimized.

@spinus

spinus Dec 22, 2015

Contributor

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 :-)

@spinus

spinus Dec 22, 2015

Contributor

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 :-)

This comment has been minimized.

@copumpkin

copumpkin Dec 22, 2015

Member

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

@copumpkin

copumpkin Dec 22, 2015

Member

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

This comment has been minimized.

@spinus

spinus Dec 22, 2015

Contributor

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.

@spinus

spinus Dec 22, 2015

Contributor

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.

nixos/modules/security/pam.nix
- '';
+ U2F = {
+ enabled = mkOption {
+ default = config.security.pam.U2F.enabled;

This comment has been minimized.

@copumpkin

copumpkin Dec 22, 2015

Member

Isn't this a circular default? Or what am I missing?

Edit: oh, hmm, there's another module further down...

@copumpkin

copumpkin Dec 22, 2015

Member

Isn't this a circular default? Or what am I missing?

Edit: oh, hmm, there's another module further down...

This comment has been minimized.

@spinus

spinus Dec 22, 2015

Contributor

:) yup.

@spinus

spinus Dec 22, 2015

Contributor

:) yup.

@copumpkin

This comment has been minimized.

Show comment
Hide comment
@copumpkin

copumpkin Dec 22, 2015

Member

Why the duplicate module?

Member

copumpkin commented Dec 22, 2015

Why the duplicate module?

@spinus

This comment has been minimized.

Show comment
Hide comment
@spinus

spinus Dec 22, 2015

Contributor

What do you mean?

Contributor

spinus commented Dec 22, 2015

What do you mean?

@copumpkin

This comment has been minimized.

Show comment
Hide comment
@copumpkin

copumpkin Dec 23, 2015

Member

@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.

Member

copumpkin commented Dec 23, 2015

@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

This comment has been minimized.

Show comment
Hide comment
@spinus

spinus Dec 23, 2015

Contributor

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 :)

Contributor

spinus commented Dec 23, 2015

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

This comment has been minimized.

Show comment
Hide comment
@spinus

spinus Jan 5, 2016

Contributor

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

Contributor

spinus commented Jan 5, 2016

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

@spinus

This comment has been minimized.

Show comment
Hide comment
@spinus

spinus Jan 11, 2016

Contributor

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

Contributor

spinus commented Jan 11, 2016

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

@spinus

This comment has been minimized.

Show comment
Hide comment
@spinus

spinus Jan 23, 2016

Contributor

@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)

Contributor

spinus commented Jan 23, 2016

@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)

@spinus spinus referenced this pull request Mar 11, 2016

Closed

Yubico U2F #10382

@jagajaga

This comment has been minimized.

Show comment
Hide comment
Member

jagajaga commented Mar 11, 2016

ping @copumpkin

@spacekitteh

This comment has been minimized.

Show comment
Hide comment
@spacekitteh

spacekitteh Nov 4, 2016

Contributor

@grahamc security :)

Contributor

spacekitteh commented Nov 4, 2016

@grahamc security :)

nixos/modules/security/pam.nix
- with the associated U2F key.
- '';
+ u2f = {
+ enabled = mkOption {

This comment has been minimized.

@fpletz

fpletz Nov 28, 2016

Member

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

@fpletz

fpletz Nov 28, 2016

Member

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

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

This comment has been minimized.

@fpletz

fpletz Nov 28, 2016

Member

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

@fpletz

fpletz Nov 28, 2016

Member

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

This comment has been minimized.

@spinus

spinus Nov 28, 2016

Contributor

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

Thank you for your review.

@spinus

spinus Nov 28, 2016

Contributor

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

Thank you for your review.

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.
+ '';

This comment has been minimized.

@fpletz

fpletz Nov 28, 2016

Member

Wrong indention.

@fpletz

fpletz Nov 28, 2016

Member

Wrong indention.

@jb55

This comment has been minimized.

Show comment
Hide comment
@jb55

jb55 Jan 16, 2017

Contributor

status of this?

Contributor

jb55 commented Jan 16, 2017

status of this?

@spinus

This comment has been minimized.

Show comment
Hide comment
@spinus

spinus Jan 17, 2017

Contributor

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

Contributor

spinus commented Jan 17, 2017

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

@makefu

This comment has been minimized.

Show comment
Hide comment
@makefu

makefu Mar 24, 2017

Contributor

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

Contributor

makefu commented Mar 24, 2017

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

@spinus

This comment has been minimized.

Show comment
Hide comment
@spinus

spinus Mar 25, 2017

Contributor

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?

Contributor

spinus commented Mar 25, 2017

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

This comment has been minimized.

Show comment
Hide comment
@jb55

jb55 Mar 25, 2017

Contributor
Contributor

jb55 commented Mar 25, 2017

@spinus

This comment has been minimized.

Show comment
Hide comment
@spinus

spinus Mar 25, 2017

Contributor

@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.

Contributor

spinus commented Mar 25, 2017

@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

This comment has been minimized.

Show comment
Hide comment
@spinus

spinus Mar 25, 2017

Contributor

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?

Contributor

spinus commented Mar 25, 2017

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?

pam-u2f: refactor, docs about u2f_keys path
* change enableU2F option to u2f.* set
* add few u2f options (not all) to customize pam-u2f module
* document default u2f_keys locations
@spinus

This comment has been minimized.

Show comment
Hide comment
@spinus

spinus Apr 18, 2017

Contributor

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 :)

Contributor

spinus commented Apr 18, 2017

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

This comment has been minimized.

Show comment
Hide comment
@jb55

jb55 May 3, 2017

Contributor
Contributor

jb55 commented May 3, 2017

@spinus

This comment has been minimized.

Show comment
Hide comment
@spinus

spinus May 8, 2017

Contributor

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

Contributor

spinus commented May 8, 2017

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

@jb55

This comment has been minimized.

Show comment
Hide comment
@jb55

jb55 May 8, 2017

Contributor
Contributor

jb55 commented May 8, 2017

@spinus

This comment has been minimized.

Show comment
Hide comment
@spinus

spinus May 8, 2017

Contributor

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)

Contributor

spinus commented May 8, 2017

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

This comment has been minimized.

Show comment
Hide comment
@ixmatus

ixmatus May 23, 2017

Contributor

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

Contributor

ixmatus commented May 23, 2017

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

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