From 4bd6bcc9e9e23e4ed462bf192b6e368aad286336 Mon Sep 17 00:00:00 2001 From: Sanskar Jaiswal Date: Tue, 8 Aug 2023 17:29:59 +0530 Subject: [PATCH 1/3] helmrepo: adopt Kubernetes TLS secrets for `.spec.certSecretRef` Adopt Kubernetes TLS secrets API to check for TLS data in the Secret referred to by `.spec.certSecretRef`, i.e. check for keys `tls.crt` and `tls.key` for the certificate and private key. Use `ca.crt` for the CA certificate. Signed-off-by: Sanskar Jaiswal --- api/v1beta2/helmrepository_types.go | 19 +- ...ce.toolkit.fluxcd.io_helmrepositories.yaml | 13 +- docs/api/v1beta2/source.md | 36 +++- docs/spec/v1beta2/helmrepositories.md | 38 ++-- .../controller/helmchart_controller_test.go | 22 +-- .../helmrepository_controller_oci_test.go | 10 +- .../helmrepository_controller_test.go | 75 +++++++- internal/helm/getter/client_opts.go | 79 +------- internal/helm/getter/client_opts_test.go | 102 +--------- internal/tls/config.go | 140 ++++++++++++++ internal/tls/config_test.go | 178 ++++++++++++++++++ 11 files changed, 487 insertions(+), 225 deletions(-) create mode 100644 internal/tls/config.go create mode 100644 internal/tls/config_test.go diff --git a/api/v1beta2/helmrepository_types.go b/api/v1beta2/helmrepository_types.go index 4da992aba..e1df71568 100644 --- a/api/v1beta2/helmrepository_types.go +++ b/api/v1beta2/helmrepository_types.go @@ -56,10 +56,21 @@ type HelmRepositorySpec struct { // +optional SecretRef *meta.LocalObjectReference `json:"secretRef,omitempty"` - // CertSecretRef specifies the Secret containing the TLS authentication - // data. The secret must contain a 'certFile' and 'keyFile', and/or 'caFile' - // fields. It takes precedence over the values specified in the Secret - // referred to by `.spec.secretRef`. + // CertSecretRef can be given the name of a Secret containing + // either or both of + // + // - a PEM-encoded client certificate (`tls.crt`) and private + // key (`tls.key`); + // - a PEM-encoded CA certificate (`ca.crt`) + // + // and whichever are supplied, will be used for connecting to the + // registry. The client cert and key are useful if you are + // authenticating with a certificate; the CA cert is useful if + // you are using a self-signed server certificate. The Secret must + // be of type `Opaque` or `kubernetes.io/tls`. + // + // It takes precedence over the values specified in the Secret referred + // to by `.spec.secretRef`. // +optional CertSecretRef *meta.LocalObjectReference `json:"certSecretRef,omitempty"` diff --git a/config/crd/bases/source.toolkit.fluxcd.io_helmrepositories.yaml b/config/crd/bases/source.toolkit.fluxcd.io_helmrepositories.yaml index c9a6b3fc7..6de6911d8 100644 --- a/config/crd/bases/source.toolkit.fluxcd.io_helmrepositories.yaml +++ b/config/crd/bases/source.toolkit.fluxcd.io_helmrepositories.yaml @@ -297,10 +297,15 @@ spec: - namespaceSelectors type: object certSecretRef: - description: CertSecretRef specifies the Secret containing the TLS - authentication data. The secret must contain a 'certFile' and 'keyFile', - and/or 'caFile' fields. It takes precedence over the values specified - in the Secret referred to by `.spec.secretRef`. + description: "CertSecretRef can be given the name of a Secret containing + either or both of \n - a PEM-encoded client certificate (`tls.crt`) + and private key (`tls.key`); - a PEM-encoded CA certificate (`ca.crt`) + \n and whichever are supplied, will be used for connecting to the + registry. The client cert and key are useful if you are authenticating + with a certificate; the CA cert is useful if you are using a self-signed + server certificate. The Secret must be of type `Opaque` or `kubernetes.io/tls`. + \n It takes precedence over the values specified in the Secret referred + to by `.spec.secretRef`." properties: name: description: Name of the referent. diff --git a/docs/api/v1beta2/source.md b/docs/api/v1beta2/source.md index be0c454ed..73899644f 100644 --- a/docs/api/v1beta2/source.md +++ b/docs/api/v1beta2/source.md @@ -811,10 +811,20 @@ github.com/fluxcd/pkg/apis/meta.LocalObjectReference (Optional) -

CertSecretRef specifies the Secret containing the TLS authentication -data. The secret must contain a ‘certFile’ and ‘keyFile’, and/or ‘caFile’ -fields. It takes precedence over the values specified in the Secret -referred to by .spec.secretRef.

+

CertSecretRef can be given the name of a Secret containing +either or both of

+ +

and whichever are supplied, will be used for connecting to the +registry. The client cert and key are useful if you are +authenticating with a certificate; the CA cert is useful if +you are using a self-signed server certificate. The Secret must +be of type Opaque or kubernetes.io/tls.

+

It takes precedence over the values specified in the Secret referred +to by .spec.secretRef.

@@ -2503,10 +2513,20 @@ github.com/fluxcd/pkg/apis/meta.LocalObjectReference (Optional) -

CertSecretRef specifies the Secret containing the TLS authentication -data. The secret must contain a ‘certFile’ and ‘keyFile’, and/or ‘caFile’ -fields. It takes precedence over the values specified in the Secret -referred to by .spec.secretRef.

+

CertSecretRef can be given the name of a Secret containing +either or both of

+ +

and whichever are supplied, will be used for connecting to the +registry. The client cert and key are useful if you are +authenticating with a certificate; the CA cert is useful if +you are using a self-signed server certificate. The Secret must +be of type Opaque or kubernetes.io/tls.

+

It takes precedence over the values specified in the Secret referred +to by .spec.secretRef.

diff --git a/docs/spec/v1beta2/helmrepositories.md b/docs/spec/v1beta2/helmrepositories.md index dc42624bb..9e4c7bcc2 100644 --- a/docs/spec/v1beta2/helmrepositories.md +++ b/docs/spec/v1beta2/helmrepositories.md @@ -467,32 +467,33 @@ flux create secret oci ghcr-auth \ --password=${GITHUB_PAT} ``` -**Note:** Support for specifying TLS authentication data using this API has been +**Warning:** Support for specifying TLS authentication data using this API has been deprecated. Please use [`.spec.certSecretRef`](#cert-secret-reference) instead. If the controller uses the secret specfied by this field to configure TLS, then a deprecation warning will be logged. ### Cert secret reference -`.spec.certSecretRef.name` is an optional field to specify a secret containing TLS -certificate data. The secret can contain the following keys: +`.spec.certSecretRef.name` is an optional field to specify a secret containing +TLS certificate data. The secret can contain the following keys: -* `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 -the other will lead to an error. -* `caFile`, to specify the CA certificate used to verify the server, which is required -if the server is using a self-signed certificate. +* `tls.crt` and `tls.key`, to specify the client certificate and private key used +for TLS client authentication. These must be used in conjunction, i.e. +specifying one without the other will lead to an error. +* `ca.crt`, to specify the CA certificate used to verify the server, which is +required if the server is using a self-signed certificate. -If the server is using a self-signed certificate and has TLS client authentication enabled, -all three values are required. +If the server is using a self-signed certificate and has TLS client +authentication enabled, all three values are required. -All the files in the secret are expected to be [PEM-encoded][pem-encoding]. Assuming you have -three files; `client.key`, `client.crt` and `ca.crt` for the client private key, client -certificate and the CA certificate respectively, you can generate the required secret using -the `flux creat secret helm` command: +The Secret should be of type `Opaque` or `kubernetes.io/tls`. All the files in +the Secret are expected to be [PEM-encoded][pem-encoding]. Assuming you have +three files; `client.key`, `client.crt` and `ca.crt` for the client private key, +client certificate and the CA certificate respectively, you can generate the +required Secret using the `flux create secret tls` command: ```sh -flux create secret helm tls --key-file=client.key --cert-file=client.crt --ca-file=ca.crt +flux create secret tls --tls-key-file=client.key --tls-crt-file=client.crt --ca-crt-file=ca.crt ``` Example usage: @@ -515,11 +516,12 @@ kind: Secret metadata: name: example-tls namespace: default +type: kubernetes.io/tls # or Opaque data: - certFile: - keyFile: + tls.crt: + tls.key: # NOTE: Can be supplied without the above values - caFile: + ca.crt: ``` ### Pass credentials diff --git a/internal/controller/helmchart_controller_test.go b/internal/controller/helmchart_controller_test.go index c0ad94380..9d45271dc 100644 --- a/internal/controller/helmchart_controller_test.go +++ b/internal/controller/helmchart_controller_test.go @@ -2248,7 +2248,7 @@ func TestHelmChartReconciler_reconcileSourceFromOCI_authStrategy(t *testing.T) { registryOpts registryOptions secretOpts secretOptions secret *corev1.Secret - certsecret *corev1.Secret + certSecret *corev1.Secret insecure bool provider string providerImg string @@ -2363,16 +2363,16 @@ func TestHelmChartReconciler_reconcileSourceFromOCI_authStrategy(t *testing.T) { Type: corev1.SecretTypeDockerConfigJson, Data: map[string][]byte{}, }, - certsecret: &corev1.Secret{ + certSecret: &corev1.Secret{ ObjectMeta: metav1.ObjectMeta{ Name: "certs-secretref", }, Data: map[string][]byte{ - "caFile": []byte("invalid caFile"), + "ca.crt": []byte("invalid caFile"), }, }, assertConditions: []metav1.Condition{ - *conditions.TrueCondition(sourcev1.FetchFailedCondition, "Unknown", "unknown build error: failed to construct Helm client's TLS config: cannot append certificate into certificate pool: invalid caFile"), + *conditions.TrueCondition(sourcev1.FetchFailedCondition, "Unknown", "unknown build error: failed to construct Helm client's TLS config: cannot append certificate into certificate pool: invalid CA certificate"), }, }, { @@ -2393,14 +2393,14 @@ func TestHelmChartReconciler_reconcileSourceFromOCI_authStrategy(t *testing.T) { Type: corev1.SecretTypeDockerConfigJson, Data: map[string][]byte{}, }, - certsecret: &corev1.Secret{ + certSecret: &corev1.Secret{ ObjectMeta: metav1.ObjectMeta{ Name: "certs-secretref", }, Data: map[string][]byte{ - "caFile": tlsCA, - "certFile": clientPublicKey, - "keyFile": clientPrivateKey, + "ca.crt": tlsCA, + "tls.crt": clientPublicKey, + "tls.key": clientPrivateKey, }, }, assertConditions: []metav1.Condition{ @@ -2472,11 +2472,11 @@ func TestHelmChartReconciler_reconcileSourceFromOCI_authStrategy(t *testing.T) { clientBuilder.WithObjects(tt.secret) } - if tt.certsecret != nil { + if tt.certSecret != nil { repo.Spec.CertSecretRef = &meta.LocalObjectReference{ - Name: tt.certsecret.Name, + Name: tt.certSecret.Name, } - clientBuilder.WithObjects(tt.certsecret) + clientBuilder.WithObjects(tt.certSecret) } clientBuilder.WithObjects(repo) diff --git a/internal/controller/helmrepository_controller_oci_test.go b/internal/controller/helmrepository_controller_oci_test.go index d1252e709..2a33115c7 100644 --- a/internal/controller/helmrepository_controller_oci_test.go +++ b/internal/controller/helmrepository_controller_oci_test.go @@ -325,12 +325,12 @@ func TestHelmRepositoryOCIReconciler_authStrategy(t *testing.T) { Name: "certs-secretref", }, Data: map[string][]byte{ - "caFile": []byte("invalid caFile"), + "ca.crt": []byte("invalid caFile"), }, }, assertConditions: []metav1.Condition{ *conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingWithRetryReason, "processing object: new generation 0 -> 1"), - *conditions.FalseCondition(meta.ReadyCondition, sourcev1.AuthenticationFailedReason, "cannot append certificate into certificate pool: invalid caFile"), + *conditions.FalseCondition(meta.ReadyCondition, sourcev1.AuthenticationFailedReason, "cannot append certificate into certificate pool: invalid CA certificate"), }, }, { @@ -356,9 +356,9 @@ func TestHelmRepositoryOCIReconciler_authStrategy(t *testing.T) { Name: "certs-secretref", }, Data: map[string][]byte{ - "caFile": tlsCA, - "certFile": clientPublicKey, - "keyFile": clientPrivateKey, + "ca.crt": tlsCA, + "tls.crt": clientPublicKey, + "tls.key": clientPrivateKey, }, }, assertConditions: []metav1.Condition{ diff --git a/internal/controller/helmrepository_controller_test.go b/internal/controller/helmrepository_controller_test.go index 370cac0ed..ae0273f1f 100644 --- a/internal/controller/helmrepository_controller_test.go +++ b/internal/controller/helmrepository_controller_test.go @@ -56,6 +56,7 @@ import ( "github.com/fluxcd/source-controller/internal/helm/repository" sreconcile "github.com/fluxcd/source-controller/internal/reconcile" "github.com/fluxcd/source-controller/internal/reconcile/summarize" + stls "github.com/fluxcd/source-controller/internal/tls" ) func TestHelmRepositoryReconciler_deleteBeforeFinalizer(t *testing.T) { @@ -434,16 +435,76 @@ func TestHelmRepositoryReconciler_reconcileSource(t *testing.T) { Name: "ca-file", }, Data: map[string][]byte{ - "caFile": tlsCA, + "ca.crt": tlsCA, + }, + }, + beforeFunc: func(t *WithT, obj *helmv1.HelmRepository, rev digest.Digest) { + obj.Spec.CertSecretRef = &meta.LocalObjectReference{Name: "ca-file"} + }, + 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"), + }, + }, + { + name: "HTTPS with certSecretRef 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{ + "ca.crt": tlsCA, }, }, beforeFunc: func(t *WithT, obj *helmv1.HelmRepository, rev digest.Digest) { obj.Spec.CertSecretRef = &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: "HTTPS with 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, + }, + }, + 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", @@ -502,7 +563,7 @@ func TestHelmRepositoryReconciler_reconcileSource(t *testing.T) { Name: "invalid-ca", }, Data: map[string][]byte{ - "caFile": []byte("invalid"), + "ca.crt": []byte("invalid"), }, }, beforeFunc: func(t *WithT, obj *helmv1.HelmRepository, rev digest.Digest) { @@ -512,7 +573,7 @@ func TestHelmRepositoryReconciler_reconcileSource(t *testing.T) { }, wantErr: true, assertConditions: []metav1.Condition{ - *conditions.TrueCondition(sourcev1.FetchFailedCondition, sourcev1.AuthenticationFailedReason, "cannot append certificate into certificate pool: invalid caFile"), + *conditions.TrueCondition(sourcev1.FetchFailedCondition, sourcev1.AuthenticationFailedReason, "cannot append certificate into certificate pool: invalid CA certificate"), *conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingReason, "foo"), *conditions.UnknownCondition(meta.ReadyCondition, "foo", "bar"), }, @@ -769,10 +830,16 @@ func TestHelmRepositoryReconciler_reconcileSource(t *testing.T) { if tt.url != "" { repoURL = tt.url } - tlsConf, _, serr = getter.TLSClientConfigFromSecret(*secret, repoURL) + tlsConf, _, serr = stls.KubeTLSClientConfigFromSecret(*secret, repoURL) if serr != nil { validSecret = false } + if tlsConf == nil { + tlsConf, _, serr = stls.TLSClientConfigFromSecret(*secret, repoURL) + if serr != nil { + validSecret = false + } + } newChartRepo, err = repository.NewChartRepository(obj.Spec.URL, "", testGetters, tlsConf, getterOpts...) } else { newChartRepo, err = repository.NewChartRepository(obj.Spec.URL, "", testGetters, nil) diff --git a/internal/helm/getter/client_opts.go b/internal/helm/getter/client_opts.go index 58248d5b6..4e77f290a 100644 --- a/internal/helm/getter/client_opts.go +++ b/internal/helm/getter/client_opts.go @@ -19,10 +19,8 @@ package getter import ( "context" "crypto/tls" - "crypto/x509" "errors" "fmt" - "net/url" "os" "path" @@ -37,6 +35,7 @@ import ( helmv1 "github.com/fluxcd/source-controller/api/v1beta2" "github.com/fluxcd/source-controller/internal/helm/registry" soci "github.com/fluxcd/source-controller/internal/oci" + stls "github.com/fluxcd/source-controller/internal/tls" ) const ( @@ -47,16 +46,6 @@ const ( var ErrDeprecatedTLSConfig = errors.New("TLS configured in a deprecated manner") -// 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 -} - // ClientOpts contains the various options to use while constructing // a Helm repository client. type ClientOpts struct { @@ -91,7 +80,7 @@ func GetClientOpts(ctx context.Context, c client.Client, obj *helmv1.HelmReposit var ( certSecret *corev1.Secret - tlsBytes *TLSBytes + tlsBytes *stls.TLSBytes certFile string keyFile string caFile string @@ -105,7 +94,7 @@ func GetClientOpts(ctx context.Context, c client.Client, obj *helmv1.HelmReposit return nil, "", fmt.Errorf("failed to get TLS authentication secret '%s/%s': %w", obj.GetNamespace(), obj.Spec.CertSecretRef.Name, err) } - hrOpts.TlsConfig, tlsBytes, err = TLSClientConfigFromSecret(*certSecret, url) + hrOpts.TlsConfig, tlsBytes, err = stls.KubeTLSClientConfigFromSecret(*certSecret, url) if err != nil { return nil, "", fmt.Errorf("failed to construct Helm client's TLS config: %w", err) } @@ -128,8 +117,8 @@ func GetClientOpts(ctx context.Context, c client.Client, obj *helmv1.HelmReposit // 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 { - hrOpts.TlsConfig, tlsBytes, err = TLSClientConfigFromSecret(*authSecret, url) + if hrOpts.TlsConfig == nil && !ociRepo { + hrOpts.TlsConfig, tlsBytes, err = stls.TLSClientConfigFromSecret(*authSecret, url) if err != nil { return nil, "", fmt.Errorf("failed to construct Helm client's TLS config: %w", err) } @@ -162,7 +151,7 @@ func GetClientOpts(ctx context.Context, c client.Client, obj *helmv1.HelmReposit if err != nil { return nil, "", fmt.Errorf("cannot create temporary directory: %w", err) } - certFile, keyFile, caFile, err = StoreTLSCertificateFiles(tlsBytes, dir) + certFile, keyFile, caFile, err = storeTLSCertificateFiles(tlsBytes, dir) if err != nil { return nil, "", fmt.Errorf("cannot write certs files to path: %w", err) } @@ -198,60 +187,8 @@ func fetchSecret(ctx context.Context, c client.Client, name, namespace string) ( return &secret, nil } -// TLSClientConfigFromSecret attempts to construct a TLS client config -// for the given v1.Secret. It returns the TLS client config or an error. -// -// 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, *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, nil - case (len(certBytes) > 0 && len(keyBytes) == 0) || (len(keyBytes) > 0 && len(certBytes) == 0): - return nil, nil, fmt.Errorf("invalid '%s' secret data: fields 'certFile' and 'keyFile' require each other's presence", - secret.Name) - } - - tlsConf := &tls.Config{} - if len(certBytes) > 0 && len(keyBytes) > 0 { - cert, err := tls.X509KeyPair(certBytes, keyBytes) - if err != nil { - return nil, nil, err - } - tlsConf.Certificates = append(tlsConf.Certificates, cert) - } - - if len(caBytes) > 0 { - cp, err := x509.SystemCertPool() - if err != nil { - return nil, nil, fmt.Errorf("cannot retrieve system certificate pool: %w", err) - } - if !cp.AppendCertsFromPEM(caBytes) { - return nil, nil, fmt.Errorf("cannot append certificate into certificate pool: invalid caFile") - } - - tlsConf.RootCAs = cp - } - - tlsConf.BuildNameToCertificate() - - u, err := url.Parse(repositoryUrl) - if err != nil { - return nil, nil, fmt.Errorf("cannot parse repository URL: %w", err) - } - - tlsConf.ServerName = u.Hostname() - - return tlsConf, &TLSBytes{ - CertBytes: certBytes, - KeyBytes: keyBytes, - CABytes: caBytes, - }, nil -} - -// StoreTLSCertificateFiles writes the certs files to the given path and returns the files paths. -func StoreTLSCertificateFiles(tlsBytes *TLSBytes, path string) (string, string, string, error) { +// storeTLSCertificateFiles writes the certs files to the given path and returns the files paths. +func storeTLSCertificateFiles(tlsBytes *stls.TLSBytes, path string) (string, string, string, error) { var ( certFile string keyFile string diff --git a/internal/helm/getter/client_opts_test.go b/internal/helm/getter/client_opts_test.go index 6b031851d..91bcd32f8 100644 --- a/internal/helm/getter/client_opts_test.go +++ b/internal/helm/getter/client_opts_test.go @@ -18,11 +18,6 @@ package getter import ( "context" - "crypto/rand" - "crypto/rsa" - "crypto/x509" - "encoding/pem" - "math/big" "os" "testing" "time" @@ -58,7 +53,7 @@ func TestGetClientOpts(t *testing.T) { Name: "ca-file", }, Data: map[string][]byte{ - "caFile": tlsCA, + "ca.crt": tlsCA, }, }, authSecret: &corev1.Secret{ @@ -160,42 +155,6 @@ func TestGetClientOpts(t *testing.T) { } } -func Test_tlsClientConfigFromSecret(t *testing.T) { - tlsSecretFixture := validTlsSecret(t) - - tests := []struct { - name string - secret corev1.Secret - modify func(secret *corev1.Secret) - wantErr bool - wantNil bool - }{ - {"certFile, keyFile and caFile", tlsSecretFixture, nil, false, false}, - {"without certFile", tlsSecretFixture, func(s *corev1.Secret) { delete(s.Data, "certFile") }, true, true}, - {"without keyFile", tlsSecretFixture, func(s *corev1.Secret) { delete(s.Data, "keyFile") }, true, true}, - {"without caFile", tlsSecretFixture, func(s *corev1.Secret) { delete(s.Data, "caFile") }, false, false}, - {"empty", corev1.Secret{}, nil, false, true}, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - secret := tt.secret.DeepCopy() - if tt.modify != nil { - tt.modify(secret) - } - - got, _, err := TLSClientConfigFromSecret(*secret, "") - if (err != nil) != tt.wantErr { - t.Errorf("TLSClientConfigFromSecret() error = %v, wantErr %v", err, tt.wantErr) - return - } - if tt.wantNil && got != nil { - t.Error("TLSClientConfigFromSecret() != nil") - return - } - }) - } -} - func TestGetClientOpts_registryTLSLoginOption(t *testing.T) { tlsCA, err := os.ReadFile("../../controller/testdata/certs/ca.pem") if err != nil { @@ -215,7 +174,7 @@ func TestGetClientOpts_registryTLSLoginOption(t *testing.T) { Name: "ca-file", }, Data: map[string][]byte{ - "caFile": tlsCA, + "ca.crt": tlsCA, }, }, authSecret: &corev1.Secret{ @@ -307,60 +266,3 @@ func TestGetClientOpts_registryTLSLoginOption(t *testing.T) { }) } } - -// 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 { - key, err := rsa.GenerateKey(rand.Reader, 2048) - if err != nil { - t.Fatal("Private key cannot be created.", err.Error()) - } - - certTemplate := x509.Certificate{ - SerialNumber: big.NewInt(1337), - } - cert, err := x509.CreateCertificate(rand.Reader, &certTemplate, &certTemplate, &key.PublicKey, key) - if err != nil { - t.Fatal("Certificate cannot be created.", err.Error()) - } - - ca := &x509.Certificate{ - SerialNumber: big.NewInt(7331), - IsCA: true, - ExtKeyUsage: []x509.ExtKeyUsage{x509.ExtKeyUsageClientAuth, x509.ExtKeyUsageServerAuth}, - KeyUsage: x509.KeyUsageDigitalSignature | x509.KeyUsageCertSign, - } - - caPrivKey, err := rsa.GenerateKey(rand.Reader, 4096) - if err != nil { - t.Fatal("CA private key cannot be created.", err.Error()) - } - - caBytes, err := x509.CreateCertificate(rand.Reader, ca, ca, &caPrivKey.PublicKey, caPrivKey) - if err != nil { - t.Fatal("CA certificate cannot be created.", err.Error()) - } - - keyPem := pem.EncodeToMemory(&pem.Block{ - Type: "RSA PRIVATE KEY", - Bytes: x509.MarshalPKCS1PrivateKey(key), - }) - - certPem := pem.EncodeToMemory(&pem.Block{ - Type: "CERTIFICATE", - Bytes: cert, - }) - - caPem := pem.EncodeToMemory(&pem.Block{ - Type: "CERTIFICATE", - Bytes: caBytes, - }) - - return corev1.Secret{ - Data: map[string][]byte{ - "certFile": []byte(certPem), - "keyFile": []byte(keyPem), - "caFile": []byte(caPem), - }, - } -} diff --git a/internal/tls/config.go b/internal/tls/config.go new file mode 100644 index 000000000..9d9eee9f7 --- /dev/null +++ b/internal/tls/config.go @@ -0,0 +1,140 @@ +/* +Copyright 2023 The Flux authors + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package tls + +import ( + "crypto/tls" + "crypto/x509" + "fmt" + neturl "net/url" + + corev1 "k8s.io/api/core/v1" +) + +const CACrtKey = "ca.crt" + +// 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 +} + +// KubeTLSClientConfigFromSecret returns a TLS client config as a `tls.Config` +// object and in its bytes representation. The secret is expected to have the +// following keys: +// - tls.key, for the private key +// - tls.crt, for the certificate +// - 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. +func KubeTLSClientConfigFromSecret(secret corev1.Secret, url string) (*tls.Config, *TLSBytes, error) { + return tlsClientConfigFromSecret(secret, url, true) +} + +// TLSClientConfigFromSecret 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 TLSClientConfigFromSecret(secret corev1.Secret, url string) (*tls.Config, *TLSBytes, error) { + return tlsClientConfigFromSecret(secret, url, false) +} + +// tlsClientConfigFromSecret attempts to construct and return a TLS client +// config from the given Secret. If the Secret does not contain any TLS +// data, it returns nil. +// +// kubernetesTLSKeys is a boolean indicating whether to check the Secret +// for keys expected to be present in a Kubernetes TLS Secret. Based on its +// value, the Secret is checked for the following keys: +// - tls.key/keyFile for the private key +// - tls.crt/certFile for the certificate +// - 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) + } + + var certBytes, keyBytes, caBytes []byte + if kubernetesTLSKeys { + certBytes, keyBytes, caBytes = secret.Data[corev1.TLSCertKey], secret.Data[corev1.TLSPrivateKeyKey], secret.Data[CACrtKey] + } else { + certBytes, keyBytes, caBytes = secret.Data["certFile"], secret.Data["keyFile"], secret.Data["caFile"] + } + + switch { + case len(certBytes)+len(keyBytes)+len(caBytes) == 0: + return nil, nil, nil + case (len(certBytes) > 0 && len(keyBytes) == 0) || (len(keyBytes) > 0 && len(certBytes) == 0): + return nil, nil, fmt.Errorf("invalid '%s' secret data: both certificate and private key need to be provided", + secret.Name) + } + + tlsConf := &tls.Config{ + MinVersion: tls.VersionTLS12, + } + if len(certBytes) > 0 && len(keyBytes) > 0 { + cert, err := tls.X509KeyPair(certBytes, keyBytes) + if err != nil { + return nil, nil, err + } + tlsConf.Certificates = append(tlsConf.Certificates, cert) + } + + if len(caBytes) > 0 { + cp, err := x509.SystemCertPool() + if err != nil { + return nil, nil, fmt.Errorf("cannot retrieve system certificate pool: %w", err) + } + if !cp.AppendCertsFromPEM(caBytes) { + return nil, nil, fmt.Errorf("cannot append certificate into certificate pool: invalid CA certificate") + } + + tlsConf.RootCAs = cp + } + + if url != "" { + u, err := neturl.Parse(url) + if err != nil { + return nil, nil, fmt.Errorf("cannot parse repository URL: %w", err) + } + + tlsConf.ServerName = u.Hostname() + } + + return tlsConf, &TLSBytes{ + CertBytes: certBytes, + KeyBytes: keyBytes, + CABytes: caBytes, + }, nil +} diff --git a/internal/tls/config_test.go b/internal/tls/config_test.go new file mode 100644 index 000000000..728b988b7 --- /dev/null +++ b/internal/tls/config_test.go @@ -0,0 +1,178 @@ +/* +Copyright 2023 The Flux authors + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package tls + +import ( + "crypto/rand" + "crypto/rsa" + "crypto/x509" + "encoding/pem" + "fmt" + "math/big" + "net/url" + "testing" + + . "github.com/onsi/gomega" + corev1 "k8s.io/api/core/v1" +) + +func Test_tlsClientConfigFromSecret(t *testing.T) { + kubernetesTlsSecretFixture := validTlsSecret(t, true) + 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: "tls.crt, tls.key and ca.crt", + secret: kubernetesTlsSecretFixture, + modify: nil, + tlsKeys: true, + url: "https://example.com", + }, + { + name: "certFile, keyFile and caFile", + secret: tlsSecretFixture, + modify: nil, + tlsKeys: false, + url: "https://example.com", + }, + { + name: "without tls.crt", + secret: kubernetesTlsSecretFixture, + modify: func(s *corev1.Secret) { delete(s.Data, "tls.crt") }, + tlsKeys: true, + wantErr: true, + wantNil: true, + }, + { + name: "without tls.key", + secret: kubernetesTlsSecretFixture, + modify: func(s *corev1.Secret) { delete(s.Data, "tls.key") }, + tlsKeys: true, + wantErr: true, + wantNil: true, + }, + { + name: "without ca.crt", + secret: kubernetesTlsSecretFixture, + modify: func(s *corev1.Secret) { delete(s.Data, "ca.crt") }, + tlsKeys: true, + }, + { + name: "empty secret", + secret: corev1.Secret{}, + tlsKeys: true, + wantNil: true, + }, + { + name: "invalid secret type", + secret: corev1.Secret{Type: corev1.SecretTypeDockerConfigJson}, + wantErr: true, + wantNil: true, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + g := NewWithT(t) + secret := tt.secret.DeepCopy() + if tt.modify != nil { + tt.modify(secret) + } + + tlsConfig, _, err := tlsClientConfigFromSecret(*secret, tt.url, tt.tlsKeys) + 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 != "" { + u, _ := url.Parse(tt.url) + g.Expect(u.Hostname()).To(Equal(tlsConfig.ServerName)) + } + }) + } +} + +// validTlsSecret creates a secret containing key pair and CA certificate that are +// valid from a syntax (minimum requirements) perspective. +func validTlsSecret(t *testing.T, kubernetesTlsKeys bool) corev1.Secret { + t.Helper() + key, err := rsa.GenerateKey(rand.Reader, 2048) + if err != nil { + t.Fatal("Private key cannot be created.", err.Error()) + } + + certTemplate := x509.Certificate{ + SerialNumber: big.NewInt(1337), + } + cert, err := x509.CreateCertificate(rand.Reader, &certTemplate, &certTemplate, &key.PublicKey, key) + if err != nil { + t.Fatal("Certificate cannot be created.", err.Error()) + } + + ca := &x509.Certificate{ + SerialNumber: big.NewInt(7331), + IsCA: true, + ExtKeyUsage: []x509.ExtKeyUsage{x509.ExtKeyUsageClientAuth, x509.ExtKeyUsageServerAuth}, + KeyUsage: x509.KeyUsageDigitalSignature | x509.KeyUsageCertSign, + } + + caPrivKey, err := rsa.GenerateKey(rand.Reader, 4096) + if err != nil { + t.Fatal("CA private key cannot be created.", err.Error()) + } + + caBytes, err := x509.CreateCertificate(rand.Reader, ca, ca, &caPrivKey.PublicKey, caPrivKey) + if err != nil { + t.Fatal("CA certificate cannot be created.", err.Error()) + } + + keyPem := pem.EncodeToMemory(&pem.Block{ + Type: "RSA PRIVATE KEY", + Bytes: x509.MarshalPKCS1PrivateKey(key), + }) + + certPem := pem.EncodeToMemory(&pem.Block{ + Type: "CERTIFICATE", + Bytes: cert, + }) + + caPem := pem.EncodeToMemory(&pem.Block{ + Type: "CERTIFICATE", + Bytes: caBytes, + }) + + crtKey := corev1.TLSCertKey + pkKey := corev1.TLSPrivateKeyKey + caKey := CACrtKey + if !kubernetesTlsKeys { + crtKey = "certFile" + pkKey = "keyFile" + caKey = "caFile" + } + return corev1.Secret{ + Data: map[string][]byte{ + crtKey: []byte(certPem), + pkKey: []byte(keyPem), + caKey: []byte(caPem), + }, + } +} From 6fe3c963119cb09475ab9dfe2f463d7c4fda6de4 Mon Sep 17 00:00:00 2001 From: Sanskar Jaiswal Date: Wed, 9 Aug 2023 12:14:36 +0530 Subject: [PATCH 2/3] ocirepo: adopt Kubernetes style TLS secrets for .spec.certSecretRef Adopt Kubernetes TLS secrets API to check for TLS data in the Secret referred to by `.spec.certSecretRef`, i.e. check for keys `tls.crt` and `tls.key` for the certificate and private key. Use `ca.crt` for the CA certificate. Deprecate the usage of `caFile`, `certFile` and `keyFile` keys. Signed-off-by: Sanskar Jaiswal --- api/v1beta2/ocirepository_types.go | 20 +++--- ...rce.toolkit.fluxcd.io_ocirepositories.yaml | 10 +-- docs/api/v1beta2/source.md | 26 ++++--- docs/spec/v1beta2/ocirepositories.md | 72 ++++++++++++------- .../controller/ocirepository_controller.go | 32 ++++----- .../ocirepository_controller_test.go | 55 +++++++++++++- 6 files changed, 145 insertions(+), 70 deletions(-) diff --git a/api/v1beta2/ocirepository_types.go b/api/v1beta2/ocirepository_types.go index 9019da519..299f20a52 100644 --- a/api/v1beta2/ocirepository_types.go +++ b/api/v1beta2/ocirepository_types.go @@ -97,17 +97,21 @@ type OCIRepositorySpec struct { // +optional ServiceAccountName string `json:"serviceAccountName,omitempty"` - // CertSecretRef can be given the name of a secret containing + // CertSecretRef can be given the name of a Secret containing // either or both of // - // - a PEM-encoded client certificate (`certFile`) and private - // key (`keyFile`); - // - a PEM-encoded CA certificate (`caFile`) + // - a PEM-encoded client certificate (`tls.crt`) and private + // key (`tls.key`); + // - a PEM-encoded CA certificate (`ca.crt`) // - // and whichever are supplied, will be used for connecting to the - // registry. The client cert and key are useful if you are - // authenticating with a certificate; the CA cert is useful if - // you are using a self-signed server certificate. + // and whichever are supplied, will be used for connecting to the + // registry. The client cert and key are useful if you are + // authenticating with a certificate; the CA cert is useful if + // you are using a self-signed server certificate. The Secret must + // be of type `Opaque` or `kubernetes.io/tls`. + // + // Note: Support for the `caFile`, `certFile` and `keyFile` keys have + // been deprecated. // +optional CertSecretRef *meta.LocalObjectReference `json:"certSecretRef,omitempty"` diff --git a/config/crd/bases/source.toolkit.fluxcd.io_ocirepositories.yaml b/config/crd/bases/source.toolkit.fluxcd.io_ocirepositories.yaml index 8fd16bf16..df40334a4 100644 --- a/config/crd/bases/source.toolkit.fluxcd.io_ocirepositories.yaml +++ b/config/crd/bases/source.toolkit.fluxcd.io_ocirepositories.yaml @@ -50,13 +50,15 @@ spec: description: OCIRepositorySpec defines the desired state of OCIRepository properties: certSecretRef: - description: "CertSecretRef can be given the name of a secret containing - either or both of \n - a PEM-encoded client certificate (`certFile`) - and private key (`keyFile`); - a PEM-encoded CA certificate (`caFile`) + description: "CertSecretRef can be given the name of a Secret containing + either or both of \n - a PEM-encoded client certificate (`tls.crt`) + and private key (`tls.key`); - a PEM-encoded CA certificate (`ca.crt`) \n and whichever are supplied, will be used for connecting to the registry. The client cert and key are useful if you are authenticating with a certificate; the CA cert is useful if you are using a self-signed - server certificate." + server certificate. The Secret must be of type `Opaque` or `kubernetes.io/tls`. + \n Note: Support for the `caFile`, `certFile` and `keyFile` keys + have been deprecated." properties: name: description: Name of the referent. diff --git a/docs/api/v1beta2/source.md b/docs/api/v1beta2/source.md index 73899644f..3d58db692 100644 --- a/docs/api/v1beta2/source.md +++ b/docs/api/v1beta2/source.md @@ -1119,17 +1119,20 @@ github.com/fluxcd/pkg/apis/meta.LocalObjectReference (Optional) -

CertSecretRef can be given the name of a secret containing +

CertSecretRef can be given the name of a Secret containing either or both of

    -
  • a PEM-encoded client certificate (certFile) and private -key (keyFile);
    • -
  • a PEM-encoded CA certificate (caFile)
    • +
  • a PEM-encoded client certificate (tls.crt) and private +key (tls.key);
    • +
  • a PEM-encoded CA certificate (ca.crt)

and whichever are supplied, will be used for connecting to the registry. The client cert and key are useful if you are authenticating with a certificate; the CA cert is useful if -you are using a self-signed server certificate.

+you are using a self-signed server certificate. The Secret must +be of type Opaque or kubernetes.io/tls.

+

Note: Support for the caFile, certFile and keyFile keys have +been deprecated.

@@ -3024,17 +3027,20 @@ github.com/fluxcd/pkg/apis/meta.LocalObjectReference (Optional) -

CertSecretRef can be given the name of a secret containing +

CertSecretRef can be given the name of a Secret containing either or both of

    -
  • a PEM-encoded client certificate (certFile) and private -key (keyFile);
    • -
  • a PEM-encoded CA certificate (caFile)
    • +
  • a PEM-encoded client certificate (tls.crt) and private +key (tls.key);
    • +
  • a PEM-encoded CA certificate (ca.crt)

and whichever are supplied, will be used for connecting to the registry. The client cert and key are useful if you are authenticating with a certificate; the CA cert is useful if -you are using a self-signed server certificate.

+you are using a self-signed server certificate. The Secret must +be of type Opaque or kubernetes.io/tls.

+

Note: Support for the caFile, certFile and keyFile keys have +been deprecated.

diff --git a/docs/spec/v1beta2/ocirepositories.md b/docs/spec/v1beta2/ocirepositories.md index 67276a955..2db354930 100644 --- a/docs/spec/v1beta2/ocirepositories.md +++ b/docs/spec/v1beta2/ocirepositories.md @@ -310,42 +310,62 @@ fetch the image pull secrets attached to the service account and use them for au **Note:** that for a publicly accessible image repository, you don't need to provide a `secretRef` nor `serviceAccountName`. -### TLS Certificates +### Cert secret reference -`.spec.certSecretRef` field names a secret with TLS certificate data. This is for two separate -purposes: +`.spec.certSecretRef.name` is an optional field to specify a secret containing +TLS certificate data. The secret can contain the following keys: -- to provide a client certificate and private key, if you use a certificate to authenticate with - the container registry; and, -- to provide a CA certificate, if the registry uses a self-signed certificate. +* `tls.crt` and `tls.key`, to specify the client certificate and private key used +for TLS client authentication. These must be used in conjunction, i.e. +specifying one without the other will lead to an error. +* `ca.crt`, to specify the CA certificate used to verify the server, which is +required if the server is using a self-signed certificate. -These will often go together, if you are hosting a container registry yourself. All the files in the -secret are expected to be [PEM-encoded][pem-encoding]. This is an ASCII format for certificates and -keys; `openssl` and such tools will typically give you an option of PEM output. +If the server is using a self-signed certificate and has TLS client +authentication enabled, all three values are required. -Assuming you have obtained a certificate file and private key and put them in the files `client.crt` -and `client.key` respectively, you can create a secret with `kubectl` like this: +The Secret should be of type `Opaque` or `kubernetes.io/tls`. All the files in +the Secret are expected to be [PEM-encoded][pem-encoding]. Assuming you have +three files; `client.key`, `client.crt` and `ca.crt` for the client private key, +client certificate and the CA certificate respectively, you can generate the +required Secret using the `flux create secret tls` command: -```bash -kubectl create secret generic tls-certs \ - --from-file=certFile=client.crt \ - --from-file=keyFile=client.key +```sh +flux create secret tls --tls-key-file=client.key --tls-crt-file=client.crt --ca-crt-file=ca.crt ``` -You could also [prepare a secret and encrypt it][sops-guide]; the important bit is that the data -keys in the secret are `certFile` and `keyFile`. - -If you have a CA certificate for the client to use, the data key for that is `caFile`. Adapting the -previous example, if you have the certificate in the file `ca.crt`, and the client certificate and -key as before, the whole command would be: +Example usage: -```bash -kubectl create secret generic tls-certs \ - --from-file=certFile=client.crt \ - --from-file=keyFile=client.key \ - --from-file=caFile=ca.crt +```yaml +--- +apiVersion: source.toolkit.fluxcd.io/v1beta2 +kind: OCIRepository +metadata: + name: example + namespace: default +spec: + interval: 5m0s + url: oci://example.com + certSecretRef: + name: example-tls +--- +apiVersion: v1 +kind: Secret +metadata: + name: example-tls + namespace: default +type: kubernetes.io/tls # or Opaque +data: + tls.crt: + tls.key: + # NOTE: Can be supplied without the above values + ca.crt: ``` +**Warning:** Support for the `caFile`, `certFile` and `keyFile` keys have been +deprecated. If you have any Secrets using these keys and specified in an +OCIRepository, the controller will log a deprecation warning. + ### Insecure `.spec.insecure` is an optional field to allow connecting to an insecure (HTTP) diff --git a/internal/controller/ocirepository_controller.go b/internal/controller/ocirepository_controller.go index 23939ecb8..f10735408 100644 --- a/internal/controller/ocirepository_controller.go +++ b/internal/controller/ocirepository_controller.go @@ -18,8 +18,6 @@ package controller import ( "context" - "crypto/tls" - "crypto/x509" "errors" "fmt" "io" @@ -71,6 +69,7 @@ import ( soci "github.com/fluxcd/source-controller/internal/oci" sreconcile "github.com/fluxcd/source-controller/internal/reconcile" "github.com/fluxcd/source-controller/internal/reconcile/summarize" + "github.com/fluxcd/source-controller/internal/tls" "github.com/fluxcd/source-controller/internal/util" ) @@ -841,29 +840,22 @@ func (r *OCIRepositoryReconciler) transport(ctx context.Context, obj *ociv1.OCIR } transport := remote.DefaultTransport.(*http.Transport).Clone() - tlsConfig := transport.TLSClientConfig - - if clientCert, ok := certSecret.Data[oci.ClientCert]; ok { - // parse and set client cert and secret - if clientKey, ok := certSecret.Data[oci.ClientKey]; ok { - cert, err := tls.X509KeyPair(clientCert, clientKey) - if err != nil { - return nil, err - } - tlsConfig.Certificates = append(tlsConfig.Certificates, cert) - } else { - return nil, fmt.Errorf("'%s' found in secret, but no %s", oci.ClientCert, oci.ClientKey) - } + tlsConfig, _, err := tls.KubeTLSClientConfigFromSecret(certSecret, "") + if err != nil { + return nil, err } - - if caCert, ok := certSecret.Data[oci.CACert]; ok { - syscerts, err := x509.SystemCertPool() + if tlsConfig == nil { + tlsConfig, _, err = tls.TLSClientConfigFromSecret(certSecret, "") if err != nil { return nil, err } - syscerts.AppendCertsFromPEM(caCert) - tlsConfig.RootCAs = syscerts + if tlsConfig != nil { + ctrl.LoggerFrom(ctx). + Info("warning: specifying TLS auth data via `certFile`/`keyFile`/`caFile` is deprecated, please use `tls.crt`/`tls.key`/`ca.crt` instead") + } } + transport.TLSClientConfig = tlsConfig + return transport, nil } diff --git a/internal/controller/ocirepository_controller_test.go b/internal/controller/ocirepository_controller_test.go index ee8f3af80..30fc10bae 100644 --- a/internal/controller/ocirepository_controller_test.go +++ b/internal/controller/ocirepository_controller_test.go @@ -557,6 +557,31 @@ func TestOCIRepository_reconcileSource_authStrategy(t *testing.T) { }, }), }, + tlsCertSecret: &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "ca-file", + }, + Data: map[string][]byte{ + "ca.crt": tlsCA, + }, + }, + assertConditions: []metav1.Condition{ + *conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingReason, "building artifact: new revision '' for ''"), + *conditions.UnknownCondition(meta.ReadyCondition, meta.ProgressingReason, "building artifact: new revision '' for ''"), + }, + }, + { + name: "HTTPS with valid certfile using deprecated keys", + want: sreconcile.ResultSuccess, + registryOpts: registryOptions{ + withTLS: true, + }, + craneOpts: []crane.Option{crane.WithTransport(&http.Transport{ + TLSClientConfig: &tls.Config{ + RootCAs: pool, + }, + }), + }, tlsCertSecret: &corev1.Secret{ ObjectMeta: metav1.ObjectMeta{ Name: "ca-file", @@ -605,11 +630,37 @@ func TestOCIRepository_reconcileSource_authStrategy(t *testing.T) { Name: "ca-file", }, Data: map[string][]byte{ + "ca.crt": []byte("invalid"), + }, + }, + assertConditions: []metav1.Condition{ + *conditions.TrueCondition(sourcev1.FetchFailedCondition, ociv1.AuthenticationFailedReason, "cannot append certificate into certificate pool: invalid CA certificate"), + }, + }, + { + name: "HTTPS with certfile using both caFile and ca.crt ignores caFile", + want: sreconcile.ResultSuccess, + registryOpts: registryOptions{ + withTLS: true, + }, + craneOpts: []crane.Option{crane.WithTransport(&http.Transport{ + TLSClientConfig: &tls.Config{ + RootCAs: pool, + }, + }), + }, + tlsCertSecret: &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "ca-file", + }, + Data: map[string][]byte{ + "ca.crt": tlsCA, "caFile": []byte("invalid"), }, }, assertConditions: []metav1.Condition{ - *conditions.TrueCondition(sourcev1.FetchFailedCondition, ociv1.OCIPullFailedReason, "failed to determine artifact digest"), + *conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingReason, "building artifact: new revision '' for ''"), + *conditions.UnknownCondition(meta.ReadyCondition, meta.ProgressingReason, "building artifact: new revision '' for ''"), }, }, { @@ -1257,7 +1308,7 @@ func TestOCIRepository_reconcileSource_verifyOCISourceSignature(t *testing.T) { Generation: 1, }, Data: map[string][]byte{ - "caFile": tlsCA, + "ca.crt": tlsCA, }, } From 2a7f67de48ed0f1239349658f66d75a102d38778 Mon Sep 17 00:00:00 2001 From: Sanskar Jaiswal Date: Tue, 8 Aug 2023 17:56:00 +0530 Subject: [PATCH 3/3] gitrepo: add support for specifying CA data via `ca.crt` Check the auth secret for the `ca.crt` key for CA certificate data. `ca.crt` takes precdence over `caFile`. Signed-off-by: Sanskar Jaiswal --- docs/spec/v1/gitrepositories.md | 7 ++--- .../gitrepository_controller_test.go | 26 +++++++++++++++++++ 2 files changed, 30 insertions(+), 3 deletions(-) diff --git a/docs/spec/v1/gitrepositories.md b/docs/spec/v1/gitrepositories.md index 066f765fb..4170d9f1b 100644 --- a/docs/spec/v1/gitrepositories.md +++ b/docs/spec/v1/gitrepositories.md @@ -161,8 +161,9 @@ data: #### HTTPS Certificate Authority To provide a Certificate Authority to trust while connecting with a Git -repository over HTTPS, the referenced Secret can contain a `.data.caFile` -value. +repository over HTTPS, the referenced Secret's `.data` can contain a `ca.crt` +or `caFile` key. `ca.crt` takes precedence over `caFile`, i.e. if both keys +are present, the value of `ca.crt` will be taken into consideration. ```yaml --- @@ -173,7 +174,7 @@ metadata: namespace: default type: Opaque data: - caFile: + ca.crt: ``` #### SSH authentication diff --git a/internal/controller/gitrepository_controller_test.go b/internal/controller/gitrepository_controller_test.go index 85c96dcd2..62b8dadac 100644 --- a/internal/controller/gitrepository_controller_test.go +++ b/internal/controller/gitrepository_controller_test.go @@ -410,6 +410,32 @@ func TestGitRepositoryReconciler_reconcileSource_authStrategy(t *testing.T) { *conditions.UnknownCondition(meta.ReadyCondition, meta.ProgressingReason, "building artifact: new upstream revision 'master@sha1:'"), }, }, + { + name: "HTTPS with CAFile secret with both ca.crt and caFile keys makes Reconciling=True and ignores caFile", + protocol: "https", + server: options{ + publicKey: tlsPublicKey, + privateKey: tlsPrivateKey, + ca: tlsCA, + }, + secret: &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "ca-file", + }, + Data: map[string][]byte{ + "ca.crt": tlsCA, + "caFile": []byte("invalid"), + }, + }, + beforeFunc: func(obj *sourcev1.GitRepository) { + obj.Spec.SecretRef = &meta.LocalObjectReference{Name: "ca-file"} + }, + want: sreconcile.ResultSuccess, + assertConditions: []metav1.Condition{ + *conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingReason, "building artifact: new upstream revision 'master@sha1:'"), + *conditions.UnknownCondition(meta.ReadyCondition, meta.ProgressingReason, "building artifact: new upstream revision 'master@sha1:'"), + }, + }, { name: "HTTPS with invalid CAFile secret makes CheckoutFailed=True and returns error", protocol: "https",