Fix secretBytes adding unintended padding (#556)

* Fix secretBytes adding unintended padding

* Add more SecretBytes test scenarios

* Add CHANGELOG entry about breaking secret padding change

* Add SecretBytes tests explanation comments

CHANGELOG.md

@@ -32,8 +32,16 @@
   - In some scenarios `X-Forwarded-User` will now be empty. Use `X-Forwarded-Email` instead.
   - In some scenarios, this may break setting Basic Auth on upstream or responses.
     Use `--prefer-email-to-user` to restore falling back to the Email in these cases.
+- [#556](https://github.com/oauth2-proxy/oauth2-proxy/pull/556) Remove unintentional auto-padding of secrets that were too short
+  - Previously, after cookie-secrets were opportunistically base64 decoded to raw bytes,
+    they were padded to have a length divisible by 4.
+  - This led to wrong sized secrets being valid AES lengths of 16, 24, or 32  bytes. Or it led to confusing errors
+    reporting an invalid length of 20 or 28 when the user input cookie-secret was not that length.
+  - Now we will only base64 decode a cookie-secret to raw bytes if it is 16, 24, or 32 bytes long. Otherwise, we will convert
+    the direct cookie-secret to bytes without silent padding added.
 
 ## Changes since v5.1.1
+- [#556](https://github.com/oauth2-proxy/oauth2-proxy/pull/556) Remove unintentional auto-padding of secrets that were too short (@NickMeves)
 - [#538](https://github.com/oauth2-proxy/oauth2-proxy/pull/538) Refactor sessions/utils.go functionality to other areas (@NickMeves)
 - [#503](https://github.com/oauth2-proxy/oauth2-proxy/pull/503) Implements --real-client-ip-header option to select the header from which to obtain a proxied client's IP (@Izzette)
 - [#529](https://github.com/oauth2-proxy/oauth2-proxy/pull/529) Add local test environments for testing changes and new features (@JoelSpeed)

oauthproxy_test.go

@@ -898,7 +898,7 @@ func NewProcessCookieTest(opts ProcessCookieTestOpts, modifiers ...OptionsModifi
 	}
 	pcTest.opts.ClientID = "asdfljk"
 	pcTest.opts.ClientSecret = "lkjfdsig"
-	pcTest.opts.Cookie.Secret = "0123456789abcdefabcd"
+	pcTest.opts.Cookie.Secret = "0123456789abcdef0123456789abcdef"
 	// First, set the CookieRefresh option so proxy.AesCipher is created,
 	// needed to encrypt the access_token.
 	pcTest.opts.Cookie.Refresh = time.Hour

options_test.go

@@ -233,7 +233,7 @@ func TestCookieRefreshMustBeLessThanCookieExpire(t *testing.T) {
 	o := testOptions()
 	assert.Equal(t, nil, o.Validate())
 
-	o.Cookie.Secret = "0123456789abcdefabcd"
+	o.Cookie.Secret = "0123456789abcdef"
 	o.Cookie.Refresh = o.Cookie.Expire
 	assert.NotEqual(t, nil, o.Validate())
 

pkg/encryption/cipher.go

@@ -19,27 +19,22 @@ import (
 
 // SecretBytes attempts to base64 decode the secret, if that fails it treats the secret as binary
 func SecretBytes(secret string) []byte {
-	b, err := base64.URLEncoding.DecodeString(addPadding(secret))
+	b, err := base64.RawURLEncoding.DecodeString(strings.TrimRight(secret, "="))
 	if err == nil {
-		return []byte(addPadding(string(b)))
+		// Only return decoded form if a valid AES length
+		// Don't want unintentional decoding resulting in invalid lengths confusing a user
+		// that thought they used a 16, 24, 32 length string
+		for _, i := range []int{16, 24, 32} {
+			if len(b) == i {
+				return b
 			}
+		}
+	}
+	// If decoding didn't work or resulted in non-AES compliant length,
+	// assume the raw string was the intended secret
 	return []byte(secret)
 }
 
-func addPadding(secret string) string {
-	padding := len(secret) % 4
-	switch padding {
-	case 1:
-		return secret + "==="
-	case 2:
-		return secret + "=="
-	case 3:
-		return secret + "="
-	default:
-		return secret
-	}
-}
-
 // cookies are stored in a 3 part (value + timestamp + signature) to enforce that the values are as originally set.
 // additionally, the 'value' is encrypted so it's opaque to the browser
 

pkg/encryption/cipher_test.go

@@ -1,14 +1,87 @@
 package encryption
 
 import (
+	"crypto/rand"
 	"crypto/sha1"
 	"crypto/sha256"
 	"encoding/base64"
+	"fmt"
+	"io"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
 )
 
+func TestSecretBytesEncoded(t *testing.T) {
+	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)
+
+			// We test both padded & raw Base64 to ensure we handle both
+			// potential user input routes for Base64
+			base64Padded := base64.URLEncoding.EncodeToString(secret)
+			sb := SecretBytes(base64Padded)
+			assert.Equal(t, secret, sb)
+			assert.Equal(t, len(sb), secretSize)
+
+			base64Raw := base64.RawURLEncoding.EncodeToString(secret)
+			sb = SecretBytes(base64Raw)
+			assert.Equal(t, secret, sb)
+			assert.Equal(t, len(sb), secretSize)
+		})
+	}
+}
+
+// A string that isn't intended as Base64 and still decodes (but to unintended length)
+// will return the original secret as bytes
+func TestSecretBytesEncodedWrongSize(t *testing.T) {
+	for _, secretSize := range []int{15, 20, 28, 33, 44} {
+		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)
+
+			// We test both padded & raw Base64 to ensure we handle both
+			// potential user input routes for Base64
+			base64Padded := base64.URLEncoding.EncodeToString(secret)
+			sb := SecretBytes(base64Padded)
+			assert.NotEqual(t, secret, sb)
+			assert.NotEqual(t, len(sb), secretSize)
+			// The given secret is returned as []byte
+			assert.Equal(t, base64Padded, string(sb))
+
+			base64Raw := base64.RawURLEncoding.EncodeToString(secret)
+			sb = SecretBytes(base64Raw)
+			assert.NotEqual(t, secret, sb)
+			assert.NotEqual(t, len(sb), secretSize)
+			// The given secret is returned as []byte
+			assert.Equal(t, base64Raw, string(sb))
+		})
+	}
+}
+
+func TestSecretBytesNonBase64(t *testing.T) {
+	trailer := "equals=========="
+	assert.Equal(t, trailer, string(SecretBytes(trailer)))
+
+	raw16 := "asdflkjhqwer)(*&"
+	sb16 := SecretBytes(raw16)
+	assert.Equal(t, raw16, string(sb16))
+	assert.Equal(t, 16, len(sb16))
+
+	raw24 := "asdflkjhqwer)(*&CJEN#$%^"
+	sb24 := SecretBytes(raw24)
+	assert.Equal(t, raw24, string(sb24))
+	assert.Equal(t, 24, len(sb24))
+
+	raw32 := "asdflkjhqwer)(*&1234lkjhqwer)(*&"
+	sb32 := SecretBytes(raw32)
+	assert.Equal(t, raw32, string(sb32))
+	assert.Equal(t, 32, len(sb32))
+}
+
 func TestSignAndValidate(t *testing.T) {
 	seed := "0123456789abcdef"
 	key := "cookie-name"