diff --git a/pkg/rotator/rotator.go b/pkg/rotator/rotator.go index dc00c85..19c383f 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,6 +454,11 @@ func injectCertToApiService(apiService *unstructured.Unstructured, certPem []byt if !found { return errors.New("`spec` field not found in APIService") } + 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 } @@ -733,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) @@ -778,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 @@ -826,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 441bae8..1cdf2e4 100644 --- a/pkg/rotator/rotator_test.go +++ b/pkg/rotator/rotator_test.go @@ -334,6 +334,28 @@ 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", + }, + 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{ @@ -379,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 { @@ -564,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 {