From 711ab42b5a48149c91a430e032ffd443883a59d6 Mon Sep 17 00:00:00 2001 From: Jorge Turrado Date: Tue, 21 Nov 2023 12:49:13 +0100 Subject: [PATCH 1/4] feat: Ensure that insecureSkipTLSVerify is false before setting caBundle on APIService Signed-off-by: Jorge Turrado --- pkg/rotator/rotator.go | 3 +++ pkg/rotator/rotator_test.go | 18 ++++++++++++++++++ 2 files changed, 21 insertions(+) diff --git a/pkg/rotator/rotator.go b/pkg/rotator/rotator.go index 68fc576..a14a15f 100644 --- a/pkg/rotator/rotator.go +++ b/pkg/rotator/rotator.go @@ -437,6 +437,9 @@ func injectCertToApiService(apiService *unstructured.Unstructured, certPem []byt if !found { return errors.New("`spec` field not found in APIService") } + if err := unstructured.SetNestedField(apiService.Object, false, "spec", "insecureSkipTLSVerify"); err != nil { + return err + } if err := unstructured.SetNestedField(apiService.Object, base64.StdEncoding.EncodeToString(certPem), "spec", "caBundle"); err != nil { return err } diff --git a/pkg/rotator/rotator_test.go b/pkg/rotator/rotator_test.go index 441bae8..6eff35e 100644 --- a/pkg/rotator/rotator_test.go +++ b/pkg/rotator/rotator_test.go @@ -334,6 +334,24 @@ func TestReconcileWebhook(t *testing.T) { }, }, }, + { + "apiservice-insecure-tls", APIService, nil, []string{"spec", "caBundle"}, &apiregistrationv1.APIService{ + ObjectMeta: metav1.ObjectMeta{ + Name: "v1alpha2.example.com", + }, + Spec: apiregistrationv1.APIServiceSpec{ + Group: "example.com", + GroupPriorityMinimum: 1, + Version: "v1alpha2", + VersionPriority: 1, + InsecureSkipTLSVerify: true, + Service: &apiregistrationv1.ServiceReference{ + Namespace: "kube-system", + Name: "example-api", + }, + }, + }, + }, { "externaldataprovider", ExternalDataProvider, nil, []string{"spec", "caBundle"}, &externaldatav1beta1.Provider{ TypeMeta: metav1.TypeMeta{ From b6658e1229fe9402a67dd7ff9b697da6ab20c9a7 Mon Sep 17 00:00:00 2001 From: Jorge Turrado Date: Fri, 1 Dec 2023 00:33:36 +0100 Subject: [PATCH 2/4] add a comment for the test case Signed-off-by: Jorge Turrado --- pkg/rotator/rotator_test.go | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/pkg/rotator/rotator_test.go b/pkg/rotator/rotator_test.go index 6eff35e..67e5af6 100644 --- a/pkg/rotator/rotator_test.go +++ b/pkg/rotator/rotator_test.go @@ -335,6 +335,10 @@ func TestReconcileWebhook(t *testing.T) { }, }, { + // As InsecureSkipTLSVerify: true isn't compatible with caBundle + // we cover that the process sets InsecureSkipTLSVerify: false. + // If there is any issue patching InsecureSkipTLSVerify the test fails + // due InsecureSkipTLSVerify & caBundle incompatibility "apiservice-insecure-tls", APIService, nil, []string{"spec", "caBundle"}, &apiregistrationv1.APIService{ ObjectMeta: metav1.ObjectMeta{ Name: "v1alpha2.example.com", From 944182ba8fbb5bc8784a7481506b7a561175f246 Mon Sep 17 00:00:00 2001 From: Jorge Turrado Date: Wed, 7 Feb 2024 09:52:35 +0100 Subject: [PATCH 3/4] make replace optional Signed-off-by: Jorge Turrado --- pkg/rotator/rotator.go | 19 +++++++++++++------ pkg/rotator/rotator_test.go | 3 ++- 2 files changed, 15 insertions(+), 7 deletions(-) diff --git a/pkg/rotator/rotator.go b/pkg/rotator/rotator.go index 01e5eb6..15cfdb5 100644 --- a/pkg/rotator/rotator.go +++ b/pkg/rotator/rotator.go @@ -173,6 +173,7 @@ func AddRotator(mgr manager.Manager, cr *CertRotator) error { needLeaderElection: cr.RequireLeaderElection, refreshCertIfNeededDelegate: cr.refreshCertIfNeeded, fieldOwner: cr.FieldOwner, + removeInsecureSkipTLSVerify: cr.RemoveInsecureSkipTLSVerify, } if err := addController(mgr, reconciler); err != nil { return err @@ -247,6 +248,9 @@ type CertRotator struct { // CertName and Keyname override certificate path CertName string KeyName string + // RemoveInsecureSkipTLSVerify sets if InsecureSkipTLSVerify has to + // be removed from apiservices during the patch process + RemoveInsecureSkipTLSVerify bool certsMounted chan struct{} certsNotMounted chan struct{} @@ -387,7 +391,7 @@ func (cr *CertRotator) refreshCerts(refreshCA bool, secret *corev1.Secret) error return nil } -func injectCert(updatedResource *unstructured.Unstructured, certPem []byte, webhookType WebhookType) error { +func injectCert(updatedResource *unstructured.Unstructured, certPem []byte, webhookType WebhookType, removeInsecureSkipTLSVerify bool) error { switch webhookType { case Validating: return injectCertToWebhook(updatedResource, certPem) @@ -396,7 +400,7 @@ func injectCert(updatedResource *unstructured.Unstructured, certPem []byte, webh case CRDConversion: return injectCertToConversionWebhook(updatedResource, certPem) case APIService: - return injectCertToApiService(updatedResource, certPem) + return injectCertToApiService(updatedResource, certPem, removeInsecureSkipTLSVerify) case ExternalDataProvider: return injectCertToExternalDataProvider(updatedResource, certPem) } @@ -442,7 +446,7 @@ func injectCertToConversionWebhook(crd *unstructured.Unstructured, certPem []byt return nil } -func injectCertToApiService(apiService *unstructured.Unstructured, certPem []byte) error { +func injectCertToApiService(apiService *unstructured.Unstructured, certPem []byte, removeInsecureSkipTLSVerify bool) error { _, found, err := unstructured.NestedMap(apiService.Object, "spec") if err != nil { return err @@ -450,8 +454,10 @@ func injectCertToApiService(apiService *unstructured.Unstructured, certPem []byt if !found { return errors.New("`spec` field not found in APIService") } - if err := unstructured.SetNestedField(apiService.Object, false, "spec", "insecureSkipTLSVerify"); err != nil { - return err + if removeInsecureSkipTLSVerify { + if err := unstructured.SetNestedField(apiService.Object, false, "spec", "insecureSkipTLSVerify"); err != nil { + return err + } } if err := unstructured.SetNestedField(apiService.Object, base64.StdEncoding.EncodeToString(certPem), "spec", "caBundle"); err != nil { return err @@ -736,6 +742,7 @@ type ReconcileWH struct { ctx context.Context secretKey types.NamespacedName webhooks []WebhookInfo + removeInsecureSkipTLSVerify bool wasCAInjected *atomic.Bool needLeaderElection bool refreshCertIfNeededDelegate func() (bool, error) @@ -829,7 +836,7 @@ func (r *ReconcileWH) ensureCerts(certPem []byte) error { } log.Info("Ensuring CA cert", "name", webhook.Name, "gvk", gvk) - if err := injectCert(updatedResource, certPem, webhook.Type); err != nil { + if err := injectCert(updatedResource, certPem, webhook.Type, r.removeInsecureSkipTLSVerify); err != nil { log.Error(err, "Unable to inject cert to webhook.") anyError = err continue diff --git a/pkg/rotator/rotator_test.go b/pkg/rotator/rotator_test.go index 67e5af6..0f18dd2 100644 --- a/pkg/rotator/rotator_test.go +++ b/pkg/rotator/rotator_test.go @@ -401,7 +401,8 @@ func TestReconcileWebhook(t *testing.T) { Type: tt.webhookType, }, }, - FieldOwner: fieldOwner, + FieldOwner: fieldOwner, + RemoveInsecureSkipTLSVerify: true, } wh, ok := tt.webhookConfig.DeepCopyObject().(client.Object) if !ok { From 64b274ae6f51d3f1517e3603ce8b8979eb61c096 Mon Sep 17 00:00:00 2001 From: Jorge Turrado Date: Sun, 31 Mar 2024 17:40:12 +0200 Subject: [PATCH 4/4] add a test Signed-off-by: Jorge Turrado --- pkg/rotator/rotator.go | 2 +- pkg/rotator/rotator_test.go | 168 ++++++++++++++++++++++++++++++++++++ 2 files changed, 169 insertions(+), 1 deletion(-) diff --git a/pkg/rotator/rotator.go b/pkg/rotator/rotator.go index 15cfdb5..19c383f 100644 --- a/pkg/rotator/rotator.go +++ b/pkg/rotator/rotator.go @@ -788,7 +788,7 @@ func (r *ReconcileWH) Reconcile(ctx context.Context, request reconcile.Request) artifacts, err := buildArtifactsFromSecret(secret) if err != nil { crLog.Error(err, "secret is not well-formed, cannot update webhook configurations") - return reconcile.Result{}, nil + return reconcile.Result{}, err } // Ensure certs on webhooks diff --git a/pkg/rotator/rotator_test.go b/pkg/rotator/rotator_test.go index 0f18dd2..1cdf2e4 100644 --- a/pkg/rotator/rotator_test.go +++ b/pkg/rotator/rotator_test.go @@ -587,6 +587,174 @@ func TestNamespacedCache(t *testing.T) { wg.Wait() } +// Verifies that the rotator doesn't remove InsecureSkipTLSVerify when it's not flagged +func TestRemoveInsecureSkipTLSVerify(t *testing.T) { + testCases := []struct { + name string + apiservice apiregistrationv1.APIService + removeInsecureSkipTLSVerify bool + expectedResult bool + }{ + { + name: "remove-enabled-and-insecure-active", + apiservice: apiregistrationv1.APIService{ + ObjectMeta: metav1.ObjectMeta{ + Name: "v1alpha1.verify.com", + }, + Spec: apiregistrationv1.APIServiceSpec{ + Group: "verify.com", + GroupPriorityMinimum: 1, + Version: "v1alpha1", + VersionPriority: 1, + InsecureSkipTLSVerify: true, + Service: &apiregistrationv1.ServiceReference{ + Namespace: "kube-system", + Name: "example-api", + }, + }, + }, + removeInsecureSkipTLSVerify: true, + expectedResult: true, + }, + { + name: "remove-disabled-and-insecure-active", + apiservice: apiregistrationv1.APIService{ + ObjectMeta: metav1.ObjectMeta{ + Name: "v1alpha2.verify.com", + }, + Spec: apiregistrationv1.APIServiceSpec{ + Group: "verify.com", + GroupPriorityMinimum: 1, + Version: "v1alpha2", + VersionPriority: 1, + InsecureSkipTLSVerify: true, + Service: &apiregistrationv1.ServiceReference{ + Namespace: "kube-system", + Name: "example-api", + }, + }, + }, + removeInsecureSkipTLSVerify: false, + expectedResult: false, + }, + { + name: "remove-disabled-and-insecure-inactive", + apiservice: apiregistrationv1.APIService{ + ObjectMeta: metav1.ObjectMeta{ + Name: "v1alpha3.verify.com", + }, + Spec: apiregistrationv1.APIServiceSpec{ + Group: "verify.com", + GroupPriorityMinimum: 1, + Version: "v1alpha3", + VersionPriority: 1, + InsecureSkipTLSVerify: false, + Service: &apiregistrationv1.ServiceReference{ + Namespace: "kube-system", + Name: "example-api", + }, + }, + }, + removeInsecureSkipTLSVerify: false, + expectedResult: true, + }, + { + name: "remove-enabled-and-insecure-inactive", + apiservice: apiregistrationv1.APIService{ + ObjectMeta: metav1.ObjectMeta{ + Name: "v1alpha4.verify.com", + }, + Spec: apiregistrationv1.APIServiceSpec{ + Group: "verify.com", + GroupPriorityMinimum: 1, + Version: "v1alpha4", + VersionPriority: 1, + InsecureSkipTLSVerify: false, + Service: &apiregistrationv1.ServiceReference{ + Namespace: "kube-system", + Name: "example-api", + }, + }, + }, + removeInsecureSkipTLSVerify: true, + expectedResult: true, + }, + } + + for _, tt := range testCases { + ctx, cancelFunc := context.WithCancel(context.Background()) + defer cancelFunc() + g := gomega.NewWithT(t) + mgr := setupManager(g) + c := mgr.GetClient() + key := types.NamespacedName{Namespace: tt.name, Name: "cert"} + createSecret(ctx, g, c, key) + + rotator := &CertRotator{ + SecretKey: key, + RemoveInsecureSkipTLSVerify: tt.removeInsecureSkipTLSVerify, + FieldOwner: "test", + Webhooks: []WebhookInfo{ + { + Name: tt.apiservice.Name, + Type: APIService, + }, + }, + } + err := AddRotator(mgr, rotator) + g.Expect(err).NotTo(gomega.HaveOccurred(), "adding rotator") + + wg := StartTestManager(ctx, mgr, g) + + // The reader (cache) will be initialized in AddRotator. + g.Expect(rotator.reader).ToNot(gomega.BeNil()) + + // Wait for it to populate + if !rotator.reader.WaitForCacheSync(ctx) { + t.Fatal("waiting for cache to populate") + } + + // Wait for certificates to generated + ensureCertWasGenerated(ctx, g, c, key) + + wh := &tt.apiservice + err = c.Create(ctx, wh) + g.Expect(err).NotTo(gomega.HaveOccurred(), "creating apiservice") + + // Sleep 1 seconds to ensure the operation + time.Sleep(1 * time.Second) + + whu := &unstructured.Unstructured{} + err = c.Scheme().Convert(wh, whu, nil) + g.Expect(err).NotTo(gomega.HaveOccurred(), "cannot convert to webhook to unstructured") + + key = client.ObjectKeyFromObject(whu) + + if err := c.Get(ctx, key, whu); err != nil { + t.Fatal("recovering wh") + } + + webhooks := extractWebhooks(g, whu, nil) + for _, w := range webhooks { + _, found, err := unstructured.NestedFieldNoCopy(w.(map[string]interface{}), []string{"spec", "caBundle"}...) + if err != nil { + t.Fatal("error checking the caBundle status") + } + + if found && !tt.expectedResult { + t.Fatal("caBundle has been found but not found is the expected result") + } + if !found && tt.expectedResult { + t.Fatal("caBundle hasn¡t been found but found is the expected result") + } + } + + cancelFunc() + wg.Wait() + } + +} + func ensureCertWasGenerated(ctx context.Context, g *gomega.WithT, c client.Reader, key types.NamespacedName) { var secret corev1.Secret g.Eventually(func() bool {