From f787fc70465ca4cbaf80917ec461a691076dbc19 Mon Sep 17 00:00:00 2001 From: Sanskar Jaiswal Date: Thu, 31 Aug 2023 15:52:08 +0530 Subject: [PATCH] helmrepo: fix Secret type check for TLS via `.spec.secretRef` This is a regression fix introduced in a302c71 which would wrongly check for the type of the Secret specified in `.spec.secretRef` while configuring TLS data. Introduce `LegacyTLSClientConfigFromSecret` which does not check the Secret type while constructing the TLS config. Signed-off-by: Sanskar Jaiswal --- .../helmrepository_controller_test.go | 64 +++++++++++++++++++ internal/helm/getter/client_opts.go | 6 +- internal/tls/config.go | 45 +++++++++---- internal/tls/config_test.go | 35 ++++++---- 4 files changed, 123 insertions(+), 27 deletions(-) diff --git a/internal/controller/helmrepository_controller_test.go b/internal/controller/helmrepository_controller_test.go index ae0273f1f..2c90ae917 100644 --- a/internal/controller/helmrepository_controller_test.go +++ b/internal/controller/helmrepository_controller_test.go @@ -506,6 +506,38 @@ func TestHelmRepositoryReconciler_reconcileSource(t *testing.T) { t.Expect(artifact.Revision).ToNot(BeEmpty()) }, }, + { + // Regression test for: https://github.com/fluxcd/source-controller/issues/1218 + name: "HTTPS with docker config secretRef and caFile key makes ArtifactOutdated=True", + protocol: "https", + server: options{ + publicKey: tlsPublicKey, + privateKey: tlsPrivateKey, + ca: tlsCA, + }, + secret: &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "ca-file", + }, + Data: map[string][]byte{ + "caFile": tlsCA, + }, + Type: corev1.SecretTypeDockerConfigJson, + }, + beforeFunc: func(t *WithT, obj *helmv1.HelmRepository, rev digest.Digest) { + obj.Spec.SecretRef = &meta.LocalObjectReference{Name: "ca-file"} + }, + want: sreconcile.ResultSuccess, + assertConditions: []metav1.Condition{ + *conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingReason, "building artifact: new index revision"), + *conditions.UnknownCondition(meta.ReadyCondition, meta.ProgressingReason, "building artifact: new index revision"), + }, + afterFunc: func(t *WithT, obj *helmv1.HelmRepository, artifact sourcev1.Artifact, chartRepo *repository.ChartRepository) { + t.Expect(chartRepo.Path).ToNot(BeEmpty()) + t.Expect(chartRepo.Index).ToNot(BeNil()) + t.Expect(artifact.Revision).ToNot(BeEmpty()) + }, + }, { name: "HTTP without secretRef makes ArtifactOutdated=True", protocol: "http", @@ -550,6 +582,38 @@ func TestHelmRepositoryReconciler_reconcileSource(t *testing.T) { t.Expect(artifact.Revision).ToNot(BeEmpty()) }, }, + { + // Regression test for: https://github.com/fluxcd/source-controller/issues/1218 + name: "HTTP with docker config secretRef sets Reconciling=True", + protocol: "http", + server: options{ + username: "git", + password: "1234", + }, + secret: &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "basic-auth", + }, + Data: map[string][]byte{ + "username": []byte("git"), + "password": []byte("1234"), + }, + Type: corev1.SecretTypeDockerConfigJson, + }, + beforeFunc: func(t *WithT, obj *helmv1.HelmRepository, rev digest.Digest) { + obj.Spec.SecretRef = &meta.LocalObjectReference{Name: "basic-auth"} + }, + want: sreconcile.ResultSuccess, + assertConditions: []metav1.Condition{ + *conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingReason, "building artifact: new index revision"), + *conditions.UnknownCondition(meta.ReadyCondition, meta.ProgressingReason, "building artifact: new index revision"), + }, + afterFunc: func(t *WithT, obj *helmv1.HelmRepository, artifact sourcev1.Artifact, chartRepo *repository.ChartRepository) { + t.Expect(chartRepo.Path).ToNot(BeEmpty()) + t.Expect(chartRepo.Index).ToNot(BeNil()) + t.Expect(artifact.Revision).ToNot(BeEmpty()) + }, + }, { name: "HTTPS with invalid CAFile in certSecretRef makes FetchFailed=True and returns error", protocol: "https", diff --git a/internal/helm/getter/client_opts.go b/internal/helm/getter/client_opts.go index 4e77f290a..5c2755bf5 100644 --- a/internal/helm/getter/client_opts.go +++ b/internal/helm/getter/client_opts.go @@ -115,10 +115,10 @@ func GetClientOpts(ctx context.Context, c client.Client, obj *helmv1.HelmReposit } 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.secretRef`. + // If the TLS config is nil, i.e. one couldn't be constructed using + // `.spec.certSecretRef`, then try to use `.spec.secretRef`. if hrOpts.TlsConfig == nil && !ociRepo { - hrOpts.TlsConfig, tlsBytes, err = stls.TLSClientConfigFromSecret(*authSecret, url) + hrOpts.TlsConfig, tlsBytes, err = stls.LegacyTLSClientConfigFromSecret(*authSecret, url) if err != nil { return nil, "", fmt.Errorf("failed to construct Helm client's TLS config: %w", err) } diff --git a/internal/tls/config.go b/internal/tls/config.go index 9d9eee9f7..841c9538e 100644 --- a/internal/tls/config.go +++ b/internal/tls/config.go @@ -45,9 +45,10 @@ type TLSBytes struct { // - ca.crt, for the CA certificate // // Secrets with no certificate, private key, AND CA cert are ignored. If only a -// certificate OR private key is found, an error is returned. +// certificate OR private key is found, an error is returned. The Secret type +// can be blank, Opaque or kubernetes.io/tls. func KubeTLSClientConfigFromSecret(secret corev1.Secret, url string) (*tls.Config, *TLSBytes, error) { - return tlsClientConfigFromSecret(secret, url, true) + return tlsClientConfigFromSecret(secret, url, true, true) } // TLSClientConfigFromSecret returns a TLS client config as a `tls.Config` @@ -58,9 +59,23 @@ func KubeTLSClientConfigFromSecret(secret corev1.Secret, url string) (*tls.Confi // - caFile, for the CA certificate // // Secrets with no certificate, private key, AND CA cert are ignored. If only a -// certificate OR private key is found, an error is returned. +// certificate OR private key is found, an error is returned. The Secret type +// can be blank, Opaque or kubernetes.io/tls. func TLSClientConfigFromSecret(secret corev1.Secret, url string) (*tls.Config, *TLSBytes, error) { - return tlsClientConfigFromSecret(secret, url, false) + return tlsClientConfigFromSecret(secret, url, false, true) +} + +// LegacyTLSClientConfigFromSecret returns a TLS client config as a `tls.Config` +// object and in its bytes representation. The secret is expected to have the +// following keys: +// - keyFile, for the private key +// - certFile, for the certificate +// - caFile, for the CA certificate +// +// Secrets with no certificate, private key, AND CA cert are ignored. If only a +// certificate OR private key is found, an error is returned. +func LegacyTLSClientConfigFromSecret(secret corev1.Secret, url string) (*tls.Config, *TLSBytes, error) { + return tlsClientConfigFromSecret(secret, url, false, false) } // tlsClientConfigFromSecret attempts to construct and return a TLS client @@ -75,14 +90,20 @@ func TLSClientConfigFromSecret(secret corev1.Secret, url string) (*tls.Config, * // - ca.crt/caFile for the CA certificate // The keys should adhere to a single convention, i.e. a Secret with tls.key // and certFile is invalid. -func tlsClientConfigFromSecret(secret corev1.Secret, url string, kubernetesTLSKeys bool) (*tls.Config, *TLSBytes, error) { - // Only Secrets of type Opaque and TLS are allowed. We also allow Secrets with a blank - // type, to avoid having to specify the type of the Secret for every test case. - // Since a real Kubernetes Secret is of type Opaque by default, its safe to allow this. - switch secret.Type { - case corev1.SecretTypeOpaque, corev1.SecretTypeTLS, "": - default: - return nil, nil, fmt.Errorf("cannot use secret '%s' to construct TLS config: invalid secret type: '%s'", secret.Name, secret.Type) +// +// checkType is a boolean indicating whether to check the Secret type. If true +// and the Secret's type is not blank, Opaque or kubernetes.io/tls, then an +// error is returned. +func tlsClientConfigFromSecret(secret corev1.Secret, url string, kubernetesTLSKeys bool, checkType bool) (*tls.Config, *TLSBytes, error) { + if checkType { + // Only Secrets of type Opaque and TLS are allowed. We also allow Secrets with a blank + // type, to avoid having to specify the type of the Secret for every test case. + // Since a real Kubernetes Secret is of type Opaque by default, its safe to allow this. + switch secret.Type { + case corev1.SecretTypeOpaque, corev1.SecretTypeTLS, "": + default: + return nil, nil, fmt.Errorf("cannot use secret '%s' to construct TLS config: invalid secret type: '%s'", secret.Name, secret.Type) + } } var certBytes, keyBytes, caBytes []byte diff --git a/internal/tls/config_test.go b/internal/tls/config_test.go index 728b988b7..949142a07 100644 --- a/internal/tls/config_test.go +++ b/internal/tls/config_test.go @@ -35,13 +35,14 @@ func Test_tlsClientConfigFromSecret(t *testing.T) { tlsSecretFixture := validTlsSecret(t, false) tests := []struct { - name string - secret corev1.Secret - modify func(secret *corev1.Secret) - tlsKeys bool - url string - wantErr bool - wantNil bool + name string + secret corev1.Secret + modify func(secret *corev1.Secret) + tlsKeys bool + checkType bool + url string + wantErr bool + wantNil bool }{ { name: "tls.crt, tls.key and ca.crt", @@ -86,10 +87,20 @@ func Test_tlsClientConfigFromSecret(t *testing.T) { wantNil: true, }, { - name: "invalid secret type", - secret: corev1.Secret{Type: corev1.SecretTypeDockerConfigJson}, - wantErr: true, - wantNil: true, + name: "docker config secret with type checking enabled", + secret: tlsSecretFixture, + modify: func(secret *corev1.Secret) { secret.Type = corev1.SecretTypeDockerConfigJson }, + tlsKeys: false, + checkType: true, + wantErr: true, + wantNil: true, + }, + { + name: "docker config secret with type checking disabled", + secret: tlsSecretFixture, + modify: func(secret *corev1.Secret) { secret.Type = corev1.SecretTypeDockerConfigJson }, + tlsKeys: false, + url: "https://example.com", }, } for _, tt := range tests { @@ -100,7 +111,7 @@ func Test_tlsClientConfigFromSecret(t *testing.T) { tt.modify(secret) } - tlsConfig, _, err := tlsClientConfigFromSecret(*secret, tt.url, tt.tlsKeys) + tlsConfig, _, err := tlsClientConfigFromSecret(*secret, tt.url, tt.tlsKeys, tt.checkType) g.Expect(err != nil).To(Equal(tt.wantErr), fmt.Sprintf("expected error: %v, got: %v", tt.wantErr, err)) g.Expect(tlsConfig == nil).To(Equal(tt.wantNil)) if tt.url != "" {