- Notifications
You must be signed in to change notification settings - Fork2.2k
[carry 1738] Clear hook environ variables on empty Env#4323
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:main
Are you sure you want to change the base?
Uh oh!
There was an error while loading.Please reload this page.
Conversation
d3908aa to6cedbc9Comparetests/integration/hooks.bats Outdated
| "env": ["mm=tt"] | ||
| }] | ||
| }' | ||
| mm=nn runc run"test_hook-$hook" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Not sure what this like is doing here. A debug leftover?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Only StartContainer hook is running in the container.
tests/integration/hooks.bats Outdated
| 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"], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
You can just have
"args": ["/bin/env"],which prints all environment, and then check below that$output contains^mm=tt$.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
My bad, I mean"path", not"args".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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
tests/integration/hooks.bats Outdated
| "path": "/bin/sh", | ||
| "args": ["/bin/sh", "-c", "[[ \"$mm\" == \"nn\" ]] && echo \"$mm\" && exit 1 || exit 0"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Same here, you can use
"path": "/bin/env"and check that there is no output.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
tests/integration/hooks.bats Outdated
| done | ||
| } | ||
| @test"runc run [hook without env]" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I would add what we check here ("hook without env does not inherit host env"). Maybe add a bug reference as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I guess this test fails today (without the PR), but it will be great to confirm it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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 left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Thanks for carrying the fix!
tests/integration/hooks.bats Outdated
| "env": ["mm=tt"] | ||
| }] | ||
| }' | ||
| mm=nn runc run"test_hook-$hook" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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?
tests/integration/hooks.bats Outdated
| @test"runc run [hook with env]" { | ||
| update_config'.process.args = ["/bin/true"]' | ||
| update_config'.process.env = ["mm=nn"]' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
What if we use a more readable name?
| update_config'.process.env = ["mm=nn"]' | |
| update_config'.process.env = ["container_var=yes"]' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Because we do a test in one loopfor 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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
What about self-descriptive names such asTEST_VAR for variable name, andhost_value andTEST_VAR=container_value (for values), or some such, to increase the test code readability.
tests/integration/hooks.bats Outdated
| done | ||
| } | ||
| @test"runc run [hook without env]" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I guess this test fails today (without the PR), but it will be great to confirm it
6cedbc9 to827bbdbComparelifubang commentedJun 21, 2024
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. |
1 similar comment
This comment was marked as duplicate.
This comment was marked as duplicate.
kolyshkin commentedJun 26, 2024
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 commentedJun 27, 2024
I had some time to look some
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.
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. |
53ccb05 toeb74b56CompareThe 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 emptyenvironent: $ 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 inthose 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/#CmdSigned-off-by: W. Trevor King <wking@tremily.us>(cherry picked from commitc11bd33)Signed-off-by: lfbzhm <lifubang@acmcoder.com>eb74b56 toa1861dcCompareSigned-off-by: lfbzhm <lifubang@acmcoder.com>
a1861dc toc635e4bCompare
kolyshkin left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Left a few comments, mostly about the test.
We also need a changelog entry as this might bring compatibility issues.
@opencontainers/runc-maintainers PTAL
| Stderr:&stderr, | ||
| } | ||
| ifcmd.Env==nil { | ||
| cmd.Env= []string{} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Maybe add a comment like
| cmd.Env= []string{} | |
| // IfEnvis not specified, do not inherit the current environment. | |
| cmd.Env= []string{} |
| @test"runc run [hook with env property]" { | ||
| update_config'.process.args = ["/bin/true"]' | ||
| update_config'.process.env = ["TEST_VAR=val"]' | ||
| # All hooks except Poststop. | ||
| forhookin prestart createRuntime createContainer startContainer poststart;do | ||
| echo"testing hook$hook" | ||
| # shellcheck disable=SC2016 | ||
| update_config'.hooks = { | ||
| "'$hook'": [{ | ||
| "path": "/bin/sh", | ||
| "args": ["/bin/sh", "-c", "[ \"$TEST_VAR\"==\"val\" ] && echo yes, we got val from the env TEST_VAR && exit 1 || exit 0"], | ||
| "env": ["TEST_VAR=val"] | ||
| }] | ||
| }' | ||
| TEST_VAR="val" runc run"test_hook-$hook" | ||
| ["$status"-ne 0 ] | ||
| [["$output"==*"yes, we got val from the env TEST_VAR"* ]] | ||
| done | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
So, this test case checks thathooks.$name.env is set as expected ($TEST_VAR == "val", but the test code also sets the same TEST_ENV=val for
- process.env
- host env
It's not clear why 1 and 2 are needed, and why they use the same name and value.
To me, if you want to check that host env and/or process env is not leaking into hook's env, you should use different names, and check they are not present.
Or, just don't set either host or process env, and merely check that hook env is exactly the same as specified.
| @test"runc run [hook without env property should not inherit host env]" { | ||
| update_config'.process.args = ["/bin/true"]' | ||
| update_config'.process.env = ["TEST_VAR=val"]' | ||
| # All hooks except Poststop. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Why not test poststop?
| # https://github.com/opencontainers/runtime-spec/blob/v1.0.1/config.md#posix-platform-hooks | ||
| @test"runc run [hook without env property should not inherit host env]" { | ||
| update_config'.process.args = ["/bin/true"]' | ||
| update_config'.process.env = ["TEST_VAR=val"]' | ||
| # All hooks except Poststop. | ||
| forhookin prestart createRuntime createContainer startContainer poststart;do | ||
| echo"testing hook$hook" | ||
| # shellcheck disable=SC2016 | ||
| update_config'.hooks = { | ||
| "'$hook'": [{ | ||
| "path": "/bin/sh", | ||
| "args": ["/bin/sh", "-c", "[[ \"$TEST_VAR\" == \"val\" ]] && echo \"$TEST_VAR\" && exit 1 || exit 0"] | ||
| }] | ||
| }' | ||
| TEST_VAR="val" runc run"test_hook-$hook" | ||
| ["$status"-eq 0 ] | ||
| done |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Unlike the previous@test, this one actually makes sense (but maybe add a comment that here we setTEST_VAR=val forrunc run and for the process spec, and check that neither one leaks to hook's env.
| @test"runc run [hook with env property]" { | ||
| update_config'.process.args = ["/bin/true"]' | ||
| update_config'.process.env = ["TEST_VAR=val"]' | ||
| # All hooks except Poststop. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Why not test poststop?
Carry#1738
The runtime spechas:
And running
execleor similar withNULLenvresults in an empty environent:Go's
Cmd.Env, on the other hand, has: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.