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

grub: add support for passwords #65231

Merged
merged 3 commits into from Jul 1, 2020
Merged

Conversation

buckley310
Copy link
Contributor

@buckley310 buckley310 commented Jul 21, 2019

This patch adds support for user accounts/passwords in GRUB 2.
When configured, everything but the default boot option in GRUB is password-protected.

Motivation for this change

This feature adds a small layer of physical security to NixOS systems.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • 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 nix-review --run "nix-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.

@marzipankaiser
Copy link
Contributor

Works for me

@JohnAZoidberg
Copy link
Member

@0x4A6F please state what testing you have done and on what basis you are approving the PR.

@0x4A6F
Copy link
Member

0x4A6F commented Nov 11, 2019

@0x4A6F please state what testing you have done and on what basis you are approving the PR.

I've provisioned a system with <nixpkgs>=this pr and used some boilerplate with following config included.

# ./include/pr/pr-65231.nix
{ config, pkgs, ... }:
  
# nix run nixpkgs.grub2
# grub-mkpasswd-pbkdf2

{

  boot.loader.grub.users = {
    root = { password_pbkdf2 = "grub.pbkdf2.sha512.10000.redacted"; };
  };

}
  • booting default entry without ./include/pr/pr-65231.nix
  • booting default entry with ./include/pr/pr-65231.nix
  • denied access to available generations and editing cmdline (with wrong user or wrong password)
  • accessing available generations (after input of user and valid password)
  • editing cmdline of default entry (after input of user and valid password)

@JohnAZoidberg, if you could come up with other scenarios, I'm eager to test.

@buckley310
Copy link
Contributor Author

I have just tested with the following config. Of the 5 "boot.loader" lines, I tested them one at a time with the others commented out.

{ config, pkgs, ... }:
let
  pw = "zzz";
  pwh = "grub.pbkdf2.sha512.10000.BA1BE1 ...[snip]... D10FBBF04CF4";
in {

  boot.loader.grub.users.root.password = pw;
  boot.loader.grub.users.root.passwordFile = "${pkgs.writeText "pw" pw}";
  boot.loader.grub.users.root.hashedPassword = pwh;
  boot.loader.grub.users.root.hashedPasswordFile = "${pkgs.writeText "pwh" pwh}";
  boot.loader.grub.users.root.hashedPassword = "not a valid hash";

  ...[snip]...
}

@rnhmjoj
Copy link
Contributor

rnhmjoj commented Mar 27, 2020

I tested every options and it's working as expected. The only issue I've found is that nixos-rebuild build-vm-with-bootloader fails with no meaningful if the hash or the password file is invalid.

I guess if you nixos-rebuild switch your system the error will be explicit but I have yet to test it.

@rnhmjoj
Copy link
Contributor

rnhmjoj commented Apr 21, 2020

ping

@buckley310
Copy link
Contributor Author

buckley310 commented Jun 8, 2020

Is there a good way to emit an error from this script during a build-vm event?

@rnhmjoj
Copy link
Contributor

rnhmjoj commented Jun 12, 2020

I think what you are doing is just fine, the problem must be build-vm-with-bootloader is ignoring failures in the install script. I tested with nixos-install and it does work:

installing the boot loader...
setting up /etc...
updating GRUB 2 menu...
Can't read file 'doesnotexist.pwd'! at /nix/store/qps12bpls270qk8pb298abjzsjvnb00w-install-grub.pl line 264.

It would be good to have a NixOS installer test for grub with passwords but I get the OCR to work in the grub screen. I'll try some more before giving up.
Otherwise I'll go on and merge this as is.

@rnhmjoj
Copy link
Contributor

rnhmjoj commented Jun 13, 2020

So, I managed to get the output of grub to the serial console, which is easier to deal with.
I added a test to check if the password protection and booting works.

@rnhmjoj
Copy link
Contributor

rnhmjoj commented Jun 23, 2020

@tfc can you take a look at the test driver part?

def process_serial_output() -> None:
for _line in self.process.stdout:
# Ignore undecodable bytes that may occur in boot menus
line = _line.decode(errors="ignore").replace("\r", "").rstrip()
try:
self.last_lines.put(line, block=False)
except queue.Full:
Copy link
Contributor

Choose a reason for hiding this comment

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

i would rather see first that we really have the problem that some test fails because it goes OOM or gets too slow processing serial output, before we begin dealing with silently failing tests because we drop serial lines without further notice because a small 1000-line buffer overruns.

Copy link
Contributor

@rnhmjoj rnhmjoj Jun 23, 2020

Choose a reason for hiding this comment

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

Sorry, I don't understand what you're asking here. Do you want this operation to be blocking or are you proposing to increase the buffer size (as above)?

Copy link
Contributor

Choose a reason for hiding this comment

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

i think that it would be a good idea to:

  • make the buffer size infinite (get that with empty Queue constructor args)
  • make the put calls blocking, assuming the queue never runs full
  • revisit the question if it is really necessary to model the notion of a 100-lines "screen", because it makes a difference for the caller if the "screen buffer" has 1, 10, or 100 screens stored currently. there is no way to get to the "last" screen. wouldn't it suffice to give wait_for_console_text semantics that are like "does the serial output match my regex since boot/i called this last time"?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, I removed the size limit and stored the lines received since wait_for_console_text is called in a StringIO.
This way we efficiently append lines into the buffer and can match multiline regexes.

Copy link
Contributor

Choose a reason for hiding this comment

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

ping @tfc

Copy link
Contributor

Choose a reason for hiding this comment

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

LGTM!

buckley310 and others added 3 commits June 23, 2020 19:01
This patch adds support for user accounts/passwords in GRUB 2.
When configured, everything but the default option is password-protected.
This method is similar to wait_for_text but is based on matching
serial console lines instead of the VGA output.
@rnhmjoj
Copy link
Contributor

rnhmjoj commented Jul 1, 2020

Thank you for the review!

@rnhmjoj rnhmjoj merged commit dab676b into NixOS:master Jul 1, 2020
@buckley310 buckley310 deleted the grub-password branch October 8, 2020 13:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants