Skip to content

image/docker: use unified configfile search for cert directories #746

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.

Sign up for GitHub

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

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

image/docker: use unified configfile search for cert directories #746

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0df042b
image/docker: use unified configfile search for cert directories
jankaluza Apr 2, 2026
e01a5f9
Check for certDir before calling SetupCertificates.
jankaluza Apr 10, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
46 changes: 21 additions & 25 deletions image/docker/docker_client.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,8 +33,9 @@ import (
"go.podman.io/image/v5/pkg/sysregistriesv2"
"go.podman.io/image/v5/pkg/tlsclientconfig"
"go.podman.io/image/v5/types"
"go.podman.io/storage/pkg/configfile"
"go.podman.io/storage/pkg/fileutils"
"go.podman.io/storage/pkg/homedir"
"go.podman.io/storage/pkg/unshare"
"golang.org/x/sync/semaphore"
)

Expand All @@ -60,19 +61,10 @@ const (
backoffMaxDelay = 60 * time.Second
)

type certPath struct {
path string
absolute bool
var perHostCertDirs = []string{
etcDir + "/docker/certs.d",
Copy link
Copy Markdown
Member

@Luap99 Luap99 Apr 8, 2026

Choose a reason for hiding this comment

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

@mtrmac Do we still want this path? API wise it seems rather ugly to define that search order with that additional path.

Copy link
Copy Markdown
Member Author

@jankaluza jankaluza Apr 9, 2026

Choose a reason for hiding this comment

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

That's a good point and it would make the API cleaner definitely.

Copy link
Copy Markdown
Contributor

@mtrmac mtrmac Apr 9, 2026

Choose a reason for hiding this comment

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

We do need it. It’s never been deprecated by us, and it’s the one place users can use to affect both consumers.

See e.g. https://github.com/search?q=repo%3Aopenshift%2Fmachine-config-operator%20%2Fdocker%2Fcerts.d&type=code for a minimal proof.

}

var (
homeCertDir = filepath.FromSlash(".config/containers/certs.d")
perHostCertDirs = []certPath{
{path: etcDir + "/containers/certs.d", absolute: true},
{path: etcDir + "/docker/certs.d", absolute: true},
}
)

// extensionSignature and extensionSignatureList come from github.com/openshift/origin/pkg/dockerregistry/server/signaturedispatcher.go:
// signature represents a Docker image signature.
type extensionSignature struct {
Expand Down Expand Up @@ -167,22 +159,26 @@ func dockerCertDir(sys *types.SystemContext, hostPort string) (string, error) {
return filepath.Join(sys.DockerPerHostCertDirPath, hostPort), nil
}

var (
hostCertDir string
fullCertDirPath string
)
conf := &configfile.Directory{
Name: "certs",
UserId: unshare.GetRootlessUID(),
ExtraDirs: perHostCertDirs,
}

for _, perHostCertDir := range append([]certPath{{path: filepath.Join(homedir.Get(), homeCertDir), absolute: false}}, perHostCertDirs...) {
if sys != nil && sys.RootForImplicitAbsolutePaths != "" && perHostCertDir.absolute {
hostCertDir = filepath.Join(sys.RootForImplicitAbsolutePaths, perHostCertDir.path)
} else {
hostCertDir = perHostCertDir.path
}
if sys != nil && sys.RootForImplicitAbsolutePaths != "" {
conf.RootForImplicitAbsolutePaths = sys.RootForImplicitAbsolutePaths
}

dirs, err := configfile.ContainersResourceDirs(conf)
if err != nil {
return "", err
}

fullCertDirPath = filepath.Join(hostCertDir, hostPort)
for _, baseDir := range dirs {
fullCertDirPath := filepath.Join(baseDir, hostPort)
err := fileutils.Exists(fullCertDirPath)
if err == nil {
break
return fullCertDirPath, nil
}
if os.IsNotExist(err) {
continue
Expand All @@ -193,7 +189,7 @@ func dockerCertDir(sys *types.SystemContext, hostPort string) (string, error) {
}
return "", err
}
return fullCertDirPath, nil
return "", nil
Copy link
Copy Markdown
Contributor

@mtrmac mtrmac Apr 9, 2026

Choose a reason for hiding this comment

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

Hum, the code is not actually ready to consume "". I guess we never ran into that.

Copy link
Copy Markdown
Member Author

@jankaluza jankaluza Apr 10, 2026

Choose a reason for hiding this comment

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

Well, the code works, but it is not as good as it could be. The previous code returned non-existing directory while the current code returns "" in this case. I think "" is cleaner, because returning non-existing directory looks weird to me.

The directory is then passed to tlsclientconfig.SetupCertificates(certDir, tlsClientConfig) as certDir. This function tries to read that empty dir and returns nil error in case it does not exist.

But I will add check for empty certDir to not even call SetupCertificates. Tell me if you want me to go back to old behavior.

}

// newDockerClientFromRef returns a new dockerClient instance for refHostname (a host a specified in the Docker image reference, not canonicalized to dockerRegistry)
Expand Down
34 changes: 25 additions & 9 deletions image/docker/docker_client_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import (
"net/http"
"net/http/httptest"
"net/url"
"os"
"path/filepath"
"strings"
"testing"
Expand All @@ -21,21 +22,36 @@ import (
)

func TestDockerCertDir(t *testing.T) {
const nondefaultFullPath = "/this/is/not/the/default/full/path"
const nondefaultPerHostDir = "/this/is/not/the/default/certs.d"
t.Helper()

tempRoot := t.TempDir()

nondefaultFullPath := filepath.Join(tempRoot, "nondefault", "full", "path")
nondefaultPerHostDir := filepath.Join(tempRoot, "nondefault", "certs.d")
const variableReference = "$HOME"
const rootPrefix = "/root/prefix"
rootPrefix := filepath.Join(tempRoot, "rootprefix")
const registryHostPort = "thishostdefinitelydoesnotexist:5000"

systemPerHostResult := filepath.Join(perHostCertDirs[len(perHostCertDirs)-1].path, registryHostPort)
hostDirs := []string{
"/etc/containers/certs.d",
"/etc/docker/certs.d",
}

// Create RootForImplicitAbsolutePaths-prefixed locations.
for _, d := range hostDirs {
require.NoError(t, os.MkdirAll(filepath.Join(rootPrefix, d, registryHostPort), 0o755))
}
// Create nondefault per-host override directory.
require.NoError(t, os.MkdirAll(filepath.Join(nondefaultPerHostDir, registryHostPort), 0o755))

for _, c := range []struct {
sys *types.SystemContext
expected string
}{
// The common case
{nil, systemPerHostResult},
// There is a context, but it does not override the path.
{&types.SystemContext{}, systemPerHostResult},
// Work with nil SystemContext.
{nil, ""},
// Work with empty SystemContext.
{&types.SystemContext{}, ""},
// Full path overridden
{&types.SystemContext{DockerCertPath: nondefaultFullPath}, nondefaultFullPath},
// Per-host path overridden
Expand All @@ -54,7 +70,7 @@ func TestDockerCertDir(t *testing.T) {
// Root overridden
{
&types.SystemContext{RootForImplicitAbsolutePaths: rootPrefix},
filepath.Join(rootPrefix, systemPerHostResult),
filepath.Join(rootPrefix, "/etc/containers/certs.d", registryHostPort),
},
// Root and path overrides present simultaneously,
{
Expand Down
21 changes: 20 additions & 1 deletion image/docs/containers-certs.d.5.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,28 @@
containers-certs.d - Directory for storing custom container-registry TLS configurations

# DESCRIPTION
A custom TLS configuration for a container registry can be configured by creating a directory under `$HOME/.config/containers/certs.d` or `/etc/containers/certs.d`.
A custom TLS configuration for a container registry can be configured by creating a directory under `$XDG_CONFIG_HOME/containers/certs.d` (or `$HOME/.config/containers/certs.d` if `XDG_CONFIG_HOME` is unset), `/etc/containers/certs.d`, or `/usr/share/containers/certs.d`.
The name of the directory must correspond to the `host`[`:port`] of the registry (e.g., `my-registry.com:5000`).

Depending on whether the process is running as root or rootless, additional configuration directories are consulted to allow for system-wide defaults and per-user overrides:

- For both rootful and rootless:
- `/usr/share/containers/certs.d/`
- `/etc/containers/certs.d/`
- `/etc/docker/certs.d/`
- For rootful (UID == 0):
- `/usr/share/containers/certs.rootful.d/`
- `/etc/containers/certs.rootful.d/`
- For rootless (UID > 0):
- `/usr/share/containers/certs.rootless.d/`
- `/usr/share/containers/certs.rootless.d/<UID>/`
- `/etc/containers/certs.rootless.d/`
- `/etc/containers/certs.rootless.d/<UID>/`
- For per-user configuration:
- `$XDG_CONFIG_HOME/containers/certs.d/` (or `$HOME/.config/containers/certs.d/` if `XDG_CONFIG_HOME` is unset)

If a given `host`[`:port`] directory exists in multiple locations, the effective configuration is determined by the unified configfile search order: user configuration takes precedence over `/etc`, which in turn takes precedence over `/usr/share`.

The port part presence / absence must precisely match the port usage in image references,
e.g. to affect `podman pull registry.example/foo`,
use a directory named `registry.example`, not `registry.example:443`.
Expand Down
95 changes: 95 additions & 0 deletions storage/pkg/configfile/parse.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,9 @@ var (
// This can be overridden at build time with the following go linker flag:
// -ldflags '-X go.podman.io/storage/pkg/configfile.adminOverrideConfigPath=$your_path'
adminOverrideConfigPath = getAdminOverrideConfigPath()

// userConfigPathForResourceDirs is a test hook for ContainersResourceDirs.
userConfigPathForResourceDirs = UserConfigPath
Comment on lines +35 to +36
Copy link
Copy Markdown
Member

@Luap99 Luap99 Apr 8, 2026

Choose a reason for hiding this comment

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

it is not clear to me why do you need this? The other test does not need it? you can just sentenv the XDG_CONFIG_HOME dir?

Copy link
Copy Markdown
Member Author

@jankaluza jankaluza Apr 9, 2026

Choose a reason for hiding this comment

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

This was the best way I found out to let the userConfigPath fail with an error for the user config path resolution failure is ignored test. I admit I'm not the go expert, so maybe there is better way to do that.

In that test, I want to test that even if userConfigPath returns an error, other directories are evaluated. This behavior is aligned with the original code and I wanted to keep it.

I therefore needed to find a way how to monkey-patch the userConfigPath to generate an error so I can test its handling. Maybe I'm just trying to come up with Python solution in go lang :-).

)

type File struct {
Expand Down Expand Up @@ -308,6 +311,98 @@ func getDropInPaths(mainPath, suffix string, uid int) []string {
return paths
}

type Directory struct {
// The base name of the config directory.
// Must not be empty and must not contain the path separator.
// The full path is then constructed by the ContainersResourceDirs function.
// For example, "certs" will be joined with ".d" to form "certs.d".
Name string

// RootForImplicitAbsolutePaths is the path to an alternate root
// If not "", prefixed to any absolute paths used by default in the package.
// NOTE: This does NOT affect paths starting by $HOME or environment variables paths.
RootForImplicitAbsolutePaths string

// UserId is the id of the user running this. Used to know where to search in the
// different "rootful" and "rootless" drop in lookup paths.
UserId int

// ExtraDirs is a list of additional directories to include in the search.
ExtraDirs []string
}

// ContainersResourceDirs returns a list of configuration directories for a
// logical resource name (for example "registries" or "certs") using the
// unified configfile search semantics.
//
// The returned slice is ordered from highest to lowest priority and contains
// only directories that exist and can be accessed. Non‑existent or
// permission‑denied directories are silently skipped; other filesystem errors
// are returned to the caller.
//
// The search covers, where configured (listed here from lowest to highest precedence.
// It can be extended with additional absolute directories via extraDirs (lowest precedence).
func ContainersResourceDirs(conf *Directory) ([]string, error) {
Copy link
Copy Markdown
Member

@Luap99 Luap99 Apr 8, 2026

Choose a reason for hiding this comment

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

API wise I am not to sure we just want to return the list here?

certs.d just needs the first match starting with home, etc, /usr... so would it not be more logical to pass in the name we search as argument and the return a signle full path and exit early?

Copy link
Copy Markdown
Member Author

@jankaluza jankaluza Apr 9, 2026

Choose a reason for hiding this comment

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

I wanted to make it more generic and let the caller to decide what to do with these directories. But if you think we can just return single directory, I'm also fine changing it.

candidates := make([]string, 0, 7+len(conf.ExtraDirs))

userConfig, _ := userConfigPathForResourceDirs()
Copy link
Copy Markdown
Member

@Luap99 Luap99 Apr 8, 2026

Choose a reason for hiding this comment

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

error should not be silently ignored

Copy link
Copy Markdown
Member Author

@jankaluza jankaluza Apr 9, 2026

Choose a reason for hiding this comment

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

This is aligned with the old code - it used homedir.Get() which ignores the error the same way:

func Get() string {
	homedir, _ := unshare.HomeDir()
	return homedir
}

I can add warning here in the ContainersResourceDirs so the error message about missing homedir is logged. I believe we still want to try other directories if homedir cannot be found, so this error should not be fatal.

if userConfig != "" {
userConfig = filepath.Join(userConfig, conf.Name)
candidates = append(candidates, userConfig+dropInSuffix)

}

overrideConfig := adminOverrideConfigPath
if overrideConfig != "" {
overrideConfig = filepath.Join(overrideConfig, conf.Name)
if conf.RootForImplicitAbsolutePaths != "" {
overrideConfig = filepath.Join(conf.RootForImplicitAbsolutePaths, overrideConfig)
}
overridePaths := getDropInPaths(overrideConfig, "", conf.UserId)
for i := len(overridePaths) - 1; i >= 0; i-- {
candidates = append(candidates, overridePaths[i])
}
}

for i := len(conf.ExtraDirs) - 1; i >= 0; i-- {
dir := conf.ExtraDirs[i]
if conf.RootForImplicitAbsolutePaths != "" {
dir = filepath.Join(conf.RootForImplicitAbsolutePaths, dir)
}
candidates = append(candidates, dir)
}

defaultConfig := systemConfigPath
if defaultConfig != "" {
defaultConfig = filepath.Join(defaultConfig, conf.Name)
if conf.RootForImplicitAbsolutePaths != "" {
defaultConfig = filepath.Join(conf.RootForImplicitAbsolutePaths, defaultConfig)
}
defaultPaths := getDropInPaths(defaultConfig, "", conf.UserId)
for i := len(defaultPaths) - 1; i >= 0; i-- {
candidates = append(candidates, defaultPaths[i])
}
}

dirs := make([]string, 0, len(candidates)+len(conf.ExtraDirs))

for _, dir := range candidates {
info, err := os.Stat(dir)
if err != nil {
if errors.Is(err, fs.ErrNotExist) || errors.Is(err, fs.ErrPermission) {
Copy link
Copy Markdown
Member

@Luap99 Luap99 Apr 8, 2026

Choose a reason for hiding this comment

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

why do we skip permission errors?

Copy link
Copy Markdown
Member Author

@jankaluza jankaluza Apr 9, 2026

Choose a reason for hiding this comment

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

I thought it is a good idea to fallback to next directory if we cannot access some directory. In my opinion, this function should find valid directory which we can read files from.

Thinking about it now, maybe showing a warning in the ErrPermission case would be good thing to do. Or do you think this condition should be removed?

continue
}
return nil, err
}
if !info.IsDir() {
continue
}
dirs = append(dirs, dir)
}

return dirs, nil
}

func moduleDirectories(defaultConfig, overrideConfig, userConfig string) []string {
const moduleSuffix = ".modules"
modules := make([]string, 0, 3)
Expand Down
Loading
Loading