Handle not-logged-in public repos with credsStore

[fahhem]

Tested with a docker config that only has a credsStore but without an account for index.docker.io. Previously, this would result in a 401 when requesting the realm URL, but now it succeeds.

{
        "auths": {},
        "credsStore": "devpod"
}

[thesayyn]

Thanks for the PR. This sounds like non-spec behavior I have never seen a docker credential helper returning empty username and password.

[fahhem]

Usually they're used for private repositories, but for devpod it's used for all repositories, including public docker hub ones.

[thesayyn]

I'd know for sure if there was reference implementations doing the same thing, othewise this sounds like a something non-spec compliant cred helper would do.

[fahhem]

Is there a spec for this? If so, I can take it to the devpod repo and ask them to fix it instead. I don't care where I fix this, I just don't want to maintain a patched rules_oci nor a patched devpod.

If there's no spec, and it's just based on implementations, then rules_oci should be resilient to more implementations.

[thesayyn]

only one that i am aware of is; https://github.com/docker/cli/blob/master/docs/reference/commandline/login.md#credential-helper-protocol

[fahhem]

Unfortunately, that doesn't specify what to output when no username is needed/available. Both an empty string vs json with empty values seem like valid guesses based on that page.

[fahhem]

@thesayyn This seems like spec-compliant behavior that should be handled. I looked into changing devpod, and in doing so I tried looking at docker's source code that credential helpers use or are based on. It looks to me like they would do the same behavior (returning a blank Username and Password rather than an empty {}): https://github.com/docker/docker-credential-helpers/blob/10845d8f94cd3c7dbbea05ac8e6e3af16acc68f2/credentials/credentials.go#L160

Could you approve or close this PR and I'll maintain my fork indefinitely, but without bothering to upstream further?

[thesayyn]

It looks to me like they would do the same behavior (returning a blank Username and Password rather than an empty {})

Yes, that makes sense, i am not sure how this translates to the wire though, but if docker pull works with same configuration and it doesn't with oci_pull and then its bug here.

I'd feel more comfortable if there was a test here in rules_oci so we don't break this in the future?

[fahhem]

I've added a test, but unfortunately I can't figure out how to run your tests successfully on my machine. I get 404/401 failures in the e2e auth tests that already exist, so I apologize that I'll be relying on your github actions to test it for me. That's exacerbated by the fact that I don't know what the no-auth assertion should look like until I get it to run correctly at least once

[thesayyn]

That test was broken anyway, thank you for adding the test case.

[fahhem]

Are the tests expected to mostly fail this way? Do you need some help fixing the tests?

[thesayyn]

Are the tests expected to mostly fail this way

No they shouldn't be failing. Can you rebase your branch?

[fahhem]

Looks like I need approval to run the CI workflows after the rebase

[fahhem]

Looks like they fail the same way, including in another unrelated PR from a week ago. I figured it out, looks like #759 forgot one last leading underscore to remove. With that fixed all the tests pass again