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

treewide: Replace uses of replace-literal with replace-secret to avoid leaking secrets #121708

Merged
merged 6 commits into from
May 19, 2021
Merged

treewide: Replace uses of replace-literal with replace-secret to avoid leaking secrets #121708

merged 6 commits into from
May 19, 2021

Conversation

talyz
Copy link
Contributor

@talyz talyz commented May 4, 2021

Motivation for this change

Using replace-literal (or similar) to insert secrets leaks the secrets through the replace-literal process' /proc/<pid>/cmdline file. This introduces a tiny replace-secret utility which solves this by reading the secret straight from the file instead, which also usually simplifies the code a bit, since we're always reading the secret from a file anyway.

I've focused on replacing uses of replace-literal here, since they translate very easily, but sed usage and other similar ones should also be replaced in the future.

The mpd and mpdscribble changes have not been tested with real credentials, so help from someone who uses them would be appreciated.

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.

@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 May 4, 2021
grahamc
grahamc reviewed May 4, 2021
View reviewed changes
pkgs/build-support/replace-secret/replace-secret.py Outdated
'/proc/<pid>/cmdline', unlike when 'sed' or 'replace' is used.

"""
with open(secret_filename) as sf, open(filename, 'r+') as f:
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 set a umask or something?

Copy link
Contributor Author

@talyz talyz May 4, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

It does the replacement in-place, so the file's permissions are retained. I guess it could force the permissions to something restrictive, but this feels like a surprising behavior..

@talyz talyz force-pushed the replace-secret branch from 1ddca4d to 482140d Compare May 5, 2021 08:58
@talyz talyz mentioned this pull request May 5, 2021
Merged
10 tasks
@talyz talyz added the 1.severity: security Issues which raise a security issue, or PRs that fix one label May 5, 2021
@talyz talyz requested review from flokli, a user, aanderse, doronbehar and Sohalt May 7, 2021 06:47
@doronbehar
Copy link
Contributor

I can confirm that both mpd and mpdscribble launch fine with this change merged to nixos-unstable.

doronbehar
doronbehar approved these changes May 7, 2021
View reviewed changes
Copy link
Contributor

@doronbehar doronbehar left a comment

Choose a reason for hiding this comment

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

Diff LGTM.

@fricklerhandwerk
Copy link
Contributor

LGTM

doronbehar
doronbehar reviewed May 14, 2021
View reviewed changes
Copy link
Contributor

@doronbehar doronbehar left a comment

Choose a reason for hiding this comment

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

All commits touching replace-secret.py should be squashed.

pkgs/build-support/replace-secret/replace-secret.nix Outdated Show resolved Hide resolved
nixos/modules/services/web-apps/discourse.nix
@@ -713,11 +710,12 @@ in
cfg.siteSettings
"/run/discourse/config/nixos_site_settings.json"
}
install -T -m 0400 -o discourse ${discourseConf} /run/discourse/config/discourse.conf
install -T -m 0600 -o discourse ${discourseConf} /run/discourse/config/discourse.conf
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this change related?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, this is required for it to be able to write to the file. replace-literal ignores permissions (or maybe the -f flag also has that effect), but replace-secret doesn't.

nixos/modules/services/web-apps/discourse.nix
${mkSecretReplacement cfg.database.passwordFile}
${mkSecretReplacement cfg.mail.outgoing.passwordFile}
${mkSecretReplacement cfg.redis.passwordFile}
${mkSecretReplacement cfg.secretKeyBaseFile}
chmod 0400 /run/discourse/config/discourse.conf
Copy link
Contributor

Choose a reason for hiding this comment

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

This change too perhaps is worth merging, but should at least come in a separate commit.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This makes sure the file has the same permissions as before the commit, but sets it after inserting the secrets.

@talyz talyz force-pushed the replace-secret branch from 616ff85 to 3432971 Compare May 14, 2021 07:24
@doronbehar doronbehar requested a review from infinisil May 14, 2021 08:22
@talyz talyz force-pushed the replace-secret branch from 3432971 to 4daff86 Compare May 14, 2021 14:39
@talyz talyz mentioned this pull request May 16, 2021
Closed
andir
andir requested changes May 16, 2021
View reviewed changes
pkgs/build-support/replace-secret/replace-secret.nix
@@ -0,0 +1,22 @@
{ stdenv, lib, python3 }:
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps add a check phase here that does some trivial usage of the command so we can be sure it still works without having to run nixos tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, that's a good idea. I added two simple checks. If the diff looks okay to you, I'll squash it.

@talyz talyz force-pushed the replace-secret branch from 71788f6 to 23ceade Compare May 19, 2021 06:26
talyz added 6 commits May 19, 2021 09:32
@talyz
replace-secret: Init
27f8f69 
Add a small utility script which securely replaces secrets in
files. Doing this with `sed`, `replace-literal` or similar utilities
leaks the secrets through the spawned process' `/proc/<pid>/cmdline` file.
@talyz
nixos/discourse: Use replace-secret to avoid leaking secrets
38398fa 
Using `replace-literal` to insert secrets leaks the secrets through
the `replace-literal` process' `/proc/<pid>/cmdline`
file. `replace-secret` solves this by reading the secret straight from
the file instead, which also simplifies the code a bit.
@talyz
nixos/gitlab: Use replace-secret to avoid leaking secrets
7842e89 
Using `replace-literal` to insert secrets leaks the secrets through
the `replace-literal` process' `/proc/<pid>/cmdline`
file. `replace-secret` solves this by reading the secret straight from
the file instead, which also simplifies the code a bit.
@talyz
nixos/mpdscribble: Use replace-secret to avoid leaking secrets
3a29b7b 
Using `replace-literal` to insert secrets leaks the secrets through
the `replace-literal` process' `/proc/<pid>/cmdline`
file. `replace-secret` solves this by reading the secret straight from
the file instead, which also simplifies the code a bit.
@talyz
nixos/mpd: Use replace-secret to avoid leaking secrets
88b76d5 
Using `replace-literal` to insert secrets leaks the secrets through
the `replace-literal` process' `/proc/<pid>/cmdline`
file. `replace-secret` solves this by reading the secret straight from
the file instead.
@talyz
nixos/keycloak: Use replace-secret to avoid leaking secrets
380b52c 
Using `replace-literal` to insert secrets leaks the secrets through
the `replace-literal` process' `/proc/<pid>/cmdline`
file. `replace-secret` solves this by reading the secret straight from
the file instead, which also simplifies the code a bit.
@talyz talyz force-pushed the replace-secret branch from 23ceade to 380b52c Compare May 19, 2021 07:33
@talyz talyz merged commit f131787 into NixOS:master May 19, 2021
@talyz talyz deleted the replace-secret branch May 19, 2021 07:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@grahamc grahamc grahamc left review comments

@andir andir andir requested changes

@doronbehar doronbehar doronbehar approved these changes

@fricklerhandwerk fricklerhandwerk fricklerhandwerk approved these changes

@flokli flokli Awaiting requested review from flokli

@aanderse aanderse Awaiting requested review from aanderse

@Sohalt Sohalt Awaiting requested review from Sohalt

@infinisil infinisil Awaiting requested review from infinisil

Assignees
No one assigned
Labels
1.severity: security Issues which raise a security issue, or PRs that fix one 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/` 8.has: package (new) This PR adds a new package 10.rebuild-darwin: 1-10 10.rebuild-darwin: 1 10.rebuild-linux: 1-10 11.by: package-maintainer This PR was created by the maintainer of the package it changes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@talyz @doronbehar @fricklerhandwerk @grahamc @andir