[v4.4.1-rhel] Add file switch for pre-exec hooks #18347
Conversation
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: TomSweeneyRedHat The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
the
approved
|
I need to make some BZs for this, WIP, and I'll add links here. |
|
We didn't document these at all, right? |
|
No, we are documenting these as of 8.8/9.2. That's why the change, RH Security Alerts, didn't like the default directories. |
|
@edsantiago the smoke test keeps smoking and I need to get this merged today if at all possible. Anything I can do to clear the smoke other than pressing the button once more? |
|
@TomSweeneyRedHat you might be asking the wrong person: I know nothing whatsoever about Windows. It's pretty clear that it's not a flake, though, so whoever reviews can just press the Merge button. |
|
@edsantiago just double checking your phraseology, "pretty clear that it's not a flake", should have been "pretty clear that it's a flake", correct? |
|
I meant "not a flake", as in, I expect it to fail repeatedly. The windows smoke test constantly starts failing, for some reason or another that I've never paid attention to, and requires hand-tuning by a Windows expert. I do not believe it is worth your or my or anyone's time to worry about this failure. |
|
Thanks for the clarification Ed. |
|
LGTM |
|
My 2c, I can’t see how the Windows failure could be related and I’m fine with not blocking on that test. As for the change itself, discussed elsewhere. |
TomSweeneyRedHat
added
the
do-not-merge/hold
|
I've been poking at some things, and I'm going to try a slightly different approach. I'll check for a podman_preexec_hooks.txt file in /etc/containers. If the file is found, then we'll do the reads of the file as we had prior to this change. |
TomSweeneyRedHat
force-pushed
the
dev/tsweeney/4.4.1-preexec
branch
from
ece2187
to
3a75f1c
Compare
TomSweeneyRedHat
changed the title
| if (preexec_ptr == NULL) { | ||
| return; | ||
| } | ||
|
|
There was a problem hiding this comment.
I think you forgot to actually read the file.
Since I don't understand the threat model that this addresses, I'm going to ask: how paranoid do we need to get here? Do we need to check all parent directories to make sure they're not group/world writable? That this file, and all parents, are owned by root?
There was a problem hiding this comment.
And paranoia, there is no right level! The folks at security alert gave me two options, 1) check a config file presence, or value within one, or, 2) EnvVar check. I went with the EnvVar at first as the code change was real easy, but after further discussions/thinking, I decided to drop a file in place.
I've a hunch this will be changed further over time, but the security folks said this is sufficient for now.
There was a problem hiding this comment.
Ooops, I've a code error here with the access call, need to touch that up. I shouldn't code when in a meeting....
TomSweeneyRedHat
force-pushed
the
dev/tsweeney/4.4.1-preexec
branch
3 times, most recently
from
8ada2ac
to
d85e16e
Compare
|
OK, just made one last change to tweak the comment. Thanks @giuseppe for the access() suggestion, that was really slick. Fingers crossed for tests. |
| @@ -263,6 +263,13 @@ do_preexec_hooks_dir (const char *dir, char **argv, int argc) | |||
| static void | |||
| do_preexec_hooks (char **argv, int argc) | |||
| { | |||
| // Access the preexec_hooks_dir indicator file | |||
| // return without processing if the file doesn't exist | |||
| char preexec_hooks_path[] = "/etc/containers/podman_preexec_hooks.txt"; | |||
There was a problem hiding this comment.
Implementation LGTM. (I’m not sure why this needs to exist, looking just at the stated intent.)
(This isn’t really worth a delay in this PR, but if we are going to add similar code to the main branch…:)
- Stylistically, putting a macro at the top, beside
ETC_PREEXEC_HOOKS, would make it clearer that this is a ~choice, not an essential component of this individual function. - It’s fine to call
access("…")without the intermediatechar[]variable.
The long term goal was to provide the customer with a way to turn on the preexec_hooks processing of scripts by having some kind of configuration that could be read. I had tried putting it into containers.conf to start, but that turned out to be unyieldly quickly, and time is of the essence for this fix. That is mostly due to the fact that this code is preexecution and in C, the containers.conf file is read in Go much further down the stack. After first trying this process using an ENVVAR, I have thought it over and chatted with others, and will now look for a /etc/containers/podman_preexec_hooks.txt file to exist. If the admin had put one in there, we will then process the files in the directories /usr/libexec/podman/pre-exec-hooks and /etc/containers/pre-exec-hooks. Thoughts/suggestions gratefully accepted. This will be a 8.8/9.2 ZeroDay fix and will need to be backported to the v4.4.1-rhel branch. Signed-off-by: tomsweeneyredhat <tsweeney@redhat.com>
TomSweeneyRedHat
force-pushed
the
dev/tsweeney/4.4.1-preexec
branch
from
d85e16e
to
3db7f4c
Compare
|
I've implemented Ed's test suggestions from #18331 |
|
@Luap99 @mheon @edsantiago @giuseppe All tests are passing, except the windows smoke test which per Ed is a known issue here in this branch. Can this be LGTM'd and merged so Jindrich can build a module before he leaves today? |
|
I truly hate this, but am holding my nose and have merged. |
|
I am not sure why this was merged. I don't think this is needed, I had comments about this feature in #18331 |
The long term goal was to provide the customer with a way to turn on the
preexec_hooks processing of scripts by having some kind of configuration
that could be read. I had tried putting it into containers.conf to
start, but that turned out to be unyieldly quickly, and time is of
the essence for this fix. That is mostly due to the fact that this
code is preexecution and in C, the containers.conf file is read in
Go much further down the stack.
After first trying this process using an ENVVAR, I have
thought it over and chatted with others, and will now look for a
/etc/containers/podman_preexec_hooks.txt file to exist. If the admin
had put one in there, we will then process the files in the
directories /usr/libexec/podman/pre-exec-hooks
and /etc/containers/pre-exec-hooks.
Thoughts/suggestions gratefully accepted. This will be a 8.8/9.2 ZeroDay
fix and will need to be backported to the v4.4.1-rhel branch.
[NO NEW TESTS NEEDED]
Signed-off-by: tomsweeneyredhat tsweeney@redhat.com
Does this PR introduce a user-facing change?