Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Watch cert files and reload on change #141

Merged
merged 10 commits into from
Mar 6, 2024
101 changes: 101 additions & 0 deletions admission-webhook/cert_reloader.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,101 @@
package main

import (
"crypto/tls"
"sync"

"github.com/fsnotify/fsnotify"
"github.com/sirupsen/logrus"
)

type CertLoader interface {
CertPath() string
KeyPath() string
LoadCertificate() (*tls.Certificate, error)
}

type CertReloader struct {
sync.Mutex
certPath string
keyPath string
certificate *tls.Certificate
}

func NewCertReloader(certPath, keyPath string) *CertReloader {
return &CertReloader{
certPath: certPath,
keyPath: keyPath,
}
}

func (cr *CertReloader) CertPath() string {
return cr.certPath
}

func (cr *CertReloader) KeyPath() string {
return cr.keyPath
}

// LoadCertificate loads or reloads the certificate from disk.
func (cr *CertReloader) LoadCertificate() (*tls.Certificate, error) {
cr.Lock()
defer cr.Unlock()

cert, err := tls.LoadX509KeyPair(cr.certPath, cr.keyPath)
if err != nil {
return nil, err
}
cr.certificate = &cert
return cr.certificate, nil
}

// GetCertificateFunc returns a function that can be assigned to tls.Config.GetCertificate
func (cr *CertReloader) GetCertificateFunc() func(*tls.ClientHelloInfo) (*tls.Certificate, error) {
return func(chi *tls.ClientHelloInfo) (*tls.Certificate, error) {
return cr.certificate, nil
}
}

func watchCertFiles(certLoader CertLoader) {
watcher, err := fsnotify.NewWatcher()
if err != nil {
logrus.Errorf("error creating watcher: %v", err)
}

go func() {
defer watcher.Close()

for {
select {
case event, ok := <-watcher.Events:
if !ok {
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")
}
}
case err, ok := <-watcher.Errors:
if !ok {
logrus.Errorf("watcher error returned !ok: %v", err)
return
jsturtevant marked this conversation as resolved.
Show resolved Hide resolved
}
logrus.Errorf("watcher error: %v", err)
}
}
}()

err = watcher.Add(certLoader.CertPath())
if err != nil {
logrus.Fatalf("error watching certificate file: %v", err)
}
err = watcher.Add(certLoader.KeyPath())
if err != nil {
logrus.Fatalf("error watching key file: %v", err)
}
}
114 changes: 114 additions & 0 deletions admission-webhook/cert_reloader_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,114 @@
package main

import (
"crypto/tls"
"os"
"testing"

"github.com/stretchr/testify/assert"
)

// TestCertReloader tests the reloading functionality of the certificate.
func TestCertReloader(t *testing.T) {
// Create temporary cert and key files
tmpCertFile, err := os.CreateTemp("", "cert*.pem")
if err != nil {
t.Fatalf("Failed to create temp cert file: %v", err)
}
defer os.Remove(tmpCertFile.Name()) // clean up

tmpKeyFile, err := os.CreateTemp("", "key*.pem")
if err != nil {
t.Fatalf("Failed to create temp key file: %v", err)
}
defer os.Remove(tmpKeyFile.Name()) // clean up

// Write initial cert and key to temp files
initialCertData, _ := os.ReadFile("testdata/cert.pem")
if err := os.WriteFile(tmpCertFile.Name(), initialCertData, 0644); err != nil {
t.Fatalf("Failed to write to temp cert file: %v", err)
}

initialKeyData, _ := os.ReadFile("testdata/key.pem")
if err := os.WriteFile(tmpKeyFile.Name(), initialKeyData, 0644); err != nil {
t.Fatalf("Failed to write to temp key file: %v", err)
}

// Setup CertReloader with temp files
certReloader := NewCertReloader(tmpCertFile.Name(), tmpKeyFile.Name())
_, err = certReloader.LoadCertificate()
if err != nil {
t.Fatalf("Failed to load initial certificate: %v", err)
}

// Mocking a certificate change by writing new data to the files
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)
}

// Simulate reloading
_, err = certReloader.LoadCertificate()
if err != nil {
t.Fatalf("Failed to reload certificate: %v", err)
}
}

type mockCertLoader struct {
certPath string
keyPath string
loadCertFunc func() (*tls.Certificate, error)
}

func (m *mockCertLoader) CertPath() string {
return m.certPath
}

func (m *mockCertLoader) KeyPath() string {
return m.keyPath
}

func (m *mockCertLoader) LoadCertificate() (*tls.Certificate, error) {
return m.loadCertFunc()
}

func TestWatchingCertFiles(t *testing.T) {
tmpCertFile, err := os.CreateTemp("", "cert*.pem")
if err != nil {
t.Fatalf("Failed to create temp cert file: %v", err)
}
defer os.Remove(tmpCertFile.Name())

tmpKeyFile, err := os.CreateTemp("", "key*.pem")
if err != nil {
t.Fatalf("Failed to create temp key file: %v", err)
}
defer os.Remove(tmpKeyFile.Name())

loadCertFuncChan := make(chan bool)

cl := &mockCertLoader{
certPath: tmpCertFile.Name(),
keyPath: tmpKeyFile.Name(),
loadCertFunc: func() (*tls.Certificate, error) {
loadCertFuncChan <- true
return &tls.Certificate{}, nil
},
}

go func() {
defer close(loadCertFuncChan)

called := <-loadCertFuncChan
assert.True(t, called)
}()

watchCertFiles(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
}
1 change: 1 addition & 0 deletions admission-webhook/go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ module github.com/kubernetes-sigs/windows-gmsa/admission-webhook
go 1.21

require (
github.com/fsnotify/fsnotify v1.7.0
github.com/mitchellh/go-homedir v1.1.0
github.com/sirupsen/logrus v1.9.3
github.com/stretchr/testify v1.8.4
Expand Down
2 changes: 2 additions & 0 deletions admission-webhook/go.sum
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@ github.com/davecgh/go-spew v1.1.1 h1:vj9j/u1bqnvCEfJOwUhtlOARqs3+rkHYY13jYWTU97c
github.com/davecgh/go-spew v1.1.1/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38=
github.com/emicklei/go-restful/v3 v3.11.0 h1:rAQeMHw1c7zTmncogyy8VvRZwtkmkZ4FxERmMY4rD+g=
github.com/emicklei/go-restful/v3 v3.11.0/go.mod h1:6n3XBCmQQb25CM2LCACGz8ukIrRry+4bhvbpWn3mrbc=
github.com/fsnotify/fsnotify v1.7.0 h1:8JEhPFa5W2WU7YfeZzPNqzMP6Lwt7L2715Ggo0nosvA=
github.com/fsnotify/fsnotify v1.7.0/go.mod h1:40Bi/Hjc2AVfZrqy+aj+yEI+/bRxZnMJyTJwOpGvigM=
github.com/go-logr/logr v1.3.0 h1:2y3SDp0ZXuc6/cjLSZ+Q3ir+QB9T/iG5yYRXqsagWSY=
github.com/go-logr/logr v1.3.0/go.mod h1:9T104GzyrTigFIr8wt5mBrctHMim0Nb2HLGrmQ40KvY=
github.com/go-openapi/jsonpointer v0.19.6/go.mod h1:osyAmYz/mB/C3I+WsTTSgw1ONzaLJoLCyoi6/zppojs=
Expand Down
39 changes: 39 additions & 0 deletions admission-webhook/integration_tests/integration_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package integrationtests

import (
"context"
"encoding/base64"
"encoding/json"
"fmt"
"html/template"
Expand Down Expand Up @@ -365,6 +366,42 @@ func TestDeployV1CredSpecGetAllVersions(t *testing.T) {
assert.Equal(t, v1alpha1CredSpec.Object["credSpec"], v1CredSpec.Object["credSpec"])
}

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
*/
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)
defer tearDownFunc()

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)

// apply the new cert & key pair
renderedTemplate := renderTemplate(t, testConfig, newSecretTemplate)
success, _, _ := applyManifest(t, renderedTemplate)
assert.True(t, success)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this ensures the secret got applied correctly, Do we also need to check that the webhook got the secret and is still running?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i need some pointers for this. what's a good way to check the webhook got the secret and is still running?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a bit involved since we need to:

As a bonus (maybe seperate test), we could also verify that requests to the webhook always return during this process. I.e we don't drop requests during rotation.

This is a bit involved so if you want to skip it, we can resolve the remaining comments, merge the PR and I can do a follow up after but if you up for the challenge that works too

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i'm going to skip it in the interest of getting this PR merged so my colleague can move forward with his task. i'm a slow coder and don't want to hold everyone up. really appreciate the detailed pointers getting me this far and your willingness to do the follow-up 🙏

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could you add a TODO with the comments about for this test? Then I think we are good to go.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just pushed up the comments. formatting is somehow screwed up. this also removed your lgtm

}

/* Helpers */

type testConfig struct {
Expand All @@ -378,6 +415,8 @@ type testConfig struct {
RoleBindingName string
Image string
ExtraSpecLines []string
Cert string
Key string
}

// integrationTestSetup creates a new namespace to play in, and returns a function to
Expand Down
8 changes: 8 additions & 0 deletions admission-webhook/integration_tests/templates/new-secret.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
apiVersion: v1
kind: Secret
metadata:
name: {{ .TestName }}
namespace: {{ .Namespace }}
data:
tls_private_key: {{ .Key }}
tls_certificate: {{ .Cert }}
6 changes: 5 additions & 1 deletion admission-webhook/main.go
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package main

import (
"flag"
"fmt"
"os"
"strconv"
Expand All @@ -13,12 +14,15 @@ import (
func main() {
initLogrus()

enableCertReload := flag.Bool("cert-reload", false, "enable certificate reload")
flag.Parse()

kubeClient, err := createKubeClient()
if err != nil {
panic(err)
}

webhook := newWebhook(kubeClient)
webhook := newWebhookWithOptions(kubeClient, WithCertReload(*enableCertReload))

tlsConfig := &tlsConfig{
crtPath: env("TLS_CRT"),
Expand Down
29 changes: 29 additions & 0 deletions admission-webhook/testdata/cert.pem
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
-----BEGIN CERTIFICATE-----
MIIFCTCCAvGgAwIBAgIUDlTAXZUZ0DGcutGxcspszdn4lAowDQYJKoZIhvcNAQEL
BQAwFDESMBAGA1UEAwwJbG9jYWxob3N0MB4XDTI0MDIwODA2MDAyN1oXDTM0MDIw
NTA2MDAyN1owFDESMBAGA1UEAwwJbG9jYWxob3N0MIICIjANBgkqhkiG9w0BAQEF
AAOCAg8AMIICCgKCAgEAg1kVSkS0tl8+VMBH8nVwfXy3yNgDS5a8uWAFv62O1vI4
KEOClNhPaFvihL7ubi3rhCmHoUfbIhe1MMqwV7mEXI+AVObs7Sf8feX5WkW6zJyF
AByH8O37fGFfEsISJBcMwmS5/gCShPfUuG2vr7Zg28brxU+Ixp1DhA87X+A+CEG0
JTH2LDiKMv86At/olH/IeDOH7j2tD25MThDN+xyKa9u2cpy73GcF822haUtFzmVX
mA8Mw4Qs5B2lMPY3k/C2UaqRDnNFu+0U011hvAGFA4+Jw4Cvpy13/4kQQZ0JHSOD
oy+jtbpqMQJn2oCMQ9DX0WQTS7E4W03y5gKS4v8xkUneAWuWoSwTm8TXRoAXbT3n
ZqDXmdy69ckLiLgn/w5uAeKjeHdk522QiJ2MHqYRLJbzUQ6LsrYdcR3nhh1pgh5K
tdnuz7HQtg77KR9g1X1aAT20SqV9rV85FwWI50dTfg8ehWXOSuXlZMlRUuMOn0a0
iAyw+rCbaLvmuXwPZxuk/PPW+4lWwE1OqHSjs3iHl/fZM5AfmVS9Av3n3JRD2hrA
2aoOnjiFSjI/9qzcjUl8LTvrzGFt3QWcVRDzMqNQW8qPvBFvxrRcZlPTElRM023p
TO8P3k4n08EGgY+dV9s3xC6dnIkyVp7b2UtEDAC6E53mI2e+wG5uYrKu4QSaM9MC
AwEAAaNTMFEwHQYDVR0OBBYEFCEw8jWYUa8ed+R5O9dxhUSVqBJwMB8GA1UdIwQY
MBaAFCEw8jWYUa8ed+R5O9dxhUSVqBJwMA8GA1UdEwEB/wQFMAMBAf8wDQYJKoZI
hvcNAQELBQADggIBAAE7F7qZ4B5PzqydYRJ0Er39kdfEugs0L3/LYwHfQ4eJKedI
CNXnSW+2kh5kQW9YnXy77VewZ/wtaZyGndNNocRKlN6X2yr97HOtuysdSvHuNmUl
Dyk9brgPcRJK4YizO44DHuQmn0LUhxbeph2VjYxs6B5XEdD6aGFpljWNCHzuWqao
so3uF588lhudSPVkx9VEWHF/N5BKQeJr6gPy1BB8rlSkD8ImxHmq7ledV7ri0mCS
o5WO+17kaLBvyj5H8sN/M+zWPKMHLohe5BXFWwlgl8nGnaXNW0HaKhgyjGTZBZJe
u5kTVQnhTDUo706K4BC8Zz78L6Xcb44FMUIRF5yZ8iKN2M6mPmEQmrE6aKEmiNxc
j0Yfz5MGumog8goYEgOkxp49aI6zojOq7GskDVKo9NxPsfotASriDOhpe106F/yK
cboL1oeL3pAjgICgwdy2pNawjDVNt8cadU4RAxF+m0gpa/xAsGhlz5YKsdOv/7V8
Lb7KguUyYHmyBFwzJluhBrUWrGpPEKxdjrMbn/9G8b7AbXyV2w/9bwqkLvFU6qyJ
vuv4HpHIywsm9tST8p62RDVRbWlYfBwWIsnz4sraOPJXt8SU9QE3XI3MLw3iiEbs
1oToxKEj+6KbAgaC81cIkohZ+6RYnX+huhhe89Mgg0r7wWnt+huHiKLhGnm/
-----END CERTIFICATE-----
Loading
Loading