From 2b15ba0bcfc4fc734625637f73d29b28100d0287 Mon Sep 17 00:00:00 2001 From: Nick Meves Date: Sat, 7 Nov 2020 14:58:47 -0800 Subject: [PATCH] Remove v5 JSON session support --- CHANGELOG.md | 2 + pkg/apis/sessions/legacy_v5_tester.go | 87 ------------------------- pkg/apis/sessions/session_state.go | 67 ++++--------------- pkg/apis/sessions/session_state_test.go | 73 --------------------- pkg/sessions/cookie/session_store.go | 16 +---- pkg/sessions/persistence/ticket.go | 35 +--------- pkg/sessions/persistence/ticket_test.go | 66 ------------------- 7 files changed, 16 insertions(+), 330 deletions(-) delete mode 100644 pkg/apis/sessions/legacy_v5_tester.go diff --git a/CHANGELOG.md b/CHANGELOG.md index cbbf89bf..f272757e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,7 @@ ## Important Notes +- [#905](https://github.com/oauth2-proxy/oauth2-proxy/pull/905) Existing sessions from v6.0.0 or earlier are no longer valid. They will trigger a reauthentication. - [#826](https://github.com/oauth2-proxy/oauth2-proxy/pull/826) `skip-auth-strip-headers` now applies to all requests, not just those where authentication would be skipped. - [#789](https://github.com/oauth2-proxy/oauth2-proxy/pull/789) `--skip-auth-route` is (almost) backwards compatible with `--skip-auth-regex` - We are marking `--skip-auth-regex` as DEPRECATED and will remove it in the next major version. @@ -33,6 +34,7 @@ ## Changes since v6.1.1 +- [#905](https://github.com/oauth2-proxy/oauth2-proxy/pull/905) Remove v5 legacy sessions support (@NickMeves) - [#904](https://github.com/oauth2-proxy/oauth2-proxy/pull/904) Set `skip-auth-strip-headers` to `true` by default (@NickMeves) - [#826](https://github.com/oauth2-proxy/oauth2-proxy/pull/826) Integrate new header injectors into project (@JoelSpeed) - [#898](https://github.com/oauth2-proxy/oauth2-proxy/pull/898) Migrate documentation to Docusaurus (@JoelSpeed) diff --git a/pkg/apis/sessions/legacy_v5_tester.go b/pkg/apis/sessions/legacy_v5_tester.go deleted file mode 100644 index 8fab4eae..00000000 --- a/pkg/apis/sessions/legacy_v5_tester.go +++ /dev/null @@ -1,87 +0,0 @@ -package sessions - -import ( - "fmt" - "testing" - "time" - - "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/encryption" - "github.com/stretchr/testify/assert" -) - -const LegacyV5TestSecret = "0123456789abcdefghijklmnopqrstuv" - -// LegacyV5TestCase provides V5 JSON based test cases for legacy fallback code -type LegacyV5TestCase struct { - Input string - Error bool - Output *SessionState -} - -// CreateLegacyV5TestCases makes various V5 JSON sessions as test cases -// -// Used for `apis/sessions/session_state_test.go` & `sessions/redis/redis_store_test.go` -// -// TODO: Remove when this is deprecated (likely V7) -func CreateLegacyV5TestCases(t *testing.T) (map[string]LegacyV5TestCase, encryption.Cipher, encryption.Cipher) { - created := time.Now() - createdJSON, err := created.MarshalJSON() - assert.NoError(t, err) - createdString := string(createdJSON) - e := time.Now().Add(time.Duration(1) * time.Hour) - eJSON, err := e.MarshalJSON() - assert.NoError(t, err) - eString := string(eJSON) - - cfbCipher, err := encryption.NewCFBCipher([]byte(LegacyV5TestSecret)) - assert.NoError(t, err) - legacyCipher := encryption.NewBase64Cipher(cfbCipher) - - testCases := map[string]LegacyV5TestCase{ - "User & email unencrypted": { - Input: `{"Email":"user@domain.com","User":"just-user"}`, - Error: true, - }, - "Only email unencrypted": { - Input: `{"Email":"user@domain.com"}`, - Error: true, - }, - "Just user unencrypted": { - Input: `{"User":"just-user"}`, - Error: true, - }, - "User and Email unencrypted while rest is encrypted": { - Input: fmt.Sprintf(`{"Email":"user@domain.com","User":"just-user","AccessToken":"I6s+ml+/MldBMgHIiC35BTKTh57skGX24w==","IDToken":"xojNdyyjB1HgYWh6XMtXY/Ph5eCVxa1cNsklJw==","RefreshToken":"qEX0x6RmASxo4dhlBG6YuRs9Syn/e9sHu/+K","CreatedAt":%s,"ExpiresOn":%s}`, createdString, eString), - Error: true, - }, - "Full session with cipher": { - Input: fmt.Sprintf(`{"Email":"FsKKYrTWZWrxSOAqA/fTNAUZS5QWCqOBjuAbBlbVOw==","User":"rT6JP3dxQhxUhkWrrd7yt6c1mDVyQCVVxw==","AccessToken":"I6s+ml+/MldBMgHIiC35BTKTh57skGX24w==","IDToken":"xojNdyyjB1HgYWh6XMtXY/Ph5eCVxa1cNsklJw==","RefreshToken":"qEX0x6RmASxo4dhlBG6YuRs9Syn/e9sHu/+K","CreatedAt":%s,"ExpiresOn":%s}`, createdString, eString), - Output: &SessionState{ - Email: "user@domain.com", - User: "just-user", - AccessToken: "token1234", - IDToken: "rawtoken1234", - CreatedAt: &created, - ExpiresOn: &e, - RefreshToken: "refresh4321", - }, - }, - "Minimal session encrypted with cipher": { - Input: `{"Email":"EGTllJcOFC16b7LBYzLekaHAC5SMMSPdyUrg8hd25g==","User":"rT6JP3dxQhxUhkWrrd7yt6c1mDVyQCVVxw=="}`, - Output: &SessionState{ - Email: "user@domain.com", - User: "just-user", - }, - }, - "Unencrypted User, Email and AccessToken": { - Input: `{"Email":"user@domain.com","User":"just-user","AccessToken":"X"}`, - Error: true, - }, - "Unencrypted User, Email and IDToken": { - Input: `{"Email":"user@domain.com","User":"just-user","IDToken":"XXXX"}`, - Error: true, - }, - } - - return testCases, cfbCipher, legacyCipher -} diff --git a/pkg/apis/sessions/session_state.go b/pkg/apis/sessions/session_state.go index 03bc747a..8e9d006b 100644 --- a/pkg/apis/sessions/session_state.go +++ b/pkg/apis/sessions/session_state.go @@ -2,7 +2,6 @@ package sessions import ( "bytes" - "encoding/json" "errors" "fmt" "io" @@ -18,15 +17,17 @@ import ( // SessionState is used to store information about the currently authenticated user session type SessionState struct { - AccessToken string `json:",omitempty" msgpack:"at,omitempty"` - IDToken string `json:",omitempty" msgpack:"it,omitempty"` - CreatedAt *time.Time `json:",omitempty" msgpack:"ca,omitempty"` - ExpiresOn *time.Time `json:",omitempty" msgpack:"eo,omitempty"` - RefreshToken string `json:",omitempty" msgpack:"rt,omitempty"` - Email string `json:",omitempty" msgpack:"e,omitempty"` - User string `json:",omitempty" msgpack:"u,omitempty"` - Groups []string `json:",omitempty" msgpack:"g,omitempty"` - PreferredUsername string `json:",omitempty" msgpack:"pu,omitempty"` + CreatedAt *time.Time `msgpack:"ca,omitempty"` + ExpiresOn *time.Time `msgpack:"eo,omitempty"` + + AccessToken string `msgpack:"at,omitempty"` + IDToken string `msgpack:"it,omitempty"` + RefreshToken string `msgpack:"rt,omitempty"` + + Email string `msgpack:"e,omitempty"` + User string `msgpack:"u,omitempty"` + Groups []string `msgpack:"g,omitempty"` + PreferredUsername string `msgpack:"pu,omitempty"` } // IsExpired checks whether the session has expired @@ -146,52 +147,6 @@ func DecodeSessionState(data []byte, c encryption.Cipher, compressed bool) (*Ses return &ss, nil } -// LegacyV5DecodeSessionState decodes a legacy JSON session cookie string into a SessionState -func LegacyV5DecodeSessionState(v string, c encryption.Cipher) (*SessionState, error) { - var ss SessionState - err := json.Unmarshal([]byte(v), &ss) - if err != nil { - return nil, fmt.Errorf("error unmarshalling session: %w", err) - } - - for _, s := range []*string{ - &ss.User, - &ss.Email, - &ss.PreferredUsername, - &ss.AccessToken, - &ss.IDToken, - &ss.RefreshToken, - } { - err := into(s, c.Decrypt) - if err != nil { - return nil, err - } - } - err = ss.validate() - if err != nil { - return nil, err - } - - return &ss, nil -} - -// codecFunc is a function that takes a []byte and encodes/decodes it -type codecFunc func([]byte) ([]byte, error) - -func into(s *string, f codecFunc) error { - // Do not encrypt/decrypt nil or empty strings - if s == nil || *s == "" { - return nil - } - - d, err := f([]byte(*s)) - if err != nil { - return err - } - *s = string(d) - return nil -} - // lz4Compress compresses with LZ4 // // The Compress:Decompress ratio is 1:Many. LZ4 gives fastest decompress speeds diff --git a/pkg/apis/sessions/session_state_test.go b/pkg/apis/sessions/session_state_test.go index 4a91bdec..c28420a9 100644 --- a/pkg/apis/sessions/session_state_test.go +++ b/pkg/apis/sessions/session_state_test.go @@ -4,7 +4,6 @@ import ( "crypto/rand" "fmt" "io" - mathrand "math/rand" "testing" "time" @@ -257,78 +256,6 @@ func TestEncodeAndDecodeSessionState(t *testing.T) { } } -// TestLegacyV5DecodeSessionState confirms V5 JSON sessions decode -// -// TODO: Remove when this is deprecated (likely V7) -func TestLegacyV5DecodeSessionState(t *testing.T) { - testCases, cipher, legacyCipher := CreateLegacyV5TestCases(t) - - for testName, tc := range testCases { - t.Run(testName, func(t *testing.T) { - // Legacy sessions fail in DecodeSessionState which results in - // the fallback to LegacyV5DecodeSessionState - _, err := DecodeSessionState([]byte(tc.Input), cipher, false) - assert.Error(t, err) - _, err = DecodeSessionState([]byte(tc.Input), cipher, true) - assert.Error(t, err) - - ss, err := LegacyV5DecodeSessionState(tc.Input, legacyCipher) - if tc.Error { - assert.Error(t, err) - assert.Nil(t, ss) - return - } - assert.NoError(t, err) - compareSessionStates(t, tc.Output, ss) - }) - } -} - -// Test_into tests the into helper function used in LegacyV5DecodeSessionState -// -// TODO: Remove when this is deprecated (likely V7) -func Test_into(t *testing.T) { - const charset = "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789" - - // 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) - - cfb, err := encryption.NewCFBCipher(secret) - assert.NoError(t, err) - c := encryption.NewBase64Cipher(cfb) - - // Check no errors with empty or nil strings - empty := "" - assert.Equal(t, nil, into(&empty, c.Encrypt)) - assert.Equal(t, nil, into(&empty, c.Decrypt)) - assert.Equal(t, nil, into(nil, c.Encrypt)) - assert.Equal(t, nil, into(nil, c.Decrypt)) - - // Test various sizes tokens might be - for _, dataSize := range []int{10, 100, 1000, 5000, 10000} { - t.Run(fmt.Sprintf("%d", dataSize), func(t *testing.T) { - b := make([]byte, dataSize) - for i := range b { - b[i] = charset[mathrand.Intn(len(charset))] - } - data := string(b) - originalData := data - - assert.Equal(t, nil, into(&data, c.Encrypt)) - assert.NotEqual(t, originalData, data) - - assert.Equal(t, nil, into(&data, c.Decrypt)) - assert.Equal(t, originalData, data) - }) - } - }) - } -} - func compareSessionStates(t *testing.T, expected *SessionState, actual *SessionState) { if expected.CreatedAt != nil { assert.NotNil(t, actual.CreatedAt) diff --git a/pkg/sessions/cookie/session_store.go b/pkg/sessions/cookie/session_store.go index 461e08ea..4a546cfc 100644 --- a/pkg/sessions/cookie/session_store.go +++ b/pkg/sessions/cookie/session_store.go @@ -60,7 +60,7 @@ func (s *SessionStore) Load(req *http.Request) (*sessions.SessionState, error) { return nil, errors.New("cookie signature not valid") } - session, err := sessionFromCookie(val, s.CookieCipher) + session, err := sessions.DecodeSessionState(val, s.CookieCipher, true) if err != nil { return nil, err } @@ -98,20 +98,6 @@ func (s *SessionStore) cookieForSession(ss *sessions.SessionState) ([]byte, erro return ss.EncodeSessionState(s.CookieCipher, true) } -// sessionFromCookie deserializes a session from a cookie value -func sessionFromCookie(v []byte, c encryption.Cipher) (s *sessions.SessionState, err error) { - ss, err := sessions.DecodeSessionState(v, c, true) - // If anything fails (Decrypt, LZ4, MessagePack), try legacy JSON decode - // LZ4 will likely fail for wrong header after AES-CFB spits out garbage - // data from trying to decrypt JSON it things is ciphertext - if err != nil { - // Legacy used Base64 + AES CFB - legacyCipher := encryption.NewBase64Cipher(c) - return sessions.LegacyV5DecodeSessionState(string(v), legacyCipher) - } - return ss, nil -} - // setSessionCookie adds the user's session cookie to the response func (s *SessionStore) setSessionCookie(rw http.ResponseWriter, req *http.Request, val []byte, created time.Time) error { cookies, err := s.makeSessionCookie(req, val, created) diff --git a/pkg/sessions/persistence/ticket.go b/pkg/sessions/persistence/ticket.go index eb3aafc4..020f35e9 100644 --- a/pkg/sessions/persistence/ticket.go +++ b/pkg/sessions/persistence/ticket.go @@ -2,7 +2,6 @@ package persistence import ( "crypto/aes" - "crypto/cipher" "crypto/rand" "encoding/base64" "encoding/hex" @@ -123,8 +122,6 @@ func (t *ticket) saveSession(s *sessions.SessionState, saver saveFunc) error { // loadSession loads a session from the disk store via the passed loadFunc // using the ticket.id as the key. It then decodes the SessionState using // ticket.secret to make the AES-GCM cipher. -// -// TODO (@NickMeves): Remove legacyV5LoadSession support in V7 func (t *ticket) loadSession(loader loadFunc) (*sessions.SessionState, error) { ciphertext, err := loader(t.id) if err != nil { @@ -134,11 +131,8 @@ func (t *ticket) loadSession(loader loadFunc) (*sessions.SessionState, error) { if err != nil { return nil, err } - ss, err := sessions.DecodeSessionState(ciphertext, c, false) - if err != nil { - return t.legacyV5LoadSession(ciphertext) - } - return ss, nil + + return sessions.DecodeSessionState(ciphertext, c, false) } // clearSession uses the passed clearFunc to delete a session stored with a @@ -203,28 +197,3 @@ func (t *ticket) makeCipher() (encryption.Cipher, error) { } return c, nil } - -// legacyV5LoadSession loads a Redis session created in V5 with historical logic -// -// TODO (@NickMeves): Remove in V7 -func (t *ticket) legacyV5LoadSession(resultBytes []byte) (*sessions.SessionState, error) { - block, err := aes.NewCipher(t.secret) - if err != nil { - return nil, fmt.Errorf("failed to create a legacy AES-CFB cipher from the ticket secret: %v", err) - } - - stream := cipher.NewCFBDecrypter(block, t.secret) - stream.XORKeyStream(resultBytes, resultBytes) - - cfbCipher, err := encryption.NewCFBCipher(encryption.SecretBytes(t.options.Secret)) - if err != nil { - return nil, err - } - legacyCipher := encryption.NewBase64Cipher(cfbCipher) - - session, err := sessions.LegacyV5DecodeSessionState(string(resultBytes), legacyCipher) - if err != nil { - return nil, err - } - return session, nil -} diff --git a/pkg/sessions/persistence/ticket_test.go b/pkg/sessions/persistence/ticket_test.go index 0a121bb0..001a306f 100644 --- a/pkg/sessions/persistence/ticket_test.go +++ b/pkg/sessions/persistence/ticket_test.go @@ -1,14 +1,9 @@ package persistence import ( - "crypto/aes" - "crypto/cipher" - "crypto/rand" "encoding/base64" "errors" "fmt" - "io" - "testing" "time" "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/apis/options" @@ -153,64 +148,3 @@ var _ = Describe("Session Ticket Tests", func() { }) }) }) - -// TestLegacyV5DecodeSession tests the fallback to LegacyV5DecodeSession -// when a V5 encoded session is in Redis -// -// TODO (@NickMeves): Remove when this is deprecated (likely V7) -func Test_legacyV5LoadSession(t *testing.T) { - testCases, _, _ := sessions.CreateLegacyV5TestCases(t) - - for testName, tc := range testCases { - t.Run(testName, func(t *testing.T) { - g := NewWithT(t) - - secret := make([]byte, aes.BlockSize) - _, err := io.ReadFull(rand.Reader, secret) - g.Expect(err).ToNot(HaveOccurred()) - tckt := &ticket{ - secret: secret, - options: &options.Cookie{ - Secret: base64.RawURLEncoding.EncodeToString([]byte(sessions.LegacyV5TestSecret)), - }, - } - - encrypted, err := legacyStoreValue(tc.Input, tckt.secret) - g.Expect(err).ToNot(HaveOccurred()) - - ss, err := tckt.legacyV5LoadSession(encrypted) - if tc.Error { - g.Expect(err).To(HaveOccurred()) - g.Expect(ss).To(BeNil()) - return - } - g.Expect(err).ToNot(HaveOccurred()) - - // Compare sessions without *time.Time fields - exp := *tc.Output - exp.CreatedAt = nil - exp.ExpiresOn = nil - act := *ss - act.CreatedAt = nil - act.ExpiresOn = nil - g.Expect(exp).To(Equal(act)) - }) - } -} - -// legacyStoreValue implements the legacy V5 Redis persistence AES-CFB value encryption -// -// TODO (@NickMeves): Remove when this is deprecated (likely V7) -func legacyStoreValue(value string, ticketSecret []byte) ([]byte, error) { - ciphertext := make([]byte, len(value)) - block, err := aes.NewCipher(ticketSecret) - if err != nil { - return nil, fmt.Errorf("error initiating cipher block: %v", err) - } - - // Use secret as the Initialization Vector too, because each entry has it's own key - stream := cipher.NewCFBEncrypter(block, ticketSecret) - stream.XORKeyStream(ciphertext, []byte(value)) - - return ciphertext, nil -}