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

Don't write a new aws config file when importing #259

Merged
merged 2 commits into from Jun 21, 2023

Conversation

AndyTitu
Copy link
Contributor

@AndyTitu AndyTitu commented May 8, 2023

Overview

It seems the aws-vault importer is creating the .aws/config file if it doesn't exist. We should make sure that importers only read.

Type of change

  • Created a new plugin
  • Improved an existing plugin
  • Fixed a bug in an existing plugin
  • Improved contributor utilities or experience

How To Test

op plugin init aws and import credentials

Changelog

AWS Shell Plugin importer never creates a .aws/config file if one does not exist already.

@AndyTitu AndyTitu requested a review from florisvdg May 9, 2023 11:15
@AndyTitu AndyTitu added the waiting-on-reviewer signals that a certain PR is waiting for a review from a 1Password team member label May 9, 2023
@AndyTitu AndyTitu requested a review from accraw June 19, 2023 11:28
Copy link
Contributor

@accraw accraw left a comment

Choose a reason for hiding this comment

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

Did a functional test to ensure the file is not created and I can still import profiles, both passed.

@AndyTitu AndyTitu requested a review from jpcoenen June 21, 2023 10:00
Copy link
Contributor

@arunsathiya arunsathiya left a comment

Choose a reason for hiding this comment

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

The overall changes here look good to me -- both code-wise, and functional testing.

@arunsathiya
Copy link
Contributor

One question I have outside of the review is: if a ~/.aws/config file does not exist, the aws-vault functionality (without shell plugin in its way) prompts only for access key ID and secret key, but the two of them alone cannot be used without the region.

I believe that's expected behavior with aws-vault, but the larger question I have on my mind is, we completely skip the aws-vault importer when ~/.aws/config does not exist -- is that expected?

Do you think we should explore a flow that imports credential from aws-vault, if the credential exists, along with region sourced from AWS_REGION envvar, again if it exists?

@AndyTitu
Copy link
Contributor Author

Do you think we should explore a flow that imports credential from aws-vault, if the credential exists, along with region sourced from AWS_REGION envvar, again if it exists?

Good question! We are already checking for the AWS_DEFAULT_REGION envvar to import an existing region for the disk importer. Since importers are all called together, there is no need to duplicate checking for a region env var inside the aws-vault importer as well.
I agree we could also check the config file for getting a region. We should only check the default profile though, as the information stored in 1Password acts as a default profile -- it is globally used in all other profiles. Each individual profile can then specify its own region inside its section in the config.
I have opened a starter issue for adding this logic in the importer: #302

access key ID and secret key, but the two of them alone cannot be used without the region.

They can be used without a region except for when using MFA, in which case we error out if we can't find a region.

@AndyTitu AndyTitu merged commit 2d28d62 into main Jun 21, 2023
4 checks passed
@AndyTitu AndyTitu deleted the andi_t/dont-create-file-when-importing branch June 21, 2023 14:35
@arunsathiya arunsathiya removed the waiting-on-reviewer signals that a certain PR is waiting for a review from a 1Password team member label Aug 15, 2023
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

3 participants