Skip to content

Fix AWS Region with Default region #478

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Fix AWS Region with Default region #478

wants to merge 3 commits into from
+4 −6

Conversation

NickLaMuro
Copy link

Overview

When using the aws plugin, if the DEFAULT_REGION is set in the 1Password item, it will override any usage of AWS_REGION, which should take president normally with the aws cli, preventing the usage of being able to connect to other regions unless you regularly change the DEFAULT_REGION value in the 1Password each time.

Type of change

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

Related Issue(s)

  • Resolves: #
  • Relates: #

How To Test

  • Set a DEFAULT_REGION value in your 1Password item you use for the aws plugin
  • Run a aws s3 ls (or something similar)
    • Validate these are in the region of DEFAULT_REGION
  • Run AWS_REGION=different-region aws s3 ls, using a different-region then the one you have configured
    • Validate the buckets from the previous command are different

In the previous version, this would return the same value for both commands, and this should fix that.

Other options is quering for something specific by ARN that exists in only in the DEFAULT_REGION, and with this patch, the second command will fail, where previously it would find it.

Changelog

Fix DEFAULT_REGION logic for aws plugin to allow for overrides using AWS_REGION.

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Aug 6, 2024

⚠️ This PR contains unsigned commits. To get your PR merged, please sign those commits (git rebase --exec 'git commit -S --amend --no-edit -n' @{upstream}) and force push them to this branch (git push --force-with-lease).

If you're new to commit signing, there are different ways to set it up:

Sign commits with gpg

Follow the steps below to set up commit signing with gpg:

  1. Generate a GPG key
  2. Add the GPG key to your GitHub account
  3. Configure git to use your GPG key for commit signing
Sign commits with ssh-agent

Follow the steps below to set up commit signing with ssh-agent:

  1. Generate an SSH key and add it to ssh-agent
  2. Add the SSH key to your GitHub account
  3. Configure git to use your SSH key for commit signing
Sign commits with 1Password

You can also sign commits using 1Password, which lets you sign commits with biometrics without the signing key leaving the local 1Password process.

Learn how to use 1Password to sign your commits.

Watch the demo

@NickLaMuro NickLaMuro force-pushed the fix-local-aws-config-default-region-issues branch from c743f7f to 2d45c84 Compare August 6, 2024 00:42
@NickLaMuro
[aws/sts_provisioner.go] Don't favor using default_region
0e4308b 
Default region should only be used to fill in a region when
awsConfig.Region doesn't exist (and set it accordingly, but not be the
sole determinant on if there is a difference.  It should defer to what
is already does in the `else if` later, where it sets it if it isn't
blank, but doesn't error.

Right now, it will error if `default region` is set in the 1Password
item, but in reality, it should only do that if `region` (aka:
`hasRegularRegion`) is set.

This updates this logic to reflect that.
@NickLaMuro
[aws/sts_provisioner.go] Remove usage of hasRegion
ec4ef7a 
Now not used by the error check below, so removing.
@NickLaMuro NickLaMuro force-pushed the fix-local-aws-config-default-region-issues branch from 2d45c84 to ec4ef7a Compare August 6, 2024 00:58
@NickLaMuro NickLaMuro force-pushed the fix-local-aws-config-default-region-issues branch from 97aed46 to f3cded3 Compare August 6, 2024 01:33
edif2008
edif2008 reviewed Apr 24, 2025
View reviewed changes
Copy link
Member

@edif2008 edif2008 left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution.

I've left some comments concerning the backwards compatibility of these changes. Could you please take a look and adjust your changes accordingly?
Let me know if you have any questions or need additional guidance. 😄

plugins/aws/access_key_test.go
@@ -643,14 +643,14 @@ func TestResolveLocalAnd1PasswordConfigurations(t *testing.T) {
description: "has region both in 1Password and local config, but values differ",
itemFields: map[sdk.FieldName]string{
fieldname.OneTimePassword: "515467",
fieldname.DefaultRegion: "us-east-2",
fieldname.Region: "us-east-2",
Copy link
Member

Choose a reason for hiding this comment

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

Instead of changing this test, can you add a new one in which the Region is configurerd? In this way, we ensure that this change is backwards compatible.

plugins/aws/sts_provisioner.go
@@ -250,8 +248,8 @@ Learn how to add an OTP field to your item:
https://developer.1password.com/docs/cli/shell-plugins/aws/#optional-set-up-multi-factor-authentication`, awsConfig.MfaSerial)
}

if hasRegion && awsConfig.Region != "" && region != awsConfig.Region {
return fmt.Errorf("your local AWS configuration (config file or environment variable) has a different default region than the one specified in 1Password")
if hasRegularRegion && awsConfig.Region != "" && region != awsConfig.Region {
Copy link
Member

Choose a reason for hiding this comment

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

This change looks backwards incompatible to me.

What we're trying to achieve is the following:

  • If the 1Password item doesn't have any region defined (default region nor region), then look for the default region in the config
  • If the 1Password item contains default region, use that one
  • If the 1Password item contains region, use that one
  • If the 1Password item contains default region and we set the environment variable AWS_REGION, then use the value from AWS_REGION

Out of these the following scenario seems to fail now:

A user has a default region in ~/.aws/config file and a default region defined in 1Password item. The shell plugin will now use the value in ~/.aws/config instead of the one in the 1Password item.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe this table explains it better in terms of the expected order of precedence:

AWS config file 1P item default region field 1P item region field AWS_REGION env var Expected result
✅ ❌ ❌ ❌ AWS config file value
✅ ✅ ❌ ❌ 1P item default region value
✅ ❌ ✅ ❌ 1P item region value
✅ ✅ ❌ ✅ AWS_REGION env var value

Out of these 4 cases, the second one doesn't work as expected with the current changes.

Are there any other scenarios that you're thinking of that should be covered by this PR in terms of order of precendence?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@edif2008 edif2008 edif2008 left review comments

@johnsonch johnsonch johnsonch approved these changes

At least 2 approving reviews are required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@NickLaMuro @johnsonch @edif2008