From 0011fc9fb0dddcb0b971c4c41160c240ba2a42ab Mon Sep 17 00:00:00 2001
From: Marko Mikulicic <mkm@bitnami.com>
Date: Mon, 5 Aug 2019 22:53:21 +0200
Subject: [PATCH] Honour creation time during rotation

Instead of rotating every period seconds after the pod boots, take into consideration
the timestamp of the last secret and create the next one only when it gets stale.

Ref #190

Also tested by setting rotation period 5m and then periodically killing the pod
and checking that it indeed rotated the keys only after 5m.

```
while true; do ke -n kube-system get pod -lname=sealed-secrets-controller -oname |xargs ke -n kube-system delete; ke -n kube-system get secret -l sealedsecrets.bitnami.com/sealed-secrets-key=active; sleep 10s; done
```
---
 cmd/controller/keys.go      | 15 ++++++++++-
 cmd/controller/main.go      | 18 ++++++++-----
 cmd/controller/main_test.go | 54 +++++++++++++++++++++++++++++++++++++
 3 files changed, 79 insertions(+), 8 deletions(-)

diff --git a/cmd/controller/keys.go b/cmd/controller/keys.go
index 0a397f4e0..ffc6431e9 100644
--- a/cmd/controller/keys.go
+++ b/cmd/controller/keys.go
@@ -54,7 +54,19 @@ func readKey(secret v1.Secret) (*rsa.PrivateKey, []*x509.Certificate, error) {
 	}
 }
 
-func writeKey(client kubernetes.Interface, key *rsa.PrivateKey, certs []*x509.Certificate, namespace, label, prefix string) (string, error) {
+type writeKeyOpt func(*writeKeyOpts)
+type writeKeyOpts struct{ creationTime metav1.Time }
+
+func writeKeyWithCreationTime(t metav1.Time) writeKeyOpt {
+	return func(opts *writeKeyOpts) { opts.creationTime = t }
+}
+
+func writeKey(client kubernetes.Interface, key *rsa.PrivateKey, certs []*x509.Certificate, namespace, label, prefix string, optSetters ...writeKeyOpt) (string, error) {
+	var opts writeKeyOpts
+	for _, o := range optSetters {
+		o(&opts)
+	}
+
 	certbytes := []byte{}
 	for _, cert := range certs {
 		certbytes = append(certbytes, pem.EncodeToMemory(&pem.Block{Type: certUtil.CertificateBlockType, Bytes: cert.Raw})...)
@@ -66,6 +78,7 @@ func writeKey(client kubernetes.Interface, key *rsa.PrivateKey, certs []*x509.Ce
 			Labels: map[string]string{
 				label: "active",
 			},
+			CreationTimestamp: opts.creationTime,
 		},
 		Data: map[string][]byte{
 			v1.TLSPrivateKeyKey: pem.EncodeToMemory(&pem.Block{Type: keyutil.RSAPrivateKeyBlockType, Bytes: x509.MarshalPKCS1PrivateKey(key)}),
diff --git a/cmd/controller/main.go b/cmd/controller/main.go
index eeb81ebc7..f02b85922 100644
--- a/cmd/controller/main.go
+++ b/cmd/controller/main.go
@@ -117,12 +117,8 @@ func myNamespace() string {
 // A period of 0 disables automatic rotation, but manual rotation (e.g. triggered by SIGUSR1)
 // is still honoured.
 func initKeyRotation(registry *KeyRegistry, period time.Duration) (func(), error) {
-	// Create a new key only if it's the first key or if we have automatic key rotation.
-	// Since the rotation period might be longer than the average pod run time (eviction, updates, crashes etc)
-	// we err on the side of increased rotation frequency rather than overshooting the rotation goals.
-	//
-	// TODO(mkm): implement rotation cadence based on resource times rather than just an in-process timer.
-	if period != 0 || len(registry.keys) == 0 {
+	// Create a new key only if it's the first key.
+	if len(registry.keys) == 0 {
 		if _, err := registry.generateKey(); err != nil {
 			return nil, err
 		}
@@ -136,7 +132,15 @@ func initKeyRotation(registry *KeyRegistry, period time.Duration) (func(), error
 	if period == 0 {
 		return keyGenFunc, nil
 	}
-	return ScheduleJobWithTrigger(period, period, keyGenFunc), nil
+
+	// If key rotation is enabled, we'll rotate the key when the most recent
+	// key becomes stale (older than period).
+	mostRecentKeyAge := time.Since(registry.mostRecentKey.creationTime)
+	initialDelay := period - mostRecentKeyAge
+	if initialDelay < 0 {
+		initialDelay = 0
+	}
+	return ScheduleJobWithTrigger(initialDelay, period, keyGenFunc), nil
 }
 
 func initKeyGenSignalListener(trigger func()) {
diff --git a/cmd/controller/main_test.go b/cmd/controller/main_test.go
index f233f6c29..809fd0abc 100644
--- a/cmd/controller/main_test.go
+++ b/cmd/controller/main_test.go
@@ -159,6 +159,60 @@ func TestReuseKey(t *testing.T) {
 	}
 }
 
+func TestRotateStaleKey(t *testing.T) {
+	rand := testRand()
+	key, err := rsa.GenerateKey(rand, 512)
+	if err != nil {
+		t.Fatalf("Failed to generate test key: %v", err)
+	}
+
+	cert, err := signKey(rand, key)
+	if err != nil {
+		t.Fatalf("signKey failed: %v", err)
+	}
+
+	// we'll simulate the existence of a secret that is about to expire
+	// by making it old enough so that it's just "staleness" short of using
+	// the full rotation "period".
+	const (
+		period    = 20 * time.Second
+		staleness = 100 * time.Millisecond
+		oldAge    = period - staleness
+	)
+	client := fake.NewSimpleClientset()
+	_, err = writeKey(client, key, []*x509.Certificate{cert}, "namespace", SealedSecretsKeyLabel, "prefix",
+		writeKeyWithCreationTime(metav1.NewTime(time.Now().Add(-oldAge))))
+	if err != nil {
+		t.Errorf("writeKey() failed with: %v", err)
+	}
+
+	registry, err := initKeyRegistry(client, rand, "namespace", "prefix", SealedSecretsKeyLabel, 1024)
+	if err != nil {
+		t.Fatalf("initKeyRegistry() returned err: %v", err)
+	}
+
+	_, err = initKeyRotation(registry, period)
+	if err != nil {
+		t.Fatalf("initKeyRotation() returned err: %v", err)
+	}
+
+	client.ClearActions()
+
+	maxWait := 1 * time.Second
+	endTime := time.Now().Add(maxWait)
+	successful := false
+	for time.Now().Before(endTime) {
+		time.Sleep(50 * time.Millisecond)
+		if hasAction(client, "create", "secrets") {
+			successful = true
+			break
+		}
+	}
+	if !successful {
+		t.Errorf("trigger function failed to activate early key generation")
+	}
+}
+
 func writeLegacyKey(client kubernetes.Interface, key *rsa.PrivateKey, certs []*x509.Certificate, namespace, name string) (string, error) {
 	certbytes := []byte{}
 	for _, cert := range certs {