allow privileged IMP to linger during flux-imp run to support signal forwarding
#188
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
grondo
force-pushed
the
imp-run-linger
branch
from
e1204b7 to
0ccc432
Compare
Problem: Unlike `exec`, `flux-imp run` doesn't linger and instead directly executes the target command. This then requires the use of `flux-imp kill` to kill the privlieged process(es) that are a result. Have the IMP fork() and exec() the run command. The lingering IMP process can then forward signals delivered to it, including the use of SIGUSR1 as a surrogate for SIGKILL.
grondo
force-pushed
the
imp-run-linger
branch
from
0ccc432 to
c5323d1
Compare
garlick
previously approved these changes
Nov 1, 2024
This was referenced Nov 1, 2024
Closed
|
Actually there's a couple issues still remaining here:
|
Problem: The IMP run implementation sets the real user and group id of the process to the effective user and group id in the privileged parent, but this makes it so that the invoking user can no longer deliver signals to the IMP parent process. Move the setuid()/setgid() calls to the child process just before execve(2) is called. The parent IMP thereby maintains the real uid/gid of the invoking user and can handle forwarding of signals from that user to the invoked run command. Since the parent IMP process no longer has a real userid of 0/root, update the call that obtains the userid for setting USER and HOME to use the effective uid.
Problem: There is no test that ensures the privileged IMP lingers to handle signal delivery with `flux-imp run`. Add a test to t2002-imp-run.t.
grondo
force-pushed
the
imp-run-linger
branch
from
c5323d1 to
309b1c3
Compare
|
I had forgotten that I also figured out some issues with the test and that appears to now be working as well. This could definitely use some more real world testing. |
grondo
changed the title
flux-imp run to support signal forwardingflux-imp run to support signal forwarding
|
Removed WIP. This seems to be working as designed now, and is much more robust than the |
|
Excellent! Will try to get that done tomorrow morning. |
garlick
approved these changes
Nov 4, 2024
|
Thanks! I've set mwp |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR changes the
flux-imp runimplementation to have the privileged IMP linger and delegate signals to its child, which the calling user would not otherwise have privilege to do.WIP while I test this out locally.