From fd058e4bbd8ab044f468ada508e34346b2ab0677 Mon Sep 17 00:00:00 2001 From: Becca Petrin Date: Thu, 21 Nov 2019 15:28:29 -0800 Subject: [PATCH 01/11] add sts_region param to aws auth client config --- builtin/credential/aws/backend.go | 50 +++++++++++------ builtin/credential/aws/client.go | 1 - builtin/credential/aws/path_config_client.go | 22 +++++++- builtin/credential/aws/path_login.go | 19 ++++++- builtin/credential/aws/path_role_test.go | 59 ++++++++++++++++++++ 5 files changed, 129 insertions(+), 22 deletions(-) diff --git a/builtin/credential/aws/backend.go b/builtin/credential/aws/backend.go index 8c52b4b79417..e9e81a4365b4 100644 --- a/builtin/credential/aws/backend.go +++ b/builtin/credential/aws/backend.go @@ -238,22 +238,33 @@ func (b *backend) resolveArnToRealUniqueId(ctx context.Context, s logical.Storag if err != nil { return "", err } - // This odd-looking code is here because IAM is an inherently global service. IAM and STS ARNs - // don't have regions in them, and there is only a single global endpoint for IAM; see - // http://docs.aws.amazon.com/general/latest/gr/rande.html#iam_region - // However, the ARNs do have a partition in them, because the GovCloud and China partitions DO - // have their own separate endpoints, and the partition is encoded in the ARN. If Amazon's Go SDK - // would allow us to pass a partition back to the IAM client, it would be much simpler. But it - // doesn't appear that's possible, so in order to properly support GovCloud and China, we do a - // circular dance of extracting the partition from the ARN, finding any arbitrary region in the - // partition, and passing that region back back to the SDK, so that the SDK can figure out the - // proper partition from the arbitrary region we passed in to look up the endpoint. - // Sigh - region := getAnyRegionForAwsPartition(entity.Partition) - if region == nil { - return "", fmt.Errorf("unable to resolve partition %q to a region", entity.Partition) + clientConf, err := b.lockedClientConfigEntry(ctx, s) + if err != nil { + return "", err } - iamClient, err := b.clientIAM(ctx, s, region.ID(), entity.AccountNumber) + regionID := "" + switch { + case clientConf != nil && clientConf.STSRegion != "": + regionID = clientConf.STSRegion + default: + // This odd-looking code is here because IAM is an inherently global service. IAM and STS ARNs + // don't have regions in them, and there is only a single global endpoint for IAM; see + // http://docs.aws.amazon.com/general/latest/gr/rande.html#iam_region + // However, the ARNs do have a partition in them, because the GovCloud and China partitions DO + // have their own separate endpoints, and the partition is encoded in the ARN. If Amazon's Go SDK + // would allow us to pass a partition back to the IAM client, it would be much simpler. But it + // doesn't appear that's possible, so in order to properly support GovCloud and China, we do a + // circular dance of extracting the partition from the ARN, finding any arbitrary region in the + // partition, and passing that region back back to the SDK, so that the SDK can figure out the + // proper partition from the arbitrary region we passed in to look up the endpoint. + // Sigh + region, err := getAnyRegionForAwsPartition(entity.Partition) + if err != nil { + return "", err + } + regionID = region.ID() + } + iamClient, err := b.clientIAM(ctx, s, regionID, entity.AccountNumber) if err != nil { return "", awsutil.AppendLogicalError(err) } @@ -293,18 +304,21 @@ func (b *backend) resolveArnToRealUniqueId(ctx context.Context, s logical.Storag // Adapted from https://docs.aws.amazon.com/sdk-for-go/api/aws/endpoints/ // the "Enumerating Regions and Endpoint Metadata" section -func getAnyRegionForAwsPartition(partitionId string) *endpoints.Region { +func getAnyRegionForAwsPartition(partitionId string) (*endpoints.Region, error) { resolver := endpoints.DefaultResolver() partitions := resolver.(endpoints.EnumPartitions).Partitions() for _, p := range partitions { if p.ID() == partitionId { + if _, ok := p.Services()["sts"]; !ok { + return nil, fmt.Errorf("entity partition ID %q doesn't support sts, please set 'sts_region' parameter to designate a region", partitionId) + } for _, r := range p.Regions() { - return &r + return &r, nil } } } - return nil + return nil, fmt.Errorf("no matching partition ID found for %q, please set 'sts_region' parameter to designate a region", partitionId) } const backendHelp = ` diff --git a/builtin/credential/aws/client.go b/builtin/credential/aws/client.go index 90524c5c2a5b..1c4813e3d24b 100644 --- a/builtin/credential/aws/client.go +++ b/builtin/credential/aws/client.go @@ -265,7 +265,6 @@ func (b *backend) clientIAM(ctx context.Context, s logical.Storage, region, acco // Create an AWS config object using a chain of providers var awsConfig *aws.Config awsConfig, err = b.getClientConfig(ctx, s, region, stsRole, accountID, "iam") - if err != nil { return nil, err } diff --git a/builtin/credential/aws/path_config_client.go b/builtin/credential/aws/path_config_client.go index 18cd8749e01c..f06873d70b06 100644 --- a/builtin/credential/aws/path_config_client.go +++ b/builtin/credential/aws/path_config_client.go @@ -42,6 +42,12 @@ func (b *backend) pathConfigClient() *framework.Path { Description: "URL to override the default generated endpoint for making AWS STS API calls.", }, + "sts_region": { + Type: framework.TypeString, + Default: "", + Description: "The region ID for the sts_endpoint, if set.", + }, + "iam_server_id_header_value": { Type: framework.TypeString, Default: "", @@ -127,6 +133,7 @@ func (b *backend) pathConfigClientRead(ctx context.Context, req *logical.Request "endpoint": clientConfig.Endpoint, "iam_endpoint": clientConfig.IAMEndpoint, "sts_endpoint": clientConfig.STSEndpoint, + "sts_region": clientConfig.STSRegion, "iam_server_id_header_value": clientConfig.IAMServerIdHeaderValue, "max_retries": clientConfig.MaxRetries, }, @@ -217,7 +224,7 @@ func (b *backend) pathConfigClientCreateUpdate(ctx context.Context, req *logical stsEndpointStr, ok := data.GetOk("sts_endpoint") if ok { if configEntry.STSEndpoint != stsEndpointStr.(string) { - // We don't directly cache STS clients as they are ever directly used. + // We don't directly cache STS clients as they are never directly used. // However, they are potentially indirectly used as credential providers // for the EC2 and IAM clients, and thus we would be indirectly caching // them there. So, if we change the STS endpoint, we should flush those @@ -229,6 +236,18 @@ func (b *backend) pathConfigClientCreateUpdate(ctx context.Context, req *logical configEntry.STSEndpoint = data.Get("sts_endpoint").(string) } + stsRegionStr, ok := data.GetOk("sts_region") + if ok { + if configEntry.STSRegion != stsRegionStr.(string) { + // Region is used when building STS clients. As such, all the comments + // regarding the sts_endpoint changing apply here as well. + changedCreds = true + configEntry.STSRegion = stsRegionStr.(string) + } + } else if req.Operation == logical.CreateOperation { + configEntry.STSRegion = data.Get("sts_region").(string) + } + headerValStr, ok := data.GetOk("iam_server_id_header_value") if ok { if configEntry.IAMServerIdHeaderValue != headerValStr.(string) { @@ -281,6 +300,7 @@ type clientConfig struct { Endpoint string `json:"endpoint"` IAMEndpoint string `json:"iam_endpoint"` STSEndpoint string `json:"sts_endpoint"` + STSRegion string `json:"sts_region"` IAMServerIdHeaderValue string `json:"iam_server_id_header_value"` MaxRetries int `json:"max_retries"` } diff --git a/builtin/credential/aws/path_login.go b/builtin/credential/aws/path_login.go index 94e34d631f0c..56d2f13ff216 100644 --- a/builtin/credential/aws/path_login.go +++ b/builtin/credential/aws/path_login.go @@ -1633,8 +1633,23 @@ func (e *iamEntity) canonicalArn() string { // This returns the "full" ARN of an iamEntity, how it would be referred to in AWS proper func (b *backend) fullArn(ctx context.Context, e *iamEntity, s logical.Storage) (string, error) { - // Not assuming path is reliable for any entity types - client, err := b.clientIAM(ctx, s, getAnyRegionForAwsPartition(e.Partition).ID(), e.AccountNumber) + clientConf, err := b.lockedClientConfigEntry(ctx, s) + if err != nil { + return "", err + } + regionID := "" + switch { + case clientConf != nil && clientConf.STSRegion != "": + regionID = clientConf.STSRegion + default: + // Not assuming path is reliable for any entity types + region, err := getAnyRegionForAwsPartition(e.Partition) + if err != nil { + return "", err + } + regionID = region.ID() + } + client, err := b.clientIAM(ctx, s, regionID, e.AccountNumber) if err != nil { return "", errwrap.Wrapf("error creating IAM client: {{err}}", err) } diff --git a/builtin/credential/aws/path_role_test.go b/builtin/credential/aws/path_role_test.go index 13e58c173843..4e7ccaf39a35 100644 --- a/builtin/credential/aws/path_role_test.go +++ b/builtin/credential/aws/path_role_test.go @@ -986,6 +986,65 @@ func TestAwsVersion(t *testing.T) { } } +// This test was used to reproduces https://github.com/hashicorp/vault/issues/7418 +// and verify its fix. +func TestRoleResolutionWithSTSEndpointConfigured(t *testing.T) { + t.Skip("skipping test because it hits real endpoints") + + config := logical.TestBackendConfig() + storage := &logical.InmemStorage{} + config.StorageView = storage + + b, err := Backend(config) + if err != nil { + t.Fatal(err) + } + + err = b.Setup(context.Background(), config) + if err != nil { + t.Fatal(err) + } + + // configure the client with an sts endpoint that should be used in creating the role + data := map[string]interface{}{ + "sts_endpoint": "https://sts.eu-west-1.amazonaws.com", + // Note - if you comment this out, you can reproduce the error shown + // in the linked GH issue above. This essentially reproduces the problem + // we had when we didn't have an sts_region field. + "sts_region": "eu-west-1", + } + resp, err := b.HandleRequest(context.Background(), &logical.Request{ + Operation: logical.CreateOperation, + Path: "config/client", + Data: data, + Storage: storage, + }) + if err != nil { + t.Fatal(err) + } + if resp != nil && resp.IsError() { + t.Fatalf("failed to create the role entry; resp: %#v", resp) + } + + data = map[string]interface{}{ + "auth_type": iamAuthType, + "bound_iam_principal_arn": "arn:aws:iam::123456789012:assumed-role/MyRole/foo", + "resolve_aws_unique_ids": true, + } + resp, err = b.HandleRequest(context.Background(), &logical.Request{ + Operation: logical.CreateOperation, + Path: "role/MyRoleName", + Data: data, + Storage: storage, + }) + if err != nil { + t.Fatal(err) + } + if resp != nil && resp.IsError() { + t.Fatalf("failed to create the role entry; resp: %#v", resp) + } +} + func resolveArnToFakeUniqueId(_ context.Context, _ logical.Storage, _ string) (string, error) { return "FakeUniqueId1", nil } From e6739b39b04281d89a318adaf10921bff4b1e701 Mon Sep 17 00:00:00 2001 From: Becca Petrin Date: Thu, 21 Nov 2019 16:33:41 -0800 Subject: [PATCH 02/11] break if region unfound --- builtin/credential/aws/backend.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/builtin/credential/aws/backend.go b/builtin/credential/aws/backend.go index e9e81a4365b4..fcd076cd3ab6 100644 --- a/builtin/credential/aws/backend.go +++ b/builtin/credential/aws/backend.go @@ -316,6 +316,9 @@ func getAnyRegionForAwsPartition(partitionId string) (*endpoints.Region, error) for _, r := range p.Regions() { return &r, nil } + // Just in case a matching region wasn't found, we should break here + // so we can error below. + break } } return nil, fmt.Errorf("no matching partition ID found for %q, please set 'sts_region' parameter to designate a region", partitionId) From 8eec903d1cae8924b02055a08fe9ee6ec4b70d6b Mon Sep 17 00:00:00 2001 From: Becca Petrin Date: Fri, 22 Nov 2019 11:36:43 -0800 Subject: [PATCH 03/11] increase test coverage --- .../credential/aws/path_config_client_test.go | 20 ++++++++++++++++++- 1 file changed, 19 insertions(+), 1 deletion(-) diff --git a/builtin/credential/aws/path_config_client_test.go b/builtin/credential/aws/path_config_client_test.go index ab6fa9b45a70..493d20d9df00 100644 --- a/builtin/credential/aws/path_config_client_test.go +++ b/builtin/credential/aws/path_config_client_test.go @@ -44,6 +44,7 @@ func TestBackend_pathConfigClient(t *testing.T) { data := map[string]interface{}{ "sts_endpoint": "https://my-custom-sts-endpoint.example.com", + "sts_region": "us-east-2", "iam_server_id_header_value": "vault_server_identification_314159", } resp, err = b.HandleRequest(context.Background(), &logical.Request{ @@ -52,7 +53,6 @@ func TestBackend_pathConfigClient(t *testing.T) { Data: data, Storage: storage, }) - if err != nil { t.Fatal(err) } @@ -75,8 +75,18 @@ func TestBackend_pathConfigClient(t *testing.T) { t.Fatalf("expected iam_server_id_header_value: '%#v'; returned iam_server_id_header_value: '%#v'", data["iam_server_id_header_value"], resp.Data["iam_server_id_header_value"]) } + if resp.Data["sts_endpoint"] != data["sts_endpoint"] { + t.Fatalf("expected sts_endpoint: '%#v'; returned sts_endpoint: '%#v'", + data["sts_endpoint"], resp.Data["sts_endpoint"]) + } + if resp.Data["sts_region"] != data["sts_region"] { + t.Fatalf("expected sts_region: '%#v'; returned sts_region: '%#v'", + data["sts_region"], resp.Data["sts_region"]) + } data = map[string]interface{}{ + "sts_endpoint": "https://my-custom-sts-endpoint2.example.com", + "sts_region": "us-west-1", "iam_server_id_header_value": "vault_server_identification_2718281", } resp, err = b.HandleRequest(context.Background(), &logical.Request{ @@ -108,4 +118,12 @@ func TestBackend_pathConfigClient(t *testing.T) { t.Fatalf("expected iam_server_id_header_value: '%#v'; returned iam_server_id_header_value: '%#v'", data["iam_server_id_header_value"], resp.Data["iam_server_id_header_value"]) } + if resp.Data["sts_endpoint"] != data["sts_endpoint"] { + t.Fatalf("expected sts_endpoint: '%#v'; returned sts_endpoint: '%#v'", + data["sts_endpoint"], resp.Data["sts_endpoint"]) + } + if resp.Data["sts_region"] != data["sts_region"] { + t.Fatalf("expected sts_region: '%#v'; returned sts_region: '%#v'", + data["sts_region"], resp.Data["sts_region"]) + } } From 1dbb501ae3cbfd7bbb313b35490b064e904a3310 Mon Sep 17 00:00:00 2001 From: Becca Petrin Date: Thu, 5 Dec 2019 15:47:12 -0800 Subject: [PATCH 04/11] perform region override in client.go --- builtin/credential/aws/backend.go | 53 ++++++++---------------- builtin/credential/aws/client.go | 10 ++++- builtin/credential/aws/path_login.go | 19 +-------- builtin/credential/aws/path_role_test.go | 2 +- 4 files changed, 29 insertions(+), 55 deletions(-) diff --git a/builtin/credential/aws/backend.go b/builtin/credential/aws/backend.go index fcd076cd3ab6..8c52b4b79417 100644 --- a/builtin/credential/aws/backend.go +++ b/builtin/credential/aws/backend.go @@ -238,33 +238,22 @@ func (b *backend) resolveArnToRealUniqueId(ctx context.Context, s logical.Storag if err != nil { return "", err } - clientConf, err := b.lockedClientConfigEntry(ctx, s) - if err != nil { - return "", err + // This odd-looking code is here because IAM is an inherently global service. IAM and STS ARNs + // don't have regions in them, and there is only a single global endpoint for IAM; see + // http://docs.aws.amazon.com/general/latest/gr/rande.html#iam_region + // However, the ARNs do have a partition in them, because the GovCloud and China partitions DO + // have their own separate endpoints, and the partition is encoded in the ARN. If Amazon's Go SDK + // would allow us to pass a partition back to the IAM client, it would be much simpler. But it + // doesn't appear that's possible, so in order to properly support GovCloud and China, we do a + // circular dance of extracting the partition from the ARN, finding any arbitrary region in the + // partition, and passing that region back back to the SDK, so that the SDK can figure out the + // proper partition from the arbitrary region we passed in to look up the endpoint. + // Sigh + region := getAnyRegionForAwsPartition(entity.Partition) + if region == nil { + return "", fmt.Errorf("unable to resolve partition %q to a region", entity.Partition) } - regionID := "" - switch { - case clientConf != nil && clientConf.STSRegion != "": - regionID = clientConf.STSRegion - default: - // This odd-looking code is here because IAM is an inherently global service. IAM and STS ARNs - // don't have regions in them, and there is only a single global endpoint for IAM; see - // http://docs.aws.amazon.com/general/latest/gr/rande.html#iam_region - // However, the ARNs do have a partition in them, because the GovCloud and China partitions DO - // have their own separate endpoints, and the partition is encoded in the ARN. If Amazon's Go SDK - // would allow us to pass a partition back to the IAM client, it would be much simpler. But it - // doesn't appear that's possible, so in order to properly support GovCloud and China, we do a - // circular dance of extracting the partition from the ARN, finding any arbitrary region in the - // partition, and passing that region back back to the SDK, so that the SDK can figure out the - // proper partition from the arbitrary region we passed in to look up the endpoint. - // Sigh - region, err := getAnyRegionForAwsPartition(entity.Partition) - if err != nil { - return "", err - } - regionID = region.ID() - } - iamClient, err := b.clientIAM(ctx, s, regionID, entity.AccountNumber) + iamClient, err := b.clientIAM(ctx, s, region.ID(), entity.AccountNumber) if err != nil { return "", awsutil.AppendLogicalError(err) } @@ -304,24 +293,18 @@ func (b *backend) resolveArnToRealUniqueId(ctx context.Context, s logical.Storag // Adapted from https://docs.aws.amazon.com/sdk-for-go/api/aws/endpoints/ // the "Enumerating Regions and Endpoint Metadata" section -func getAnyRegionForAwsPartition(partitionId string) (*endpoints.Region, error) { +func getAnyRegionForAwsPartition(partitionId string) *endpoints.Region { resolver := endpoints.DefaultResolver() partitions := resolver.(endpoints.EnumPartitions).Partitions() for _, p := range partitions { if p.ID() == partitionId { - if _, ok := p.Services()["sts"]; !ok { - return nil, fmt.Errorf("entity partition ID %q doesn't support sts, please set 'sts_region' parameter to designate a region", partitionId) - } for _, r := range p.Regions() { - return &r, nil + return &r } - // Just in case a matching region wasn't found, we should break here - // so we can error below. - break } } - return nil, fmt.Errorf("no matching partition ID found for %q, please set 'sts_region' parameter to designate a region", partitionId) + return nil } const backendHelp = ` diff --git a/builtin/credential/aws/client.go b/builtin/credential/aws/client.go index 1c4813e3d24b..5fbddbdc43ff 100644 --- a/builtin/credential/aws/client.go +++ b/builtin/credential/aws/client.go @@ -37,14 +37,19 @@ func (b *backend) getRawClientConfig(ctx context.Context, s logical.Storage, reg endpoint := aws.String("") var maxRetries int = aws.UseServiceDefaultRetries if config != nil { - // Override the default endpoint with the configured endpoint. + // Override the defaults with configured values. switch { case clientType == "ec2" && config.Endpoint != "": endpoint = aws.String(config.Endpoint) case clientType == "iam" && config.IAMEndpoint != "": endpoint = aws.String(config.IAMEndpoint) case clientType == "sts" && config.STSEndpoint != "": - endpoint = aws.String(config.STSEndpoint) + if config.STSEndpoint != "" { + endpoint = aws.String(config.STSEndpoint) + } + if config.STSRegion != "" { + region = config.STSRegion + } } credsConfig.AccessKey = config.AccessKey @@ -265,6 +270,7 @@ func (b *backend) clientIAM(ctx context.Context, s logical.Storage, region, acco // Create an AWS config object using a chain of providers var awsConfig *aws.Config awsConfig, err = b.getClientConfig(ctx, s, region, stsRole, accountID, "iam") + if err != nil { return nil, err } diff --git a/builtin/credential/aws/path_login.go b/builtin/credential/aws/path_login.go index 56d2f13ff216..94e34d631f0c 100644 --- a/builtin/credential/aws/path_login.go +++ b/builtin/credential/aws/path_login.go @@ -1633,23 +1633,8 @@ func (e *iamEntity) canonicalArn() string { // This returns the "full" ARN of an iamEntity, how it would be referred to in AWS proper func (b *backend) fullArn(ctx context.Context, e *iamEntity, s logical.Storage) (string, error) { - clientConf, err := b.lockedClientConfigEntry(ctx, s) - if err != nil { - return "", err - } - regionID := "" - switch { - case clientConf != nil && clientConf.STSRegion != "": - regionID = clientConf.STSRegion - default: - // Not assuming path is reliable for any entity types - region, err := getAnyRegionForAwsPartition(e.Partition) - if err != nil { - return "", err - } - regionID = region.ID() - } - client, err := b.clientIAM(ctx, s, regionID, e.AccountNumber) + // Not assuming path is reliable for any entity types + client, err := b.clientIAM(ctx, s, getAnyRegionForAwsPartition(e.Partition).ID(), e.AccountNumber) if err != nil { return "", errwrap.Wrapf("error creating IAM client: {{err}}", err) } diff --git a/builtin/credential/aws/path_role_test.go b/builtin/credential/aws/path_role_test.go index 4e7ccaf39a35..3c332261bad5 100644 --- a/builtin/credential/aws/path_role_test.go +++ b/builtin/credential/aws/path_role_test.go @@ -986,7 +986,7 @@ func TestAwsVersion(t *testing.T) { } } -// This test was used to reproduces https://github.com/hashicorp/vault/issues/7418 +// This test was used to reproduce https://github.com/hashicorp/vault/issues/7418 // and verify its fix. func TestRoleResolutionWithSTSEndpointConfigured(t *testing.T) { t.Skip("skipping test because it hits real endpoints") From 7b8888286e266225b2377d2db0d67905fc589404 Mon Sep 17 00:00:00 2001 From: Becca Petrin Date: Thu, 5 Dec 2019 15:48:19 -0800 Subject: [PATCH 05/11] only check STSEndpoint once --- builtin/credential/aws/client.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/builtin/credential/aws/client.go b/builtin/credential/aws/client.go index 5fbddbdc43ff..e495dd257967 100644 --- a/builtin/credential/aws/client.go +++ b/builtin/credential/aws/client.go @@ -43,7 +43,7 @@ func (b *backend) getRawClientConfig(ctx context.Context, s logical.Storage, reg endpoint = aws.String(config.Endpoint) case clientType == "iam" && config.IAMEndpoint != "": endpoint = aws.String(config.IAMEndpoint) - case clientType == "sts" && config.STSEndpoint != "": + case clientType == "sts": if config.STSEndpoint != "" { endpoint = aws.String(config.STSEndpoint) } From 66a08b82acbbcd379891b427326bc779d71d92f0 Mon Sep 17 00:00:00 2001 From: Becca Petrin Date: Fri, 6 Dec 2019 15:25:38 -0800 Subject: [PATCH 06/11] changes from feedback --- builtin/credential/aws/path_config_client.go | 2 -- builtin/credential/aws/path_role_test.go | 5 ++++- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/builtin/credential/aws/path_config_client.go b/builtin/credential/aws/path_config_client.go index f06873d70b06..228488e30220 100644 --- a/builtin/credential/aws/path_config_client.go +++ b/builtin/credential/aws/path_config_client.go @@ -244,8 +244,6 @@ func (b *backend) pathConfigClientCreateUpdate(ctx context.Context, req *logical changedCreds = true configEntry.STSRegion = stsRegionStr.(string) } - } else if req.Operation == logical.CreateOperation { - configEntry.STSRegion = data.Get("sts_region").(string) } headerValStr, ok := data.GetOk("iam_server_id_header_value") diff --git a/builtin/credential/aws/path_role_test.go b/builtin/credential/aws/path_role_test.go index 3c332261bad5..aa5ea3ef6c60 100644 --- a/builtin/credential/aws/path_role_test.go +++ b/builtin/credential/aws/path_role_test.go @@ -988,6 +988,9 @@ func TestAwsVersion(t *testing.T) { // This test was used to reproduce https://github.com/hashicorp/vault/issues/7418 // and verify its fix. +// Please run it at least 3 times to ensure that passing tests are due to actually +// passing, rather than the region being randomly chosen tying to the one in the +// test through luck. func TestRoleResolutionWithSTSEndpointConfigured(t *testing.T) { t.Skip("skipping test because it hits real endpoints") @@ -1028,7 +1031,7 @@ func TestRoleResolutionWithSTSEndpointConfigured(t *testing.T) { data = map[string]interface{}{ "auth_type": iamAuthType, - "bound_iam_principal_arn": "arn:aws:iam::123456789012:assumed-role/MyRole/foo", + "bound_iam_principal_arn": "arn:aws:iam::123456789012:role/MyRole", "resolve_aws_unique_ids": true, } resp, err = b.HandleRequest(context.Background(), &logical.Request{ From db4f297a7e1be337b759c4e492df0eaf54a103c6 Mon Sep 17 00:00:00 2001 From: Becca Petrin Date: Fri, 6 Dec 2019 16:14:42 -0800 Subject: [PATCH 07/11] automate test to run if values given --- builtin/credential/aws/path_role_test.go | 25 ++++++++++++++++++++++-- 1 file changed, 23 insertions(+), 2 deletions(-) diff --git a/builtin/credential/aws/path_role_test.go b/builtin/credential/aws/path_role_test.go index aa5ea3ef6c60..6cf7aa44f033 100644 --- a/builtin/credential/aws/path_role_test.go +++ b/builtin/credential/aws/path_role_test.go @@ -2,6 +2,8 @@ package awsauth import ( "context" + "github.com/hashicorp/vault/helper/awsutil" + "os" "reflect" "strings" "testing" @@ -992,7 +994,26 @@ func TestAwsVersion(t *testing.T) { // passing, rather than the region being randomly chosen tying to the one in the // test through luck. func TestRoleResolutionWithSTSEndpointConfigured(t *testing.T) { - t.Skip("skipping test because it hits real endpoints") + if enabled := os.Getenv("VAULT_ACC"); enabled == "" { + t.Skip() + } + + // ex. "arn:aws:iam::123456789012:role/MyRole" + assumableRoleArn := os.Getenv("AWS_ASSUMABLE_ROLE_ARN") + if assumableRoleArn == "" { + t.Skip("skipping because AWS_ASSUMABLE_ROLE_ARN is unset") + } + + // Ensure aws credentials are available locally for testing. + credsConfig := &awsutil.CredentialsConfig{} + credsChain, err := credsConfig.GenerateCredentialChain() + if err != nil { + t.SkipNow() + } + _, err = credsChain.Get() + if err != nil { + t.SkipNow() + } config := logical.TestBackendConfig() storage := &logical.InmemStorage{} @@ -1031,7 +1052,7 @@ func TestRoleResolutionWithSTSEndpointConfigured(t *testing.T) { data = map[string]interface{}{ "auth_type": iamAuthType, - "bound_iam_principal_arn": "arn:aws:iam::123456789012:role/MyRole", + "bound_iam_principal_arn": assumableRoleArn, "resolve_aws_unique_ids": true, } resp, err = b.HandleRequest(context.Background(), &logical.Request{ From 03ade828d17955b73117c49d078666040856511b Mon Sep 17 00:00:00 2001 From: Becca Petrin Date: Fri, 6 Dec 2019 16:17:55 -0800 Subject: [PATCH 08/11] fix imports --- builtin/credential/aws/path_role_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/builtin/credential/aws/path_role_test.go b/builtin/credential/aws/path_role_test.go index 6cf7aa44f033..1077ad7cddd1 100644 --- a/builtin/credential/aws/path_role_test.go +++ b/builtin/credential/aws/path_role_test.go @@ -2,13 +2,13 @@ package awsauth import ( "context" - "github.com/hashicorp/vault/helper/awsutil" "os" "reflect" "strings" "testing" "github.com/go-test/deep" + "github.com/hashicorp/vault/helper/awsutil" "github.com/hashicorp/vault/sdk/helper/policyutil" "github.com/hashicorp/vault/sdk/helper/strutil" "github.com/hashicorp/vault/sdk/logical" From e23d8977db4886393f09e53387029cbb17f349c4 Mon Sep 17 00:00:00 2001 From: Becca Petrin Date: Tue, 10 Dec 2019 09:42:42 -0800 Subject: [PATCH 09/11] improve explanation regarding role arn Co-Authored-By: Joel Thompson --- builtin/credential/aws/path_role_test.go | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/builtin/credential/aws/path_role_test.go b/builtin/credential/aws/path_role_test.go index 1077ad7cddd1..e9ab630fa608 100644 --- a/builtin/credential/aws/path_role_test.go +++ b/builtin/credential/aws/path_role_test.go @@ -998,7 +998,10 @@ func TestRoleResolutionWithSTSEndpointConfigured(t *testing.T) { t.Skip() } - // ex. "arn:aws:iam::123456789012:role/MyRole" + /* ARN of an AWS role that Vault can query during testing. + This role should exist in your current AWS account and your credentials + should have iam:GetRole permissions to query it. + */ assumableRoleArn := os.Getenv("AWS_ASSUMABLE_ROLE_ARN") if assumableRoleArn == "" { t.Skip("skipping because AWS_ASSUMABLE_ROLE_ARN is unset") From 13dfdc12974a2aec8268d5579361f8b4baf6cf38 Mon Sep 17 00:00:00 2001 From: Becca Petrin Date: Tue, 10 Dec 2019 09:44:07 -0800 Subject: [PATCH 10/11] use shared testing var --- builtin/credential/aws/path_role_test.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/builtin/credential/aws/path_role_test.go b/builtin/credential/aws/path_role_test.go index 1077ad7cddd1..eca1ece0fb20 100644 --- a/builtin/credential/aws/path_role_test.go +++ b/builtin/credential/aws/path_role_test.go @@ -9,6 +9,7 @@ import ( "github.com/go-test/deep" "github.com/hashicorp/vault/helper/awsutil" + vlttesting "github.com/hashicorp/vault/helper/testhelpers/logical" "github.com/hashicorp/vault/sdk/helper/policyutil" "github.com/hashicorp/vault/sdk/helper/strutil" "github.com/hashicorp/vault/sdk/logical" @@ -994,7 +995,7 @@ func TestAwsVersion(t *testing.T) { // passing, rather than the region being randomly chosen tying to the one in the // test through luck. func TestRoleResolutionWithSTSEndpointConfigured(t *testing.T) { - if enabled := os.Getenv("VAULT_ACC"); enabled == "" { + if enabled := os.Getenv(vlttesting.TestEnvVar); enabled == "" { t.Skip() } From 458a41896f4e66e7ecc7bd1343167e4ea8111451 Mon Sep 17 00:00:00 2001 From: Becca Petrin Date: Tue, 10 Dec 2019 15:58:10 -0800 Subject: [PATCH 11/11] fail if acc tests and aws creds unset --- builtin/credential/aws/path_role_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/builtin/credential/aws/path_role_test.go b/builtin/credential/aws/path_role_test.go index 07f54f4011cc..f3daec40b5dc 100644 --- a/builtin/credential/aws/path_role_test.go +++ b/builtin/credential/aws/path_role_test.go @@ -1012,7 +1012,7 @@ func TestRoleResolutionWithSTSEndpointConfigured(t *testing.T) { credsConfig := &awsutil.CredentialsConfig{} credsChain, err := credsConfig.GenerateCredentialChain() if err != nil { - t.SkipNow() + t.Fatal(err) } _, err = credsChain.Get() if err != nil {