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 <[email protected]>
fluxcd · Aug 18, 2023 · 581b0a9 · 581b0a9
1 parent 64e648f
commit 581b0a9
Show file tree

Hide file tree

Showing 6 changed files with 123 additions and 80 deletions.
diff --git a/api/v1beta2/ocirepository_types.go b/api/v1beta2/ocirepository_types.go
@@ -100,14 +100,17 @@ type OCIRepositorySpec struct {
 	// 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.
+
+	//  Note: Support for the `caFile`, `certFile` and `keyFile` keys has
+	//  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
@@ -50,13 +50,8 @@ 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`)
-                  \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."
+                description: 'Note: Support for the `caFile`, `certFile` and `keyFile`
+                  keys has been deprecated.'
                 properties:
                   name:
                     description: Name of the referent.

diff --git a/docs/api/v1beta2/source.md b/docs/api/v1beta2/source.md
@@ -1109,17 +1109,8 @@ github.com/fluxcd/pkg/apis/meta.LocalObjectReference
 </td>
 <td>
 <em>(Optional)</em>
-<p>CertSecretRef can be given the name of a secret containing
-either or both of</p>
-<ul>
-<li>a PEM-encoded client certificate (<code>certFile</code>) and private
-key (<code>keyFile</code>);</li>
-<li>a PEM-encoded CA certificate (<code>caFile</code>)</li>
-</ul>
-<p>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.</p>
+<p>Note: Support for the <code>caFile</code>, <code>certFile</code> and <code>keyFile</code> keys has
+been deprecated.</p>
 </td>
 </tr>
 <tr>
@@ -3004,17 +2995,8 @@ github.com/fluxcd/pkg/apis/meta.LocalObjectReference
 </td>
 <td>
 <em>(Optional)</em>
-<p>CertSecretRef can be given the name of a secret containing
-either or both of</p>
-<ul>
-<li>a PEM-encoded client certificate (<code>certFile</code>) and private
-key (<code>keyFile</code>);</li>
-<li>a PEM-encoded CA certificate (<code>caFile</code>)</li>
-</ul>
-<p>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.</p>
+<p>Note: Support for the <code>caFile</code>, <code>certFile</code> and <code>keyFile</code> keys has
+been deprecated.</p>
 </td>
 </tr>
 <tr>

diff --git 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 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: <BASE64>
+  tls.key: <BASE64>
+  # NOTE: Can be supplied without the above values
+  ca.crt: <BASE64>
 ```
 
+**Note:** 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
@@ -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
@@ -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 '<revision>' for '<url>'"),
+				*conditions.UnknownCondition(meta.ReadyCondition, meta.ProgressingReason, "building artifact: new revision '<revision>' for '<url>'"),
+			},
+		},
+		{
+			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 '<revision>' for '<url>'"),
+				*conditions.UnknownCondition(meta.ReadyCondition, meta.ProgressingReason, "building artifact: new revision '<revision>' for '<url>'"),
 			},
 		},
 		{
@@ -1257,7 +1308,7 @@ func TestOCIRepository_reconcileSource_verifyOCISourceSignature(t *testing.T) {
 			Generation: 1,
 		},
 		Data: map[string][]byte{
-			"caFile": tlsCA,
+			"ca.crt": tlsCA,
 		},
 	}