Skip to content

journal: give the ability to enable/disable systemd-journald-audit.so… - #25687

Merged
poettering merged 1 commit into
systemd:mainfrom
fbuihuu:disable-audit-messages-reading
Jan 11, 2023
Merged

journal: give the ability to enable/disable systemd-journald-audit.so…#25687
poettering merged 1 commit into
systemd:mainfrom
fbuihuu:disable-audit-messages-reading

Conversation

@fbuihuu

@fbuihuu fbuihuu commented Dec 9, 2022

Copy link
Copy Markdown
Contributor

…cket

Before this patch the only way to prevent journald from reading the audit messages was to mask systemd-journald-audit.socket. However this had main drawback that downstream couldn't ship the socket disabled by default (beside the fact that masking units is not supposed to be the usual way to disable them).

Fixes #15777

@github-actions github-actions Bot added units please-review PR is ready for (re-)review by a maintainer labels Dec 9, 2022
@fbuihuu

fbuihuu commented Dec 9, 2022

Copy link
Copy Markdown
Contributor Author

I also considered introducing a new option such as ReadAuditLogs= or so but the problem with this approach is that even if journald close the audit fd, PID1 still has one reference which would accumulate up to 128Mo without being freed.

I'm not sure why PID1 keeps a reference after it passes the fd to the service though.

@Werkov

Werkov commented Dec 12, 2022

Copy link
Copy Markdown
Contributor

The messages processing or mere listening audit socket can cause some performance headaches.
This change makes sense since it makes the audit processing configurable via standard mechanisms (e.g. presets).

I may suggest a convenience comment into units/systemd-journald.service.in with a notice that it's not a complete list of sockets. (Afterall, one can only be sure after runtime check of systemctl show -p TriggerdBy because of inverse deps.)

@Werkov Werkov added the journal label Dec 12, 2022
@fbuihuu

fbuihuu commented Dec 12, 2022

Copy link
Copy Markdown
Contributor Author

I may suggest a convenience comment into units/systemd-journald.service.in with a notice that it's not a complete list of sockets. (Afterall, one can only be sure after runtime check of systemctl show -p TriggerdBy because of inverse deps.)

Indeed it's a good suggestion. I'll add a comment if there's a chance for this PR to be accepted.

@poettering

poettering commented Jan 9, 2023

Copy link
Copy Markdown
Member

hmm, i guess this would be ok to do.

But could you please make three additions:

  1. update the man page of journald, add a quick sentence somewhere where we say we read from audit, indicating that this depends on whether systemd-journald-audit.socket is enabled.

  2. Also in the man page of journald.conf, near Audit= explanation say it only has an effect if the socket unit is enabled.

  3. if journald initializes, I think it would be good to output a quick informational string saying whether audit logging is on or not.

@poettering poettering added reviewed/needs-rework 🔨 PR has been reviewed and needs another round of reworks and removed please-review PR is ready for (re-)review by a maintainer labels Jan 9, 2023
@fbuihuu

fbuihuu commented Jan 9, 2023

Copy link
Copy Markdown
Contributor Author

@poettering thanks for the feedback. Before addressing the points you raised, I'd like to know why PID1 keeps the socket fds opened from its side once they have been handed over to the services.

I'm asking because the best solution IMO would be to introduce a new config option but it's currently not ideal because even if journald closes the audit socket, PID1 keeps a reference on it, which prevents the kernel from freeing the resources allocated for the audit socket.

@TriMoon

TriMoon commented Jan 9, 2023

Copy link
Copy Markdown

@fbuihuu Maybe PID1 needs extra functionality to detect if the fd, used for the audit socket, gets closed so it frees it again. 🤔

I guess no-one of the coders even thought about the possibility of that socket getting closed because the default was/is to always have a service read from that socket...
Why code for something that is expected tobe always available...

But then there comes another hick-up: What should happen when the service gets activated again manually afterwards by the operator or otherwise ?
PID1 should keep polling for availability to prevent the need for a reboot? 🤷‍♀️

@poettering

Copy link
Copy Markdown
Member

@poettering thanks for the feedback. Before addressing the points you raised, I'd like to know why PID1 keeps the socket fds opened from its side once they have been handed over to the services.

to make restarts work? i.e. that we hand the same sockets down without losing a single message.

@poettering

poettering commented Jan 9, 2023

Copy link
Copy Markdown
Member

I'm asking because the best solution IMO would be to introduce a new config option but it's currently not ideal because even if journald closes the audit socket, PID1 keeps a reference on it, which prevents the kernel from freeing the resources allocated for the audit socket.

You could just issue the equivalent bus call of "systemctl stop systemd-journald-audit.socket" from journald so that PID1 closes the sockets. That's kinda what a socket unit is after all: the encapsulation of a listening socket, and via "start" you allocate/bind it, and via "stop" you close it.

But please, let's not do that. Let's not add multiple layers of config here, that do the same thing. There's nothing wrong with just telling people "hey, if you want audit gone, use systemctl disable". this is not something that needs to be excessively user friendly, it's not a common thing to do. As long as there's a way it doesn't matter if its a "systemctl disable" or config options, and redundance is a bad thing.

@fbuihuu

fbuihuu commented Jan 10, 2023

Copy link
Copy Markdown
Contributor Author

to make restarts work? i.e. that we hand the same sockets down without losing a single message.

Hmm isn't the service supposed to send the socket fds it wants to keep opened back to PID1 in that case (via sd_notify()) ?

@poettering

Copy link
Copy Markdown
Member

Hmm isn't the service supposed to send the socket fds it wants to keep opened back to PID1 in that case (via sd_notify()) ?

That's for stuff the daemon itself allocates. For example, if a daemon accepts connections on a listener socket. The listener socket would be allocated by PID 1, but the connection sockets would be accept()ed by the daemon itself. If those are long-running connections it might make sense to hand them over to PID 1 so that they can be continued to be processed after a restart.

Moreover, if the listener fd itself would first be handed to the service, and then handed back to PID 1 from there this would not just be more work, but also racy: if the service dies before it can send the listener fd back up, then it is lost. Socket activation is really supposed to be about reliability though: the daemon always can recover the service if it dies, by passing the original listener fd back in, so that the daemon can continue where it left of.

@fbuihuu

fbuihuu commented Jan 10, 2023

Copy link
Copy Markdown
Contributor Author

If the daemon fails to send connection sockets it setup itself to PID1 or any fds that are part of its state, it's likely that it won't be able to recover properly if it dies unexpectedly, no ?

Anyway, I'll update the PR.

@poettering

Copy link
Copy Markdown
Member

If the daemon fails to send connection sockets it setup itself to PID1 or any fds that are part of its state, it's likely that it won't be able to recover properly if it dies unexpectedly, no ?

dunno, failures often are transient, i.e. resource issues triggered on overloaded systems. since these issues are recoverable we should make recovery obvious.

generally the assumption is that we can assert the service manager is reasonably good quality code. it's harder to assert the same for all possible services. hence it's good not leaving too much chance for the service code to fuck up, and make it robust by not even giving the services the chance to fuck up.

@fbuihuu
fbuihuu force-pushed the disable-audit-messages-reading branch from 7eebabe to 270f674 Compare January 11, 2023 07:47
@github-actions github-actions Bot added please-review PR is ready for (re-)review by a maintainer and removed reviewed/needs-rework 🔨 PR has been reviewed and needs another round of reworks labels Jan 11, 2023
@fbuihuu

fbuihuu commented Jan 11, 2023

Copy link
Copy Markdown
Contributor Author

@poettering new version submitted, PTAL.

Comment thread src/journal/journald-server.c Outdated
Comment thread src/journal/journald-server.c Outdated
Comment thread man/journald.conf.xml Outdated
@poettering poettering added reviewed/needs-rework 🔨 PR has been reviewed and needs another round of reworks and removed please-review PR is ready for (re-)review by a maintainer labels Jan 11, 2023
@fbuihuu
fbuihuu force-pushed the disable-audit-messages-reading branch from 270f674 to 6b1ddc1 Compare January 11, 2023 08:43
@github-actions github-actions Bot added please-review PR is ready for (re-)review by a maintainer and removed reviewed/needs-rework 🔨 PR has been reviewed and needs another round of reworks labels Jan 11, 2023
@fbuihuu

fbuihuu commented Jan 11, 2023

Copy link
Copy Markdown
Contributor Author

@poettering is that better ? thanks

@poettering poettering added good-to-merge/waiting-for-ci 👍 PR is good to merge, but CI hasn't passed at time of review. Please merge if you see CI has passed and removed please-review PR is ready for (re-)review by a maintainer labels Jan 11, 2023
@keszybz keszybz removed the good-to-merge/waiting-for-ci 👍 PR is good to merge, but CI hasn't passed at time of review. Please merge if you see CI has passed label Jan 11, 2023
@keszybz

keszybz commented Jan 11, 2023

Copy link
Copy Markdown
Member

Unfortunately this changes the default: before systemd-journald-audit.socket was enabled by default, now it is disabled by default. Please also add a setting in presets to make it enabled by default.

…cket

Before this patch the only way to prevent journald from reading the audit
messages was to mask systemd-journald-audit.socket. However this had main
drawback that downstream couldn't ship the socket disabled by default (beside
the fact that masking units is not supposed to be the usual way to disable
them).

Fixes systemd#15777
@fbuihuu
fbuihuu force-pushed the disable-audit-messages-reading branch from 6b1ddc1 to b8b0095 Compare January 11, 2023 14:23
@fbuihuu

fbuihuu commented Jan 11, 2023

Copy link
Copy Markdown
Contributor Author

Unfortunately this changes the default: before systemd-journald-audit.socket was enabled by default, now it is disabled by default. Please also add a setting in presets to make it enabled by default.

Good catch, fixed. Thanks.

@poettering
poettering merged commit 2aba770 into systemd:main Jan 11, 2023
@mbiebl

mbiebl commented Jan 16, 2023

Copy link
Copy Markdown
Contributor

Just curious: what's the benefit of disabling systemd-journald-audit.socket over just using Audit=no ?


[Install]
WantedBy=sockets.target
WantedBy=systemd-journald.service

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why WantedBy=systemd-journald.service? Shouldn't sockets.target be sufficient and more closely mirror the previous state where there only was a static link in socket.target.wants

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

journald is an early-boot service that should work as intended on its own. sockets.target is a pulled in as part of basic.target, i.e. when you boot at least into "single user mode". That's substantially more than just logging.

So in order to support systems that want to boot exactly one other service + journal i think it makes sense to hook it into systemd-journald.service directly. (not saying this is a reallly relevant scenario, but hey, probably still worth supporting).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

IIRC, it's also useful when starting/stopping journald manually at runtime. So if one decides to enable the collection of audit messages after the system booted, he would simply need to issue systemctl enable systemd-journald-audit.socket; systemctl restart systemd-journald

@poettering

Copy link
Copy Markdown
Member

Just curious: what's the benefit of disabling systemd-journald-audit.socket over just using Audit=no ?

The former disables audit message collection of journald, the latter controls audit message generation of the kernel. The distinction matters once other audit implementations should be used in conjunction

@Werkov

Werkov commented Jan 16, 2023

Copy link
Copy Markdown
Contributor

Just curious: what's the benefit of disabling systemd-journald-audit.socket over just using Audit=no ?

Also, not having the audit socket (idling in PID1) reduces performance impact (on systems where audit isn't intended).

@fbuihuu
fbuihuu deleted the disable-audit-messages-reading branch January 16, 2023 13:00
@mbiebl

This comment was marked as resolved.

@poettering

This comment was marked as resolved.

@mbiebl

mbiebl commented Jan 16, 2023

Copy link
Copy Markdown
Contributor

So we now have these combinations afaics. What happens in those different states? And in what situations should I use Audit=no and where disable the socket?

Audit Socket
on enabled
off enabled
unset enabled
on disabled
off disabled
unset disabled

@keszybz

keszybz commented Jan 26, 2023

Copy link
Copy Markdown
Member

Unfortunately this change exposed a significant bug in systemd transaction logic. I'll file a separate bug with the details.

@poettering

Copy link
Copy Markdown
Member

So we now have these combinations afaics. What happens in those different states? And in what situations should I use Audit=no and where disable the socket?

The socket unit opens the audit socket. if you don't open it no audit for you, none at all. It's a complete kill switch of any audit logic inside of journald.

Audit=yes|no is a wrapper around a specific command that can be issued on the audit socket. if you don't have the audit socket in the first place, we cannot issue that command, hence Audit= is without effect.

Audit=yes tells the kernel via the audit socket to generate audit events. Audit=no tells the kernel to not do so. Audit unset skips the step entirely.

You can also issue the command that Audit= wraps via "auditctl -e…" from outside. This allows journald to listen for audit events on the socket all the time, but until that external invocation is done no audit events will actually be seen.

Basically the socket unit enablement controls whether a) systemd will consume any audit messages that are generated on the system, and b) it controls whether Audit= will have any effect whatsoever.

You might wonder what the point of enabling the socket, but then not using Audit=yes would be. In that mode journald will be a "silent listener", so it won't get any messages until some other tool enables the stuff explicity, such as the original audit tools.

@fbuihuu

fbuihuu commented Feb 1, 2023

Copy link
Copy Markdown
Contributor Author

Unfortunately this change exposed a significant bug in systemd transaction logic. I'll file a separate bug with the details.

@keszybz, could you share the details ?

@dtardon

dtardon commented Feb 1, 2023

Copy link
Copy Markdown
Collaborator

Unfortunately this change exposed a significant bug in systemd transaction logic. I'll file a separate bug with the details.

@keszybz, could you share the details ?

#26216 . It's linked just below the post.

arianvp added a commit to arianvp/nixpkgs that referenced this pull request Feb 7, 2025
nixpkgs-ci Bot pushed a commit to NixOS/nixpkgs that referenced this pull request Feb 11, 2025
This was broken due to systemd/systemd#25687
but we never noticed.

(cherry picked from commit 13b1062)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Development

Successfully merging this pull request may close these issues.

Provide ability to configure systemd to not listen to audit messages

7 participants