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.

Sign up for GitHub

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

Jump to bottom

nixos/luksroot: add renewKey option #107038

Closed
wants to merge 2 commits into from
Closed

nixos/luksroot: add renewKey option #107038

wants to merge 2 commits into from

Conversation

hugolgst
Copy link
Member

@hugolgst hugolgst commented Dec 16, 2020 •
edited

Thanks to @cgallay for the change.

Motivation for this change

Add an option to allow to choose if the LUKS module should renew the LUKS key header at each boot which may not be necessary in given cases.
It can also be an issue when there is not enough entropy at the boot stage when the LUKS key header is trying to be renewed.
Let the default value as true to keep the current behavior when renewKey is not present.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • 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 nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/prs-ready-for-review/3032/433

@hugolgst
Copy link
Member Author

Gonna rebase that, thanks @SuperSandro2000

@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/prs-ready-for-review/3032/451

@hugolgst hugolgst added this to the 21.05 milestone Feb 5, 2021
@hugolgst hugolgst mentioned this pull request Mar 8, 2021
Merged
10 tasks
@hugolgst hugolgst self-assigned this Mar 8, 2021
@hugolgst hugolgst requested a review from fogti March 10, 2021 19:43
@hugolgst
nixos/luksroot: add renewKey option
01beccd 
Add an option to allow to choose if the module should renew the LUKS key
header at each boot which may not be neccesary in given cases.

Co-authored-by: Hugo Lageneste <hugo.lageneste@visium.ch>
@hugolgst hugolgst requested a review from fogti March 15, 2021 13:17
@jonringer
Copy link
Contributor

do we have any luks users which have verified that this works as intended? Messing with low level partitions is not something I want to "accidentally" break for someone.

@hugolgst
Copy link
Member Author

I implemented it and tested it on my encrypted NixOS machine.

@hugolgst hugolgst mentioned this pull request Mar 24, 2021
Closed
3 tasks
@jonringer
Copy link
Contributor

I implemented it and tested it on my encrypted NixOS machine.

I would really like to have another person verify. Just to avoid the, "works on my machine" possibility

@Artturin
Copy link
Member

Please rebase.

Whats the command to check if the header was renewed

@hugolgst
Copy link
Member Author

I will rebase.
The code for the renewal is the same as it was already. The change consists in adding a parameter to choose if you want the renewal to be effective or not.
You can try to input wrong passwords three times to see if the header is renewed or not.

SuperSandro2000
SuperSandro2000 reviewed Oct 20, 2021
View reviewed changes
nixos/modules/system/boot/luksroot.nix
echo "Warning: Could not update LUKS key, current challenge persists!"
fi

rm -f /crypt-ramfs/new_key
Copy link
Member

Choose a reason for hiding this comment

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

Should this be behind a TRAP?

nixos/modules/system/boot/luksroot.nix
new_k_luks="$(echo | pbkdf2-sha512 ${toString yubikey.keyLength} $new_iterations $new_response | rbtohex)"
fi

echo -n "$new_k_luks" | hextorb > /crypt-ramfs/new_key
Copy link
Member

Choose a reason for hiding this comment

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

Should /crypt-ramfs/new_key be generated with mktemp?

Copy link
Member Author

Choose a reason for hiding this comment

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

Shall we proceed with these suggestions in another PR since this one is just to add the renewKey option?

Copy link
Member

Choose a reason for hiding this comment

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

idk, because indentation changed this line has a change.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes just because of a condition

@hugolgst
Copy link
Member Author

I am struggling with the rebase which adds thousands of commits in the tree and makes it impossible to retrieve the right ones. Any ideas why?

@aquarial
Copy link
Contributor

aquarial commented Nov 3, 2021

What do you mean "retrieve the right ones". What rebase command are you running?

git pull origin master
git rebase master features/luksroot-renewkey
# fix merge conflicts (yubikey --> dev.yubikey, indentation, etc)

@Artturin
Copy link
Member

Artturin commented Nov 3, 2021

https://medium.com/@topspinj/how-to-git-rebase-into-a-forked-repo-c9f05e821c8a

@LunNova
Copy link
Member

LunNova commented Feb 9, 2022

This PR was manually given the needs_reviewer status instead of using marvin's command, so it can't be progressed by marvin and won't be automatically assigned a reviewer.

@hugolgst If you would like marvin to run on this PR, you can comment

/marvin opt-in
/status needs_reviewer

@hugolgst
Copy link
Member Author

hugolgst commented Feb 9, 2022

/marvin opt-in

@marvin-mk2 marvin-mk2 bot added the marvin label Feb 9, 2022
@marvin-mk2
Copy link

marvin-mk2 bot commented Feb 9, 2022

Hi! I'm an experimental bot. My goal is to guide this PR through its stages, hopefully ending with a merge. You can read up on the usage here.

@hugolgst
Copy link
Member Author

hugolgst commented Feb 9, 2022

/status needs_reviewer

@marvin-mk2
Copy link

marvin-mk2 bot commented Feb 12, 2022

Reminder: Please review!

This Pull Request is awaiting review. If you are the assigned reviewer, please have a look. Try to find another reviewer if necessary. If you can't, please say so. If the status is not accurate, please change it. If nothing happens, this PR will be should be put back in the needs_reviewer queue in one day.

Note: This feature is currently broken. The bot will not actually change the status. If you see this message multiple times, please request a status change manually.

3 similar comments
@marvin-mk2
Copy link

marvin-mk2 bot commented Feb 15, 2022

Reminder: Please review!

This Pull Request is awaiting review. If you are the assigned reviewer, please have a look. Try to find another reviewer if necessary. If you can't, please say so. If the status is not accurate, please change it. If nothing happens, this PR will be should be put back in the needs_reviewer queue in one day.

Note: This feature is currently broken. The bot will not actually change the status. If you see this message multiple times, please request a status change manually.

@marvin-mk2
Copy link

marvin-mk2 bot commented Feb 18, 2022

Reminder: Please review!

This Pull Request is awaiting review. If you are the assigned reviewer, please have a look. Try to find another reviewer if necessary. If you can't, please say so. If the status is not accurate, please change it. If nothing happens, this PR will be should be put back in the needs_reviewer queue in one day.

Note: This feature is currently broken. The bot will not actually change the status. If you see this message multiple times, please request a status change manually.

@marvin-mk2
Copy link

marvin-mk2 bot commented Feb 21, 2022

Reminder: Please review!

This Pull Request is awaiting review. If you are the assigned reviewer, please have a look. Try to find another reviewer if necessary. If you can't, please say so. If the status is not accurate, please change it. If nothing happens, this PR will be should be put back in the needs_reviewer queue in one day.

Note: This feature is currently broken. The bot will not actually change the status. If you see this message multiple times, please request a status change manually.

@hugolgst hugolgst closed this Feb 21, 2022
@MatthewCroughan MatthewCroughan mentioned this pull request Jan 12, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@thiagokokada thiagokokada thiagokokada left review comments

@SuperSandro2000 SuperSandro2000 SuperSandro2000 left review comments

@fogti fogti fogti approved these changes

@Mic92 Mic92 Awaiting requested review from Mic92

@glittershark glittershark Awaiting requested review from glittershark

Assignees

@hugolgst hugolgst

Labels
6.topic: nixos 8.has: module (update) 9.needs: community feedback 10.rebuild-darwin: 0 10.rebuild-linux: 1-10 awaiting_reviewer marvin
Projects
None yet
Milestone
21.05
Development

Successfully merging this pull request may close these issues.

None yet
9 participants
@hugolgst @nixos-discourse @jonringer @Artturin @aquarial @LunNova @thiagokokada @fogti @SuperSandro2000