imp: support helper to get input to exec command #163
Merged
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
Codecov Report
@@ Coverage Diff @@
## master #163 +/- ##
==========================================
- Coverage 87.08% 86.91% -0.18%
==========================================
Files 33 35 +2
Lines 4082 4187 +105
==========================================
+ Hits 3555 3639 +84
- Misses 527 548 +21
|
grondo
changed the title
Problem: We'd like to use pipe2(2), but this function is not available on all platforms. Rather than generate an error at compile time, check for availability of pipe2(2) in configure and fail to configure if this function is not found.
Problem: There is no convenient function to split a string into tokens on whitespace. Add a convenience function argsplit() to libutil for this purpose.
Problem: There are no unit tests for the libutil argsplit() function. Add a set of simple unit tests for this function.
Problem: The FD_CLOEXEC flag is not set on the pipe fds used by the IMP's privilege separation mechanism. This could present a security vulnerability if even the unprivileged IMP executes another process, since access to the privsep pipe could allow untrusted code to send data to the privileged IMP process. Use pipe2(2) and set the FD_CLOEXEC on the privsep pipes at creation time.
Problem: IMP logging functions may overwrite errno, which means callers would have to save errno across logging calls if they want errno unchanged. This seems unreasonable. Save and restore errno in IMP logging calls imp_say(), imp_warn(), and imp_debug().
|
I've tested this with a modified flux-core and it does seem to work as intended. Removing WIP to mark this ready for review. |
grondo
changed the title
garlick
approved these changes
May 15, 2023
src/imp/exec/safe_popen.c
Outdated
Comment on lines
85
to
87
| int saved_errno = errno; | ||
| (void) kill (sp->pid, SIGKILL); | ||
| errno = saved_errno; |
There was a problem hiding this comment.
Maybe fdopen(3) could come before the fork(2) call to avoid the open loop kill(2) on the error path?
src/imp/exec/exec.c
Outdated
Comment on lines
410
to
419
| if ((helper = getenv ("FLUX_IMP_EXEC_HELPER")) | ||
| && strlen (helper) > 0) { | ||
| /* Read input from helper command */ | ||
| imp_exec_init_helper (exec, helper); | ||
| } | ||
| else { | ||
| /* Read input from stdin, cmdline: */ | ||
| imp_exec_init_stream (exec, stdin); | ||
| } |
There was a problem hiding this comment.
Might be worth noting why an empty string value for FLUX_IMP_EXEC_HELPER is not treated as an error?
There was a problem hiding this comment.
Actually that's a good question. Maybe it should just be an error? For some reason I was thinking it was idiomatic to have an empty environment variable be treated as unset, as it makes things like this possible VARIABLE= command to "unset" a possibly set VARIABLE for the execution of command. Maybe I'll just say that in the comment.
There was a problem hiding this comment.
Upon deep reflection, probably it is most clear and safest to return an error if FLUX_IMP_EXEC_HELPER is empty. This wasn't used in testing so it is easy to make this change.
Problem: The IMP will need popen-like functionality to fork an optional helper process to gather input, but popen(3) unnecessarily calls out to the shell which could be insecure. Add a safer version of popen(3) in imp/exec/safe_popen.[ch]. This version splits the command argument into tokens separated by whitespace and passes that directly to execvp(2) instead of passing to `sh -c`.
Problem: There is no testing for the IMP's safe_popen() implementation. Add a set of simple unit tests for safe_popen() in src/imp/test.
Problem: The IMP is only capable of reading its "input" on stdin, which means the Flux execution system must write this data to each IMP process invoked for a job. For large jobs, this could become a scalability problem. Additionally, there are certain use cases where it would be useful to keep the stdio descriptors free in the job shell, but this method for reading input makes that impossible. Support an optional FLUX_IMP_EXEC_HELPER environment variable in the IMP which, when set, causes the IMP to invoke the specified helper program to gather the input, instead of expecting input on stdin. The value in FLUX_IMP_EXEC_HELPER is passed directly to safe_popen(), so it will be tokenized as a command line separated by whitespace, but no other substitutions or processing is performed as would happen with the shell for instance. This reduces the probability of bugs introduced by the FLUX_IMP_EXEC_HELPER environment variable, while still allowing some flexibility in the command line, for instance FLUX_IMP_EXEC_HELPER="flux imp-exec-helper" will work as expected.
Problem: The initial few expected failure tests in t2000-imp-exec.t "succeed" because the current user is not configured to use the IMP, not because of bad input or an invalid commandline. Add a config file that allows the user executing the testsuite to use the IMP so that the tests fail for the expected reason.
Problem: The bash function `let` is used in the final test in t2000-imp-exec.t. Replace the use of `let` with a POSIX compliant arithmetic expansion.
Problem: There are no tests of using FLUX_IMP_EXEC_HELPER to gather input to the IMP. Add a set of tests that exercise the use of FLUX_IMP_EXEC_HELPER.
|
Ok, force pushed the suggested fixes. An empty |
garlick
added a commit
to garlick/flux-rfc
that referenced
this pull request
Oct 17, 2024
Problem: the RFC states that the IMP takes its input on stdin to avoid placing sensitive data on the command line, but stdin is no longer used for this. Now the IMP obtains its input by calling a helper program provided by the instance instead of stdin. The helper is run from the unprivileged part of the IMP. For now, just drop the incorrect detail which wasn't necessary in that part of the text anyway. See also: flux-framework/flux-security#163
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 adds support for a new
FLUX_IMP_EXEC_HELPERenvironment variable which, when present, allows the input toflux-imp execto be obtained from a command provided by the instance using the IMP, instead of written to stdin.Though it should be safe to use
popen(3)to run the helper, since this occurs in the unprivileged IMP, the command inFLUX_IMP_EXEC_HELPERis split into tokens on whitespace and passed directly toexecvp()to avoid the shell interpreting the value resulting in unintended or malicious bugs.Still a WIP until some testing on the flux-core side is complete.
Fixes #105.