From c8117d8e349f58ad118a771c0a96b31e34f690b1 Mon Sep 17 00:00:00 2001 From: Todd Short Date: Fri, 14 Jun 2024 13:02:03 -0400 Subject: [PATCH] Update certificate support Remove `rootCAs` from the `NewDefaultUnpacker` API, the argument is no longer used for HTTP transport. Add `CertificateData` to the ImageSource struct. This is PEM-encoded data (straight from a Secret[tls.crt]) to be used to validate the certificate used to access an image regidstry (works along side the `InsecureSkipTLSVerify` option). Signed-off-by: Todd Short --- api/v1alpha2/bundle_types.go | 2 ++ cmd/core/main.go | 2 +- cmd/helm/main.go | 2 +- .../core.rukpak.io_bundledeployments.yaml | 8 +++++++ pkg/source/image_registry.go | 23 +++++++++++++++---- pkg/source/unpacker.go | 12 +--------- test/e2e/registry_provisioner_test.go | 20 ++++++++++++++-- .../imageregistry/image-registry-secure.sh | 1 + test/tools/imageregistry/image-registry.sh | 1 + 9 files changed, 51 insertions(+), 20 deletions(-) diff --git a/api/v1alpha2/bundle_types.go b/api/v1alpha2/bundle_types.go index 602e8ffb..d877e2d4 100644 --- a/api/v1alpha2/bundle_types.go +++ b/api/v1alpha2/bundle_types.go @@ -67,6 +67,8 @@ type ImageSource struct { // This should not be used in a production environment. // +optional InsecureSkipTLSVerify bool `json:"insecureSkipTLSVerify,omitempty"` + // CertificateData contains the PEM data of the certificate that is to be used for the TLS connection + CertificateData string `json:"certificateData,omitempty"` } type GitSource struct { diff --git a/cmd/core/main.go b/cmd/core/main.go index f3ecccbe..810e41ba 100644 --- a/cmd/core/main.go +++ b/cmd/core/main.go @@ -199,7 +199,7 @@ func main() { os.Exit(1) } - unpacker, err := source.NewDefaultUnpacker(mgr, systemNamespace, unpackCacheDir, rootCAs) + unpacker, err := source.NewDefaultUnpacker(mgr, systemNamespace, unpackCacheDir) if err != nil { setupLog.Error(err, "unable to setup bundle unpacker") os.Exit(1) diff --git a/cmd/helm/main.go b/cmd/helm/main.go index b7a5e982..394897a4 100644 --- a/cmd/helm/main.go +++ b/cmd/helm/main.go @@ -189,7 +189,7 @@ func main() { os.Exit(1) } - unpacker, err := source.NewDefaultUnpacker(mgr, systemNamespace, unpackCacheDir, rootCAs) + unpacker, err := source.NewDefaultUnpacker(mgr, systemNamespace, unpackCacheDir) if err != nil { setupLog.Error(err, "unable to setup bundle unpacker") os.Exit(1) diff --git a/manifests/base/apis/crds/core.rukpak.io_bundledeployments.yaml b/manifests/base/apis/crds/core.rukpak.io_bundledeployments.yaml index e6a6425e..7445a6db 100644 --- a/manifests/base/apis/crds/core.rukpak.io_bundledeployments.yaml +++ b/manifests/base/apis/crds/core.rukpak.io_bundledeployments.yaml @@ -223,6 +223,10 @@ spec: description: Image is the bundle image that backs the content of this bundle. properties: + certificateData: + description: CertificateData contains the PEM data of the + certificate that is to be used for the TLS connection + type: string insecureSkipTLSVerify: description: |- InsecureSkipTLSVerify indicates that TLS certificate validation should be skipped. @@ -468,6 +472,10 @@ spec: description: Image is the bundle image that backs the content of this bundle. properties: + certificateData: + description: CertificateData contains the PEM data of the + certificate that is to be used for the TLS connection + type: string insecureSkipTLSVerify: description: |- InsecureSkipTLSVerify indicates that TLS certificate validation should be skipped. diff --git a/pkg/source/image_registry.go b/pkg/source/image_registry.go index 745d8b3c..0214803f 100644 --- a/pkg/source/image_registry.go +++ b/pkg/source/image_registry.go @@ -4,6 +4,7 @@ import ( "archive/tar" "context" "crypto/tls" + "crypto/x509" "errors" "fmt" "io/fs" @@ -64,14 +65,25 @@ func (i *ImageRegistry) Unpack(ctx context.Context, bundle *rukpakv1alpha2.Bundl remoteOpts = append(remoteOpts, remote.WithAuthFromKeychain(authChain)) } + transport := remote.DefaultTransport.(*http.Transport).Clone() + if transport.TLSClientConfig == nil { + transport.TLSClientConfig = &tls.Config{ + InsecureSkipVerify: false, + MinVersion: tls.VersionTLS12, + } // nolint:gosec + } if bundle.Spec.Source.Image.InsecureSkipTLSVerify { - insecureTransport := remote.DefaultTransport.(*http.Transport).Clone() - if insecureTransport.TLSClientConfig == nil { - insecureTransport.TLSClientConfig = &tls.Config{} // nolint:gosec + transport.TLSClientConfig.InsecureSkipVerify = true // nolint:gosec + } + if bundle.Spec.Source.Image.CertificateData != "" { + pool, err := x509.SystemCertPool() + if err != nil || pool == nil { + pool = x509.NewCertPool() } - insecureTransport.TLSClientConfig.InsecureSkipVerify = true // nolint:gosec - remoteOpts = append(remoteOpts, remote.WithTransport(insecureTransport)) + transport.TLSClientConfig.RootCAs = pool + transport.TLSClientConfig.RootCAs.AppendCertsFromPEM([]byte(bundle.Spec.Source.Image.CertificateData)) } + remoteOpts = append(remoteOpts, remote.WithTransport(transport)) digest, isDigest := imgRef.(name.Digest) if isDigest { @@ -141,6 +153,7 @@ func unpackedResult(fsys fs.FS, bundle *rukpakv1alpha2.BundleDeployment, ref str Ref: ref, ImagePullSecretName: bundle.Spec.Source.Image.ImagePullSecretName, InsecureSkipTLSVerify: bundle.Spec.Source.Image.InsecureSkipTLSVerify, + CertificateData: bundle.Spec.Source.Image.CertificateData, }, }, State: StateUnpacked, diff --git a/pkg/source/unpacker.go b/pkg/source/unpacker.go index bbe3a696..64852517 100644 --- a/pkg/source/unpacker.go +++ b/pkg/source/unpacker.go @@ -2,11 +2,8 @@ package source import ( "context" - "crypto/tls" - "crypto/x509" "fmt" "io/fs" - "net/http" "sigs.k8s.io/controller-runtime/pkg/manager" @@ -103,14 +100,7 @@ func (s *unpacker) Cleanup(ctx context.Context, bundle *rukpakv1alpha2.BundleDep // source types. // // TODO: refactor NewDefaultUnpacker due to growing parameter list -func NewDefaultUnpacker(mgr manager.Manager, namespace, cacheDir string, rootCAs *x509.CertPool) (Unpacker, error) { - httpTransport := http.DefaultTransport.(*http.Transport).Clone() - if httpTransport.TLSClientConfig == nil { - httpTransport.TLSClientConfig = &tls.Config{ - MinVersion: tls.VersionTLS12, - } - } - httpTransport.TLSClientConfig.RootCAs = rootCAs +func NewDefaultUnpacker(mgr manager.Manager, namespace, cacheDir string) (Unpacker, error) { return NewUnpacker(map[rukpakv1alpha2.SourceType]Unpacker{ rukpakv1alpha2.SourceTypeImage: &ImageRegistry{ BaseCachePath: cacheDir, diff --git a/test/e2e/registry_provisioner_test.go b/test/e2e/registry_provisioner_test.go index 728792b4..1b9b0535 100644 --- a/test/e2e/registry_provisioner_test.go +++ b/test/e2e/registry_provisioner_test.go @@ -7,8 +7,10 @@ import ( . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" + corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/meta" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/types" "sigs.k8s.io/controller-runtime/pkg/client" rukpakv1alpha2 "github.com/operator-framework/rukpak/api/v1alpha2" @@ -23,6 +25,19 @@ var _ = Describe("registry provisioner bundle", func() { ) BeforeEach(func() { ctx = context.Background() + var secret corev1.Secret + + name := types.NamespacedName{ + Name: "rukpak-e2e-registry", + Namespace: "rukpak-e2e", + } + err := c.Get(ctx, name, &secret) + Expect(err).ToNot(HaveOccurred()) + Expect(secret.Type).To(Equal(corev1.SecretTypeTLS)) + data, ok := secret.Data["ca.crt"] + Expect(ok).To(BeTrue()) + Expect(data).ToNot(BeNil()) + certData := string(data[:]) bd = &rukpakv1alpha2.BundleDeployment{ ObjectMeta: metav1.ObjectMeta{ @@ -38,12 +53,13 @@ var _ = Describe("registry provisioner bundle", func() { Type: rukpakv1alpha2.SourceTypeImage, Image: &rukpakv1alpha2.ImageSource{ Ref: fmt.Sprintf("%v/%v", ImageRepo, "registry:valid"), - InsecureSkipTLSVerify: true, + InsecureSkipTLSVerify: false, + CertificateData: certData, }, }, }, } - err := c.Create(ctx, bd) + err = c.Create(ctx, bd) Expect(err).ToNot(HaveOccurred()) }) AfterEach(func() { diff --git a/test/tools/imageregistry/image-registry-secure.sh b/test/tools/imageregistry/image-registry-secure.sh index 916ccb44..24fa86d2 100755 --- a/test/tools/imageregistry/image-registry-secure.sh +++ b/test/tools/imageregistry/image-registry-secure.sh @@ -47,6 +47,7 @@ spec: isCA: true dnsNames: - ${name}-secure.${namespace}.svc + - ${name}-secure.${namespace}.svc.cluster.local privateKey: algorithm: ECDSA size: 256 diff --git a/test/tools/imageregistry/image-registry.sh b/test/tools/imageregistry/image-registry.sh index b3d2d6af..d13d6fdf 100755 --- a/test/tools/imageregistry/image-registry.sh +++ b/test/tools/imageregistry/image-registry.sh @@ -47,6 +47,7 @@ spec: isCA: true dnsNames: - ${name}.${namespace}.svc + - ${name}.${namespace}.svc.cluster.local privateKey: algorithm: ECDSA size: 256