-
Notifications
You must be signed in to change notification settings - Fork 327
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
Enable the setting of the file mode, owner and group of the UNIX domain control socket #1000
Conversation
Just to confirm when to get the control-uid and control-gid? |
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!.
|
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? Here's my suggestion.
I saw the Based on the above, it's ok to put the
|
If by
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.
The
We set the permissions as provided on the command line...
The
Its open all access permissions are set. So no, it doesn't effect the listening socket. I simply set the given permissions on the 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... |
The controller process is spawned by the main process with the root user.
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?
Do we need to specify the listening sockets in the configuration? |
Then from the strace output. No, the control socket is NOT created by the controller process, but by the main process.
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 |
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. #840
Yep, the A few understanding related to source code, feel free to read it.
So it's ok to call
Since before we added related setting mode for the parent directory, it makes sense to add this together. But both are fine anyway. |
That's right. This is only about the control socket which is normally very restricted. |
Yes, I'm well aware of that. In fact if you read the commit messages you will see that. |
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.
The socket does not exist at the above stage. That's simply creating the directory for it. |
6842f64
to
73a84ec
Compare
Bail out of nxt_file_chown() if both owner and group are NULL. Range diff
|
73a84ec
to
c3d602d
Compare
Remove extraneous '.'s from summary lines.
|
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. |
c3d602d
to
bc69333
Compare
Removed <> from Closes: tag so GH recognises it.
|
bc69333
to
f55f5b4
Compare
Fix up some of the error messages in nxt_file_chown()
Expand on the commit message that introduces nxt_file_chown()
|
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. |
f55f5b4
to
21c4046
Compare
|
Thanks @hongzhidao |
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. 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 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 Reviewed-by: Zhidao Hong <z.hong@f5.com> Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
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 Unit itself. This commit adds three new options --control-mode Set the mode of the socket, e.g 644 --control-user Set the user/owner of the socket, e.g unit --control-group Set the group of the socket, e.g unit Of course these only have an affect when using a UNIX Domain Socket for the control socket. Requested-by: michaelkosir <https://github.com/michaelkosir> Requested-by: chopanovv <https://github.com/chopanovv> Link: <nginx#254> Link: <nginx#980> Closes: nginx#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>
Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
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> Acked-by: Andrew Clayton <a.clayton@nginx.com> Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
21c4046
to
1dca860
Compare
|
This PR comprises four patches...
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).
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.g660
.Yeah...
Update to
unitc
from @lcrilly due to--control
now being ambiguous in terms of a unitd options search string.