git.iko.soy/typeable/oauth2-proxy
on master help sign in
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

commit7e5c8bb57983210322b028301e99a8cf4d9083d3 authorNick Meves <nick.meves@greenhouse.io> dateThu 21 May 2020, 11:29 (6 years ago) parentd228d5a
5 files changed+94−18
CHANGELOG.md+8 oauthproxy_test.go+1−1 options_test.go+1−1 pkg/encryption/cipher.go+11−16 pkg/encryption/cipher_test.go+73
CHANGELOG.md+8
@@ -32,8 +32,16 @@
3232 - In some scenarios `X-Forwarded-User` will now be empty. Use `X-Forwarded-Email` instead.
3333 - In some scenarios, this may break setting Basic Auth on upstream or responses.
3434 Use `--prefer-email-to-user` to restore falling back to the Email in these cases.
35+- [#556](https://github.com/oauth2-proxy/oauth2-proxy/pull/556) Remove unintentional auto-padding of secrets that were too short
36+ - Previously, after cookie-secrets were opportunistically base64 decoded to raw bytes,
37+ they were padded to have a length divisible by 4.
38+ - This led to wrong sized secrets being valid AES lengths of 16, 24, or 32 bytes. Or it led to confusing errors
39+ reporting an invalid length of 20 or 28 when the user input cookie-secret was not that length.
40+ - 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
41+ the direct cookie-secret to bytes without silent padding added.
3542
3643## Changes since v5.1.1
44+- [#556](https://github.com/oauth2-proxy/oauth2-proxy/pull/556) Remove unintentional auto-padding of secrets that were too short (@NickMeves)
3745- [#538](https://github.com/oauth2-proxy/oauth2-proxy/pull/538) Refactor sessions/utils.go functionality to other areas (@NickMeves)
3846- [#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)
3947- [#529](https://github.com/oauth2-proxy/oauth2-proxy/pull/529) Add local test environments for testing changes and new features (@JoelSpeed)
oauthproxy_test.go+1−1
@@ -898,7 +898,7 @@ func NewProcessCookieTest(opts ProcessCookieTestOpts, modifiers ...OptionsModifi
898898 }
899899 pcTest.opts.ClientID = "asdfljk"
900900 pcTest.opts.ClientSecret = "lkjfdsig"
901− pcTest.opts.Cookie.Secret = "0123456789abcdefabcd"
901+ pcTest.opts.Cookie.Secret = "0123456789abcdef0123456789abcdef"
902902 // First, set the CookieRefresh option so proxy.AesCipher is created,
903903 // needed to encrypt the access_token.
904904 pcTest.opts.Cookie.Refresh = time.Hour
options_test.go+1−1
@@ -233,7 +233,7 @@ func TestCookieRefreshMustBeLessThanCookieExpire(t *testing.T) {
233233 o := testOptions()
234234 assert.Equal(t, nil, o.Validate())
235235
236− o.Cookie.Secret = "0123456789abcdefabcd"
236+ o.Cookie.Secret = "0123456789abcdef"
237237 o.Cookie.Refresh = o.Cookie.Expire
238238 assert.NotEqual(t, nil, o.Validate())
239239
pkg/encryption/cipher.go+11−16
@@ -19,27 +19,22 @@ import (
1919
2020// SecretBytes attempts to base64 decode the secret, if that fails it treats the secret as binary
2121func SecretBytes(secret string) []byte {
22− b, err := base64.URLEncoding.DecodeString(addPadding(secret))
22+ b, err := base64.RawURLEncoding.DecodeString(strings.TrimRight(secret, "="))
2323 if err == nil {
24− return []byte(addPadding(string(b)))
24+ // Only return decoded form if a valid AES length
25+ // Don't want unintentional decoding resulting in invalid lengths confusing a user
26+ // that thought they used a 16, 24, 32 length string
27+ for _, i := range []int{16, 24, 32} {
28+ if len(b) == i {
29+ return b
30+ }
31+ }
2532 }
33+ // If decoding didn't work or resulted in non-AES compliant length,
34+ // assume the raw string was the intended secret
2635 return []byte(secret)
2736}
2837
29−func addPadding(secret string) string {
30− padding := len(secret) % 4
31− switch padding {
32− case 1:
33− return secret + "==="
34− case 2:
35− return secret + "=="
36− case 3:
37− return secret + "="
38− default:
39− return secret
40− }
41−}
42−
4338// cookies are stored in a 3 part (value + timestamp + signature) to enforce that the values are as originally set.
4439// additionally, the 'value' is encrypted so it's opaque to the browser
4540
pkg/encryption/cipher_test.go+73
@@ -1,14 +1,87 @@
11package encryption
22
33import (
4+ "crypto/rand"
45 "crypto/sha1"
56 "crypto/sha256"
67 "encoding/base64"
8+ "fmt"
9+ "io"
710 "testing"
811
912 "github.com/stretchr/testify/assert"
1013)
1114
15+func TestSecretBytesEncoded(t *testing.T) {
16+ for _, secretSize := range []int{16, 24, 32} {
17+ t.Run(fmt.Sprintf("%d", secretSize), func(t *testing.T) {
18+ secret := make([]byte, secretSize)
19+ _, err := io.ReadFull(rand.Reader, secret)
20+ assert.Equal(t, nil, err)
21+
22+ // We test both padded & raw Base64 to ensure we handle both
23+ // potential user input routes for Base64
24+ base64Padded := base64.URLEncoding.EncodeToString(secret)
25+ sb := SecretBytes(base64Padded)
26+ assert.Equal(t, secret, sb)
27+ assert.Equal(t, len(sb), secretSize)
28+
29+ base64Raw := base64.RawURLEncoding.EncodeToString(secret)
30+ sb = SecretBytes(base64Raw)
31+ assert.Equal(t, secret, sb)
32+ assert.Equal(t, len(sb), secretSize)
33+ })
34+ }
35+}
36+
37+// A string that isn't intended as Base64 and still decodes (but to unintended length)
38+// will return the original secret as bytes
39+func TestSecretBytesEncodedWrongSize(t *testing.T) {
40+ for _, secretSize := range []int{15, 20, 28, 33, 44} {
41+ t.Run(fmt.Sprintf("%d", secretSize), func(t *testing.T) {
42+ secret := make([]byte, secretSize)
43+ _, err := io.ReadFull(rand.Reader, secret)
44+ assert.Equal(t, nil, err)
45+
46+ // We test both padded & raw Base64 to ensure we handle both
47+ // potential user input routes for Base64
48+ base64Padded := base64.URLEncoding.EncodeToString(secret)
49+ sb := SecretBytes(base64Padded)
50+ assert.NotEqual(t, secret, sb)
51+ assert.NotEqual(t, len(sb), secretSize)
52+ // The given secret is returned as []byte
53+ assert.Equal(t, base64Padded, string(sb))
54+
55+ base64Raw := base64.RawURLEncoding.EncodeToString(secret)
56+ sb = SecretBytes(base64Raw)
57+ assert.NotEqual(t, secret, sb)
58+ assert.NotEqual(t, len(sb), secretSize)
59+ // The given secret is returned as []byte
60+ assert.Equal(t, base64Raw, string(sb))
61+ })
62+ }
63+}
64+
65+func TestSecretBytesNonBase64(t *testing.T) {
66+ trailer := "equals=========="
67+ assert.Equal(t, trailer, string(SecretBytes(trailer)))
68+
69+ raw16 := "asdflkjhqwer)(*&"
70+ sb16 := SecretBytes(raw16)
71+ assert.Equal(t, raw16, string(sb16))
72+ assert.Equal(t, 16, len(sb16))
73+
74+ raw24 := "asdflkjhqwer)(*&CJEN#$%^"
75+ sb24 := SecretBytes(raw24)
76+ assert.Equal(t, raw24, string(sb24))
77+ assert.Equal(t, 24, len(sb24))
78+
79+ raw32 := "asdflkjhqwer)(*&1234lkjhqwer)(*&"
80+ sb32 := SecretBytes(raw32)
81+ assert.Equal(t, raw32, string(sb32))
82+ assert.Equal(t, 32, len(sb32))
83+}
84+
1285func TestSignAndValidate(t *testing.T) {
1386 seed := "0123456789abcdef"
1487 key := "cookie-name"