-
Notifications
You must be signed in to change notification settings - Fork 378
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
WIP: Add a /etc/containers/auth.json
#1746
Conversation
Specifically for example I plan to ensure that bootc to reads this file and not |
(Lightly tested with an override to point skopeo at this) |
For OpenShift I think it would make sense to have |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! Using a PR to discuss the details is definitely helpful. Just a quick first impression for now.
For OpenShift I think it would make sense to have
/var/lib/kubelet/config.json
be a symlink to/etc/containers/auth.json
.
Yes, if there is any kind of computer where having a system-wide credentials file makes sense, an OpenShift node should be that. OTOH does this mean we are giving up on some SELinux restrictions on root
? The /etc/containers
path is necessarily unrestricted. I didn’t look into how the Kubelet is, or isn’t restricted.
Maybe we could do it the other way, and symlink /etc/containers/auth.json
to the Kubelet path.
@@ -204,7 +208,17 @@ 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 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding the new paths to this function means that writers would never write to systemPath
without a specific override; is that intended?
I think that’s actually a good idea, so that root
’s interactive login doesn’t write to a noninteractively-used file, but it should be documented.
@@ -204,7 +208,17 @@ 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") != "" | |||
runningAsRoot := os.Getuid() == 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At a first glance it seems that this should use rootless.GetRootlessEUID
. (Admittedly the rest of this file doesn‘t, and it probably should as well…)
pkg/docker/config/config.go
Outdated
} | ||
perms := st.Mode().Perm() | ||
if (perms & 04) > 0 { | ||
return dockerConfigFile{}, fmt.Errorf("refusing to process %s with world read permissions", path) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The documentation of the field says “readable by group or other”, the man page says “mode 0600”.
I’m not sure what we want[1] but it should be consistent.
[1] Starting with 0600 would allow us to possibly relax the restriction later, so that seems to be the conservative choice. Also, we are only reading the file when the user is root, so no other permissions seem necessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For now I chose to error on group/world read permissions. Forcing 0600 also forces write permissions, but I can imagine cases where one might not want that. (Not that it really matters because root usually has CAP_DAC_OVERRIDE
)
@@ -204,7 +208,17 @@ 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 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With growing complexity, it seems more important to have unit tests for this. But that’s the very last step.
Good question. Today,
It could make sense to introduce a dedicated |
A variant of this is that we could:
This I think would get about 45% of the value with 1% of the work. |
I’m not thinking so much whether we could isolate this in But that’s just me rehashing the previous discussion, I guess. It seems to me that single-purpose appliances can use |
Overall, so far I’m not convinced that adding this default path is clearly worthwhile. But it’s useful enough in some contexts that I’m fine with it being added. |
I think it is possible because today most invocations of podman are going to start from
Well, we have to account for mixed setups. For example, a valid user story here is a company/organization deploying Linux desktop systems using So I see this as just adding a dedicated file for "the system" without conflicting with any optional setup.
Thanks; this is a useful debate to have for sure because once added, it can't be removed and so there's long term maintenance implications for sure. |
(BTW OpenShift, at least in some versions, runs Podman from inside a NetworkManager hook… it’s containers all the way down.) |
Instead of packing and unpacking it into two variables in various places. Prep for adding a third member of this struct as part of containers#1746
Done in #1765 |
Instead of packing and unpacking it into two variables in various places. Prep for adding a third member of this struct as part of containers#1746 Signed-off-by: Colin Walters <[email protected]>
I wouldn’t be too surprised if they were insufficiently isolated from the per-user state. If so, that’s not intentional. |
Instead of packing and unpacking it into two variables in various places. Prep for adding a third member of this struct as part of containers#1746 Signed-off-by: Colin Walters <[email protected]>
76b414f
to
838b0f9
Compare
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]>
838b0f9
to
e998b2f
Compare
Yeah true 😢 But an attempt to default SELinux confine NetworkManager hooks IIRC failed, and I think the status quo there is the default is unconfined. |
OK I rebased 🏄 this - it's definitely cleaner on top of #1765 Fixed some comments and issues, but still more to do. |
return dockerConfigFile{}, fmt.Errorf("stat %s: %w", path.path, err) | ||
} | ||
perms := st.Mode().Perm() | ||
if (perms & 044) > 0 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
!= 0
seems a better fit with the “bit set” format.
We might also want to investigate https://systemd.io/CREDENTIALS/ here. |
OK yes I dug into this a bit more recently and I think it'd be nice to support this more natively. The credentials path is a bit orthogonal in a way to a plain old My proposal here is basically we document and support a flow like this:
This is invoked by provisioning/configuration management tooling (and the choice of filename here is just a recommendation). Same as all other uses of systemd credentials. Then anything which wants to read this (e.g. skopeo inspect/podman pull) needs to be invoked from a systemd unit, and set Now what we would do inside containers/image here is check whether Here's an example command I used to test this flow:
Except the benefit of supporting
(Cleanly separating out a distinct pull secret) Then when invoking systemd units, one would set |
I recently came across https://access.redhat.com/solutions/7002142 which has a really bad suggestion for how to make |
@cgwalters, what are your thoughts on this issue today? I am carefully walking over the bootc docs and think that the status quo does not offer a good UX. Having an I didn't read the code yet but would expect that |
What would the protections on that file be by default 600 would be my suggestion, and then people who wanted to share it with non root users could open it up, since it has a non encrypted password embedded in it. |
Oh yes I think we should definitely still do this. |
@vrothberg That link isn’t publicly available |
My thoughts in general:
|
I'd agree that in the general case of per-image/per-service pull secrets makes total sense. But I do feel for people trying out container-based systems at a smaller scale (especially the important proof-of-concept phase), it's highly likely they start with just one authenticated registry and they'd be perfectly fine with a global secret to start (especially one that applied to bootc and podman), with the ability to override for individual images being the next step.
This would definitely be nice indeed. For clarity and completeness, this would hopefully really be having an auth file stack that implemented the https://uapi-group.org/specifications/specs/configuration_files_specification/ |
As long as they are at the end of the search list = do not incur a cost for applications with more explicit credential locations, why not. |
I randomly came across https://docs.redhat.com/en/documentation/red_hat_enterprise_linux/8/html-single/composing_a_customized_rhel_system_image/index#con_the-container-registry-credentials_assembly_pushing-a-container-to-a-register-and-embedding-it-into-a-image today which created another path for pull secrets - if I had to guess at the rationale it'd be similar. |
That looks like a single application having a place to configure credentials used for that application (and, of course, it’s fine for such things to exist); if that expresses a need for need for a system-wide credential store, I don’t know enough to see it. |
I do continue to think that many simple use cases would be happy with a default global path and |
Closing in favor of #2600 where development continues |
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
homedirectory. On image based systems,
/root
may be dynamicallymutable state in contrast to
/etc
which may be managedby 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]