nit

Posted Sep 27, 2024 14:17 UTC (Fri) by djm (subscriber, #11651)
Parent article: Debian changes OpenSSH packaging

OpenSSH was patched so that it could notify systemd when it was ready to accept connections, using the libsystemd library

Nit: we patched OpenSSH to notify systemd without using the libsystemd library. We would have done it sooner if there was a published specification for the interface, but documentation on how to interact with systemd outside the actual implementation was lacking.

That aside, we really welcome Debian's work to upstream their changes - we've accepted almost everything that they've proposed.

nit

Posted Sep 27, 2024 15:46 UTC (Fri) by jzb (editor, #7867) [Link] (1 responses)

Sorry - wasn't that _after_ the XZ incident, though? Perhaps I should've written "Debian OpenSSH had been patched" or something similar to make that clearer?

nit

Posted Sep 27, 2024 20:03 UTC (Fri) by bluca (subscriber, #118303) [Link]

Yes, that's correct, most generalist distributions patched it to use libsystemd to fix certain issues, for many years. After the xz exploit we started pushing again to get the patch upstream (the bugzilla ticket about this had been open for years too), open coding the integration instead of using libsystemd, and this was merged.

nit

Posted Sep 27, 2024 20:51 UTC (Fri) by Cyberax (✭ supporter ✭, #52523) [Link]

> we've accepted almost everything that they've proposed.
How about MPTCP?

Thanks for integrating based on a protocol rather than dependencies

Posted Sep 28, 2024 11:56 UTC (Sat) by fredrik (subscriber, #232) [Link] (2 responses)

Feels like stating the obvious here, but: Thank you for using that approach in OpenSSH.

Using a protocol rather than calling a function in a library dependency is of course an excellent way to isolate the calling code from any vulnerabilities or other problems in a library implementation or its transitive dependencies.

In this case, when the protocol is trivial, it is also an easy choice. Or at least it became trivial once the protocol was documented. Before then, perhaps the most obvious way was to use libsystemd? Just because no other option was apparent.

Anyway, it seems to be a good strategy to reduce attack vectors for vulnerabilities, to prefer integration based on protocols over added library dependencies.

Thanks for integrating based on a protocol rather than dependencies

Posted Sep 30, 2024 7:27 UTC (Mon) by LtWorf (subscriber, #124958) [Link] (1 responses)

The "protocol" is a static string to send over a socket.

Thanks for integrating based on a protocol rather than dependencies

Posted Sep 30, 2024 8:40 UTC (Mon) by cortana (subscriber, #24596) [Link]

... which has been documented in sd_notify(3) since systemd v1!

nit

Posted Sep 28, 2024 16:55 UTC (Sat) by intelfx (subscriber, #130118) [Link] (3 responses)

> We would have done it sooner if there was a published specification for the interface, but documentation on how to interact with systemd outside the actual implementation was lacking.

This is pretty disingenuous.

The sd_notify interface was stable since it was first ever documented in 2010, and systemd had *always* offered a standalone implementation of that protocol that was supposed to be imported into other projects's sources.

nit

Posted Sep 28, 2024 19:53 UTC (Sat) by atnot (subscriber, #124910) [Link] (2 responses)

I'm pretty sure that standalone version was in fact added by bluca in response to discussions on this very website?

But nevertheless, I did implement it myself in python around 2020 and don't think I had to try very hard. So clearly it wasn't impossible to figure this out, that seems ahistorical to me. Especially since to my memory the original discussions about integrating sd-notify were rejected primarily on grounds of "double fork is fine" and "systemd bad" rather than some concern for adding one more linker entry being a deal breaker.

nit

Posted Sep 28, 2024 20:01 UTC (Sat) by intelfx (subscriber, #130118) [Link]

> I'm pretty sure that standalone version was in fact added by bluca in response to discussions on this very website?

The sample implementation in the manpage itself — yes, was added very recently.

But even the very first version of the sd_notify(3) manpage was clearly saying that a reference implementation, designed for vendoring into other projects (and thus written to C89 standards without any extensions), was maintained in src/sd-daemon.[ch] within systemd sources (300 SLOC, of which the majority were attributable to other functions that had no bearing on sd_notify() itself and could be left out if not needed).

re: nit

Posted Sep 30, 2024 9:02 UTC (Mon) by grawity (subscriber, #80596) [Link]

No, the entirety of libsystemd used to be standalone from the beginning – it was originally shipped as a self-contained sd-daemon.{c,h} intended to be copy-pasted (whole or in parts) into one's own project, for about a year before it became a shared library in ~2011.

nit

Posted Oct 11, 2024 2:59 UTC (Fri) by turistu (guest, #164830) [Link]

except that the abstract unix domain socket "support" in that patch is purely perfunctory -- i.e. it does not work, and hasn't been tested at all. Why was it added in the first place?

from the linked patch:

if ((path = getenv("NOTIFY_SOCKET")) == NULL || strlen(path) == 0
...
if (strlcpy(addr.sun_path, path,
...
/* Support for abstract socket */
if (addr.sun_path[0] == '@')
	addr.sun_path[0] = 0;
...
if (connect(fd, &addr, sizeof(addr)) != 0) {

Assuming that NOTIFY_SOCKET is "@/foo" that will not connect to the abstract unix socket with the address "\0/foo" (as intended) but to the one with the address "\0/foo\0\0\0...+ other 100 null bytes".

And btw, why doing a connect() + write() on a single-use datagram socket instead of a simple sendto()?

Also, is close(-1) undefined behaviour that should be checked against?

nit

Posted Oct 20, 2024 15:18 UTC (Sun) by hauleth (guest, #174122) [Link]

> but documentation on how to interact with systemd outside the actual implementation was lacking

From what I know it was documented at least since 2019, as at that time I have implemented `sd_notify` in Erlang, and I was using manpages for specification.