Skip to content
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.

Sign up for GitHub

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

Jump to bottom

fix Azure Container Registry anonymous repo image pull error #74715

Merged
merged 1 commit into from
Mar 1, 2019
Merged

fix Azure Container Registry anonymous repo image pull error #74715

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
7 changes: 7 additions & 0 deletions pkg/credentialprovider/azure/azure_credentials.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -206,6 +206,13 @@ func (a *acrProvider) Provide() credentialprovider.DockerConfig {
cfg[url] = *cred
}
}

// add ACR anonymous repo support: use empty username and password for anonymous access
cfg["*.azurecr.*"] = credentialprovider.DockerConfigEntry{
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What all does this match? does this match "foo.azurecr.bar.com" ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sivagms Yes, it match, but I think it does not matter since if user use private repo, suppose they should provide pull image secret by themselves.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That’s a bit scary. Some malicious user can just change the registry name in the chart to receive the AKS SP and password

Copy link
Member Author

@andyzhangx andyzhangx Mar 1, 2019 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

currently the matching patterns are like following:
( *. azurecr.io, SP) ( *. azurecr.cn, SP) ( * . azurecr . * , empty password)
Per offline discussion, it's safe
Thanks for the good point!

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the order of trying the matching? Will it always first try the empty user name one, or always the SP one?
Which is most likely usage here? I mean depending on the usage, you may want to optimize for the most common scenario.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@yuwaMSFT2 good question, now it's this order: ".azurecr.io", ".azurecr.*", which means use SP first, and then empty username/password, SP first should be the better choice in a long period.

Username: "",
Password: "",
Email: dummyRegistryEmail,
}
return cfg
}

Expand Down
8 changes: 4 additions & 4 deletions pkg/credentialprovider/azure/azure_credentials_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,14 +76,14 @@ func Test(t *testing.T) {

creds := provider.Provide()

if len(creds) != len(result) {
t.Errorf("Unexpected list: %v, expected length %d", creds, len(result))
if len(creds) != len(result)+1 {
t.Errorf("Unexpected list: %v, expected length %d", creds, len(result)+1)
}
for _, cred := range creds {
if cred.Username != "foo" {
if cred.Username != "" && cred.Username != "foo" {
t.Errorf("expected 'foo' for username, saw: %v", cred.Username)
}
if cred.Password != "bar" {
if cred.Password != "" && cred.Password != "bar" {
t.Errorf("expected 'bar' for password, saw: %v", cred.Username)
}
}
Expand Down