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.

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

Ignore malformed secrets when creating keychain #1834

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 15 additions & 5 deletions pkg/authn/kubernetes/keychain.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ package kubernetes
import (
"context"
"encoding/json"
"fmt"
"errors"
"net"
"net/url"
"path/filepath"
Expand Down Expand Up @@ -132,7 +132,6 @@ func New(ctx context.Context, client kubernetes.Interface, opt Options) (authn.K
}
}
}

return NewFromPullSecrets(ctx, pullSecrets)
}

Expand Down Expand Up @@ -165,17 +164,22 @@ func NewFromPullSecrets(ctx context.Context, secrets []corev1.Secret) (authn.Key
}

var cfg dockerConfigJSON
// hasValid tracks if at least one of the given secrets provided is valid and contains at least
// one valid auth.
var hasValid bool

// From: https://github.com/kubernetes/kubernetes/blob/0dcafb1f37ee522be3c045753623138e5b907001/pkg/credentialprovider/keyring.go
for _, secret := range secrets {
if b, exists := secret.Data[corev1.DockerConfigJsonKey]; secret.Type == corev1.SecretTypeDockerConfigJson && exists && len(b) > 0 {
if err := json.Unmarshal(b, &cfg); err != nil {
return nil, err
logs.Warn.Printf("cannot decode secret %s/%s; ignoring", secret.Namespace, secret.Name)
continue
}
}
if b, exists := secret.Data[corev1.DockerConfigKey]; secret.Type == corev1.SecretTypeDockercfg && exists && len(b) > 0 {
if err := json.Unmarshal(b, &cfg.Auths); err != nil {
return nil, err
logs.Warn.Printf("cannot decode secret %s/%s; ignoring", secret.Namespace, secret.Name)
continue
}
}

Expand All @@ -186,9 +190,11 @@ func NewFromPullSecrets(ctx context.Context, secrets []corev1.Secret) (authn.Key
}
parsed, err := url.Parse(value)
if err != nil {
return nil, fmt.Errorf("Entry %q in dockercfg invalid (%w)", value, err)
logs.Warn.Printf("entry %q in dockercfg secret %s/%s invalid (%s); ignoring", secret.Namespace, secret.Name, value, err)
continue

Choose a reason for hiding this comment

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

Instead of continuing.
Can we throw an error if none of the secrets work? May be thats a failure mode?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!

}

hasValid = true
// The docker client allows exact matches:
// foo.bar.com/namespace
// Or hostname matches:
Expand Down Expand Up @@ -218,6 +224,10 @@ func NewFromPullSecrets(ctx context.Context, secrets []corev1.Secret) (authn.Key
// when matching for creds
sort.Sort(sort.Reverse(sort.StringSlice(keyring.index)))
}
if !hasValid && len(secrets) > 0 {
return nil, errors.New("could not find a valid secret")
}

return keyring, nil
}

Expand Down
68 changes: 68 additions & 0 deletions pkg/authn/kubernetes/keychain_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -132,6 +132,49 @@ func TestServiceAccountNotFound(t *testing.T) {
testResolve(t, kc, registry(t, "fake.registry.io"), authn.Anonymous)
}

func TestMalformedSecretsIgnored(t *testing.T) {
secrets := []corev1.Secret{
// Malformed .dockerconfigjson is ignored
{
ObjectMeta: metav1.ObjectMeta{
Name: "bad-dockerconfigjson",
Namespace: "ns",
},
Type: corev1.SecretTypeDockerConfigJson,
Data: map[string][]byte{
corev1.DockerConfigJsonKey: []byte("bad-base64"),
},
},
// Malformed .dockercfg is ignored
{
ObjectMeta: metav1.ObjectMeta{
Name: "bad-dockercfg",
Namespace: "ns",
},
Type: corev1.SecretTypeDockercfg,
Data: map[string][]byte{
corev1.DockerConfigKey: []byte("bad-base64"),
},
},
// Malformed registry URL is ignored
*dockerCfgSecretType.Create(t, "ns", "secret", `\tfake.registry.io`, authn.AuthConfig{
Username: "ignored", Password: "ignored",
}),
// Correct secret is used
*dockerCfgSecretType.Create(t, "ns", "secret", "fake.registry.io", authn.AuthConfig{
Username: "user", Password: "pass",
}),
}

kc, err := NewFromPullSecrets(context.Background(), secrets)
if err != nil {
t.Fatalf("NewFromPullSecrets() = %v", err)
}

expectedAuth := &authn.Basic{Username: "user", Password: "pass"}
testResolve(t, kc, registry(t, "fake.registry.io"), expectedAuth)
}

func TestImagePullSecretAttachedServiceAccount(t *testing.T) {
username, password := "foo", "bar"
client := fakeclient.NewSimpleClientset(&corev1.ServiceAccount{
Expand Down Expand Up @@ -413,6 +456,31 @@ func TestAuthWithGlobs(t *testing.T) {
testResolve(t, kc, repo(t, "blah.registry.io/repo"), authn.FromConfig(auth))
}

func TestNoValidAuthsFound(t *testing.T) {
secrets := []corev1.Secret{
// Malformed .dockerconfigjson is ignored
{
ObjectMeta: metav1.ObjectMeta{
Name: "bad-dockerconfigjson",
Namespace: "ns",
},
Type: corev1.SecretTypeDockerConfigJson,
Data: map[string][]byte{
corev1.DockerConfigJsonKey: []byte("bad-base64"),
},
},
}

kc, err := NewFromPullSecrets(context.Background(), secrets)
if kc != nil {
t.Fatal("NewFromPullSecrets() = expected nil keychain")
}
expectedErr := "could not find a valid secret"
if err == nil || fmt.Sprintf("%s", err) != expectedErr {
t.Fatalf("NewFromPullSecrets() = want error %q, got %q", expectedErr, err)
}
}

func testResolve(t *testing.T, kc authn.Keychain, target authn.Resource, expectedAuth authn.Authenticator) {
t.Helper()

Expand Down
Loading