From 139e6efadc4f65d805304b3421931b5f954ecb3b Mon Sep 17 00:00:00 2001 From: Mike Mondragon Date: Wed, 8 Jan 2025 09:23:49 -0800 Subject: [PATCH 1/7] Additional documentation about the nature of the OktaYamlConfigProfile struct. --- internal/config/config.go | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/internal/config/config.go b/internal/config/config.go index eb1e7b6..49ff4a3 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -208,7 +208,9 @@ type OktaYamlConfig struct { } `yaml:"awscli"` } -// OktaYamlConfigProfile represents config settings that are indexed by profile name +// OktaYamlConfigProfile represents config settings that are indexed by profile +// name. This is a convenience struct pretty printing profile information from +// the list profiles command cmd/root/profileslist/profiles-list.go type OktaYamlConfigProfile struct { AllProfiles string `yaml:"all-profiles"` AuthzID string `yaml:"authz-id"` From 40eee205c832bbdc293407acf8e8858b00bc21da Mon Sep 17 00:00:00 2001 From: Mike Mondragon Date: Wed, 8 Jan 2025 09:25:37 -0800 Subject: [PATCH 2/7] Okta config path helper function doesn't need to be public. --- internal/config/config.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/internal/config/config.go b/internal/config/config.go index 49ff4a3..46d239f 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -420,7 +420,7 @@ func readConfig() (Attributes, error) { viper.SetConfigType("yaml") yamlData, err := yaml.Marshal(&profiles) if err != nil { - path, _ := OktaConfigPath() + path, _ := oktaConfigPath() fmt.Fprintf(os.Stderr, "WARNING: error reading from %q: %+v.\n\n", path, err) } if err == nil { @@ -943,9 +943,9 @@ func (c *Config) UserAgent() string { return longUserAgent } -// OktaConfigPath returns OS specific path to the okta config file, for example +// oktaConfigPath returns OS specific path to the okta config file, for example // $HOME/.okta/okta.yaml -func OktaConfigPath() (path string, err error) { +func oktaConfigPath() (path string, err error) { var homeDir string homeDir, err = os.UserHomeDir() if err != nil { @@ -958,7 +958,7 @@ func OktaConfigPath() (path string, err error) { // OktaConfig returns an Okta YAML Config object representation of $HOME/.okta/okta.yaml func OktaConfig() (config *OktaYamlConfig, err error) { - configPath, err := OktaConfigPath() + configPath, err := oktaConfigPath() if err != nil { return } From de0593ab0ec1e7b4f46f48d25dda56ef6bc97f42 Mon Sep 17 00:00:00 2001 From: Mike Mondragon Date: Wed, 8 Jan 2025 09:34:04 -0800 Subject: [PATCH 3/7] Refactor to idiomatic naming of constructors and variables to prepare for a follow on feature refactor. --- cmd/root/web/web.go | 2 +- internal/config/config.go | 12 ++++++------ internal/webssoauth/webssoauth.go | 12 ++++++------ 3 files changed, 13 insertions(+), 13 deletions(-) diff --git a/cmd/root/web/web.go b/cmd/root/web/web.go index 99cf795..f84de69 100644 --- a/cmd/root/web/web.go +++ b/cmd/root/web/web.go @@ -86,7 +86,7 @@ func NewWebCommand() *cobra.Command { return err } - _, err = config.OktaConfig() + _, err = config.NewOktaYamlConfig() if err != nil { if _, pathError := err.(*fs.PathError); !pathError { // Warn if okta.yaml exists and there is an error with it. diff --git a/internal/config/config.go b/internal/config/config.go index 46d239f..fca7f7d 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -399,8 +399,8 @@ func getFlagNameFromProfile(awsProfile, flag string) string { // ReadConfigProfileKeys returns the config profile names func (c *Config) ReadConfigProfileKeys() ([]string, error) { // Side loading multiple profiles from okta.yaml file if it exists - if oktaConfig, err := OktaConfig(); err == nil { - profiles := oktaConfig.AWSCLI.PROFILES + if oktaYamlConfig, err := NewOktaYamlConfig(); err == nil { + profiles := oktaYamlConfig.AWSCLI.PROFILES keys := make([]string, 0, len(profiles)) @@ -415,8 +415,8 @@ func (c *Config) ReadConfigProfileKeys() ([]string, error) { func readConfig() (Attributes, error) { // Side loading multiple profiles from okta.yaml file if it exists - if oktaConfig, err := OktaConfig(); err == nil { - profiles := oktaConfig.AWSCLI.PROFILES + if oktaYamlConfig, err := NewOktaYamlConfig(); err == nil { + profiles := oktaYamlConfig.AWSCLI.PROFILES viper.SetConfigType("yaml") yamlData, err := yaml.Marshal(&profiles) if err != nil { @@ -956,8 +956,8 @@ func oktaConfigPath() (path string, err error) { return } -// OktaConfig returns an Okta YAML Config object representation of $HOME/.okta/okta.yaml -func OktaConfig() (config *OktaYamlConfig, err error) { +// NewOktaYamlConfig returns an Okta YAML Config object representation of $HOME/.okta/okta.yaml +func NewOktaYamlConfig() (config *OktaYamlConfig, err error) { configPath, err := oktaConfigPath() if err != nil { return diff --git a/internal/webssoauth/webssoauth.go b/internal/webssoauth/webssoauth.go index 8a42fb7..b37aadb 100644 --- a/internal/webssoauth/webssoauth.go +++ b/internal/webssoauth/webssoauth.go @@ -250,9 +250,9 @@ func (w *WebSSOAuthentication) selectFedApp(apps []*okta.Application) (string, e choices := make([]string, len(apps)) var selected string var configIDPs map[string]string - oktaConfig, err := config.OktaConfig() + oktaYamlConfig, err := config.NewOktaYamlConfig() if err == nil { - configIDPs = oktaConfig.AWSCLI.IDPS + configIDPs = oktaYamlConfig.AWSCLI.IDPS } for i, app := range apps { @@ -459,10 +459,10 @@ func (w *WebSSOAuthentication) choiceFriendlyLabelRole(arn string, roles map[str // promptForRole prompt operator for the AWS Role ARN given a slice of Role ARNs func (w *WebSSOAuthentication) promptForRole(idp string, roleARNs []string) (roleARN string, err error) { - oktaConfig, err := config.OktaConfig() + oktaYamlConfig, err := config.NewOktaYamlConfig() var configRoles map[string]string if err == nil { - configRoles = oktaConfig.AWSCLI.ROLES + configRoles = oktaYamlConfig.AWSCLI.ROLES } if len(roleARNs) == 1 || w.config.AWSIAMRole() != "" { @@ -527,8 +527,8 @@ func (w *WebSSOAuthentication) promptForRole(idp string, roleARNs []string) (rol // to pretty print out the IdP name again. func (w *WebSSOAuthentication) promptForIDP(idpARNs []string) (idpARN string, err error) { var configIDPs map[string]string - if oktaConfig, cErr := config.OktaConfig(); cErr == nil { - configIDPs = oktaConfig.AWSCLI.IDPS + if oktaYamlConfig, cErr := config.NewOktaYamlConfig(); cErr == nil { + configIDPs = oktaYamlConfig.AWSCLI.IDPS } if len(idpARNs) == 0 { From 82a266fdc4986725b88bbc5746ce2980d3511e41 Mon Sep 17 00:00:00 2001 From: Mike Mondragon Date: Wed, 8 Jan 2025 09:43:30 -0800 Subject: [PATCH 4/7] Refactor struct naming for follow on refactor. --- internal/config/config.go | 14 ++++++++------ internal/m2mauth/m2mauth_test.go | 2 +- internal/webssoauth/webssoauth_test.go | 2 +- 3 files changed, 10 insertions(+), 8 deletions(-) diff --git a/internal/config/config.go b/internal/config/config.go index fca7f7d..598e662 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -283,8 +283,8 @@ type Config struct { clock Clock } -// Attributes config construction -type Attributes struct { +// ConfigAttributes attributes for config construction +type ConfigAttributes struct { AllProfiles bool AuthzID string AWSCredentials string @@ -319,7 +319,7 @@ type Attributes struct { // 2. ENV variables // 3. .env file func EvaluateSettings() (*Config, error) { - cfgAttrs, err := readConfig() + cfgAttrs, err := loadConfigAttributesFromFlagsAndVars() if err != nil { return nil, err } @@ -327,7 +327,7 @@ func EvaluateSettings() (*Config, error) { } // NewConfig create config from attributes -func NewConfig(attrs *Attributes) (*Config, error) { +func NewConfig(attrs *ConfigAttributes) (*Config, error) { var err error cfg := &Config{ allProfiles: attrs.AllProfiles, @@ -413,7 +413,9 @@ func (c *Config) ReadConfigProfileKeys() ([]string, error) { return nil, nil } -func readConfig() (Attributes, error) { +// loadConfigAttributesFromFlagsAndVars helper function to load configuration +// attributes with viper by inspecting CLI flags then environment variables. +func loadConfigAttributesFromFlagsAndVars() (ConfigAttributes, error) { // Side loading multiple profiles from okta.yaml file if it exists if oktaYamlConfig, err := NewOktaYamlConfig(); err == nil { profiles := oktaYamlConfig.AWSCLI.PROFILES @@ -444,7 +446,7 @@ func readConfig() (Attributes, error) { awsProfile = "default" } - attrs := Attributes{ + attrs := ConfigAttributes{ AllProfiles: viper.GetBool(getFlagNameFromProfile(awsProfile, AllProfilesFlag)), AuthzID: viper.GetString(getFlagNameFromProfile(awsProfile, AuthzIDFlag)), AWSCredentials: viper.GetString(getFlagNameFromProfile(awsProfile, AWSCredentialsFlag)), diff --git a/internal/m2mauth/m2mauth_test.go b/internal/m2mauth/m2mauth_test.go index 2096b6d..b62e86f 100644 --- a/internal/m2mauth/m2mauth_test.go +++ b/internal/m2mauth/m2mauth_test.go @@ -92,7 +92,7 @@ func TestM2MAuthAccessToken(t *testing.T) { } func setupTest(t *testing.T) (*config.Config, func(t *testing.T)) { - attrs := &config.Attributes{ + attrs := &config.ConfigAttributes{ OrgDomain: os.Getenv("OKTA_AWSCLI_ORG_DOMAIN"), OIDCAppID: os.Getenv("OKTA_AWSCLI_OIDC_CLIENT_ID"), AWSIAMRole: os.Getenv("OKTA_AWSCLI_IAM_ROLE"), diff --git a/internal/webssoauth/webssoauth_test.go b/internal/webssoauth/webssoauth_test.go index 49c89a9..a069c47 100644 --- a/internal/webssoauth/webssoauth_test.go +++ b/internal/webssoauth/webssoauth_test.go @@ -78,7 +78,7 @@ func TestWebSSOAuthAccessToken(t *testing.T) { } func setupTest(t *testing.T) (*config.Config, func(t *testing.T)) { - attrs := &config.Attributes{ + attrs := &config.ConfigAttributes{ OrgDomain: os.Getenv("OKTA_AWSCLI_ORG_DOMAIN"), OIDCAppID: os.Getenv("OKTA_AWSCLI_OIDC_CLIENT_ID"), } From 9fac884131845cd2d22aeb510553b460bf8dd0c3 Mon Sep 17 00:00:00 2001 From: Mike Mondragon Date: Wed, 8 Jan 2025 09:46:56 -0800 Subject: [PATCH 5/7] Refactor name to better describe intent of the NewEvaluatedConfig config construtor. --- cmd/root/debug/debug.go | 2 +- cmd/root/m2m/m2m.go | 2 +- cmd/root/profileslist/profiles-list.go | 2 +- cmd/root/web/web.go | 2 +- internal/config/config.go | 5 +++-- 5 files changed, 7 insertions(+), 6 deletions(-) diff --git a/cmd/root/debug/debug.go b/cmd/root/debug/debug.go index c21ca82..df603a1 100644 --- a/cmd/root/debug/debug.go +++ b/cmd/root/debug/debug.go @@ -31,7 +31,7 @@ func NewDebugCommand() *cobra.Command { Use: "debug", Short: "Simple debug of okta.yaml and exit", RunE: func(cmd *cobra.Command, args []string) error { - config, err := config.EvaluateSettings() + config, err := config.NewEvaluatedConfig() if err != nil { return err } diff --git a/cmd/root/m2m/m2m.go b/cmd/root/m2m/m2m.go index 6a7df7f..cb99066 100644 --- a/cmd/root/m2m/m2m.go +++ b/cmd/root/m2m/m2m.go @@ -71,7 +71,7 @@ func NewM2MCommand() *cobra.Command { Use: "m2m", Short: "Machine to machine / headless authorization", RunE: func(cmd *cobra.Command, args []string) error { - config, err := config.EvaluateSettings() + config, err := config.NewEvaluatedConfig() if err != nil { return err } diff --git a/cmd/root/profileslist/profiles-list.go b/cmd/root/profileslist/profiles-list.go index 8194f22..d15fcbc 100644 --- a/cmd/root/profileslist/profiles-list.go +++ b/cmd/root/profileslist/profiles-list.go @@ -30,7 +30,7 @@ func NewProfilesListCommand() *cobra.Command { Use: "list-profiles", Short: "Lists profile names in ~/.okta/okta.yaml", RunE: func(cmd *cobra.Command, args []string) error { - config, err := config.EvaluateSettings() + config, err := config.NewEvaluatedConfig() if err != nil { return err } diff --git a/cmd/root/web/web.go b/cmd/root/web/web.go index f84de69..2ab60ca 100644 --- a/cmd/root/web/web.go +++ b/cmd/root/web/web.go @@ -81,7 +81,7 @@ func NewWebCommand() *cobra.Command { Use: "web", Short: "Human oriented authentication and device authorization", RunE: func(cmd *cobra.Command, args []string) error { - cfg, err := config.EvaluateSettings() + cfg, err := config.NewEvaluatedConfig() if err != nil { return err } diff --git a/internal/config/config.go b/internal/config/config.go index 598e662..ebb9e23 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -314,11 +314,12 @@ type ConfigAttributes struct { WriteAWSCredentials bool } -// EvaluateSettings Returns a new config gathering values in this order of precedence: +// NewEvaluatedConfig Returns a new config loading and evaluating attributes in +// this order of precedence: // 1. CLI flags // 2. ENV variables // 3. .env file -func EvaluateSettings() (*Config, error) { +func NewEvaluatedConfig() (*Config, error) { cfgAttrs, err := loadConfigAttributesFromFlagsAndVars() if err != nil { return nil, err From 9f8a7ecf1d12420c5d212f0df40a1f861fbeff21 Mon Sep 17 00:00:00 2001 From: Mike Mondragon Date: Thu, 9 Jan 2025 14:47:56 -0800 Subject: [PATCH 6/7] Logger interface to control stdio printing depending on which format okta-aws-cli is outputting to. Closes #247 --- cmd/root/debug/debug.go | 5 +- cmd/root/profileslist/profiles-list.go | 6 +- internal/config/config.go | 80 +++++++++++++++----------- internal/exec/exec.go | 21 ++++--- internal/logger/full.go | 34 +++++++++++ internal/logger/logger.go | 23 ++++++++ internal/logger/terse.go | 34 +++++++++++ internal/m2mauth/m2mauth.go | 4 +- internal/output/envvar.go | 20 +++---- internal/output/process_credentials.go | 3 +- internal/webssoauth/webssoauth.go | 18 +++--- 11 files changed, 174 insertions(+), 74 deletions(-) create mode 100644 internal/logger/full.go create mode 100644 internal/logger/logger.go create mode 100644 internal/logger/terse.go diff --git a/cmd/root/debug/debug.go b/cmd/root/debug/debug.go index df603a1..0cdde6d 100644 --- a/cmd/root/debug/debug.go +++ b/cmd/root/debug/debug.go @@ -17,9 +17,6 @@ package debug import ( - "fmt" - "os" - "github.com/spf13/cobra" "github.com/okta/okta-aws-cli/internal/config" @@ -37,7 +34,7 @@ func NewDebugCommand() *cobra.Command { } err = config.RunConfigChecks() // NOTE: still print out the done message, even if there was an error it will get printed as well - fmt.Fprintf(os.Stderr, "debugging okta-aws-cli config $HOME/.okta/okta.yaml is complete\n") + config.Logger.Warn("debugging okta-aws-cli config $HOME/.okta/okta.yaml is complete\n") if err != nil { return err } diff --git a/cmd/root/profileslist/profiles-list.go b/cmd/root/profileslist/profiles-list.go index d15fcbc..8ac8b54 100644 --- a/cmd/root/profileslist/profiles-list.go +++ b/cmd/root/profileslist/profiles-list.go @@ -17,8 +17,6 @@ package profileslist import ( - "fmt" - "github.com/spf13/cobra" "github.com/okta/okta-aws-cli/internal/config" @@ -35,7 +33,7 @@ func NewProfilesListCommand() *cobra.Command { return err } - fmt.Println("Profiles:") + config.Logger.Info("Profiles:\n") keys, err := config.ReadConfigProfileKeys() if err != nil { @@ -43,7 +41,7 @@ func NewProfilesListCommand() *cobra.Command { } for _, key := range keys { - fmt.Printf(" %s\n", key) + config.Logger.Info(" %s\n", key) } return nil diff --git a/internal/config/config.go b/internal/config/config.go index ebb9e23..d00ea70 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -29,6 +29,8 @@ import ( "github.com/spf13/viper" "gopkg.in/yaml.v2" + + "github.com/okta/okta-aws-cli/internal/logger" ) // longUserAgent the long user agent value @@ -281,6 +283,7 @@ type Config struct { shortUserAgent bool writeAWSCredentials bool clock Clock + Logger logger.Logger } // ConfigAttributes attributes for config construction @@ -324,7 +327,18 @@ func NewEvaluatedConfig() (*Config, error) { if err != nil { return nil, err } - return NewConfig(&cfgAttrs) + var config *Config + if config, err = NewConfig(&cfgAttrs); err != nil { + return nil, err + } + switch cfgAttrs.Format { + case ProcessCredentialsFormat: + config.Logger = &logger.TerseLogger{} + default: + config.Logger = &logger.FullLogger{} + } + + return config, nil } // NewConfig create config from attributes @@ -994,137 +1008,137 @@ awscli: "arn:aws:iam::012345678901:role/admin": "Dev Admin" "arn:aws:iam::012345678901:role/operator": "Dev Ops" ` - fmt.Fprintf(os.Stderr, "Given this YAML as an example template of okta.yaml for reference:\n%s\n", exampleYaml) + c.Logger.Warn("Given this YAML as an example template of okta.yaml for reference:\n%s\n", exampleYaml) homeDir, err := os.UserHomeDir() if err != nil { - fmt.Fprintf(os.Stderr, "WARNING: can't find user home directory $HOME\n") - fmt.Fprintf(os.Stderr, " see https://pkg.go.dev/os#UserHomeDir\n") + c.Logger.Warn("WARNING: can't find user home directory $HOME\n") + c.Logger.Warn(" see https://pkg.go.dev/os#UserHomeDir\n") return } - fmt.Fprintf(os.Stderr, "found home directory %q\n", homeDir) + c.Logger.Warn("found home directory %q\n", homeDir) configPath := filepath.Join(homeDir, DotOkta, OktaYaml) yamlConfig, err := os.ReadFile(configPath) if err != nil { - fmt.Fprintf(os.Stderr, "WARNING: can't read okta config %q\n", configPath) + c.Logger.Warn("WARNING: can't read okta config %q\n", configPath) return } - fmt.Fprintf(os.Stderr, "okta.yaml is readable %q\n", configPath) + c.Logger.Warn("okta.yaml is readable %q\n", configPath) conf := map[string]any{} err = yaml.Unmarshal(yamlConfig, &conf) if err != nil { - fmt.Fprintf(os.Stderr, "WARNING: okta.yaml is invalid yaml format\n") + c.Logger.Warn("WARNING: okta.yaml is invalid yaml format\n") return } - fmt.Fprintf(os.Stderr, "okta.yaml is valid yaml\n") + c.Logger.Warn("okta.yaml is valid yaml\n") awscli, ok := conf["awscli"] if !ok { - fmt.Fprintf(os.Stderr, "WARNING: okta.yaml missing \"awscli\" section\n") + c.Logger.Warn("WARNING: okta.yaml missing \"awscli\" section\n") return } - fmt.Fprintf(os.Stderr, "okta.yaml has root \"awscli\" section\n") + c.Logger.Warn("okta.yaml has root \"awscli\" section\n") if awscli == nil { - fmt.Fprintf(os.Stderr, "WARNING: okta.yaml \"awscli\" section has no values\n") + c.Logger.Warn("WARNING: okta.yaml \"awscli\" section has no values\n") return } _awscli, ok := awscli.(map[any]any) if !ok { - fmt.Fprintf(os.Stderr, "WARNING: okta.yaml \"awscli\" is not a map of values\n") + c.Logger.Warn("WARNING: okta.yaml \"awscli\" is not a map of values\n") } idps, ok := _awscli["idps"] if !ok { - fmt.Fprintf(os.Stderr, "WARNING: okta.yaml missing \"awscli.idps\" section\n") + c.Logger.Warn("WARNING: okta.yaml missing \"awscli.idps\" section\n") return } if idps == nil { - fmt.Fprintf(os.Stderr, "WARNING: okta.yaml \"awscli.idps\" section has no values\n") + c.Logger.Warn("WARNING: okta.yaml \"awscli.idps\" section has no values\n") return } // map[interface {}]interface {} _idps, ok := idps.(map[any]any) if !ok { - fmt.Fprintf(os.Stderr, "WARNING: okta.yaml \"awscli.idps\" section is not a map of ARN string key to friendly string label values\n") + c.Logger.Warn("WARNING: okta.yaml \"awscli.idps\" section is not a map of ARN string key to friendly string label values\n") return } if len(_idps) == 0 { - fmt.Fprintf(os.Stderr, "WARNING: okta.yaml \"awscli.idps\" section is an empty map of ARN string key to friendly string label values\n") + c.Logger.Warn("WARNING: okta.yaml \"awscli.idps\" section is an empty map of ARN string key to friendly string label values\n") return } for k, v := range _idps { if _, ok := k.(string); !ok { - fmt.Fprintf(os.Stderr, "okta.yaml \"awscli.idps\" value of ARN key \"%v\" is not a string\n", k) + c.Logger.Warn("okta.yaml \"awscli.idps\" value of ARN key \"%v\" is not a string\n", k) return } if _, ok := v.(string); !ok { - fmt.Fprintf(os.Stderr, "okta.yaml \"awscli.idps\" ARN key %q's friendly label value \"%v\" is not a string\n", k, v) + c.Logger.Warn("okta.yaml \"awscli.idps\" ARN key %q's friendly label value \"%v\" is not a string\n", k, v) return } } - fmt.Fprintf(os.Stderr, "okta.yaml \"awscli.idps\" section is a map of %d ARN string keys to friendly string label values\n", len(_idps)) + c.Logger.Warn("okta.yaml \"awscli.idps\" section is a map of %d ARN string keys to friendly string label values\n", len(_idps)) roles, ok := _awscli["roles"] if !ok { - fmt.Fprintf(os.Stderr, "WARNING: okta.yaml missing \"awscli.roles\" section\n") + c.Logger.Warn("WARNING: okta.yaml missing \"awscli.roles\" section\n") return } if roles == nil { - fmt.Fprintf(os.Stderr, "WARNING: okta.yaml \"awscli.roles\" section has no values\n") + c.Logger.Warn("WARNING: okta.yaml \"awscli.roles\" section has no values\n") return } _roles, ok := roles.(map[any]any) if !ok { - fmt.Fprintf(os.Stderr, "WARNING: okta.yaml \"awscli.roles\" section is not a map of ARN string key to friendly string label values\n") + c.Logger.Warn("WARNING: okta.yaml \"awscli.roles\" section is not a map of ARN string key to friendly string label values\n") return } if len(_roles) == 0 { - fmt.Fprintf(os.Stderr, "WARNING: okta.yaml \"awscli.roles\" section is an empty map of ARN string key to friendly string label values\n") + c.Logger.Warn("WARNING: okta.yaml \"awscli.roles\" section is an empty map of ARN string key to friendly string label values\n") return } for k, v := range _roles { if _, ok := k.(string); !ok { - fmt.Fprintf(os.Stderr, "okta.yaml \"awscli.roles\" value of ARN key \"%v\" is not a string\n", k) + c.Logger.Warn("okta.yaml \"awscli.roles\" value of ARN key \"%v\" is not a string\n", k) return } if _, ok := v.(string); !ok { - fmt.Fprintf(os.Stderr, "okta.yaml \"awscli.roles\" ARN key %q's friendly label value \"%v\" is not a string\n", k, v) + c.Logger.Warn("okta.yaml \"awscli.roles\" ARN key %q's friendly label value \"%v\" is not a string\n", k, v) return } } - fmt.Fprintf(os.Stderr, "okta.yaml \"awscli.roles\" section is a map of %d ARN string keys to friendly string label values\n", len(_roles)) + c.Logger.Warn("okta.yaml \"awscli.roles\" section is a map of %d ARN string keys to friendly string label values\n", len(_roles)) profiles, ok := _awscli["profiles"] if !ok { - fmt.Fprintf(os.Stderr, "WARNING: okta.yaml missing \"awscli.profiles\" section\n") + c.Logger.Warn("WARNING: okta.yaml missing \"awscli.profiles\" section\n") return } if profiles == nil { - fmt.Fprintf(os.Stderr, "WARNING: okta.yaml \"awscli.profiles\" section has no values\n") + c.Logger.Warn("WARNING: okta.yaml \"awscli.profiles\" section has no values\n") return } _profiles, ok := profiles.(map[any]any) if !ok { - fmt.Fprintf(os.Stderr, "WARNING: okta.yaml \"awscli.profiles\" section is not a map of separate config settings keyed by profile name\n") + c.Logger.Warn("WARNING: okta.yaml \"awscli.profiles\" section is not a map of separate config settings keyed by profile name\n") return } if len(_profiles) == 0 { - fmt.Fprintf(os.Stderr, "WARNING: okta.yaml \"awscli.profiles\" section is an empty map of separate config settings keyed by profile name\n") + c.Logger.Warn("WARNING: okta.yaml \"awscli.profiles\" section is an empty map of separate config settings keyed by profile name\n") return } - fmt.Fprintf(os.Stderr, "okta.yaml \"awscli.profiles\" section is a map of %d separate config settings keyed by profile name\n", len(_profiles)) + c.Logger.Warn("okta.yaml \"awscli.profiles\" section is a map of %d separate config settings keyed by profile name\n", len(_profiles)) - fmt.Fprintf(os.Stderr, "okta.yaml is OK\n") + c.Logger.Warn("okta.yaml is OK\n") return nil } diff --git a/internal/exec/exec.go b/internal/exec/exec.go index 8872678..8d57fa0 100644 --- a/internal/exec/exec.go +++ b/internal/exec/exec.go @@ -23,17 +23,19 @@ import ( "strings" oaws "github.com/okta/okta-aws-cli/internal/aws" + "github.com/okta/okta-aws-cli/internal/config" "github.com/okta/okta-aws-cli/internal/utils" ) // Exec is a executor / a process runner type Exec struct { - name string - args []string + name string + args []string + config *config.Config } // NewExec Create a new executor -func NewExec() (*Exec, error) { +func NewExec(c *config.Config) (*Exec, error) { args := []string{} foundArgs := false for _, arg := range os.Args { @@ -55,8 +57,9 @@ func NewExec() (*Exec, error) { name := args[0] args = args[1:] ex := &Exec{ - name: name, - args: args, + name: name, + args: args, + config: c, } return ex, nil @@ -85,14 +88,14 @@ func (e *Exec) Run(cc *oaws.CredentialContainer) error { out, err := cmd.Output() if ee, ok := err.(*osexec.ExitError); ok { - fmt.Fprintf(os.Stderr, "error running process\n") - fmt.Fprintf(os.Stderr, "%s %s\n", e.name, strings.Join(e.args, " ")) - fmt.Fprintf(os.Stderr, utils.PassThroughStringNewLineFMT, ee.Stderr) + e.config.Logger.Warn("error running process\n") + e.config.Logger.Warn("%s %s\n", e.name, strings.Join(e.args, " ")) + e.config.Logger.Warn(utils.PassThroughStringNewLineFMT, ee.Stderr) } if err != nil { return err } - fmt.Printf("%s", string(out)) + e.config.Logger.Info("%s", string(out)) return nil } diff --git a/internal/logger/full.go b/internal/logger/full.go new file mode 100644 index 0000000..613b24e --- /dev/null +++ b/internal/logger/full.go @@ -0,0 +1,34 @@ +/* + * Copyright (c) 2025-Present, Okta, Inc. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package logger + +import ( + "fmt" + "os" +) + +// FullLogger logger for stderr (warn) and stdout (info) logging +type FullLogger struct { +} + +func (l *FullLogger) Info(format string, a ...any) (int, error) { + return fmt.Fprintf(os.Stdout, format, a...) +} + +func (l *FullLogger) Warn(format string, a ...any) (int, error) { + return fmt.Fprintf(os.Stderr, format, a...) +} diff --git a/internal/logger/logger.go b/internal/logger/logger.go new file mode 100644 index 0000000..c3ca8ec --- /dev/null +++ b/internal/logger/logger.go @@ -0,0 +1,23 @@ +/* + * Copyright (c) 2025-Present, Okta, Inc. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package logger + +// Logger Interface used for logging ouput +type Logger interface { + Info(format string, a ...any) (int, error) + Warn(format string, a ...any) (int, error) +} diff --git a/internal/logger/terse.go b/internal/logger/terse.go new file mode 100644 index 0000000..a701ae8 --- /dev/null +++ b/internal/logger/terse.go @@ -0,0 +1,34 @@ +/* + * Copyright (c) 2025-Present, Okta, Inc. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package logger + +import ( + "fmt" + "os" +) + +// TerseLogger logger that only prints info level lines +type TerseLogger struct { +} + +func (l *TerseLogger) Info(format string, a ...any) (int, error) { + return fmt.Fprintf(os.Stdout, format, a...) +} + +func (l *TerseLogger) Warn(format string, a ...any) (int, error) { + return -1, nil +} diff --git a/internal/m2mauth/m2mauth.go b/internal/m2mauth/m2mauth.go index b976fa8..5083333 100644 --- a/internal/m2mauth/m2mauth.go +++ b/internal/m2mauth/m2mauth.go @@ -68,7 +68,7 @@ func NewM2MAuthentication(cfg *config.Config) (*M2MAuthentication, error) { // Check if exec arg is present and that there are args for it before doing any work if cfg.Exec() { - if _, err := exec.NewExec(); err != nil { + if _, err := exec.NewExec(cfg); err != nil { return nil, err } } @@ -103,7 +103,7 @@ func (m *M2MAuthentication) EstablishIAMCredentials() error { } if m.config.Exec() { - exe, _ := exec.NewExec() + exe, _ := exec.NewExec(m.config) if err := exe.Run(cc); err != nil { return err } diff --git a/internal/output/envvar.go b/internal/output/envvar.go index efc1138..ee67b9b 100644 --- a/internal/output/envvar.go +++ b/internal/output/envvar.go @@ -17,8 +17,6 @@ package output import ( - "fmt" - "os" "runtime" oaws "github.com/okta/okta-aws-cli/internal/aws" @@ -45,20 +43,20 @@ func (e *EnvVar) Output(c *config.Config, cc *oaws.CredentialContainer) error { SecretAccessKey: cc.SecretAccessKey, SessionToken: cc.SessionToken, } - fmt.Fprintf(os.Stderr, "\n") + c.Logger.Warn("\n") if runtime.GOOS == "windows" { - fmt.Printf("setx AWS_ACCESS_KEY_ID %s\n", evc.AccessKeyID) - fmt.Printf("setx AWS_SECRET_ACCESS_KEY %s\n", evc.SecretAccessKey) - fmt.Printf("setx AWS_SESSION_TOKEN %s\n", evc.SessionToken) + c.Logger.Info("setx AWS_ACCESS_KEY_ID %s\n", evc.AccessKeyID) + c.Logger.Info("setx AWS_SECRET_ACCESS_KEY %s\n", evc.SecretAccessKey) + c.Logger.Info("setx AWS_SESSION_TOKEN %s\n", evc.SessionToken) if e.LegacyAWSVariables { - fmt.Printf("setx AWS_SECURITY_TOKEN %s\n", evc.SessionToken) + c.Logger.Info("setx AWS_SECURITY_TOKEN %s\n", evc.SessionToken) } } else { - fmt.Printf("export AWS_ACCESS_KEY_ID=%s\n", evc.AccessKeyID) - fmt.Printf("export AWS_SECRET_ACCESS_KEY=%s\n", evc.SecretAccessKey) - fmt.Printf("export AWS_SESSION_TOKEN=%s\n", evc.SessionToken) + c.Logger.Info("export AWS_ACCESS_KEY_ID=%s\n", evc.AccessKeyID) + c.Logger.Info("export AWS_SECRET_ACCESS_KEY=%s\n", evc.SecretAccessKey) + c.Logger.Info("export AWS_SESSION_TOKEN=%s\n", evc.SessionToken) if e.LegacyAWSVariables { - fmt.Printf("export AWS_SECURITY_TOKEN=%s\n", evc.SessionToken) + c.Logger.Info("export AWS_SECURITY_TOKEN=%s\n", evc.SessionToken) } } diff --git a/internal/output/process_credentials.go b/internal/output/process_credentials.go index 028cd9e..c1f3f88 100644 --- a/internal/output/process_credentials.go +++ b/internal/output/process_credentials.go @@ -18,7 +18,6 @@ package output import ( "encoding/json" - "fmt" oaws "github.com/okta/okta-aws-cli/internal/aws" "github.com/okta/okta-aws-cli/internal/config" @@ -51,6 +50,6 @@ func (p *ProcessCredentials) Output(c *config.Config, cc *oaws.CredentialContain return err } - fmt.Printf("%s", credJSON) + c.Logger.Info("%s", credJSON) return nil } diff --git a/internal/webssoauth/webssoauth.go b/internal/webssoauth/webssoauth.go index b37aadb..3369712 100644 --- a/internal/webssoauth/webssoauth.go +++ b/internal/webssoauth/webssoauth.go @@ -135,7 +135,7 @@ func NewWebSSOAuthentication(cfg *config.Config) (token *WebSSOAuthentication, e // Check if exec arg is present and that there are args for it before doing any work if cfg.Exec() { - if _, err := exec.NewExec(); err != nil { + if _, err := exec.NewExec(cfg); err != nil { return nil, err } } @@ -275,7 +275,7 @@ func (w *WebSSOAuthentication) selectFedApp(apps []*okta.Application) (string, e if err != nil { return "", err } - fmt.Fprintln(os.Stderr, rich) + w.config.Logger.Warn(rich) } return app.ID, nil @@ -334,7 +334,7 @@ func (w *WebSSOAuthentication) establishTokenWithFedAppID(clientID, fedAppID str } if w.config.Exec() { - exe, _ := exec.NewExec() + exe, _ := exec.NewExec(w.config) if err := exe.Run(cc); err != nil { return err } @@ -344,7 +344,7 @@ func (w *WebSSOAuthentication) establishTokenWithFedAppID(clientID, fedAppID str for cc := range ccch { err = output.RenderAWSCredential(w.config, cc) if err != nil { - fmt.Fprintf(os.Stderr, "failed to render credential %s: %s\n", cc.Profile, err) + w.config.Logger.Warn("failed to render credential %s: %s\n", cc.Profile, err) continue } } @@ -406,7 +406,7 @@ func (w *WebSSOAuthentication) awsAssumeRoleWithSAML(iar *idpAndRole, assertion, }) if p, err := w.fetchAWSAccountAlias(sessCopy); err != nil { org := "org" - fmt.Fprintf(os.Stderr, "unable to determine account alias, setting alias name to %q\n", org) + w.config.Logger.Warn("unable to determine account alias, setting alias name to %q\n", org) profileName = org } else { profileName = p @@ -491,7 +491,7 @@ func (w *WebSSOAuthentication) promptForRole(idp string, roleARNs []string) (rol if err != nil { return "", err } - fmt.Fprintln(os.Stderr, rich) + w.config.Logger.Warn(rich) } return roleARN, nil } @@ -552,7 +552,7 @@ func (w *WebSSOAuthentication) promptForIDP(idpARNs []string) (idpARN string, er if err != nil { return "", err } - fmt.Fprintln(os.Stderr, rich) + w.config.Logger.Warn(rich) return idpARN, nil } @@ -1121,7 +1121,7 @@ func ConsolePrint(config *config.Config, format string, a ...any) { return } - fmt.Fprintf(os.Stderr, format, a...) + config.Logger.Warn(format, a...) } func (w *WebSSOAuthentication) consolePrint(format string, a ...any) { @@ -1141,7 +1141,7 @@ func (w *WebSSOAuthentication) fetchAllAWSCredentialsWithSAMLRole(idpRolesMap ma defer wg.Done() cc, err := w.awsAssumeRoleWithSAML(iar, assertion, region) if err != nil { - fmt.Fprintf(os.Stderr, "failed to fetch AWS creds IdP %q, and Role %q, error:\n%+v\n", iar.idp, iar.role, err) + w.config.Logger.Warn("failed to fetch AWS creds IdP %q, and Role %q, error:\n%+v\n", iar.idp, iar.role, err) return } ccch <- cc From ee05f322fe7c77cd846fddc4cc531ff01d346b18 Mon Sep 17 00:00:00 2001 From: Mike Mondragon Date: Thu, 9 Jan 2025 15:07:48 -0800 Subject: [PATCH 7/7] qc --- cmd/root/web/web.go | 9 +++++++-- internal/config/config.go | 10 +++++----- internal/logger/full.go | 13 +++++++------ internal/logger/logger.go | 4 ++-- internal/logger/terse.go | 13 ++++++------- internal/m2mauth/m2mauth_test.go | 2 +- internal/webssoauth/webssoauth_test.go | 2 +- 7 files changed, 29 insertions(+), 24 deletions(-) diff --git a/cmd/root/web/web.go b/cmd/root/web/web.go index 2ab60ca..2f5121f 100644 --- a/cmd/root/web/web.go +++ b/cmd/root/web/web.go @@ -27,6 +27,11 @@ import ( "github.com/okta/okta-aws-cli/internal/webssoauth" ) +const ( + // InvalidGrant constant + InvalidGrant = "invalid_grant" +) + var ( flags = []cliFlag.Flag{ { @@ -111,7 +116,7 @@ func NewWebCommand() *cobra.Command { err = wsa.EstablishIAMCredentials() apiErr, ok = err.(*okta.APIError) if ok { - if apiErr.ErrorType == "invalid_grant" && webssoauth.RemoveCachedAccessToken() { + if apiErr.ErrorType == InvalidGrant && webssoauth.RemoveCachedAccessToken() { webssoauth.ConsolePrint(cfg, "Cached access token appears to be stale, removing token and retrying device authorization ...\n\n") continue } @@ -124,7 +129,7 @@ func NewWebCommand() *cobra.Command { } if err != nil { - if apiErr != nil && apiErr.ErrorType == "invalid_grant" { + if apiErr != nil && apiErr.ErrorType == InvalidGrant { webssoauth.ConsolePrint(cfg, "Authentication failed after multiple attempts. Please log out of Okta in your browser and log back in to resolve the issue.\n") } return err diff --git a/internal/config/config.go b/internal/config/config.go index d00ea70..8f19fff 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -286,8 +286,8 @@ type Config struct { Logger logger.Logger } -// ConfigAttributes attributes for config construction -type ConfigAttributes struct { +// Attributes attributes for config construction +type Attributes struct { AllProfiles bool AuthzID string AWSCredentials string @@ -342,7 +342,7 @@ func NewEvaluatedConfig() (*Config, error) { } // NewConfig create config from attributes -func NewConfig(attrs *ConfigAttributes) (*Config, error) { +func NewConfig(attrs *Attributes) (*Config, error) { var err error cfg := &Config{ allProfiles: attrs.AllProfiles, @@ -430,7 +430,7 @@ func (c *Config) ReadConfigProfileKeys() ([]string, error) { // loadConfigAttributesFromFlagsAndVars helper function to load configuration // attributes with viper by inspecting CLI flags then environment variables. -func loadConfigAttributesFromFlagsAndVars() (ConfigAttributes, error) { +func loadConfigAttributesFromFlagsAndVars() (Attributes, error) { // Side loading multiple profiles from okta.yaml file if it exists if oktaYamlConfig, err := NewOktaYamlConfig(); err == nil { profiles := oktaYamlConfig.AWSCLI.PROFILES @@ -461,7 +461,7 @@ func loadConfigAttributesFromFlagsAndVars() (ConfigAttributes, error) { awsProfile = "default" } - attrs := ConfigAttributes{ + attrs := Attributes{ AllProfiles: viper.GetBool(getFlagNameFromProfile(awsProfile, AllProfilesFlag)), AuthzID: viper.GetString(getFlagNameFromProfile(awsProfile, AuthzIDFlag)), AWSCredentials: viper.GetString(getFlagNameFromProfile(awsProfile, AWSCredentialsFlag)), diff --git a/internal/logger/full.go b/internal/logger/full.go index 613b24e..d5d2b05 100644 --- a/internal/logger/full.go +++ b/internal/logger/full.go @@ -22,13 +22,14 @@ import ( ) // FullLogger logger for stderr (warn) and stdout (info) logging -type FullLogger struct { -} +type FullLogger struct{} -func (l *FullLogger) Info(format string, a ...any) (int, error) { - return fmt.Fprintf(os.Stdout, format, a...) +// Info prints formatted message to stdout +func (l *FullLogger) Info(format string, a ...any) { + _, _ = fmt.Fprintf(os.Stdout, format, a...) } -func (l *FullLogger) Warn(format string, a ...any) (int, error) { - return fmt.Fprintf(os.Stderr, format, a...) +// Warn prints formatted message to stderr +func (l *FullLogger) Warn(format string, a ...any) { + _, _ = fmt.Fprintf(os.Stderr, format, a...) } diff --git a/internal/logger/logger.go b/internal/logger/logger.go index c3ca8ec..4cda76f 100644 --- a/internal/logger/logger.go +++ b/internal/logger/logger.go @@ -18,6 +18,6 @@ package logger // Logger Interface used for logging ouput type Logger interface { - Info(format string, a ...any) (int, error) - Warn(format string, a ...any) (int, error) + Info(format string, a ...any) + Warn(format string, a ...any) } diff --git a/internal/logger/terse.go b/internal/logger/terse.go index a701ae8..6e57d9a 100644 --- a/internal/logger/terse.go +++ b/internal/logger/terse.go @@ -22,13 +22,12 @@ import ( ) // TerseLogger logger that only prints info level lines -type TerseLogger struct { -} +type TerseLogger struct{} -func (l *TerseLogger) Info(format string, a ...any) (int, error) { - return fmt.Fprintf(os.Stdout, format, a...) +// Info prints formatted message to stdout +func (l *TerseLogger) Info(format string, a ...any) { + _, _ = fmt.Fprintf(os.Stdout, format, a...) } -func (l *TerseLogger) Warn(format string, a ...any) (int, error) { - return -1, nil -} +// Warn is a no-op, it prints nothing +func (l *TerseLogger) Warn(format string, a ...any) {} diff --git a/internal/m2mauth/m2mauth_test.go b/internal/m2mauth/m2mauth_test.go index b62e86f..2096b6d 100644 --- a/internal/m2mauth/m2mauth_test.go +++ b/internal/m2mauth/m2mauth_test.go @@ -92,7 +92,7 @@ func TestM2MAuthAccessToken(t *testing.T) { } func setupTest(t *testing.T) (*config.Config, func(t *testing.T)) { - attrs := &config.ConfigAttributes{ + attrs := &config.Attributes{ OrgDomain: os.Getenv("OKTA_AWSCLI_ORG_DOMAIN"), OIDCAppID: os.Getenv("OKTA_AWSCLI_OIDC_CLIENT_ID"), AWSIAMRole: os.Getenv("OKTA_AWSCLI_IAM_ROLE"), diff --git a/internal/webssoauth/webssoauth_test.go b/internal/webssoauth/webssoauth_test.go index a069c47..49c89a9 100644 --- a/internal/webssoauth/webssoauth_test.go +++ b/internal/webssoauth/webssoauth_test.go @@ -78,7 +78,7 @@ func TestWebSSOAuthAccessToken(t *testing.T) { } func setupTest(t *testing.T) (*config.Config, func(t *testing.T)) { - attrs := &config.ConfigAttributes{ + attrs := &config.Attributes{ OrgDomain: os.Getenv("OKTA_AWSCLI_ORG_DOMAIN"), OIDCAppID: os.Getenv("OKTA_AWSCLI_OIDC_CLIENT_ID"), }