diff --git a/.github/workflows/build.yaml b/.github/workflows/build.yaml index f0585ab6..812bcd4c 100644 --- a/.github/workflows/build.yaml +++ b/.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 + diff --git a/admission-webhook/cert_reloader.go b/admission-webhook/cert_reloader.go index 6580fe7c..cfee8657 100644 --- a/admission-webhook/cert_reloader.go +++ b/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,16 +71,15 @@ 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 { @@ -86,6 +87,9 @@ func watchCertFiles(certLoader CertLoader) { return } logrus.Errorf("watcher error: %v", err) + case <-ctx.Done(): + logrus.Info("stopping certificate watcher") + return } } }() diff --git a/admission-webhook/cert_reloader_test.go b/admission-webhook/cert_reloader_test.go index 463a2c44..f79c2556 100644 --- a/admission-webhook/cert_reloader_test.go +++ b/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 } diff --git a/admission-webhook/integration_tests/integration_test.go b/admission-webhook/integration_tests/integration_test.go index 6d5c8e3d..28c476a5 100644 --- a/admission-webhook/integration_tests/integration_test.go +++ b/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 */ diff --git a/admission-webhook/make/helm.mk b/admission-webhook/make/helm.mk index e3d4a04a..84ce8979 100644 --- a/admission-webhook/make/helm.mk +++ b/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 diff --git a/admission-webhook/webhook.go b/admission-webhook/webhook.go index 06f772d4..7c2244bd 100644 --- a/admission-webhook/webhook.go +++ b/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(), diff --git a/charts/gmsa/templates/issuer.yaml b/charts/gmsa/templates/issuer.yaml index bb198a63..6893c782 100644 --- a/charts/gmsa/templates/issuer.yaml +++ b/charts/gmsa/templates/issuer.yaml @@ -13,6 +13,10 @@ 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 @@ -20,6 +24,29 @@ 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 -}} \ No newline at end of file diff --git a/charts/gmsa/templates/mutatingwebhook.yaml b/charts/gmsa/templates/mutatingwebhook.yaml index 53510fbc..b8a55340 100644 --- a/charts/gmsa/templates/mutatingwebhook.yaml +++ b/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: diff --git a/charts/gmsa/templates/validatingwebhook.yaml b/charts/gmsa/templates/validatingwebhook.yaml index ea079c0b..41e7c478 100644 --- a/charts/gmsa/templates/validatingwebhook.yaml +++ b/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: