Enable the setting of the file mode, owner and group of the UNIX domain control socket

[ac000]

This PR comprises four patches...

1) Add nxt_file_chown().

This adds a wrapper around chown(2). It's slightly long-winded due to using the thread safe versions (getpwnnam_r(3) & getgrnam_r(3)) of getpwnam(3) & getgrnam(3).

2) Allow to set the permissions of the Unix domain control socket.

This allows the user to set the file mode, owner & group of the UNIX domain control socket via three new options; --control-mode, --control-user & --control-group.

Any combination of the above can be used. mode is standard octal format, e.g 660.

3) Docs: Update man page for new --control-* options.

Yeah...

4) Tools: disambiguate unitc control socket detection.

Update to unitc from @lcrilly due to --control now being ambiguous in terms of a unitd options search string.

[hongzhidao]

Allow to set the permissions of the Unix domain control socket.

Just to confirm when to get the control-uid and control-gid? unitd starting or reconfiguration? I mean if the user changes his "user/group" information before reconfiguration, do we need to calculate uid/gid again? The key point is if it makes sense to move getpwnam_r() into nxt_runtime_conf_read_cmd().

[ac000]

On Wed, 08 Nov 2023 18:23:08 -0800 hongzhidao @.***> wrote:

Allow to set the permissions of the Unix domain control socket.

Just to confirm when to get the control-uid and control-gid? unitd starting or reconfiguration?

This is done via command line options at startup... If you want to set them differently then you either

1) Change them via chmod(1)/chown(1)

2) Restart unit with different options.

I mean if the user changes his "user/group" information before reconfiguration, do we need to calculate uid/gid again?

You mean if they change the UID/GID of a username/group?

I'd say that is highly unlikely. These things shouldn't really be changing, doing so can cause all kinds of breakage...

Besides, if they did that they would need to immediately reset the owner/group, not just the next time they did a reconfigure...

I really don't think this is a case worth optimising for...

The key point is if it makes sense to move getpwnam_r() into nxt_runtime_conf_read_cmd().

That would be a much bigger patch as we'd need to have the router process inform the main process about it, like we do for the listen sockets.

If we ever have requests to handle such a scenario we can look at it then.

But the answer is probably use chown(1) for these rarer than once in a blue moon situations... and as I say this would need happen as soon as, not just when a reconfigure is done.

In fact the reconfigure may not even work because the permissions now don't allow it!.

[hongzhidao]

Thanks for the explanation, it makes things clearer.

Does the setting of the file mode only work for control listening which is created by the controller process? If yes, it looks like we don't need to care about the reconfiguration since the controller process starts as the root user, and the listening that happens in the router process doesn't set the mode.

Here's my suggestion.

diff -r e079c44a8340 src/nxt_controller.c
--- a/src/nxt_controller.c      Wed Nov 08 18:37:02 2023 +0000
+++ b/src/nxt_controller.c      Thu Nov 09 14:41:33 2023 +0800
@@ -696,6 +696,8 @@
         const char *path = ls->sockaddr->u.sockaddr_un.sun_path;

         nxt_fs_mkdir_parent((const u_char *) path, 0755);
+
+        /* set file mode */
     }
 #endif

I saw the nxt_file_chown() is put in the nxt_listen_socket_create(), and it reminded me if all the listening would set the file mode, then I asked questions about reconfiguration.

Based on the above, it's ok to put the getpwnam_r() into nxt_file_chown(). And you could consider setting related default values earlier.

    access = rt->control_mode > 0 ? rt->control_mode : S_IRUSR | S_IWUSR;

diff -r e079c44a8340 src/nxt_runtime.c
--- a/src/nxt_runtime.c Wed Nov 08 18:37:02 2023 +0000
+++ b/src/nxt_runtime.c Thu Nov 09 14:46:57 2023 +0800
@@ -790,6 +790,8 @@
     rt->control = NXT_CONTROL_SOCK;
     rt->tmp = NXT_TMPDIR;

+    /* set default value */
+
     nxt_memzero(&rt->capabilities, sizeof(nxt_capabilities_t));

[ac000]

Does the setting of the file mode only work for control listening which is created by the controller process?

If by controller process you mean main process, then yes.

I saw the nxt_file_chown() is put in the nxt_listen_socket_create(), and it reminded me if all the listening would set the file mode, then I asked questions about reconfiguration.

With this config

{
    "listeners": {
        "unix:/tmp/unit-listen.sock": {
            "pass": "routes"
        }
    },

    "routes": [
        {
            "match": {
                "uri": "*"
            },

            "action": {
                  "share": "/tmp"
            }
        }
    ]
}

When starting unit

$ strace -e bind,chmod,chown /opt/unit/sbin/unitd --no-daemon --control-mode 644 --control-user andrew
2023/11/09 12:38:14 [warn] 21186#21186 Unit is running unprivileged, then it cannot use arbitrary user and group.
bind(6, {sa_family=AF_UNIX, sun_path="/opt/unit/control.unit.sock.tmp"}, 34) = 0
chmod("/opt/unit/control.unit.sock.tmp", 0644) = 0
chown("/opt/unit/control.unit.sock.tmp", 1000, -1) = 0
2023/11/09 12:38:14 [info] 21186#21186 unit 1.32.0 started
bind(10, {sa_family=AF_UNIX, sun_path="/tmp/unit-listen.sock"}, 24) = 0
chmod("/tmp/unit-listen.sock", 0666)    = 0

Lets break that down.

bind(6, {sa_family=AF_UNIX, sun_path="/opt/unit/control.unit.sock.tmp"}, 34) = 0

The control socket is created.

chmod("/opt/unit/control.unit.sock.tmp", 0644) = 0
chown("/opt/unit/control.unit.sock.tmp", 1000, -1) = 0

We set the permissions as provided on the command line...

bind(10, {sa_family=AF_UNIX, sun_path="/tmp/unit-listen.sock"}, 24) = 0

The listen socket is created.

chmod("/tmp/unit-listen.sock", 0666)    = 0

Its open all access permissions are set.

So no, it doesn't effect the listening socket. I simply set the given permissions on the control socket at the same location where we set the default ones, which will be used if not specified when starting unit.

Just in case there is any doubt about the above...

$ ls -l /tmp/unit-listen.sock /opt/unit/control.unit.sock 
srw-r--r-- 1 andrew andrew 0 Nov  9 12:34 /opt/unit/control.unit.sock
srw-rw-rw- 1 andrew andrew 0 Nov  9 12:34 /tmp/unit-listen.sock

So it looks right to me...

[hongzhidao]

If by controller process you mean main process, then yes.

The controller process is spawned by the main process with the root user.

$ ls -l /tmp/unit-listen.sock /opt/unit/control.unit.sock srw-r--r-- 1 andrew andrew 0 Nov 9 12:34 /opt/unit/control.unit.sock srw-rw-rw- 1 andrew andrew 0 Nov 9 12:34 /tmp/unit-listen.sock

It looks it the function of setting the file mode applies to the UNIX controller socket and the UNIX listening socket in the configuration, correct?

Of course these only have an affect when using a UNIX Domain Socket for the control socket.

Do we need to specify the listening sockets in the configuration?

[ac000]

If by controller process you mean main process, then yes.

The controller process is spawned by the main process with the root user.

Then from the strace output. No, the control socket is NOT created by the controller process, but by the main process.

$ ls -l /tmp/unit-listen.sock /opt/unit/control.unit.sock srw-r--r-- 1 andrew andrew 0 Nov 9 12:34 /opt/unit/control.unit.sock srw-rw-rw- 1 andrew andrew 0 Nov 9 12:34 /tmp/unit-listen.sock

It looks it the function of setting the file mode applies to the UNIX controller socket and the UNIX listening socket in the configuration, correct?

No.

The listen socket permissions where not effect by the --control-* options.

You can see the listen socket is mode 666 not 644 and you can see there was no chown(2) on it...

[hongzhidao]

Of course these only have an affect when using a UNIX Domain Socket for the control socket.

So it only works for the UNIX control socket. It does nothing on the listening sockets related to configuration. It's about the requirement. A related issue discussed the requirement. https://github.com/nginx/unit/issues/840

You can see the listen socket is mode 666 not 644 and you can see there was no chown(2) on it...

Yep, the nxt_file_chown() isn't called for the listening socket from the configuration.

A few understanding related to source code, feel free to read it.

1. The controller process is spawned by the main process with the root user.
2. All the listening sockets are created by the main process, including the control socket and listening sockets from the configuration.
3. The nxt_listen_socket_create() is called for the control socket. And nxt_main_listening_socket() is called for the listening sockets in the configuration.

So it's ok to call nxt_file_chown() in the nxt_listen_socket_create(), and you could consider putting it in nxt_controller.c.

diff -r e079c44a8340 src/nxt_controller.c
--- a/src/nxt_controller.c      Wed Nov 08 18:37:02 2023 +0000
+++ b/src/nxt_controller.c      Thu Nov 09 23:05:13 2023 +0800
@@ -696,6 +696,8 @@
         const char *path = ls->sockaddr->u.sockaddr_un.sun_path;

         nxt_fs_mkdir_parent((const u_char *) path, 0755);
+
+        /* call nxt_file_chown() */
     }
 #endif

Since before we added related setting mode for the parent directory, it makes sense to add this together. But both are fine anyway.

[ac000]

So it only works for the UNIX control socket. It does nothing on the listening sockets related to configuration. It's about the requirement. A related issue discussed the requirement. https://github.com/nginx/unit/issues/840

That's right. This is only about the control socket which is normally very restricted.

[ac000]

A related issue discussed the requirement. https://github.com/nginx/unit/issues/840

Yes, I'm well aware of that. In fact if you read the commit messages you will see that.

[ac000]

I think it makes sense to do it where we currently do it. I see no reason to either do the default chown(2) in one place and the new stuff in another, or to move them all to some other place. Doing it at the same place we create the socket seems to be the logical thing.

diff -r e079c44a8340 src/nxt_controller.c --- a/src/nxt_controller.c Wed Nov 08 18:37:02 2023 +0000 +++ b/src/nxt_controller.c Thu Nov 09 23:05:13 2023 +0800 @@ -696,6 +696,8 @@ const char *path = ls->sockaddr->u.sockaddr_un.sun_path;

     nxt_fs_mkdir_parent((const u_char *) path, 0755);

+

• / call nxt_file_chown() / }

  endif

Since before we added related setting mode for the parent directory, it makes sense to add this together. But both are fine anyway.

The socket does not exist at the above stage. That's simply creating the directory for it.

[ac000]

Bail out of nxt_file_chown() if both owner and group are NULL.

Range diff

1:  9b054b65 ! 1:  0b7156c3 Add nxt_file_chown().
    @@ src/nxt_file.c: nxt_file_set_access(nxt_file_name_t *name, nxt_file_access_t acc
     +    char   *buf;
     +    long   bufsize;
     +    gid_t  gid = ~0;
     +    uid_t  uid = ~0;
     +
    ++    if (owner == NULL && group == NULL) {
    ++        return NXT_OK;
    ++    }
    ++
     +    if (owner != NULL) {
     +        struct passwd  pwd, *result;
     +
     +        bufsize = sysconf(_SC_GETPW_R_SIZE_MAX);
     +        if (bufsize == -1) {
2:  3a594f6c = 2:  c000fab9 Allow to set the permissions of the Unix domain control socket.
3:  3db87693 = 3:  af7d2047 Docs: Update man page for new --control-* options.
4:  6842f649 = 4:  73a84ec6 Tools: disambiguate unitc control socket detection.

[ac000]

Remove extraneous '.'s from summary lines.

$ git range-diff 73a84ec6...c3d602d7
1:  0b7156c3 ! 1:  5543c299 Add nxt_file_chown().
    @@
      ## Metadata ##
     Author: Andrew Clayton <a.clayton@nginx.com>

      ## Commit message ##
    -    Add nxt_file_chown().
    +    Add nxt_file_chown()

         This wraps chown(2) but takes the user/owner and group as strings.

         It's a little long winded as it uses the thread safe versions of
         getpwnam()/getgrname() which require a little more work.
2:  c000fab9 ! 2:  b5a5a969 Allow to set the permissions of the Unix domain control socket.
    @@
      ## Metadata ##
     Author: Andrew Clayton <a.clayton@nginx.com>

      ## Commit message ##
    -    Allow to set the permissions of the Unix domain control socket.
    +    Allow to set the permissions of the Unix domain control socket

         Several users in GitHub have asked for the ability to set the
         permissions of the unitd UNIX Domain control socket.

         This can of course be done externally, but can be done much cleaner by
3:  af7d2047 ! 3:  550fbed5 Docs: Update man page for new --control-* options.
    @@
      ## Metadata ##
     Author: Andrew Clayton <a.clayton@nginx.com>

      ## Commit message ##
    -    Docs: Update man page for new --control-* options.
    +    Docs: Update man page for new --control-* options

         Signed-off-by: Andrew Clayton <a.clayton@nginx.com>

      ## docs/man/man8/unitd.8.in ##
     @@
4:  73a84ec6 ! 4:  c3d602d7 Tools: disambiguate unitc control socket detection.
    @@
      ## Metadata ##
     Author: Liam Crilly <liam.crilly@nginx.com>

      ## Commit message ##
    -    Tools: disambiguate unitc control socket detection.
    +    Tools: disambiguate unitc control socket detection

         Now that unitd has multiple --control* startup options, locating the
         address of the control socket requires additional precision.

         Signed-off-by: Liam Crilly <liam.crilly@nginx.com>

[lcrilly]

Adding this here before I forget about it, and in case it's helpful for the documentation.

This feature allows an unprivileged user to configure a vanilla installation of Unit. Change the systemd(1) configuration for Unit thus

sudo echo "DAEMON_ARGS=\"--control-group `id -gn` --control-mode 660\"' > /etc/default/unit

Looking forward to this arriving in the next release.

[ac000]

Removed <> from Closes: tag so GH recognises it.

$ git range-diff c3d602d7...bc69333d
1:  b5a5a969 ! 1:  d08a29ef Allow to set the permissions of the Unix domain control socket
    @@ Commit message
         Requested-by: chopanovv <https://github.com/chopanovv>
         Link: <https://github.com/nginx/unit/issues/254>
         Link: <https://github.com/nginx/unit/issues/980>
    -    Closes: <https://github.com/nginx/unit/issues/840>
    +    Closes: https://github.com/nginx/unit/issues/840
         Tested-by: Liam Crilly <liam.crilly@nginx.com>
         Signed-off-by: Andrew Clayton <a.clayton@nginx.com>

2:  550fbed5 = 2:  435a8736 Docs: Update man page for new --control-* options
3:  c3d602d7 = 3:  bc69333d Tools: disambiguate unitc control socket detection

[ac000]

Fix up some of the error messages in nxt_file_chown()

• In nxt_sprintf() long is printed with only %l
• Display errno information

Expand on the commit message that introduces nxt_file_chown()

$ git range-diff bc69333d...f55f5b48
1:  5543c299 ! 1:  16f2a7e3 Add nxt_file_chown()
    @@ Commit message
         This function will be used by the following commit that allows to set
         the permissions of the Unix domain control socket.

    -    We need to cast uid & gid to long in nxt_thread_log_alert() to appease
    -    clang-ast as it's adamant that uid/gid are unsigned ints, but chown(2)
    -    takes -1 for these values and it'd be nice to show them in the error
    -    message.
    +    We need to cast uid & gid to long in the call to nxt_thread_log_alert()
    +    to appease clang-ast as it's adamant that uid/gid are unsigned ints, but
    +    chown(2) takes -1 for these values to indicate don't change this item,
    +    and it'd be nice to show them in the error message.
    +
    +    Note that getpwnam()/getgrname() don't define "not found" as an error as
    +    per their man page
    +
    +      The  formulation given above under "RETURN VALUE" is from POSIX.1-2001.
    +      It does not call "not found" an error, and hence does not specify  what
    +      value errno might have in this situation.  But that makes it impossible
    +      to  recognize  errors.   One  might argue that according to POSIX errno
    +      should be left unchanged if an entry is not found.  Experiments on var‐
    +      ious UNIX-like systems show that lots of different values occur in this
    +      situation: 0, ENOENT, EBADF, ESRCH, EWOULDBLOCK,  EPERM,  and  probably
    +      others.
    +
    +    Thus if we log an error from these functions we can end up with the
    +    slightly humorous error message
    +
    +      2024/02/12 15:15:12 [alert] 99404#99404 getpwnam_r("noddy", ...) failed (0: Success) (User not found) while creating listening socket on unix:/opt/unit/control.unit.sock

         Signed-off-by: Andrew Clayton <a.clayton@nginx.com>

    @@ src/nxt_file.c: nxt_file_set_access(nxt_file_name_t *name, nxt_file_access_t acc
     +
     +        err = getpwnam_r(owner, &pwd, buf, bufsize, &result);
     +        if (result == NULL) {
    -+            nxt_thread_log_alert("getpwnam_r(\"%s\", ...) failed.%s",
    -+                                 owner, err == 0 ? " User not found" : "");
    ++            nxt_thread_log_alert("getpwnam_r(\"%s\", ...) failed %E %s",
    ++                                 owner, nxt_errno,
    ++                                 err == 0 ? "(User not found)" : "");
     +            goto out_err_free;
     +        }
     +
    @@ src/nxt_file.c: nxt_file_set_access(nxt_file_name_t *name, nxt_file_access_t acc
     +
     +        err = getgrnam_r(group, &grp, buf, bufsize, &result);
     +        if (result == NULL) {
    -+            nxt_thread_log_alert("getgrnam_r(\"%s\", ...) failed.%s",
    -+                                 group, err == 0 ? " Group not found" : "");
    ++            nxt_thread_log_alert("getgrnam_r(\"%s\", ...) failed %E %s",
    ++                                 group, nxt_errno,
    ++                                 err == 0 ? "(Group not found)" : "");
     +            goto out_err_free;
     +        }
     +
    @@ src/nxt_file.c: nxt_file_set_access(nxt_file_name_t *name, nxt_file_access_t acc
     +        return NXT_OK;
     +    }
     +
    -+    nxt_thread_log_alert("chown(\"%FN\", %ld, %ld) failed %E", name,
    ++    nxt_thread_log_alert("chown(\"%FN\", %l, %l) failed %E", name,
     +                         owner != NULL ? (long)uid : -1,
     +                         group != NULL ? (long)gid : -1, nxt_errno);
     +
2:  d08a29ef = 2:  ece8b9cb Allow to set the permissions of the Unix domain control socket
3:  435a8736 = 3:  90eeb067 Docs: Update man page for new --control-* options
4:  bc69333d = 4:  f55f5b48 Tools: disambiguate unitc control socket detection

[ac000]

@hongzhidao

Just to clarify and for the avoidance of doubt.

These patches are purely about being able to set the user/group and permissions on the UNIX domain control socket at start up. The listening sockets are not effected.

[ac000]

• Add a space after some casts
• Fix typo
• Align the --control-{user,group} options help text

$ git range-diff f55f5b48...21c4046d
1:  16f2a7e3 ! 1:  72b6d7d4 Add nxt_file_chown()
    @@ src/nxt_file.c: nxt_file_set_access(nxt_file_name_t *name, nxt_file_access_t acc
     +        nxt_free(buf);
     +    }
     +
    -+    if (nxt_fast_path(chown((const char *)name, uid, gid) == 0)) {
    ++    if (nxt_fast_path(chown((const char *) name, uid, gid) == 0)) {
     +        return NXT_OK;
     +    }
     +
     +    nxt_thread_log_alert("chown(\"%FN\", %l, %l) failed %E", name,
    -+                         owner != NULL ? (long)uid : -1,
    -+                         group != NULL ? (long)gid : -1, nxt_errno);
    ++                         owner != NULL ? (long) uid : -1,
    ++                         group != NULL ? (long) gid : -1, nxt_errno);
     +
     +    return NXT_ERROR;
     +
2:  ece8b9cb ! 2:  57bfe39d Allow to set the permissions of the Unix domain control socket
    @@ src/nxt_runtime.c: nxt_runtime_conf_read_cmd(nxt_task_t *task, nxt_runtime_t *rt
     +        "  --control-mode MODE  set mode of the control API socket\n"
     +        "                       default: 0600\n"
     +        "\n"
    -+        "  --control-user USER  set the owner of the control API socket\n"
    ++        "  --control-user USER    set the owner of the control API socket\n"
     +        "\n"
    -+        "  --control-group GROUP  set the greup of the control API socket\n"
    ++        "  --control-group GROUP  set the group of the control API socket\n"
     +        "\n"
              "  --pid FILE           set pid filename\n"
              "                       default: \"" NXT_PID "\"\n"
3:  90eeb067 = 3:  e8d05e60 Docs: Update man page for new --control-* options
4:  f55f5b48 = 4:  21c4046d Tools: disambiguate unitc control socket detection

[ac000]

Thanks @hongzhidao

[ac000]

• Rebase with master
• Add some Reviewed-by tags

$ git range-diff 5f5e9fb1...1dca8602
1:  52b38264 ! 1:  34b3a812 Add nxt_file_chown()
    @@ Commit message

           2024/02/12 15:15:12 [alert] 99404#99404 getpwnam_r("noddy", ...) failed (0: Success) (User not found) while creating listening socket on unix:/opt/unit/control.unit.sock

    +    Reviewed-by: Zhidao Hong <z.hong@f5.com>
         Signed-off-by: Andrew Clayton <a.clayton@nginx.com>

      ## src/nxt_file.c ##
2:  0700c71c ! 2:  b500c36d Allow to set the permissions of the Unix domain control socket
    @@ Commit message
         Link: <https://github.com/nginx/unit/issues/980>
         Closes: https://github.com/nginx/unit/issues/840
         Tested-by: Liam Crilly <liam.crilly@nginx.com>
    +    Reviewed-by: Zhidao Hong <z.hong@f5.com>
         Signed-off-by: Andrew Clayton <a.clayton@nginx.com>

      ## src/nxt_listen_socket.c ##
3:  d685a970 = 3:  2bd3b418 Docs: Update man page for new --control-* options
4:  5f5e9fb1 = 4:  1dca8602 Tools: disambiguate unitc control socket detection