Fix Azure Container Registry authentication

[simongottschlag]

Fixes #1255

I'm not sure if this is the best or preferred way of handling this, another way may be to add a check to NewKeychainFromHelper() that checks if the username is <token> and adds IdentityToken to the AuthConfig{}.

Any and all feedback is welcome and if you see that I've missed something obvious and that's why I've done this, please point it out and I'll fix the issue where it's located.

[simongottschlag]

I will try to fix the corporate CLA on Monday.

[codecov-commenter]

Codecov Report

Merging #1256 (94987f2) into main (890d5b3) will decrease coverage by 0.02%.
The diff coverage is 40.00%.

@@            Coverage Diff             @@
##             main    #1256      +/-   ##
==========================================
- Coverage   73.77%   73.74%   -0.03%     
==========================================
  Files         111      111              
  Lines        8285     8289       +4     
==========================================
+ Hits         6112     6113       +1     
- Misses       1571     1573       +2     
- Partials      602      603       +1     

Impacted Files	Coverage Δ
pkg/authn/keychain.go	81.53% <40.00%> (+0.28%)	⬆️
pkg/v1/layout/write.go	48.88% <0.00%> (-0.48%)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 890d5b3...94987f2. Read the comment docs.

[imjasonh]

Thanks so much for your investigation into why this was broken, and for your thorough description, and for your PR. If we're ever in the same place and you'd like a beer, it's on me. 🍻

I think I'd be okay changing NewKeychainFromHelper to check if username == "<token>" and pass the password as the auth config's IdentityToken, if that's how I'm supposed to interpret this section of the docs for docker login:

If the secret being stored is an identity token, the Username should be set to <token>.

(...and if that's practically how cred helpers in the wild work today)

If that's the case then we'd only need to change NewKeychainFromHelper and not also have to change cosign and downstream callers to use the IdentityTokenHelper variant.

What do you think?

cc @jonjohnsonjr

[simongottschlag]

@imjasonh hi! 😊

No problem at all! Just fun 🎉 Beer sounds nice 🍻

I agree with you and will update the PR. 😊👍

[imjasonh]

Hey, so I think if we change this line to pass r.RegistryStr() instead of r.String() (which includes the repo component) then it should also work around chrismellard/docker-credential-acr-env#3

https://github.com/google/go-containerregistry/blob/main/pkg/authn/keychain.go#L157

I think all parts of this bug can be fixed inside our own code 🎉

[simongottschlag]

@imjasonh nice catch! Do you want it in a separate PR or is it OK in this one?

[imjasonh]

I'm fine to include it in this one.

[simongottschlag]

@imjasonh added! I've validated that it's working in my environment (only Azure though).

[simongottschlag]

We should have signed the corporate CLA from our side today, but I think it needs to be counter signed by Google and we're waiting for that right now. I can't see anything about it when logged in at least. I'll check back tomorrow and see if there's any updates.

[imjasonh]

We should have signed the corporate CLA from our side today, but I think it needs to be counter signed by Google and we're waiting for that right now. I can't see anything about it when logged in at least. I'll check back tomorrow and see if there's any updates.

If it's okay, I've opened #1265 which adds on this PR and includes unit tests. And since I'm already cleared for the CLA, we can merge it without waiting for Google. I'd like to get this included in cosign soon so they can cut a v1.5.1 with this fix for Azure users such as yourself.

(If you'd prefer to wait for the Google CLA, please let me know and I'll close mine, I don't mean to step on any toes)

Thanks again for your excellent work, and I look forward to working with you again more in the future! 👍

[simongottschlag]

Go for it! 🎉 Just close this one when you've merged yours 😊