-
Notifications
You must be signed in to change notification settings - Fork 28
Use registry host rather than URL for creds lookup #176
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
Use registry host rather than URL for creds lookup #176
Conversation
Signed-off-by: itowlson <[email protected]>
|
keirlawson/docker_credential#7 I think the least-bad option here might be to check both with and without scheme. |
Signed-off-by: itowlson <[email protected]>
|
@lann I have added checking with scheme if checking without scheme fails. I'm not able to test with "with scheme" fallback because my creds helper is firmly team "no scheme." It sounds like you've needed scheme in the past so if you're able to give this a try then that would be super helpful - thanks! |
|
I believe "with scheme" is required for |
|
I know folks are busy but I'd be terribly grateful if someone was able to look at this - thanks! |
|
Sorry @itowlson this fell off my radar. Forgive my dayquil brain, but how do I test this? |
|
@fibonacci1729 Unfortunately, I am not sure. I know that the current code does not work on GitHub actions or WSL, but clearly worked for the original authors, which suggests maybe it worked on... Mac and native Linux? @lann suggests a relationship to I guess the answer is "find something that does require schemes and make sure it still works in that environment," but I'm afraid I don't know what that environment is: presumably whatever environment was used for the original work on wkg push...? |
|
I've verified that things seem to work on my mac (not that we didn't know that already). @lann can you provide any clarify here? |
|
Not sure if this was @lann 's intent, but looking into the related Writing a test for this in a platform-agnostic way seems like it'd be tricky though. |
|
@tbrockman Credential stores may vary - my WSL stores credentials in |
|
I've verified no regression in at least one scenario, which I think is good enough given the limited risk here. |
There is an issue in GitHub actions where
wkg publishfails with codeDENIEDeven after a successfuldocker login.The reason appears to be that
wkgasksdocker-credentialto look up credentials for the schemefulserver_url(e.g.https://ghcr.io), whereas~/.docker/config.jsoncontains bare hosts (e.g.ghcr.io).docker-credentialdoes not normalise away the schema of the passed-in lookup key, so never matches the schemeful entry.This PR proposes changing the lookup to use the
oci_registryof the backend type, rather than the URL.This also fixes an issue on WSL, where the
docker-credential-desktop.exestore also requires matching the bare host.That said, I don't know what else is relying on having the full URL. The current code has been there for nearly 18 months and I'm assuming it was done that way for a reason. So if we want to be more conservative, we can try a more complicated solution where we test with the
oci_registryfirst, and if that fails, try with the constructedserver_url. I'll let wiser heads decide whether that's worth it!