-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
use DynamicUser=yes for all cockpit components #16811
Changes from all commits
26f5aa8
5911fb9
8ae0d32
e438ef2
73875c2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -31,11 +31,13 @@ nodist_systemdunit_DATA = \ | |
|
||
dist_systemdunit_DATA = \ | ||
src/systemd/cockpit-session.socket \ | ||
src/systemd/cockpit-session-socket-user.service \ | ||
src/systemd/cockpit-ws-user.service \ | ||
src/systemd/system-cockpithttps.slice \ | ||
src/systemd/cockpit-wsinstance-http.socket \ | ||
src/systemd/cockpit-wsinstance-https-factory.socket \ | ||
src/systemd/[email protected] \ | ||
src/systemd/cockpit-wsinstance-socket-user.service \ | ||
$(NULL) | ||
|
||
# ----------------- | ||
|
@@ -57,11 +59,6 @@ tmpfilesconfdir = $(prefix)/lib/tmpfiles.d | |
systemdgenerated += $(nodist_tmpfilesconf_DATA) | ||
nodist_tmpfilesconf_DATA = src/systemd/tmpfiles.d/cockpit-ws.conf | ||
|
||
# ----------------- | ||
# sysusers | ||
sysusersconfdir = $(prefix)/lib/sysusers.d | ||
dist_sysusersconf_DATA = src/systemd/sysusers.d/cockpit-wsinstance.conf | ||
|
||
# ----------------- | ||
# Policykit | ||
polkitdir = $(datadir)/polkit-1/actions | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,11 @@ | ||
[Unit] | ||
Description=Dynamic user for /run/cockpit/session socket | ||
BindsTo=cockpit-session.socket | ||
|
||
[Service] | ||
DynamicUser=yes | ||
User=cockpit-session-socket | ||
Group=cockpit-session-socket | ||
Type=oneshot | ||
ExecStart=/bin/true | ||
RemainAfterExit=yes |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,10 +1,13 @@ | ||
[Unit] | ||
Description=Initiator socket for Cockpit sessions | ||
PartOf=cockpit.service | ||
Requires=cockpit-session-socket-user.service | ||
After=cockpit-session-socket-user.service | ||
|
||
[Socket] | ||
ListenStream=/run/cockpit/session | ||
SocketUser=root | ||
SocketGroup=cockpit-wsinstance | ||
SocketGroup=cockpit-session-socket | ||
SocketMode=0660 | ||
RemoveOnStop=yes | ||
Accept=yes |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,11 @@ | ||
[Unit] | ||
Description=Dynamic user for /run/cockpit/wsinstance/ sockets | ||
BindsTo=cockpit.service | ||
|
||
[Service] | ||
DynamicUser=yes | ||
User=cockpit-wsinstance-socket | ||
Group=cockpit-wsinstance-socket | ||
Type=oneshot | ||
ExecStart=/bin/true | ||
RemainAfterExit=yes |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,8 +4,8 @@ Documentation=man:cockpit-ws(8) | |
Requires=cockpit.socket | ||
Requires=cockpit-wsinstance-http.socket cockpit-wsinstance-https-factory.socket | ||
# ensure our DynamicUser exists | ||
Requires=cockpit-ws-user.service | ||
After=cockpit-ws-user.service | ||
Requires=cockpit-ws-user.service cockpit-wsinstance-socket-user.service | ||
After=cockpit-ws-user.service cockpit-wsinstance-socket-user.service | ||
# we need to start after the sockets so that we can instantly forward incoming requests | ||
After=cockpit-wsinstance-http.socket cockpit-wsinstance-https-factory.socket | ||
|
||
|
@@ -17,6 +17,7 @@ ExecStartPre=+@libexecdir@/cockpit-certificate-ensure --for-cockpit-tls | |
ExecStart=@libexecdir@/cockpit-tls | ||
User=cockpit-ws | ||
Group=cockpit-ws | ||
SupplementaryGroups=cockpit-wsinstance-socket | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. And ditto. We should have |
||
NoNewPrivileges=true | ||
ProtectSystem=strict | ||
ProtectHome=true | ||
|
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -73,8 +73,8 @@ use of systemd features. | |
reads the fingerprint from stdin, and asks systemd to start a new | ||
[[email protected]](../src/ws/[email protected]) | ||
and .service pair. | ||
* Each instance runs in its own systemd cgroup, as another unprivileged system | ||
user `cockpit-wsinstance`. | ||
* Each instance runs in its own systemd cgroup, as another unprivileged | ||
dynamic system user `cockpit-wsinstance-socket`. | ||
* cockpit-tls exports the client certificates to `/run/cockpit/tls/<fingerprint>` | ||
while there is at least one open connection with that certificate, i. e. as | ||
long as there is an active Cockpit session. | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -332,7 +332,7 @@ class TestConnection(testlib.MachineCase): | |
# number of https instances is bounded (DoS prevention) | ||
# with MaxTasks=200 und 2 threads per ws instance we should have a | ||
# rough limit of 100 instances, so at some point curl should start failing | ||
m.execute("runuser -u cockpit-ws -- sh -ec 'RC=1; for i in `seq 120`; do " | ||
m.execute("runuser -u cockpit-wsinstance-socket -- sh -ec 'RC=1; for i in `seq 120`; do " | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It occurs to me that if we ran this as |
||
" echo -n $i | nc %s -U /run/cockpit/wsinstance/https-factory.sock;" | ||
" curl --silent --head --max-time 5 --unix-socket /run/cockpit/wsinstance/https@$i.sock http://dummy > /dev/null || RC=0; " | ||
"done; exit $RC'" % n_opt) | ||
|
@@ -1005,7 +1005,7 @@ until pgrep -f '^(/usr/[^ ]+/[^ /]*python[^ /]* )?/usr/bin/cockpit-bridge'; do s | |
|
||
# ws with plain --no-tls should fail after login with mismatching Origin (expected http, got https) | ||
m.execute("systemctl start cockpit-session.socket") | ||
m.spawn(f"runuser -u cockpit-wsinstance -- {self.ws_executable} --no-tls -p 9099", | ||
m.spawn(f"runuser -u cockpit-session-socket -- {self.ws_executable} --no-tls -p 9099", | ||
"ws-notls.log") | ||
m.wait_for_cockpit_running(tls=True) | ||
|
||
|
@@ -1038,7 +1038,7 @@ until pgrep -f '^(/usr/[^ ]+/[^ /]*python[^ /]* )?/usr/bin/cockpit-bridge'; do s | |
self.allow_browser_errors("Error reading machine id") | ||
|
||
# ws with --for-tls-proxy accepts only https origins, thus should work | ||
m.spawn(f"runuser -u cockpit-wsinstance -- {self.ws_executable} --for-tls-proxy -p 9099 -a 127.0.0.1", | ||
m.spawn(f"runuser -u cockpit-session-socket -- {self.ws_executable} --for-tls-proxy -p 9099 -a 127.0.0.1", | ||
"ws-fortlsproxy.log") | ||
m.wait_for_cockpit_running(tls=True) | ||
b.open(f"https://{b.address}:{b.port}/system") | ||
|
@@ -1441,7 +1441,7 @@ server { | |
|
||
def run_ws(extra_opts=""): | ||
m.spawn( | ||
f"runuser -u cockpit-wsinstance -- {self.libexecdir}/cockpit-ws " | ||
f"runuser -u cockpit-session-socket -- {self.libexecdir}/cockpit-ws " | ||
f"--address=127.0.0.1 --for-tls-proxy {extra_opts}", "ws.log") | ||
m.wait_for_cockpit_running() | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -378,6 +378,7 @@ authentication via sssd/FreeIPA. | |
%{_unitdir}/cockpit-motd.service | ||
%{_unitdir}/cockpit.socket | ||
%{_unitdir}/cockpit-ws-user.service | ||
%{_unitdir}/cockpit-session-socket-user.service | ||
%{_unitdir}/cockpit-session.socket | ||
%{_unitdir}/[email protected] | ||
%{_unitdir}/cockpit-wsinstance-http.socket | ||
|
@@ -386,9 +387,9 @@ authentication via sssd/FreeIPA. | |
%{_unitdir}/[email protected] | ||
%{_unitdir}/[email protected] | ||
%{_unitdir}/[email protected] | ||
%{_unitdir}/cockpit-wsinstance-socket-user.service | ||
%{_unitdir}/system-cockpithttps.slice | ||
%{_prefix}/%{__lib}/tmpfiles.d/cockpit-ws.conf | ||
%{_sysusersdir}/cockpit-wsinstance.conf | ||
%{pamdir}/pam_ssh_add.so | ||
%{pamdir}/pam_cockpit_cert.so | ||
%{_libexecdir}/cockpit-ws | ||
|
@@ -407,11 +408,6 @@ authentication via sssd/FreeIPA. | |
%ghost %{_sharedstatedir}/selinux/%{selinuxtype}/active/modules/200/%{name} | ||
|
||
%pre ws | ||
# HACK: old RPM and even Fedora's current RPM don't properly support sysusers | ||
# https://github.com/rpm-software-management/rpm/issues/3073 | ||
getent group cockpit-wsinstance >/dev/null || groupadd -r cockpit-wsinstance | ||
getent passwd cockpit-wsinstance >/dev/null || useradd -r -g cockpit-wsinstance -d /nonexisting -s /sbin/nologin -c "User for cockpit-ws instances" cockpit-wsinstance | ||
|
||
if %{_sbindir}/selinuxenabled 2>/dev/null; then | ||
%selinux_relabel_pre -s %{selinuxtype} | ||
fi | ||
|
@@ -446,6 +442,11 @@ if test -f %{_sysconfdir}/pam.d/cockpit && grep -q pam_cockpit_cert %{_sysconfd | |
echo '**** WARNING:' | ||
fi | ||
|
||
# remove obsolete system user on upgrade (replaced with DynamicUser in version 330) | ||
if getent passwd cockpit-wsinstance >/dev/null; then | ||
userdel cockpit-wsinstance | ||
fi | ||
|
||
%preun ws | ||
%systemd_preun cockpit.socket cockpit.service | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,18 +5,19 @@ ${env:deb_systemdsystemunitdir}/cockpit-motd.service | |
${env:deb_systemdsystemunitdir}/cockpit.socket | ||
${env:deb_systemdsystemunitdir}/cockpit-session.socket | ||
${env:deb_systemdsystemunitdir}/[email protected] | ||
${env:deb_systemdsystemunitdir}/cockpit-session-socket-user.service | ||
${env:deb_systemdsystemunitdir}/cockpit-ws-user.service | ||
${env:deb_systemdsystemunitdir}/cockpit-wsinstance-http.service | ||
${env:deb_systemdsystemunitdir}/cockpit-wsinstance-http.socket | ||
${env:deb_systemdsystemunitdir}/[email protected] | ||
${env:deb_systemdsystemunitdir}/cockpit-wsinstance-https-factory.socket | ||
${env:deb_systemdsystemunitdir}/[email protected] | ||
${env:deb_systemdsystemunitdir}/[email protected] | ||
${env:deb_systemdsystemunitdir}/cockpit-wsinstance-socket-user.service | ||
${env:deb_systemdsystemunitdir}/system-cockpithttps.slice | ||
${env:deb_pamlibdir}/security/pam_ssh_add.so | ||
${env:deb_pamlibdir}/security/pam_cockpit_cert.so | ||
usr/lib/tmpfiles.d/cockpit-ws.conf | ||
usr/lib/sysusers.d/cockpit-wsinstance.conf | ||
usr/lib/cockpit/cockpit-session | ||
usr/lib/cockpit/cockpit-ws | ||
usr/lib/cockpit/cockpit-wsinstance-factory | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reading this, even as someone who understands how this pile of software works, is confusing.
We've used
cockpit-ws
forcockpit-tls
andcockpit-wsinstance
forcockpit-ws
for a while now for reasons that are no longer relevant. I think we should take the time to change these usernames:cockpit-tls
forcockpit-tls
andcockpit-ws
forcockpit-ws
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to be careful about "cockpit-ws", as we used to have that static user, and /etc/cockpit/ws-certs.d/* may still be owned by it on old systems. In general, +1 for cleaning up the user names, though!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For that reason I added postinst scripts which remove the cockpit-wsinstance user on upgrade, but not for cockpit-ws. I don't want to get into the business of randomly chowning files in /etc. You can just too easily screw up shared certificates, backups, break Ansible scripts, etc.