From f617407cb2b01cebe57a3e230f472cf1f6ca0c5e Mon Sep 17 00:00:00 2001 From: Yoni Bettan Date: Sun, 4 Dec 2022 14:02:40 +0200 Subject: [PATCH] Adding TLS options to the sign logic. When checking if the `signed` image already exist or when pulling the image to sign, we should be able to set some TLS options. This flow follows the same logic we have for builds. In additions, this commit also update the type of the `ModuleLoaderContainerSpec.RegistryTLS` API so we use the default values when not specified. Signed-off-by: Yoni Bettan --- api/v1beta1/module_types.go | 8 ++- api/v1beta1/zz_generated.deepcopy.go | 7 +-- ...kmm.sigs.k8s.io_managedclustermodules.yaml | 32 +++++++++++ config/crd/bases/kmm.sigs.k8s.io_modules.yaml | 31 +++++++++++ internal/build/job/maker.go | 5 +- internal/build/job/maker_test.go | 4 +- internal/build/job/manager_test.go | 6 +-- internal/module/helper.go | 2 +- internal/module/helper_test.go | 12 ++--- internal/preflight/preflight_test.go | 11 ++-- internal/sign/job/manager_test.go | 6 +-- internal/sign/job/signer.go | 17 ++++++ internal/sign/job/signer_test.go | 54 +++++++++++++++++++ 13 files changed, 167 insertions(+), 28 deletions(-) diff --git a/api/v1beta1/module_types.go b/api/v1beta1/module_types.go index 7227d8f06..216e23dec 100644 --- a/api/v1beta1/module_types.go +++ b/api/v1beta1/module_types.go @@ -54,7 +54,7 @@ type Build struct { // +optional // BaseImageRegistryTLS contains settings determining how to access registries of the base images in the build-process' Dockerfile. - BaseImageRegistryTLS TLSOptions `json:"baseImageRegistryTLS"` + BaseImageRegistryTLS TLSOptions `json:"baseImageRegistryTLS,omitempty"` // +optional // Secrets is an optional list of secrets to be made available to the build system. @@ -72,6 +72,10 @@ type Sign struct { // Image to sign, ignored if a Build is present, required otherwise UnsignedImage string `json:"unsignedImage,omitempty"` + // +optional + // UnsignedImageRegistryTLS contains settings determining how to access registries of the unsigned image. + UnsignedImageRegistryTLS TLSOptions `json:"unsignedImageRegistryTLS,omitempty"` + // a secret containing the private key used to sign kernel modules for secureboot KeySecret *v1.LocalObjectReference `json:"keySecret"` @@ -184,7 +188,7 @@ type ModuleLoaderContainerSpec struct { // +optional // RegistryTLS set the TLS configs for accessing the registry of the module-loader's image. - RegistryTLS *TLSOptions `json:"registryTLS"` + RegistryTLS TLSOptions `json:"registryTLS"` } type ModuleLoaderSpec struct { diff --git a/api/v1beta1/zz_generated.deepcopy.go b/api/v1beta1/zz_generated.deepcopy.go index c05edbe35..dc991be91 100644 --- a/api/v1beta1/zz_generated.deepcopy.go +++ b/api/v1beta1/zz_generated.deepcopy.go @@ -448,11 +448,7 @@ func (in *ModuleLoaderContainerSpec) DeepCopyInto(out *ModuleLoaderContainerSpec } } in.Modprobe.DeepCopyInto(&out.Modprobe) - if in.RegistryTLS != nil { - in, out := &in.RegistryTLS, &out.RegistryTLS - *out = new(TLSOptions) - **out = **in - } + out.RegistryTLS = in.RegistryTLS } // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new ModuleLoaderContainerSpec. @@ -638,6 +634,7 @@ func (in *PreflightValidationStatus) DeepCopy() *PreflightValidationStatus { // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *Sign) DeepCopyInto(out *Sign) { *out = *in + out.UnsignedImageRegistryTLS = in.UnsignedImageRegistryTLS if in.KeySecret != nil { in, out := &in.KeySecret, &out.KeySecret *out = new(v1.LocalObjectReference) diff --git a/config/crd/bases/kmm.sigs.k8s.io_managedclustermodules.yaml b/config/crd/bases/kmm.sigs.k8s.io_managedclustermodules.yaml index 4f3c1412e..2c99a7930 100644 --- a/config/crd/bases/kmm.sigs.k8s.io_managedclustermodules.yaml +++ b/config/crd/bases/kmm.sigs.k8s.io_managedclustermodules.yaml @@ -2203,6 +2203,22 @@ spec: description: Image to sign, ignored if a Build is present, required otherwise type: string + unsignedImageRegistryTLS: + description: UnsignedImageRegistryTLS contains + settings determining how to access registries + of the unsigned image. + properties: + insecure: + description: If Insecure is true, the operator + will be able to access a registry in an + insecure (plain HTTP) protocol. + type: boolean + insecureSkipTLSVerify: + description: If InsecureSkipTLSVerify, the + operator will accept any certificate provided + by the registry. + type: boolean + type: object required: - certSecret - keySecret @@ -2335,6 +2351,22 @@ spec: description: Image to sign, ignored if a Build is present, required otherwise type: string + unsignedImageRegistryTLS: + description: UnsignedImageRegistryTLS contains settings + determining how to access registries of the unsigned + image. + properties: + insecure: + description: If Insecure is true, the operator + will be able to access a registry in an insecure + (plain HTTP) protocol. + type: boolean + insecureSkipTLSVerify: + description: If InsecureSkipTLSVerify, the operator + will accept any certificate provided by the + registry. + type: boolean + type: object required: - certSecret - keySecret diff --git a/config/crd/bases/kmm.sigs.k8s.io_modules.yaml b/config/crd/bases/kmm.sigs.k8s.io_modules.yaml index 8f70abc99..f4e08ed4b 100644 --- a/config/crd/bases/kmm.sigs.k8s.io_modules.yaml +++ b/config/crd/bases/kmm.sigs.k8s.io_modules.yaml @@ -2108,6 +2108,22 @@ spec: description: Image to sign, ignored if a Build is present, required otherwise type: string + unsignedImageRegistryTLS: + description: UnsignedImageRegistryTLS contains settings + determining how to access registries of the unsigned + image. + properties: + insecure: + description: If Insecure is true, the operator + will be able to access a registry in an insecure + (plain HTTP) protocol. + type: boolean + insecureSkipTLSVerify: + description: If InsecureSkipTLSVerify, the operator + will accept any certificate provided by the + registry. + type: boolean + type: object required: - certSecret - keySecret @@ -2238,6 +2254,21 @@ spec: description: Image to sign, ignored if a Build is present, required otherwise type: string + unsignedImageRegistryTLS: + description: UnsignedImageRegistryTLS contains settings + determining how to access registries of the unsigned + image. + properties: + insecure: + description: If Insecure is true, the operator will + be able to access a registry in an insecure (plain + HTTP) protocol. + type: boolean + insecureSkipTLSVerify: + description: If InsecureSkipTLSVerify, the operator + will accept any certificate provided by the registry. + type: boolean + type: object required: - certSecret - keySecret diff --git a/internal/build/job/maker.go b/internal/build/job/maker.go index 6567759fd..f3b8471ad 100644 --- a/internal/build/job/maker.go +++ b/internal/build/job/maker.go @@ -81,12 +81,13 @@ func (m *maker) MakeJobTemplate( containerImage = module.IntermediateImageName(mod.Name, mod.Namespace, containerImage) } + registryTLS := module.TLSOptions(mod.Spec, km) specTemplate := m.specTemplate( mod.Spec, buildConfig, targetKernel, containerImage, - km.RegistryTLS, + registryTLS, pushImage) specTemplateHash, err := m.getHashAnnotationValue(ctx, buildConfig.DockerfileConfigMap.Name, mod.Namespace, &specTemplate) @@ -175,7 +176,7 @@ func (m *maker) containerArgs( args = append(args, "--skip-tls-verify-pull") } - if registryTLS != nil { + if pushImage { if registryTLS.Insecure { args = append(args, "--insecure") } diff --git a/internal/build/job/maker_test.go b/internal/build/job/maker_test.go index f3fb6d2a7..def92f787 100644 --- a/internal/build/job/maker_test.go +++ b/internal/build/job/maker_test.go @@ -293,7 +293,7 @@ var _ = Describe("MakeJobTemplate", func() { DockerfileConfigMap: &dockerfileConfigMap, }, "--insecure-pull", - true, + false, ), Entry( "BaseImageRegistryTLS.InsecureSkipTLSVerify", @@ -317,7 +317,7 @@ var _ = Describe("MakeJobTemplate", func() { &kmmv1beta1.TLSOptions{InsecureSkipTLSVerify: true}, &kmmv1beta1.Build{DockerfileConfigMap: &dockerfileConfigMap}, "--skip-tls-verify", - false, + true, ), ) diff --git a/internal/build/job/manager_test.go b/internal/build/job/manager_test.go index 3d251fa69..0527e5934 100644 --- a/internal/build/job/manager_test.go +++ b/internal/build/job/manager_test.go @@ -72,7 +72,7 @@ var _ = Describe("ShouldSync", func() { } gomock.InOrder( - reg.EXPECT().ImageExists(ctx, imageName, nil, gomock.Any()).Return(true, nil), + reg.EXPECT().ImageExists(ctx, imageName, gomock.Any(), gomock.Any()).Return(true, nil), ) mgr := NewBuildManager(clnt, nil, nil, reg) @@ -102,7 +102,7 @@ var _ = Describe("ShouldSync", func() { } gomock.InOrder( - reg.EXPECT().ImageExists(ctx, imageName, nil, gomock.Any()).Return(false, errors.New("generic-registry-error")), + reg.EXPECT().ImageExists(ctx, imageName, gomock.Any(), gomock.Any()).Return(false, errors.New("generic-registry-error")), ) mgr := NewBuildManager(clnt, nil, nil, reg) @@ -133,7 +133,7 @@ var _ = Describe("ShouldSync", func() { } gomock.InOrder( - reg.EXPECT().ImageExists(ctx, imageName, nil, gomock.Any()).Return(false, nil), + reg.EXPECT().ImageExists(ctx, imageName, gomock.Any(), gomock.Any()).Return(false, nil), ) mgr := NewBuildManager(clnt, nil, nil, reg) diff --git a/internal/module/helper.go b/internal/module/helper.go index 08e943068..e00c7267f 100644 --- a/internal/module/helper.go +++ b/internal/module/helper.go @@ -17,7 +17,7 @@ func TLSOptions(modSpec kmmv1beta1.ModuleSpec, km kmmv1beta1.KernelMapping) *kmm if km.RegistryTLS != nil { return km.RegistryTLS } - return modSpec.ModuleLoader.Container.RegistryTLS + return &modSpec.ModuleLoader.Container.RegistryTLS } // AppendToTag adds the specified tag to the image name cleanly, i.e. by avoiding messing up diff --git a/internal/module/helper_test.go b/internal/module/helper_test.go index 9cf08e534..4b2ba807e 100644 --- a/internal/module/helper_test.go +++ b/internal/module/helper_test.go @@ -40,7 +40,7 @@ var _ = Describe("TLSOptions", func() { Spec: kmmv1beta1.ModuleSpec{ ModuleLoader: kmmv1beta1.ModuleLoaderSpec{ Container: kmmv1beta1.ModuleLoaderContainerSpec{ - RegistryTLS: &kmmv1beta1.TLSOptions{}, + RegistryTLS: kmmv1beta1.TLSOptions{}, }, }, }, @@ -48,7 +48,7 @@ var _ = Describe("TLSOptions", func() { km := kmmv1beta1.KernelMapping{} Expect( - TLSOptions(mod.Spec, km), + *TLSOptions(mod.Spec, km), ).To( Equal(mod.Spec.ModuleLoader.Container.RegistryTLS), ) @@ -241,7 +241,7 @@ var _ = Describe("ImageExists", func() { It("should return true if the image exists", func() { gomock.InOrder( - mockRegistry.EXPECT().ImageExists(ctx, imageName, nil, nil).Return(true, nil), + mockRegistry.EXPECT().ImageExists(ctx, imageName, gomock.Any(), nil).Return(true, nil), ) exists, err := ImageExists(ctx, clnt, mockRegistry, mod.Spec, namespace, km, imageName) @@ -252,7 +252,7 @@ var _ = Describe("ImageExists", func() { It("should return false if the image does not exist", func() { gomock.InOrder( - mockRegistry.EXPECT().ImageExists(ctx, imageName, nil, nil).Return(false, nil), + mockRegistry.EXPECT().ImageExists(ctx, imageName, gomock.Any(), nil).Return(false, nil), ) exists, err := ImageExists(ctx, clnt, mockRegistry, mod.Spec, namespace, km, imageName) @@ -263,7 +263,7 @@ var _ = Describe("ImageExists", func() { It("should return an error if the registry call fails", func() { gomock.InOrder( - mockRegistry.EXPECT().ImageExists(ctx, imageName, nil, nil).Return(false, errors.New("some-error")), + mockRegistry.EXPECT().ImageExists(ctx, imageName, gomock.Any(), nil).Return(false, errors.New("some-error")), ) exists, err := ImageExists(ctx, clnt, mockRegistry, mod.Spec, namespace, km, imageName) @@ -283,7 +283,7 @@ var _ = Describe("ImageExists", func() { } gomock.InOrder( - mockRegistry.EXPECT().ImageExists(ctx, imageName, nil, gomock.Not(gomock.Nil())).Return(false, nil), + mockRegistry.EXPECT().ImageExists(ctx, imageName, gomock.Any(), gomock.Not(gomock.Nil())).Return(false, nil), ) exists, err := ImageExists(ctx, clnt, mockRegistry, mod.Spec, namespace, km, imageName) diff --git a/internal/preflight/preflight_test.go b/internal/preflight/preflight_test.go index 56fccb20a..a7df96514 100644 --- a/internal/preflight/preflight_test.go +++ b/internal/preflight/preflight_test.go @@ -215,7 +215,8 @@ var _ = Describe("preflightHelper_verifyImage", func() { repoConfig := ®istry.RepoPullConfig{} digestLayer := v1stream.Layer{} gomock.InOrder( - mockRegistryAPI.EXPECT().GetLayersDigests(context.Background(), containerImage, nil, gomock.Any()).Return(digests, repoConfig, nil), + mockRegistryAPI.EXPECT().GetLayersDigests(context.Background(), containerImage, gomock.Any(), + gomock.Any()).Return(digests, repoConfig, nil), mockRegistryAPI.EXPECT().GetLayerByDigest(digests[1], repoConfig).Return(&digestLayer, nil), mockRegistryAPI.EXPECT().VerifyModuleExists(&digestLayer, "/opt", kernelVersion, "simple-kmod.ko").Return(true), ) @@ -228,7 +229,8 @@ var _ = Describe("preflightHelper_verifyImage", func() { It("get layers digest failed", func() { mapping := kmmv1beta1.KernelMapping{ContainerImage: containerImage} - mockRegistryAPI.EXPECT().GetLayersDigests(context.Background(), containerImage, nil, gomock.Any()).Return(nil, nil, fmt.Errorf("some error")) + mockRegistryAPI.EXPECT().GetLayersDigests(context.Background(), containerImage, gomock.Any(), + gomock.Any()).Return(nil, nil, fmt.Errorf("some error")) res, message := ph.verifyImage(context.Background(), &mapping, mod, kernelVersion) @@ -241,7 +243,8 @@ var _ = Describe("preflightHelper_verifyImage", func() { digests := []string{"digest0", "digest1"} repoConfig := ®istry.RepoPullConfig{} gomock.InOrder( - mockRegistryAPI.EXPECT().GetLayersDigests(context.Background(), containerImage, nil, gomock.Any()).Return(digests, repoConfig, nil), + mockRegistryAPI.EXPECT().GetLayersDigests(context.Background(), containerImage, gomock.Any(), + gomock.Any()).Return(digests, repoConfig, nil), mockRegistryAPI.EXPECT().GetLayerByDigest(digests[1], repoConfig).Return(nil, fmt.Errorf("some error")), ) @@ -257,7 +260,7 @@ var _ = Describe("preflightHelper_verifyImage", func() { repoConfig := ®istry.RepoPullConfig{} digestLayer := v1stream.Layer{} gomock.InOrder( - mockRegistryAPI.EXPECT().GetLayersDigests(context.Background(), containerImage, nil, gomock.Any()).Return(digests, repoConfig, nil), + mockRegistryAPI.EXPECT().GetLayersDigests(context.Background(), containerImage, gomock.Any(), gomock.Any()).Return(digests, repoConfig, nil), mockRegistryAPI.EXPECT().GetLayerByDigest(digests[0], repoConfig).Return(&digestLayer, nil), mockRegistryAPI.EXPECT().VerifyModuleExists(&digestLayer, "/opt", kernelVersion, "simple-kmod.ko").Return(false), ) diff --git a/internal/sign/job/manager_test.go b/internal/sign/job/manager_test.go index af9f03961..d90d44e1c 100644 --- a/internal/sign/job/manager_test.go +++ b/internal/sign/job/manager_test.go @@ -71,7 +71,7 @@ var _ = Describe("JobManager", func() { } gomock.InOrder( - reg.EXPECT().ImageExists(ctx, imageName, nil, gomock.Any()).Return(true, nil), + reg.EXPECT().ImageExists(ctx, imageName, gomock.Any(), gomock.Any()).Return(true, nil), ) mgr := NewSignJobManager(clnt, nil, nil, reg) @@ -101,7 +101,7 @@ var _ = Describe("JobManager", func() { } gomock.InOrder( - reg.EXPECT().ImageExists(ctx, imageName, nil, gomock.Any()).Return(false, errors.New("generic-registry-error")), + reg.EXPECT().ImageExists(ctx, imageName, gomock.Any(), gomock.Any()).Return(false, errors.New("generic-registry-error")), ) mgr := NewSignJobManager(clnt, nil, nil, reg) @@ -132,7 +132,7 @@ var _ = Describe("JobManager", func() { } gomock.InOrder( - reg.EXPECT().ImageExists(ctx, imageName, nil, gomock.Any()).Return(false, nil), + reg.EXPECT().ImageExists(ctx, imageName, gomock.Any(), gomock.Any()).Return(false, nil), ) mgr := NewSignJobManager(clnt, nil, nil, reg) diff --git a/internal/sign/job/signer.go b/internal/sign/job/signer.go index 8fc9cad1e..5f061f9de 100644 --- a/internal/sign/job/signer.go +++ b/internal/sign/job/signer.go @@ -12,6 +12,7 @@ import ( "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" kmmv1beta1 "github.com/kubernetes-sigs/kernel-module-management/api/v1beta1" + "github.com/kubernetes-sigs/kernel-module-management/internal/module" "github.com/kubernetes-sigs/kernel-module-management/internal/sign" "github.com/kubernetes-sigs/kernel-module-management/internal/utils" ) @@ -62,6 +63,14 @@ func (m *signer) MakeJobTemplate( if pushImage { args = append(args, "-signedimage", km.ContainerImage) + + registryTLS := module.TLSOptions(mod.Spec, km) + if registryTLS.Insecure { + args = append(args, "--insecure") + } + if registryTLS.InsecureSkipTLSVerify { + args = append(args, "--skip-tls-verify") + } } else { args = append(args, "-no-push") } @@ -80,6 +89,14 @@ func (m *signer) MakeJobTemplate( args = append(args, "-filestosign", strings.Join(signConfig.FilesToSign, ":")) } + if signConfig.UnsignedImageRegistryTLS.Insecure { + args = append(args, "--insecure-pull") + } + + if signConfig.UnsignedImageRegistryTLS.InsecureSkipTLSVerify { + args = append(args, "--skip-tls-verify-pull") + } + volumes := []v1.Volume{ utils.MakeSecretVolume(signConfig.KeySecret, "key", "key.priv"), utils.MakeSecretVolume(signConfig.CertSecret, "cert", "public.der"), diff --git a/internal/sign/job/signer_test.go b/internal/sign/job/signer_test.go index 3a6c8a40b..ee4edc062 100644 --- a/internal/sign/job/signer_test.go +++ b/internal/sign/job/signer_test.go @@ -267,4 +267,58 @@ var _ = Describe("MakeJobTemplate", func() { false, ), ) + + DescribeTable("should set correct kmod-signer TLS flags", func(kmRegistryTLS, + unsignedImageRegistryTLS kmmv1beta1.TLSOptions, expectedFlag string) { + + km := kmmv1beta1.KernelMapping{ + RegistryTLS: &kmRegistryTLS, + Sign: &kmmv1beta1.Sign{ + UnsignedImage: signedImage, + UnsignedImageRegistryTLS: unsignedImageRegistryTLS, + }, + } + + gomock.InOrder( + helper.EXPECT().GetRelevantSign(mod.Spec, km).Return(km.Sign), + ) + + actual, err := m.MakeJobTemplate(mod, km, kernelVersion, labels, "", true, &mod) + + Expect(err).NotTo(HaveOccurred()) + Expect(actual.Spec.Template.Spec.Containers[0].Args).To(ContainElement(expectedFlag)) + }, + Entry( + "filelist and push", + kmmv1beta1.TLSOptions{ + Insecure: true, + }, + kmmv1beta1.TLSOptions{}, + "--insecure", + ), + Entry( + "filelist and push", + kmmv1beta1.TLSOptions{ + InsecureSkipTLSVerify: true, + }, + kmmv1beta1.TLSOptions{}, + "--skip-tls-verify", + ), + Entry( + "filelist and push", + kmmv1beta1.TLSOptions{}, + kmmv1beta1.TLSOptions{ + Insecure: true, + }, + "--insecure-pull", + ), + Entry( + "filelist and push", + kmmv1beta1.TLSOptions{}, + kmmv1beta1.TLSOptions{ + InsecureSkipTLSVerify: true, + }, + "--skip-tls-verify-pull", + ), + ) })