Add docker.io to unqualified image name #12321
Conversation
|
Also, I would prefer having a constant somewhere rather than hardcoding docker.io, but I haven't found one in the existing codebase. I doubt the moby project is going to change that soon, so maybe that's fine. |
Since that's the default behavior of moby engine, the compat API need to reflect that. Fixes containers#12320 Signed-off-by: Michael Scherer <[email protected]>
| if err != nil && shortnames.IsShortName(fromImage) { | ||
| fromImage = fmt.Sprintf("%s/%s", "docker.io", fromImage) | ||
| _, err = shortnames.Resolve(runtime.SystemContext(), fromImage) | ||
| } |
There was a problem hiding this comment.
I was suggesting something like:
if err != nil && shortnames.IsShortName(fromImage) {
names, err = shortnames.Resolve(runtime.SystemContext(), fromImage)
if len(names) == 1 {
fromImage=names[0]
} else {
fromImage = fmt.Sprintf("%s/%s", "docker.io", fromImage)
}
}
There was a problem hiding this comment.
That way if I have a shortname mapping, Podman service will use it, if there is not one then fail over to docker.io.
There was a problem hiding this comment.
Oh yes, my bad; I will fix it.
There was a problem hiding this comment.
but if a software ask for ubi8, shortname.Resolve shouldn't fail in the first place, so it wouldn't go in that condition, no ?
There was a problem hiding this comment.
I am not sure what you are asking. shortnames.IsShortName("ubi8") should return true.
Then I would want to pull registry.access.redhat.com/ubi8 not docker.io/ubi8.
There was a problem hiding this comment.
Yes, but the code just before (line 257):
_, err := shortnames.Resolve(runtime.SystemContext(), fromImage)
will return nil in err if fromImage is a short name alias, since it can be resolved.
If there is no error, fromImage will not be modified.
There was a problem hiding this comment.
This is ignoring errors in short-name resolution which I think is something we cannot do. Let's go one step back and first define the problem we want to solve. This looks like a way to big shotgun to me.
|
Also, it seems github do not refresh comments (or I have a issue with my browser), as I just seen your 2nd comment on the 1st version of the PR, sorry. |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mscherer, rhatdan The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
the
approved
There was a problem hiding this comment.
I do not agree with the change. It is a breaking one that can be avoided.
If users wish to only resolve to docker.io, they are free to change that in /etc/containers/registries.conf. We also need to care about users migrating over from RHEL 7 (atomic) Docker which supports search registries.
|
/hold |
openshift-ci
bot
added
the
do-not-merge/hold
|
Sorry for the ping-pong, @mscherer. |
|
This is only for the compatibility API, which does not effect RHEL7 Docker. |
I am convinced it does affect RHEL7 Docker. Scripts on RHEL 7 are only speaking compat API, so it will impact upgrades. |
|
There was no support for docker-compose or docker-py on RHEL7. We don't support them now. |
|
Also if we say this is for podman 4.0 then we can have breaking change. |
|
It doesn't change my take on the proposed change. If users want to resolve to |
|
This does not fix the issue for docker-py and docker-compose on non MAC. |
Can we define exactly what the problem is? |
|
Docker compatibility means shortnames map to docker.io. |
|
Ok so the current problem is that the go code from moby, used by a few projects (for example, woodpecker, which I am trying to get running on Fedora without using Docker) will convert a call of: to a POST request to So even if a client use a fully qualified name with the registry, Moby client go code switch to a registry-less name (eg, "familiar name" in the moby API), and is assuming to have a docker daemon that will default to docker.io registry on the other side. While the proper engineering fix is to convince moby upstream to change that practice, I doubt this will happen (eg, podman origin story). |
Okay, so we agree on the problem. I stick with configuring |
|
I pondered about the triggering a error on the shortname path, but from a quick look, it seems this requires the attacker to be privileged enough to mess with it on the system. There is 8 errors condition on the shortname code:
Out of 8, 6 seems non applicable. There is the issue with ParseNormalizedNamed and parseUnnormalizedShortName. But the only way a attacker would be able to trigger a error there is by asking to parse attacker controlled string (eg, the image name). But then, if the problem is that a attacker can provide a string that create a parse error to later have that string be prefixed by "docker.io", the attacker can simply just prefix docker.io themself and be done with it. So I understand your point about being security sensitive, but I can't see how it would be used. Currently, the only type of attack would result in changing a image name, and either requires to be root (or a equivalent, eg, enough access to change config), or to control the image name in the 1st place. As for the uncompleteness of the patch, I agree. However, I do not see on top of my head any others operations that would deal with remote registries in the docker API. And asking to change |
Ignoring an error from as security-sensitive API (and ignoring a global configuration the rest of the entire code base is using) is a recipe for running into problems.
I guarantee there are dozens of code paths where this may happen. Also consider long-term maintainability. Sticking to a global configuration that all code paths use makes sure we're not regressing or diverging in the future. Hard-coding one location and leaving the rest untouched does not give that guarantee.
I sympathize with your desire and I find search registries painful for such cases but there is no one-size-fits-all solution. Also as mentioned before, the atomic Docker in RHEL7 and older Fedoras is using these search registries. Existing users may very well depend on the current behavior. Using Podman 4.0 as an "excuse" for a breaking change is not attractive to me, since there is a way to support both scenarios by not changing anything (again Ignoring |
|
The root of the problem is that we can either be compatible with Docker API (which is not secure), or maintain the current behavior (not compatible with Docker API). So if we drop the compatibility (and I would say that not being compatible with the reference client implementation on a CreateContainer is not a niche operation), where the limitation should be documented ? |
Why is that not secure? Aliases would still work and configuring "docker.io" only is unambiguous.
Yes.
That really depends. Some distributions set the search registries to "docker.io" only and there the issue doesn't apply.
I am very biased since I spent so much time on this topic. |
|
You making every user that could hit this, have the issue, and telling them to modify there registries.conf. |
I am a bit lost. You said that ignoring the shortname policy (eg, defaulting to choosing docker.io when the policy is to ask users to choose) is a security risk. So the docker policy of defaulting to docker.io is a security risk.
Yeah, but they are not the distros I use. RHEL/Centos do have multiple search registries, Fedora does too. And podman as a project want to promote having multiple registries, so I think having to choose between compatibility with existing tooling, and a long term goal of decentralization is not a great tradeoff to decide
As a user of woodpecker, the required config change should be in the error message, because that's what is displayed when i try to use podman (in red). As a sysadmin, a line in the log that say "insecure compat API use detected, do X if you want to make it work" would surely help, as that's where I would look. As for others personas, I guess people from support might want to have a KB article on their respective knowledge base (eg, access.rh.c for RH, whatever Suse use for Suse, etc), and I guess the website could be updated, but there is no section on the api compat. A blog post could help for sure, that's how I found about the docker API compat (since the initial plan for woodpecker was to add a podman backend, but this doubled the size of the vendor directory and added several non go dependecnies). |
So far this is the first time a user reports this issue. And we must not ignore backwards compatibility: turning it around, this PR would break all existing deployments depending on the current behavior but without a workaround or solution. And again, we had consensus on this very behavior, and I am afraid changing that too quickly is risky.
That is also why we enforce docker.io for I also want users migrating on the CLI to work out of the box, but they may face the very same issue as well. The ask here is to have a separate short-name resolution for the compat API. I don't know how to do that since |
Only ignoring the errors is risky. If there is only one search registry, the resolution will work without a problem and there is no risk at all. So defaulting to docker.io is not an issue but we must not ignore errors. |
Given that's already my 3rd PR to make the compat API inline with Moby behavior ( #12279 , #12318 ), I suspect the compat API is maybe not used that much. While #12279 is a corner case, #12318 is noticeable, since it would break anytime you try to create a container with a fully qualified name. However, as the issue can be worked around easily by downloading the image manually a first time, I suspect people just used the workaround. But to go back on fixing the issue, would a patch that replace runtime.SystemContext() with a specific context just for the compat API be ok ? Eg, have a specific context that force docker.io as a single search registry, and inherit the whole system context for all others settings. This way, we use that context everywhere in the compat API (and so it get used everywhere below), we do not have to ignore the errors, and it will not requires users to change the behavior of the non compat API, nor to change the config for that. |
I fear not since it is only changing this specific code path while all others would behave differently in the compat API. Even changing everything in c/podman would not be sufficient since we're calling c/buildah and c/common as well. |
If we change everywhere in
But so, shouldn't buildah and common inherit the context of the request from the podman code ? |
That varies depending on the call sites and the APIs. When it comes to search registries, the source of truth is |
If it wasn't the case, this would be a bug with or without the change, but would it be detectable ? EG, is there a way to run the test suite that would trigger a bug ? Cause even if I say "I audited the code", I suspect this wouldn't be sufficient. I guess one way would be to run the test suite and check that registry files are read only once, or something like that (or instrument the code to log a string each time the object is created or use a global var, something like that). But so, altering the systemContext would be a acceptable fix ? |
Not necessarily a bug but at least an inconsistency.
Not for me, no. Again, enforcing docker.io in the code is not an acceptable change to me. I am convinced that if users wish to only resolve to docker.io, then they should change |
|
Do we want to discuss this at the cabal? I think there's a compatibility case to be made for the default prefixing. |
Sure we can |
Ok, so another proposal, what about disabling the compat API if there is multiple registries ? People wishing to use podman as a docker replacement would need to be explicit by changing the configuration. |
Please consider what I wrote above already. There are users of the compat API relying on that behavior. Breaking backwards compatibility is something we need to consider. I understand that your specific use case doesn't work out-of-box on Fedora but there is an easy workaround by editing |
|
Following @mheon suggestion, and @vrothberg agreement, I added the topic on the agenda on hackmd, and on my calendar as well. Should I also send a email to the list ? |
|
@mscherer I've tweaked the description in the md just a bit for a little more context. I sent a note to the list earlier pointing at the md for the agenda. If you wanted to send another, I don't think it would hurt. |
|
It would be good to have @rhatdan and @mtrmac in today's cabal as well. Note that we need to make sure to not turn in circles since time is limited. Since we had this very conversation multiple times, I would love this to be the last one. Once there is a new consensus, I will write it down somewhere here in the docs so we have something physical to refer to. |
|
I will be there. |
|
So, closing this PR since that would not be the right way to solve the bug (and we can discuss on the initial bug ). However, I wonder if the error message could be improved, since right now, the message is geared toward interactive use, and that's not suitable for the API. |
|
Thanks for pushing this forwad, @mscherer. Once |
|
For the record …
docker/docker compatibility means that. projectatomic/docker compatibility means shortnames resolve to the search path. We never broke that, to my knowledge (and we have had ample opportunity to consider breaking it, and did discuss it IIRC, at the time of the
Yes, this inability to tell whether a client using the upstream Note that this really matters only for clients that have a multi-element search path. And the ultimate quoted failure:
Requires both a legacy system with the multi-element search path configured, and a system with configuration fresh enough to have the non-permissive short name mode chosen. Such systems definitely can exist, but the enforcing configuration is there exactly to motivate clients’ transition to being explicit about the intended name. Users of such systems can, I think, either do just that, and modify clients to submit fully-qualified names, or change the configuration (probably to set up the search path to contain only All new distributions, and new installations of future (current?) versions of old distributions, should only ship with Does all of this break in some other way I’m missing? |
|
@mtrmac We had a discussion on this yesterday. and we came to the conclusion that for the Compatibilty API that we should enable to do what is best for the users. We talked about potentially having a flag in SystemContext to either allow callers to specify Docker.IO Mode, or to specify the unqualifiedRegistrieslist. Then it would be up to engines to decide how they want to handle overriding the systems registries.list. |
github-actions
bot
added
the
locked - please file new issue/PR
Since that's the default behavior of moby engine, the compat
API need to reflect that.
[ NO NEW TESTS NEEDED ]
Signed-off-by: Michael Scherer [email protected]
What this PR does / why we need it:
This PR should fixe #12320
How to verify it
Same as my other PR, trying to get woodpecker running.
Which issue(s) this PR fixes:
Fixes #12320
Special notes for your reviewer:
There is no test added, because adding them would requires to download image from docker.io, but this doesn't seems wise due to the rate limit in place on the registry.