Skip to content

Commit d516627

Browse files
aryan9600aryan9600
authored
Merge pull request #1220 from fluxcd/fix-helm-tls
helmrepo: fix Secret type check for TLS via `.spec.secretRef`
2 parents ec6877a + f787fc7 commit d516627

File tree

4 files changed

+123
-27
lines changed

4 files changed

+123
-27
lines changed

‎internal/controller/helmrepository_controller_test.go

Lines changed: 64 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -506,6 +506,38 @@ func TestHelmRepositoryReconciler_reconcileSource(t *testing.T) {
506506
t.Expect(artifact.Revision).ToNot(BeEmpty())
507507
},
508508
},
509+
{
510+
// Regression test for: https://github.com/fluxcd/source-controller/issues/1218
511+
name: "HTTPS with docker config secretRef and caFile key makes ArtifactOutdated=True",
512+
protocol: "https",
513+
server: options{
514+
publicKey: tlsPublicKey,
515+
privateKey: tlsPrivateKey,
516+
ca: tlsCA,
517+
},
518+
secret: &corev1.Secret{
519+
ObjectMeta: metav1.ObjectMeta{
520+
Name: "ca-file",
521+
},
522+
Data: map[string][]byte{
523+
"caFile": tlsCA,
524+
},
525+
Type: corev1.SecretTypeDockerConfigJson,
526+
},
527+
beforeFunc: func(t *WithT, obj *helmv1.HelmRepository, rev digest.Digest) {
528+
obj.Spec.SecretRef = &meta.LocalObjectReference{Name: "ca-file"}
529+
},
530+
want: sreconcile.ResultSuccess,
531+
assertConditions: []metav1.Condition{
532+
*conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingReason, "building artifact: new index revision"),
533+
*conditions.UnknownCondition(meta.ReadyCondition, meta.ProgressingReason, "building artifact: new index revision"),
534+
},
535+
afterFunc: func(t *WithT, obj *helmv1.HelmRepository, artifact sourcev1.Artifact, chartRepo *repository.ChartRepository) {
536+
t.Expect(chartRepo.Path).ToNot(BeEmpty())
537+
t.Expect(chartRepo.Index).ToNot(BeNil())
538+
t.Expect(artifact.Revision).ToNot(BeEmpty())
539+
},
540+
},
509541
{
510542
name: "HTTP without secretRef makes ArtifactOutdated=True",
511543
protocol: "http",
@@ -550,6 +582,38 @@ func TestHelmRepositoryReconciler_reconcileSource(t *testing.T) {
550582
t.Expect(artifact.Revision).ToNot(BeEmpty())
551583
},
552584
},
585+
{
586+
// Regression test for: https://github.com/fluxcd/source-controller/issues/1218
587+
name: "HTTP with docker config secretRef sets Reconciling=True",
588+
protocol: "http",
589+
server: options{
590+
username: "git",
591+
password: "1234",
592+
},
593+
secret: &corev1.Secret{
594+
ObjectMeta: metav1.ObjectMeta{
595+
Name: "basic-auth",
596+
},
597+
Data: map[string][]byte{
598+
"username": []byte("git"),
599+
"password": []byte("1234"),
600+
},
601+
Type: corev1.SecretTypeDockerConfigJson,
602+
},
603+
beforeFunc: func(t *WithT, obj *helmv1.HelmRepository, rev digest.Digest) {
604+
obj.Spec.SecretRef = &meta.LocalObjectReference{Name: "basic-auth"}
605+
},
606+
want: sreconcile.ResultSuccess,
607+
assertConditions: []metav1.Condition{
608+
*conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingReason, "building artifact: new index revision"),
609+
*conditions.UnknownCondition(meta.ReadyCondition, meta.ProgressingReason, "building artifact: new index revision"),
610+
},
611+
afterFunc: func(t *WithT, obj *helmv1.HelmRepository, artifact sourcev1.Artifact, chartRepo *repository.ChartRepository) {
612+
t.Expect(chartRepo.Path).ToNot(BeEmpty())
613+
t.Expect(chartRepo.Index).ToNot(BeNil())
614+
t.Expect(artifact.Revision).ToNot(BeEmpty())
615+
},
616+
},
553617
{
554618
name: "HTTPS with invalid CAFile in certSecretRef makes FetchFailed=True and returns error",
555619
protocol: "https",

‎internal/helm/getter/client_opts.go

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -115,10 +115,10 @@ func GetClientOpts(ctx context.Context, c client.Client, obj *helmv1.HelmReposit
115115
}
116116
hrOpts.GetterOpts = append(hrOpts.GetterOpts, opts...)
117117

118-
// If the TLS config is nil, i.e. one couldn't be constructed using `.spec.certSecretRef`
119-
// then try to use `.spec.secretRef`.
118+
// If the TLS config is nil, i.e. one couldn't be constructed using
119+
// `.spec.certSecretRef`, then try to use `.spec.secretRef`.
120120
if hrOpts.TlsConfig == nil && !ociRepo {
121-
hrOpts.TlsConfig, tlsBytes, err = stls.TLSClientConfigFromSecret(*authSecret, url)
121+
hrOpts.TlsConfig, tlsBytes, err = stls.LegacyTLSClientConfigFromSecret(*authSecret, url)
122122
if err != nil {
123123
return nil, "", fmt.Errorf("failed to construct Helm client's TLS config: %w", err)
124124
}

‎internal/tls/config.go

Lines changed: 33 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -45,9 +45,10 @@ type TLSBytes struct {
4545
// - ca.crt, for the CA certificate
4646
//
4747
// Secrets with no certificate, private key, AND CA cert are ignored. If only a
48-
// certificate OR private key is found, an error is returned.
48+
// certificate OR private key is found, an error is returned. The Secret type
49+
// can be blank, Opaque or kubernetes.io/tls.
4950
func KubeTLSClientConfigFromSecret(secret corev1.Secret, url string) (*tls.Config, *TLSBytes, error) {
50-
return tlsClientConfigFromSecret(secret, url, true)
51+
return tlsClientConfigFromSecret(secret, url, true, true)
5152
}
5253

5354
// TLSClientConfigFromSecret returns a TLS client config as a `tls.Config`
@@ -58,9 +59,23 @@ func KubeTLSClientConfigFromSecret(secret corev1.Secret, url string) (*tls.Confi
5859
// - caFile, for the CA certificate
5960
//
6061
// Secrets with no certificate, private key, AND CA cert are ignored. If only a
61-
// certificate OR private key is found, an error is returned.
62+
// certificate OR private key is found, an error is returned. The Secret type
63+
// can be blank, Opaque or kubernetes.io/tls.
6264
func TLSClientConfigFromSecret(secret corev1.Secret, url string) (*tls.Config, *TLSBytes, error) {
63-
return tlsClientConfigFromSecret(secret, url, false)
65+
return tlsClientConfigFromSecret(secret, url, false, true)
66+
}
67+
68+
// LegacyTLSClientConfigFromSecret returns a TLS client config as a `tls.Config`
69+
// object and in its bytes representation. The secret is expected to have the
70+
// following keys:
71+
// - keyFile, for the private key
72+
// - certFile, for the certificate
73+
// - caFile, for the CA certificate
74+
//
75+
// Secrets with no certificate, private key, AND CA cert are ignored. If only a
76+
// certificate OR private key is found, an error is returned.
77+
func LegacyTLSClientConfigFromSecret(secret corev1.Secret, url string) (*tls.Config, *TLSBytes, error) {
78+
return tlsClientConfigFromSecret(secret, url, false, false)
6479
}
6580

6681
// tlsClientConfigFromSecret attempts to construct and return a TLS client
@@ -75,14 +90,20 @@ func TLSClientConfigFromSecret(secret corev1.Secret, url string) (*tls.Config, *
7590
// - ca.crt/caFile for the CA certificate
7691
// The keys should adhere to a single convention, i.e. a Secret with tls.key
7792
// and certFile is invalid.
78-
func tlsClientConfigFromSecret(secret corev1.Secret, url string, kubernetesTLSKeys bool) (*tls.Config, *TLSBytes, error) {
79-
// Only Secrets of type Opaque and TLS are allowed. We also allow Secrets with a blank
80-
// type, to avoid having to specify the type of the Secret for every test case.
81-
// Since a real Kubernetes Secret is of type Opaque by default, its safe to allow this.
82-
switch secret.Type {
83-
case corev1.SecretTypeOpaque, corev1.SecretTypeTLS, "":
84-
default:
85-
return nil, nil, fmt.Errorf("cannot use secret '%s' to construct TLS config: invalid secret type: '%s'", secret.Name, secret.Type)
93+
//
94+
// checkType is a boolean indicating whether to check the Secret type. If true
95+
// and the Secret's type is not blank, Opaque or kubernetes.io/tls, then an
96+
// error is returned.
97+
func tlsClientConfigFromSecret(secret corev1.Secret, url string, kubernetesTLSKeys bool, checkType bool) (*tls.Config, *TLSBytes, error) {
98+
if checkType {
99+
// Only Secrets of type Opaque and TLS are allowed. We also allow Secrets with a blank
100+
// type, to avoid having to specify the type of the Secret for every test case.
101+
// Since a real Kubernetes Secret is of type Opaque by default, its safe to allow this.
102+
switch secret.Type {
103+
case corev1.SecretTypeOpaque, corev1.SecretTypeTLS, "":
104+
default:
105+
return nil, nil, fmt.Errorf("cannot use secret '%s' to construct TLS config: invalid secret type: '%s'", secret.Name, secret.Type)
106+
}
86107
}
87108

88109
var certBytes, keyBytes, caBytes []byte

‎internal/tls/config_test.go

Lines changed: 23 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -35,13 +35,14 @@ func Test_tlsClientConfigFromSecret(t *testing.T) {
3535
tlsSecretFixture := validTlsSecret(t, false)
3636

3737
tests := []struct {
38-
name string
39-
secret corev1.Secret
40-
modify func(secret *corev1.Secret)
41-
tlsKeys bool
42-
url string
43-
wantErr bool
44-
wantNil bool
38+
name string
39+
secret corev1.Secret
40+
modify func(secret *corev1.Secret)
41+
tlsKeys bool
42+
checkType bool
43+
url string
44+
wantErr bool
45+
wantNil bool
4546
}{
4647
{
4748
name: "tls.crt, tls.key and ca.crt",
@@ -86,10 +87,20 @@ func Test_tlsClientConfigFromSecret(t *testing.T) {
8687
wantNil: true,
8788
},
8889
{
89-
name: "invalid secret type",
90-
secret: corev1.Secret{Type: corev1.SecretTypeDockerConfigJson},
91-
wantErr: true,
92-
wantNil: true,
90+
name: "docker config secret with type checking enabled",
91+
secret: tlsSecretFixture,
92+
modify: func(secret *corev1.Secret) { secret.Type = corev1.SecretTypeDockerConfigJson },
93+
tlsKeys: false,
94+
checkType: true,
95+
wantErr: true,
96+
wantNil: true,
97+
},
98+
{
99+
name: "docker config secret with type checking disabled",
100+
secret: tlsSecretFixture,
101+
modify: func(secret *corev1.Secret) { secret.Type = corev1.SecretTypeDockerConfigJson },
102+
tlsKeys: false,
103+
url: "https://example.com",
93104
},
94105
}
95106
for _, tt := range tests {
@@ -100,7 +111,7 @@ func Test_tlsClientConfigFromSecret(t *testing.T) {
100111
tt.modify(secret)
101112
}
102113

103-
tlsConfig, _, err := tlsClientConfigFromSecret(*secret, tt.url, tt.tlsKeys)
114+
tlsConfig, _, err := tlsClientConfigFromSecret(*secret, tt.url, tt.tlsKeys, tt.checkType)
104115
g.Expect(err != nil).To(Equal(tt.wantErr), fmt.Sprintf("expected error: %v, got: %v", tt.wantErr, err))
105116
g.Expect(tlsConfig == nil).To(Equal(tt.wantNil))
106117
if tt.url != "" {

0 commit comments

Comments
 (0)