Merge pull request #145 from jsturtevant/cert-rotation-e2e-chart

Cert rotation for e2e chart

.github/workflows/build.yaml

@@ -113,3 +113,18 @@ jobs:
       env:
         T: integration
         DEPLOY_METHOD: chart
+  integration-rotation-enabled:
+    runs-on: ubuntu-20.04
+    steps:
+    - uses: actions/checkout@v2
+    - name: Set up Go
+      uses: actions/setup-go@v2
+      with:
+        go-version: '1.21'
+    - id: test-runner
+      uses: ./.github/actions/tests
+      env:
+        T: integration
+        DEPLOY_METHOD: chart
+        HELM_INSTALL_FLAGS_FLAGS: --set certificates.certReload.enabled=true
+

admission-webhook/cert_reloader.go

@@ -1,6 +1,7 @@
 package main
 
 import (
+	"context"
 	"crypto/tls"
 	"sync"
 
@@ -56,7 +57,8 @@ func (cr *CertReloader) GetCertificateFunc() func(*tls.ClientHelloInfo) (*tls.Ce
 	}
 }
 
-func watchCertFiles(certLoader CertLoader) {
+func watchCertFiles(ctx context.Context, certLoader CertLoader) {
+	logrus.Infof("Starting certificate watcher on path %v and %v", certLoader.CertPath(), certLoader.KeyPath())
 	watcher, err := fsnotify.NewWatcher()
 	if err != nil {
 		logrus.Errorf("error creating watcher: %v", err)
@@ -69,23 +71,25 @@ func watchCertFiles(certLoader CertLoader) {
 			select {
 			case event, ok := <-watcher.Events:
 				if !ok {
+					logrus.Errorf("watcher events returned !ok: %v", err)
 					return
 				}
-				if event.Op&fsnotify.Write == fsnotify.Write || event.Op&fsnotify.Rename == fsnotify.Rename {
-					logrus.Infof("detected change in certificate file: %v", event.Name)
-					_, err := certLoader.LoadCertificate()
-					if err != nil {
-						logrus.Errorf("error reloading certificate: %v", err)
-					} else {
-						logrus.Infof("successfully reloaded certificate")
-					}
+				logrus.Infof("detected change in certificate file: %v", event.Name)
+				_, err := certLoader.LoadCertificate()
+				if err != nil {
+					logrus.Errorf("error reloading certificate: %v", err)
+				} else {
+					logrus.Infof("successfully reloaded certificate")
 				}
 			case err, ok := <-watcher.Errors:
 				if !ok {
 					logrus.Errorf("watcher error returned !ok: %v", err)
 					return
 				}
 				logrus.Errorf("watcher error: %v", err)
+			case <-ctx.Done():
+				logrus.Info("stopping certificate watcher")
+				return
 			}
 		}
 	}()

admission-webhook/cert_reloader_test.go

@@ -1,6 +1,7 @@
 package main
 
 import (
+	"context"
 	"crypto/tls"
 	"os"
 	"testing"
@@ -86,6 +87,7 @@ func TestWatchingCertFiles(t *testing.T) {
 	defer os.Remove(tmpKeyFile.Name())
 
 	loadCertFuncChan := make(chan bool)
+	done := make(chan bool)
 
 	cl := &mockCertLoader{
 		certPath: tmpCertFile.Name(),
@@ -97,18 +99,19 @@ func TestWatchingCertFiles(t *testing.T) {
 	}
 
 	go func() {
-		defer close(loadCertFuncChan)
-
 		called := <-loadCertFuncChan
 		assert.True(t, called)
+		done <- true
 	}()
 
-	watchCertFiles(cl)
+	ctx, cancel := context.WithCancel(context.Background())
+	defer cancel()
+	watchCertFiles(ctx, cl)
 
 	newCertData, _ := os.ReadFile("testdata/cert.pem")
 	if err := os.WriteFile(tmpCertFile.Name(), newCertData, 0644); err != nil {
 		t.Fatalf("Failed to write new data to cert file: %v", err)
 	}
 
-	<-loadCertFuncChan
+	<-done
 }

admission-webhook/integration_tests/integration_test.go

@@ -2,7 +2,6 @@ package integrationtests
 
 import (
 	"context"
-	"encoding/base64"
 	"encoding/json"
 	"fmt"
 	"html/template"
@@ -11,7 +10,9 @@ import (
 	"path"
 	"reflect"
 	"strconv"
+	"strings"
 	"testing"
+	"time"
 
 	"github.com/stretchr/testify/assert"
 	corev1 "k8s.io/api/core/v1"
@@ -368,20 +369,30 @@ func TestDeployV1CredSpecGetAllVersions(t *testing.T) {
 
 func TestPossibleToUpdatePodWithNewCert(t *testing.T) {
 	/** TODO:
-		 * - update the webhook pod to use the new flag
-	     * - make a request to create a pod to make sure it works (already done)
-	     * - get a blessed certificate from the API server
-		 *   (https://github.com/kubernetes-sigs/windows-gmsa/blob/141/admission-webhook/deploy/create-signed-cert.sh)
-	     * - update existing secret in place and wait for the pod to get new secrets which can take time
-		 *   (https://kubernetes.io/docs/concepts/configuration/secret/#using-secrets-as-files-from-a-pod) - similar to what you are doing here
-	     * - kubectl exec into the running pod to see that the secret changed
-		 *   (using utils like https://github.com/ycheng-kareo/windows-gmsa/blob/watch-reload-cert/admission-webhook/integration_tests/kube.go#L199)
-	     * - make a request to create a pod to verify that it still works (pod := waitForPodToComeUp(t, testConfig.Namespace, "app="+testName))
-		 * - add a separate test to verify that requests to the webhook always return during this process
-	*/
+	 * - add a separate test to verify that requests to the webhook always return during this process
+	 */
+
+	webHookNs := os.Getenv("NAMESPACE")
+	webHookDeploymentName := os.Getenv("DEPLOYMENT_NAME")
+	webhook, err := kubeClient(t).AppsV1().Deployments(webHookNs).Get(context.Background(), webHookDeploymentName, metav1.GetOptions{})
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	rotationEnabled := false
+	for arg := range webhook.Spec.Template.Spec.Containers[0].Args {
+		if strings.Contains(webhook.Spec.Template.Spec.Containers[0].Args[arg], "--cert-reload=true") {
+			rotationEnabled = true
+		}
+	}
+
+	if !rotationEnabled {
+		t.Skip("Skipping test as rotation is not enabled")
+	}
+
 	testName := "possible-to-update-pod-with-new-cert"
 	credSpecTemplates := []string{"credspec-0"}
-	newSecretTemplate := "new-secret"
+
 	templates := []string{"credspecs-users-rbac-role", "service-account", "sa-rbac-binding", "single-pod-with-container-level-gmsa"}
 
 	testConfig, tearDownFunc := integrationTestSetup(t, testName, credSpecTemplates, templates)
@@ -390,16 +401,66 @@ func TestPossibleToUpdatePodWithNewCert(t *testing.T) {
 	pod := waitForPodToComeUp(t, testConfig.Namespace, "app="+testName)
 	assert.Equal(t, expectedCredSpec0, extractContainerCredSpecContents(t, pod, testName))
 
-	// read test cert & key
-	newCert, _ := os.ReadFile("../testdata/cert.pem")
-	newKey, _ := os.ReadFile("../testdata/key.pem")
-	testConfig.Cert = base64.StdEncoding.EncodeToString(newCert)
-	testConfig.Key = base64.StdEncoding.EncodeToString(newKey)
+	deployMethod := os.Getenv("DEPLOY_METHOD")
+	if deployMethod == "chart" {
+		runCommandOrFail(t, "cmctl", "renew", webHookDeploymentName, "-n", webHookNs)
 
-	// apply the new cert & key pair
-	renderedTemplate := renderTemplate(t, testConfig, newSecretTemplate)
-	success, _, _ := applyManifest(t, renderedTemplate)
-	assert.True(t, success)
+		var (
+			timeout = 30 * time.Second
+			start   = time.Now()
+		)
+
+		for {
+			success, stdout, stderr := runKubectlCommand(t, "get", "certificaterequest", webHookNs+"-2", "--namespace", webHookNs, "-o", "jsonpath='{.status.conditions[?(@.type==\"Ready\")].status}'")
+			if !success {
+				fmt.Printf("Warning: command failed with %s, %s\n", stdout, stderr)
+				continue
+			}
+
+			if stdout == "'True'" {
+				break
+			} else {
+				fmt.Printf("Warning: status was %s", stdout)
+			}
+
+			if time.Since(start) >= timeout {
+				t.Fatal("Timeout: Unable to get the certificate request status")
+			}
+
+			time.Sleep(1 * time.Second)
+		}
+	} else {
+		/** TODO:
+		     * - get a blessed certificate from the API server
+			 *   (https://github.com/kubernetes-sigs/windows-gmsa/blob/141/admission-webhook/deploy/create-signed-cert.sh)
+			 *   runCommandOrFail(t, fmt."create-signed-cert.sh --service $NAME --namespace $NAMESPACE --certs-dir $CERTS_DIR", testConfig.Namespace)
+		     * - update existing secret in place and wait for the pod to get new secrets which can take time
+			 *   (https://kubernetes.io/docs/concepts/configuration/secret/#using-secrets-as-files-from-a-pod) - similar to what you are doing here
+		     * - kubectl exec into the running pod to see that the secret changed
+			 *   (using utils like https://github.com/ycheng-kareo/windows-gmsa/blob/watch-reload-cert/admission-webhook/integration_tests/kube.go#L199)
+		**/
+
+		t.Skip("Non chart deployment method not supported for this test")
+	}
+
+	// it takes ~60 seconds for the webhook to pick up the new certificate
+	// so this first run makes sure the old cert still works
+	testName2 := testName + "after-rotation"
+	testConfig2, tearDownFunc2 := integrationTestSetup(t, testName2, credSpecTemplates, templates)
+	defer tearDownFunc2()
+
+	pod2 := waitForPodToComeUp(t, testConfig2.Namespace, "app="+testName2)
+	assert.Equal(t, expectedCredSpec0, extractContainerCredSpecContents(t, pod2, testName2))
+
+	// sleep a bit to ensure the the secret has been propagated to the pod
+	time.Sleep(90 * time.Second)
+
+	testName3 := testName + "after-rotation-propagated"
+	testConfig3, tearDownFunc3 := integrationTestSetup(t, testName3, credSpecTemplates, templates)
+	defer tearDownFunc3()
+
+	pod3 := waitForPodToComeUp(t, testConfig3.Namespace, "app="+testName3)
+	assert.Equal(t, expectedCredSpec0, extractContainerCredSpecContents(t, pod3, testName3))
 }
 
 /* Helpers */

admission-webhook/make/helm.mk

@@ -9,6 +9,10 @@ endif
 install-helm:
 	curl https://raw.githubusercontent.com/helm/helm/master/scripts/get-helm-3 | bash
 
+.PHONY: install-cmctl
+install-cmctl:
+	go install github.com/cert-manager/cmctl/v2@latest
+
 .PHONY: helm-chart
 helm-chart:
 	$(HELM) package ../charts/gmsa -d ../charts/repo/
@@ -36,8 +40,10 @@ remove_chart:
 # the deploy script as documented in the README, using $K8S_GMSA_DEPLOY_CHART_REPO and
 # $K8S_GMSA_DEPLOY_CHART_VERSION env variables to build the download URL. If VERSION is
 # not set then latest is used.
+# the HELM_INSTALL_FLAGS_FLAGS env var can be set to eg run only specific tests, e.g.:
+# HELM_INSTALL_FLAGS_FLAGS='--set certificates.certReload.enabled=true' make deploy_chart
 .PHONY: _deploy_chart
-_deploy_chart:  _copy_image _deploy_certmanager
+_deploy_chart:  _copy_image _deploy_certmanager install-cmctl
 ifeq ($(K8S_GMSA_CHART),)
 	@ echo "Cannot call target $@ without setting K8S_GMSA_CHART"
 	exit 1

admission-webhook/webhook.go

@@ -104,13 +104,14 @@ func (webhook *webhook) start(port int, tlsConfig *tlsConfig, listeningChan chan
 		err = webhook.server.Serve(keepAliveListener)
 	} else {
 		if webhook.config.EnableCertReload {
+			logrus.Infof("Webhook certificate reload enabled")
 			certReloader := NewCertReloader(tlsConfig.crtPath, tlsConfig.keyPath)
 			_, err = certReloader.LoadCertificate()
 			if err != nil {
 				return err
 			}
 
-			go watchCertFiles(certReloader)
+			go watchCertFiles(context.Background(), certReloader)
 
 			webhook.server.TLSConfig = &tls.Config{
 				GetCertificate: certReloader.GetCertificateFunc(),

charts/gmsa/templates/issuer.yaml

@@ -13,13 +13,40 @@ spec:
     kind: Issuer
     name: {{ .Release.Name }}
   secretName: {{ .Values.certificates.secretName }}
+  {{- if .Values.certificates.certReload.enabled }}
+  privateKey:
+    rotationPolicy: Always
+  {{- end }}
 ---
 {{ template "cert-manager.apiversion" . }}
 kind: Issuer
 metadata:
   name: {{ .Release.Name }}
   namespace: {{ .Release.Namespace }}
   labels: {{ include "gmsa.chartref" . | nindent 4 }}
+spec:
+  ca:
+    secretName: {{ .Release.Name }}-root-ca
+---
+{{ template "cert-manager.apiversion" . }}
+kind: ClusterIssuer
+metadata:
+  name: {{ .Release.Name }}-ca
 spec:
   selfSigned: {}
-{{- end -}}
+---
+{{ template "cert-manager.apiversion" . }}
+kind: Certificate
+metadata:
+  name: {{ .Release.Name }}-ca
+  namespace: {{ .Release.Namespace }}
+spec:
+  isCA: true
+  commonName: {{ .Release.Name }}-ca
+  secretName: {{ .Release.Name }}-root-ca
+  issuerRef:
+    name: {{ .Release.Name }}-ca
+    kind: ClusterIssuer
+    group: cert-manager.io
+---
+{{- end -}}

charts/gmsa/templates/mutatingwebhook.yaml

@@ -4,7 +4,7 @@ metadata:
   name: {{ .Release.Name }}
   {{- if .Values.certificates.certManager.enabled }}
   annotations:
-    cert-manager.io/inject-ca-from: {{ .Release.Namespace }}/{{ .Release.Name }}
+    cert-manager.io/inject-ca-from: {{ .Release.Namespace }}/{{ .Release.Name }}-ca
   {{- end }}
   labels: {{ include "gmsa.chartref" . | nindent 4 }}
 webhooks:

charts/gmsa/templates/validatingwebhook.yaml

@@ -4,7 +4,7 @@ metadata:
   name: {{ .Release.Name }}
   {{- if .Values.certificates.certManager.enabled }}
   annotations:
-    cert-manager.io/inject-ca-from: {{ .Release.Namespace }}/{{ .Release.Name }}
+    cert-manager.io/inject-ca-from: {{ .Release.Namespace }}/{{ .Release.Name }}-ca
   {{- end }}
   labels: {{ include "gmsa.chartref" . | nindent 4 }}
 webhooks: