[systemd-devel] [PATCH] core/socket: Add REMOTE_IP environment variable for Accept=true

[Zbigniew Jędrzejewski-Szmek]

On Wed, Mar 04, 2015 at 03:15:02PM -0800, Shawn Landden wrote:
> Fix handling of abstract unix domain sockets too.
Please split it into two patches.

> ---
>  TODO                     |  2 --
>  man/systemd.socket.xml   |  5 ++++-
>  src/core/service.c       | 24 ++++++++++++++++++++++++
>  src/shared/socket-util.c | 25 +++++++++++++++++++------
>  4 files changed, 47 insertions(+), 9 deletions(-)
> 
> diff --git a/TODO b/TODO
> index ae32388..780084a 100644
> --- a/TODO
> +++ b/TODO
> @@ -164,8 +164,6 @@ Features:
>  * as soon as we have kdbus, and sender timestamps, revisit coalescing multiple parallel daemon reloads:
>    http://lists.freedesktop.org/archives/systemd-devel/2014-December/025862.html
>  
> -* set $REMOTE_IP (or $REMOTE_ADDR/$REMOTE_PORT) environment variable when doing per-connection socket activation. use format introduced by xinetd or CGI for this
> -
>  * the install state probably shouldn't get confused by generated units, think dbus1/kdbus compat!
>  
>  * in systemctl list-unit-files: show the install value the presets would suggest for a service in a third column
> diff --git a/man/systemd.socket.xml b/man/systemd.socket.xml
> index 3938345..8796d7b 100644
> --- a/man/systemd.socket.xml
> +++ b/man/systemd.socket.xml
> @@ -357,7 +357,10 @@
>          daemons designed for usage with
>          <citerefentry><refentrytitle>inetd</refentrytitle><manvolnum>8</manvolnum></citerefentry>
>          to work unmodified with systemd socket
> -        activation.</para></listitem>
> +        activation.</para>
> +        <para>For IPv4 and IPv6 connections the <varname>REMOTE_IP</varname>
> +        environment variable will be set with remote IP and port seperated by a
> +        colon (for SOCK_RAW the port is the IP protocol).</para></listitem>
Would be nice to have a note here that say that this is the same as xinetd etc.

>        </varlistentry>
>  
>        <varlistentry>
> diff --git a/src/core/service.c b/src/core/service.c
> index a89ff3f..0942072 100644
> --- a/src/core/service.c
> +++ b/src/core/service.c
> @@ -1119,6 +1119,30 @@ static int service_spawn(
>                          goto fail;
>                  }
>  
> +        if (s->accept_socket.unit) {
> +                union sockaddr_union pn;
> +                socklen_t pnlen = sizeof(pn);
> +                _cleanup_free_ char *remote_addr = NULL;
> +
> +                r = getpeername(s->socket_fd, &pn.sa, &pnlen);
This happens before the fork, right? You cannot call a blocking function
like this in PID 1. This could be called either asynchronously, or
after the fork, in the child thread. The latter seems easier.

> +                if (r < 0) {
> +                        r = -errno;
> +                        goto fail;
> +                }
> +
> +                if (pn.sa.sun_family == AF_INET ||
> +                    pn.sa.sun_family == AF_INET6) {
> +                        r = sockaddr_pretty(&pn.sa, pnlen, true, &remote_addr);
> +                        if (r < 0)
> +                                goto fail;
> +
> +                        if (asprintf(our_env + n_env++, "REMOTE_IP=%s", remote_addr) < 0) {
> +                                r = -ENOMEM;
> +                                goto fail;
> +                        }
> +                }
> +        }
> +
>          final_env = strv_env_merge(2, UNIT(s)->manager->environment, our_env, NULL);
>          if (!final_env) {
>                  r = -ENOMEM;
> diff --git a/src/shared/socket-util.c b/src/shared/socket-util.c
> index 74d90fa..dbe2bf7 100644
> --- a/src/shared/socket-util.c
> +++ b/src/shared/socket-util.c
> @@ -522,19 +522,32 @@ int sockaddr_pretty(const struct sockaddr *_sa, socklen_t salen, bool translate_
>                                  return -ENOMEM;
>  
>                  } else if (sa->un.sun_path[0] == 0) {
> +                        void *i;
>                          /* abstract */
>  
> -                        /* FIXME: We assume we can print the
> -                         * socket path here and that it hasn't
> -                         * more than one NUL byte. That is
> -                         * actually an invalid assumption */
> -
> +                        /* see unix(7), abstract is wierd */
> +                        i = memchr(&sa->un.sun_path + 1, '\0', sizeof(sa->un.sun_path) - 1);

Wouldn't it be better to cescape the string? We could say that
socket paths are cunescaped when reading, and cescaped when pretty
printing. Nobody sane uses sockets with zeros in the name, except
by istake, so it wouldn't really matter for normal use, but it would
help catch errors. Seems more useful than punting like this.

> +                        if (i)
> +                                for (i = (char *)i + 1;
> +                                     (char *)i < &sa->un.sun_path[sizeof(sa->un.sun_path)];
> +                                     i = (char *)i + 1)
> +                                        if (*(char *)i != '\0') {
> +                                                p = strdup("<abstract unprintable>");
> +                                                if (!p)
> +                                                        return -ENOMEM;
> +
> +                                                goto end;
> +                                        }
> +
> +                        /* no non-NUL bytes after second NUL (if any) */
>                          p = new(char, sizeof(sa->un.sun_path)+1);
>                          if (!p)
>                                  return -ENOMEM;

Zbyszek