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/i3lock: i3lock program with u2fSupport option #205578

Merged
merged 2 commits into from
Dec 22, 2022

Conversation

dev-null-undefined
Copy link
Contributor

@dev-null-undefined dev-null-undefined commented Dec 11, 2022

Description of changes

Created programs.i3lock.
With option u2fSupport, to create wrapper around i3lock with setuid set and pam u2fAuth enabled.

Which enables yubikey logins when screen locked using i3lock

setuid is workaround a known bug in i3lock i3/i3lock#198

Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • 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/)
  • 23.05 Release Notes (or backporting 22.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
    • (Release notes changes) Ran nixos/doc/manual/md-to-db.sh to update generated release notes
  • Fits CONTRIBUTING.md.

@github-actions github-actions bot added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/` labels Dec 11, 2022
@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 labels Dec 11, 2022
@RaitoBezarius
Copy link
Member

Shouldn't this be programs.i3lock and have a u2fSupport option? I feel like this is a very small module for a very trivial thing that can be documented but I have no strong feeling.

@dev-null-undefined
Copy link
Contributor Author

dev-null-undefined commented Dec 17, 2022

Shouldn't this be programs.i3lock and have a u2fSupport option? I feel like this is a very small module for a very trivial thing that can be documented but I have no strong feeling.

There would be no need for you to use it as a program instead of package, which could be confusing, i3lock works just fine as a package when you don't want the u2fSupport.
I do agree that it would probably be better for documentation but it would also make it 2 step process to enable first enable i3lock program which would have no other effect then adding i3lock into your pkgs and then enabling suboption u2fSupport which would wrap it and enable the u2f.

I feel like that could be confusing instead of being more documenting but that's just how I feel about it, but that's just me so if you think the other way would be better I can definitely change it.

PS:
I also wished to make this work with option for choosing your i3lock package for example: i3lock-color etc..
But every package has different binary name :( not sure if there is any workaround but it would be also nice to have.

@RaitoBezarius
Copy link
Member

Shouldn't this be programs.i3lock and have a u2fSupport option? I feel like this is a very small module for a very trivial thing that can be documented but I have no strong feeling.

There would be no need for you to use it as a program instead of package, which could be confusing, i3lock works just fine as a package when you don't want the u2fSupport. I do agree that it would probably be better for documentation but it would also make it 2 step process to enable first enable i3lock program which would have no other effect then adding i3lock into your pkgs and then enabling suboption u2fSupport which would wrap it and enable the u2f.

I feel like that could be confusing instead of being more documenting but that's just how I feel about it, but that's just me so if you think the other way would be better I can definitely change it.

Technically, programs like this already exist: gpg, etc. Of course, they are often correlated with setuid binaries stuff.
But I would not be surprised some of them can work without programs with some serious limitations.
So i3lock do not feel that different to me. Also, I think as long as this is well documented in search.nixos.org and release notes, then, it's fine.

But, I have no strong feeling on that to be honest, I would love to have other i3lock users weigh in here.

PS: I also wished to make this work with option for choosing your i3lock package for example: i3lock-color etc.. But every package has different binary name :( not sure if there is any workaround but it would be also nice to have.

Adding a symlink to i3lock for the closure of each of this package?

@dev-null-undefined
Copy link
Contributor Author

dev-null-undefined commented Dec 17, 2022

Thanks for the response!
Will change it as you suggested add symlinks, to each i3lock pkg, so that you can chose your own i3lock pkg.
Which I will add as an option to the program.i3lock.

@dev-null-undefined dev-null-undefined changed the title nixos/i3lock-u2f: i3lock wrapper with u2fAuth nixos/i3lock: i3lock program with u2fSupport option Dec 18, 2022
@dev-null-undefined dev-null-undefined force-pushed the i3lock-yubikey branch 2 times, most recently from c967891 to a824932 Compare December 18, 2022 16:18
@ofborg ofborg bot requested review from malyn and NickHu December 18, 2022 16:21
@ofborg ofborg bot added 10.rebuild-darwin: 1-10 and removed 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin labels Dec 18, 2022
@dev-null-undefined
Copy link
Contributor Author

@RaitoBezarius What do you think about that?
Tested all the edited packages and everything seems to be working nicely.
Also thanks to your recommendations it looks much cleaner as well.

@RaitoBezarius
Copy link
Member

Result of nixpkgs-review pr 205578 run on x86_64-linux 1

2 packages blacklisted:
  • nixos-install-tools
  • tests.nixos-functions.nixos-test
6 packages built:
  • betterlockscreen
  • i3lock-blur
  • i3lock-color
  • i3lock-fancy
  • i3lock-fancy-rapid
  • multilockscreen

Copy link
Member

@RaitoBezarius RaitoBezarius left a comment

Choose a reason for hiding this comment

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

Some minor changes to do but sounds good

nixos/modules/programs/i3lock.nix Outdated Show resolved Hide resolved
nixos/modules/programs/i3lock.nix Outdated Show resolved Hide resolved
nixos/modules/programs/i3lock.nix Outdated Show resolved Hide resolved
nixos/modules/programs/i3lock.nix Outdated Show resolved Hide resolved
nixos/modules/programs/i3lock.nix Show resolved Hide resolved
i3lock program with option to enable u2fAuth.
Added symlink `i3lock` to the output binary,
In order to enable `programs.i3lock.u2fSupport` which relies on file with name
`i3lock` inside out dir.
@dev-null-undefined dev-null-undefined requested review from RaitoBezarius and removed request for malyn and NickHu December 18, 2022 20:37
@ofborg ofborg bot requested review from NickHu and malyn December 18, 2022 20:42
@dev-null-undefined
Copy link
Contributor Author

Should be all done now except for the literalExample which seems to be deprecated, so I left it as it is.
Thanks again for all the help.

@RaitoBezarius
Copy link
Member

Result of nixpkgs-review pr 205578 run on x86_64-linux 1

2 packages blacklisted:
  • nixos-install-tools
  • tests.nixos-functions.nixos-test
6 packages built:
  • betterlockscreen
  • i3lock-blur
  • i3lock-color
  • i3lock-fancy
  • i3lock-fancy-rapid
  • multilockscreen

Copy link
Member

@RaitoBezarius RaitoBezarius 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 to me!

@RaitoBezarius RaitoBezarius merged commit 5431f7c into NixOS:master Dec 22, 2022
@dev-null-undefined dev-null-undefined deleted the i3lock-yubikey branch December 22, 2022 00:17
@cbr9
Copy link

cbr9 commented Aug 26, 2023

Hi, how can this be used with betterlockscreen?

@dev-null-undefined
Copy link
Contributor Author

Hi, how can this be used with betterlockscreen?

From quickly checking on my phone it probably will not work.
But the fix should be quiet simply just creating link named i3lock just like the second commit of this PR did.

Note: I am not familiar with betterlockscreen I am making the assumption that it is just some sort of i3lock wrapper.

@cbr9
Copy link

cbr9 commented Aug 26, 2023

I solved it. Betterlockscreen is a wrapper around i3lock-color. It has a config file where you can specify the path to the i3lock-color binary. Since this binary is symlinked to i3lock, all that's necessary is to set i3lockcolor_bin="/run/wrappers/bin/i3lock" in said config.

@dev-null-undefined
Copy link
Contributor Author

I solved it. Betterlockscreen is a wrapper around i3lock-color. It has a config file where you can specify the path to the i3lock-color binary. Since this binary is symlinked to i3lock, all that's necessary is to set i3lockcolor_bin="/run/wrappers/bin/i3lock" in said config.

Noice good to know. My fix wouldn't work now that I checked how the betterlockscreen works. Glad you found solution. There isn't probably better way to do this right now :( at least not one that I can think of, since it's using wrapper with set PATH variable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/` 10.rebuild-darwin: 1-10 10.rebuild-linux: 1-10
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants