-
Notifications
You must be signed in to change notification settings - Fork 34
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Okta #50
Okta #50
Conversation
@@ -1,63 +1,80 @@ | |||
Key Conjurer |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ignore this, this is mostly my autoformatter going crazy. This will be updated in a separate request when we ahve time.
) | ||
|
||
// Duo scripts the Duo Web API interaction | ||
type Duo struct { | ||
logger *logrus.Entry |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The logging behind Duo was removed because all of the messages that were logged were always logged right before an error - so the messages were moved into the error handling so the caller can handle them.
@@ -1,14 +1,11 @@ | |||
package oneloginduo | |||
package duo |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was moved to re-use the Duo functionality in the Okta client
@@ -0,0 +1,301 @@ | |||
package okta |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file handles working with the /api/v1/authn
API from Okta. This isn't provided to us by the SDK so we have to do it ourselves.
return fmt.Errorf("unable to create %s reason: %w", fp, err) | ||
} | ||
|
||
defer file.Close() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Writes the configuration file changes, if any, and closes the file handle on shutdown
config.TTL = uint(ttl) | ||
return nil | ||
}, | ||
} | ||
|
||
var setTimeRemainingCmd = &cobra.Command{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Logging in was pointless here, so it was removed
|
||
var setTTLCmd = &cobra.Command{ | ||
Use: "ttl <ttl>", | ||
Short: "Sets ttl value in number of hours.", | ||
Long: "Sets ttl value in number of hours.", | ||
Args: cobra.ExactArgs(1), | ||
RunE: func(cmd *cobra.Command, args []string) error { | ||
userData, err := keyconjurer.Login(keyConjurerRcPath, false) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Logging in was pointless here, so it was removed
@@ -0,0 +1,86 @@ | |||
package main |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the new functionality for using the Bastion account, which is a workaround due to technical limitations in AD if I remember correctly. The purpose of this command is to allow users to retrieve temporary session credentials for a "bastion" account, with which they can then "assume role" into any other account.
@@ -23,6 +23,10 @@ | |||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Auto formatter went wild, sorry.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Continuing the review. Just want to get the current comments in before going offline.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me
This PR addresses all the changes required for supporting Okta within KeyConjurer. The README should be ignored as while it is partially up to date it hasn't been updated due to a focus on functionality. I've left comments inline of decisions made if they aren't obvious or seem strange.
Also please do not be intimidated by the file count. A large number of those files were me moving files from one folder to another. This was done in order to better share code between the client and the server or to reduce unnecessary nesting.
That said, this PR was always destined to be large, and was around +2500-2000 before any of the auto-formatting and colocation of code because the changes that needed to be made essentially touched every aspect of the application.