[carry 1738] Clear hook environ variables on empty Env #4323

lifubang · 2024-06-19T14:49:06Z

The runtime spec has:

env (array of strings, OPTIONAL) with the same semantics as IEEE Std 1003.1-2008's environ.

And running execle or similar with NULL env results in an empty environent:

$ cat test.c
#include <unistd.h>

int main()
{
  return execle("/usr/bin/env", "env", NULL, NULL);
}
$ cc -o test test.c
$ ./test
…no output…

Go's Cmd.Env, on the other hand, has:

If Env is nil, the new process uses the current process's environment.

This commit works around that by setting ~~a single dummy environment variable~~ []string{} in those cases to avoid leaking the runtime environment into the hooks.

The runtime spec has [1]: * env (array of strings, OPTIONAL) with the same semantics as IEEE Std 1003.1-2008's environ. And running execle or similar with NULL env results in an empty environent: $ cat test.c #include <unistd.h> int main() { return execle("/usr/bin/env", "env", NULL, NULL); } $ cc -o test test.c $ ./test ...no output... Go's Cmd.Env, on the other hand, has [2]: If Env is nil, the new process uses the current process's environment. This commit works around that by setting Env to an empty slice in those cases to avoid leaking the runtime environment into the hooks. [1]: https://github.com/opencontainers/runtime-spec/blob/v1.0.1/config.md#posix-platform-hooks [2]: https://golang.org/pkg/os/exec/#Cmd Signed-off-by: W. Trevor King <wking@tremily.us> (cherry picked from commit c11bd33) Signed-off-by: lfbzhm <lifubang@acmcoder.com>

kolyshkin · 2024-06-19T19:15:39Z

tests/integration/hooks.bats

+				"env": ["mm=tt"]
+			}]
+		}'
+		mm=nn runc run "test_hook-$hook"


Not sure what this like is doing here. A debug leftover?

Or maybe you wanted to check that this var is not visible? But it should be a different var name/value.

If you want a different name, what about runc_env or similar?

In my brain, the Prestart, Poststart, CreateContainer and CreateRuntime hooks are running in the host, not in the container? So they use host's env when running the hook.

Only StartContainer hook is running in the container.

kolyshkin · 2024-06-19T19:17:24Z

tests/integration/hooks.bats

+		update_config '.hooks = {
+			"'$hook'": [{
+				"path": "/bin/sh",
+				"args": ["/bin/sh", "-c", "[ \"$mm\"==\"tt\" ] && echo yes, we got tt from the env mm && exit 1 || exit 0"],


You can just have

"args": ["/bin/env"],

which prints all environment, and then check below that $output contains ^mm=tt$.

My bad, I mean "path", not "args".

In the beginning, I wanted to test it like the way you provided, but we should know that if the hook running success, there is no output. We should raise an error to get the output.
https://github.com/opencontainers/runc/blob/main/libcontainer/configs/config.go#L487-L493

kolyshkin · 2024-06-19T19:20:10Z

tests/integration/hooks.bats

+				"path": "/bin/sh",
+				"args": ["/bin/sh", "-c", "[[ \"$mm\" == \"nn\" ]] && echo \"$mm\" && exit 1 || exit 0"]


Same here, you can use

"path": "/bin/env"

and check that there is no output.

#4323 (comment)

kolyshkin · 2024-06-19T19:21:17Z

tests/integration/hooks.bats

+	done
+}
+
+@test "runc run [hook without env]" {


I would add what we check here ("hook without env does not inherit host env"). Maybe add a bug reference as well.

I guess this test fails today (without the PR), but it will be great to confirm it

I guess this test fails today (without the PR), but it will be great to confirm it

Yes, you are right, I have tested before I submit this PR.

rata

Thanks for carrying the fix!

rata · 2024-06-21T10:00:07Z

tests/integration/hooks.bats

+				"env": ["mm=tt"]
+			}]
+		}'
+		mm=nn runc run "test_hook-$hook"


Or maybe you wanted to check that this var is not visible? But it should be a different var name/value.

If you want a different name, what about runc_env or similar?

rata · 2024-06-21T10:01:07Z

tests/integration/hooks.bats

+
+@test "runc run [hook with env]" {
+	update_config '.process.args = ["/bin/true"]'
+	update_config '.process.env = ["mm=nn"]'


What if we use a more readable name?

Suggested change

update_config '.process.env = ["mm=nn"]'

update_config '.process.env = ["container_var=yes"]'

Because we do a test in one loop for hook in prestart createRuntime createContainer startContainer poststart; do, but as mentioned in #4323 (comment), the var may be in host, or may be in container, but we should use one var name, so we should not use a specific var name.

rata · 2024-06-21T10:02:26Z

tests/integration/hooks.bats

+	done
+}
+
+@test "runc run [hook without env]" {


I guess this test fails today (without the PR), but it will be great to confirm it

Signed-off-by: lfbzhm <lifubang@acmcoder.com>

lifubang · 2024-06-21T10:51:42Z

Even though this is a bug, but it has existed for many years, so if we merge this, it may cause a break change for docker and other tools which use runc for the container runtime. We should check a config.json file generated by containerd to see if there is a env config for hooks.

kolyshkin · 2024-06-26T01:37:09Z

What I see is crun works the same way, ie the current environment is passed on to the hook. Which is obviously wrong, but the fix may bring in some compatibility issues. Not sure how to assess if it's going to be the case...

lifubang · 2024-06-27T09:47:08Z

We should check a config.json file generated by containerd to see if there is a env config for hooks.

I had some time to look some config.json files in my company's cluster:

In k8s cluster: no hooks;
In docker swarm cluster: only one prestart hook, and using a absolute path:

cat config.json | jq .hooks
{
  "prestart": [
    {
      "path": "/proc/1158/exe",
      "args": [
        "libnetwork-setkey",
        "-exec-root=/var/run/docker",
        "2123cdedbd057cd58fba1c9869d410fcd1e6588f705e0eda6deb345d9a9670c0",
        "406aba1a8aaa"
      ]
    }
  ]
}

I also test the binary compiled from this PR with docker, it works fine for a normal use.

but the fix may bring in some compatibility issues.

It seems that there is no compatibility issues for a normal use, but I don't know whether there are issues for some other unknown complex scenarios.

lifubang mentioned this pull request Jun 19, 2024

libcontainer/configs/config: Clear hook environ variables on empty Env #1738

Closed

lifubang force-pushed the feat-carry-1738-hook-clear-env branch 2 times, most recently from d3908aa to 6cedbc9 Compare June 19, 2024 15:30

kolyshkin reviewed Jun 19, 2024

View reviewed changes

rata reviewed Jun 21, 2024

View reviewed changes

add tests for env param in hooks

827bbdb

Signed-off-by: lfbzhm <lifubang@acmcoder.com>

lifubang force-pushed the feat-carry-1738-hook-clear-env branch from 6cedbc9 to 827bbdb Compare June 21, 2024 10:25

This comment was marked as duplicate.

Sign in to view

lifubang mentioned this pull request Jun 27, 2024

libct: speedup process.Env handling #4325

Open

lifubang mentioned this pull request Jul 16, 2024

fix runc's poststart behaviour doesn't match the runtime-spec #4348

Draft

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[carry 1738] Clear hook environ variables on empty Env #4323

[carry 1738] Clear hook environ variables on empty Env #4323

lifubang commented Jun 19, 2024

kolyshkin Jun 19, 2024

rata Jun 21, 2024

lifubang Jun 21, 2024

lifubang Jun 21, 2024

kolyshkin Jun 19, 2024

kolyshkin Jun 19, 2024

lifubang Jun 21, 2024

kolyshkin Jun 19, 2024

lifubang Jun 21, 2024

kolyshkin Jun 19, 2024

rata Jun 21, 2024

lifubang Jun 21, 2024

rata left a comment

rata Jun 21, 2024

rata Jun 21, 2024

lifubang Jun 21, 2024 •

edited

Loading

rata Jun 21, 2024

lifubang commented Jun 21, 2024

This comment was marked as duplicate.

kolyshkin commented Jun 26, 2024

lifubang commented Jun 27, 2024

		"path": "/bin/sh",
		"args": ["/bin/sh", "-c", "[[ \"$mm\" == \"nn\" ]] && echo \"$mm\" && exit 1 \|\| exit 0"]

	update_config '.process.env = ["mm=nn"]'
	update_config '.process.env = ["container_var=yes"]'

[carry 1738] Clear hook environ variables on empty Env #4323

Are you sure you want to change the base?

[carry 1738] Clear hook environ variables on empty Env #4323

Conversation

lifubang commented Jun 19, 2024

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

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

rata left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lifubang Jun 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lifubang commented Jun 21, 2024

This comment was marked as duplicate.

kolyshkin commented Jun 26, 2024

lifubang commented Jun 27, 2024

lifubang Jun 21, 2024 •

edited

Loading