podman create with non-existing --authfile should error on pull

[vrothberg]

podman create seems to ignore (or not use?) the non-existing authfile while podman pull fails immediately.

podman (lint) $ podman images
REPOSITORY  TAG         IMAGE ID    CREATED     SIZE
podman (lint) $ podman create --authfile /tmp/idonotexist busybox
Resolved "busybox" as an alias (/etc/containers/registries.conf.d/000-shortnames.conf)
Trying to pull docker.io/library/busybox:latest...
Getting image source signatures
Copying blob 71d064a1ac7d done  
Copying config b539af69bc done  
Writing manifest to image destination
Storing signatures
67b48cd18eee3c9c0b2715ebb1b5acc36636fcce4aa07225c8faf44dea5dcb9e
podman (lint) $ podman pull --authfile /tmp/idonotexist busybox
Error: stat /tmp/idonotexist: no such file or directory

Detected in #18931 (comment) where the test must be fixed as well.

[vrothberg]

Ah, OK. That is actually expected behavior in c/image. Authfiles are not expected to exist:
https://github.com/containers/image/blob/37a978d2154618d8b6b9415ab173d503228e83c5/pkg/docker/config/config.go#L552-L559

I am personally OK with this behavior but desire consistency across commands. As shown above, pull will fail as Podman stats the file.

@Luap99 WDYT?

[Luap99]

That is pretty much the same discussion as in #18413.

If c/image silently ignores a non existing path then I agree we should error out in podman. However I still believe the correct thing is to only error out at the place were the file is actually used, otherwise we will always face TOCTOU bugs, but if c/image never does that then it is unavoidable I guess.

And I really do not get why c/image would behave in such a way, AFAICT it does so since the beginning: containers/image@f28367e1a
Is it really a sane behaviour of a library to silently ignore the given authfile just because it does not exists? This leads to hard to debug errors because I would assume the following pull/push would fail if you typo the path instead of clear error that your path is invalid.

Fixing this in the podman cli seems wrong as it duplicates the checks for each individual command. And then how does this compare to skopeo, buildah and cri-o? Do they also make sure to check for that?!

cc @mtrmac

[mtrmac]

I quite agree that a library, when explicitly instructed to read a path, should fail if that path is missing. (Especially now that setting SystemContext.AuthFilePath completely disables reading other locations, which was not always the case.)

OTOH when the user does not make any such explicit instruction, and the library is reading the default location, silently ignoring a missing file is very often the right thing to do — consider a fresh user account running an anonymous podman pull quay.io/some-public/image; it would make no sense to fail until the user runs podman login something_unrelated_why_would_the_user_do_that, especially when the file is in /run and deleted on every reboot. That’s the basic explanation for why the code path to ignore missing files exists in c/image in the first place, though not for why the check is sloppy.

---

Where this gets difficult is that Podman, and other callers, like to set the c/image options without user input, e.g.

podman/cmd/podman/images/pull.go

Line 108 in 3907df6

flags.StringVar(&pullOptions.Authfile, authfileFlagName, auth.GetDefaultAuthFile(), "Path of the authentication file. Use REGISTRY_AUTH_FILE environment variable to override")

sets that based on an environment variable, and that variable is, in turn, possibly set by default: https://github.com/containers/skopeo/blob/caf8c552c67aeae0bbfb79f1571511b4c3234a7e/contrib/skopeoimage/stable/Containerfile#L44 .

So failing if c/image sees SystemContext.AuthFilePath pointing at a missing file would, right now, introduce failures in previously-working setups.

(Not centralizing the defaults deep in c/image leads to such things. OTOH c/image would prefer to not silently depend on environment variables, so a reasonably clean solution accommodating both, something like https://github.com/containers/image/blob/main/pkg/cli/environment/environment.go , is more work for both the caller and the callee.)

[vrothberg]

I do not feel strongly unless we risk breaking existing workloads / users. Once we've settled on how we want things to behave, we can make sure --authfile works consistently across the various commands.

[rhatdan]

Can we check if the caller is using a default value, if yes then don't complain on NOEXIST, else complain?

[mtrmac]

We can’t tell by the value. We would need a new field in SystemContext (either AuthFilePathExplicitlySpecified string, or AuthFilePathWasExplicitlySpecified bool, and keeping the current behavior of silently ignoring missing files for existing callers). And then callers would opt in, either Podman directly or in c/image/pkg/cli/environment.

[github-actions]

A friendly reminder that this issue had no activity for 30 days.

[rhatdan]

I believe @vrothberg is working on this one.

[vrothberg]

I believe @vrothberg is working on this one.

It's very low on my priority list so I unassigned to make space if others want to tackle it.

[rhatdan]

Ok I think if the client specifies a path, then the path should be verified.