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

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

Fix AWS Region with Default region #478

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
0e4308b
[aws/sts_provisioner.go] Don't favor using default_region
NickLaMuro Aug 5, 2024
ec4ef7a
[aws/sts_provisioner.go] Remove usage of hasRegion
NickLaMuro Aug 5, 2024
f3cded3
[access_key_test.go] Fix test
NickLaMuro Aug 6, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions plugins/aws/access_key_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.

},
awsConfig: &confighelpers.Config{
ProfileName: "dev",
MfaSerial: "arn:aws:iam::123456789012:mfa/user",
Region: "us-east-1",
},
err: fmt.Errorf("your local AWS configuration (config file or environment variable) has a different default region than the one specified in 1Password"),
err: fmt.Errorf("your local AWS configuration (config file or environment variable) has a different region than the one specified in 1Password"),
},
} {
t.Run(scenario.description, func(t *testing.T) {
Expand Down
6 changes: 2 additions & 4 deletions plugins/aws/sts_provisioner.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -227,8 +227,6 @@ func resolveLocalAnd1PasswordConfigurations(itemFields map[sdk.FieldName]string,
region = defaultRegion
}

hasRegion := hasDefaultRegion || hasRegularRegion

// only 1Password OTPs are supported
if awsConfig.MfaToken != "" || awsConfig.MfaProcess != "" || awsConfig.MfaPromptMethod != "" {
return fmt.Errorf("only 1Password-backed OTP authentication is supported by the MFA worklfow of the AWS shell plugin")
Expand All @@ -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?

Copy link
Author

@NickLaMuro NickLaMuro Jul 2, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Sorry, getting back to this as I have been a bit swamped at work lately, and as you will probably notice below... it isn't a simple "one line" response to your answer...

If the 1Password item contains default region, use that one

So, this is where I have disagreement, though, recognize it has a nuanced take, and won't die on a hill over it. However, I think it is worth stating all the facts before making a decision.

To start, what is currently in place is wrong: treating "region" and "default region" as the same, when "default region" should only be used to in absence of a --region and $AWS_REGION.

In addition, up in the test file you say this:

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.

about this change:

 			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",

I personally take issue with it for two reasons:

  1. The spec description doesn't match the arguments that are passed. It passes DefaultRegion from the fieldname, but the region is what is being described in the description:. So this test will need to change regardless to properly reflect what it is actually trying to test.
  2. I disagree with the premise this should stay the same for backwards compatibility, because again, I think this functionality is broken. Either:
    a. It should be describing fieldname.region, which then there should be an error when conflicting with what is in the ~/.aws/config already (what I changed the test to be currently).
    b. It should be describing fieldname.defaultRegion, where we then need to set up a proper order of precedence for this plugin and then describe what should actually be tested here.

And to be clear, I should write another test case in addition to what was already there, but when I original was writing this up (9 months ago at this point), I was having trouble getting specs working locally, so I sorta gave up... but I will add another test once we decide on the a/b from above (or, as I get to later, a better understanding of how this plugin should behave, as I argue it is unclear).

So regarding the existing region/default_region precedence from AWS, we have a decent number documents to sort of derive what it is for the AWS CLI/SDK before introducing the logic from this plugin:

I think the first 3 links are the most useful for trying to understand the general patterns across the CLI and all of the SDKs AWS provides, but from what I found, the description in the Ruby (my main language, so also am most comfortable with it) was the most concise example for an SDK.

Generally speaking, region precedence is derived in this order of priority of CLI FLAGS > ENV VARS > ~/.aws/config, with a more detail list below (ordered from highest precedence to least):

  • Directly with a CLI flag (--region) or client object (in Ruby... AWS::EC2::Client.new(:region => "..."))
  • Directly in a SDK's config object (if it exists)
    • in Ruby: Aws.config(:region => "...")
  • $AWS_REGION set in the ENV
  • $AWS_DEFAULT_REGION set in the ENV
  • The region = for the current ~/.aws/config profile, either provided as:
    • --profile (for the CLI specifically)
    • AWS_PROFILE="my_profile" (everything)
    • (you might be able to define a profile in a SDK programmatically as well, but didn't take the time to look up a example)
  • The region for the default config in the ~/.aws/config
    • I will refer to this as [default]region going forward

Of note is the $AWS_DEFAULT_REGION env var. My default assumption (prior to researching and looking it up), would be to think that it would be of a lesser priority than then current profile's region (so if AWS_PROFILE=foo is set, and the [foo]region is set, it actually supersede that that and $AWS_DEFAULT_REGION will have the higher priority).

The following example can be used as an example explain a bunch of different scenarios (though, I don't think it is exhaustive):

~/.aws/config

[default]
region = "us-east-1"

[uk]
region = "eu-west-2"

$ aws sts get-caller-identity --debug 2>&1 | grep client_region
# sets the region to us-east-1 (from `default` profile in `~/.aws/config`)
$ AWS_REGION=us-west-1 aws sts get-caller-identity --debug 2>&1 | grep client_region
# sets the region to us-west-1 (from `AWS_REGION`)
$ AWS_DEFAULT_REGION=ca-central-1 aws sts get-caller-identity --debug 2>&1 | grep client_region
# sets the region to ca-central-1 (from `AWS_DEFAULT_REGION`)
$ AWS_DEFAULT_REGION=ca-central-1 AWS_REGION=us-west-1 aws sts get-caller-identity --debug 2>&1 | grep client_region
# sets the region to us-west-1 (from `AWS_REGION`)
$ AWS_DEFAULT_REGION=ca-central-1 aws sts get-caller-identity --profile uk --debug 2>&1 | grep client_region
# sets the region to ca-central-1 (from `AWS_DEFAULT_REGION`)
$ aws sts get-caller-identity --profile uk --debug 2>&1 | grep client_region
# sets the region to eu-west-2 (from `uk` profile in `~/.aws/config`)

Again, I would personally would assume normally that AWS_DEFAULT_REGION would be treated more like [default]region in the ~/.aws/config, just with a slightly higher priority, but instead, it completely supersedes the precedence even the [uk]region above. I guess the thought process from AWS' perspective is any ENV takes priority over anything in the ~/.aws/config, versus anything "default region" being evaluated first, then anything specifically "region" being evaluated after (how I personally would like it to be and makes more sense in my eyes).

Where I think a decision needs to be made is where 1Password, and this plugin, should inject itself in this mess, which I think is a non-trival question to answer.

For me personally , I think having defaultRegion be the lowest priority beyond anything above makes the most sense to me. Basically, set region to that if nothing else exists to set it, otherwise use what is provided through any other means (including fieldname.region from 1Password), and not error at all. This will act different than how fieldname.region currently works, where it will error if there is a conflict. To me, this is the most useful, where I don't want to be editing my defaultRegion in 1Password ever, and it is simply there to provide something when all else is absent, and probably most people will be using ~/.aws/config anyway, so a default can really be set there.

If, instead, we are treating this more similar to $AWS_DEFAULT_REGION, which, based on the current documentation is a reasonable conclusion to come to:

Then I don't really see this having errors on around it, and it is simply a backup or initial setting of $AWS_DEFAULT_REGION isn't already set, but still will take more precedence than what is in the ~/.aws/config. What was here previously was then erroring if either fieldname.region or fieldname.defaultRegion is defined, which I think don't think makes any sense all, and what I am trying to change here. But I can at least understand logically making it behave more like the existing $AWS_DEFAULT_REGION ENV var it is meant to stand in for, regardless if it is confusing to me personally or not.

With region, I can more understand that field having errors, since without them, you could mask errors/unintended behaviour when used in conjunction with ~/.aws/config. However, I don't see anywhere in the docs above where fieldname.region is even mentioned to define it, so I this as kind of a hidden feature anyway and makes it hard to for me to determine what the expectations are. I may had a bit better of an idea of the intentions when I original wrote up this PR, as I did spend some time doing some git history explanation into this plugin, but any knowledge of that has escaped me at this point.

Personally, I don't see any utility in having the region in 1Password without it being able to be overwritten by a profile anyway (so, the errors are kind of an anti-feature to me), but also that isn't my biggest concern with this PR, and instead separating fieldname.defaultRegion from the fieldname.region is.

Main point with all of this is that without defining the order of precedence, and what each of these fields are supposed to do in the context of the greater existing precedence and expectations (set by AWS), then there isn't much else that I can do here before that and we should probably define that first (ideally in the documentation).

At this point, I could see what I am writing here more as the start of a GitHub Issue, which in hindsight, maybe is what I should have started with in the first place (miscalculation on the quagmire I was getting myself into I guess, so mostly self induced and a realization in hindsight). So if that makes more sense, we can do that and disregard this PR until the expectations of how this plugin should treat these fields is determined. I would say even having this discussion in regards to the documentation makes more sense, but I am not sure there is a opensource repo for me to do that against so that might not be an option.

Curious on your thoughts on what makes sense going forward. Thanks.

return fmt.Errorf("your local AWS configuration (config file or environment variable) has a different region than the one specified in 1Password")
} else if awsConfig.Region == "" {
awsConfig.Region = region
}
Expand Down