Skip to content
This repository has been archived by the owner on May 18, 2021. It is now read-only.

Add export command to copy tokens to credentials file #194

Merged
merged 4 commits into from
Sep 3, 2019

Conversation

mike-zorn
Copy link
Contributor

This resolves #189. Apologies for not getting consent via an issue before submitting the PR: the workarounds in that issue didn't work for me and this seemed to be the most straightforward path to get this to work for my scenario.

This adds a new command, export which will write credentials to the credentials file for the profile which was passed into the command.

@nickatsegment nickatsegment self-requested a review August 28, 2019 18:22
@nickatsegment
Copy link
Contributor

I feel pretty strongly that this could be done with a one-off Python script or something outside of aws-okta.

BUT, because you went to the effort, and got all the thumbs, seems like there's a user desire that I don't see. So, I'll merge under the condition that @ApeChimp is the maintainer of this subcommand. Please add an entry for this file in CODEOWNERS.

We need a better name that makes it clear its going to modify a file. I know I was the one who suggested export, but now it's actually updating a file. Maybe update-credentials-file? Kind of long-winded... open to ideas.

Can you point ansible at a different path for credentials? Keeping temporary creds in a user-level config file feels a little weird to me. Not a huge deal.

@nickatsegment
Copy link
Contributor

One more alternative: boto3 (which I believe ansible uses) supports assuming roles itself, directly. Have you tried that?

@toddlipcon
Copy link

I'd find this useful as well. My use case is running some integration tests from a Java IDE, where the integration tests require AWS privileges. I can invoke the IDE from within a shell with the environment variables present, but when they expire I have to restart the IDE to pick up the new token, which makes for quite an interruption in my dev workflow.

@nickatsegment
Copy link
Contributor

I'd find this useful as well. My use case is running some integration tests from a Java IDE, where the integration tests require AWS privileges. I can invoke the IDE from within a shell with the environment variables present, but when they expire I have to restart the IDE to pick up the new token, which makes for quite an interruption in my dev workflow.

Can you not instruct the IDE to wrap the test command in aws-okta exec, rather than wrapping the whole IDE?

@toddlipcon
Copy link

Can you not instruct the IDE to wrap the test command in aws-okta exec, rather than wrapping the whole IDE?

Might be possible? I'm just using normal IDEA "run unit test" functionality here, and while I see ways to configure which JVM to use, I didn't want to go making custom JAVA_HOMEs with wrapper scripts in place of 'java'.

@mike-zorn
Copy link
Contributor Author

So, I'll merge under the condition that @ApeChimp is the maintainer of this subcommand. Please add an entry for this file in CODEOWNERS.

According to the code owners docs, code owners must have write access to the repository. I'm happy to maintain this command, but I want to verify that you actually want me to have write access to the repository.

We need a better name that makes it clear its going to modify a file. I know I was the one who suggested export, but now it's actually updating a file. Maybe update-credentials-file? Kind of long-winded... open to ideas.

Seems fine. I'll make this change. In hindsight, export is kinda a bad name considering that env exports the credentials...

Can you point ansible at a different path for credentials? Keeping temporary creds in a user-level config file feels a little weird to me. Not a huge deal.

I think so. The issue for me is that ansible isn't the only tool we use that uses our credentials file. We've for better or worse designed many of our aws workflows around switching profiles and expecting credentials to exist in the credentials file.

One more alternative: boto3 (which I believe ansible uses) supports assuming roles itself, directly. Have you tried that?

So the issue here is that we use okta to sso into each account. We don't want to allow roles from one account to assume roles in another because we want to manage access to each account in one place: okta.

@nickatsegment
Copy link
Contributor

According to the code owners docs, code owners must have write access to the repository. I'm happy to maintain this command, but I want to verify that you actually want me to have write access to the repository.

Well dang, yeah, that's too much. Maybe just put a comment at the top of the file then.

Can you point ansible at a different path for credentials? Keeping temporary creds in a user-level config file feels a little weird to me. Not a huge deal.

I think so. The issue for me is that ansible isn't the only tool we use that uses our credentials file. We've for better or worse designed many of our aws workflows around switching profiles and expecting credentials to exist in the credentials file.

Gotcha. What do you think about making it a required argument? Say aws-okta write-to-credentials <profile> <credentials-file> e.g aws-okta write-to-credentials profile ~/.aws/credentials. That makes it pretty explicit that we're going to be modifying that file, and allows the security conscious to write it to an ephemeral file. Writing to a specific file might be used to avoid racing on the main file as well.

One more alternative: boto3 (which I believe ansible uses) supports assuming roles itself, directly. Have you tried that?

So the issue here is that we use okta to sso into each account. We don't want to allow roles from one account to assume roles in another because we want to manage access to each account in one place: okta.

Understood, thanks for clarifying.

- Rename export command to write-to-credentials
- add required file argument
- comment at top of file to indicate maintainer
Copy link
Contributor

@nickatsegment nickatsegment left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome, thanks for working in my requests!

@nickatsegment nickatsegment merged commit 0aa9e84 into segmentio:master Sep 3, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Suggestion: Write env output to aws credentials file
3 participants