From ce2e92bc578452a45bedf8c6413a2d8f43367469 Mon Sep 17 00:00:00 2001 From: Nick Meves Date: Sun, 10 May 2020 09:44:04 -0700 Subject: [PATCH] Improve design of Base64Cipher wrapping other ciphers. Have it take in a cipher init function as an argument. Remove the confusing `newCipher` method that matched legacy behavior and returns a Base64Cipher(CFBCipher) -- instead explicitly ask for that in the uses. --- pkg/apis/sessions/session_state_test.go | 14 +++-- pkg/encryption/cipher.go | 22 +++----- pkg/encryption/cipher_test.go | 74 ++++++++++++++++--------- pkg/sessions/session_store_test.go | 2 +- pkg/validation/options.go | 2 +- 5 files changed, 67 insertions(+), 47 deletions(-) diff --git a/pkg/apis/sessions/session_state_test.go b/pkg/apis/sessions/session_state_test.go index 349bd734..c6ca0c07 100644 --- a/pkg/apis/sessions/session_state_test.go +++ b/pkg/apis/sessions/session_state_test.go @@ -17,10 +17,14 @@ func timePtr(t time.Time) *time.Time { return &t } +func NewCipher(secret []byte) (encryption.Cipher, error) { + return encryption.NewBase64Cipher(encryption.NewCFBCipher, secret) +} + func TestSessionStateSerialization(t *testing.T) { - c, err := encryption.NewCipher([]byte(secret)) + c, err := NewCipher([]byte(secret)) assert.Equal(t, nil, err) - c2, err := encryption.NewCipher([]byte(altSecret)) + c2, err := NewCipher([]byte(altSecret)) assert.Equal(t, nil, err) s := &sessions.SessionState{ Email: "user@domain.com", @@ -53,9 +57,9 @@ func TestSessionStateSerialization(t *testing.T) { } func TestSessionStateSerializationWithUser(t *testing.T) { - c, err := encryption.NewCipher([]byte(secret)) + c, err := NewCipher([]byte(secret)) assert.Equal(t, nil, err) - c2, err := encryption.NewCipher([]byte(altSecret)) + c2, err := NewCipher([]byte(altSecret)) assert.Equal(t, nil, err) s := &sessions.SessionState{ User: "just-user", @@ -201,7 +205,7 @@ func TestDecodeSessionState(t *testing.T) { eJSON, _ := e.MarshalJSON() eString := string(eJSON) - c, err := encryption.NewCipher([]byte(secret)) + c, err := NewCipher([]byte(secret)) assert.NoError(t, err) testCases := []testCase{ diff --git a/pkg/encryption/cipher.go b/pkg/encryption/cipher.go index 8eddc7d0..63986312 100644 --- a/pkg/encryption/cipher.go +++ b/pkg/encryption/cipher.go @@ -135,16 +135,6 @@ func (c *DefaultCipher) DecryptInto(s *string) error { return into(c.Decrypt, s) } -// NewCipher returns a new aes Cipher for encrypting cookie values -// This defaults to the Base64 Cipher to align with legacy Encrypt/Decrypt functionality -func NewCipher(secret []byte) (Cipher, error) { - cfb, err := NewCFBCipher(secret) - if err != nil { - return nil, err - } - return NewBase64Cipher(cfb) -} - type Base64Cipher struct { DefaultCipher Cipher Cipher @@ -152,7 +142,11 @@ type Base64Cipher struct { // NewBase64Cipher returns a new AES Cipher for encrypting cookie values // and wrapping them in Base64 -- Supports Legacy encryption scheme -func NewBase64Cipher(c Cipher) (Cipher, error) { +func NewBase64Cipher(initCipher func([]byte) (Cipher, error), secret []byte) (Cipher, error) { + c, err := initCipher(secret) + if err != nil { + return nil, err + } return &Base64Cipher{Cipher: c}, nil } @@ -170,7 +164,7 @@ func (c *Base64Cipher) Encrypt(value []byte) ([]byte, error) { func (c *Base64Cipher) Decrypt(ciphertext []byte) ([]byte, error) { encrypted, err := base64.StdEncoding.DecodeString(string(ciphertext)) if err != nil { - return nil, fmt.Errorf("failed to decrypt cookie value %s", err) + return nil, fmt.Errorf("failed to base64 decode value %s", err) } return c.Cipher.Decrypt(encrypted) @@ -206,9 +200,7 @@ func (c *CFBCipher) Encrypt(value []byte) ([]byte, error) { // Decrypt an AES CFB ciphertext func (c *CFBCipher) Decrypt(ciphertext []byte) ([]byte, error) { if len(ciphertext) < aes.BlockSize { - return nil, fmt.Errorf("encrypted value should be "+ - "at least %d bytes, but is only %d bytes", - aes.BlockSize, len(ciphertext)) + return nil, fmt.Errorf("encrypted value should be at least %d bytes, but is only %d bytes", aes.BlockSize, len(ciphertext)) } iv := ciphertext[:aes.BlockSize] diff --git a/pkg/encryption/cipher_test.go b/pkg/encryption/cipher_test.go index 85ec5ce2..074cef44 100644 --- a/pkg/encryption/cipher_test.go +++ b/pkg/encryption/cipher_test.go @@ -102,7 +102,7 @@ func TestSignAndValidate(t *testing.T) { func TestEncodeAndDecodeAccessToken(t *testing.T) { const secret = "0123456789abcdefghijklmnopqrstuv" const token = "my access token" - c, err := NewCipher([]byte(secret)) + c, err := NewBase64Cipher(NewCFBCipher, []byte(secret)) assert.Equal(t, nil, err) encoded, err := c.Encrypt([]byte(token)) @@ -121,7 +121,7 @@ func TestEncodeAndDecodeAccessTokenB64(t *testing.T) { secret, err := base64.URLEncoding.DecodeString(secretBase64) assert.Equal(t, nil, err) - c, err := NewCipher([]byte(secret)) + c, err := NewBase64Cipher(NewCFBCipher, []byte(secret)) assert.Equal(t, nil, err) encoded, err := c.Encrypt([]byte(token)) @@ -135,14 +135,12 @@ func TestEncodeAndDecodeAccessTokenB64(t *testing.T) { } func TestEncryptAndDecrypt(t *testing.T) { - var err error - - // Test our 3 cipher types - for _, initCipher := range []func([]byte) (Cipher, error){NewCipher, NewCFBCipher, NewGCMCipher} { + // Test our 2 cipher types + for _, initCipher := range []func([]byte) (Cipher, error){NewCFBCipher, NewGCMCipher} { // Test all 3 valid AES sizes for _, secretSize := range []int{16, 24, 32} { secret := make([]byte, secretSize) - _, err = io.ReadFull(rand.Reader, secret) + _, err := io.ReadFull(rand.Reader, secret) assert.Equal(t, nil, err) c, err := initCipher(secret) @@ -167,27 +165,55 @@ func TestEncryptAndDecrypt(t *testing.T) { } } -func TestDecryptWrongSecret(t *testing.T) { +func TestEncryptAndDecryptBase64(t *testing.T) { + // Test our cipher types wrapped in Base64 encoder + for _, initCipher := range []func([]byte) (Cipher, error){NewCFBCipher, NewGCMCipher} { + // Test all 3 valid AES sizes + for _, secretSize := range []int{16, 24, 32} { + secret := make([]byte, secretSize) + _, err := io.ReadFull(rand.Reader, secret) + assert.Equal(t, nil, err) + + c, err := NewBase64Cipher(initCipher, secret) + assert.Equal(t, nil, err) + + // Test various sizes sessions might be + for _, dataSize := range []int{10, 100, 1000, 5000, 10000} { + data := make([]byte, dataSize) + _, err := io.ReadFull(rand.Reader, data) + assert.Equal(t, nil, err) + + encrypted, err := c.Encrypt(data) + assert.Equal(t, nil, err) + assert.NotEqual(t, encrypted, data) + + decrypted, err := c.Decrypt(encrypted) + assert.Equal(t, nil, err) + assert.Equal(t, data, decrypted) + assert.NotEqual(t, encrypted, decrypted) + } + } + } +} + +func TestDecryptCFBWrongSecret(t *testing.T) { secret1 := []byte("0123456789abcdefghijklmnopqrstuv") secret2 := []byte("9876543210abcdefghijklmnopqrstuv") - // Test CFB & Base64 (GCM is authenticated, it errors differently) - for _, initCipher := range []func([]byte) (Cipher, error){NewCipher, NewCFBCipher} { - c1, err := initCipher(secret1) - assert.Equal(t, nil, err) + c1, err := NewCFBCipher(secret1) + assert.Equal(t, nil, err) - c2, err := initCipher(secret2) - assert.Equal(t, nil, err) + c2, err := NewCFBCipher(secret2) + assert.Equal(t, nil, err) - data := []byte("f3928pufm982374dj02y485dsl34890u2t9nd4028s94dm58y2394087dhmsyt29h8df") + data := []byte("f3928pufm982374dj02y485dsl34890u2t9nd4028s94dm58y2394087dhmsyt29h8df") - ciphertext, err := c1.Encrypt(data) - assert.Equal(t, nil, err) + ciphertext, err := c1.Encrypt(data) + assert.Equal(t, nil, err) - wrongData, err := c2.Decrypt(ciphertext) - assert.Equal(t, nil, err) - assert.NotEqual(t, data, wrongData) - } + wrongData, err := c2.Decrypt(ciphertext) + assert.Equal(t, nil, err) + assert.NotEqual(t, data, wrongData) } func TestDecryptGCMWrongSecret(t *testing.T) { @@ -211,13 +237,11 @@ func TestDecryptGCMWrongSecret(t *testing.T) { } func TestIntermixCiphersErrors(t *testing.T) { - var err error - // Encrypt with GCM, Decrypt with CFB: Results in Garbage data // Test all 3 valid AES sizes for _, secretSize := range []int{16, 24, 32} { secret := make([]byte, secretSize) - _, err = io.ReadFull(rand.Reader, secret) + _, err := io.ReadFull(rand.Reader, secret) assert.Equal(t, nil, err) gcm, err := NewGCMCipher(secret) @@ -248,7 +272,7 @@ func TestIntermixCiphersErrors(t *testing.T) { // Test all 3 valid AES sizes for _, secretSize := range []int{16, 24, 32} { secret := make([]byte, secretSize) - _, err = io.ReadFull(rand.Reader, secret) + _, err := io.ReadFull(rand.Reader, secret) assert.Equal(t, nil, err) gcm, err := NewGCMCipher(secret) diff --git a/pkg/sessions/session_store_test.go b/pkg/sessions/session_store_test.go index f1919eb0..1a60aa0d 100644 --- a/pkg/sessions/session_store_test.go +++ b/pkg/sessions/session_store_test.go @@ -367,7 +367,7 @@ var _ = Describe("NewSessionStore", func() { _, err := rand.Read(secret) Expect(err).ToNot(HaveOccurred()) cookieOpts.Secret = base64.URLEncoding.EncodeToString(secret) - cipher, err := encryption.NewCipher(encryption.SecretBytes(cookieOpts.Secret)) + cipher, err := encryption.NewBase64Cipher(encryption.NewCFBCipher, encryption.SecretBytes(cookieOpts.Secret)) Expect(err).ToNot(HaveOccurred()) Expect(cipher).ToNot(BeNil()) opts.Cipher = cipher diff --git a/pkg/validation/options.go b/pkg/validation/options.go index ccb96c79..b22882d0 100644 --- a/pkg/validation/options.go +++ b/pkg/validation/options.go @@ -62,7 +62,7 @@ func Validate(o *options.Options) error { len(encryption.SecretBytes(o.Cookie.Secret)), suffix)) } else { var err error - cipher, err = encryption.NewCipher(encryption.SecretBytes(o.Cookie.Secret)) + cipher, err = encryption.NewBase64Cipher(encryption.NewCFBCipher, encryption.SecretBytes(o.Cookie.Secret)) if err != nil { msgs = append(msgs, fmt.Sprintf("cookie-secret error: %v", err)) }