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/haproxy: Revive the haproxy user and group #80904

Merged
merged 1 commit into from Mar 11, 2020

Conversation

talyz
Copy link
Contributor

@talyz talyz commented Feb 23, 2020

Motivation for this change

Running haproxy with DynamicUser = true doesn't really work, since it prohibits specifying a TLS certificate bundle with limited permissions. This revives the haproxy user and group without reverting the other useful changes made in 954e234 and also adds options to specify which user and group haproxy runs as.

Things done
  • Tested with NixOps on AWS
  • 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.

@talyz
Copy link
Contributor Author

talyz commented Feb 23, 2020

@GrahamcOfBorg test haproxy

@jonringer
Copy link
Contributor

@infinisil is there a way to solve this tls issue with dynamicuser?

@peterhoeg
Copy link
Member

Instead of having haproxy be responsible for reading the cert from a certain location, why not have a 3rd party service take care of it? That way you separate the concerns.

@aanderse
Copy link
Member

@jonringer I believe the suggested way of doing so is to place the sensitive data under the /var/lib/${DynamicUser} directory and setting StateDirectoryMode to something adequately restrictive. RuntimeDirectory might be better than StateDirectory, depending on the situation.

I have tested this technique with a DynamicUser service combined with the nixops deployment.keys mechanism with initial success. After seeing a great talk on nixops by @talyz I'm inclined to believe this might work out well for him. I'd love to hear some more opinions, though.

@peterhoeg I assume you mean something like vault? That sounds like an even better solution!

@aanderse
Copy link
Member

aanderse commented Mar 6, 2020

@talyz I really don't want to leave you hanging on this, especially if the situation were that this is causing you prod issues.

Have any of our comments been helpful, or is this PR still the way you would like to proceed?

@talyz
Copy link
Contributor Author

talyz commented Mar 6, 2020

@aanderse Sorry, I haven't had the time to investigate this further yet, but reading up on the StateDirectory, per your suggestion, it seems file ownership will be automatically adjusted, so that should work for me. I'll have to try it and report back. If it works, the solution would be to specify a StateDirectory = "haproxy" in the service config and document how to feed haproxy a TLS certificate bundle, since it's not obvious and a common use case for haproxy is TLS termination.

Don't worry, it's not an issue for me yet, since I'm running stable in production. :) Glad to hear you liked the talk, btw. :)

@infinisil
Copy link
Member

infinisil commented Mar 6, 2020

I'm not entirely getting what this TLS issue is exactly, but there's settings like ReadOnlyPaths (man systemd.exec) which allow configuring access to other paths, which should work with DynamicUser

@talyz
Copy link
Contributor Author

talyz commented Mar 7, 2020

@infinisil The issue is about setting strict permissions on a file so that only haproxy is able to read it - the TLS private key and certificate in this case.

@talyz
Copy link
Contributor Author

talyz commented Mar 10, 2020

@aanderse I've tried getting this to work with StateDirectory now, but no luck :/ For security reasons, when DynamicUser=true, the actual files are put in the /var/lib/private/haproxy directory and /var/lib/haproxy is just a symlink to it. If the /var/lib/haproxy directory exists before haproxy is run, the service exits with a failure:

haproxy.service: Failed to set up special execution directory in /var/lib: File exists
haproxy.service: Failed at step STATE_DIRECTORY spawning /nix/store/xxxx-haproxy-2.0.10/sbin/haproxy: File exists

Therefore we end up with a chicken-or-egg problem here where the service needs the secrets to start, but refuses to start when the files are already there.

I also tried putting the files directly into /var/lib/private/haproxy, which also didn't work, since permissions are only adjusted on first service start - if the files are sent again after the service has been started once, it will fail to start again due to insufficient permissions. Had this worked, it still wouldn't have been a pretty solution - having to specify different paths for where to put files and where to access them from, which would require hard-coding the access path, etc.

Curiously, specifying users.users.haproxy.extraGroups = [ "keys" ]; works, but this is because the haproxy user is then created (uid 1000 in my case), which is pretty bad and completely negates DynamicUser.

With all this in mind, I don't really see any other options than going forward with this PR, unless someone else knows a better way.

@Mic92
Copy link
Member

Mic92 commented Mar 10, 2020

In future we might have a better solution using NixOS/rfcs#59 However not using DynamicUser should be fine.

@aanderse
Copy link
Member

I was under the impression SupplementaryGroups worked with DynamicUser... but I haven't tested that.

@@ -448,7 +448,7 @@ in
#tcpcryptd = 93; # unused
firebird = 95;
keys = 96;
#haproxy = 97; # DynamicUser as of 2019-11-08
haproxy = 97;
Copy link
Member

Choose a reason for hiding this comment

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

We probably don't need to allocate a fixed uid/gid though. All files could be chowned at startup.

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated to use dynamically allocated ids instead.

Running haproxy with "DynamicUser = true" doesn't really work, since
it prohibits specifying a TLS certificate bundle with limited
permissions. This revives the haproxy user and group, but makes them
dynamically allocated by NixOS, rather than statically allocated. It
also adds options to specify which user and group haproxy runs as.
@talyz
Copy link
Contributor Author

talyz commented Mar 11, 2020

@Mic92 Yup, that rfc sounds like a good solution when it's implemented, but for now, I think this is the way to go.

@Mic92
Copy link
Member

Mic92 commented Mar 11, 2020

@GrahamcOfBorg test haproxy

@Mic92 Mic92 merged commit 9aa23e3 into NixOS:master Mar 11, 2020
@Mic92
Copy link
Member

Mic92 commented Mar 11, 2020

#82350

@peterhoeg
Copy link
Member

Being a little late to the party obviously doesn't give me much right to complain but there is one regression from this change - the additional restrictions implied by DynamicUser should be added back:

If DynamicUser= is enabled, RemoveIPC= and PrivateTmp= are implied (and cannot be turned off). This ensures that the lifetime
of IPC objects and temporary files created by the executed processes is bound to the runtime of the service, and hence the lifetime of the dynamic
user/group. Since /tmp/ and /var/tmp/ are usually the only world-writable directories on a system this ensures that a unit making use of dynamic
user/group allocation cannot leave files around after unit termination. Furthermore NoNewPrivileges= and RestrictSUIDSGID= are implicitly enabled (and
cannot be disabled), to ensure that processes invoked cannot take benefit or create SUID/SGID files or directories. Moreover ProtectSystem=strict and
ProtectHome=read-only are implied,

@talyz talyz deleted the haproxy-fixes branch June 16, 2021 07:59
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