libcontainer/configs/config: Clear hook environ variables on empty Env by wking · Pull Request #1738 · opencontainers/runc

wking · 2018-02-24T00:59:43Z

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.

cyphar · 2018-02-24T02:23:55Z

libcontainer/configs/config.go

 		Stderr: &stderr,
 	}
+	if cmd.Env == nil {
+		cmd.Env = []string{"_GO_DOES_NOT_PROVIDE_A_WAY_TO_CLEAR_ENV="}


Does cmd.Env = []string{} also not work?

Does cmd.Env = []string{} also not work?

Looks like it does :). Fixed with 5f6ed84 → 96e8770.

justincormack · 2018-02-27T10:58:35Z

Why can't you just set it to []string{} which will give the desired outcome if the passed array is nil?

Ah sorry, was mislead by the comments above it has been changed to do this.

wking · 2018-02-27T15:15:52Z

Ah sorry, was mislead by the comments above...

I've updated the initial comment to avoid misleading others.

This shifts the matching logic out of libpod/container_internal and into the hook package, where we can reuse it after vendoring into CRI-O. It also adds unit tests with almost-complete coverage. Now libpod is even more isolated from the hook internals, which makes it fairly straightforward to bump the hook config file to 1.0.0. I've dubbed the old format 0.1.0, although it doesn't specify an explicit version. Motivation for some of my changes with 1.0.0: * Add an explicit version field. This will make any future JSON structure migrations more straightforward by avoiding the need for version-guessing heuristics. * Collect the matching properties in a new When sub-structure. This makes the root Hook structure easier to understand, because you don't have to read over all the matching properties when wrapping your head around Hook. * Replace the old 'hook' and 'arguments' with a direct embedding of the runtime-spec's hook structure. This provides access to additional upstream properties (args[0], env, and timeout) and avoids the complication of a CRI-O-specific analog structure. * Add a 'when.always' property. You can usually accomplish this effect in another way (e.g. when.commands = [".*"]), but having a boolean explicitly for this use-case makes for easier reading and writing. * Replace the previous annotations array with an annotations map. The 0.1.0 approach matched only the values regardless of key, and that seems unreliable. * Replace 'cmds' with 'when.commands', because while there are a few ways to abbreviate "commands", there's only one way to write it out in full ;). This gives folks one less thing to remember when writing hook JSON. * Replace the old "inject if any specified condition matches" with "inject if all specified conditions match". This allows for more precise targeting. Users that need more generous targeting can recover the previous behavior by creating a separate 1.0.0 hook file for each specified 0.1.0 condition. I've added doc-compat support for the various pluralizations of the 0.1.0 properties. Previously, the docs and code were not in agreement. More on this particular facet in [1]. I've updated the docs to point out that the annotations being matched are the OCI config annotations. This differs from CRI-O, where the annotations used are the Kubernetes-supplied annotations [2,3]. For example, io.kubernetes.cri-o.Volumes [4] is part of CRI-O's runtime config annotations [5], but not part of the Kubernetes-supplied annotations CRI-O uses for matching hooks. The Monitor method supports the CRI-O use-case [6]. podman doesn't need it directly, but CRI-O will need it when we vendor this package there. I've used nvidia-container-runtime-hook for the annotation examples because Dan mentioned the Nvidia folks as the motivation behind annotation matching. The environment variables are documented in [7]. The 0.1.0 hook config, which does not allow for environment variables, only works because runc currently leaks the host environment into the hooks [8]. I haven't been able to find documentation for their usual annotation trigger or hook-install path, so I'm just guessing there. [1]: cri-o/cri-o#1235 [2]: https://github.com/kubernetes-incubator/cri-o/blob/v1.10.0/server/container_create.go#L760 [3]: https://github.com/kubernetes-incubator/cri-o/blob/v1.10.0/server/container_create.go#L772 [4]: https://github.com/kubernetes-incubator/cri-o/blob/v1.10.0/pkg/annotations/annotations.go#L97-L98 [5]: https://github.com/kubernetes-incubator/cri-o/blob/v1.10.0/server/container_create.go#L830-L834 [6]: cri-o/cri-o#1345 [7]: https://github.com/NVIDIA/nvidia-container-runtime/tree/v1.3.0-1#environment-variables-oci-spec [8]: opencontainers/runc#1738 Signed-off-by: W. Trevor King <wking@tremily.us>

This shifts the matching logic out of libpod/container_internal and into the hook package, where we can reuse it after vendoring into CRI-O. It also adds unit tests with almost-complete coverage. Now libpod is even more isolated from the hook internals, which makes it fairly straightforward to bump the hook config file to 1.0.0. I've dubbed the old format 0.1.0, although it doesn't specify an explicit version. Motivation for some of my changes with 1.0.0: * Add an explicit version field. This will make any future JSON structure migrations more straightforward by avoiding the need for version-guessing heuristics. * Collect the matching properties in a new When sub-structure. This makes the root Hook structure easier to understand, because you don't have to read over all the matching properties when wrapping your head around Hook. * Replace the old 'hook' and 'arguments' with a direct embedding of the runtime-spec's hook structure. This provides access to additional upstream properties (args[0], env, and timeout) and avoids the complication of a CRI-O-specific analog structure. * Add a 'when.always' property. You can usually accomplish this effect in another way (e.g. when.commands = [".*"]), but having a boolean explicitly for this use-case makes for easier reading and writing. * Replace the previous annotations array with an annotations map. The 0.1.0 approach matched only the values regardless of key, and that seems unreliable. * Replace 'cmds' with 'when.commands', because while there are a few ways to abbreviate "commands", there's only one way to write it out in full ;). This gives folks one less thing to remember when writing hook JSON. * Replace the old "inject if any specified condition matches" with "inject if all specified conditions match". This allows for more precise targeting. Users that need more generous targeting can recover the previous behavior by creating a separate 1.0.0 hook file for each specified 0.1.0 condition. I've added doc-compat support for the various pluralizations of the 0.1.0 properties. Previously, the docs and code were not in agreement. More on this particular facet in [1]. I've updated the docs to point out that the annotations being matched are the OCI config annotations. This differs from CRI-O, where the annotations used are the Kubernetes-supplied annotations [2,3]. For example, io.kubernetes.cri-o.Volumes [4] is part of CRI-O's runtime config annotations [5], but not part of the Kubernetes-supplied annotations CRI-O uses for matching hooks. The Monitor method supports the CRI-O use-case [6]. podman doesn't need it directly, but CRI-O will need it when we vendor this package there. I've used nvidia-container-runtime-hook for the annotation examples because Dan mentioned the Nvidia folks as the motivation behind annotation matching. The environment variables are documented in [7]. The 0.1.0 hook config, which does not allow for environment variables, only works because runc currently leaks the host environment into the hooks [8]. I haven't been able to find documentation for their usual annotation trigger or hook-install path, so I'm just guessing there. [1]: cri-o/cri-o#1235 [2]: https://github.com/kubernetes-incubator/cri-o/blob/v1.10.0/server/container_create.go#L760 [3]: https://github.com/kubernetes-incubator/cri-o/blob/v1.10.0/server/container_create.go#L772 [4]: https://github.com/kubernetes-incubator/cri-o/blob/v1.10.0/pkg/annotations/annotations.go#L97-L98 [5]: https://github.com/kubernetes-incubator/cri-o/blob/v1.10.0/server/container_create.go#L830-L834 [6]: cri-o/cri-o#1345 [7]: https://github.com/NVIDIA/nvidia-container-runtime/tree/v1.3.0-1#environment-variables-oci-spec [8]: opencontainers/runc#1738 Signed-off-by: W. Trevor King <wking@tremily.us> Closes: #686 Approved by: mheon

kolyshkin · 2024-06-15T06:58:37Z

This makes sense but needs a test case (in e.g. tests/integration/hooks.bats)

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>

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

Carried in #4323

kolyshkin · 2024-06-23T19:48:48Z

Closing in favor of #4323

wking mentioned this pull request Feb 24, 2018

How can we get nvidia hook to work with CRI-O. cri-o/cri-o#1222

Closed

cyphar reviewed Feb 24, 2018

View reviewed changes

wking force-pushed the hook-clear-env branch from 5f6ed84 to 96e8770 Compare February 24, 2018 02:35

wking mentioned this pull request Mar 5, 2018

config: Clarify execution environment for hooks opencontainers/runtime-spec#953

Merged

wking mentioned this pull request May 29, 2018

podman with bind mount leaving cgroup debris and prevents container restart containers/podman#730

Closed

kolyshkin force-pushed the hook-clear-env branch from 96e8770 to c11bd33 Compare June 15, 2024 06:58

lifubang mentioned this pull request Jun 19, 2024

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

Open

kolyshkin closed this Jun 23, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Comments

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

libcontainer/configs/config: Clear hook environ variables on empty Env#1738
wking wants to merge 1 commit intoopencontainers:mainfrom
wking:hook-clear-env

wking commented Feb 24, 2018 •

edited

Loading

Uh oh!

cyphar Feb 24, 2018

Uh oh!

wking Feb 24, 2018

Uh oh!

justincormack commented Feb 27, 2018 •

edited

Loading

Uh oh!

wking commented Feb 27, 2018

Uh oh!

kolyshkin commented Jun 15, 2024

Uh oh!

lifubang commented Jun 19, 2024

Uh oh!

kolyshkin commented Jun 23, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Comments

Conversation

wking commented Feb 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cyphar Feb 24, 2018

Choose a reason for hiding this comment

Uh oh!

wking Feb 24, 2018

Choose a reason for hiding this comment

Uh oh!

justincormack commented Feb 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wking commented Feb 27, 2018

Uh oh!

kolyshkin commented Jun 15, 2024

Uh oh!

lifubang commented Jun 19, 2024

Uh oh!

kolyshkin commented Jun 23, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

wking commented Feb 24, 2018 •

edited

Loading

justincormack commented Feb 27, 2018 •

edited

Loading