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

RFC: Stop depending on libsystemd for socket activation #171

Closed
wants to merge 1 commit into from

Conversation

EliteTK
Copy link

@EliteTK EliteTK commented Mar 31, 2024

I've been wanting to implement this for over a year and haven't really finished my original patching attempt. But given recent events (the xz and libsystemd + sshd fiasco) I have decided to give it another go.

I am under no impression that the changes should be included as is, as I've made little to no effort to familiarise myself with the project's coding style and other aspects. But before I commit any more time to this, I wanted to ask for comments to see if this change would even be appealing.

For some background: I am a fan of non-conventional service management and have been working on my own system management suite for some time. I think that socket activation as a concept is quite cool and is not unique to systemd. Unfortunately, a bunch of services which support it, do so by depending on libsystemd which is not often shipped by distributions which do not utilise systemd.

As such, I propose that the dependency on libsystemd is dropped in favour of a simple implementation of the systemd socket activation protocol.

The patch, as it currently stands, tries to match exactly the features which were previously offered by sd_listen_fds and sd_is_socket. Although I've taken some liberties to increase the verbosity of the logging in the latter case.

While inevitably in my time dealing with service management I've perused the systemd sources, I didn't re-implement any of this from memory and instead relied solely on the libsystemd man pages and my own knowledge of how to implement these features. So I don't believe there's any copyright issue here.

As libsystemd is usually only available on systems using systemd, it is
not possible to use socket activation with pcscd on systems which do not
use systemd.

The socket activation mechanism used by systemd is relatively simple,
and re-implementing it is trivial. Socket activation can be convenient
even when systemd is not used.

This patch removes the optional libsystemd dependency and re-implements
the core functionality which was originally depended on in order to
enable socket-activation on all systems assuming the circumstances are
correct.

This change maintains full compatibility with systemd.
@EliteTK EliteTK marked this pull request as draft March 31, 2024 21:59
@LudovicRousseau
Copy link
Owner

If you do not like systemd you can disable it using --disable-libsystemd.

In your case what/who is creating the socket and waiting for an connection?

@EliteTK
Copy link
Author

EliteTK commented Apr 1, 2024

The issue with --disable-libsystemd is specifically that it disables socket activation support. I want to be able to make use of socket activation without needing to have libsystemd. I don't normally control the build flags of my distributions and even if I did, having to write a stub libsystemd AND handle building pcscd is a lot less convenient than just getting pcscd to support socket activation regardless of the presence of libsystemd.

In my case, I have my own code which does socket activation. Here is an example snippet of code which implements socket activation in a systemd compatible way:

#include <stdio.h>
#include <sys/socket.h>
#include <sys/un.h>
#include <unistd.h>
#include <stdlib.h>
#include <inttypes.h>

int main(int argc, char **argv)
{
	int s = socket(AF_UNIX, SOCK_STREAM, 0);
	if (s != 3) return 1;
	struct sockaddr_un sa = {
		.sun_family = AF_UNIX,
		.sun_path = "/tmp/pcscd.socket",
	};
	unlink(sa.sun_path);
	bind(3, (void *)&sa, sizeof sa);
	listen(3, 128);
	setenv("LISTEN_FDS", "1", 1);
	pid_t pid = getpid();
	char pidbuf[100];
	snprintf(pidbuf, sizeof pidbuf, "%jd", (intmax_t)pid);
	setenv("LISTEN_PID", pidbuf, 1);
	unsetenv("LISTEN_FDNAMES");
	execv(argv[1], argv + 1);
}

This is just example code (and has no error handling) but a fuller example would open the listen socket, notify the supervision suite that the service is ready, and then exec pcscd. Or potentially wait for connections before doing the exec.

In my specific use-case, the point of the socket activation would be to minimise resource usage when pcscd is not needed. But recently I've been experimenting with notification driven service startup which requires either a service readiness notification mechanism or socket activation to work correctly.

For the current userbase of people using pcscd on systems with systemd, the proposed patch would have no impact and would not remove/add any features (aside from reducing runtime dependencies). But, for people on systems without libsystemd, it would open up the possibility to use socket activation with pcscd.

@EliteTK
Copy link
Author

EliteTK commented Apr 1, 2024

Just to add to the answer. If you search github for code which is implementing the socket activation protocol, it seems I am not alone in implementing it (outside of systemd):

https://github.com/search?q=%22setenv%28%5C%22LISTEN_FDS%5C%22%22+AND+NOT+unsetenv&type=code

@LudovicRousseau
Copy link
Owner

What are the distributions you use?

@LudovicRousseau LudovicRousseau self-assigned this Apr 1, 2024
@EliteTK
Copy link
Author

EliteTK commented Apr 1, 2024

The main distro I personally care about is voidlinux ( https://github.com/void-linux/void-packages/blob/master/srcpkgs/pcsclite/template ) but I would also like to get this working on devuan.

That being said, I don't see why this shouldn't also end up working on all the BSD systems. There are also many other non-systemd distros which either use a no-op libsystemd stub or compile with --disable-libsystemd.

Just as a note to self: I need to make sure that --disable-libsystemd doesn't cause problems when passed to configure if this change is implemented.

@EliteTK
Copy link
Author

EliteTK commented Apr 1, 2024

It's worth adding. When I initially looked into this feature, I considered adding an additional feature to pcscd where you can pass --socket-activated (or some equivalent command line argument) to make pcscd ignore the result of sd_listen_fds (or lack-thereof) and assume that fd 3 is an appropriate socket. But I discarded the idea.

I am willing to consider this path again if you feel that you would rather accept a new feature than a re-write of an existing one.

@LudovicRousseau
Copy link
Owner

I don't think I will integrate your changes.
I use libsystemd to reuse existing code/lib.

Feel free to use your own patch on your systems. That is why Free Software is great: you can adapt it.

@EliteTK
Copy link
Author

EliteTK commented Apr 1, 2024

Lennart Poettering recommends re-implementing such minor functionality instead of depending on all of libsystemd ( https://news.ycombinator.com/item?id=39867126 ). But I respect your decision, although I disagree that maintaining your own patches is a convenient alternative.

What about adding generic socket activation support in the way I suggested (alongside libsystemd)? With a new --socket-activated option which would be able to handle any socket activation scheme? Would you consider that? It would not be code duplication as it would not need to handle all the details of systemd socket activation.

@EliteTK
Copy link
Author

EliteTK commented Apr 1, 2024

And there is one more compromise I would like to offer, if the one above is not acceptable:

What if the existing code is kept but, when --disable-libsystemd is passed, the new code is added as a fallback?

@LudovicRousseau
Copy link
Owner

That would a revert of patch 30e1095

I will need to think about it.

@fulalas
Copy link

fulalas commented Sep 4, 2024

I totally see @EliteTK's point. It doesn't make sense to depend on systemd if we can achieve the same result without it, and with that making the project more flexible.

I use PorteuX, a Slackware based distro, therefore it has no systemd, and to initialize this project during boot we need to implement some ugly workarounds that are anything but reliable.

If @LudovicRousseau doesn't want to implement a systemd-free solution, I believe falling back to @EliteTK's code when the flag --disable-libsystemd is passed would be the reasonable approach.

By the way, thanks for keeping this project alive! :)

@LudovicRousseau
Copy link
Owner

One solution could be that:

  • you implement the functions sd_listen_fds() and sd_is_socket() in a library
  • you provide a pkg-config file named libsystemd.pc that indicates how to link with the above library
  • you configure PCSC as if you were using the real libsystemd
  • no need to modify pcsc-lite

Would that work for you?

@LudovicRousseau
Copy link
Owner

No answer since 2 months.
I guess my proposal is OK.
Closing.

@EliteTK
Copy link
Author

EliteTK commented Nov 7, 2024

There's a number of outstanding problems with the approach:

  • It still depends on libsystemd, increasing the dependency graph size which puts the project at greater risk of the kind of libxz style security issue that my PR was initially triggered by.
  • While I think in practice it may be possible that this is solved on at least some distributions, such as those which ship elogind, it still requires getting those distributions on board to have pcscd depend on the elogind's libsystemd. It also means that if someone decides to replace libsystemd as part of a wholesale systemd compatible system supervision suite replacement, you would now need some kind of lisbsystemd shim (I think debian does something like this) which is able to dispatch calls to the right libsystemd depending on what systemd replacement you want to use.
  • Finally, (somewhat similar to my first point above but from a non-security perspective) it's still an entire libsystemd dependency to implement 33 lines of code.

But it's fine, I don't think I can convince you so you can keep this closed. I disagree that it's resolved but obviously it's your call on whether you want to take the changes or not.

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.

3 participants