Add file switch for pre-exec hooks #18331

TomSweeneyRedHat · 2023-04-24T22:24:09Z

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 [email protected]

Does this PR introduce a user-facing change?

None

TomSweeneyRedHat · 2023-04-24T22:24:28Z

@giuseppe PTAL

openshift-ci · 2023-04-24T22:24:54Z

[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:

~~OWNERS~~ [TomSweeneyRedHat]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Luap99 · 2023-04-25T10:06:40Z

We will still suggest pointing the ENVVAR at the directories /usr/libexec/podman/pre-exec-hooks and /etc/containers/pre-exec-hooks.

Should this say either or instead of and? AFAICT the env var only accepts a single dir.

TomSweeneyRedHat · 2023-04-25T13:32:27Z

Good point @Luap99 , it probably should be either directory, but not both. Will specify that in the RHEL docs.

TomSweeneyRedHat · 2023-04-25T23:45:02Z

Changing this up to check a file and then will do the reads if it is present. See #18347 for details.

flouthoc

Tests seems to be failing

giuseppe

Is the customer request tracked anywhere?

This is already growing more complex than I'd like it to be. If they want to disable pre-exec hooks, they can just remove/rename the directory with the hooks.

giuseppe · 2023-04-26T07:11:49Z

pkg/rootless/rootless_linux.c

@@ -254,6 +254,15 @@ do_preexec_hooks_dir (const char *dir, char **argv, int argc)
 static void
 do_preexec_hooks (char **argv, int argc)
 {
+  FILE *preexec_ptr;


we are leaking this file when it is opened, it must be fclose'd()

Oh shoot yes, rusty C, I'll fix that, TY

giuseppe · 2023-04-26T07:14:23Z

pkg/rootless/rootless_linux.c

+
+  // Open the preexec_hooks_dir indicator file in read mode
+  // return without processing if the file doesn't exist
+  preexec_ptr = fopen("/etc/containers/podman_preexec_hooks.txt", "r");


since we are checking only its existence, we can just use access()

And more rust flaking, I'd forgotten about access, that works out much nicer, ty.

Luap99 · 2023-04-26T13:17:01Z

So far I see discussion here and in #18347? Can we have that in one place please? We shouldn't backport anything before having it merged in main.

It is still not clear why this change is made? Has this change been communicated to the customer who wants to use this feature?

Luap99 · 2023-04-26T13:20:51Z

test/system/950-preexec-hooks.bats

+    preexec_hook_ok_file=/etc/containers/podman_preexec_hooks.txt
+    touch $preexec_hook_ok_file


These tests are run on local developer machines, please do not touch system wide config directories from within the tests. The rm below is not executed when the test error out before that or when I manually cancel for whatever reason.
cc @edsantiago

@edsantiago happy to change this if there's a better way forward.

Whoa, I hadn't even looked at system tests because this is so hellaciously confusing. So let me get this straight: the /etc/whatever file does not actually contain anything, it's just a semaphore saying "if this file exists, then it's okay for podman to run preexec hooks"? Bleagh. That is going to cost us support time in the future.

Anyhow, yeah, the touch thing, you can't do that. First, all rootless tests are going to fail. You need to skip_if_rootless. Second, you need to do the rm inside teardown(). Which will not have your $preexec_hook_ok_file defined, so please define that in outer scope (like between the loads and setup).

we cannot do skip_if_rootless, this whole feature only runs rootless.

I mean we could, but as I see this this feature was designed to prevent rootless user from doing stuff so not testing it as rootless is not great.

hahahahahahaha, well, I guess we don't test then.

TomSweeneyRedHat · 2023-04-26T13:31:09Z

The reason for both places at once, is we have a very imminent deadline. We have to get this out preferably by noon today, by noon tomorrow at the latest. That's due to the RHEL schedule and the QE and packaging teams haveing holidays shortly. We have been talking with the customer. They have told us we can't pull the functionality, and security is also pulling on us to not deliver it in its current form. I'm playing the part of Milhouse here

edsantiago · 2023-04-26T13:35:04Z

test/system/950-preexec-hooks.bats

+    PODMAN_PREEXEC_HOOKS_DIR=$preexec_hook_dir run_podman 125 pull foobar
+    PODMAN_PREEXEC_HOOKS_DIR=$preexec_hook_dir run_podman 125 pull barfoo


Third, please add error-message checks.

edsantiago · 2023-04-26T14:03:03Z

If this absolutely needs to get merged, and you really need tests, here's a possibility:
950-preexec-hooks.bats.txt

The long term goal was to provide the customer a way to turn on the preexec_hooks processing of script 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 conatiners.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 <[email protected]>

TomSweeneyRedHat · 2023-04-26T14:23:28Z

Updated this with Ed's test suggestion.

TomSweeneyRedHat · 2023-04-26T15:41:05Z

and now it looks like I'm getting bit by a flake in the boltdb tests. @edsantiago do you concur?

edsantiago · 2023-04-26T15:43:36Z

no, nothing to do with boltdb. Two of the flakes are known Cirrus ones, the other is our infamous unlinkat/EBUSY. I've restarted them all.

TomSweeneyRedHat · 2023-04-26T16:11:45Z

And all green here, but less rush to merge.

TomSweeneyRedHat · 2023-04-26T19:10:19Z

@giuseppe for some reason I can't reply to your question about "is this being tracked somewhere". This came in via the RHELBU feature that we have. There have been further discussions between Mark, Derrick Ornelas, and some of the Customer Experience people along with the customer. They are fine with this change as it stands in this PR. Product Security for RHEL was NOT fine with the code as it was before this change. They asked that we use either an EnvironmentVariable, which was admittedly securities second choice, or to read some config file to turn on the functionality. I started the change by using the EnvVar approach as it was very easy to code. We told the customer, and they came back as not being thrilled with that approach. So last night I worked on the change as you see it here.

The conversations have been going on in Slack for the most part over the past day or two, and then via the PRs in GitHub. Hope that helps, as Ed said, it's ugly, but I'm trying to find a middle ground that's not extraordinarily ugly yet keeps the customer happy.

TomSweeneyRedHat · 2023-05-11T23:12:43Z

Still green, and this is in Podman v4.2. Can we push it into main yet? @mheon?

mheon · 2023-05-11T23:31:25Z

/lgtm
Let's get the branches synced

JonnyWhatshisface · 2023-07-06T11:40:15Z

Just ran across this. I'm not quite sure where "None" came from on the question as to whether this introduces a user-interfacing change, but we most certainly noticed it and given the hooks won't be executed without the existence of that flag file, I'd argue it definitely had a user-facing impact.

For awareness, I'm the customer who requested this feature initially.

I'd like to understand why this change was introduced?

Luap99 · 2023-07-06T11:54:47Z

@JonnyWhatshisface If you are a customer please use your Red Hat support channels.
Also see #18331 (comment) for why.
If this has not been communicated properly to you then this needs to be addressed internally, this is not something we (upstream devs) can really help you with so please go through the official support channels.

Old PRs are generally never a good place to have a proper discussion.

TomSweeneyRedHat · 2023-07-10T20:45:47Z

Just as a FWIW, the discussions are ongoing in the proper RHEL channels.

openshift-ci bot added the do-not-merge/release-note-label-needed Enforce release-note requirement, even if just None label Apr 24, 2023

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 24, 2023

openshift-ci bot added release-note-none and removed do-not-merge/release-note-label-needed Enforce release-note requirement, even if just None labels Apr 25, 2023

TomSweeneyRedHat added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 25, 2023

TomSweeneyRedHat force-pushed the dev/tsweeney/hooked branch from cf1a35f to 6b7d93a Compare April 26, 2023 01:01

TomSweeneyRedHat changed the title ~~Remove default directories for pre-exec~~ Add file switch for pre-exec hooks Apr 26, 2023

TomSweeneyRedHat removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 26, 2023

flouthoc reviewed Apr 26, 2023

View reviewed changes

giuseppe requested changes Apr 26, 2023

View reviewed changes

giuseppe reviewed Apr 26, 2023

View reviewed changes

TomSweeneyRedHat mentioned this pull request Apr 26, 2023

[v4.4.1-rhel] Add file switch for pre-exec hooks #18347

Merged

TomSweeneyRedHat force-pushed the dev/tsweeney/hooked branch from 6b7d93a to 2fb5c93 Compare April 26, 2023 13:20

Luap99 reviewed Apr 26, 2023

View reviewed changes

edsantiago reviewed Apr 26, 2023

View reviewed changes

TomSweeneyRedHat force-pushed the dev/tsweeney/hooked branch from 2fb5c93 to c8e423b Compare April 26, 2023 14:21

openshift-ci bot assigned mheon May 11, 2023

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label May 11, 2023

openshift-merge-robot merged commit 189b09d into containers:main May 11, 2023

TomSweeneyRedHat deleted the dev/tsweeney/hooked branch June 30, 2023 21:42

github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Oct 9, 2023

github-actions bot locked as resolved and limited conversation to collaborators Oct 9, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add file switch for pre-exec hooks #18331

Add file switch for pre-exec hooks #18331

TomSweeneyRedHat commented Apr 24, 2023 •

edited

Loading

TomSweeneyRedHat commented Apr 24, 2023

openshift-ci bot commented Apr 24, 2023

Luap99 commented Apr 25, 2023

TomSweeneyRedHat commented Apr 25, 2023

TomSweeneyRedHat commented Apr 25, 2023

flouthoc left a comment

giuseppe left a comment

giuseppe Apr 26, 2023

TomSweeneyRedHat Apr 26, 2023

giuseppe Apr 26, 2023

TomSweeneyRedHat Apr 26, 2023

Luap99 commented Apr 26, 2023

Luap99 Apr 26, 2023

TomSweeneyRedHat Apr 26, 2023

edsantiago Apr 26, 2023

Luap99 Apr 26, 2023

Luap99 Apr 26, 2023

edsantiago Apr 26, 2023

TomSweeneyRedHat commented Apr 26, 2023 •

edited

Loading

edsantiago Apr 26, 2023

edsantiago commented Apr 26, 2023

TomSweeneyRedHat commented Apr 26, 2023

TomSweeneyRedHat commented Apr 26, 2023

edsantiago commented Apr 26, 2023

TomSweeneyRedHat commented Apr 26, 2023

TomSweeneyRedHat commented Apr 26, 2023

TomSweeneyRedHat commented May 11, 2023

mheon commented May 11, 2023

JonnyWhatshisface commented Jul 6, 2023 •

edited

Loading

Luap99 commented Jul 6, 2023

TomSweeneyRedHat commented Jul 10, 2023

		preexec_hook_ok_file=/etc/containers/podman_preexec_hooks.txt
		touch $preexec_hook_ok_file

		PODMAN_PREEXEC_HOOKS_DIR=$preexec_hook_dir run_podman 125 pull foobar
		PODMAN_PREEXEC_HOOKS_DIR=$preexec_hook_dir run_podman 125 pull barfoo

Add file switch for pre-exec hooks #18331

Add file switch for pre-exec hooks #18331

Conversation

TomSweeneyRedHat commented Apr 24, 2023 • edited Loading

Does this PR introduce a user-facing change?

TomSweeneyRedHat commented Apr 24, 2023

openshift-ci bot commented Apr 24, 2023

Luap99 commented Apr 25, 2023

TomSweeneyRedHat commented Apr 25, 2023

TomSweeneyRedHat commented Apr 25, 2023

flouthoc left a comment

Choose a reason for hiding this comment

giuseppe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Luap99 commented Apr 26, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TomSweeneyRedHat commented Apr 26, 2023 • edited Loading

Choose a reason for hiding this comment

edsantiago commented Apr 26, 2023

TomSweeneyRedHat commented Apr 26, 2023

TomSweeneyRedHat commented Apr 26, 2023

edsantiago commented Apr 26, 2023

TomSweeneyRedHat commented Apr 26, 2023

TomSweeneyRedHat commented Apr 26, 2023

TomSweeneyRedHat commented May 11, 2023

mheon commented May 11, 2023

JonnyWhatshisface commented Jul 6, 2023 • edited Loading

Luap99 commented Jul 6, 2023

TomSweeneyRedHat commented Jul 10, 2023

TomSweeneyRedHat commented Apr 24, 2023 •

edited

Loading

TomSweeneyRedHat commented Apr 26, 2023 •

edited

Loading

JonnyWhatshisface commented Jul 6, 2023 •

edited

Loading