From 2e8db08345c57312165833e3517cc3981f36f11f Mon Sep 17 00:00:00 2001 From: Soule BA Date: Tue, 30 May 2023 11:17:23 +0200 Subject: [PATCH] refactor TLSconfig funcs Signed-off-by: Soule BA --- api/v1beta2/zz_generated.deepcopy.go | 3 - docs/spec/v1beta2/helmrepositories.md | 30 ---- go.mod | 4 - internal/controller/helmchart_controller.go | 25 ++- .../controller/helmchart_controller_test.go | 12 +- .../controller/helmrepository_controller.go | 2 +- .../helmrepository_controller_oci.go | 152 ++++++------------ .../helmrepository_controller_oci_test.go | 39 ++++- .../helmrepository_controller_test.go | 2 +- .../ocirepository_controller_test.go | 14 +- internal/controller/suite_test.go | 145 +++++++++-------- .../controller/testdata/certs/ca-csr.json | 3 +- internal/controller/testdata/certs/ca-key.pem | 6 +- internal/controller/testdata/certs/ca.csr | 14 +- internal/controller/testdata/certs/ca.pem | 18 +-- .../controller/testdata/certs/server-csr.json | 3 +- .../controller/testdata/certs/server-key.pem | 6 +- internal/controller/testdata/certs/server.csr | 13 +- internal/controller/testdata/certs/server.pem | 20 +-- internal/helm/getter/client_opts.go | 137 +++++++++++++--- internal/helm/getter/client_opts_test.go | 116 ++++++++++++- internal/helm/registry/auth.go | 9 ++ internal/helm/registry/client.go | 35 ++-- 23 files changed, 476 insertions(+), 332 deletions(-) diff --git a/api/v1beta2/zz_generated.deepcopy.go b/api/v1beta2/zz_generated.deepcopy.go index d96a83282..5c2169a33 100644 --- a/api/v1beta2/zz_generated.deepcopy.go +++ b/api/v1beta2/zz_generated.deepcopy.go @@ -577,14 +577,11 @@ func (in *HelmRepositorySpec) DeepCopyInto(out *HelmRepositorySpec) { *out = new(meta.LocalObjectReference) **out = **in } -<<<<<<< HEAD if in.CertSecretRef != nil { in, out := &in.CertSecretRef, &out.CertSecretRef *out = new(meta.LocalObjectReference) **out = **in } -======= ->>>>>>> 3df4c49 (refactoring and fix tests) out.Interval = in.Interval if in.Timeout != nil { in, out := &in.Timeout, &out.Timeout diff --git a/docs/spec/v1beta2/helmrepositories.md b/docs/spec/v1beta2/helmrepositories.md index ec216c0a1..e121b01e3 100644 --- a/docs/spec/v1beta2/helmrepositories.md +++ b/docs/spec/v1beta2/helmrepositories.md @@ -459,16 +459,8 @@ a deprecation warning will be logged. ### Cert secret reference -<<<<<<< HEAD -**Note:** TLS authentication is not yet supported by OCI Helm repositories. - `.spec.certSecretRef.name` is an optional field to specify a secret containing TLS certificate data. The secret can contain the following keys: -======= -To provide TLS credentials to use while connecting with the Helm repository, -the referenced Secret is expected to contain `.data.certFile` and -`.data.keyFile`, and/or `.data.caFile` values. ->>>>>>> 3df4c49 (refactoring and fix tests) * `certFile` and `keyFile`, to specify the client certificate and private key used for TLS client authentication. These must be used in conjunction, i.e. specifying one without @@ -515,28 +507,6 @@ data: caFile: ``` -#### Provide TLS credentials in a secret of type kubernetes.io/dockerconfigjson - -For OCI Helm repositories, Kubernetes secrets of type [kubernetes.io/dockerconfigjson](https://kubernetes.io/docs/concepts/configuration/secret/#secret-types) -are also supported. It is possible to append TLS credentials to the secret data. - -For example: - -```yaml -apiVersion: v1 -kind: Secret -metadata: - name: example-tls - namespace: default -type: kubernetes.io/dockerconfigjson -data: - .dockerconfigjson: - certFile: - keyFile: - # NOTE: Can be supplied without the above values - caFile: -``` - ### Pass credentials `.spec.passCredentials` is an optional field to allow the credentials from the diff --git a/go.mod b/go.mod index 5773ddbea..dd879376d 100644 --- a/go.mod +++ b/go.mod @@ -40,12 +40,8 @@ require ( github.com/fluxcd/pkg/tar v0.2.0 github.com/fluxcd/pkg/testserver v0.4.0 github.com/fluxcd/pkg/version v0.2.2 -<<<<<<< HEAD github.com/fluxcd/source-controller/api v1.0.0 -======= - github.com/fluxcd/source-controller/api v1.0.0-rc.5 github.com/foxcpp/go-mockdns v1.0.0 ->>>>>>> 4e0d792 (Adapting setupRegistryServer to be able to use https with the docker) github.com/go-git/go-billy/v5 v5.4.1 github.com/go-git/go-git/v5 v5.8.1 github.com/go-logr/logr v1.2.4 diff --git a/internal/controller/helmchart_controller.go b/internal/controller/helmchart_controller.go index 548b4bc53..3bc1cebf8 100644 --- a/internal/controller/helmchart_controller.go +++ b/internal/controller/helmchart_controller.go @@ -510,7 +510,8 @@ func (r *HelmChartReconciler) buildFromHelmRepository(ctx context.Context, obj * if err != nil { return chartRepoConfigErrorReturn(err, obj) } - clientOpts, err := getter.GetClientOpts(ctxTimeout, r.Client, repo, normalizedURL) + + clientOpts, certsTmpDir, err := getter.GetClientOpts(ctxTimeout, r.Client, repo, normalizedURL) if err != nil && !errors.Is(err, getter.ErrDeprecatedTLSConfig) { e := &serror.Event{ Err: err, @@ -519,6 +520,10 @@ func (r *HelmChartReconciler) buildFromHelmRepository(ctx context.Context, obj * conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, e.Reason, e.Err.Error()) return sreconcile.ResultEmpty, e } + if certsTmpDir != "" { + // this happens when the tls certs are stored in a temporary directory + defer os.RemoveAll(certsTmpDir) + } getterOpts := clientOpts.GetterOpts // Initialize the chart repository @@ -534,7 +539,7 @@ func (r *HelmChartReconciler) buildFromHelmRepository(ctx context.Context, obj * // this is needed because otherwise the credentials are stored in ~/.docker/config.json. // TODO@souleb: remove this once the registry move to Oras v2 // or rework to enable reusing credentials to avoid the unneccessary handshake operations - registryClient, credentialsFile, err := r.RegistryClientGenerator(clientOpts.RegLoginOpt != nil) + registryClient, credentialsFile, err := r.RegistryClientGenerator(clientOpts.TlsConfig, clientOpts.RegLoginOpts[0] != nil) if err != nil { e := &serror.Event{ Err: fmt.Errorf("failed to construct Helm client: %w", err), @@ -583,8 +588,8 @@ func (r *HelmChartReconciler) buildFromHelmRepository(ctx context.Context, obj * // If login options are configured, use them to login to the registry // The OCIGetter will later retrieve the stored credentials to pull the chart - if clientOpts.RegLoginOpt != nil { - err = ociChartRepo.Login(clientOpts.RegLoginOpt) + if clientOpts.RegLoginOpts != nil { + err = ociChartRepo.Login(clientOpts.RegLoginOpts...) if err != nil { e := &serror.Event{ Err: fmt.Errorf("failed to login to OCI registry: %w", err), @@ -981,15 +986,19 @@ func (r *HelmChartReconciler) namespacedChartRepositoryCallback(ctx context.Cont ctxTimeout, cancel := context.WithTimeout(ctx, obj.Spec.Timeout.Duration) defer cancel() - clientOpts, err := getter.GetClientOpts(ctxTimeout, r.Client, obj, normalizedURL) + clientOpts, certsTmpDir, err := getter.GetClientOpts(ctxTimeout, r.Client, obj, normalizedURL) if err != nil && !errors.Is(err, getter.ErrDeprecatedTLSConfig) { return nil, err } + if certsTmpDir != "" { + // this happens when the tls certs are stored in a temporary directory + defer os.RemoveAll(certsTmpDir) + } getterOpts := clientOpts.GetterOpts var chartRepo repository.Downloader if helmreg.IsOCI(normalizedURL) { - registryClient, credentialsFile, err := r.RegistryClientGenerator(clientOpts.RegLoginOpt != nil) + registryClient, credentialsFile, err := r.RegistryClientGenerator(clientOpts.TlsConfig, clientOpts.RegLoginOpts[0] != nil) if err != nil { return nil, fmt.Errorf("failed to create registry client: %w", err) } @@ -1014,8 +1023,8 @@ func (r *HelmChartReconciler) namespacedChartRepositoryCallback(ctx context.Cont // If login options are configured, use them to login to the registry // The OCIGetter will later retrieve the stored credentials to pull the chart - if clientOpts.RegLoginOpt != nil { - err = ociChartRepo.Login(clientOpts.RegLoginOpt) + if clientOpts.RegLoginOpts != nil { + err = ociChartRepo.Login(clientOpts.RegLoginOpts...) if err != nil { errs = append(errs, fmt.Errorf("failed to login to OCI chart repository: %w", err)) // clean up the credentialsFile diff --git a/internal/controller/helmchart_controller_test.go b/internal/controller/helmchart_controller_test.go index dc8466744..4560ebc87 100644 --- a/internal/controller/helmchart_controller_test.go +++ b/internal/controller/helmchart_controller_test.go @@ -2205,7 +2205,7 @@ func TestHelmChartReconciler_reconcileSourceFromOCI_authStrategy(t *testing.T) { registryOpts registryOptions secretOpts secretOptions secret *corev1.Secret - withTLS bool + insecure bool provider string providerImg string want sreconcile.Result @@ -2302,7 +2302,7 @@ func TestHelmChartReconciler_reconcileSourceFromOCI_authStrategy(t *testing.T) { registryOpts: registryOptions{ withTLS: true, }, - withTLS: true, + insecure: true, secretOpts: secretOptions{ username: testRegistryUsername, password: testRegistryPassword, @@ -2326,7 +2326,7 @@ func TestHelmChartReconciler_reconcileSourceFromOCI_authStrategy(t *testing.T) { registryOpts: registryOptions{ withTLS: true, }, - withTLS: true, + insecure: true, secretOpts: secretOptions{ username: testRegistryUsername, password: testRegistryPassword, @@ -2361,9 +2361,9 @@ func TestHelmChartReconciler_reconcileSourceFromOCI_authStrategy(t *testing.T) { server, err := setupRegistryServer(ctx, workspaceDir, tt.registryOpts) g.Expect(err).NotTo(HaveOccurred()) - if tt.withTLS { - defer server.stopSrv() - } + t.Cleanup(func() { + server.Close() + }) // Load a test chart chartData, err := os.ReadFile(chartPath) diff --git a/internal/controller/helmrepository_controller.go b/internal/controller/helmrepository_controller.go index 1b6161ee0..d34792e37 100644 --- a/internal/controller/helmrepository_controller.go +++ b/internal/controller/helmrepository_controller.go @@ -398,7 +398,7 @@ func (r *HelmRepositoryReconciler) reconcileSource(ctx context.Context, sp *patc return sreconcile.ResultEmpty, e } - clientOpts, err := getter.GetClientOpts(ctx, r.Client, obj, normalizedURL) + clientOpts, _, err := getter.GetClientOpts(ctx, r.Client, obj, normalizedURL) if err != nil { if errors.Is(err, getter.ErrDeprecatedTLSConfig) { ctrl.LoggerFrom(ctx). diff --git a/internal/controller/helmrepository_controller_oci.go b/internal/controller/helmrepository_controller_oci.go index e78086a12..d6b5935ee 100644 --- a/internal/controller/helmrepository_controller_oci.go +++ b/internal/controller/helmrepository_controller_oci.go @@ -307,50 +307,55 @@ func (r *HelmRepositoryOCIReconciler) reconcile(ctx context.Context, sp *patch.S conditions.Delete(obj, meta.StalledCondition) var ( - authenticator authn.Authenticator - keychain authn.Keychain - tlsConfig *tls.Config - tmpCertsDir string - tlsLoginOpt helmreg.LoginOption - err error + authenticator authn.Authenticator + keychain authn.Keychain + tlsConfig *tls.Config + tlsBytes *getter.TLSBytes + certFile, keyFile, caFile string + err error ) // Configure any authentication related options. if obj.Spec.SecretRef != nil { - // Attempt to retrieve secret. - name := types.NamespacedName{ - Namespace: obj.GetNamespace(), - Name: obj.Spec.SecretRef.Name, - } - var secret corev1.Secret - if err := r.Client.Get(ctx, name, &secret); err != nil { - conditions.MarkFalse(obj, meta.ReadyCondition, sourcev1.AuthenticationFailedReason, err.Error()) - result, retErr = ctrl.Result{}, err - return - } - keychain, err = authFromSecret(ctx, r.Client, obj.Spec.URL, secret) + secret, err := r.getSecret(ctx, obj) if err != nil { conditions.MarkFalse(obj, meta.ReadyCondition, sourcev1.AuthenticationFailedReason, err.Error()) result, retErr = ctrl.Result{}, err return } - tlsConfig, err = getter.TLSClientConfigFromSecret(secret, obj.Spec.URL) + keychain, err = authFromSecret(ctx, r.Client, obj.Spec.URL, *secret) if err != nil { conditions.MarkFalse(obj, meta.ReadyCondition, sourcev1.AuthenticationFailedReason, err.Error()) result, retErr = ctrl.Result{}, err return } - tlsLoginOpt, tmpCertsDir, err = makeTLSLoginOption(&secret) + tlsConfig, tlsBytes, err = getter.TLSClientConfigFromSecret(*secret, obj.Spec.URL) if err != nil { conditions.MarkFalse(obj, meta.ReadyCondition, sourcev1.AuthenticationFailedReason, err.Error()) result, retErr = ctrl.Result{}, err return } - defer func() { - if err := os.RemoveAll(tmpCertsDir); err != nil { - r.eventLogf(ctx, obj, corev1.EventTypeWarning, meta.FailedReason, - "failed to delete temporary certificates directory: %s", err) + // Now that we have checked for TLS config, persist the certs files to the path if needed. + if tlsBytes != nil { + certsTmpDir, err := os.MkdirTemp("", "helm-repo-oci-certs") + if err != nil { + conditions.MarkFalse(obj, meta.ReadyCondition, sourcev1.AuthenticationFailedReason, err.Error()) + result, retErr = ctrl.Result{}, err + return } - }() + defer func() { + if err := os.RemoveAll(certsTmpDir); err != nil { + r.eventLogf(ctx, obj, corev1.EventTypeWarning, meta.FailedReason, + "failed to delete temporary certificates directory: %s", err) + } + }() + + certFile, keyFile, caFile, err = getter.StoreCertsFiles(tlsBytes, certsTmpDir) + if err != nil { + conditions.MarkFalse(obj, meta.ReadyCondition, sourcev1.AuthenticationFailedReason, err.Error()) + result, retErr = ctrl.Result{}, err + return + } + } } else if obj.Spec.Provider != helmv1.GenericOCIProvider && obj.Spec.Type == helmv1.HelmRepositoryTypeOCI { auth, authErr := soci.OIDCAuth(ctxTimeout, obj.Spec.URL, obj.Spec.Provider) if authErr != nil && !errors.Is(authErr, oci.ErrUnconfiguredProvider) { @@ -401,8 +406,11 @@ func (r *HelmRepositoryOCIReconciler) reconcile(ctx context.Context, sp *patch.S // Attempt to login to the registry if credentials are provided. if loginOpt != nil { opts := []helmreg.LoginOption{loginOpt} - if tlsLoginOpt != nil { - opts = append(opts, tlsLoginOpt) + if tlsBytes != nil { + tlsLoginOpt := registry.TLSLoginOption(certFile, keyFile, caFile) + if tlsLoginOpt != nil { + opts = append(opts, tlsLoginOpt) + } } err = chartRepo.Login(opts...) if err != nil { @@ -430,6 +438,22 @@ func (r *HelmRepositoryOCIReconciler) reconcileDelete(ctx context.Context, obj * return ctrl.Result{}, nil } +func (r *HelmRepositoryOCIReconciler) getSecret(ctx context.Context, obj *helmv1.HelmRepository) (*corev1.Secret, error) { + if obj.Spec.SecretRef == nil { + return nil, nil + } + name := types.NamespacedName{ + Namespace: obj.GetNamespace(), + Name: obj.Spec.SecretRef.Name, + } + var secret corev1.Secret + err := r.Client.Get(ctx, name, &secret) + if err != nil { + return nil, err + } + return &secret, nil +} + // eventLogf records events, and logs at the same time. // // This log is different from the debug log in the EventRecorder, in the sense @@ -471,80 +495,6 @@ func makeLoginOption(auth authn.Authenticator, keychain authn.Keychain, registry return nil, nil } -func makeTLSLoginOption(secret *corev1.Secret) (helmreg.LoginOption, string, error) { - var errs []error - certFile, keyFile, caFile, tmpDir, err := certsFilesFromSecret(secret) - if err != nil { - errs = append(errs, err) - if tmpDir != "" { - if err := os.RemoveAll(tmpDir); err != nil { - errs = append(errs, err) - } - } - return nil, "", kerrors.NewAggregate(errs) - } - - if (certFile != "" && keyFile != "") || caFile != "" { - return helmreg.LoginOptTLSClientConfig(certFile, keyFile, caFile), tmpDir, nil - } - - return nil, "", nil -} - -func certsFilesFromSecret(secret *corev1.Secret) (string, string, string, string, error) { - certBytes, keyBytes, caBytes := secret.Data["certFile"], secret.Data["keyFile"], secret.Data["caFile"] - switch { - case len(certBytes)+len(keyBytes)+len(caBytes) == 0: - return "", "", "", "", nil - case (len(certBytes) > 0 && len(keyBytes) == 0) || (len(keyBytes) > 0 && len(certBytes) == 0): - return "", "", "", "", fmt.Errorf("invalid '%s' secret data: fields 'certFile' and 'keyFile' require each other's presence", - secret.Name) - } - - var ( - certFile string - keyFile string - caFile string - err error - ) - - // create temporary folder to store the certs - tmpDir, err := os.MkdirTemp("", "helm-repo-oci-certs") - if err != nil { - return "", "", "", "", err - } - - if len(certBytes) > 0 && len(keyBytes) > 0 { - certFile, err = writeTofile(certBytes, "cert.pem", tmpDir) - if err != nil { - return "", "", "", "", err - } - keyFile, err = writeTofile(keyBytes, "key.pem", tmpDir) - if err != nil { - return "", "", "", "", err - } - } - if len(caBytes) > 0 { - caFile, err = writeTofile(caBytes, "ca.pem", tmpDir) - if err != nil { - return "", "", "", "", err - } - } - return certFile, keyFile, caFile, tmpDir, nil -} - -func writeTofile(data []byte, filename, tmpDir string) (string, error) { - file, err := os.CreateTemp(tmpDir, filename) - if err != nil { - return "", err - } - defer file.Close() - if _, err := file.Write(data); err != nil { - return "", err - } - return file.Name(), nil -} - func conditionsDiff(a, b []string) []string { bMap := make(map[string]struct{}, len(b)) for _, j := range b { diff --git a/internal/controller/helmrepository_controller_oci_test.go b/internal/controller/helmrepository_controller_oci_test.go index 432ef8ee0..dafb28618 100644 --- a/internal/controller/helmrepository_controller_oci_test.go +++ b/internal/controller/helmrepository_controller_oci_test.go @@ -170,9 +170,10 @@ func TestHelmRepositoryOCIReconciler_authStrategy(t *testing.T) { name string url string registryOpts registryOptions - withTLS bool + insecure bool secretOpts secretOptions secret *corev1.Secret + certsSecret *corev1.Secret provider string providerImg string want ctrl.Result @@ -277,6 +278,13 @@ func TestHelmRepositoryOCIReconciler_authStrategy(t *testing.T) { Name: "auth-secretref", }, Type: corev1.SecretTypeDockerConfigJson, + Data: map[string][]byte{}, + }, + certsSecret: &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "certs-secretref", + }, + Type: corev1.SecretTypeTLS, Data: map[string][]byte{ "caFile": []byte("invalid caFile"), }, @@ -303,6 +311,13 @@ func TestHelmRepositoryOCIReconciler_authStrategy(t *testing.T) { Type: corev1.SecretTypeDockerConfigJson, Data: map[string][]byte{}, }, + certsSecret: &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "certs-secretref", + }, + Type: corev1.SecretTypeTLS, + Data: map[string][]byte{}, + }, assertConditions: []metav1.Condition{ *conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingWithRetryReason, "processing object: new generation"), *conditions.FalseCondition(meta.ReadyCondition, sourcev1.AuthenticationFailedReason, "failed to login to registry"), @@ -314,7 +329,7 @@ func TestHelmRepositoryOCIReconciler_authStrategy(t *testing.T) { registryOpts: registryOptions{ withTLS: true, }, - withTLS: true, + insecure: true, secretOpts: secretOptions{ username: testRegistryUsername, password: testRegistryPassword, @@ -324,6 +339,13 @@ func TestHelmRepositoryOCIReconciler_authStrategy(t *testing.T) { Name: "auth-secretref", }, Type: corev1.SecretTypeDockerConfigJson, + Data: map[string][]byte{}, + }, + certsSecret: &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "certs-secretref", + }, + Type: corev1.SecretTypeTLS, Data: map[string][]byte{ "caFile": tlsCA, "certFile": tlsPublicKey, @@ -347,9 +369,9 @@ func TestHelmRepositoryOCIReconciler_authStrategy(t *testing.T) { workspaceDir := t.TempDir() server, err := setupRegistryServer(ctx, workspaceDir, tt.registryOpts) g.Expect(err).NotTo(HaveOccurred()) - if tt.withTLS { - defer server.stopSrv() - } + t.Cleanup(func() { + server.Close() + }) obj := &helmv1.HelmRepository{ ObjectMeta: metav1.ObjectMeta{ GenerateName: "auth-strategy-", @@ -386,6 +408,13 @@ func TestHelmRepositoryOCIReconciler_authStrategy(t *testing.T) { } } + if tt.certsSecret != nil { + clientBuilder.WithObjects(tt.certsSecret) + obj.Spec.CertSecretRef = &meta.LocalObjectReference{ + Name: tt.certsSecret.Name, + } + } + r := &HelmRepositoryOCIReconciler{ Client: clientBuilder.Build(), EventRecorder: record.NewFakeRecorder(32), diff --git a/internal/controller/helmrepository_controller_test.go b/internal/controller/helmrepository_controller_test.go index bd3e45f6a..0dfd2d65f 100644 --- a/internal/controller/helmrepository_controller_test.go +++ b/internal/controller/helmrepository_controller_test.go @@ -760,7 +760,7 @@ func TestHelmRepositoryReconciler_reconcileSource(t *testing.T) { if tt.url != "" { repoURL = tt.url } - tlsConf, serr = getter.TLSClientConfigFromSecret(*secret, repoURL) + tlsConf, _, serr = getter.TLSClientConfigFromSecret(*secret, repoURL) if serr != nil { validSecret = false } diff --git a/internal/controller/ocirepository_controller_test.go b/internal/controller/ocirepository_controller_test.go index 51584f421..1cfcbf18c 100644 --- a/internal/controller/ocirepository_controller_test.go +++ b/internal/controller/ocirepository_controller_test.go @@ -379,7 +379,7 @@ func TestOCIRepository_reconcileSource_authStrategy(t *testing.T) { craneOpts []crane.Option secretOpts secretOptions tlsCertSecret *corev1.Secret - withTLS bool + insecure bool provider string providerImg string want sreconcile.Result @@ -500,7 +500,7 @@ func TestOCIRepository_reconcileSource_authStrategy(t *testing.T) { registryOpts: registryOptions{ withTLS: true, }, - withTLS: true, + insecure: true, craneOpts: []crane.Option{crane.WithTransport(&http.Transport{ TLSClientConfig: &tls.Config{ RootCAs: pool, @@ -530,7 +530,7 @@ func TestOCIRepository_reconcileSource_authStrategy(t *testing.T) { registryOpts: registryOptions{ withTLS: true, }, - withTLS: true, + insecure: true, craneOpts: []crane.Option{crane.WithTransport(&http.Transport{ TLSClientConfig: &tls.Config{ RootCAs: pool, @@ -549,7 +549,7 @@ func TestOCIRepository_reconcileSource_authStrategy(t *testing.T) { registryOpts: registryOptions{ withTLS: true, }, - withTLS: true, + insecure: true, craneOpts: []crane.Option{crane.WithTransport(&http.Transport{ TLSClientConfig: &tls.Config{ RootCAs: pool, @@ -622,9 +622,9 @@ func TestOCIRepository_reconcileSource_authStrategy(t *testing.T) { workspaceDir := t.TempDir() server, err := setupRegistryServer(ctx, workspaceDir, tt.registryOpts) - if tt.withTLS { - defer server.stopSrv() - } + t.Cleanup(func() { + server.Close() + }) g.Expect(err).NotTo(HaveOccurred()) diff --git a/internal/controller/suite_test.go b/internal/controller/suite_test.go index a1fccd7b5..6956c1e3c 100644 --- a/internal/controller/suite_test.go +++ b/internal/controller/suite_test.go @@ -19,13 +19,12 @@ package controller import ( "bytes" "context" - "crypto/tls" - "crypto/x509" "fmt" "io" + "io/ioutil" + "log" "math/rand" "net" - "net/http" "os" "path/filepath" "testing" @@ -41,6 +40,7 @@ import ( "k8s.io/client-go/kubernetes/scheme" "k8s.io/client-go/tools/record" ctrl "sigs.k8s.io/controller-runtime" + "sigs.k8s.io/controller-runtime/pkg/client" "github.com/distribution/distribution/v3/configuration" dcontext "github.com/distribution/distribution/v3/context" @@ -79,6 +79,7 @@ const ( ) var ( + k8sClient client.Client testEnv *testenv.Environment testStorage *Storage testServer *testserver.ArtifactServer @@ -100,9 +101,11 @@ var ( ) var ( - tlsPublicKey []byte - tlsPrivateKey []byte - tlsCA []byte + tlsPublicKey []byte + tlsPrivateKey []byte + tlsCA []byte + clientPublicKey []byte + clientPrivateKey []byte ) var ( @@ -119,13 +122,18 @@ type registryClientTestServer struct { registryHost string workspaceDir string registryClient *helmreg.Client - // A mock DNS server needed for TLS connection testing. - srv *mockdns.Server + dnsServer *mockdns.Server } type registryOptions struct { - withBasicAuth bool - withTLS bool + withBasicAuth bool + withTLS bool + withClientCertAuth bool + // Allow disbaling DNS mocking since Helm OCI doesn't yet suppot + // insecure OCI registries, which means we need Docker's automatic + // connection downgrading if the registry is hosted on localhost. + // Once Helm OCI supports insecure registries, we can get rid of this. + disableDNSMocking bool } func setupRegistryServer(ctx context.Context, workspaceDir string, opts registryOptions) (*registryClientTestServer, error) { @@ -141,77 +149,47 @@ func setupRegistryServer(ctx context.Context, workspaceDir string, opts registry server.out = &out // init test client - if opts.withTLS { - pool := x509.NewCertPool() - if !pool.AppendCertsFromPEM(tlsCA) { - return nil, fmt.Errorf("failed to append CA certificate") - } - cert, err := tls.X509KeyPair(tlsPublicKey, tlsPrivateKey) - if err != nil { - return nil, fmt.Errorf("failed to load TLS key pair: %s", err) - } - httpClient := &http.Client{ - Transport: &http.Transport{ - TLSClientConfig: &tls.Config{ - RootCAs: pool, - Certificates: []tls.Certificate{cert}, - }, - }, - } - - client, err := helmreg.NewClient( - helmreg.ClientOptDebug(true), - helmreg.ClientOptWriter(server.out), - helmreg.ClientOptHTTPClient(httpClient), - ) - if err != nil { - return nil, fmt.Errorf("failed to create registry client: %s", err) - } - server.registryClient = client - } else { - client, err := helmreg.NewClient( - helmreg.ClientOptDebug(true), - helmreg.ClientOptWriter(server.out), - ) - if err != nil { - return nil, fmt.Errorf("failed to create registry client: %s", err) - } - server.registryClient = client + client, err := helmreg.NewClient( + helmreg.ClientOptDebug(true), + helmreg.ClientOptWriter(server.out), + ) + if err != nil { + return nil, fmt.Errorf("failed to create registry client: %s", err) } + server.registryClient = client config := &configuration.Configuration{} port, err := freeport.GetFreePort() if err != nil { return nil, fmt.Errorf("failed to get free port: %s", err) } - config.HTTP.Addr = fmt.Sprintf("127.0.0.1:%d", port) - config.HTTP.DrainTimeout = time.Duration(10) * time.Second - config.Storage = map[string]configuration.Parameters{"inmemory": map[string]interface{}{}} + server.registryHost = fmt.Sprintf("localhost:%d", port) - if opts.withTLS { - // docker `MatchLocalhost` is a host match function which returns true for - // localhost, and is used to enforce http for localhost requests." - // That function does not handle matching of ip addresses in octal, - // decimal or hex form. - server.registryHost = fmt.Sprintf("0x7f000001:%d", port) - // As of Go 1.20, Go may lookup "0x7f000001" as a DNS entry and fail. - // Using a mock DNS server to handle the address. - server.srv, err = mockdns.NewServer(map[string]mockdns.Zone{ - "0x7f000001.": { + + // Change the registry host to a host which is not localhost and + // mock DNS to map example.com to 127.0.0.1. + // This is required because Docker enforces HTTP if the registry + // is hosted on localhost/127.0.0.1. + if !opts.disableDNSMocking { + server.registryHost = fmt.Sprintf("example.com:%d", port) + // Disable DNS server logging as it is extremely chatty. + dnsLog := log.Default() + dnsLog.SetOutput(ioutil.Discard) + server.dnsServer, err = mockdns.NewServerWithLogger(map[string]mockdns.Zone{ + "example.com.": { A: []string{"127.0.0.1"}, }, - }, false) + }, dnsLog, false) if err != nil { - return nil, fmt.Errorf("failed to create mock DNS server: %s", err) + return nil, err } - server.srv.PatchNet(net.DefaultResolver) - - config.HTTP.TLS.Certificate = "testdata/certs/server.pem" - config.HTTP.TLS.Key = "testdata/certs/server-key.pem" - config.HTTP.TLS.ClientCAs = []string{"testdata/certs/ca.pem"} - + server.dnsServer.PatchNet(net.DefaultResolver) } + config.HTTP.Addr = fmt.Sprintf(":%d", port) + config.HTTP.DrainTimeout = time.Duration(10) * time.Second + config.Storage = map[string]configuration.Parameters{"inmemory": map[string]interface{}{}} + if opts.withBasicAuth { // create htpasswd file (w BCrypt, which is required) pwBytes, err := bcrypt.GenerateFromPassword([]byte(testRegistryPassword), bcrypt.DefaultCost) @@ -233,6 +211,15 @@ func setupRegistryServer(ctx context.Context, workspaceDir string, opts registry } } + if opts.withTLS { + config.HTTP.TLS.Certificate = "testdata/certs/server.pem" + config.HTTP.TLS.Key = "testdata/certs/server-key.pem" + // Configure CA certificates only if client cert authentication is enabled. + if opts.withClientCertAuth { + config.HTTP.TLS.ClientCAs = []string{"testdata/certs/ca.pem"} + } + } + // setup logger options config.Log.AccessLog.Disabled = true config.Log.Level = "error" @@ -251,10 +238,10 @@ func setupRegistryServer(ctx context.Context, workspaceDir string, opts registry return server, nil } -func (s *registryClientTestServer) stopSrv() { - if s.srv != nil { +func (r *registryClientTestServer) Close() { + if r.dnsServer != nil { mockdns.UnpatchNet(net.DefaultResolver) - s.srv.Close() + r.dnsServer.Close() } } @@ -270,6 +257,12 @@ func TestMain(m *testing.M) { ) var err error + // Initialize a cacheless client for tests that need the latest objects. + k8sClient, err = client.New(testEnv.Config, client.Options{Scheme: scheme.Scheme}) + if err != nil { + panic(fmt.Sprintf("failed to create k8s client: %v", err)) + } + testServer, err = testserver.NewTempArtifactServer() if err != nil { panic(fmt.Sprintf("Failed to create a temporary storage server: %v", err)) @@ -289,11 +282,13 @@ func TestMain(m *testing.M) { panic(fmt.Sprintf("failed to create workspace directory: %v", err)) } testRegistryServer, err = setupRegistryServer(ctx, testWorkspaceDir, registryOptions{ - withBasicAuth: true, + withBasicAuth: true, + disableDNSMocking: true, }) if err != nil { panic(fmt.Sprintf("Failed to create a test registry server: %v", err)) } + defer testRegistryServer.Close() if err := (&GitRepositoryReconciler{ Client: testEnv, @@ -414,6 +409,14 @@ func initTestTLS() { if err != nil { panic(err) } + clientPrivateKey, err = os.ReadFile("testdata/certs/client-key.pem") + if err != nil { + panic(err) + } + clientPublicKey, err = os.ReadFile("testdata/certs/client.pem") + if err != nil { + panic(err) + } } func newTestStorage(s *testserver.HTTPServer) (*Storage, error) { diff --git a/internal/controller/testdata/certs/ca-csr.json b/internal/controller/testdata/certs/ca-csr.json index 7cffb0e30..941277bb1 100644 --- a/internal/controller/testdata/certs/ca-csr.json +++ b/internal/controller/testdata/certs/ca-csr.json @@ -4,7 +4,6 @@ "127.0.0.1", "localhost", "example.com", - "www.example.com", - "0x7f000001" + "www.example.com" ] } diff --git a/internal/controller/testdata/certs/ca-key.pem b/internal/controller/testdata/certs/ca-key.pem index 16bca20ab..b69de5ab5 100644 --- a/internal/controller/testdata/certs/ca-key.pem +++ b/internal/controller/testdata/certs/ca-key.pem @@ -1,5 +1,5 @@ -----BEGIN EC PRIVATE KEY----- -MHcCAQEEIOXssMUfnHMMuufQpshjlYrR3zAbgcrkIrMqGVFQK6BpoAoGCCqGSM49 -AwEHoUQDQgAE5FNAUf6HpeTiM58p6N3NAsstI0RCvptmQy9b0eBUuW1+2ZGfQf2D -80FbuFw6zR4wr7A4PMLeLrVJNY5EY5b/TA== +MHcCAQEEIOH/u9dMcpVcZ0+X9Fc78dCTj8SHuXawhLjhu/ej64WToAoGCCqGSM49 +AwEHoUQDQgAEruH/kPxtX3cyYR2G7TYmxLq6AHyzo/NGXc9XjGzdJutE2SQzn37H +dvSJbH+Lvqo9ik0uiJVRVdCYD1j7gNszGA== -----END EC PRIVATE KEY----- diff --git a/internal/controller/testdata/certs/ca.csr b/internal/controller/testdata/certs/ca.csr index be5c862b9..baa8aeb26 100644 --- a/internal/controller/testdata/certs/ca.csr +++ b/internal/controller/testdata/certs/ca.csr @@ -1,9 +1,9 @@ -----BEGIN CERTIFICATE REQUEST----- -MIIBKzCB0gIBADAZMRcwFQYDVQQDEw5leGFtcGxlLmNvbSBDQTBZMBMGByqGSM49 -AgEGCCqGSM49AwEHA0IABORTQFH+h6Xk4jOfKejdzQLLLSNEQr6bZkMvW9HgVLlt -ftmRn0H9g/NBW7hcOs0eMK+wODzC3i61STWORGOW/0ygVzBVBgkqhkiG9w0BCQ4x -SDBGMEQGA1UdEQQ9MDuCCWxvY2FsaG9zdIILZXhhbXBsZS5jb22CD3d3dy5leGFt -cGxlLmNvbYIKMHg3ZjAwMDAwMYcEfwAAATAKBggqhkjOPQQDAgNIADBFAiBQLSRn -qPbbMyBZSlLcCH23gBTcR4axsAw4fCazBjMJ2wIhAPQ671kzMSn63wInDjWxsdF1 -t0c4ZH8jHpkUEAQ74UVW +MIIBIDCBxgIBADAZMRcwFQYDVQQDEw5leGFtcGxlLmNvbSBDQTBZMBMGByqGSM49 +AgEGCCqGSM49AwEHA0IABK7h/5D8bV93MmEdhu02JsS6ugB8s6PzRl3PV4xs3Sbr +RNkkM59+x3b0iWx/i76qPYpNLoiVUVXQmA9Y+4DbMxigSzBJBgkqhkiG9w0BCQ4x +PDA6MDgGA1UdEQQxMC+CCWxvY2FsaG9zdIILZXhhbXBsZS5jb22CD3d3dy5leGFt +cGxlLmNvbYcEfwAAATAKBggqhkjOPQQDAgNJADBGAiEAkw85nyLhJssyCYsaFvRU +EErhu66xHPJug/nG50uV5OoCIQCUorrflOSxfChPeCe4xfwcPv7FpcCYbKVYtGzz +b34Wow== -----END CERTIFICATE REQUEST----- diff --git a/internal/controller/testdata/certs/ca.pem b/internal/controller/testdata/certs/ca.pem index 188443c3d..080bd24e6 100644 --- a/internal/controller/testdata/certs/ca.pem +++ b/internal/controller/testdata/certs/ca.pem @@ -1,11 +1,11 @@ -----BEGIN CERTIFICATE----- -MIIBhjCCAS2gAwIBAgIUE/XiwPHY1izwrIwbQiXxqO/Q8KswCgYIKoZIzj0EAwIw -GTEXMBUGA1UEAxMOZXhhbXBsZS5jb20gQ0EwHhcNMjMwNTI0MTQyMTAwWhcNMjgw -NTIyMTQyMTAwWjAZMRcwFQYDVQQDEw5leGFtcGxlLmNvbSBDQTBZMBMGByqGSM49 -AgEGCCqGSM49AwEHA0IABORTQFH+h6Xk4jOfKejdzQLLLSNEQr6bZkMvW9HgVLlt -ftmRn0H9g/NBW7hcOs0eMK+wODzC3i61STWORGOW/0yjUzBRMA4GA1UdDwEB/wQE -AwIBBjAPBgNVHRMBAf8EBTADAQH/MB0GA1UdDgQWBBRSGfT6hp/EUOgqOjRm4Vzs -Pt8QiTAPBgNVHREECDAGhwR/AAABMAoGCCqGSM49BAMCA0cAMEQCIGqg/rOrI9Oq -EBEmPyCtehmKWLcpKDBMTzZAponByVlwAiATsP/meJNiEsMvOK2Q7UFvi2+c3Rcj -NUQAcAm8iEtJQQ== +MIIBhzCCAS2gAwIBAgIUdsAtiX3gN0uk7ddxASWYE/tdv0wwCgYIKoZIzj0EAwIw +GTEXMBUGA1UEAxMOZXhhbXBsZS5jb20gQ0EwHhcNMjAwNDE3MDgxODAwWhcNMjUw +NDE2MDgxODAwWjAZMRcwFQYDVQQDEw5leGFtcGxlLmNvbSBDQTBZMBMGByqGSM49 +AgEGCCqGSM49AwEHA0IABK7h/5D8bV93MmEdhu02JsS6ugB8s6PzRl3PV4xs3Sbr +RNkkM59+x3b0iWx/i76qPYpNLoiVUVXQmA9Y+4DbMxijUzBRMA4GA1UdDwEB/wQE +AwIBBjAPBgNVHRMBAf8EBTADAQH/MB0GA1UdDgQWBBQGyUiU1QEZiMAqjsnIYTwZ +4yp5wzAPBgNVHREECDAGhwR/AAABMAoGCCqGSM49BAMCA0gAMEUCIQDzdtvKdE8O +1+WRTZ9MuSiFYcrEz7Zne7VXouDEKqKEigIgM4WlbDeuNCKbqhqj+xZV0pa3rweb +OD8EjjCMY69RMO0= -----END CERTIFICATE----- diff --git a/internal/controller/testdata/certs/server-csr.json b/internal/controller/testdata/certs/server-csr.json index 9fb0c7962..0baf11601 100644 --- a/internal/controller/testdata/certs/server-csr.json +++ b/internal/controller/testdata/certs/server-csr.json @@ -4,7 +4,6 @@ "127.0.0.1", "localhost", "example.com", - "www.example.com", - "0x7f000001" + "www.example.com" ] } diff --git a/internal/controller/testdata/certs/server-key.pem b/internal/controller/testdata/certs/server-key.pem index d147ad6a5..5054ff39f 100644 --- a/internal/controller/testdata/certs/server-key.pem +++ b/internal/controller/testdata/certs/server-key.pem @@ -1,5 +1,5 @@ -----BEGIN EC PRIVATE KEY----- -MHcCAQEEIJ68GBu37yyAfzMnKMgo950R8OYoGrt6DUwVg1Wy5c+ZoAoGCCqGSM49 -AwEHoUQDQgAE/2T6ANTLUGqz/QWJi35H6ijcaDabFwU2IxqNu9iwwQz7e+CkCqcu -6wLAn1Q8URpG3CGTS2td0a9v2zPqSi9eAQ== +MHcCAQEEIKQbEXV6nljOHMmPrWVWQ+JrAE5wsbE9iMhfY7wlJgXOoAoGCCqGSM49 +AwEHoUQDQgAE+53oBGlrvVUTelSGYji8GNHVhVg8jOs1PeeLuXCIZjQmctHLFEq3 +fE+mGxCL93MtpYzlwIWBf0m7pEGQre6bzg== -----END EC PRIVATE KEY----- diff --git a/internal/controller/testdata/certs/server.csr b/internal/controller/testdata/certs/server.csr index 941dd4f92..5caf7b39c 100644 --- a/internal/controller/testdata/certs/server.csr +++ b/internal/controller/testdata/certs/server.csr @@ -1,9 +1,8 @@ -----BEGIN CERTIFICATE REQUEST----- -MIIBJzCBzwIBADAWMRQwEgYDVQQDEwtleGFtcGxlLmNvbTBZMBMGByqGSM49AgEG -CCqGSM49AwEHA0IABP9k+gDUy1Bqs/0FiYt+R+oo3Gg2mxcFNiMajbvYsMEM+3vg -pAqnLusCwJ9UPFEaRtwhk0trXdGvb9sz6kovXgGgVzBVBgkqhkiG9w0BCQ4xSDBG -MEQGA1UdEQQ9MDuCCWxvY2FsaG9zdIILZXhhbXBsZS5jb22CD3d3dy5leGFtcGxl -LmNvbYIKMHg3ZjAwMDAwMYcEfwAAATAKBggqhkjOPQQDAgNHADBEAiAyNdC5xrMi -E3MopSm30C0D3WI0dV2ygCC6NsB/VyKkyAIgYGG4A1F8EDn4e9T7237GIg21a0tc -oD7WyWEgeLOzb9c= +MIIBHDCBwwIBADAWMRQwEgYDVQQDEwtleGFtcGxlLmNvbTBZMBMGByqGSM49AgEG +CCqGSM49AwEHA0IABPud6ARpa71VE3pUhmI4vBjR1YVYPIzrNT3ni7lwiGY0JnLR +yxRKt3xPphsQi/dzLaWM5cCFgX9Ju6RBkK3um86gSzBJBgkqhkiG9w0BCQ4xPDA6 +MDgGA1UdEQQxMC+CCWxvY2FsaG9zdIILZXhhbXBsZS5jb22CD3d3dy5leGFtcGxl +LmNvbYcEfwAAATAKBggqhkjOPQQDAgNIADBFAiB5A6wvQ5x6g/zhiyn+wLzXsOaB +Gb/F25p/zTHHQqZbkwIhAPUgWzy/2bs6eZEi97bSlaRdmrqHwqT842t5sEwGyXNV -----END CERTIFICATE REQUEST----- diff --git a/internal/controller/testdata/certs/server.pem b/internal/controller/testdata/certs/server.pem index 232a57eb4..11c655a0b 100644 --- a/internal/controller/testdata/certs/server.pem +++ b/internal/controller/testdata/certs/server.pem @@ -1,13 +1,13 @@ -----BEGIN CERTIFICATE----- -MIIB+TCCAZ6gAwIBAgIUIGxnexcJoQ+H1wPEkwe5c7gwxbAwCgYIKoZIzj0EAwIw -GTEXMBUGA1UEAxMOZXhhbXBsZS5jb20gQ0EwHhcNMjMwNTI0MTQyMTAwWhcNMzMw -NTIxMTQyMTAwWjAWMRQwEgYDVQQDEwtleGFtcGxlLmNvbTBZMBMGByqGSM49AgEG -CCqGSM49AwEHA0IABP9k+gDUy1Bqs/0FiYt+R+oo3Gg2mxcFNiMajbvYsMEM+3vg -pAqnLusCwJ9UPFEaRtwhk0trXdGvb9sz6kovXgGjgcYwgcMwDgYDVR0PAQH/BAQD +MIIB7TCCAZKgAwIBAgIUB+17B8PU05wVTzRHLeG+S+ybZK4wCgYIKoZIzj0EAwIw +GTEXMBUGA1UEAxMOZXhhbXBsZS5jb20gQ0EwHhcNMjAwNDE3MDgxODAwWhcNMzAw +NDE1MDgxODAwWjAWMRQwEgYDVQQDEwtleGFtcGxlLmNvbTBZMBMGByqGSM49AgEG +CCqGSM49AwEHA0IABPud6ARpa71VE3pUhmI4vBjR1YVYPIzrNT3ni7lwiGY0JnLR +yxRKt3xPphsQi/dzLaWM5cCFgX9Ju6RBkK3um86jgbowgbcwDgYDVR0PAQH/BAQD AgWgMB0GA1UdJQQWMBQGCCsGAQUFBwMBBggrBgEFBQcDAjAMBgNVHRMBAf8EAjAA -MB0GA1UdDgQWBBQ1Z+HHTrEjwJsOvNaXPhoiC3whBjAfBgNVHSMEGDAWgBRSGfT6 -hp/EUOgqOjRm4VzsPt8QiTBEBgNVHREEPTA7gglsb2NhbGhvc3SCC2V4YW1wbGUu -Y29tgg93d3cuZXhhbXBsZS5jb22CCjB4N2YwMDAwMDGHBH8AAAEwCgYIKoZIzj0E -AwIDSQAwRgIhAIlapRW3ve2Q0I+dkUkJZM3pSjEUj5Vnr5+XAGa1Hj7vAiEAtoXs -AGr5+WetlgkeRmxIqEAxtm8DED72dg5rTDueU04= +MB0GA1UdDgQWBBTM8HS5EIlVMBYv/300jN8PEArUgDAfBgNVHSMEGDAWgBQGyUiU +1QEZiMAqjsnIYTwZ4yp5wzA4BgNVHREEMTAvgglsb2NhbGhvc3SCC2V4YW1wbGUu +Y29tgg93d3cuZXhhbXBsZS5jb22HBH8AAAEwCgYIKoZIzj0EAwIDSQAwRgIhAOgB +5W82FEgiTTOmsNRekkK5jUPbj4D4eHtb2/BI7ph4AiEA2AxHASIFBdv5b7Qf5prb +bdNmUCzAvVuCAKuMjg2OPrE= -----END CERTIFICATE----- diff --git a/internal/helm/getter/client_opts.go b/internal/helm/getter/client_opts.go index 2af928c8e..770e9f62c 100644 --- a/internal/helm/getter/client_opts.go +++ b/internal/helm/getter/client_opts.go @@ -23,6 +23,7 @@ import ( "errors" "fmt" "net/url" + "os" "github.com/fluxcd/pkg/oci" "github.com/google/go-containerregistry/pkg/authn" @@ -37,6 +38,12 @@ import ( soci "github.com/fluxcd/source-controller/internal/oci" ) +const ( + certFile = "cert.pem" + keyFile = "key.pem" + caFile = "ca.pem" +) + var ErrDeprecatedTLSConfig = errors.New("TLS configured in a deprecated manner") // ClientOpts contains the various options to use while constructing @@ -44,16 +51,28 @@ var ErrDeprecatedTLSConfig = errors.New("TLS configured in a deprecated manner") type ClientOpts struct { Authenticator authn.Authenticator Keychain authn.Keychain - RegLoginOpt helmreg.LoginOption + RegLoginOpts []helmreg.LoginOption TlsConfig *tls.Config GetterOpts []helmgetter.Option } +// TLSBytes contains the bytes of the TLS files. +type TLSBytes struct { + // CertBytes is the bytes of the certificate file. + CertBytes []byte + // KeyBytes is the bytes of the key file. + KeyBytes []byte + // CaBytes is the bytes of the CA file. + CaBytes []byte +} + // GetClientOpts uses the provided HelmRepository object and a normalized // URL to construct a HelmClientOpts object. If obj is an OCI HelmRepository, // then the returned options object will also contain the required registry // auth mechanisms. -func GetClientOpts(ctx context.Context, c client.Client, obj *helmv1.HelmRepository, url string) (*ClientOpts, error) { +// A temporary directory is created to store the certs files if needed. It is the +// caller's responsibility to clean up the directory. +func GetClientOpts(ctx context.Context, c client.Client, obj *helmv1.HelmRepository, url string) (*ClientOpts, string, error) { hrOpts := &ClientOpts{ GetterOpts: []helmgetter.Option{ helmgetter.WithURL(url), @@ -63,18 +82,25 @@ func GetClientOpts(ctx context.Context, c client.Client, obj *helmv1.HelmReposit } ociRepo := obj.Spec.Type == helmv1.HelmRepositoryTypeOCI - var certSecret *corev1.Secret - var err error + var ( + certSecret *corev1.Secret + tlsBytes *TLSBytes + certFile string + keyFile string + caFile string + dir string + err error + ) // Check `.spec.certSecretRef` first for any TLS auth data. if obj.Spec.CertSecretRef != nil { certSecret, err = fetchSecret(ctx, c, obj.Spec.CertSecretRef.Name, obj.GetNamespace()) if err != nil { - return nil, fmt.Errorf("failed to get TLS authentication secret '%s/%s': %w", obj.GetNamespace(), obj.Spec.CertSecretRef.Name, err) + return nil, "", fmt.Errorf("failed to get TLS authentication secret '%s/%s': %w", obj.GetNamespace(), obj.Spec.CertSecretRef.Name, err) } - hrOpts.TlsConfig, err = TLSClientConfigFromSecret(*certSecret, url) + hrOpts.TlsConfig, tlsBytes, err = TLSClientConfigFromSecret(*certSecret, url) if err != nil { - return nil, fmt.Errorf("failed to construct Helm client's TLS config: %w", err) + return nil, "", fmt.Errorf("failed to construct Helm client's TLS config: %w", err) } } @@ -83,22 +109,22 @@ func GetClientOpts(ctx context.Context, c client.Client, obj *helmv1.HelmReposit if obj.Spec.SecretRef != nil { authSecret, err = fetchSecret(ctx, c, obj.Spec.SecretRef.Name, obj.GetNamespace()) if err != nil { - return nil, fmt.Errorf("failed to get authentication secret '%s/%s': %w", obj.GetNamespace(), obj.Spec.SecretRef.Name, err) + return nil, "", fmt.Errorf("failed to get authentication secret '%s/%s': %w", obj.GetNamespace(), obj.Spec.SecretRef.Name, err) } // Construct actual Helm client options. opts, err := GetterOptionsFromSecret(*authSecret) if err != nil { - return nil, fmt.Errorf("failed to configure Helm client: %w", err) + return nil, "", fmt.Errorf("failed to configure Helm client: %w", err) } hrOpts.GetterOpts = append(hrOpts.GetterOpts, opts...) // If the TLS config is nil, i.e. one couldn't be constructed using `.spec.certSecretRef` - // then try to use `.spec.certSecretRef`. + // then try to use `.spec.secretRef`. if hrOpts.TlsConfig == nil { - hrOpts.TlsConfig, err = TLSClientConfigFromSecret(*authSecret, url) + hrOpts.TlsConfig, tlsBytes, err = TLSClientConfigFromSecret(*authSecret, url) if err != nil { - return nil, fmt.Errorf("failed to construct Helm client's TLS config: %w", err) + return nil, "", fmt.Errorf("failed to construct Helm client's TLS config: %w", err) } // Constructing a TLS config using the auth secret is deprecated behavior. if hrOpts.TlsConfig != nil { @@ -107,15 +133,27 @@ func GetClientOpts(ctx context.Context, c client.Client, obj *helmv1.HelmReposit } if ociRepo { + // Now that we have checked for TLS config, persist the certs files to the path if needed. + if tlsBytes != nil { + dir, err = os.MkdirTemp("", "helm-repo-oci-certs") + if err != nil { + return nil, "", fmt.Errorf("cannot create temporary directory: %w", err) + } + certFile, keyFile, caFile, err = StoreCertsFiles(tlsBytes, dir) + if err != nil { + return nil, "", fmt.Errorf("cannot write certs files to path: %w", err) + } + } + hrOpts.Keychain, err = registry.LoginOptionFromSecret(url, *authSecret) if err != nil { - return nil, fmt.Errorf("failed to configure login options: %w", err) + return nil, "", fmt.Errorf("failed to configure login options: %w", err) } } } else if obj.Spec.Provider != helmv1.GenericOCIProvider && obj.Spec.Type == helmv1.HelmRepositoryTypeOCI && ociRepo { authenticator, authErr := soci.OIDCAuth(ctx, obj.Spec.URL, obj.Spec.Provider) if authErr != nil && !errors.Is(authErr, oci.ErrUnconfiguredProvider) { - return nil, fmt.Errorf("failed to get credential from '%s': %w", obj.Spec.Provider, authErr) + return nil, "", fmt.Errorf("failed to get credential from '%s': %w", obj.Spec.Provider, authErr) } if authenticator != nil { hrOpts.Authenticator = authenticator @@ -123,16 +161,21 @@ func GetClientOpts(ctx context.Context, c client.Client, obj *helmv1.HelmReposit } if ociRepo { - hrOpts.RegLoginOpt, err = registry.NewLoginOption(hrOpts.Authenticator, hrOpts.Keychain, url) + loginOpt, err := registry.NewLoginOption(hrOpts.Authenticator, hrOpts.Keychain, url) if err != nil { - return nil, err + return nil, "", err + } + hrOpts.RegLoginOpts = []helmreg.LoginOption{loginOpt} + tlsLoginOpt := registry.TLSLoginOption(certFile, keyFile, caFile) + if tlsLoginOpt != nil { + hrOpts.RegLoginOpts = append(hrOpts.RegLoginOpts, tlsLoginOpt) } } if deprecatedTLSConfig { err = ErrDeprecatedTLSConfig } - return hrOpts, err + return hrOpts, dir, err } func fetchSecret(ctx context.Context, c client.Client, name, namespace string) (*corev1.Secret, error) { @@ -152,13 +195,14 @@ func fetchSecret(ctx context.Context, c client.Client, name, namespace string) ( // // Secrets with no certFile, keyFile, AND caFile are ignored, if only a // certBytes OR keyBytes is defined it returns an error. -func TLSClientConfigFromSecret(secret corev1.Secret, repositoryUrl string) (*tls.Config, error) { +// If shouldStore is true, it will write the certs files to the path. +func TLSClientConfigFromSecret(secret corev1.Secret, repositoryUrl string) (*tls.Config, *TLSBytes, error) { certBytes, keyBytes, caBytes := secret.Data["certFile"], secret.Data["keyFile"], secret.Data["caFile"] switch { case len(certBytes)+len(keyBytes)+len(caBytes) == 0: - return nil, nil + return nil, nil, nil case (len(certBytes) > 0 && len(keyBytes) == 0) || (len(keyBytes) > 0 && len(certBytes) == 0): - return nil, fmt.Errorf("invalid '%s' secret data: fields 'certFile' and 'keyFile' require each other's presence", + return nil, nil, fmt.Errorf("invalid '%s' secret data: fields 'certFile' and 'keyFile' require each other's presence", secret.Name) } @@ -166,7 +210,7 @@ func TLSClientConfigFromSecret(secret corev1.Secret, repositoryUrl string) (*tls if len(certBytes) > 0 && len(keyBytes) > 0 { cert, err := tls.X509KeyPair(certBytes, keyBytes) if err != nil { - return nil, err + return nil, nil, err } tlsConf.Certificates = append(tlsConf.Certificates, cert) } @@ -174,10 +218,10 @@ func TLSClientConfigFromSecret(secret corev1.Secret, repositoryUrl string) (*tls if len(caBytes) > 0 { cp, err := x509.SystemCertPool() if err != nil { - return nil, fmt.Errorf("cannot retrieve system certificate pool: %w", err) + return nil, nil, fmt.Errorf("cannot retrieve system certificate pool: %w", err) } if !cp.AppendCertsFromPEM(caBytes) { - return nil, fmt.Errorf("cannot append certificate into certificate pool: invalid caFile") + return nil, nil, fmt.Errorf("cannot append certificate into certificate pool: invalid caFile") } tlsConf.RootCAs = cp @@ -187,10 +231,53 @@ func TLSClientConfigFromSecret(secret corev1.Secret, repositoryUrl string) (*tls u, err := url.Parse(repositoryUrl) if err != nil { - return nil, fmt.Errorf("cannot parse repository URL: %w", err) + return nil, nil, fmt.Errorf("cannot parse repository URL: %w", err) } tlsConf.ServerName = u.Hostname() - return tlsConf, nil + return tlsConf, &TLSBytes{ + CertBytes: certBytes, + KeyBytes: keyBytes, + CaBytes: caBytes, + }, nil +} + +// StoreCertsFiles writes the certs files to the path and returns the path of the files. +func StoreCertsFiles(tlsBytes *TLSBytes, path string) (string, string, string, error) { + var ( + certFile string + keyFile string + caFile string + err error + ) + if len(tlsBytes.CertBytes) > 0 && len(tlsBytes.KeyBytes) > 0 { + certFile, err = writeTofile(tlsBytes.CertBytes, certFile, path) + if err != nil { + return "", "", "", err + } + keyFile, err = writeTofile(tlsBytes.KeyBytes, keyFile, path) + if err != nil { + return "", "", "", err + } + } + if len(tlsBytes.CaBytes) > 0 { + caFile, err = writeTofile(tlsBytes.CaBytes, caFile, path) + if err != nil { + return "", "", "", err + } + } + return certFile, keyFile, caFile, nil +} + +func writeTofile(data []byte, filename, tmpDir string) (string, error) { + file, err := os.CreateTemp(tmpDir, filename) + if err != nil { + return "", err + } + defer file.Close() + if _, err := file.Write(data); err != nil { + return "", err + } + return file.Name(), nil } diff --git a/internal/helm/getter/client_opts_test.go b/internal/helm/getter/client_opts_test.go index 2231e2a52..e97f29d0f 100644 --- a/internal/helm/getter/client_opts_test.go +++ b/internal/helm/getter/client_opts_test.go @@ -149,7 +149,7 @@ func TestGetClientOpts(t *testing.T) { } c := clientBuilder.Build() - clientOpts, err := GetClientOpts(context.TODO(), c, helmRepo, "https://ghcr.io/dummy") + clientOpts, _, err := GetClientOpts(context.TODO(), c, helmRepo, "https://ghcr.io/dummy") if tt.err != nil { g.Expect(err).To(Equal(tt.err)) } else { @@ -183,7 +183,7 @@ func Test_tlsClientConfigFromSecret(t *testing.T) { tt.modify(secret) } - got, err := TLSClientConfigFromSecret(*secret, "") + got, _, err := TLSClientConfigFromSecret(*secret, "") if (err != nil) != tt.wantErr { t.Errorf("TLSClientConfigFromSecret() error = %v, wantErr %v", err, tt.wantErr) return @@ -196,6 +196,118 @@ func Test_tlsClientConfigFromSecret(t *testing.T) { } } +func Test_registryTLSoginOption(t *testing.T) { + tlsCA, err := os.ReadFile("../../controller/testdata/certs/ca.pem") + if err != nil { + t.Errorf("could not read CA file: %s", err) + } + + tests := []struct { + name string + certSecret *corev1.Secret + authSecret *corev1.Secret + wantNil bool + }{ + { + "caFile", + &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "ca-file", + }, + Data: map[string][]byte{ + "caFile": tlsCA, + }, + }, + &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "auth-oci", + }, + Data: map[string][]byte{ + "username": []byte("user"), + "password": []byte("pass"), + }, + }, + false, + }, + { + "without caFile", + &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "ca-file", + }, + Data: map[string][]byte{}, + }, + &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "auth-oci", + }, + Data: map[string][]byte{ + "username": []byte("user"), + "password": []byte("pass"), + }, + }, + true, + }, + { + "empty", + nil, + &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "auth-oci", + }, + Data: map[string][]byte{ + "username": []byte("user"), + "password": []byte("pass"), + }, + }, + true, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + helmRepo := &helmv1.HelmRepository{ + Spec: helmv1.HelmRepositorySpec{ + Timeout: &metav1.Duration{ + Duration: time.Second, + }, + Type: helmv1.HelmRepositoryTypeOCI, + }, + } + + clientBuilder := fakeclient.NewClientBuilder() + + if tt.authSecret != nil { + clientBuilder.WithObjects(tt.authSecret.DeepCopy()) + helmRepo.Spec.SecretRef = &meta.LocalObjectReference{ + Name: tt.authSecret.Name, + } + } + + if tt.certSecret != nil { + clientBuilder.WithObjects(tt.certSecret.DeepCopy()) + helmRepo.Spec.CertSecretRef = &meta.LocalObjectReference{ + Name: tt.certSecret.Name, + } + } + c := clientBuilder.Build() + + clientOpts, tmpDir, err := GetClientOpts(context.TODO(), c, helmRepo, "https://ghcr.io/dummy") + if err != nil { + t.Errorf("GetClientOpts() error = %v", err) + return + } + if tmpDir != "" { + defer os.RemoveAll(tmpDir) + } + if tt.wantNil && len(clientOpts.RegLoginOpts) != 1 { + // we should have a login option but no TLS option + t.Error("registryTLSoginOption() != nil") + return + } + }) + } +} + // validTlsSecret creates a secret containing key pair and CA certificate that are // valid from a syntax (minimum requirements) perspective. func validTlsSecret(t *testing.T) corev1.Secret { diff --git a/internal/helm/registry/auth.go b/internal/helm/registry/auth.go index c48ec0b2b..08b1a98e6 100644 --- a/internal/helm/registry/auth.go +++ b/internal/helm/registry/auth.go @@ -154,3 +154,12 @@ func NewLoginOption(auth authn.Authenticator, keychain authn.Keychain, registryU return nil, nil } + +// TLSLoginOptionFromSecret returns a LoginOption that can be used to configure the TLS client. +func TLSLoginOption(certFile, keyFile, caFile string) registry.LoginOption { + if (certFile != "" && keyFile != "") || caFile != "" { + return registry.LoginOptTLSClientConfig(certFile, keyFile, caFile) + } + + return nil +} diff --git a/internal/helm/registry/client.go b/internal/helm/registry/client.go index e584f6b3b..7ac0d3d0b 100644 --- a/internal/helm/registry/client.go +++ b/internal/helm/registry/client.go @@ -62,34 +62,19 @@ func ClientGenerator(tlsConfig *tls.Config, isLogin bool) (*registry.Client, str } func newClient(credentialsFile string, tlsConfig *tls.Config) (*registry.Client, error) { - if tlsConfig != nil { - return newClientWithTLS(credentialsFile, tlsConfig) - } - return newDefaultClient(credentialsFile) -} - -func newDefaultClient(credentialsFile string) (*registry.Client, error) { - if credentialsFile == "" { - return registry.NewClient(registry.ClientOptWriter(io.Discard)) + opts := []registry.ClientOption{ + registry.ClientOptWriter(io.Discard), } - return registry.NewClient(registry.ClientOptWriter(io.Discard), - registry.ClientOptCredentialsFile(credentialsFile)) -} - -func newClientWithTLS(credentialsFile string, tlsConfig *tls.Config) (*registry.Client, error) { - if credentialsFile == "" { - return registry.NewClient(registry.ClientOptWriter(io.Discard), - registry.ClientOptHTTPClient(&http.Client{ - Transport: &http.Transport{ - TLSClientConfig: tlsConfig, - }, - })) - } - return registry.NewClient(registry.ClientOptWriter(io.Discard), - registry.ClientOptCredentialsFile(credentialsFile), - registry.ClientOptHTTPClient(&http.Client{ + if tlsConfig != nil { + opts = append(opts, registry.ClientOptHTTPClient(&http.Client{ Transport: &http.Transport{ TLSClientConfig: tlsConfig, }, })) + } + if credentialsFile != "" { + opts = append(opts, registry.ClientOptCredentialsFile(credentialsFile)) + } + + return registry.NewClient(opts...) }