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

Follow up auth #2600

Draft
wants to merge 5 commits into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
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
11 changes: 9 additions & 2 deletions docs/containers-auth.json.5.md
Original file line number Diff line number Diff line change
Expand Up @@ -6,16 +6,23 @@ containers-auth.json - syntax for the registry authentication file
# DESCRIPTION

A file in JSON format controlling authentication against container image registries.
The primary (read/write) file is stored at `${XDG_RUNTIME_DIR}/containers/auth.json` on Linux;
The primary (read/write) per-user file is stored at `${XDG_RUNTIME_DIR}/containers/auth.json` on Linux;
on Windows and macOS, at `$HOME/.config/containers/auth.json`.

When searching for the credential for a registry, the following files will be read in sequence until the valid credential is found:
There is also a system-global `/etc/containers/auth.json` path and `/etc/containers/auth.d/` directory with drop-in per-repo files.
Copy link
Member

Choose a reason for hiding this comment

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

I think these paths are only used on Linux. We should probably highlight that here in the docs.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also I think we should support the full https://uapi-group.org/specifications/specs/configuration_files_specification/ and hence we should handle /run/containers/auth.d and /usr/lib/containers/auth.d as well.

Copy link
Collaborator

Choose a reason for hiding this comment

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

That makes little sense in combination with the requireUserOnly logic. Either it is a whole-system configuration, or a root-only configuration.

/usr/… credentials are outright inconsistent with the idea of “hermetic /usr” supposedly motivating the design of that spec,.

(My vague intuition is that a whole-system readable-to-all configuration, like OS subscription credentials, makes sense as a new feature; a new feature specific to root-only or even systemd-root-only seems strictly inferior to specifically passing a service-specific credential option to that one specific command, so it does not make sense to add. But, also, I might well have forgotten an important argument from earlier discussions, and I haven’t revisited that yet.)

Copy link
Contributor

Choose a reason for hiding this comment

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

/usr/… credentials are outright inconsistent with the idea of “hermetic /usr” supposedly motivating the design of that spec,.

Yes and no. It is definitely the case that a toplevel design goal of the UAPI group and systemd is to support split "golden vendor generic OS image in /usr" and "user local config in /etc".

But at the same time actually for people who are making custom derived OS images, it absolutely makes sense to put some of this configuration in /usr.

To expand, this difference is exactly a huge thing that divides "Fedora CoreOS" from "Fedora bootc". In CoreOS you must put your content in /etc, but in the bootc model we definitely encourage putting it in /usr precisely because it "version locks" your config and the OS together - you own both parts as a single transactional unit.

This also relates to another thing that we in the bootc world kind of disagree with which is that we really want to support having e.g. LUKS for everything in / including the operating system and hence including /usr. The systemd/UAPI design keeps pushing the idea that "everything in /usr is open FOSS" but I don't think that's the reality.

It's of course not ideal to put "secret data" in /usr but, it's not any different in reality than putting it in /etc or /root either from the bootc PoV. In some cases it's just "bootstrap secrets" that may live in the OS image that only have a relatively limited blast radius if leaked (e.g. they're just pull secrets, not push secrets for example).

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Author

Choose a reason for hiding this comment

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

@cgwalters I took a look at full specs for https://uapi-group.org/specifications/specs/configuration_files_specification/ and turned out that my understanding of the logic evaluation is slightly different from the reality. Here's a specific example and it takes into account just /etc and /usr/ uapi-group/specifications#125 (comment) Adding a third one /run would be even more cumbersome to implement. Do you still want to see this done?

Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't there an implementation of the config file spec for Go we could use? I think we shouldn't try to reimplement it all here just inside the authfile bits - we desperately (IMO) want drop-ins in other places in just our own stack, ref containers/storage#1885

For Rust we have https://docs.rs/liboverdrop/latest/liboverdrop/ which we use in bootc.

Copy link
Author

Choose a reason for hiding this comment

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

I googled and did not find any, I asked on go forum.

Copy link
Author

Choose a reason for hiding this comment

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

@cgwalters It appears that there is not anything in go that would be handling config files spec per UAPI.
I agree that we should not implement it here and maybe we should not also because none of instances found for config file/dir management in containers library implements fully the UAPI. I looked at other configs that support drop-ins and every implementation is somewhat a bit specific to its files and directories.
https://github.com/containers/image/blob/main/docs/containers-registries.conf.d.5.md#configuration-precedence
https://github.com/containers/common/blob/main/docs/containers.conf.5.md#files
Can we have 3 locations where a system wide auth.json can be found and just one location for drop-in files and they would be read with the following precedence:
/usr/lib/containers/auth.json
/run/containers/auth.json
/etc/containers/auth.json
/etc/containers/auth.d/

Copy link
Contributor

Choose a reason for hiding this comment

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

It appears that there is not anything in go that would be handling config files spec per UAPI.

Hmm...yeah, it would be a scope creep to write one for sure, but it would be clearly beneficial elsewhere.

When the current process is executing inside systemd as root, these paths will be preferred.
Drop-ins always have higher precedence than the configuration file they refer to. Drop-ins are sorted in the lexicographic order. The drop-ins that are later in this order have higher precedence.

When running as a user and searching for the credential for a registry, the following files will be read in sequence until the valid credential is found:
first reading the primary (read/write) file, or the explicit override using an option of the calling application.
If credentials are not present there,
the search continues in `${XDG_CONFIG_HOME}/containers/auth.json` (usually `~/.config/containers/auth.json`), `$HOME/.docker/config.json`, `$HOME/.dockercfg`.

If the current process is not running in systemd, but is running as root, the system-global `/etc/containers/auth.json` path and `/etc/containers/auth.d/` directory will be read last.

Except for the primary (read/write) file, other files are read-only unless the user, using an option of the calling application, explicitly points at it as an override.

Note that the `/etc/containers/auth.json` file and drop-in files from `/etc/containers/auth.d/` directory must not be readable by group or world (i.e. mode `044`), or a fatal error will occur.

## FORMAT

Expand Down
94 changes: 91 additions & 3 deletions pkg/docker/config/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,15 +5,18 @@ import (
"encoding/json"
"errors"
"fmt"
"io"
"io/fs"
"os"
"os/exec"
"path/filepath"
"runtime"
"slices"
ipanova marked this conversation as resolved.
Show resolved Hide resolved
"strings"

"github.com/containers/image/v5/docker/reference"
"github.com/containers/image/v5/internal/multierr"
"github.com/containers/image/v5/internal/rootless"
"github.com/containers/image/v5/internal/set"
"github.com/containers/image/v5/pkg/sysregistriesv2"
"github.com/containers/image/v5/types"
Expand All @@ -35,6 +38,11 @@ type dockerConfigFile struct {
CredHelpers map[string]string `json:"credHelpers,omitempty"`
}

// systemPath is the global auth path preferred for systemd services.
// systemDir is the global auth directory with drop-ins preferred for systemd services.
// These paths are also considered when the process is running as root ( not in systemd).
var systemPath = authPath{path: filepath.FromSlash("/etc/containers/auth.json"), legacyFormat: false, requireUserOnly: true}
var systemDir = filepath.FromSlash("/etc/containers/auth.d")
var (
defaultPerUIDPathFormat = filepath.FromSlash("/run/containers/%d/auth.json")
xdgConfigHomePath = filepath.FromSlash("containers/auth.json")
Expand All @@ -56,6 +64,8 @@ var (
type authPath struct {
path string
legacyFormat bool
// requireUserOnly will cause a fatal error if the file is readable by group or other
requireUserOnly bool
}

// newAuthPathDefault constructs an authPath in non-legacy format.
Expand Down Expand Up @@ -143,8 +153,21 @@ func GetAllCredentials(sys *types.SystemContext) (map[string]types.DockerAuthCon
// The homeDir parameter should always be homedir.Get(), and is only intended to be overridden
// by tests.
func getAuthFilePaths(sys *types.SystemContext, homeDir string) []authPath {
runningInSystemd := os.Getenv("INVOCATION_ID") != ""
Copy link
Member

Choose a reason for hiding this comment

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

Can you elaborate on why systemd is being excluded? Running Podman in systemd by means of Quadlet is widely used, so I want to make sure to understand the reasoning behind.

Copy link
Contributor

Choose a reason for hiding this comment

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

See below, it isn't about excluding systemd cases, the current logic just changes the priority order for the search to search the system paths first - but in order to avoid breaking people today that are e.g. writing to /root/.config we still need to search those paths too.

The ordering logic here is IMO up for debate of course.

Copy link
Member

Choose a reason for hiding this comment

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

Can you elaborate why the order should be different when running in systemd? Certainly, we need to continue using the existing paths but I'd expect .d files to behave consistently.

Copy link
Contributor

Choose a reason for hiding this comment

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

After this we'll have two big sources of auth: "user config" and "system config".

My thought was that when running in systemd we prefer system config over root's (or whatever user identity) first. This would avoid e.g. SELinux denials from system services trying to access root's home directory etc. It would be a fully backwards compatible change because the system wide paths wouldn't exist.

runningAsRoot := rootless.GetRootlessEUID() == 0
runningSystemdPrivileged := runningInSystemd && runningAsRoot

paths := []authPath{}
pathToAuth, userSpecifiedPath, err := getPathToAuth(sys)

// If we're in systemd, prefer the system global auth with drop-ins first.
insertedGlobalPath := false
if !userSpecifiedPath && runningSystemdPrivileged {
rootpaths, _ := walkAuthDir()
paths = append(paths, rootpaths...)
insertedGlobalPath = true
}

if err == nil {
paths = append(paths, pathToAuth)
} else {
Expand All @@ -169,10 +192,60 @@ func getAuthFilePaths(sys *types.SystemContext, homeDir string) []authPath {
paths = append(paths,
authPath{path: filepath.Join(homeDir, dockerLegacyHomePath), legacyFormat: true},
)
// If we didn't already insert the global path and drop-in files from the auth.d dir,
// do it at the end if we're running as root.
// This will ensure the same semantics for code executed as systemd units and run
// from an interactive shell (as root) as long as there's no user-root owned configs.
if !insertedGlobalPath && runningAsRoot {
rootpaths, _ := walkAuthDir()
paths = append(paths, rootpaths...)
}
}
return paths
}

// Walk the /etc/containers/auth.d/ directory and return the drop-in paths with global system path /etc/containers/auth.json.
// Drop-ins always have higher precedence than the configuration file they refer to and are sorted in the lexicographic order.
// The drop-ins that are later in this order have higher precedence.
func walkAuthDir() ([]authPath, error) {
paths := []authPath{}
// append global system path
paths = append(paths, systemPath)

err := filepath.WalkDir(systemDir,
// WalkFunc to read additional configs
func(path string, d fs.DirEntry, err error) error {
switch {
case err != nil:
// return error (could be a permission problem)
return err
case d.IsDir():
if path != systemDir {
// make sure to not recurse into sub-directories
return filepath.SkipDir
}
// ignore directories
return nil
default:
// only add *.json files
if strings.HasSuffix(path, ".json") {
systemDropinPath := authPath{path: filepath.FromSlash(path), legacyFormat: false, requireUserOnly: true}
paths = append(paths, systemDropinPath)
}
return nil
}
},
)
// reverse the order so latest appended file from drop-ins has precedence
slices.Reverse(paths)
if err != nil && !os.IsNotExist(err) {
// Ignore IsNotExist errors: most systems won't have a auth.d directory.
return paths, fmt.Errorf("reading auth.d: %w", err)
}

return paths, nil
}

// GetCredentials returns the registry credentials matching key, appropriate for
// sys and the users’ configuration.
// If an entry is not found, an empty struct is returned.
Expand Down Expand Up @@ -567,7 +640,7 @@ func getPathToAuthWithOS(sys *types.SystemContext, goOS string) (authPath, bool,
}
// Note: RootForImplicitAbsolutePaths should not affect paths starting with $HOME
if sys.RootForImplicitAbsolutePaths != "" && goOS == "linux" {
return newAuthPathDefault(filepath.Join(sys.RootForImplicitAbsolutePaths, fmt.Sprintf(defaultPerUIDPathFormat, os.Getuid()))), false, nil
return newAuthPathDefault(filepath.Join(sys.RootForImplicitAbsolutePaths, fmt.Sprintf(defaultPerUIDPathFormat, rootless.GetRootlessEUID()))), false, nil
}
}
if goOS != "linux" {
Expand All @@ -587,7 +660,7 @@ func getPathToAuthWithOS(sys *types.SystemContext, goOS string) (authPath, bool,
} // else ignore err and let the caller fail accessing xdgRuntimeDirPath.
return newAuthPathDefault(filepath.Join(runtimeDir, xdgRuntimeDirPath)), false, nil
}
return newAuthPathDefault(fmt.Sprintf(defaultPerUIDPathFormat, os.Getuid())), false, nil
return newAuthPathDefault(fmt.Sprintf(defaultPerUIDPathFormat, rootless.GetRootlessEUID())), false, nil
}

// parse unmarshals the credentials stored in the auth.json file and returns it
Expand All @@ -596,14 +669,29 @@ func getPathToAuthWithOS(sys *types.SystemContext, goOS string) (authPath, bool,
func (path authPath) parse() (dockerConfigFile, error) {
var fileContents dockerConfigFile

raw, err := os.ReadFile(path.path)
f, err := os.Open(path.path)
if err != nil {
if os.IsNotExist(err) {
fileContents.AuthConfigs = map[string]dockerAuthConfig{}
return fileContents, nil
}
return dockerConfigFile{}, err
}
defer f.Close()
if path.requireUserOnly {
st, err := f.Stat()
if err != nil {
return dockerConfigFile{}, fmt.Errorf("stat %s: %w", path.path, err)
}
perms := st.Mode().Perm()
if (perms & 044) != 0 {
return dockerConfigFile{}, fmt.Errorf("refusing to process %s with group or world read permissions", path.path)
}
}
raw, err := io.ReadAll(f)
if err != nil {
return dockerConfigFile{}, fmt.Errorf("reading %s: %w", path.path, err)
}

if path.legacyFormat {
if err = json.Unmarshal(raw, &fileContents.AuthConfigs); err != nil {
Expand Down