From 014fa682be348bafce6b29d0a570ee35d44d9ea2 Mon Sep 17 00:00:00 2001 From: Nick Meves Date: Mon, 1 Jun 2020 16:19:27 -0700 Subject: [PATCH] Add EncryptInto/DecryptInto Unit Tests --- CHANGELOG.md | 1 + pkg/apis/sessions/session_state.go | 35 +++------ pkg/apis/sessions/session_state_test.go | 12 +-- pkg/encryption/cipher.go | 55 ++++++++------ pkg/encryption/cipher_test.go | 97 +++++++++++++++++++------ pkg/encryption/utils.go | 1 - 6 files changed, 124 insertions(+), 77 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index f35a3b43..f3cca2b0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -55,6 +55,7 @@ ## Changes since v5.1.1 +- [#539](https://github.com/oauth2-proxy/oauth2-proxy/pull/539) Refactor encryption ciphers and add AES-GCM support (@NickMeves) - [#601](https://github.com/oauth2-proxy/oauth2-proxy/pull/601) Ensure decrypted user/email are valid UTF8 (@JoelSpeed) - [#560](https://github.com/oauth2-proxy/oauth2-proxy/pull/560) Fallback to UserInfo is User ID claim not present (@JoelSpeed) - [#598](https://github.com/oauth2-proxy/oauth2-proxy/pull/598) acr_values no longer sent to IdP when empty (@ScottGuymer) diff --git a/pkg/apis/sessions/session_state.go b/pkg/apis/sessions/session_state.go index c014aee2..24377c4a 100644 --- a/pkg/apis/sessions/session_state.go +++ b/pkg/apis/sessions/session_state.go @@ -104,24 +104,18 @@ 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 { - if !utf8.ValidString(decryptedEmail) { - return nil, errors.New("invalid value for decrypted email") - } - ss.Email = decryptedEmail + // Backward compatibility with using unencrypted Email or User + // Decryption errors will leave original string + err = c.DecryptInto(&ss.Email) + if err == nil { + if !utf8.ValidString(ss.Email) { + return nil, errors.New("invalid value for decrypted email") } } - // Backward compatibility with using unencrypted User - if ss.User != "" { - decryptedUser, errUser := stringDecrypt(ss.User, c) - if errUser == nil { - if !utf8.ValidString(decryptedUser) { - return nil, errors.New("invalid value for decrypted user") - } - ss.User = decryptedUser + err = c.DecryptInto(&ss.User) + if err == nil { + if !utf8.ValidString(ss.User) { + return nil, errors.New("invalid value for decrypted user") } } @@ -139,12 +133,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 -} diff --git a/pkg/apis/sessions/session_state_test.go b/pkg/apis/sessions/session_state_test.go index c6ca0c07..d48ec502 100644 --- a/pkg/apis/sessions/session_state_test.go +++ b/pkg/apis/sessions/session_state_test.go @@ -17,14 +17,14 @@ func timePtr(t time.Time) *time.Time { return &t } -func NewCipher(secret []byte) (encryption.Cipher, error) { +func newTestCipher(secret []byte) (encryption.Cipher, error) { return encryption.NewBase64Cipher(encryption.NewCFBCipher, secret) } func TestSessionStateSerialization(t *testing.T) { - c, err := NewCipher([]byte(secret)) + c, err := newTestCipher([]byte(secret)) assert.Equal(t, nil, err) - c2, err := NewCipher([]byte(altSecret)) + c2, err := newTestCipher([]byte(altSecret)) assert.Equal(t, nil, err) s := &sessions.SessionState{ Email: "user@domain.com", @@ -57,9 +57,9 @@ func TestSessionStateSerialization(t *testing.T) { } func TestSessionStateSerializationWithUser(t *testing.T) { - c, err := NewCipher([]byte(secret)) + c, err := newTestCipher([]byte(secret)) assert.Equal(t, nil, err) - c2, err := NewCipher([]byte(altSecret)) + c2, err := newTestCipher([]byte(altSecret)) assert.Equal(t, nil, err) s := &sessions.SessionState{ User: "just-user", @@ -205,7 +205,7 @@ func TestDecodeSessionState(t *testing.T) { eJSON, _ := e.MarshalJSON() eString := string(eJSON) - c, err := NewCipher([]byte(secret)) + c, err := newTestCipher([]byte(secret)) assert.NoError(t, err) testCases := []testCase{ diff --git a/pkg/encryption/cipher.go b/pkg/encryption/cipher.go index d0d31882..34499ba6 100644 --- a/pkg/encryption/cipher.go +++ b/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 } @@ -52,7 +33,7 @@ func NewBase64Cipher(initCipher func([]byte) (Cipher, error), secret []byte) (Ci // Encrypt encrypts a value with the embedded Cipher & Base64 encodes it func (c *Base64Cipher) Encrypt(value []byte) ([]byte, error) { - encrypted, err := c.Cipher.Encrypt([]byte(value)) + encrypted, err := c.Cipher.Encrypt(value) if err != nil { return nil, err } @@ -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) diff --git a/pkg/encryption/cipher_test.go b/pkg/encryption/cipher_test.go index ca8698a5..e80986d5 100644 --- a/pkg/encryption/cipher_test.go +++ b/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)) -} diff --git a/pkg/encryption/utils.go b/pkg/encryption/utils.go index 0d6bd86a..26be6b24 100644 --- a/pkg/encryption/utils.go +++ b/pkg/encryption/utils.go @@ -104,4 +104,3 @@ func checkHmac(input, expected string) bool { } return false } -