Merge pull request #167 from gtank/cryptofix

use GCM instead of CBC

cmd/dex-overlord/main.go

@@ -32,6 +32,8 @@ func main() {
 	keySecrets := pflag.NewBase64List(32)
 	fs.Var(keySecrets, "key-secrets", "A comma-separated list of base64 encoded 32 byte strings used as symmetric keys used to encrypt/decrypt signing key data in DB. The first key is considered the active key and used for encryption, while the others are used to decrypt.")
 
+	useOldFormat := fs.Bool("use-deprecated-secret-format", false, "In prior releases, the database used AES-CBC to encrypt keys. New deployments should use the default AES-GCM encryption.")
+
 	dbURL := fs.String("db-url", "", "DSN-formatted database connection string")
 
 	dbMigrate := fs.Bool("db-migrate", true, "perform database migrations when starting up overlord. This includes the initial DB objects creation.")
@@ -100,7 +102,7 @@ func main() {
 	userManager := user.NewManager(userRepo,
 		pwiRepo, db.TransactionFactory(dbc), user.ManagerOptions{})
 	adminAPI := admin.NewAdminAPI(userManager, userRepo, pwiRepo, *localConnectorID)
-	kRepo, err := db.NewPrivateKeySetRepo(dbc, keySecrets.BytesSlice()...)
+	kRepo, err := db.NewPrivateKeySetRepo(dbc, *useOldFormat, keySecrets.BytesSlice()...)
 	if err != nil {
 		log.Fatalf(err.Error())
 	}

cmd/dex-worker/main.go

@@ -58,6 +58,8 @@ func main() {
 	keySecrets := pflag.NewBase64List(32)
 	fs.Var(keySecrets, "key-secrets", "A comma-separated list of base64 encoded 32 byte strings used as symmetric keys used to encrypt/decrypt signing key data in DB. The first key is considered the active key and used for encryption, while the others are used to decrypt.")
 
+	useOldFormat := fs.Bool("use-deprecated-secret-format", false, "In prior releases, the database used AES-CBC to encrypt keys. New deployments should use the default AES-GCM encryption.")
+
 	dbMaxIdleConns := fs.Int("db-max-idle-conns", 0, "maximum number of connections in the idle connection pool")
 	dbMaxOpenConns := fs.Int("db-max-open-conns", 0, "maximum number of open connections to the database")
 
@@ -147,6 +149,7 @@ func main() {
 		scfg.StateConfig = &server.MultiServerConfig{
 			KeySecrets:     keySecrets.BytesSlice(),
 			DatabaseConfig: dbCfg,
+			UseOldFormat:   *useOldFormat,
 		}
 	}
 

db/key.go

@@ -89,7 +89,7 @@ type privateKeySetBlob struct {
 	Value []byte `db:"value"`
 }
 
-func NewPrivateKeySetRepo(dbm *gorp.DbMap, secrets ...[]byte) (*PrivateKeySetRepo, error) {
+func NewPrivateKeySetRepo(dbm *gorp.DbMap, useOldFormat bool, secrets ...[]byte) (*PrivateKeySetRepo, error) {
 	for i, secret := range secrets {
 		if len(secret) != 32 {
 			return nil, fmt.Errorf("key secret %d: expected 32-byte secret", i)
@@ -97,16 +97,18 @@ func NewPrivateKeySetRepo(dbm *gorp.DbMap, secrets ...[]byte) (*PrivateKeySetRep
 	}
 
 	r := &PrivateKeySetRepo{
-		dbMap:   dbm,
-		secrets: secrets,
+		dbMap:        dbm,
+		useOldFormat: useOldFormat,
+		secrets:      secrets,
 	}
 
 	return r, nil
 }
 
 type PrivateKeySetRepo struct {
-	dbMap   *gorp.DbMap
-	secrets [][]byte
+	dbMap        *gorp.DbMap
+	useOldFormat bool
+	secrets      [][]byte
 }
 
 func (r *PrivateKeySetRepo) Set(ks key.KeySet) error {
@@ -131,7 +133,14 @@ func (r *PrivateKeySetRepo) Set(ks key.KeySet) error {
 		return err
 	}
 
-	v, err := pcrypto.AESEncrypt(j, r.active())
+	var v []byte
+
+	if r.useOldFormat {
+		v, err = pcrypto.AESEncrypt(j, r.active())
+	} else {
+		v, err = pcrypto.Encrypt(j, r.active())
+	}
+
 	if err != nil {
 		return err
 	}
@@ -159,7 +168,13 @@ func (r *PrivateKeySetRepo) Get() (key.KeySet, error) {
 	var pks *key.PrivateKeySet
 	for _, secret := range r.secrets {
 		var j []byte
-		j, err = pcrypto.AESDecrypt(b.Value, secret)
+
+		if r.useOldFormat {
+			j, err = pcrypto.AESDecrypt(b.Value, secret)
+		} else {
+			j, err = pcrypto.Decrypt(b.Value, secret)
+		}
+
 		if err != nil {
 			continue
 		}

db/key_test.go

@@ -5,7 +5,7 @@ import (
 )
 
 func TestNewPrivateKeySetRepoInvalidKey(t *testing.T) {
-	_, err := NewPrivateKeySetRepo(nil, []byte("sharks"))
+	_, err := NewPrivateKeySetRepo(nil, false, []byte("sharks"))
 	if err == nil {
 		t.Fatalf("Expected non-nil error")
 	}

functional/db_test.go

@@ -155,12 +155,12 @@ func TestDBPrivateKeySetRepoSetGet(t *testing.T) {
 	}
 
 	for i, tt := range tests {
-		setRepo, err := db.NewPrivateKeySetRepo(connect(t), tt.setSecrets...)
+		setRepo, err := db.NewPrivateKeySetRepo(connect(t), false, tt.setSecrets...)
 		if err != nil {
 			t.Fatalf(err.Error())
 		}
 
-		getRepo, err := db.NewPrivateKeySetRepo(connect(t), tt.getSecrets...)
+		getRepo, err := db.NewPrivateKeySetRepo(connect(t), false, tt.getSecrets...)
 		if err != nil {
 			t.Fatalf(err.Error())
 		}

pkg/crypto/encrypt.go

@@ -7,6 +7,8 @@ import (
 	"errors"
 )
 
+const aesKeySize = 32 // force 256-bit AES
+
 // pad uses the PKCS#7 padding scheme to align the a payload to a specific block size
 func pad(plaintext []byte, bsize int) ([]byte, error) {
 	if bsize >= 256 {
@@ -33,11 +35,12 @@ func unpad(paddedtext []byte) ([]byte, error) {
 	return paddedtext[:length-(pad)], nil
 }
 
-// AESEncrypt encrypts a payloaded with an AES cipher.
+// AESEncrypt encrypts a payload using AES-CBC and PKCS#7 padding.
 // The returned ciphertext has three notable properties:
 // 1. ciphertext is aligned to the standard AES block size
 // 2. ciphertext is padded using PKCS#7
 // 3. IV is prepended to the ciphertext
+// This function is DEPRECATED. Use Encrypt() instead.
 func AESEncrypt(plaintext, key []byte) ([]byte, error) {
 	plaintext, err := pad(plaintext, aes.BlockSize)
 	if err != nil {
@@ -61,11 +64,12 @@ func AESEncrypt(plaintext, key []byte) ([]byte, error) {
 	return ciphertext, nil
 }
 
-// AESDecrypt decrypts an encrypted payload with an AES cipher.
+// AESDecrypt decrypts a payload encrypted using AES-CBC and PKCS#7 padding.
 // The decryption algorithm makes three assumptions:
 // 1. ciphertext is aligned to the standard AES block size
 // 2. ciphertext is padded using PKCS#7
 // 3. the IV is prepended to ciphertext
+// This function is DEPRECATED. Use Decrypt() instead.
 func AESDecrypt(ciphertext, key []byte) ([]byte, error) {
 	if len(ciphertext) < aes.BlockSize {
 		return nil, errors.New("ciphertext too short")
@@ -94,3 +98,51 @@ func AESDecrypt(ciphertext, key []byte) ([]byte, error) {
 
 	return unpad(plaintext)
 }
+
+// Encrypt encrypts data using 256-bit AES-GCM.
+// This both hides the content of the data and provides a check that it hasn't been altered.
+// Output takes the form nonce|ciphertext|tag where '|' indicates concatenation.
+func Encrypt(plaintext, key []byte) (ciphertext []byte, err error) {
+	if len(key) != aesKeySize {
+		return nil, aes.KeySizeError(len(key))
+	}
+
+	aes, err := aes.NewCipher(key)
+	if err != nil {
+		return nil, err
+	}
+
+	gcm, err := cipher.NewGCM(aes)
+	if err != nil {
+		return nil, err
+	}
+
+	nonce, err := RandBytes(gcm.NonceSize())
+	if err != nil {
+		return nil, err
+	}
+
+	return gcm.Seal(nonce, nonce, plaintext, nil), nil
+}
+
+// Decrypt decrypts data using 256-bit AES-GCM.
+// This both hides the content of the data and provides a check that it hasn't been altered.
+// Expects input form nonce|ciphertext|tag where '|' indicates concatenation.
+func Decrypt(ciphertext, key []byte) (plaintext []byte, err error) {
+	if len(key) != aesKeySize {
+		return nil, aes.KeySizeError(len(key))
+	}
+
+	aes, err := aes.NewCipher(key)
+	if err != nil {
+		return nil, err
+	}
+
+	gcm, err := cipher.NewGCM(aes)
+	if err != nil {
+		return nil, err
+	}
+
+	return gcm.Open(nil, ciphertext[:gcm.NonceSize()],
+		ciphertext[gcm.NonceSize():], nil)
+}

pkg/crypto/encrypt_test.go

@@ -1,6 +1,7 @@
 package crypto
 
 import (
+	"bytes"
 	"reflect"
 	"testing"
 )
@@ -91,3 +92,37 @@ func TestAESDecryptWrongKey(t *testing.T) {
 		t.Fatalf("Data decrypted with different key matches original payload")
 	}
 }
+
+func TestEncryptDecryptGCM(t *testing.T) {
+	gcmTests := []struct {
+		plaintext []byte
+		key       []byte
+	}{
+		{
+			plaintext: []byte("Hello, world!"),
+			key:       append([]byte("shark"), make([]byte, 27)...),
+		},
+	}
+
+	for _, tt := range gcmTests {
+		ciphertext, err := Encrypt(tt.plaintext, tt.key)
+		if err != nil {
+			t.Fatal(err)
+		}
+
+		plaintext, err := Decrypt(ciphertext, tt.key)
+		if err != nil {
+			t.Fatal(err)
+		}
+
+		if !bytes.Equal(plaintext, tt.plaintext) {
+			t.Errorf("plaintexts don't match")
+		}
+
+		ciphertext[0] ^= 0xff
+		plaintext, err = Decrypt(ciphertext, tt.key)
+		if err == nil {
+			t.Errorf("gcmOpen should not have worked, but did")
+		}
+	}
+}

server/config.go

@@ -49,6 +49,7 @@ type SingleServerConfig struct {
 type MultiServerConfig struct {
 	KeySecrets     [][]byte
 	DatabaseConfig db.Config
+	UseOldFormat   bool
 }
 
 func (cfg *ServerConfig) Server() (*Server, error) {
@@ -159,7 +160,7 @@ func (cfg *MultiServerConfig) Configure(srv *Server) error {
 		return fmt.Errorf("unable to initialize database connection: %v", err)
 	}
 
-	kRepo, err := db.NewPrivateKeySetRepo(dbc, cfg.KeySecrets...)
+	kRepo, err := db.NewPrivateKeySetRepo(dbc, cfg.UseOldFormat, cfg.KeySecrets...)
 	if err != nil {
 		return fmt.Errorf("unable to create PrivateKeySetRepo: %v", err)
 	}