Merge #214

214: Refactor key book-keeping r=mkmik a=mkmik

Prepares the ground for addressing #185 and #199 without adding any feature.

The only small side effect is that now private keys are attempted in random order (Go map iteration order).
This could only affect users who have turned on the key rotation feature which, as mentioned in #137, is not yet ready for prime time in the first place.
Also improves on #190.

Co-authored-by: Marko Mikulicic <[email protected]>

cmd/controller/controller.go

@@ -360,8 +360,9 @@ func (c *Controller) attemptUnseal(ss *ssv1alpha1.SealedSecret) (*corev1.Secret,
 }
 
 func attemptUnseal(ss *ssv1alpha1.SealedSecret, keyRegistry *KeyRegistry) (*corev1.Secret, error) {
-	for _, privKey := range keyRegistry.privateKeys {
-		if secret, err := ss.Unseal(scheme.Codecs, privKey); err == nil {
+	// TODO(mkm): embed the pubkey fingerprint in each sealed item so we can fetch the right key
+	for _, key := range keyRegistry.keys {
+		if secret, err := ss.Unseal(scheme.Codecs, key.private); err == nil {
 			return secret, nil
 		}
 	}

cmd/controller/funcs.go

@@ -10,10 +10,10 @@ const (
 	compromised = "compromised"
 )
 
-// ScheduleJobWithTrigger creates a long-running loop that runs a job each
-// loop
-// returns a trigger function that runs the job early when called
-func ScheduleJobWithTrigger(period time.Duration, job func()) func() {
+// ScheduleJobWithTrigger creates a long-running loop that runs a job after an initialDelay
+// and then after each period duration.
+// It returns a trigger function that runs the job early when called.
+func ScheduleJobWithTrigger(initialDelay, period time.Duration, job func()) func() {
 	trigger := make(chan struct{})
 	go func() {
 		for {
@@ -22,9 +22,10 @@ func ScheduleJobWithTrigger(period time.Duration, job func()) func() {
 		}
 	}()
 	go func() {
+		time.Sleep(initialDelay)
 		for {
-			time.Sleep(period)
 			trigger <- struct{}{}
+			time.Sleep(period)
 		}
 	}()
 	return func() {

cmd/controller/keyregistry.go

@@ -4,30 +4,40 @@ import (
 	"crypto/rsa"
 	"crypto/x509"
 	"encoding/pem"
+	"fmt"
 	"log"
+	"time"
 
+	"github.com/bitnami-labs/sealed-secrets/pkg/crypto"
 	"k8s.io/client-go/kubernetes"
 	certUtil "k8s.io/client-go/util/cert"
 )
 
+type Key struct {
+	private      *rsa.PrivateKey
+	cert         *x509.Certificate
+	fingerprint  string
+	creationTime time.Time
+}
+
 type KeyRegistry struct {
-	client      kubernetes.Interface
-	namespace   string
-	keyPrefix   string
-	keyLabel    string
-	keysize     int
-	privateKeys []*rsa.PrivateKey
-	cert        *x509.Certificate
+	client        kubernetes.Interface
+	namespace     string
+	keyPrefix     string
+	keyLabel      string
+	keysize       int
+	keys          map[string]*Key
+	mostRecentKey *Key
 }
 
 func NewKeyRegistry(client kubernetes.Interface, namespace, keyPrefix, keyLabel string, keysize int) *KeyRegistry {
 	return &KeyRegistry{
-		client:      client,
-		namespace:   namespace,
-		keyPrefix:   keyPrefix,
-		keysize:     keysize,
-		keyLabel:    keyLabel,
-		privateKeys: []*rsa.PrivateKey{},
+		client:    client,
+		namespace: namespace,
+		keyPrefix: keyPrefix,
+		keysize:   keysize,
+		keyLabel:  keyLabel,
+		keys:      map[string]*Key{},
 	}
 }
 
@@ -42,21 +52,42 @@ func (kr *KeyRegistry) generateKey() (string, error) {
 		return "", err
 	}
 	// Only store key to local store if write to k8s worked
-	kr.registerNewKey(generatedName, key, cert)
+	if err := kr.registerNewKey(generatedName, key, cert, time.Now()); err != nil {
+		return "", err
+	}
 	log.Printf("New key written to %s/%s\n", kr.namespace, generatedName)
 	log.Printf("Certificate is \n%s\n", pem.EncodeToMemory(&pem.Block{Type: certUtil.CertificateBlockType, Bytes: cert.Raw}))
 	return generatedName, nil
 }
 
-func (kr *KeyRegistry) registerNewKey(keyName string, privKey *rsa.PrivateKey, cert *x509.Certificate) {
-	kr.privateKeys = append(kr.privateKeys, privKey)
-	kr.cert = cert
+func (kr *KeyRegistry) registerNewKey(keyName string, privKey *rsa.PrivateKey, cert *x509.Certificate, creationTime time.Time) error {
+	fingerprint, err := crypto.PublicKeyFingerprint(&privKey.PublicKey)
+	if err != nil {
+		return err
+	}
+
+	k := &Key{
+		private:      privKey,
+		cert:         cert,
+		fingerprint:  fingerprint,
+		creationTime: creationTime,
+	}
+	kr.keys[k.fingerprint] = k
+
+	if kr.mostRecentKey == nil || kr.mostRecentKey.creationTime.Before(creationTime) {
+		kr.mostRecentKey = k
+	}
+
+	return nil
 }
 
 func (kr *KeyRegistry) latestPrivateKey() *rsa.PrivateKey {
-	return kr.privateKeys[len(kr.privateKeys)-1]
+	return kr.mostRecentKey.private
 }
 
-func (kr *KeyRegistry) getCert(keyname string) (*x509.Certificate, error) {
-	return kr.cert, nil
+func (kr *KeyRegistry) getCert() (*x509.Certificate, error) {
+	if kr.mostRecentKey == nil {
+		return nil, fmt.Errorf("key registry has no keys")
+	}
+	return kr.mostRecentKey.cert, nil
 }

cmd/controller/keyregistry_test.go

@@ -0,0 +1,42 @@
+package main
+
+import (
+	"testing"
+	"time"
+)
+
+func TestRegisterNewKey(t *testing.T) {
+	const keySize = 2048
+	kr := NewKeyRegistry(nil, "namespace", "prefix", "label", keySize)
+
+	if kr.mostRecentKey != nil {
+		t.Fatal("this test assumes a new key registry has no keys")
+	}
+
+	key1, cert1, err := generatePrivateKeyAndCert(keySize)
+	if err != nil {
+		t.Fatal(err)
+	}
+	t1 := time.Now()
+
+	key2, cert2, err := generatePrivateKeyAndCert(keySize)
+	if err != nil {
+		t.Fatal(err)
+	}
+	t2 := time.Now()
+
+	if err := kr.registerNewKey("k2", key2, cert2, t2); err != nil {
+		t.Fatal(err)
+	}
+	if got, want := kr.mostRecentKey.private, key2; got != want {
+		t.Errorf("got: %v, want: %v", got, want)
+	}
+
+	// key1 is older, so it shouldn't replace key2 as the mostRecentKey
+	if err := kr.registerNewKey("k1", key1, cert1, t1); err != nil {
+		t.Fatal(err)
+	}
+	if got, want := kr.mostRecentKey.private, key2; got != want {
+		t.Errorf("got: %v, want: %v", got, want)
+	}
+}

cmd/controller/main.go

@@ -89,7 +89,10 @@ func initKeyRegistry(client kubernetes.Interface, r io.Reader, namespace, prefix
 		if err != nil {
 			log.Printf("Error reading key %s: %v", secret.Name, err)
 		}
-		keyRegistry.registerNewKey(secret.Name, key, certs[0])
+		ct := secret.CreationTimestamp
+		if err := keyRegistry.registerNewKey(secret.Name, key, certs[0], ct.Time); err != nil {
+			return nil, err
+		}
 		log.Printf("----- %s", secret.Name)
 	}
 	return keyRegistry, nil
@@ -119,7 +122,7 @@ func initKeyRotation(registry *KeyRegistry, period time.Duration) (func(), error
 	// 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.privateKeys) == 0 {
+	if period != 0 || len(registry.keys) == 0 {
 		if _, err := registry.generateKey(); err != nil {
 			return nil, err
 		}
@@ -133,7 +136,7 @@ func initKeyRotation(registry *KeyRegistry, period time.Duration) (func(), error
 	if period == 0 {
 		return keyGenFunc, nil
 	}
-	return ScheduleJobWithTrigger(period, keyGenFunc), nil
+	return ScheduleJobWithTrigger(period, period, keyGenFunc), nil
 }
 
 func initKeyGenSignalListener(trigger func()) {
@@ -190,8 +193,12 @@ func main2() error {
 
 	go controller.Run(stop)
 
+	cert, err := keyRegistry.getCert()
+	if err != nil {
+		return err
+	}
 	cp := func() []*x509.Certificate {
-		return []*x509.Certificate{keyRegistry.cert}
+		return []*x509.Certificate{cert}
 	}
 
 	go httpserver(cp, controller.AttemptUnseal, controller.Rotate)

go.mod

@@ -16,6 +16,7 @@ require (
 	github.com/onsi/gomega v0.0.0-20190113212917-5533ce8a0da3
 	github.com/spf13/pflag v1.0.3
 	github.com/throttled/throttled v2.2.2+incompatible
+	golang.org/x/crypto v0.0.0-20190611184440-5c40567a22f8
 	golang.org/x/time v0.0.0-20181108054448-85acf8d2951c // indirect
 	k8s.io/api v0.0.0-20190620084959-7cf5895f2711
 	k8s.io/apimachinery v0.0.0-20190612205821-1799e75a0719

pkg/crypto/crypto.go

@@ -8,6 +8,8 @@ import (
 	"encoding/binary"
 	"errors"
 	"io"
+
+	"golang.org/x/crypto/ssh"
 )
 
 const (
@@ -17,6 +19,15 @@ const (
 // ErrTooShort indicates the provided data is too short to be valid
 var ErrTooShort = errors.New("SealedSecret data is too short")
 
+// PublicKeyFingerprint returns a fingerprint for a public key.
+func PublicKeyFingerprint(rp *rsa.PublicKey) (string, error) {
+	sp, err := ssh.NewPublicKey(rp)
+	if err != nil {
+		return "", err
+	}
+	return ssh.FingerprintSHA256(sp), nil
+}
+
 // HybridEncrypt performs a regular AES-GCM + RSA-OAEP encryption.
 // The output bytestring is:
 //   RSA ciphertext length || RSA ciphertext || AES ciphertext