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

Add /etc/containers/auth.json and /etc/containers/auth.d/ dir #2600

Draft
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

ipanova
Copy link

@ipanova ipanova commented Oct 15, 2024

This is a follow-up of this PR #1746 - original description follows (originally written by by @cgwalters ).


EDIT: Design discussion in #2614


A long-running tension in the docker/podman land is around running as a system service versus being executed by a user. (Specifically a "login user", i.e. a Unix user that can be logged
into via ssh etc.)

For login users, it makes total sense to configure the container
runtime in $HOME.

But for system services (e.g. code executed by systemd) it
is generally a bad idea to access or read the /root home
directory. On image based systems, /root may be dynamically
mutable state in contrast to /etc which may be managed
by OS upgrades, or even be read-only.

A compounding problem today is that the podman login default is to write to $XDG_RUNTIME_DIR which is not persistent - but often we want persistent state.

For these reasons, let's introduce "system wide" persistent
authfiles that follow the config files specification.

alignment with bootc/ostree

Many image based systems will want to run containers via system services, and may specifically mount /home and /root as tmpfs. Today of course, they could put authfiles elsewhere and copy them on boot into /root (or pick custom authfile locations and point at them directly) but I think many simple use cases will be happy with "default global pull secret" as a baseline starting point.

Kubelet

An important goal is that kubelet can use these locations in addition to /var/lib/kubelet/config.json as well. I am not aware of any work on kubelet upstream to change its model, but it's not required; we can recommend that kube users basically do ln -sr /var/lib/kubelet/config.json /etc/containers/auth.json perhaps.

But probably what we want is a way to tell the kubelet to also inherit the default CRI's authfile paths?

@ipanova ipanova marked this pull request as draft October 15, 2024 14:48
@ipanova ipanova force-pushed the follow-up-auth branch 3 times, most recently from e11c979 to 23e3edb Compare October 23, 2024 10:48
@ipanova
Copy link
Author

ipanova commented Oct 23, 2024

@mtrmac @vrothberg @rhatdan @cgwalters PTAL

This is a follow up PR of #1746
It does not contain tests yet.
There has been a lot of discussion but here's the summary.

What it does:

  • introduces new system-wide /etc/containers/auth.json and directory with drop-ins /etc/containers/auth.d/
  • these paths are preferred when process is ran in systemd as root
  • these paths are considered as last when process is ran by root ( not in systemd)
  • the precedence of drop-ins is being follow by these specs https://uapi-group.org/specifications/specs/configuration_files_specification/#drop-ins
  • error is raised if these paths are readle by g/o

By introducing system-wide auth file and auth.d dir it covers cases for:

  1. system services, like bootc and its logically bound images where code is executed by systemd
  2. where there is single node use with root access

@ipanova ipanova force-pushed the follow-up-auth branch 3 times, most recently from 5a15faf to 5e2dd4d Compare October 23, 2024 12:09
Copy link
Member

@vrothberg vrothberg left a comment

Choose a reason for hiding this comment

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

Can you add tests for it as well?

I guess the tests could overwrite the .d directory with a custom directory that the tests control.

on Windows and macOS, at `$HOME/.config/containers/auth.json`.

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.

@@ -143,8 +153,24 @@ 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.

@ipanova ipanova force-pushed the follow-up-auth branch 2 times, most recently from 2aa8529 to 4d0dcd8 Compare October 23, 2024 12:28
@mtrmac
Copy link
Collaborator

mtrmac commented Oct 23, 2024

I’m sorry, I am not silently ignoring this, but I’m also swamped with priority work now.


Please sign off per CONTRIBUTING.md to pass the DCO check and allow other tests to run. (Skopeo tests, are, as of today, failing on a version mismatch, that’s blocked on containers/storage#2143 )

@ipanova ipanova force-pushed the follow-up-auth branch 2 times, most recently from 5890231 to 78e97ef Compare October 25, 2024 11:00
cgwalters and others added 5 commits October 25, 2024 13:25
A long-running tension in the docker/podman land is around
running as a system service versus being executed by a user.
(Specifically a "login user", i.e. a Unix user that can be logged
 into via `ssh` etc.)

 For login users, it makes total sense to configure the container
 runtime in `$HOME`.

 But for system services (e.g. code executed by systemd) it
 is generally a bad idea to access or read the `/root` home
 directory.  On image based systems, `/root` may be dynamically
 mutable state in contrast to `/etc` which may be managed
 by OS upgrades, or even be read-only.

 For these reasons, let's introduce `/etc/contaners/auth.json`.
 If it is present, and the current process is executing in
 systemd, it will be preferred.  (There's some further logic
 around this that is explained in the manpage; please see that
 for details)

cc coreos/rpm-ostree#4180

Signed-off-by: Colin Walters <[email protected]>
Signed-off-by: Ina Panova <[email protected]>
Signed-off-by: Ina Panova <[email protected]>
Signed-off-by: Ina Panova <[email protected]>
@cgwalters
Copy link
Contributor

Since there was some legitimate continuing debate about the desirability and design of this, let's split that out into #2614

@mtrmac
Copy link
Collaborator

mtrmac commented Nov 4, 2024

An attempt at a summary of a video call, from my POV — so that we can see where there are more questions, and so that we can continue where we left off::

  • There’s a discussion about subscription-manager automatically providing registry credentials, in a way which is automatically available to everything on the node. I think that’s fine, and it should be read by truly everything (not restricted to files 0600 root:root).
  • Other than this, in the simple cases, adding more default paths specific to root or systemd services is not very attractive to me:
    • A completely task-dedicated computer (like an OpenShift node) is probably managed by automation, and that automation can point at any credential files it wants, so hard-coding defaults in c/image is unnecessary
    • A multi-purpose computer with multiple applications (“a pet”) should, typically, separate registry credentials used by different services, so that impact of vulnerabilities is constrained (both by ordinary credentials and by SELinux); making it attractive for users trying to run N services with N different credentials to put all N of them in a single file, and to make that file accessible by N services, would encourage bad security practices.
  • To me, the much more interesting proposal was to allow an “~immutable /etc”, or more precisely /etc (mostly?) “functionally” generated from ConfigMap-like inputs, in order to reduce the truly mutable state that exists on the system; and in that case those ConfigMap-like inputs need some destination to write into.
    • Overwriting /root/.config/containers/auth.json by automation is obviously undesirable
    • Overwriting /run/user/0/containers/auth.json would be a bit better, but, still, having the user-managed state from podman login interfere with / overwrite the credentials from the ConfigMaps is unintuitive [A]
  • So, something that might make sense:
    • /etc/containers/[global-]auth.json{,.d} for the subscription use case, and other truly node-global credentials
    • /etc/containers/root-auth.json{,.d} (.d to allow multiple ConfigMaps, probably — and because we would have that already implemented for global-auth.json.d anyway) for credentials only used by root (and we might enforce that they are only readable by root)
  • My questions:
    • Do we, after all, want to read root-auth.json before /root/.config/, for the systemd services (and for $unknown other use cases??), to silence SELinux denial noise? Maybe the PR as proposed is actually what we want here.
    • (Added after the call:) Looking at [A] again — do we ever want podman login and the ConfigMaps to interact / interfere? It seems to me that the answer is an obvious “no” … and that would point towards not having a default root-auth.* at all, again, and forcing services that do want to use this root-on-the-node set of secrets to use an explicit --authfile option, so that they choose between the “interactively-managed credential locations” and the “automation-managed credential locations”.

@cgwalters
Copy link
Contributor

There’s a discussion about subscription-manager automatically providing registry credentials, in a way which is automatically available to everything on the node. I think that’s fine, and it should be read by truly everything (not restricted to files 0600 root:root).

Link?

A completely task-dedicated computer (like an OpenShift node) is probably managed by automation, and that automation can point at any credential files it wants,

Yes but we have the papercut that anyone debugging things interactively needs to run podman --authfile=/var/lib/kubelet/config.json - which we arguably could and should fix today by just symlinking /run/user/0/auth.json -> /var/lib/kubelet/config.json by default.

This does relate though to the podman login discussion in that podman login becomes a trap in that it'd write to the machine managed state, but I guess we really just need to fix that ultimately by a configmap-style approach where the cluster-owned state is actually readonly anyways.

Do we, after all, want to read root-auth.json before /root/.config/, for the systemd services (and for $unknown other use cases??), to silence SELinux denial noise?

To be clear in practice I think there aren't any denials today in the Fedora policy; systemd units are effectively unconfined by default including reading/writing to home directories.

Anyways when you have a minute can you glance at #2614 ?

@mtrmac
Copy link
Collaborator

mtrmac commented Nov 4, 2024

There’s a discussion about subscription-manager automatically providing registry credentials, …

Link?

I remember an in-person discussion at DevConf; I’m not sure there is a tracker.

@ipanova ipanova changed the title Follow up auth Add /etc/containers/auth.json and /etc/containers/auth.d/ dir Nov 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants