UNIX.
agent and server.

Specification: https://www.zabbix.org/wiki/Docs/specs/ZBXNEXT-453

Feature implemented in svn://svn.zabbix.com/branches/dev/ZBXNEXT-453

(1) "User" parameter was not added to zbx_load_config() in proxy.c.

jurism Parameter added. RESOLVED.

asaveljevs CLOSED.

(2) Regarding configuration file updates:

• Section for AllowRoot mentions user 'zabbix'. It should probably mention the new User configuration directive.
• In User section, I propose to replace "run as" with "started by", so as to distinguish between the startup phase and running phase (this is similar to AllowRoot documentation).
• None of the configuration files should mention Windows. Server and proxy configuration files because server and proxy do not work on Windows, and zabbix_agentd.conf files because there is a separate example file for Windows - zabbix_agentd.win.conf.

jurism

• AllowRoot now describes the fall back onto the User parameter.
• "run as" replaced by "launched by".
• All references to the Windows platform have been removed.

RESOLVED.

asaveljevs Documentation for AllowRoot currently says: "If disabled and the server is started by 'root', the server will try to switch to user 'zabbix' or, if provided, the user specified by the User configuration option instead." How about omitting the "user 'zabbix' or, if provided, " part? Since 'zabbix' is not the hardcoded value anymore, but the default value for User, it might be more correct.

asaveljevs I still have the desire to unify things like "started under a regular user" in AllowRoot documentation, "run as non-root" and "launching user" in User. REOPENED.

jurism The documentation has been updated accordingly. RESOLVED.

asaveljevs CLOSED.

(3) The User directive does not seem to default to "zabbix" anywhere.

jurism The User directive defaults to "zabbix" now only if no User parameter is passed (i.e. the value is NULL). If the User parameter is defined but is an empty string, it is considered a valid parameter definition and the error is reported to the user, followed by termination of the process. RESOLVED.

asaveljevs In daemon_start() we have the following:

usr = zbx_strdup(NULL, (NULL != user) ? user : "zabbix");

Memory allocated for the string is not freed. In this case the following suffices (no need to introduce a separate variable):

if (NULL == user)
	user = "zabbix";

Also, note that User should only default to "zabbix" if we are started by root. If we are started by a regular user and User is not specified, we should run as that regular user.

REOPENED

jurism The code has been adjusted and your propositions have been implemented. RESOLVED.

asaveljevs CLOSED.

(4) In daemon.c, you seem to have lost "0 == getgid()" condition.

jurism
The condition was purposefully discarded.
Previously, the condition was of the following form: if (0 == getuid() || 0 == getgid()).
This condition only succeeds if a) the user is superuser, with a UID of 0 or b) the user does not have superuser privileges (is not UID 0) but belongs to the superuser group. C condition evaluation returns as soon as an unambiguous result is determined, in the case of OR logic that being truth-hood. So, even if a user is not root, but belongs to a group that has the GID of 0, the condition returns successfully.

This might seem misleading, but judging from my own personal experience, I have encountered FreeBSD systems where system administrators have added their user accounts to the 'wheel' group and have set it as their primary group. This is oftentimes justified by the logic that the user is not a direct superuser but still retains read, but not execution, capabilities of superuser controlled files. In favor of clarity and to avoid some edge case bug reports that stem from system setups similar to the one described, I decided to drop the group id comparison completely.

RESOLVED.

asaveljevs This seems to be a good idea indeed. Even if a regular user belongs to a group with ID 0, then the process is not allowed to change its user ID or group ID. So, even though it was like that since the very beginning, it looks like a bug. sasha confirms, so looks good. CLOSED.

(5) In daemon.c, lines are too long (should not be longer than 120 characters).

You can reduce indentation in one of two ways:

(a) replace

if (condition)
{
        code;
}

with

if (!condition)
        goto label;

code;

(a) replace

if (condition)
{
        code1;
        exit(FAIL);
}
else
        code2;

with

if (condition)
{
        code1;
        exit(FAIL);
}

code2;

jurism All lines exceeding the maximum length of 120 characters have been broken down and split among multiple lines. RESOLVED.

asaveljevs How about splitting the most indented code to a separate function like change_user()?

jurism I have changed the indention on the code but have not split it into a separate function like you proposed. I believe this will keep the the code more readable and the gain in indention that would be achieved is insignificant. RESOLVED.

asaveljevs Specification was changed a bit, so let's leave the discussion for later. WON'T FIX.

(6) In daemon.c, if we are being started by root and User=root, we print zbx_error("cannot escalate privileges to root!").

There are two problems with it. First, if we are started by root, then, strictly speaking, by changing to root we are not escalating privileges.

Second, the specification asks that "root" is not accepted for this option. The intention behind it, as far as I understand, is to make it clear that if a user wants Zabbix daemon to run as root, then he achieves that solely by setting AllowRoot=1.

So maybe an error message like the following would be better, supplemented by documentation in the configuration files:

zbx_error("option User does not accept value \"root\"; use AllowRoot=1 by itself to run as root");

The check might even be moved to zbx_validate_config() function.

jurism Old error message has been replaced with the one proposed by asaveljevs
RESOLVED.

asaveljevs I would still propose to mention in configuration file documentation that specifying "root" for User is not the proper way to run as root. It should be mentioned that if a user wishes to run as root, he does that by setting AllowRoot=1. If AllowRoot=1, then User option is ignored. REOPENED.

jurism A separate line has been added to the documentation of the User parameter that describes the proper way of running the various daemons under root accounts. RESOLVED.

asaveljevs Specification was changed a bit, so let's leave the discussion for later. WON'T FIX.

(7) Please take a look at minor changes in r41368.
jurism Looks great! CLOSED.

The feature has been implemented in svn://svn.zabbix.com/branches/dev/ZBXNEXT-453

We have discussed this with sasha and, since an unprivileged user can never setuid() to another user, we have changed the specification. Please see the updated document at https://www.zabbix.org/wiki/Docs/specs/ZBXNEXT-453 .

The essence of the changes and the expected behavior is now as follows:

• if we are started by root and AllowRoot=1, User is ignored;
• if we are started by root and AllowRoot=0, User is used, but it cannot equal root;
• if we are started by a regular user, User is ignored.

In the above, root is defined as someone having user ID of 0, not someone with user name "root".

jurism Behavior implemented according to the current version of the specification. User parameter is now only taken into account if daemons are launched by the superuser. RESOLVED.

asaveljevs I have adjusted the code a bit as follows:

• r41548 reverts daemon.c back to how it was in the beginning of this branch and applies only changes that are necessary for this feature request;
• r41549 updated documentation to reflect specification changes.

Please see whether these changes are OK. RESOLVED.

jurism Changes reviewed and fit the latest specification. Looks good. CLOSED.

Implemented in pre-2.3.0 (trunk) r41554.