containers / podman

Podman: A tool for managing OCI containers and pods.
https://podman.io
Apache License 2.0
23.12k stars 2.36k forks source link

Importing Go bindings calls XDG runtime checks, causing application to exit unexpectedly #23818

Open ventifus opened 2 weeks ago

ventifus commented 2 weeks ago

Issue Description

XDG runtime checks should not be performed when only the go bindings are used.

After updating our podman go bindings from 4.9.4 to 5.0.3, our application began failing with the error

time="2024-08-29T22:12:26Z" level=error msg="stat /.config: no such file or directory"

We tracked this down to https://github.com/containers/storage/blob/main/pkg/homedir/homedir_unix.go#L120-L123, where the XDG runtime checks are verifying that $HOME/.config exists. Our application runs in a container with $HOME set to /, and there is no .config, and the runtime non-root user doesn't have permission to create it.

Inserting a debug.PrintStack() in GetConfigHome() shows this stack trace:

goroutine 1 [running, locked to thread]:
runtime/debug.Stack()
/usr/lib/golang/src/runtime/debug/stack.go:24 +0x5e
runtime/debug.PrintStack()
/usr/lib/golang/src/runtime/debug/stack.go:16 +0x13
github.com/containers/storage/pkg/homedir.GetConfigHome.func1()
/app/vendor/github.com/containers/storage/pkg/homedir/homedir_unix.go:122 +0x176
sync.(*Once).doSlow(0x3e9?, 0x0?)
/usr/lib/golang/src/sync/once.go:74 +0xbf
sync.(*Once).Do(...)
/usr/lib/golang/src/sync/once.go:65
github.com/containers/storage/pkg/homedir.GetConfigHome()
/app/vendor/github.com/containers/storage/pkg/homedir/homedir_unix.go:111 +0x2c
github.com/containers/common/pkg/config.defaultConfig()
/app/vendor/github.com/containers/common/pkg/config/default.go:201 +0x7a
github.com/containers/common/pkg/config.newLocked(0xc00035fd78)
/app/vendor/github.com/containers/common/pkg/config/new.go:71 +0x2a
github.com/containers/common/pkg/config.Default()
/app/vendor/github.com/containers/common/pkg/config/new.go:63 +0xfe
github.com/containers/podman/v5/pkg/util.init.0()
/app/vendor/github.com/containers/podman/v5/pkg/util/utils.go:48 +0x13

Additional context: https://redhat-internal.slack.com/archives/CBBJY9GSX/p1725040560356209

Steps to reproduce the issue

Steps to reproduce the issue

  1. Create a go program that imports the podman bindings
  2. Run the go program as an unprivileged user with $HOME=/ or other un-writable directory.
  3. Program exits unexpectedly

Describe the results you received

Program exits with the error

time="2024-08-29T22:12:26Z" level=error msg="stat /.config: no such file or directory"

Describe the results you expected

Program runs as expected

podman info output

n/a, we are not actually running podman

Podman in a container

No

Privileged Or Rootless

None

Upstream Latest Release

Yes

Additional environment details

No response

Additional information

Additional information like issue happens only occasionally or issue happens with a particular architecture or on a particular setting

Luap99 commented 2 weeks ago

ENOENT should not be an error to begin with IMO as this is only a config dir and AFAIK all other tools work fine without a config file in the home dir.

Second the bindings should not really import any of this code I think, at least not github.com/containers/common/pkg/config

mtrmac commented 2 weeks ago

Second the bindings should not really import any of this code I think, at least not github.com/containers/common/pkg/config

This part is the Podman bug, and should be fixed. (IIRC it’s c/podman/pkg/util importing this — util packages are always an anti pattern.)


ENOENT should not be an error to begin with IMO as this is only a config dir and AFAIK all other tools work fine without a config file in the home dir.

At least podman machine is creating files in $XDG_CONFIG_HOME (and/or the fallback). So for podman machine, we do need the ability to have, or create, a reasonable …ConfigHome directory.

The situation is a non-root user with a home directory, but not allowed to write into such a home directory. I generally suggest that this is an unreasonable setup to run (the full feature set of) Podman in, and not really worth adding special cases for.

Luap99 commented 2 weeks ago

ENOENT should not be an error to begin with IMO as this is only a config dir and AFAIK all other tools work fine without a config file in the home dir.

At least podman machine is creating files in $XDG_CONFIG_HOME (and/or the fallback). So for podman machine, we do need the ability to have, or create, a reasonable …ConfigHome directory.

There are two types of callers readers and writers, readers just use this to read a config file so they just join the result to its path and try to open the file which then either fails or not so they have to handle ENOENT errors if these files are optional so stating the dir before is just a waste of a syscall. Writers on the other side have to create a further directories, AFAICT none of other tools would write into .config but rather .config/containers so they must mkdir containers anyway. Now that can fail if they just call Mkdir vs MkdirAll but looking at it it seems they are already use MkdirAll

A function name like GetConfigHome() should just return the path regadless if it does exists or not, it should especially not create the path IMO because this means a command like podman ps creates the dir for no reason whatsoever.

rhatdan commented 2 weeks ago

How about we deprecate GetConfigHome() and add ConfigHome() to do what you want, then we move all callers to ConfigHome()?

mtrmac commented 2 weeks ago

There are two types of callers readers and writers …

I think that’s all a fair analysis.

The one thing I do care about is that the permission special case remains: if we inherit $XDG_CONFIG_HOME belonging to a different user account (not owned by us), we must fail, at the very least for writers (because root writing to other users’ directory can do damage), and I’d prefer that for for readers as well.


How about we deprecate GetConfigHome() and add ConfigHome() to do what you want, then we move all callers to ConfigHome()?

👍 Sounds like a good way to make this change and ensure all aspects are reviewed.

Luap99 commented 2 weeks ago

How about we deprecate GetConfigHome() and add ConfigHome() to do what you want, then we move all callers to ConfigHome()?

I would be fine with that as well.