Skip to content

Allow UID/GID in ICINGA2_(USER|GROUP) environment variables #10538

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

Merged
merged 1 commit into from
Aug 27, 2025

Conversation

jschmidt-icinga
Copy link
Contributor

@jschmidt-icinga jschmidt-icinga commented Aug 14, 2025

refs #10307.
Closes #10308.
Closes #10321.

This is the third attempt at a PR that fixes this issue.

The approach in #10308 wasn't enough, because it still required an existing group and didn't allow to drop privileges from root to a user that's not in the database. Also it didn't account for other places that required valid users/groups, mostly the Utility::SetFileOwnership() function which is used in the node setup command which is especially relevant for containers.

@oxzi's solution in #10321 added a new command line argument that allowed to explicitly bypass dropping the permissions for the given command. This had the advantage over #10308 that it reliably worked for both user and group, but it also didn't allow dropping privileges (which to be fair, was the point) and also didn't account for the Utility function.

My solution allows to specify UID and GID in the ICINGA2_USER and ICINGA2_GROUP environment variables that override the defaults (corresponding to the same environment variables) set at build time. All the user has to do is additionally set these environment variables to the desired UID/GID combination and everything else should just work as expected. If the process is run as root, privileges should drop to the specified ids, if the process starts with these ids, they're validated against the environment variables and nothing else is done/attempted.

This issue got renewed interest with #10505, where users like me preferred to set custom UIDs/GIDs in their docker-compose.yml files for development to easier mount configuration directories. Even with this PR this is not easily possible without the ARG ICINGA_USER_ID, because the /data directory gets set up with ownership for the icinga user and its default UID/GID. But it's at least a step closer and maybe it would work with a mounted and manually set up /data directory.

@jschmidt-icinga jschmidt-icinga added this to the 2.16.0 milestone Aug 14, 2025
@jschmidt-icinga jschmidt-icinga self-assigned this Aug 14, 2025
@jschmidt-icinga jschmidt-icinga added enhancement New feature or request area/cli Command line helpers labels Aug 14, 2025
@cla-bot cla-bot bot added the cla/signed label Aug 14, 2025
@jschmidt-icinga jschmidt-icinga force-pushed the allow-uid-gid-icinga-user-and-group branch from f89ea15 to 674d7c5 Compare August 15, 2025 07:28
Copy link
Member

@oxzi oxzi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot for this PR. I have skimmed over it and like its gist, as it is simpler than the previous two approaches.

I have not tested the code, but only read the diff together with the relevant libc and system call man pages. In general, it seems fine. Please address my comments and feel free to tell me if my objections are irrelevant.

Would you please let GitHub close these other two PRs by adding "Closes #10308" and "Closes #10321" to the top message?

@jschmidt-icinga jschmidt-icinga force-pushed the allow-uid-gid-icinga-user-and-group branch 2 times, most recently from a6901fe to e0ca82e Compare August 19, 2025 07:38
@oxzi oxzi self-requested a review August 19, 2025 08:16
Copy link
Member

@oxzi oxzi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

$ icinga2 --version | head -n1
icinga2 - The Icinga 2 network monitoring daemon (version: v2.15.0-64-ge0ca82ec3)

All mighty root user, correct icinga user, "unprivileged" nobody, a non-existing UID, and a non-existing user.

$ id
uid=0(root) gid=0(root) groups=0(root)
$ id icinga
uid=5665(icinga) gid=5665(icinga) groups=5665(icinga)
$ id nobody
uid=65534(nobody) gid=65534(nogroup) groups=65534(nogroup)
$ id 2323
id: '2323': no such user
$ id nope
id: 'nope': no such user

Config validation works as root or icinga - both via names or IDs.

$ icinga2 daemon -C                                                                                                                             10:29:21 [113/633]
[2025-08-19 08:28:37 +0000] information/cli: Icinga application loader (version: v2.15.0-64-ge0ca82ec3)
[ . . . ]
[2025-08-19 08:28:37 +0000] information/cli: Finished validating the configuration file(s).
$ ICINGA2_USER=root ICINGA2_GROUP=root icinga2 daemon -C
[2025-08-19 08:28:41 +0000] information/cli: Icinga application loader (version: v2.15.0-64-ge0ca82ec3)
[ . . . ]
[2025-08-19 08:28:41 +0000] information/cli: Finished validating the configuration file(s).
$ ICINGA2_USER=0 ICINGA2_GROUP=0 icinga2 daemon -C
[2025-08-19 08:28:46 +0000] information/cli: Icinga application loader (version: v2.15.0-64-ge0ca82ec3)
[ . . . ]
[2025-08-19 08:28:46 +0000] information/cli: Finished validating the configuration file(s).
$ ICINGA2_USER=icinga ICINGA2_GROUP=icinga icinga2 daemon -C
[2025-08-19 08:29:02 +0000] information/cli: Icinga application loader (version: v2.15.0-64-ge0ca82ec3)
[ . . . ]
[2025-08-19 08:29:03 +0000] information/cli: Finished validating the configuration file(s).
$ ICINGA2_USER=5665 ICINGA2_GROUP=5665 icinga2 daemon -C
[2025-08-19 08:29:12 +0000] information/cli: Icinga application loader (version: v2.15.0-64-ge0ca82ec3)
[ . . . ]
[2025-08-19 08:29:12 +0000] information/cli: Finished validating the configuration file(s).

nobody is a valid user, but lacks permissions.

$ ICINGA2_USER=nobody ICINGA2_GROUP=nogroup icinga2 daemon -C
[2025-08-19 08:32:43 +0000] information/cli: Icinga application loader (version: v2.15.0-64-ge0ca82ec3)
[ . . . ]
[2025-08-19 08:32:43 +0000] critical/cli: Could not write vars file: Error: Function call 'mkstemp' for file '/var/cache/icinga2/icinga2.vars.tmp.3iK5av' failed with error code 13, 'Permission denied'
$ ICINGA2_USER=65534 ICINGA2_GROUP=65534 icinga2 daemon -C
[2025-08-19 08:33:00 +0000] information/cli: Icinga application loader (version: v2.15.0-64-ge0ca82ec3)
[ . . . ]
[2025-08-19 08:33:00 +0000] critical/cli: Could not write vars file: Error: Function call 'mkstemp' for file '/var/cache/icinga2/icinga2.vars.tmp.Sm0n6B' failed with error code 13, 'Permission denied'

The following would run as the unknown user 2323 and group 2323. While looking up none:none would fail - as expected.

$ ICINGA2_USER=2323 ICINGA2_GROUP=2323 icinga2 daemon -C
[2025-08-19 08:35:06 +0000] information/cli: Icinga application loader (version: v2.15.0-64-ge0ca82ec3)
[2025-08-19 08:35:06 +0000] critical/cli: Could not write vars file: Error: Function call 'mkstemp' for file '/var/cache/icinga2/icinga2.vars.tmp.ty0k7p' failed with error code 13, 'Permission denied'
$ ICINGA2_USER=nope ICINGA2_GROUP=nope icinga2 daemon -C
critical/cli: Invalid group specified: nope

Let's try big numbers :)

$ # 2**31
$ ICINGA2_USER=2147483648 ICINGA2_GROUP=2147483648 icinga2 daemon -C
[2025-08-19 08:42:09 +0000] information/cli: Icinga application loader (version: v2.15.0-64-ge0ca82ec3)
[ . . . ]
[2025-08-19 08:42:09 +0000] critical/cli: Could not write vars file: Error: Function call 'mkstemp' for file '/var/cache/icinga2/icinga2.vars.tmp.H24Sn0' failed with error code 13, 'Permission denied'

$ # 2**32 - 2
$ ICINGA2_USER=4294967294 ICINGA2_GROUP=4294967294 icinga2 daemon -C
[2025-08-19 08:43:42 +0000] information/cli: Icinga application loader (version: v2.15.0-64-ge0ca82ec3)
[ . . . ]
[2025-08-19 08:43:43 +0000] critical/cli: Could not write vars file: Error: Function call 'mkstemp' for file '/var/cache/icinga2/icinga2.vars.tmp.pnVVnu' failed with error code 13, 'Permission denied'

$ # 2**32 - 1
$ ICINGA2_USER=4294967295 ICINGA2_GROUP=4294967295 icinga2 daemon -C
critical/cli: setgid() failed with error code 22, "Invalid argument"
$ $ ICINGA2_USER=4294967295 icinga2 daemon -C
critical/cli: setuid() failed with error code 22, "Invalid argument"
critical/cli: Please re-run this command as a privileged user or using the "4294967295" account.

$ # 2**32
$ ICINGA2_USER=4294967296 ICINGA2_GROUP=4294967296 icinga2 daemon -C
critical/cli: Invalid group specified: 4294967296

Consider the special errors for $2^{32}-1$, where an error directly emerges from setgid(3) or setuid(3).


In general, looks good to me! But it would be nice to document the UID/GID support, e.g., by adding the information to the System Environment list or even adding this somewhere to the Icinga 2 CLI Commands section.

@jschmidt-icinga jschmidt-icinga force-pushed the allow-uid-gid-icinga-user-and-group branch from e0ca82e to 5b52699 Compare August 19, 2025 12:02
@jschmidt-icinga
Copy link
Contributor Author

jschmidt-icinga commented Aug 19, 2025

@oxzi Thanks for testing with the different UID/GID values.

I've added the comment and also switched to using boost::lexical_cast directly in Utility::SetFileOwnership(). I've also added a reference to user/group-id being an option to the documentation of the environment variables.

However, I'm not sure where I should document this "feature" and what I should write in 11-cli-commands.md. If you had anything specific in mind I'm happy to do so, but otherwise I think allowing ids is more intuitive than not allowing them, and simply mentioning that it's possible should be sufficient documentation.

@jschmidt-icinga jschmidt-icinga force-pushed the allow-uid-gid-icinga-user-and-group branch 2 times, most recently from e0898af to 6ea0275 Compare August 19, 2025 14:51
@jschmidt-icinga jschmidt-icinga force-pushed the allow-uid-gid-icinga-user-and-group branch from 6ea0275 to 7689f94 Compare August 20, 2025 11:14
@jschmidt-icinga
Copy link
Contributor Author

  • Moved setting the uid from pw inside the catch block as suggested to improve readability
  • Added the error message to the branch where it was missing. It was probably a mistake that this one branch was missing the error message.
  • Changed "re-run" to "rerun", even though the linked dictionary page seems to indicate the usage to be for TV reruns exclusively, other dictionaries indicate that "rerun" should also be correct in this case.

oxzi
oxzi previously approved these changes Aug 21, 2025
Copy link
Member

@oxzi oxzi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thanks and please excuse the multiple feedback rounds. However, someone more fluent in C++ should take a final look.

I have added close references to the OP for #10308 and #10321.

@jschmidt-icinga
Copy link
Contributor Author

I have added close references to the OP for #10308 and #10321.

I'm curious to see if it works. I always assumed closing other PRs that way isn't possible.

@jschmidt-icinga
Copy link
Contributor Author

  • Now catching boost::bad_lexical_cast by name.
  • Simplified if condition as requested.

oxzi
oxzi previously approved these changes Aug 22, 2025
Copy link
Member

@oxzi oxzi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested once again, seems to work as expected! Thanks.

Copy link
Contributor

@julianbrost julianbrost left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Small detail, after I've seen it, it can't be unseen 🙈

Copy link
Member

@yhabteab yhabteab left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LFTM!

Copy link
Member

@oxzi oxzi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

However, we should probably check with the reporter to see if this actually resolves the issue on OpenShift. @julianbrost just asked him yesterday, #10307 (comment). At least from my understanding, this should work.

@julianbrost
Copy link
Contributor

I'm going to merge this given that accepting both names as numeric IDs here isn't a bad idea (similar to how chown also accepts both). With that change, there should be nothing more that prevents an unpatched icinga2 binary from being used in OpenShift, the open question is just if it needs some more plumbing around it (for example in the Dockerfile or entrypoint) to make it actually work nicely.

Fixes #10307.

Hence I've reduced this to a reference so that the issue for OpenShift remains open until we have a confirmation that this is actually enough.

Closes #10308.
Closes #10321.

However, even if it isn't, I don't think we would want to revive any of these PRs (i.e. they shall be closed) but rather doing something like setting these variables inside the entrypoint.

@julianbrost julianbrost merged commit 0c2fd00 into master Aug 27, 2025
30 checks passed
@julianbrost julianbrost deleted the allow-uid-gid-icinga-user-and-group branch August 27, 2025 09:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/cli Command line helpers cla/signed enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants