diff --git a/api/v1beta2/imagerepository_types.go b/api/v1beta2/imagerepository_types.go index faa27fc9..eaee2c14 100644 --- a/api/v1beta2/imagerepository_types.go +++ b/api/v1beta2/imagerepository_types.go @@ -63,17 +63,21 @@ type ImageRepositorySpec 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 has + // been deprecated. // +optional CertSecretRef *meta.LocalObjectReference `json:"certSecretRef,omitempty"` diff --git a/config/crd/bases/image.toolkit.fluxcd.io_imagerepositories.yaml b/config/crd/bases/image.toolkit.fluxcd.io_imagerepositories.yaml index 895d630f..40075d79 100644 --- a/config/crd/bases/image.toolkit.fluxcd.io_imagerepositories.yaml +++ b/config/crd/bases/image.toolkit.fluxcd.io_imagerepositories.yaml @@ -287,13 +287,15 @@ spec: - namespaceSelectors type: object 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 + has been deprecated." properties: name: description: Name of the referent. diff --git a/docs/api/v1beta2/image-reflector.md b/docs/api/v1beta2/image-reflector.md index 6a41aac7..576479fa 100644 --- a/docs/api/v1beta2/image-reflector.md +++ b/docs/api/v1beta2/image-reflector.md @@ -473,17 +473,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

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 has +been deprecated.

@@ -658,17 +661,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

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 has +been deprecated.

diff --git a/internal/controller/imagerepository_controller.go b/internal/controller/imagerepository_controller.go index 3a5fee05..0ae58872 100644 --- a/internal/controller/imagerepository_controller.go +++ b/internal/controller/imagerepository_controller.go @@ -384,10 +384,20 @@ func (r *ImageRepositoryReconciler) setAuthOptions(ctx context.Context, obj *ima } } - tr, err := secret.TransportFromSecret(&certSecret) + tr, err := secret.TransportFromKubeTLSSecret(&certSecret) if err != nil { return nil, err } + if tr.TLSClientConfig == nil { + tr, err = secret.TransportFromSecret(&certSecret) + if err != nil { + return nil, err + } + if tr.TLSClientConfig != 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") + } + } options = append(options, remote.WithTransport(tr)) } diff --git a/internal/controller/imagerepository_controller_test.go b/internal/controller/imagerepository_controller_test.go index ac42104f..df25ac43 100644 --- a/internal/controller/imagerepository_controller_test.go +++ b/internal/controller/imagerepository_controller_test.go @@ -102,6 +102,7 @@ func TestImageRepositoryReconciler_setAuthOptions(t *testing.T) { testImg := "example.com/foo/bar" testSecretName := "test-secret" testTLSSecretName := "test-tls-secret" + testDeprecatedTLSSecretName := "test-deprecated-tls-secret" testServiceAccountName := "test-service-account" testNamespace := "test-ns" @@ -132,18 +133,27 @@ func TestImageRepositoryReconciler_setAuthOptions(t *testing.T) { testTLSSecret.Namespace = testNamespace testTLSSecret.Type = corev1.SecretTypeTLS testTLSSecret.Data = map[string][]byte{ + secret.CACrtKey: rootCertPEM, + corev1.TLSCertKey: clientCertPEM, + corev1.TLSPrivateKeyKey: clientKeyPEM, + } + + testDeprecatedTLSSecret := &corev1.Secret{} + testDeprecatedTLSSecret.Name = testDeprecatedTLSSecretName + testDeprecatedTLSSecret.Namespace = testNamespace + testDeprecatedTLSSecret.Type = corev1.SecretTypeTLS + testDeprecatedTLSSecret.Data = map[string][]byte{ secret.CACert: rootCertPEM, secret.ClientCert: clientCertPEM, secret.ClientKey: clientKeyPEM, } - // Secret with docker config and TLS secrets. - testSecretWithTLS := testSecret.DeepCopy() - testSecretWithTLS.Data = map[string][]byte{ - ".dockerconfigjson": dockerconfigjson, - secret.CACert: rootCertPEM, - secret.ClientCert: clientCertPEM, - secret.ClientKey: clientKeyPEM, + // Docker config secret with TLS data. + testDockerCfgSecretWithTLS := testSecret.DeepCopy() + testDockerCfgSecretWithTLS.Data = map[string][]byte{ + secret.CACrtKey: rootCertPEM, + corev1.TLSCertKey: clientCertPEM, + corev1.TLSPrivateKeyKey: clientKeyPEM, } // ServiceAccount without image pull secret. @@ -211,6 +221,16 @@ func TestImageRepositoryReconciler_setAuthOptions(t *testing.T) { }, }, }, + { + name: "cert secret ref with existing secret using deprecated keys", + mockObjs: []client.Object{testDeprecatedTLSSecret}, + imageRepoSpec: imagev1.ImageRepositorySpec{ + Image: testImg, + CertSecretRef: &meta.LocalObjectReference{ + Name: testDeprecatedTLSSecretName, + }, + }, + }, { name: "cert secret ref with non-existing secret", imageRepoSpec: imagev1.ImageRepositorySpec{ @@ -235,17 +255,15 @@ func TestImageRepositoryReconciler_setAuthOptions(t *testing.T) { }, }, { - name: "same secret ref and cert secret ref", - mockObjs: []client.Object{testSecretWithTLS}, + name: "cert secret ref of type docker config", + mockObjs: []client.Object{testDockerCfgSecretWithTLS}, imageRepoSpec: imagev1.ImageRepositorySpec{ Image: testImg, - SecretRef: &meta.LocalObjectReference{ - Name: testSecretName, - }, CertSecretRef: &meta.LocalObjectReference{ Name: testSecretName, }, }, + wantErr: true, }, { name: "service account without pull secret", diff --git a/internal/secret/secret.go b/internal/secret/secret.go index 677608aa..1108e0cc 100644 --- a/internal/secret/secret.go +++ b/internal/secret/secret.go @@ -34,51 +34,128 @@ import ( "k8s.io/apimachinery/pkg/types" ) -// These are intended to match the keys used in e.g., -// https://github.com/fluxcd/flux2/blob/main/cmd/flux/create_secret_helm.go, -// for consistency (and perhaps this will have its own flux create -// secret subcommand at some point). const ( ClientCert = "certFile" ClientKey = "keyFile" CACert = "caFile" + CACrtKey = "ca.crt" ) type dockerConfig struct { Auths map[string]authn.AuthConfig } +// TransportFromSecret reads the TLS data specified in the provided Secret +// and returns a transport configured with the appropriate TLS settings. +// It checks for the following keys in the Secret: +// - `caFile`, for the CA certificate +// - `certFile` and `keyFile`, for the certificate and private key +// +// If none of these keys exists in the Secret then an empty transport is +// returned. If only a certificate OR private key is found, an error is +// returned. func TransportFromSecret(certSecret *corev1.Secret) (*http.Transport, error) { // It's possible the secret doesn't contain any certs after // all and the default transport could be used; but it's // simpler here to assume a fresh transport is needed. - transport := &http.Transport{ - TLSClientConfig: &tls.Config{}, + transport := &http.Transport{} + config, err := tlsConfigFromSecret(certSecret, false) + if err != nil { + return nil, err } - tlsConfig := transport.TLSClientConfig - - if clientCert, ok := certSecret.Data[ClientCert]; ok { - // parse and set client cert and secret - if clientKey, ok := certSecret.Data[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("client certificate found, but no key") - } + if config != nil { + transport.TLSClientConfig = config + } + + return transport, nil +} + +// TransportFromKubeTLSSecret reads the TLS data specified in the provided +// Secret and returns a transport configured with the appropriate TLS settings. +// It checks for the following keys in the Secret: +// - `ca.crt`, for the CA certificate +// - `tls.crt` and `tls.key`, for the certificate and private key +// +// If none of these keys exists in the Secret then an empty transport is +// returned. If only a certificate OR private key is found, an error is +// returned. +func TransportFromKubeTLSSecret(certSecret *corev1.Secret) (*http.Transport, error) { + // It's possible the secret doesn't contain any certs after + // all and the default transport could be used; but it's + // simpler here to assume a fresh transport is needed. + transport := &http.Transport{} + config, err := tlsConfigFromSecret(certSecret, true) + if err != nil { + return nil, err + } + if config != nil { + transport.TLSClientConfig = config + } + + return transport, nil +} + +// 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. +// Copied from: https://github.com/fluxcd/source-controller/blob/052221c3d8a3ce5fd1a1328db4cc27d31bfd5e59/internal/tls/config.go#L78 +func tlsConfigFromSecret(secret *corev1.Secret, kubernetesTLSKeys bool) (*tls.Config, 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, 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[ClientCert], secret.Data[ClientKey], secret.Data[CACert] + } + + switch { + case len(certBytes)+len(keyBytes)+len(caBytes) == 0: + return nil, nil + case (len(certBytes) > 0 && len(keyBytes) == 0) || (len(keyBytes) > 0 && len(certBytes) == 0): + return nil, fmt.Errorf("invalid '%s' secret data: both certificate and private key need to be provided", + secret.Name) } - if caCert, ok := certSecret.Data[CACert]; ok { - syscerts, err := x509.SystemCertPool() + + tlsConf := &tls.Config{} + if len(certBytes) > 0 && len(keyBytes) > 0 { + cert, err := tls.X509KeyPair(certBytes, keyBytes) if err != nil { return nil, err } - syscerts.AppendCertsFromPEM(caCert) - tlsConfig.RootCAs = syscerts + tlsConf.Certificates = append(tlsConf.Certificates, cert) } - return transport, nil + if len(caBytes) > 0 { + cp, err := x509.SystemCertPool() + if err != nil { + return nil, fmt.Errorf("cannot retrieve system certificate pool: %w", err) + } + if !cp.AppendCertsFromPEM(caBytes) { + return nil, fmt.Errorf("cannot append certificate into certificate pool: invalid CA certificate") + } + + tlsConf.RootCAs = cp + } + + return tlsConf, nil + } // authFromSecret creates an Authenticator that can be given to the