Add EncryptInto/DecryptInto Unit Tests

CHANGELOG.md

@@ -54,7 +54,7 @@
   - This flag also accepts comma separated values
 
 ## Changes since v5.1.1
-
+- [#539](https://github.com/oauth2-proxy/oauth2-proxy/pull/539) Refactor encryption ciphers and add AES-GCM support (@NickMeves)
 - [#548](https://github.com/oauth2-proxy/oauth2-proxy/pull/548) Separate logging options out of main options structure (@JoelSpeed)
 - [#536](https://github.com/oauth2-proxy/oauth2-proxy/pull/536) Improvements to Session State code (@JoelSpeed)
 - [#573](https://github.com/oauth2-proxy/oauth2-proxy/pull/573) Properly parse redis urls for cluster and sentinel connections (@amnay-mo)

pkg/apis/sessions/session_state.go

@@ -102,20 +102,10 @@ func DecodeSessionState(v string, c encryption.Cipher) (*SessionState, error) {
 			PreferredUsername: ss.PreferredUsername,
 		}
 	} else {
-		// Backward compatibility with using unencrypted Email
-		if ss.Email != "" {
-			decryptedEmail, errEmail := stringDecrypt(ss.Email, c)
-			if errEmail == nil {
-				ss.Email = decryptedEmail
-			}
-		}
-		// Backward compatibility with using unencrypted User
-		if ss.User != "" {
-			decryptedUser, errUser := stringDecrypt(ss.User, c)
-			if errUser == nil {
-				ss.User = decryptedUser
-			}
-		}
+		// Backward compatibility with using unencrypted Email or User
+		// Decryption errors will leave original string
+		c.DecryptInto(&ss.Email)
+		c.DecryptInto(&ss.User)
 
 		for _, s := range []*string{
 			&ss.PreferredUsername,
@@ -131,12 +121,3 @@ func DecodeSessionState(v string, c encryption.Cipher) (*SessionState, error) {
 	}
 	return &ss, nil
 }
-
-// stringDecrypt wraps a Base64Cipher to make it string => string
-func stringDecrypt(ciphertext string, c encryption.Cipher) (string, error) {
-	value, err := c.Decrypt([]byte(ciphertext))
-	if err != nil {
-		return "", err
-	}
-	return string(value), nil
-}

pkg/encryption/cipher.go

@@ -13,30 +13,11 @@ import (
 type Cipher interface {
 	Encrypt(value []byte) ([]byte, error)
 	Decrypt(ciphertext []byte) ([]byte, error)
-    EncryptInto(s *string) error
+	EncryptInto(s *string) error
 	DecryptInto(s *string) error
 }
 
-type DefaultCipher struct {}
-
-// Encrypt is a dummy method for CommonCipher.EncryptInto support
-func (c *DefaultCipher) Encrypt(value []byte) ([]byte, error) { return value, nil }
-
-// Decrypt is a dummy method for CommonCipher.DecryptInto support
-func (c *DefaultCipher) Decrypt(ciphertext []byte) ([]byte, error) { return ciphertext, nil }
-
-// EncryptInto encrypts the value and stores it back in the string pointer
-func (c *DefaultCipher) EncryptInto(s *string) error {
-	return into(c.Encrypt, s)
-}
-
-// DecryptInto decrypts the value and stores it back in the string pointer
-func (c *DefaultCipher) DecryptInto(s *string) error {
-	return into(c.Decrypt, s)
-}
-
 type Base64Cipher struct {
-	DefaultCipher
 	Cipher Cipher
 }
 
@@ -70,8 +51,17 @@ func (c *Base64Cipher) Decrypt(ciphertext []byte) ([]byte, error) {
 	return c.Cipher.Decrypt(encrypted)
 }
 
+// EncryptInto encrypts the value and stores it back in the string pointer
+func (c *Base64Cipher) EncryptInto(s *string) error {
+	return into(c.Encrypt, s)
+}
+
+// DecryptInto decrypts the value and stores it back in the string pointer
+func (c *Base64Cipher) DecryptInto(s *string) error {
+	return into(c.Decrypt, s)
+}
+
 type CFBCipher struct {
-	DefaultCipher
 	cipher.Block
 }
 
@@ -111,8 +101,17 @@ func (c *CFBCipher) Decrypt(ciphertext []byte) ([]byte, error) {
 	return plaintext, nil
 }
 
+// EncryptInto returns an error since the encrypted data is a []byte that isn't string cast-able
+func (c *CFBCipher) EncryptInto(s *string) error {
+	return fmt.Errorf("CFBCipher is not a string->string compatible cipher")
+}
+
+// EncryptInto returns an error since the encrypted data needs to be a []byte
+func (c *CFBCipher) DecryptInto(s *string) error {
+	return fmt.Errorf("CFBCipher is not a string->string compatible cipher")
+}
+
 type GCMCipher struct {
-	DefaultCipher
 	cipher.Block
 }
 
@@ -158,6 +157,16 @@ func (c *GCMCipher) Decrypt(ciphertext []byte) ([]byte, error) {
 	return plaintext, nil
 }
 
+// EncryptInto returns an error since the encrypted data is a []byte that isn't string cast-able
+func (c *GCMCipher) EncryptInto(s *string) error {
+	return fmt.Errorf("CFBCipher is not a string->string compatible cipher")
+}
+
+// EncryptInto returns an error since the encrypted data needs to be a []byte
+func (c *GCMCipher) DecryptInto(s *string) error {
+	return fmt.Errorf("CFBCipher is not a string->string compatible cipher")
+}
+
 // codecFunc is a function that takes a string and encodes/decodes it
 type codecFunc func([]byte) ([]byte, error)
 

pkg/encryption/cipher_test.go

@@ -5,6 +5,7 @@ import (
 	"encoding/base64"
 	"fmt"
 	"io"
+	mathrand "math/rand"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
@@ -117,6 +118,80 @@ func runEncryptAndDecrypt(t *testing.T, c Cipher, dataSize int) {
 	assert.NotEqual(t, encrypted, decrypted)
 }
 
+func TestEncryptIntoAndDecryptInto(t *testing.T) {
+	// Test our 2 cipher types
+	cipherInits := map[string]func([]byte) (Cipher, error){
+		"CFB": NewCFBCipher,
+		"GCM": NewGCMCipher,
+	}
+	for name, initCipher := range cipherInits {
+		t.Run(name, func(t *testing.T) {
+			// Test all 3 valid AES sizes
+			for _, secretSize := range []int{16, 24, 32} {
+				t.Run(fmt.Sprintf("%d", secretSize), func(t *testing.T) {
+					secret := make([]byte, secretSize)
+					_, err := io.ReadFull(rand.Reader, secret)
+					assert.Equal(t, nil, err)
+
+					// Test Standard & Base64 wrapped
+					cstd, err := initCipher(secret)
+					assert.Equal(t, nil, err)
+
+					cb64, err := NewBase64Cipher(initCipher, secret)
+					assert.Equal(t, nil, err)
+
+					ciphers := map[string]Cipher{
+						"Standard": cstd,
+						"Base64":   cb64,
+					}
+
+					for cName, c := range ciphers {
+						// Check no errors with empty or nil strings
+						if cName == "Base64" {
+							empty := ""
+							assert.Equal(t, nil, c.EncryptInto(&empty))
+							assert.Equal(t, nil, c.DecryptInto(&empty))
+							assert.Equal(t, nil, c.EncryptInto(nil))
+							assert.Equal(t, nil, c.DecryptInto(nil))
+						}
+
+						t.Run(cName, func(t *testing.T) {
+							// Test various sizes sessions might be
+							for _, dataSize := range []int{10, 100, 1000, 5000, 10000} {
+								t.Run(fmt.Sprintf("%d", dataSize), func(t *testing.T) {
+									runEncryptIntoAndDecryptInto(t, c, cName, dataSize)
+								})
+							}
+						})
+					}
+				})
+			}
+		})
+	}
+}
+
+func runEncryptIntoAndDecryptInto(t *testing.T, c Cipher, cipherType string, dataSize int) {
+	const charset = "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789"
+	b := make([]byte, dataSize)
+	for i := range b {
+		b[i] = charset[mathrand.Intn(len(charset))]
+	}
+	data := string(b)
+	originalData := data
+
+	// Base64 is the only cipher that supports string->string Encrypt/Decrypt Into methods
+	if cipherType == "Base64" {
+		assert.Equal(t, nil, c.EncryptInto(&data))
+		assert.NotEqual(t, originalData, data)
+
+		assert.Equal(t, nil, c.DecryptInto(&data))
+		assert.Equal(t, originalData, data)
+	} else {
+		assert.NotEqual(t, nil, c.EncryptInto(&data))
+		assert.NotEqual(t, nil, c.DecryptInto(&data))
+	}
+}
+
 func TestDecryptCFBWrongSecret(t *testing.T) {
 	secret1 := []byte("0123456789abcdefghijklmnopqrstuv")
 	secret2 := []byte("9876543210abcdefghijklmnopqrstuv")
@@ -228,25 +303,3 @@ func TestCFBtoGCMErrors(t *testing.T) {
 		})
 	}
 }
-
-func TestEncodeIntoAndDecodeIntoAccessToken(t *testing.T) {
-	const secret = "0123456789abcdefghijklmnopqrstuv"
-	c, err := NewCipher([]byte(secret))
-	assert.Equal(t, nil, err)
-
-	token := "my access token"
-	originalToken := token
-
-	assert.Equal(t, nil, c.EncryptInto(&token))
-	assert.NotEqual(t, originalToken, token)
-
-	assert.Equal(t, nil, c.DecryptInto(&token))
-	assert.Equal(t, originalToken, token)
-
-	// Check no errors with empty or nil strings
-	empty := ""
-	assert.Equal(t, nil, c.EncryptInto(&empty))
-	assert.Equal(t, nil, c.DecryptInto(&empty))
-	assert.Equal(t, nil, c.EncryptInto(nil))
-	assert.Equal(t, nil, c.DecryptInto(nil))
-}