Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

lifubang
Copy link
Member

Carry #1738


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.

@lifubang 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
"env": ["mm=tt"]
}]
}'
mm=nn runc run "test_hook-$hook"
Copy link
Contributor

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?

Copy link
Member

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?

Copy link
Member Author

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.

Copy link
Member Author

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.

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"],
Copy link
Contributor

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$.

Copy link
Contributor

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".

Copy link
Member Author

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

Comment on lines 75 to 76
"path": "/bin/sh",
"args": ["/bin/sh", "-c", "[[ \"$mm\" == \"nn\" ]] && echo \"$mm\" && exit 1 || exit 0"]
Copy link
Contributor

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done
}

@test "runc run [hook without env]" {
Copy link
Contributor

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.

Copy link
Member

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

Copy link
Member Author

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.

Copy link
Member

@rata rata left a 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!

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

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?


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

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?

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

Copy link
Member Author

@lifubang lifubang Jun 21, 2024

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 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.

Copy link
Contributor

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 as TEST_VAR for variable name, and host_value and TEST_VAR=container_value (for values), or some such, to increase the test code readability.

done
}

@test "runc run [hook without env]" {
Copy link
Member

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

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

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
@lifubang

This comment was marked as duplicate.

@kolyshkin
Copy link
Contributor

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
Copy link
Member Author

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:

  1. In k8s cluster: no hooks;
  2. 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 lifubang force-pushed the feat-carry-1738-hook-clear-env branch 3 times, most recently from 53ccb05 to eb74b56 Compare December 11, 2024 15:33
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 <[email protected]>
(cherry picked from commit c11bd33)
Signed-off-by: lfbzhm <[email protected]>
@lifubang lifubang force-pushed the feat-carry-1738-hook-clear-env branch from eb74b56 to a1861dc Compare December 11, 2024 15:34
@lifubang lifubang force-pushed the feat-carry-1738-hook-clear-env branch from a1861dc to c635e4b Compare December 11, 2024 15:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants