From 7ade9721020468438e822b16ed7a65380cc7fbd2 Mon Sep 17 00:00:00 2001 From: Valentin Rothberg Date: Tue, 26 Sep 2023 09:41:00 +0200 Subject: [PATCH] libpod: pass entire environment to conmon Pass the _entire_ environment to conmon instead of selectively enabling only specific variables. The main reasoning is to make sure that conmon and the podman-cleanup callback process operate in the exact same environment than the initial podman process. Some configuration files may be passed via environment variables. Podman not passing those down to conmon has led to subtle and hard to debug issues in the past, so passing all down will avoid such kinds of issues in the future. Signed-off-by: Valentin Rothberg --- libpod/oci_conmon_common.go | 73 ++++++++------------------------ libpod/oci_conmon_exec_common.go | 13 +++--- test/system/030-run.bats | 3 +- 3 files changed, 25 insertions(+), 64 deletions(-) diff --git a/libpod/oci_conmon_common.go b/libpod/oci_conmon_common.go index 8cca1622f2..3eac697651 100644 --- a/libpod/oci_conmon_common.go +++ b/libpod/oci_conmon_common.go @@ -35,7 +35,6 @@ import ( "github.com/containers/podman/v4/pkg/specgenutil" "github.com/containers/podman/v4/pkg/util" "github.com/containers/podman/v4/utils" - "github.com/containers/storage/pkg/homedir" spec "github.com/opencontainers/runtime-spec/specs-go" "github.com/sirupsen/logrus" "golang.org/x/sys/unix" @@ -1042,11 +1041,6 @@ func (r *ConmonOCIRuntime) getLogTag(ctr *Container) (string, error) { func (r *ConmonOCIRuntime) createOCIContainer(ctr *Container, restoreOptions *ContainerCheckpointOptions) (int64, error) { var stderrBuf bytes.Buffer - runtimeDir, err := util.GetRuntimeDir() - if err != nil { - return 0, err - } - parentSyncPipe, childSyncPipe, err := newPipe() if err != nil { return 0, fmt.Errorf("creating socket pair: %w", err) @@ -1189,7 +1183,10 @@ func (r *ConmonOCIRuntime) createOCIContainer(ctr *Container, restoreOptions *Co } // 0, 1 and 2 are stdin, stdout and stderr - conmonEnv := r.configureConmonEnv(runtimeDir) + conmonEnv, err := r.configureConmonEnv() + if err != nil { + return 0, fmt.Errorf("configuring conmon env: %w", err) + } var filesToClose []*os.File if preserveFDs > 0 { @@ -1251,7 +1248,7 @@ func (r *ConmonOCIRuntime) createOCIContainer(ctr *Container, restoreOptions *Co if restoreOptions != nil { runtimeRestoreStarted = time.Now() } - err = startCommand(cmd, ctr) + err = cmd.Start() // regardless of whether we errored or not, we no longer need the children pipes childSyncPipe.Close() @@ -1311,38 +1308,23 @@ func (r *ConmonOCIRuntime) createOCIContainer(ctr *Container, restoreOptions *Co } // configureConmonEnv gets the environment values to add to conmon's exec struct -// TODO this may want to be less hardcoded/more configurable in the future -func (r *ConmonOCIRuntime) configureConmonEnv(runtimeDir string) []string { - var env []string - for _, e := range os.Environ() { - if strings.HasPrefix(e, "LC_") { - env = append(env, e) - } - if strings.HasPrefix(e, "LANG=") { - env = append(env, e) +func (r *ConmonOCIRuntime) configureConmonEnv() ([]string, error) { + env := os.Environ() + res := make([]string, 0, len(env)) + for _, v := range env { + if strings.HasPrefix(v, "NOTIFY_SOCKET=") { + // The NOTIFY_SOCKET must not leak into the environment. + continue } + res = append(res, v) } - if path, ok := os.LookupEnv("PATH"); ok { - env = append(env, fmt.Sprintf("PATH=%s", path)) - } - if conf, ok := os.LookupEnv("CONTAINERS_CONF"); ok { - env = append(env, fmt.Sprintf("CONTAINERS_CONF=%s", conf)) - } - if conf, ok := os.LookupEnv("CONTAINERS_CONF_OVERRIDE"); ok { - env = append(env, fmt.Sprintf("CONTAINERS_CONF_OVERRIDE=%s", conf)) - } - if conf, ok := os.LookupEnv("CONTAINERS_HELPER_BINARY_DIR"); ok { - env = append(env, fmt.Sprintf("CONTAINERS_HELPER_BINARY_DIR=%s", conf)) - } - env = append(env, fmt.Sprintf("XDG_RUNTIME_DIR=%s", runtimeDir)) - env = append(env, fmt.Sprintf("_CONTAINERS_USERNS_CONFIGURED=%s", os.Getenv("_CONTAINERS_USERNS_CONFIGURED"))) - env = append(env, fmt.Sprintf("_CONTAINERS_ROOTLESS_UID=%s", os.Getenv("_CONTAINERS_ROOTLESS_UID"))) - home := homedir.Get() - if home != "" { - env = append(env, fmt.Sprintf("HOME=%s", home)) + runtimeDir, err := util.GetRuntimeDir() + if err != nil { + return nil, err } - return env + res = append(res, "XDG_RUNTIME_DIR="+runtimeDir) + return res, nil } // sharedConmonArgs takes common arguments for exec and create/restore and formats them for the conmon CLI @@ -1422,25 +1404,6 @@ func (r *ConmonOCIRuntime) sharedConmonArgs(ctr *Container, cuuid, bundlePath, p return args } -func startCommand(cmd *exec.Cmd, ctr *Container) error { - // Make sure to unset the NOTIFY_SOCKET and reset it afterwards if needed. - switch ctr.config.SdNotifyMode { - case define.SdNotifyModeContainer, define.SdNotifyModeIgnore: - if prev := os.Getenv("NOTIFY_SOCKET"); prev != "" { - if err := os.Unsetenv("NOTIFY_SOCKET"); err != nil { - logrus.Warnf("Error unsetting NOTIFY_SOCKET %v", err) - } - defer func() { - if err := os.Setenv("NOTIFY_SOCKET", prev); err != nil { - logrus.Errorf("Resetting NOTIFY_SOCKET=%s", prev) - } - }() - } - } - - return cmd.Start() -} - // newPipe creates a unix socket pair for communication. // Returns two files - first is parent, second is child. func newPipe() (*os.File, *os.File, error) { diff --git a/libpod/oci_conmon_exec_common.go b/libpod/oci_conmon_exec_common.go index 2feeba678f..22d1217665 100644 --- a/libpod/oci_conmon_exec_common.go +++ b/libpod/oci_conmon_exec_common.go @@ -17,7 +17,6 @@ import ( "github.com/containers/podman/v4/libpod/define" "github.com/containers/podman/v4/pkg/errorhandling" "github.com/containers/podman/v4/pkg/lookup" - "github.com/containers/podman/v4/pkg/util" spec "github.com/opencontainers/runtime-spec/specs-go" "github.com/sirupsen/logrus" "golang.org/x/sys/unix" @@ -374,11 +373,6 @@ func (r *ConmonOCIRuntime) startExec(c *Container, sessionID string, options *Ex } }() - runtimeDir, err := util.GetRuntimeDir() - if err != nil { - return nil, nil, err - } - finalEnv := make([]string, 0, len(options.Env)) for k, v := range options.Env { finalEnv = append(finalEnv, fmt.Sprintf("%s=%s", k, v)) @@ -438,7 +432,10 @@ func (r *ConmonOCIRuntime) startExec(c *Container, sessionID string, options *Ex // } // } - conmonEnv := r.configureConmonEnv(runtimeDir) + conmonEnv, err := r.configureConmonEnv() + if err != nil { + return nil, nil, fmt.Errorf("configuring conmon env: %w", err) + } var filesToClose []*os.File if options.PreserveFDs > 0 { @@ -461,7 +458,7 @@ func (r *ConmonOCIRuntime) startExec(c *Container, sessionID string, options *Ex Setpgid: true, } - err = startCommand(execCmd, c) + err = execCmd.Start() // We don't need children pipes on the parent side errorhandling.CloseQuiet(childSyncPipe) diff --git a/test/system/030-run.bats b/test/system/030-run.bats index e84af4be4e..5c6751d35c 100644 --- a/test/system/030-run.bats +++ b/test/system/030-run.bats @@ -1288,7 +1288,7 @@ search | $IMAGE | done < <(parse_table "$tests") } -@test "podman --syslog passed to conmon" { +@test "podman --syslog and environment passed to conmon" { skip_if_remote "--syslog is not supported for remote clients" skip_if_journald_unavailable @@ -1298,6 +1298,7 @@ search | $IMAGE | run_podman container inspect $cid --format "{{ .State.ConmonPid }}" conmon_pid="$output" is "$(< /proc/$conmon_pid/cmdline)" ".*--exit-command-arg--syslog.*" "conmon's exit-command has --syslog set" + assert "$(< /proc/$conmon_pid/environ)" =~ "BATS_TEST_TMPDIR" "entire env is passed down to conmon (incl. BATS variables)" run_podman rm -f -t0 $cid }