From 7c3f0aa3b2f4d10ad37bc12ebf7217297c5fd141 Mon Sep 17 00:00:00 2001 From: Megan Marsh Date: Tue, 16 Oct 2018 14:15:55 -0700 Subject: [PATCH 1/3] make sure region validation catches authentication errors --- builder/amazon/common/access_config.go | 5 ++++- builder/amazon/common/ami_config.go | 6 +++++- builder/amazon/common/regions.go | 22 +++++++++++++++------- 3 files changed, 24 insertions(+), 9 deletions(-) diff --git a/builder/amazon/common/access_config.go b/builder/amazon/common/access_config.go index 4ce478dd9..16dde5965 100644 --- a/builder/amazon/common/access_config.go +++ b/builder/amazon/common/access_config.go @@ -149,7 +149,10 @@ func (c *AccessConfig) Prepare(ctx *interpolate.Context) []error { if c.RawRegion != "" && !c.SkipValidation { ec2conn := getValidationSession() - if valid := ValidateRegion(c.RawRegion, ec2conn); !valid { + valid, err := ValidateRegion(c.RawRegion, ec2conn) + if err != nil { + errs = append(errs, fmt.Errorf("error validating region: %s", err.Error())) + } else if !valid { errs = append(errs, fmt.Errorf("Unknown region: %s", c.RawRegion)) } } diff --git a/builder/amazon/common/ami_config.go b/builder/amazon/common/ami_config.go index 58c6b8827..49fc10cfe 100644 --- a/builder/amazon/common/ami_config.go +++ b/builder/amazon/common/ami_config.go @@ -112,7 +112,11 @@ func (c *AMIConfig) prepareRegions(ec2conn ec2iface.EC2API, accessConfig *Access if !c.AMISkipRegionValidation { // Verify the region is real - if valid := ValidateRegion(region, ec2conn); !valid { + ec2conn := getValidationSession() + valid, err := ValidateRegion(region, ec2conn) + if err != nil { + errs = append(errs, fmt.Errorf("error validating region: %s", err.Error())) + } else if !valid { errs = append(errs, fmt.Errorf("Unknown region: %s", region)) } } diff --git a/builder/amazon/common/regions.go b/builder/amazon/common/regions.go index 67dee4212..b76e1db57 100644 --- a/builder/amazon/common/regions.go +++ b/builder/amazon/common/regions.go @@ -15,23 +15,31 @@ func getValidationSession() *ec2.EC2 { return ec2conn } -func listEC2Regions(ec2conn ec2iface.EC2API) []string { +func listEC2Regions(ec2conn ec2iface.EC2API) ([]string, error) { var regions []string - resultRegions, _ := ec2conn.DescribeRegions(nil) + resultRegions, err := ec2conn.DescribeRegions(nil) + if err != nil { + return []string{}, err + } for _, region := range resultRegions.Regions { regions = append(regions, *region.RegionName) } - return regions + return regions, nil } // ValidateRegion returns true if the supplied region is a valid AWS // region and false if it's not. -func ValidateRegion(region string, ec2conn ec2iface.EC2API) bool { - for _, valid := range listEC2Regions(ec2conn) { +func ValidateRegion(region string, ec2conn ec2iface.EC2API) (bool, error) { + regions, err := listEC2Regions(ec2conn) + if err != nil { + return false, err + } + + for _, valid := range regions { if region == valid { - return true + return true, nil } } - return false + return false, nil } From d4767845f0adc62eeb2fdca86600c5fe400d938b Mon Sep 17 00:00:00 2001 From: Megan Marsh Date: Tue, 16 Oct 2018 15:01:13 -0700 Subject: [PATCH 2/3] fix tests --- builder/amazon/common/access_config_test.go | 6 +++--- builder/amazon/common/ami_config.go | 1 - builder/amazon/common/ami_config_test.go | 6 +++++- 3 files changed, 8 insertions(+), 5 deletions(-) diff --git a/builder/amazon/common/access_config_test.go b/builder/amazon/common/access_config_test.go index 833a6177e..c3e5a4f74 100644 --- a/builder/amazon/common/access_config_test.go +++ b/builder/amazon/common/access_config_test.go @@ -17,19 +17,19 @@ func TestAccessConfigPrepare_Region(t *testing.T) { mockConn := &mockEC2Client{} c.RawRegion = "us-east-12" - valid := ValidateRegion(c.RawRegion, mockConn) + valid, _ := ValidateRegion(c.RawRegion, mockConn) if valid { t.Fatalf("should have region validation err: %s", c.RawRegion) } c.RawRegion = "us-east-1" - valid = ValidateRegion(c.RawRegion, mockConn) + valid, _ = ValidateRegion(c.RawRegion, mockConn) if !valid { t.Fatalf("shouldn't have region validation err: %s", c.RawRegion) } c.RawRegion = "custom" - valid = ValidateRegion(c.RawRegion, mockConn) + valid, _ = ValidateRegion(c.RawRegion, mockConn) if valid { t.Fatalf("should have region validation err: %s", c.RawRegion) } diff --git a/builder/amazon/common/ami_config.go b/builder/amazon/common/ami_config.go index 49fc10cfe..7e44ae34a 100644 --- a/builder/amazon/common/ami_config.go +++ b/builder/amazon/common/ami_config.go @@ -112,7 +112,6 @@ func (c *AMIConfig) prepareRegions(ec2conn ec2iface.EC2API, accessConfig *Access if !c.AMISkipRegionValidation { // Verify the region is real - ec2conn := getValidationSession() valid, err := ValidateRegion(region, ec2conn) if err != nil { errs = append(errs, fmt.Errorf("error validating region: %s", err.Error())) diff --git a/builder/amazon/common/ami_config_test.go b/builder/amazon/common/ami_config_test.go index 0e4f28e22..9afad469c 100644 --- a/builder/amazon/common/ami_config_test.go +++ b/builder/amazon/common/ami_config_test.go @@ -55,13 +55,17 @@ func TestAMIConfigPrepare_regions(t *testing.T) { c.AMISkipRegionValidation = true var errs []error + var err error mockConn := &mockEC2Client{} if errs = c.prepareRegions(mockConn, nil, errs); len(errs) > 0 { t.Fatalf("shouldn't have err: %#v", errs) } c.AMISkipRegionValidation = false - c.AMIRegions = listEC2Regions(mockConn) + c.AMIRegions, err = listEC2Regions(mockConn) + if err != nil { + t.Fatalf("shouldn't have err: %s", err.Error()) + } if errs = c.prepareRegions(mockConn, nil, errs); len(errs) > 0 { t.Fatalf("shouldn't have err: %#v", errs) } From b0cfecf31460cda7c9844c271171ac7d415f9aae Mon Sep 17 00:00:00 2001 From: Megan Marsh Date: Wed, 17 Oct 2018 11:03:31 -0700 Subject: [PATCH 3/3] remove unnecessary valid flag --- builder/amazon/common/access_config.go | 4 +--- builder/amazon/common/access_config_test.go | 12 ++++++------ builder/amazon/common/ami_config.go | 4 +--- builder/amazon/common/regions.go | 10 ++++++---- 4 files changed, 14 insertions(+), 16 deletions(-) diff --git a/builder/amazon/common/access_config.go b/builder/amazon/common/access_config.go index 16dde5965..cb97ff91e 100644 --- a/builder/amazon/common/access_config.go +++ b/builder/amazon/common/access_config.go @@ -149,11 +149,9 @@ func (c *AccessConfig) Prepare(ctx *interpolate.Context) []error { if c.RawRegion != "" && !c.SkipValidation { ec2conn := getValidationSession() - valid, err := ValidateRegion(c.RawRegion, ec2conn) + err := ValidateRegion(c.RawRegion, ec2conn) if err != nil { errs = append(errs, fmt.Errorf("error validating region: %s", err.Error())) - } else if !valid { - errs = append(errs, fmt.Errorf("Unknown region: %s", c.RawRegion)) } } diff --git a/builder/amazon/common/access_config_test.go b/builder/amazon/common/access_config_test.go index c3e5a4f74..c94d78bff 100644 --- a/builder/amazon/common/access_config_test.go +++ b/builder/amazon/common/access_config_test.go @@ -17,20 +17,20 @@ func TestAccessConfigPrepare_Region(t *testing.T) { mockConn := &mockEC2Client{} c.RawRegion = "us-east-12" - valid, _ := ValidateRegion(c.RawRegion, mockConn) - if valid { + err := ValidateRegion(c.RawRegion, mockConn) + if err == nil { t.Fatalf("should have region validation err: %s", c.RawRegion) } c.RawRegion = "us-east-1" - valid, _ = ValidateRegion(c.RawRegion, mockConn) - if !valid { + err = ValidateRegion(c.RawRegion, mockConn) + if err != nil { t.Fatalf("shouldn't have region validation err: %s", c.RawRegion) } c.RawRegion = "custom" - valid, _ = ValidateRegion(c.RawRegion, mockConn) - if valid { + err = ValidateRegion(c.RawRegion, mockConn) + if err == nil { t.Fatalf("should have region validation err: %s", c.RawRegion) } diff --git a/builder/amazon/common/ami_config.go b/builder/amazon/common/ami_config.go index 7e44ae34a..fe3b0949c 100644 --- a/builder/amazon/common/ami_config.go +++ b/builder/amazon/common/ami_config.go @@ -112,11 +112,9 @@ func (c *AMIConfig) prepareRegions(ec2conn ec2iface.EC2API, accessConfig *Access if !c.AMISkipRegionValidation { // Verify the region is real - valid, err := ValidateRegion(region, ec2conn) + err := ValidateRegion(region, ec2conn) if err != nil { errs = append(errs, fmt.Errorf("error validating region: %s", err.Error())) - } else if !valid { - errs = append(errs, fmt.Errorf("Unknown region: %s", region)) } } diff --git a/builder/amazon/common/regions.go b/builder/amazon/common/regions.go index b76e1db57..eb8bba19b 100644 --- a/builder/amazon/common/regions.go +++ b/builder/amazon/common/regions.go @@ -1,6 +1,7 @@ package common import ( + "fmt" "github.com/aws/aws-sdk-go/aws/session" "github.com/aws/aws-sdk-go/service/ec2" "github.com/aws/aws-sdk-go/service/ec2/ec2iface" @@ -30,16 +31,17 @@ func listEC2Regions(ec2conn ec2iface.EC2API) ([]string, error) { // ValidateRegion returns true if the supplied region is a valid AWS // region and false if it's not. -func ValidateRegion(region string, ec2conn ec2iface.EC2API) (bool, error) { +func ValidateRegion(region string, ec2conn ec2iface.EC2API) error { regions, err := listEC2Regions(ec2conn) if err != nil { - return false, err + return err } for _, valid := range regions { if region == valid { - return true, nil + return nil } } - return false, nil + + return fmt.Errorf("Invalid region: %s", region) }