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

Support running xrdp daemon as user privilege like Debian does #2965

Open
metalefty opened this issue Feb 23, 2024 · 5 comments · May be fixed by #2974
Open

Support running xrdp daemon as user privilege like Debian does #2965

metalefty opened this issue Feb 23, 2024 · 5 comments · May be fixed by #2974

Comments

@metalefty
Copy link
Member

I'm thinking of adding support upstream running xrdp daemon as user privileges. Some improvements for this have already been introduced by @matt335672.

@matt335672
Copy link
Member

My suggestion would be to get this in (potentially) for v0.11.x as it will involve some config changes. Also, Debian (who support this feature out-of-band) may need a bit of time to adopt this.

@Natureshadow - any thoughts?

@matt335672
Copy link
Member

I've done a bit of design work on this. Comments welcome.

We can make this facility optional, but my thinking is we should probably try to enforce it in 2024. We should not be running xrdp as root.

Current thinking is to add these values to xrdp.ini:-

; User name and group to run xrdp as.
; These can be added with a command like this (Linux):-
;     useradd xrdp -d / -c 'xrdp daemon' -s /usr/sbin/nologin
; If you alter these, be aware that runtime_group here, and
; SessionSockdirGroup in sesman.ini MUST be the same if you want to
; run sessions.
runtime_user=xrdp
runtime_group=xrdp

and set the existing (but commented) SessionSockdirGroup in sesman.ini as follows:-

+; local sockets for the session are created. This MUST be the same
+; as runtime_group in xrdp.ini, or xrdp will not be able to connect to
+; your sessions.
+SessionSockdirGroup=xrdp

PID files then get created in /run/xrdp-pids which looks like this when xrdp drops privilege:-

$ ls -ld /var/run/xrdp-pids/
drwxrwxr-x 2 root xrdp 40 Feb 26 16:36 /var/run/xrdp-pids/

Log files are opened by root anyway, so at the moment these need no changes. Later it may be worth changing the owner of the log file to xrdp so we can fork and re-open for improved MacOS support (see #2598)

We need to listen() as root so we can support #1707 on FreeBSD. Once the listen() is done we can drop privileges.

@moobyfr
Copy link
Contributor

moobyfr commented Feb 26, 2024

Having a dedicated user could also help storing data which are currently created in /tmp . I'm not aware of living data for daemons, in latest distributions, perhaps a /var/lib/xrdp could be a better chose than / for $HOME ?

@matt335672
Copy link
Member

Hi @moobyfr

Thanks for commenting.

The data that used to be stored under /tmp is now stored under /run/xrdp - see #2731. I need to update the wiki page on this as it's out of date.

We stopped storing it in /tmp for a couple of reasons. One is polyinstantiation (#1482), and the other is that on some systems this area is periodically cleaned up which makes long running sessions unreachable!

None of this data needs to survive a reboot and it's all very small, so storing it under /run seems a much better idea.

I can't find a good recommendation for $HOME for daemons which don't need persistent data. Looking in /etc/passwd, some use /, some have /var/lib/xxx and some use something like /undefined which doesn't exist. I just used / as it's easy but I'm happy to go with a better choice if there's a reason for it.

@matt335672
Copy link
Member

Changed my mind on the sockets dir. It's a bad idea.

Best practice seems to be to create the PID file with privilege and then leave it there when the program exits (as it can't be deleted without privilege). The code that stops the daemon can do the cleanup. This seems easy to to in the init scripts for System-V and FreeBSD.

I've got something working, and hope to get it ready for review by the end of the week.

@matt335672 matt335672 linked a pull request Mar 1, 2024 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants