-
Notifications
You must be signed in to change notification settings - Fork 545
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
Azure Container Registry authentication #1350
Comments
Whoa, thank you for the detailed debugging information! cc @jdolitsky and @jonjohnsonjr |
No problem at all! The issues and PRs are created and I'm going to try and get the Google CLA signed on Monday. |
An idea I've been exploring is to use the hamilton project instead of the current ACR Helper. If we're able to get the following two through (or perhaps using some additional modules outside of hamilton) we could remove a a dependency to autorest - which would be great. 👍
|
This skips the check for the code_challenge_methods_supported claim within the OIDC discovery document when configured to connect to an Azure-based OIDC provider. AFAIK, Azure OIDC does in fact support PKCE but does not advertise this in its discovery document so this should enable additional use cases with cosign and Azure. Partially-Fixes: sigstore/cosign#1350 Signed-off-by: Bob Callaway <[email protected]>
This is now released in v1.5.1 🎉 https://github.com/sigstore/cosign/releases/tag/v1.5.1 |
* Skip strict check on PKCE discovery claim on Azure This skips the check for the code_challenge_methods_supported claim within the OIDC discovery document when configured to connect to an Azure-based OIDC provider. AFAIK, Azure OIDC does in fact support PKCE but does not advertise this in its discovery document so this should enable additional use cases with cosign and Azure. Partially-Fixes: sigstore/cosign#1350 Signed-off-by: Bob Callaway <[email protected]> * set chosenMethod value for azure Signed-off-by: Bob Callaway <[email protected]>
Description
Hi!
As always, thank you for the great work you are doing with cosign and sigstore. 🥇
There seems to be a bug with how cosign authenticates with Azure Container Registry:
After spending some time troubleshooting this, it seems to be related to two bugs in upstream packages. These are packages used by
go-containerregistry
. I'll document it here but we'll most likely need to fix the bugs upstream to get it working correctly with cosign.Bug 1
The authentication helper for Azure Container Registry doesn't parse the URL correctly, meaning it's not stripping the path and query from the URL that it receives and then tries to issue a token using it which will fail.
Reference for the method: https://github.com/chrismellard/docker-credential-acr-env/blob/fe33c00cee21eacc9fc67b6492b2c0cbe08b8bbe/pkg/credhelper/helper.go#L67
If we look at the same Elastic Container Registry (AWS) helper, we can clearly see them extracting information before passing it to their other functions: https://github.com/awslabs/amazon-ecr-credential-helper/blob/69c85dc22db6511932bbf119e1a0cc5c90c69a7f/ecr-login/ecr.go#L46
Issue: chrismellard/docker-credential-acr-env#3
PR: chrismellard/docker-credential-acr-env#4
Bug 2
The helper wrapper being used for Azure Container Registry in
go-containerregistry
is returning a username and password, where the username is<token>
.If we read the comment in their code, it says that OAuth will be used if username is set to
<token>
- but this only applies if we have anIdentityToken
instead of aPassword
which is what the helper wrapper returns.We can clearly see when using ACR locally it's using IdentityToken: (~/.docker/config.json)
I'm not sure what the best solution here is, I've tested with using my own helper wrapper that returns IdentityToken instead of password and it works as expected.
Reference for the IdentityToken check: https://github.com/google/go-containerregistry/blob/86ab60664adc985dec1a8ab30af5cddf3c610db3/pkg/v1/remote/transport/bearer.go#L138-L142
Reference for the helper wrapper not setting IdentityToken: https://github.com/google/go-containerregistry/blob/86ab60664adc985dec1a8ab30af5cddf3c610db3/pkg/authn/keychain.go#L161
Issue: google/go-containerregistry#1255
PR: google/go-containerregistry#1256
Next steps
I'm mainly creating this issue here to make sure others testing cosign can see it and to gather my thoughts after digging deep into this rabbit hole. If anyone sees any clear signs of insanity from my side, please point it out and tell me where I'm screwing up. 💯
I'll be creating issues and PRs in the other repositories and linking them here.
The text was updated successfully, but these errors were encountered: