Skip to content

Issue 6181 - RFE - Allow system to manage uid/gid at startup #6182

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

Merged
merged 1 commit into from
Jun 5, 2024

Conversation

Firstyear
Copy link
Contributor

Bug Description: We have a user who wishes to implement a non-standard configuration in which the running gid is not the primary gid of the uid that the server runs as. Currently this trips up most of our setup tools.

Rather than support dropping to an alternate gid in the server, it is simpler to allow systemd to pre-configure our user and group at start up. This needs a small number of changes.

Fix Description:

  • dscreate needs to correctly setup file ownships for dse.ldif and friends rather than relying on the server having root access and changing the perms itself
  • Our unit file needs to enable the CAP_NET_BIND privilege so that the service can bind to ports lower than 1024 without being root
  • The server needs to not attempt to change it's uid/gid if we are already running as that user/gid.

fixes: #6181

Author: William Brown william@blackhats.net.au

Review by: ???

@Firstyear Firstyear marked this pull request as draft May 29, 2024 02:28
Bug Description: We have a user who wishes to implement a non-standard configuration in which the
running gid is not the primary gid of the uid that the server runs as. Currently this trips up most
of our setup tools.

Rather than support dropping to an alternate gid in the server, it is simpler to allow systemd to
pre-configure our user and group at start up. This needs a small number of changes.

Fix Description:
- dscreate needs to correctly setup file ownships for dse.ldif and friends rather than relying on
  the server having root access and changing the perms itself
- Our unit file needs to enable the CAP_NET_BIND privilege so that the service can bind to ports
  lower than 1024 without being root
- The server needs to not attempt to change it's uid/gid if we are already running as that user/gid.

fixes: 389ds#6181

Author: William Brown <william@blackhats.net.au>

Review by: ???
@Firstyear Firstyear force-pushed the 20240529-6181-systemd-uid-gid branch from 9d55ba3 to 520016f Compare May 29, 2024 04:03
@Firstyear Firstyear marked this pull request as ready for review May 29, 2024 04:04
Copy link
Contributor

@mreynolds389 mreynolds389 left a comment

Choose a reason for hiding this comment

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

LGTM, but I'd like someone else form the team to review this as well.

@progier389
Copy link
Contributor

Looks mostly good.
My only concern is about the =+ change:
What are the problems if using plain = or =! ?

The documentation is a bit frightful:
If the executable path is prefixed with "+" then the process is executed with full privileges. In this mode privilege restrictions configured with User=, Group=, CapabilityBoundingSet= or the various file system namespacing options (such as PrivateDevices=, PrivateTmp=) are not applied to the invoked command line (but still affect any other ExecStart=, ExecStop=, … lines). However, note that this will not bypass options that apply to the whole control group, such as DevicePolicy=, see [systemd.resource-control(5)](https://www.freedesktop.org/software/systemd/man/latest/systemd.resource-control.html#) for the full list.

It is hard to fully understand the impact of such a change and I wonder if it could not lead to some security issue
especially when arbitrary code may be run through custom plugin and also that ns-slapd explicitly checks that PrivateTmp is enabled when initializing the security layers.

On the other side, we also run with nearly full privilege when using root user but
unless mistaken most of these privileges get dropped when calling setuid (which is no more called if not started as root)

@Firstyear
Copy link
Contributor Author

Looks mostly good. My only concern is about the =+ change: What are the problems if using plain = or =! ?

The documentation is a bit frightful: If the executable path is prefixed with "+" then the process is executed with full privileges. In this mode privilege restrictions configured with User=, Group=, CapabilityBoundingSet= or the various file system namespacing options (such as PrivateDevices=, PrivateTmp=) are not applied to the invoked command line (but still affect any other ExecStart=, ExecStop=, … lines). However, note that this will not bypass options that apply to the whole control group, such as DevicePolicy=, see [systemd.resource-control(5)](https://www.freedesktop.org/software/systemd/man/latest/systemd.resource-control.html#) for the full list.

We could use =!, but we can't use =.

The reason is that if we add a User/Group line, then that applies to execstartpre and execstart with =. That would break both the execstartpre scripts as both require root (systemd ask pass literally requires root, and so does selinux to change things).

So =! might work (because it ignores the user/group parts) but it also might not because of the other permission bounds we set in the script. But = will break.

It is hard to fully understand the impact of such a change and I wonder if it could not lead to some security issue especially when arbitrary code may be run through custom plugin and also that ns-slapd explicitly checks that PrivateTmp is enabled when initializing the security layers.

On the other side, we also run with nearly full privilege when using root user but unless mistaken most of these privileges get dropped when calling setuid (which is no more called if not started as root)

Yes, this prevents the need to setuid at all because we start as the isolated user in the first place.

@progier389
Copy link
Contributor

Thank you for the explanation, it makes me realize that it was only the preexec script that sets the selinux and file acl permissions that requires increased permissions and not ns-slapd as I initially thought.
So everything is fine except that I was overly paranoiac !

@progier389
Copy link
Contributor

Just a remark that does not impact the code:
We should warn users that want to use this feature that all the instances shall have the same user:
ds_systemd_ask_password_acl is a bit crude and overwrite the /var/run/systemd/ask-password acl
so the last started instance win ...

@Firstyear
Copy link
Contributor Author

No, better to be paranoid and to ask the questions. So thank you for asking them :)

Yeah, it's such a problem - we have asked upstream systemd to fix this since I think 2016 ... they simply don't care. :(

@Firstyear Firstyear merged commit d7b56a1 into 389ds:main Jun 5, 2024
11 of 195 checks passed
@Firstyear Firstyear deleted the 20240529-6181-systemd-uid-gid branch June 5, 2024 00:18
@Firstyear
Copy link
Contributor Author

By the way, we would like this in 2.2 for SUSE since that's what we currently have on our LTS - are there any objections if I backport this? We're currently waiting on some customer feedback, so once I have that I'll do it.

In the process, should I backport to other versions too?

Or is this something that initially we should leave just as a SUSE patch. I think given it's not enabled by default it's a low risk to change.

@vashirov
Copy link
Member

vashirov commented Jun 5, 2024

Let's first fix test failures before backporting, currently all tests are red, they fail with

E               subprocess.CalledProcessError: Command '['systemctl', 'start', 'dirsrv@standalone1']' returned non-zero exit status 1.

See https://github.com/389ds/389-ds-base/actions/runs/9376433608

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

Successfully merging this pull request may close these issues.

Allow systemd to manage uid/gid changes rather than calling setuid.
4 participants