Create a real Okta session & store it in the cache, refresh it on each invocation #102

mipearson · 2021-04-26T06:58:41Z

Uses https://developer.okta.com/docs/reference/api/sessions/#create-session-with-a-session-token to create a session that can be re-used, reducing the number of times we need to log in
Refreshes the session to extend its expiry each time it's used
Misc cleanups to cache logic
Add verbose/debug flags for inspecting what's going on during the login process

cache/cache.go

go.sum

Make cache package responsible for checking if the cache is enabled, simplify cache usage in caller locations

- Maybe we should also clean up the increasingly-repetitice gob.Registers in cache.go?

…ug output

rbvigilante · 2021-05-07T06:58:09Z

cli/login.go

+
+	if err == nil {
+		session.ExpiresAt = response.ExpiresAt
+		cache.Write(oktaSessionCacheKey(), *session, session.ExpiresAt.Sub(time.Now()))


Do we want to cache this for the maximum allowable time? This means it will sit around for something like a week before asking for reauthentication. If we do, perhaps it would be good to be able to configure the session duration separately?

Interestingly, I've been getting single day sessions, not entire week ones.

Interesting. When I dumped sessions, the expiry date was always in something like a week's time. That's actually why I used WriteDefault initially. I guess there's probably not a lot we can do to control this with Okta, so maybe something in config for, like, a maximum allowable session cache time might be nice? I don't know.

Added a flag to limit how long we're willing to keep an Okta session in the cache for without refresh, default to 1 day

rbvigilante · 2021-05-07T07:09:57Z

cli/login.go

@@ -293,7 +331,7 @@ func promptLogin() (okta.OktaAuthResponse, error) {

 	for unauthorised && (retries < maxLoginRetries) {
 		retries++
-		username := viper.GetString("okta.username")
+		username := oktaUsername()


I'm totally fine with this approach, but it is different to what we're doing everywhere else in the codebase; I'd generally err on the side of consistency, but I'm also very happy if we think this is a better way to handle viper to just slowly convert stuff over as we go.

oo, probably didn't need this explicit assignment here - was search-replacing.

But yeah - I'm 50/50 on it - I don't like repeated get("string") things because there's nothing there to stop a misspelled word.

Yeah, I get it. Like I think I've said before, I think there are probably some things we can do to handle the vipers a bit better; we were quite naïve about this whole thing when we first wrote this code.

Alright, let's leave it like this for now; we can come back and fix the snakes another time.

rbvigilante · 2021-05-07T07:10:50Z

okta/okta.go

+	return &session, nil
+}
+
+// TODO: DRY


Has this been left here intentionally, or should we do it as part of this PR?

Intentionally - the way that this code does its HTTP handling needs some thought.

rbvigilante · 2021-05-17T05:40:17Z

cmd/root.go


 	rootCmd.PersistentFlags().StringP("okta-username", "u", "", "Your Okta username")
 	rootCmd.PersistentFlags().String("okta-domain", "", "The domain to use for requests to Okta")
 	rootCmd.PersistentFlags().String("okta-aws-saml-endpoint", "", "The app embed path for the AWS app within Okta")
 	rootCmd.PersistentFlags().String("okta-mfa-type", "", "The Okta MFA type for login")
 	rootCmd.PersistentFlags().String("okta-mfa-provider", "", "The Okta MFA provider name for login")
+	rootCmd.PersistentFlags().Int64("okta-session-cache-limit", 86400, "The maximum amount of time we'll cache a session from Okta (in seconds)")


I think this would be better as a config file only option; it doesn't really make sense to pass it as a command-line argument?

Eh, no strong opinion either way. I like the pragma of being able to specify things on the CLI when it's easy to do so, especially for testing.

I don't actually mind much either way, but again I'd prefer consistency - if we want to allow this to be passed as a flag, we should also allow passing the other cache timeouts this way. Perhaps we could consider this another piece of work?

rbvigilante

OK, I think this is good to go!

rbvigilante · 2021-05-17T07:26:53Z

cli/login.go

@@ -293,7 +331,7 @@ func promptLogin() (okta.OktaAuthResponse, error) {

 	for unauthorised && (retries < maxLoginRetries) {
 		retries++
-		username := viper.GetString("okta.username")
+		username := oktaUsername()


Alright, let's leave it like this for now; we can come back and fix the snakes another time.

rbvigilante force-pushed the mp-use-okta-sessions branch 3 times, most recently from f96f6d4 to 5cd3bc6 Compare May 4, 2021 12:59

rbvigilante reviewed May 5, 2021

View reviewed changes

cache/cache.go Outdated Show resolved Hide resolved

rbvigilante reviewed May 5, 2021

View reviewed changes

go.sum Outdated Show resolved Hide resolved

mipearson and others added 9 commits May 7, 2021 09:47

Scrappy WIP on prototyping session support

de6249f

Unify assume role steps in print_credentials.go & shim.go

801ad1e

Make cache package responsible for checking if the cache is enabled, simplify cache usage in caller locations

Rename AssumeRoleFromCache (it doesn't always use cache)

7641d00

Rename parameters in cache.Check

a428423

Cache the session instead of the SAML payload

55cd331

- Maybe we should also clean up the increasingly-repetitice gob.Registers in cache.go?

Remove pps

457d523

De-dup gob register

d176318

go mod tidy

d6859b1

Add basic logging / verbose flag for debugging caching

ec1a8f1

mipearson force-pushed the mp-use-okta-sessions branch from 0859638 to 280f630 Compare May 7, 2021 06:19

mipearson changed the title ~~Scrappy WIP on prototyping session support~~ Create a real Okta session & store it in the cache, refresh it on each invocation May 7, 2021

mipearson marked this pull request as ready for review May 7, 2021 06:25

mipearson force-pushed the mp-use-okta-sessions branch from 280f630 to 3afb1d5 Compare May 7, 2021 06:32

Refresh our session as a way to test it between runs & add better deb…

6180520

…ug output

mipearson force-pushed the mp-use-okta-sessions branch from 3afb1d5 to 6180520 Compare May 7, 2021 06:35

Remove unneeded gob re-initialisation check

99dc9ef

rbvigilante reviewed May 7, 2021

View reviewed changes

Limit how long we cache our Okta session to a day by default

5b76430

rbvigilante reviewed May 17, 2021

View reviewed changes

Remove CLI flag for now, make config-only

bcdd418

rbvigilante approved these changes May 17, 2021

View reviewed changes

mipearson merged commit db1ac13 into master May 17, 2021

lokulin deleted the mp-use-okta-sessions branch August 17, 2022 10:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create a real Okta session & store it in the cache, refresh it on each invocation #102

Create a real Okta session & store it in the cache, refresh it on each invocation #102

mipearson commented Apr 26, 2021 •

edited

Loading

rbvigilante May 7, 2021

mipearson May 8, 2021

rbvigilante May 9, 2021

mipearson May 17, 2021

rbvigilante May 7, 2021

mipearson May 8, 2021

rbvigilante May 9, 2021

rbvigilante May 17, 2021

rbvigilante May 7, 2021

mipearson May 8, 2021

rbvigilante May 17, 2021

mipearson May 17, 2021

rbvigilante May 17, 2021

rbvigilante left a comment

rbvigilante May 17, 2021

Create a real Okta session & store it in the cache, refresh it on each invocation #102

Create a real Okta session & store it in the cache, refresh it on each invocation #102

Conversation

mipearson commented Apr 26, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rbvigilante left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mipearson commented Apr 26, 2021 •

edited

Loading