Move Cipher and Session Store initialisation out of Validation #577

JoelSpeed · 2020-05-25T13:05:11Z

Description

Refactor the code to move the initialisation of the cipher and the session store out of the validation package. Validation should be static analysis of the options configured and should not be testing anything (eg connecting to redis)

Motivation and Context

Trying to simplify the validation code and give it a clear boundary on what it should and shouldn't be doing

How Has This Been Tested?

Unit tests only

Checklist:

My change requires a change to the documentation or CHANGELOG.
I have updated the documentation/CHANGELOG accordingly.
I have created a feature (non-master) branch for my PR.

NickMeves · 2020-06-27T18:49:36Z

logging_handler_test.go

 				opts.Logging.ExcludePaths = []string{"/ping"}
 			}
 			opts.RawRedirectURL = "localhost"
 			validation.Validate(opts)


I've started altering these to:

err := validation.Validate(opts) assert.NoError(t, err)

Wherever I saw them -- otherwise nil point deference panics were creeping in that were much harder to triage the issue quickly.

And my IDE yells at me whenever I try to commit with an unhandled error 😄

Will replace some of them as I go through, don't want to expand the scope of the PR too much though so will only do it near where I've already changed stuff, can make this iterative

NickMeves · 2020-06-27T18:52:11Z

oauthproxy_test.go

 	backendURL, _ := url.Parse(backend.URL)

-	options := options.NewOptions()
+	options := baseTestOptions()


I've started renaming these to opts whenever I saw them in tests as well. I think options shadows the same name options import.

NickMeves · 2020-06-27T18:56:12Z

oauthproxy_test.go

 		return email == emailAddress
 	})
+	if err != nil {
+		panic(err)


I was trying to avoid panics in these helper structs and raised errors that any actual tests with a *testing.T needed to handle and verify no errors myself.

Want to just leave them for now and I'll handle the cleanup in the msgpack sessions PR?

If you've already sorted this in yours, then I'll leave them as panics and you can just clean up when you rebase if that's ok?

NickMeves · 2020-06-27T18:57:16Z

oauthproxy_test.go

+		proxy, err := NewOAuthProxy(opts, func(email string) bool {
 			return email == emailAddress
 		})
+		assert.Equal(t, nil, err)


Is there a benefit to this over assert.NoError(t, err) or just a style preference?

Not really, in fact NoError is probably better, have changed a bunch

NickMeves · 2020-06-27T18:59:17Z

oauthproxy_test.go

-	opts := options.NewOptions()
+	opts := baseTestOptions()
 	opts.Upstreams = append(opts.Upstreams, upstream.URL)
 	opts.ClientID = "aljsal"


These aren't needed since the baseTestOptions sets alternatives and it looks like we aren't asserting a future value is set to these in any of these tests.

I think this applies to most of these cases with lingering ClientID, ClientSecret, & Cookie.Secrets

NickMeves · 2020-06-27T19:05:45Z

oauthproxy_test.go

 	opts.Cookie.Domains = []string{"abc"}
-	store, err := cookie.NewCookieSessionStore(&opts.Session, &opts.Cookie)
-	assert.Equal(t, err, nil)
+	cipher, err := encryption.NewBase64Cipher(encryption.NewCFBCipher, encryption.SecretBytes(opts.Cookie.Secret))


I think it would be advantageous in 1 of these 2 tests to flip to the base64 version of the cookie secret option to get coverage of both formats

I've set this one to use the base64. I know what you mean, but ideally it shouldn't matter as we are testing that elsewhere, long term I'd like to see an end to end suite that allows us to flex this kind of thing, long term meaning very long term 😅

NickMeves · 2020-06-27T19:13:20Z

pkg/sessions/session_store.go


 // NewSessionStore creates a SessionStore from the provided configuration
 func NewSessionStore(opts *options.SessionOptions, cookieOpts *options.CookieOptions) (sessions.SessionStore, error) {
+	cipher, err := encryption.NewBase64Cipher(encryption.NewCFBCipher, encryption.SecretBytes(cookieOpts.Secret))


Does this potentially belong inside of the specific New<Type>SessionStore where they can initialize from the cookieOpts.Secret they have access to?

One of the things I'm looking at in the session state refactor is more variability of the cipher based on the security scenarios each store has.

At the moment I'm looking at the cookies themselves always using AES-CFB (since it doesn't have repeat IV weakness). But for the inner second encryption of DB store values, AES-GCM is much better suited.

This works fine with that design (since I intend to use CFB on all store's cookies) -- just was curious if we wanted to future proof ourselves a bit and give the particular session full control of its crypto algorithm decisions.

One of the things I'm looking at in the session state refactor is more variability of the cipher based on the security scenarios each store has.

Hadn't really thought about this as it's not something we've done so far, we've always passed the same cipher through. That said, it makes a lot of sense!

I'll take a look at moving it in, or we could do that in another PR later, up to you

Ok I moved it. We need to be vigilant about this and make sure that as we add more session stores that we ensure they're encrypted. I can't remember if the tests cover this or not, but if they don't we should make sure they do (I think there's a test that changes the cookie secret and ensures decryption fails)

NickMeves

Overall looks good to me! I left some style/design questions that we can either work out here or I can sort out in #632 as that PR bumps into them.

JoelSpeed

Updated the tests as per your suggestions, where it made sense I think anyway, I think we can iterate on this as we make changes

I haven't looked at moving the initialisation into the session store just yet, will see if I can find some time

JoelSpeed · 2020-06-28T11:17:19Z

logging_handler_test.go

 				opts.Logging.ExcludePaths = []string{"/ping"}
 			}
 			opts.RawRedirectURL = "localhost"
 			validation.Validate(opts)


Will replace some of them as I go through, don't want to expand the scope of the PR too much though so will only do it near where I've already changed stuff, can make this iterative

JoelSpeed · 2020-06-28T11:18:49Z

oauthproxy_test.go

+		proxy, err := NewOAuthProxy(opts, func(email string) bool {
 			return email == emailAddress
 		})
+		assert.Equal(t, nil, err)


Not really, in fact NoError is probably better, have changed a bunch

JoelSpeed · 2020-06-28T11:19:40Z

oauthproxy_test.go

 		return email == emailAddress
 	})
+	if err != nil {
+		panic(err)


If you've already sorted this in yours, then I'll leave them as panics and you can just clean up when you rebase if that's ok?

JoelSpeed · 2020-06-28T11:27:04Z

oauthproxy_test.go

 	opts.Cookie.Domains = []string{"abc"}
-	store, err := cookie.NewCookieSessionStore(&opts.Session, &opts.Cookie)
-	assert.Equal(t, err, nil)
+	cipher, err := encryption.NewBase64Cipher(encryption.NewCFBCipher, encryption.SecretBytes(opts.Cookie.Secret))


I've set this one to use the base64. I know what you mean, but ideally it shouldn't matter as we are testing that elsewhere, long term I'd like to see an end to end suite that allows us to flex this kind of thing, long term meaning very long term 😅

JoelSpeed · 2020-06-28T11:31:32Z

pkg/sessions/session_store.go


 // NewSessionStore creates a SessionStore from the provided configuration
 func NewSessionStore(opts *options.SessionOptions, cookieOpts *options.CookieOptions) (sessions.SessionStore, error) {
+	cipher, err := encryption.NewBase64Cipher(encryption.NewCFBCipher, encryption.SecretBytes(cookieOpts.Secret))


One of the things I'm looking at in the session state refactor is more variability of the cipher based on the security scenarios each store has.

Hadn't really thought about this as it's not something we've done so far, we've always passed the same cipher through. That said, it makes a lot of sense!

I'll take a look at moving it in, or we could do that in another PR later, up to you

NickMeves

LGTM! 🚀

JoelSpeed · 2020-06-28T17:30:16Z

Thanks for the review @NickMeves

JoelSpeed added the refactor label May 25, 2020

JoelSpeed requested a review from steakunderscore as a code owner May 25, 2020 13:05

JoelSpeed force-pushed the session-store-cipher branch 3 times, most recently from 9d3368d to be4d65d Compare May 31, 2020 14:05

JoelSpeed force-pushed the session-store-cipher branch from be4d65d to 2234306 Compare June 6, 2020 19:07

JoelSpeed force-pushed the session-store-cipher branch from 2234306 to d59f97e Compare June 14, 2020 14:41

NickMeves reviewed Jun 27, 2020

View reviewed changes

NickMeves previously approved these changes Jun 27, 2020

View reviewed changes

JoelSpeed added 3 commits June 28, 2020 12:03

Move Cipher intialisation to session store initialisation

c8dbf1c

Initialise Session Storage in NewOAuthProxy instead of validation

5ce9e75

Update changelog for session storage initialisation move

7784639

JoelSpeed dismissed NickMeves’s stale review via 7784639 June 28, 2020 11:32

JoelSpeed force-pushed the session-store-cipher branch from d59f97e to 7784639 Compare June 28, 2020 11:32

JoelSpeed commented Jun 28, 2020

View reviewed changes

Switch to in session store initialisation

6e1b3b9

JoelSpeed force-pushed the session-store-cipher branch from edb321f to 6e1b3b9 Compare June 28, 2020 11:51

NickMeves approved these changes Jun 28, 2020

View reviewed changes

JoelSpeed merged commit d9a45a3 into master Jun 28, 2020

JoelSpeed deleted the session-store-cipher branch July 5, 2020 09:02

Uh oh!

Move Cipher and Session Store initialisation out of Validation #577

Move Cipher and Session Store initialisation out of Validation #577

Uh oh!

Conversation

JoelSpeed commented May 25, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Motivation and Context

How Has This Been Tested?

Checklist:

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

NickMeves left a comment

Choose a reason for hiding this comment

Uh oh!

JoelSpeed left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

NickMeves left a comment

Choose a reason for hiding this comment

Uh oh!

JoelSpeed commented Jun 28, 2020

Uh oh!

Uh oh!

JoelSpeed commented May 25, 2020 •

edited

Loading