Merge pull request #884 from souleb/fix-874

[OCI] Static credentials should take precedence over the OIDC provider

Author: stefanprodan

controllers/helmchart_controller.go

@@ -516,10 +516,8 @@ func (r *HelmChartReconciler) buildFromHelmRepository(ctx context.Context, obj *
 		}
 
 		loginOpts = append([]helmreg.LoginOption{}, loginOpt)
-	}
-
-	if repo.Spec.Provider != sourcev1.GenericOCIProvider && repo.Spec.Type == sourcev1.HelmRepositoryTypeOCI {
-		auth, authErr := oidcAuth(ctxTimeout, repo)
+	} else if repo.Spec.Provider != sourcev1.GenericOCIProvider && repo.Spec.Type == sourcev1.HelmRepositoryTypeOCI {
+		auth, authErr := oidcAuthFromAdapter(ctxTimeout, repo.Spec.URL, repo.Spec.Provider)
 		if authErr != nil && !errors.Is(authErr, oci.ErrUnconfiguredProvider) {
 			e := &serror.Event{
 				Err:    fmt.Errorf("failed to get credential from %s: %w", repo.Spec.Provider, authErr),
@@ -991,10 +989,8 @@ func (r *HelmChartReconciler) namespacedChartRepositoryCallback(ctx context.Cont
 			}
 
 			loginOpts = append([]helmreg.LoginOption{}, loginOpt)
-		}
-
-		if repo.Spec.Provider != sourcev1.GenericOCIProvider && repo.Spec.Type == sourcev1.HelmRepositoryTypeOCI {
-			auth, authErr := oidcAuth(ctxTimeout, repo)
+		} else if repo.Spec.Provider != sourcev1.GenericOCIProvider && repo.Spec.Type == sourcev1.HelmRepositoryTypeOCI {
+			auth, authErr := oidcAuthFromAdapter(ctxTimeout, repo.Spec.URL, repo.Spec.Provider)
 			if authErr != nil && !errors.Is(authErr, oci.ErrUnconfiguredProvider) {
 				return nil, fmt.Errorf("failed to get credential from %s: %w", repo.Spec.Provider, authErr)
 			}

controllers/helmchart_controller_test.go

@@ -44,6 +44,7 @@ import (
 	kstatus "sigs.k8s.io/cli-utils/pkg/kstatus/status"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/client/fake"
+	fakeclient "sigs.k8s.io/controller-runtime/pkg/client/fake"
 	"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
 
 	"github.com/fluxcd/pkg/apis/meta"
@@ -893,21 +894,11 @@ func TestHelmChartReconciler_buildFromOCIHelmRepository(t *testing.T) {
 		chartPath = "testdata/charts/helmchart-0.1.0.tgz"
 	)
 
-	// Login to the registry
-	err := testRegistryServer.registryClient.Login(testRegistryServer.registryHost,
-		helmreg.LoginOptBasicAuth(testRegistryUsername, testRegistryPassword),
-		helmreg.LoginOptInsecure(true))
-	g.Expect(err).NotTo(HaveOccurred())
-
 	// Load a test chart
 	chartData, err := ioutil.ReadFile(chartPath)
-	g.Expect(err).NotTo(HaveOccurred())
-	metadata, err := extractChartMeta(chartData)
-	g.Expect(err).NotTo(HaveOccurred())
 
 	// Upload the test chart
-	ref := fmt.Sprintf("%s/testrepo/%s:%s", testRegistryServer.registryHost, metadata.Name, metadata.Version)
-	_, err = testRegistryServer.registryClient.Push(chartData, ref)
+	metadata, err := loadTestChartToOCI(chartData, chartPath, testRegistryServer)
 	g.Expect(err).NotTo(HaveOccurred())
 
 	storage, err := NewStorage(tmpDir, "example.com", retentionTTL, retentionRecords)
@@ -2038,6 +2029,194 @@ func TestHelmChartReconciler_notify(t *testing.T) {
 	}
 }
 
+func TestHelmChartReconciler_reconcileSourceFromOCI_authStrategy(t *testing.T) {
+	const (
+		chartPath = "testdata/charts/helmchart-0.1.0.tgz"
+	)
+
+	type secretOptions struct {
+		username string
+		password string
+	}
+
+	tests := []struct {
+		name             string
+		url              string
+		registryOpts     registryOptions
+		secretOpts       secretOptions
+		provider         string
+		providerImg      string
+		want             sreconcile.Result
+		wantErr          bool
+		assertConditions []metav1.Condition
+	}{
+		{
+			name: "HTTP without basic auth",
+			want: sreconcile.ResultSuccess,
+			assertConditions: []metav1.Condition{
+				*conditions.TrueCondition(sourcev1.ArtifactOutdatedCondition, "NewChart", "pulled '<helmchart>' chart with version '<version>'"),
+			},
+		},
+		{
+			name: "HTTP with basic auth secret",
+			want: sreconcile.ResultSuccess,
+			registryOpts: registryOptions{
+				withBasicAuth: true,
+			},
+			secretOpts: secretOptions{
+				username: testRegistryUsername,
+				password: testRegistryPassword,
+			},
+			assertConditions: []metav1.Condition{
+				*conditions.TrueCondition(sourcev1.ArtifactOutdatedCondition, "NewChart", "pulled '<helmchart>' chart with version '<version>'"),
+			},
+		},
+		{
+			name:    "HTTP registry - basic auth with invalid secret",
+			want:    sreconcile.ResultEmpty,
+			wantErr: true,
+			registryOpts: registryOptions{
+				withBasicAuth: true,
+			},
+			secretOpts: secretOptions{
+				username: "wrong-pass",
+				password: "wrong-pass",
+			},
+			assertConditions: []metav1.Condition{
+				*conditions.TrueCondition(sourcev1.FetchFailedCondition, "Unknown", "unknown build error: failed to login to OCI registry"),
+			},
+		},
+		{
+			name:        "with contextual login provider",
+			wantErr:     true,
+			provider:    "aws",
+			providerImg: "oci://123456789000.dkr.ecr.us-east-2.amazonaws.com/test",
+			assertConditions: []metav1.Condition{
+				*conditions.TrueCondition(sourcev1.FetchFailedCondition, "Unknown", "unknown build error: failed to get credential from"),
+			},
+		},
+		{
+			name: "with contextual login provider and secretRef",
+			want: sreconcile.ResultSuccess,
+			registryOpts: registryOptions{
+				withBasicAuth: true,
+			},
+			secretOpts: secretOptions{
+				username: testRegistryUsername,
+				password: testRegistryPassword,
+			},
+			provider: "azure",
+			assertConditions: []metav1.Condition{
+				*conditions.TrueCondition(sourcev1.ArtifactOutdatedCondition, "NewChart", "pulled '<helmchart>' chart with version '<version>'"),
+			},
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			g := NewWithT(t)
+
+			builder := fakeclient.NewClientBuilder().WithScheme(testEnv.GetScheme())
+			workspaceDir := t.TempDir()
+			server, err := setupRegistryServer(ctx, workspaceDir, tt.registryOpts)
+
+			g.Expect(err).NotTo(HaveOccurred())
+
+			// Load a test chart
+			chartData, err := ioutil.ReadFile(chartPath)
+
+			// Upload the test chart
+			metadata, err := loadTestChartToOCI(chartData, chartPath, server)
+			g.Expect(err).NotTo(HaveOccurred())
+			g.Expect(err).ToNot(HaveOccurred())
+
+			repo := &sourcev1.HelmRepository{
+				ObjectMeta: metav1.ObjectMeta{
+					GenerateName: "auth-strategy-",
+				},
+				Spec: sourcev1.HelmRepositorySpec{
+					Interval: metav1.Duration{Duration: interval},
+					Timeout:  &metav1.Duration{Duration: timeout},
+					Type:     sourcev1.HelmRepositoryTypeOCI,
+					Provider: sourcev1.GenericOCIProvider,
+					URL:      fmt.Sprintf("oci://%s/testrepo", server.registryHost),
+				},
+			}
+
+			if tt.provider != "" {
+				repo.Spec.Provider = tt.provider
+			}
+			// If a provider specific image is provided, overwrite existing URL
+			// set earlier. It'll fail but it's necessary to set them because
+			// the login check expects the URLs to be of certain pattern.
+			if tt.providerImg != "" {
+				repo.Spec.URL = tt.providerImg
+			}
+
+			if tt.secretOpts.username != "" && tt.secretOpts.password != "" {
+				secret := &corev1.Secret{
+					ObjectMeta: metav1.ObjectMeta{
+						Name: "auth-secretref",
+					},
+					Type: corev1.SecretTypeDockerConfigJson,
+					Data: map[string][]byte{
+						".dockerconfigjson": []byte(fmt.Sprintf(`{"auths": {%q: {"username": %q, "password": %q}}}`,
+							server.registryHost, tt.secretOpts.username, tt.secretOpts.password)),
+					},
+				}
+
+				repo.Spec.SecretRef = &meta.LocalObjectReference{
+					Name: secret.Name,
+				}
+				builder.WithObjects(secret, repo)
+			} else {
+				builder.WithObjects(repo)
+			}
+
+			obj := &sourcev1.HelmChart{
+				ObjectMeta: metav1.ObjectMeta{
+					GenerateName: "auth-strategy-",
+				},
+				Spec: sourcev1.HelmChartSpec{
+					Chart:   metadata.Name,
+					Version: metadata.Version,
+					SourceRef: sourcev1.LocalHelmChartSourceReference{
+						Kind: sourcev1.HelmRepositoryKind,
+						Name: repo.Name,
+					},
+					Interval: metav1.Duration{Duration: interval},
+				},
+			}
+
+			r := &HelmChartReconciler{
+				Client:                  builder.Build(),
+				EventRecorder:           record.NewFakeRecorder(32),
+				Getters:                 testGetters,
+				RegistryClientGenerator: registry.ClientGenerator,
+			}
+
+			var b chart.Build
+			defer func() {
+				if _, err := os.Stat(b.Path); !os.IsNotExist(err) {
+					err := os.Remove(b.Path)
+					g.Expect(err).NotTo(HaveOccurred())
+				}
+			}()
+
+			assertConditions := tt.assertConditions
+			for k := range assertConditions {
+				assertConditions[k].Message = strings.ReplaceAll(assertConditions[k].Message, "<helmchart>", metadata.Name)
+				assertConditions[k].Message = strings.ReplaceAll(assertConditions[k].Message, "<version>", metadata.Version)
+			}
+
+			got, err := r.reconcileSource(ctx, obj, &b)
+			g.Expect(err != nil).To(Equal(tt.wantErr))
+			g.Expect(got).To(Equal(tt.want))
+			g.Expect(obj.Status.Conditions).To(conditions.MatchConditions(tt.assertConditions))
+		})
+	}
+}
+
 // extractChartMeta is used to extract a chart metadata from a byte array
 func extractChartMeta(chartData []byte) (*hchart.Metadata, error) {
 	ch, err := loader.LoadArchive(bytes.NewReader(chartData))
@@ -2046,3 +2225,32 @@ func extractChartMeta(chartData []byte) (*hchart.Metadata, error) {
 	}
 	return ch.Metadata, nil
 }
+
+func loadTestChartToOCI(chartData []byte, chartPath string, server *registryClientTestServer) (*hchart.Metadata, error) {
+	// Login to the registry
+	err := server.registryClient.Login(server.registryHost,
+		helmreg.LoginOptBasicAuth(testRegistryUsername, testRegistryPassword),
+		helmreg.LoginOptInsecure(true))
+	if err != nil {
+		return nil, err
+	}
+
+	// Load a test chart
+	chartData, err = ioutil.ReadFile(chartPath)
+	if err != nil {
+		return nil, err
+	}
+	metadata, err := extractChartMeta(chartData)
+	if err != nil {
+		return nil, err
+	}
+
+	// Upload the test chart
+	ref := fmt.Sprintf("%s/testrepo/%s:%s", server.registryHost, metadata.Name, metadata.Version)
+	_, err = server.registryClient.Push(chartData, ref)
+	if err != nil {
+		return nil, err
+	}
+
+	return metadata, nil
+}

controllers/helmrepository_controller_oci.go

@@ -22,7 +22,6 @@ import (
 	"fmt"
 	"net/url"
 	"os"
-	"strings"
 	"time"
 
 	helmgetter "helm.sh/helm/v3/pkg/getter"
@@ -42,12 +41,10 @@ import (
 
 	"github.com/fluxcd/pkg/apis/meta"
 	"github.com/fluxcd/pkg/oci"
-	"github.com/fluxcd/pkg/oci/auth/login"
 	"github.com/fluxcd/pkg/runtime/conditions"
 	helper "github.com/fluxcd/pkg/runtime/controller"
 	"github.com/fluxcd/pkg/runtime/patch"
 	"github.com/fluxcd/pkg/runtime/predicates"
-	"github.com/google/go-containerregistry/pkg/name"
 
 	"github.com/fluxcd/source-controller/api/v1beta2"
 	sourcev1 "github.com/fluxcd/source-controller/api/v1beta2"
@@ -294,10 +291,8 @@ func (r *HelmRepositoryOCIReconciler) reconcile(ctx context.Context, obj *v1beta
 		if loginOpt != nil {
 			loginOpts = append(loginOpts, loginOpt)
 		}
-	}
-
-	if obj.Spec.Provider != sourcev1.GenericOCIProvider && obj.Spec.Type == sourcev1.HelmRepositoryTypeOCI {
-		auth, authErr := oidcAuth(ctxTimeout, obj)
+	} else if obj.Spec.Provider != sourcev1.GenericOCIProvider && obj.Spec.Type == sourcev1.HelmRepositoryTypeOCI {
+		auth, authErr := oidcAuthFromAdapter(ctxTimeout, obj.Spec.URL, obj.Spec.Provider)
 		if authErr != nil && !errors.Is(authErr, oci.ErrUnconfiguredProvider) {
 			e := fmt.Errorf("failed to get credential from %s: %w", obj.Spec.Provider, authErr)
 			conditions.MarkFalse(obj, meta.ReadyCondition, sourcev1.AuthenticationFailedReason, e.Error())
@@ -380,41 +375,12 @@ func (r *HelmRepositoryOCIReconciler) eventLogf(ctx context.Context, obj runtime
 	r.Eventf(obj, eventType, reason, msg)
 }
 
-// oidcAuth generates the OIDC credential authenticator based on the specified cloud provider.
-func oidcAuth(ctx context.Context, obj *sourcev1.HelmRepository) (helmreg.LoginOption, error) {
-	url := strings.TrimPrefix(obj.Spec.URL, sourcev1.OCIRepositoryPrefix)
-	ref, err := name.ParseReference(url)
-	if err != nil {
-		return nil, fmt.Errorf("failed to parse URL '%s': %w", obj.Spec.URL, err)
-	}
-
-	loginOpt, err := loginWithManager(ctx, obj.Spec.Provider, url, ref)
-	if err != nil {
-		return nil, fmt.Errorf("failed to login to registry '%s': %w", obj.Spec.URL, err)
-	}
-
-	return loginOpt, nil
-}
-
-func loginWithManager(ctx context.Context, provider, url string, ref name.Reference) (helmreg.LoginOption, error) {
-	opts := login.ProviderOptions{}
-	switch provider {
-	case sourcev1.AmazonOCIProvider:
-		opts.AwsAutoLogin = true
-	case sourcev1.AzureOCIProvider:
-		opts.AzureAutoLogin = true
-	case sourcev1.GoogleOCIProvider:
-		opts.GcpAutoLogin = true
-	}
-
-	auth, err := login.NewManager().Login(ctx, url, ref, opts)
+// oidcAuthFromAdapter generates the OIDC credential authenticator based on the specified cloud provider.
+func oidcAuthFromAdapter(ctx context.Context, url, provider string) (helmreg.LoginOption, error) {
+	auth, err := oidcAuth(ctx, url, provider)
 	if err != nil {
 		return nil, err
 	}
 
-	if auth == nil {
-		return nil, nil
-	}
-
 	return registry.OIDCAdaptHelper(auth)
 }