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

build: create and package systemd units #151

Merged
merged 2 commits into from
Jan 9, 2024
Merged

build: create and package systemd units #151

merged 2 commits into from
Jan 9, 2024

Conversation

GabrielNagy
Copy link
Contributor

Create systemd units for the socket-activated daemon and install them in the built deb.

As the daemon only communicates via D-Bus (via Unix sockets) we can activate almost all hardening options provided by systemd, lowering the exposure level reported by systemd-analyze security from 9.8 to 1.8. Looking at each of the options, the newest one we use is ProcSubset added in v247 available starting with Jammy.

Because config/cache paths are configurable (and writable) I avoided enabling ProtectSystem which mounts the system read-only.

I confirmed installing the deb will correctly enable authd.socket and immediately start authd.service.

Fixes UDENG-1877

@GabrielNagy GabrielNagy requested a review from a team as a code owner December 13, 2023 16:24
@codecov-commenter
Copy link

codecov-commenter commented Dec 13, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (06c32ac) 88.62% compared to head (e54e37a) 88.83%.
Report is 19 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #151      +/-   ##
==========================================
+ Coverage   88.62%   88.83%   +0.20%     
==========================================
  Files          34       35       +1     
  Lines        2532     2650     +118     
==========================================
+ Hits         2244     2354     +110     
- Misses        221      228       +7     
- Partials       67       68       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Collaborator

@3v1n0 3v1n0 left a comment

Choose a reason for hiding this comment

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

Looks good, just some minor comments.

debian/rules Outdated Show resolved Hide resolved
systemd/authd.service Show resolved Hide resolved
systemd/authd.socket Show resolved Hide resolved
Create systemd units for the socket-activated daemon and install them in
the built deb.

As the daemon only communicates via D-Bus (via Unix sockets) we can
activate almost all hardening options provided by systemd, lowering the
exposure level reported by `systemd-analyze security` from 9.8 to 1.8.

Because config/cache paths are configurable (and writable) I avoided
enabling ProtectSystem which mounts the system read-only.

Fixes UDENG-1877
Copy link
Member

@didrocks didrocks left a comment

Choose a reason for hiding this comment

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

Nice work! This is quite a protected service / set of units, I though have some questions and I think some suggestions:

  • did you try adding/removing users from system groups? I do not see anything blocking here, but still worth trying.
  • especially as we are executing other subprocesses like gpasswd and see its effects (even if it’s only supposed to read /etc/passwd and edit /etc/groups).
  • I think we should restrict the subprocesses we can execute to reduce the attack surface with ExecPaths= nested inside NoExecPaths (setting it to gpasswd and getent). However, I didn’t have the time last months to test why it didn’t behave as expected, so if you can have some attempts on this…

debian/rules Outdated Show resolved Hide resolved
systemd/authd.service Show resolved Hide resolved
systemd/authd.service Outdated Show resolved Hide resolved
systemd/authd.service Outdated Show resolved Hide resolved
systemd/authd.service Outdated Show resolved Hide resolved
@GabrielNagy
Copy link
Contributor Author

I think we should restrict the subprocesses we can execute to reduce the attack surface with ExecPaths= nested inside NoExecPaths (setting it to gpasswd and getent). However, I didn’t have the time last months to test why it didn’t behave as expected, so if you can have some attempts on this…

Had a look with strace and it seems that gpasswd is a Swiss-army knife type of program which executes other binaries like nscd or sss_cache which adds to the unpredictability of this.

Strangely enough, if I add the following directives to the service (taking gpasswd out of the picture) it won't start at all 🤔:

NoExecPaths=/
ExecPaths=/sbin/authd
Jan 04 12:32:53 authd-mantic (authd)[6017]: authd.service: Failed to execute /sbin/authd: Permission denied
Jan 04 12:32:53 authd-mantic (authd)[6017]: authd.service: Failed at step EXEC spawning /sbin/authd: Permission denied
Jan 04 12:32:53 authd-mantic systemd[1]: authd.service: Main process exited, code=exited, status=203/EXEC
Jan 04 12:32:53 authd-mantic systemd[1]: authd.service: Failed with result 'exit-code'.
Jan 04 12:32:53 authd-mantic systemd[1]: Failed to start authd.service - Authd daemon service.

I can't really figure out what's going on so I'm inclined to keep these directives out of the service definition for now.

especially as we are executing other subprocesses like gpasswd and see its effects (even if it’s only supposed to read /etc/passwd and edit /etc/groups).

This is a good point, running gpasswd under the service failed, and stracing it showed that we need to enable the CAP_CHOWN capability for it to be able to write to /etc/gshadow. Good catch!

- remove network restrictions which would hinder the proper
functionality of getent over external protocols like LDAP
- permit CAP_CHOWN capability for gpasswd
- document some restrictions
- use a glob approach to install services
Copy link
Member

@didrocks didrocks left a comment

Choose a reason for hiding this comment

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

Thanks for doing those changes! Everything looks good from my side, with the big mistery of Exec permissions, but it’s something we will tackel in other places too anyway in the future.

@GabrielNagy GabrielNagy merged commit 9c01851 into main Jan 9, 2024
5 checks passed
@GabrielNagy GabrielNagy deleted the systemd-units branch January 9, 2024 09:44
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.

4 participants