Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

helmrepo: fix Secret type check for TLS via .spec.secretRef #1220

Merged
merged 1 commit into from
Sep 7, 2023
Merged

helmrepo: fix Secret type check for TLS via .spec.secretRef #1220

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
64 changes: 64 additions & 0 deletions internal/controller/helmrepository_controller_test.go
View file
Open in desktop
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",
aryan9600 marked this conversation as resolved.
Show resolved Hide resolved
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
Open in desktop
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
Open in desktop
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
Open in desktop
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