Skip to content
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

🎨 Release 2.0 cleanup #280

Merged
merged 1 commit into from
Mar 21, 2019

Conversation

AlainODea
Copy link
Contributor

Problem Statement

This codebase hasn't seen a refactor and cleanup in a while. In the interest of maintainability one should be performed.

Solution

  • Breaking: clear credentials from AWS credentials file on logout

  • Breaking: make awscli use default (not generated) profile name

  • Breaking: remove config.properties from source

  • Breaking: remove log4j and its configuration

  • Clean up MFA handling code (rat nest -> mouse nest)

  • Allow fast reuse of existing, unexpired named sessions

  • Allow okta-aws {profilename} logout on macOS/Linux/*nix-likes

  • Address most SonarLint findings

  • Update third party notices (IANAL)

  • Update copyright to 2019

  • Apply Apache 2.0 header to most files

  • Update dependencies to latest stable versions

 - Breaking: clear credentials from AWS credentials file on logout

 - Breaking: make awscli use default (not generated) profile name

 - Breaking: remove config.properties from source

 - Breaking: remove log4j and its configuration

 - Clean up MFA handling code (rat nest -> mouse nest)

 - Allow fast reuse of existing, unexpired named sessions

 - Allow okta-aws {profilename} logout on macOS/Linux/*nix-likes

 - Address most SonarLint findings

 - Update third party notices (IANAL)

 - Update copyright to 2019

 - Apply Apache 2.0 header to most files

 - Update dependencies to latest stable versions
@AlainODea
Copy link
Contributor Author

@bogeylnj @aparkerlue @froshyfrosh @Joeskyyy mh-psaad mind giving this a once-over? Focus on pieces of it you are familiar with or have strong opinions about.

This is a non-trivial change, but I've been pretty thorough in my manual testing outside of certain MFA factors like Okta Push, Yubikeys, and hardware tokens which I don't have to test with.

@robkinyon-roivant @jeugene @duhaas2015 @au457415 @mrbelowski @tbenshalom you folks appear to be using this pretty heavily for production environments. Do you anticipate problems from the breaking changes I listed?

@tbenshalom
Copy link

@AlainODea We started using this tool in the last 4 weeks, so were not suffered from breaking-changes.
Also we had some problems with the PowerShell downloads in win 7, so at the end we put the jar in an internal repository, and and did the needed changes in the PS script. So currently, we have more control on the "version" our users are using.
Thanks for the tool and for the help.

@robkinyon-roivant
Copy link

@AlainODea we don't use any logout functionality, so the clearing won't matter - we just let credentials expire and, for the most part, re-run this tool every time a new action is happening. The log4j change won't matter to us because we don't look at the log messages in an automated way.

I'd like to know more about "make awscli use default (not generated) profile name" and "remove config.properties from source" - what exactly would those do?

@AlainODea
Copy link
Contributor Author

@robkinyon-roivant thank you for your feedback.

I'd like to know more about "make awscli use default (not generated) profile name" and "remove config.properties from source" - what exactly would those do?

awscli used to generate a profile name and save it to the credentials file if OKTA_PROFILE was not supplied. Now it will update the default profile in AWS CLI instead. This will overwrite the default profile credentials if someone was depending on those.

Removing config.properties from source should have no impact on people unless they are installing it by copy-pasting config.properties from the repo and updating it instead of using the installer.

@AlainODea
Copy link
Contributor Author

@tbenshalom thank you.

@AlainODea We started using this tool in the last 4 weeks, so were not suffered from breaking-changes.
Also we had some problems with the PowerShell downloads in win 7, so at the end we put the jar in an internal repository, and and did the needed changes in the PS script. So currently, we have more control on the "version" our users are using.
Thanks for the tool and for the help.

None of these changes have been released yet. This is a proposal to go to 2.0 with breaking changes. I wanted to gather feedback on the breaking changes first to avoid seriously frustrating people :)

@robkinyon-roivant
Copy link

awscli used to generate a profile name and save it to the credentials file if OKTA_PROFILE was not supplied. Now it will update the default profile in AWS CLI instead. This will overwrite the default profile credentials if someone was depending on those.

So, does this mean if we supply a OKTA_PROFILE, that awscli will set that to the default profile? Will the other profiles (e.g., I have 7 in one of our AWS accounts) still be accessible?

@jinglejengel
Copy link

Breaking: make awscli use default (not generated) profile name

Yikes, is this configurable? I like to avoid having a default AWS profile in my configs to avoid accidentally running awscli commands against unintended accounts.

@AlainODea
Copy link
Contributor Author

@Joeskyyy thank you for your feedback.

Breaking: make awscli use default (not generated) profile name

Yikes, is this configurable? I like to avoid having a default AWS profile in my configs to avoid accidentally running awscli commands against unintended accounts.

It is not configurable in the proposed changes. This can still be changed and leaving that option open is a big motivator for having this consultation with you folks before making these radical changes.

My recommendation is to use OKTA_PROFILE instead. My rationale for getting rid of awscli.java and the command is because it leads to ~/.aws/credentials clutter that it never cleans up.

Is using okta-aws {profilename} a workable alternative to awscli for you?
Would you prefer that I keep the existing autogeneration behavior in awscli?

@jinglejengel
Copy link

@AlainODea I suppose I understand the rationale. I think it's probably going to cause a lot of pain down the line, especially since most of the power users of this are going to have multiple profiles they'll be working with. I think, personally, using specific profile names is going to be much better in terms of support. Or at least being able to override not having the default profile be the be-all-end-all.

That being said, as long as one could do okta-aws my-awesome-profile I think the bandaid could be ripped off...

@AlainODea
Copy link
Contributor Author

@robkinyon-roivant good question!

awscli used to generate a profile name and save it to the credentials file if OKTA_PROFILE was not supplied. Now it will update the default profile in AWS CLI instead. This will overwrite the default profile credentials if someone was depending on those.

So, does this mean if we supply a OKTA_PROFILE, that awscli will set that to the default profile? Will the other profiles (e.g., I have 7 in one of our AWS accounts) still be accessible?

The proposed awscli command will override the OKTA_PROFILE environment variable.

You will still be able to use okta-aws {profilename} ... to choose a specific profile by name.

This bit is unchanged. The documentation is a bit misleading as it documents what WithOkta.java does with the configuration and environment, not what the bundled commands do. All of the existing bundled commands override OKTA_PROFILE, usually replacing it with a positional parameter.

@AlainODea
Copy link
Contributor Author

@bogeylnj @aparkerlue @froshyfrosh @jeugene @duhaas2015 @au457415 @mrbelowski I will be keeping this open until at least March 18th.

After March 18th, I will check if there are any deal-breakers. If there are, I will work to address them. If not, I'll merge and publish v2.0.0 with these changes.

@froshyfrosh
Copy link

@AlainODea Sorry I don't have the time for a deeper review, maybe next week I can have a closer look. So let me put it like this: I am using the the library only programmatically like this:

OktaAwsCliEnvironment environment = new OktaAwsCliEnvironment(browserAuth, oktaOrg, oktaUsername, oktaPassword ('Supplier<String>'), oktaCookiesPath, oktaProfile, oktaAwsAppUrl, awsRoleToAssume, stsDuration, awsRegion, oktaEnvMode)
AWSSessionCredentials awsCredentials = AWSCredentialsUtil.getAWSCredential(environment);

The AWSSessionCredentials are then used for several functionalities of the AWS Java SDK (accessing s3, creating EMR clusters, creating and calling Lambdas, ...).
As long as this still works, I'm happy :-)

On Monday I will ask some of my colleagues to also have a look at your proposal (maybe it's possible to wait a few more days - sorry I'm a bit late to the party).

@AlainODea
Copy link
Contributor Author

@froshyfrosh I can wait a few extra days if your team needs it. I can’t imagine your use case will be impacted by the breaking changes.

@froshyfrosh
Copy link

@AlainODea I asked my colleagues and there are no complains from our side. Thank you very much and good luck for the big update.

@AlainODea
Copy link
Contributor Author

@froshyfrosh thank you for letting me know.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants