Disable registry fallback for repository credentials #1304
Conversation
saschagrunert
force-pushed
the
no-fallback
branch
2 times, most recently
from
c91d2b3 to
950f3f7
Compare
|
LGTM |
There was a problem hiding this comment.
I may be missing something, but this doesn’t look right at all.
A login quay.io; pull quay.io/foo should find the registry-level credentials; so dropping the registry name from authKeysForRef does not make sense to me. (And also I can’t see that it makes any difference, because the normalizedAuths lookup will find quay.io anyway!)
The originally-reported problem was that a login quay.io/foo; pull quay.io/bar finds the foo credentials, and shouldn’t. AFAICS the normalizedAuths code will find that both before and after this PR.
And for completeness, login https://quay.io/v1/; pull quay.io/foo (with the login done not using our current/future tools, but something that writes such keys into auth.json directly) should continue to find the URL-keyed credential.
The original proposal talked about how this is governed by the format of the key in the config file, not by format of the query. So that’s another reason why changing authKeysForRef, but not changing the keys = []string{registry} is surprising.
Am I missing something about how this works?
Either way this is a bit tricky, so could you add tests for the three cases above to one of the GetAuth* functions, please?
saschagrunert
force-pushed
the
no-fallback
branch
2 times, most recently
from
2ea37fe to
b78c420
Compare
|
@mtrmac I think I misunderstood what was required but now added the unit tests and aligned the current implementation to it. Sorry for that, please take a look again. |
saschagrunert
force-pushed
the
no-fallback
branch
2 times, most recently
from
9b3c204 to
e5ef111
Compare
There was a problem hiding this comment.
Thanks, this looks closer.
I’m not sure whether some of the design decisions different from containers/common#659 (comment) are accidents or intentional because that proposal is broken; if that’s the case, please write down the rationale, at least here in the PR.
saschagrunert
force-pushed
the
no-fallback
branch
2 times, most recently
from
859e839 to
457c01f
Compare
|
@mtrmac PTAL, I think I addressed all of your comments. |
As mentioned in containers/common#659 (comment), we now do not normalize the registry when being in non legacy format. This means we do not match "quay.io/foo" if "quay.io/bar" is provided as credentials, because both would be normalized to "quay.io" before this patch. We also add a new corner case where auth.json has been external modified to contain an entry like "https://quay.io/v1/", which now also matches if "quay.io/foo" has been provided via the credentials. Signed-off-by: Sascha Grunert <[email protected]>
saschagrunert
force-pushed
the
no-fallback
branch
from
457c01f to
c95f2b4
Compare
As mentioned in containers/common#659 (comment), we now do not normalize the registry when being in non legacy format. This means we do not match "quay.io/foo" if "quay.io/bar" is provided as credentials, because both would be normalized to "quay.io" before this patch.
We also add a new corner case where auth.json has been external modified to contain an entry like "https://quay.io/v1/", which now also matches if "quay.io/foo" has been provided via the credentials.