Skip to content
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

Allow setting user for all modules and capabilities per module #100

Merged
merged 17 commits into from
Dec 7, 2023

Conversation

corneliusclaussen
Copy link
Contributor

When setting system_user="myuser" property under settings in a config file all modules are started as this user and its default gid.

After starting all modules the manager also switches to this user.

For each module, special Linux capabilities can be specified with e.g. capabilities: "cap_chown+ep"
in the module section.

The manager will need to start as root or sufficient capabilities to do that. If no user/capabilities are specified no change happens.

@corneliusclaussen corneliusclaussen force-pushed the feature/change_user_and_capabilities branch 4 times, most recently from 737f50b to c733613 Compare September 15, 2023 15:40
@corneliusclaussen
Copy link
Contributor Author

one thing that we also need to fix: if one child cannot be forked (e.g. capabilities config wrong), manager exits but the already forked children are not killed

@a-w50
Copy link
Contributor

a-w50 commented Sep 18, 2023

that is, what PR_SET_PDEATHSIG in the SubProcess class is for, but somehow, this doesn't always work, as it seems

@hikinggrass
Copy link
Contributor

that is, what PR_SET_PDEATHSIG in the SubProcess class is for, but somehow, this doesn't always work, as it seems

I think that's because that flag is cleared when executing setuid, so we will have to implement a workaround along these lines:
https://stackoverflow.com/a/42498370

@a-w50
Copy link
Contributor

a-w50 commented Sep 20, 2023

that is, what PR_SET_PDEATHSIG in the SubProcess class is for, but somehow, this doesn't always work, as it seems

I think that's because that flag is cleared when executing setuid, so we will have to implement a workaround along these lines: https://stackoverflow.com/a/42498370

Good find, I wasn't aware of that yet.

@corneliusclaussen corneliusclaussen force-pushed the feature/change_user_and_capabilities branch 5 times, most recently from a10fd78 to 1237657 Compare October 13, 2023 06:59
@corneliusclaussen corneliusclaussen force-pushed the feature/change_user_and_capabilities branch 3 times, most recently from 8b2da9c to 4f84d83 Compare November 12, 2023 09:34
@golovasteek
Copy link
Contributor

Hi in this PR quite nice refactoring is done, and all system-specific functions are encapsulated in a dedicated file.

Do you think system_linux.cpp would be more appropriate name for it, since the functionality there is rather linux specific and do not work unix-wide (macos, FreeBSD).

Do you think it makes sense to have one header system.hpp that declares all the system-dependent functions. And set of implementations e.g. here would be system_linux.cpp later system_macos.cpp could be added.

@corneliusclaussen
Copy link
Contributor Author

PR_SET_PDEATHSIG is now fixed, it is set after the user change. Also support to switch back to root user in manager for restarting of modules when admin panel/controller is used was added. If admin panel is disabled (which should be done on real deployments) manager does not have the caps to switch back from unprivileged user.

When setting system_user="myuser" property under settings in a config file
all modules are started as this user and its default gid.

After starting all modules the manager also switches to this user.

For each module, special Linux capabilities can be specified with e.g.
capabilities: "cap_chown+ep"
in the module section.

The manager will need to start as root or sufficient capabilities to do
that. If no user/capabilities are specified no change happens.

Implementation changes

- using SECBITS_KEEP_CAP in order to keep capabilities, when changing
  real user id
- using ambient capability set, in order to keep capabilities, when
  execve'ing
- changed name from 'system_user' to 'run_as_user'
- changed schema for capabilities in config file from string to array of
  strings
- dropped 'run_as_user' from ModuleStartInfo, could be added back, when
  configuration is possible
- known bugs:
  - incomplete error handling, when setting of capabilities fails
  - manager process doesn't change user yet (otherwise restarting of
    modules won't work), should use effective user id here
  - controller process doesn't get terminated, when forking process
    fails due to permission problems

Signed-off-by: aw <aw@pionix.de>
Signed-off-by: Cornelius Claussen <cc@pionix.de>

tmp

Signed-off-by: Cornelius Claussen <cc@pionix.de>
Signed-off-by: Cornelius Claussen <cc@pionix.de>
Signed-off-by: Cornelius Claussen <cc@pionix.de>
Signed-off-by: Cornelius Claussen <cc@pionix.de>
Signed-off-by: Cornelius Claussen <cc@pionix.de>
@corneliusclaussen corneliusclaussen force-pushed the feature/change_user_and_capabilities branch from 6a573a5 to cc628b7 Compare December 6, 2023 16:23
src/manager.cpp Outdated Show resolved Hide resolved
src/system_unix.cpp Show resolved Hide resolved
CMakeLists.txt Show resolved Hide resolved
src/system_unix.cpp Outdated Show resolved Hide resolved
src/system_unix.cpp Outdated Show resolved Hide resolved
src/system_unix.cpp Outdated Show resolved Hide resolved
Signed-off-by: Kai-Uwe Hermann <kai-uwe.hermann@pionix.de>
Signed-off-by: Kai-Uwe Hermann <kai-uwe.hermann@pionix.de>
src/system_unix.cpp Outdated Show resolved Hide resolved
hikinggrass and others added 10 commits December 7, 2023 08:29
Signed-off-by: Kai-Uwe Hermann <kai-uwe.hermann@pionix.de>
Signed-off-by: Cornelius Claussen <cc@pionix.de>
Signed-off-by: Cornelius Claussen <cc@pionix.de>
Signed-off-by: Cornelius Claussen <cc@pionix.de>
Signed-off-by: Cornelius Claussen <cc@pionix.de>
Signed-off-by: Cornelius Claussen <cc@pionix.de>
Signed-off-by: Cornelius Claussen <cc@pionix.de>
Signed-off-by: Cornelius Claussen <cc@pionix.de>
Signed-off-by: Cornelius Claussen <cc@pionix.de>
Signed-off-by: Cornelius Claussen <cc@pionix.de>
@corneliusclaussen corneliusclaussen merged commit 0f19d03 into main Dec 7, 2023
2 of 4 checks passed
@corneliusclaussen corneliusclaussen deleted the feature/change_user_and_capabilities branch December 7, 2023 12:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants