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/filesystems: ensure keys gid on /run/keys mountpoint #79248

Merged
merged 1 commit into from Feb 8, 2020

Conversation

@flokli
Copy link
Contributor

@flokli flokli commented Feb 5, 2020

boot.specialFileSystems is used to describe mount points to be set up in
stage 1 and 2.

We use it to create /run/keys already there, so sshd-in-initrd scenarios
can consume keys sent over through nixops send-keys.

However, it seems the kernel only supports the gid=… option for tmpfs,
not ramfs, causing /run/keys to be owned by the root group, not keys
group.

This was/is worked around in nixops by running a chown root:keys
/run/keys whenever pushing keys 1, and as machines had to have pushed keys
to be usable, this was pretty much always the case.

This is causing regressions in setups not provisioned via nixops, that
still use /run/keys for secrets (through cloud provider startup scripts
for example), as suddenly being an owner of the "keys" group isn't
enough to access the folder.

This PR removes the defunct gid=… option in the mount script called in
stage 1 and 2, and introduces a tmpfiles rule which takes care of fixing
up permissions as part of sysinit.target (very early in systemd bootup,
so before regular services are started).

In case of nixops deployments, this doesn't change anything.
nixops-based deployments receiving secrets from nixops send-keys in
initrd will simply have the permissions already set once tmpfiles is
started.

Fixes #42344

Motivation for this change
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.
boot.specialFileSystems is used to describe mount points to be set up in
stage 1 and 2.

We use it to create /run/keys already there, so sshd-in-initrd scenarios
can consume keys sent over through nixops send-keys.

However, it seems the kernel only supports the gid=… option for tmpfs,
not ramfs, causing /run/keys to be owned by the root group, not keys
group.

This was/is worked around in nixops by running a chown root:keys
/run/keys whenever pushing keys [1], and as machines had to have pushed keys
to be usable, this was pretty much always the case.

This is causing regressions in setups not provisioned via nixops, that
still use /run/keys for secrets (through cloud provider startup scripts
for example), as suddenly being an owner of the "keys" group isn't
enough to access the folder.

This PR removes the defunct gid=… option in the mount script called in
stage 1 and 2, and introduces a tmpfiles rule which takes care of fixing
up permissions as part of sysinit.target (very early in systemd bootup,
so before regular services are started).

In case of nixops deployments, this doesn't change anything.
nixops-based deployments receiving secrets from nixops send-keys in
initrd will simply have the permissions already set once tmpfiles is
started.

Fixes #42344

[1]: https://github.com/NixOS/nixops/blob/884d6c3994b227eb09c307e5d25d6885c9af8220/nixops/backends/__init__.py#L267-L269
@worldofpeace
Copy link
Member

@worldofpeace worldofpeace commented Feb 5, 2020

self pinging @NixOS/nixos-release-managers (I don't know why you can request review to this team)

@disassembler disassembler modified the milestones: 20.03, 20.09 Feb 5, 2020
@fpletz
fpletz approved these changes Feb 8, 2020
@fpletz fpletz merged commit 64ece8c into NixOS:master Feb 8, 2020
13 checks passed
13 checks passed
Evaluation Performance Report Evaluator Performance Report
Details
grahamcofborg-eval ^.^!
Details
grahamcofborg-eval-check-maintainers matching changed paths to changed attrs...
Details
grahamcofborg-eval-check-meta config.nix: checkMeta = true
Details
grahamcofborg-eval-darwin nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A darwin-tested
Details
grahamcofborg-eval-nixos nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release-combined.nix -A tested
Details
grahamcofborg-eval-nixos-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release.nix -A manual
Details
grahamcofborg-eval-nixos-options nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release.nix -A options
Details
grahamcofborg-eval-nixpkgs-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A manual
Details
grahamcofborg-eval-nixpkgs-tarball nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A tarball
Details
grahamcofborg-eval-nixpkgs-unstable-jobset nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A unstable
Details
grahamcofborg-eval-package-list nix-env -qa --json --file .
Details
grahamcofborg-eval-package-list-no-aliases nix-env -qa --json --file . --arg config { allowAliases = false; }
Details
@flokli flokli deleted the flokli:run-keys-group branch Feb 8, 2020
dtzWill added a commit to dtzWill/nixpkgs that referenced this pull request Feb 8, 2020
nixos/filesystems: ensure keys gid on /run/keys mountpoint

(cherry picked from commit 64ece8c)
@Mic92
Copy link
Contributor

@Mic92 Mic92 commented Feb 12, 2020

This now chown all files to be owned by root. This was not the case before.

Mic92 added a commit to Mic92/nixpkgs that referenced this pull request Feb 12, 2020
We already set owner per secret instead of using the secret group:
Introduced in NixOS#79248
@flokli
Copy link
Contributor Author

@flokli flokli commented Feb 12, 2020

This was already followed up in #79862 (merged), and backported into 20.03.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

5 participants
You can’t perform that action at this time.