Skip to content

Commit

Permalink
helmrepo: fix Secret type check for TLS via .spec.secretRef
Browse files Browse the repository at this point in the history 
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 <[email protected]>
  • Loading branch information
@aryan9600
aryan9600 committed Sep 5, 2023
1 parent ec6877a commit f787fc7
Show file tree
Hide file tree
Showing 4 changed files with 123 additions and 27 deletions.
64 changes: 64 additions & 0 deletions internal/controller/helmrepository_controller_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down Expand Up @@ -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",
Expand Down
6 changes: 3 additions & 3 deletions internal/helm/getter/client_opts.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
Expand Down
45 changes: 33 additions & 12 deletions internal/tls/config.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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`
Expand All @@ -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
Expand All @@ -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
Expand Down
35 changes: 23 additions & 12 deletions internal/tls/config_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down Expand Up @@ -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 {
Expand All @@ -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 != "" {
Expand Down

0 comments on commit f787fc7

Please sign in to comment.