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

d/aws_iam_session_context: New data source #19957

Merged
merged 29 commits into from
Jun 28, 2021
Merged

Conversation

YakDriver
Copy link
Member

@YakDriver YakDriver commented Jun 24, 2021

Community Note

  • Please vote on this pull request by adding a 👍 reaction to the original pull request comment to help the community and maintainers prioritize this request
  • Please do not leave "+1" or other comments that do not add relevant new information or questions, they generate extra noise for pull request followers and do not help prioritize the request

Closes #19958

Special thanks to @dpowley for helping storyboard a solution!

Output from acceptance/unit testing (us-west-2):

--- PASS: TestAssumedRoleRoleSessionName (0.00s)
    --- PASS: TestAssumedRoleRoleSessionName/not_an_ARN (0.00s)
    --- PASS: TestAssumedRoleRoleSessionName/regular_role_ARN (0.00s)
    --- PASS: TestAssumedRoleRoleSessionName/assumed_role_ARN (0.00s)
    --- PASS: TestAssumedRoleRoleSessionName/'assumed-role'_part_of_ARN_resource (0.00s)
    --- PASS: TestAssumedRoleRoleSessionName/user_ARN (0.00s)
    --- PASS: TestAssumedRoleRoleSessionName/assumed_role_from_AWS_example (0.00s)
    --- PASS: TestAssumedRoleRoleSessionName/multiple_slashes_in_resource (0.00s)
    --- PASS: TestAssumedRoleRoleSessionName/not_an_sts_ARN (0.00s)
    --- PASS: TestAssumedRoleRoleSessionName/role_with_path (0.00s)
    --- PASS: TestAssumedRoleRoleSessionName/wrong_service (0.00s)
--- PASS: TestAccAWSDataSourceIAMSessionContext_notAssumedRoleUser (13.05s)
--- PASS: TestAccAWSDataSourceIAMSessionContext_notAssumedRoleWithPath (16.37s)
--- PASS: TestAccAWSDataSourceIAMSessionContext_notAssumedRole (16.47s)
--- PASS: TestAccAWSDataSourceIAMSessionContext_basic (16.48s)
--- PASS: TestAccAWSDataSourceIAMSessionContext_withPath (17.07s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	13.257s

Output from acceptance/unit testing (GovCloud):

--- PASS: TestAssumedRoleRoleSessionName (0.00s)
    --- PASS: TestAssumedRoleRoleSessionName/not_an_ARN (0.00s)
    --- PASS: TestAssumedRoleRoleSessionName/regular_role_ARN (0.00s)
    --- PASS: TestAssumedRoleRoleSessionName/assumed_role_ARN (0.00s)
    --- PASS: TestAssumedRoleRoleSessionName/'assumed-role'_part_of_ARN_resource (0.00s)
    --- PASS: TestAssumedRoleRoleSessionName/user_ARN (0.00s)
    --- PASS: TestAssumedRoleRoleSessionName/assumed_role_from_AWS_example (0.00s)
    --- PASS: TestAssumedRoleRoleSessionName/multiple_slashes_in_resource (0.00s)
    --- PASS: TestAssumedRoleRoleSessionName/not_an_sts_ARN (0.00s)
    --- PASS: TestAssumedRoleRoleSessionName/role_with_path (0.00s)
    --- PASS: TestAssumedRoleRoleSessionName/wrong_service (0.00s)
--- PASS: TestAccAWSDataSourceIAMSessionContext_notAssumedRoleUser (13.05s)
--- PASS: TestAccAWSDataSourceIAMSessionContext_notAssumedRole (19.06s)
--- PASS: TestAccAWSDataSourceIAMSessionContext_notAssumedRoleWithPath (19.07s)
--- PASS: TestAccAWSDataSourceIAMSessionContext_withPath (20.06s)
--- PASS: TestAccAWSDataSourceIAMSessionContext_basic (20.07s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	23.041s

@github-actions github-actions bot added size/M Managed by automation to categorize the size of a PR. documentation Introduces or discusses updates to documentation. service/sts Issues and PRs that pertain to the sts service. tests PRs: expanded test coverage. Issues: expanded coverage, enhancements to test infrastructure. labels Jun 24, 2021
@YakDriver YakDriver added the service/iam Issues and PRs that pertain to the iam service. label Jun 24, 2021
@github-actions github-actions bot removed the service/iam Issues and PRs that pertain to the iam service. label Jun 24, 2021
}

re := regexp.MustCompile(`^assumed-role/`)
parsedARN.Resource = re.ReplaceAllString(parsedARN.Resource, "role/")
Copy link
Contributor

Choose a reason for hiding this comment

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

This will return the incorrect sourceArn for roles with paths. The sts assumed-role ARN doesn't include the path of the role. For instance, if you have IAM role ARN arn:aws:iam::{accountId}:role/{path}/{roleName} and you assume that role , the assumed-role ARN will be arn:aws:sts::{accountId}:assumed-role/{roleName}/{sessionName}. To derive the sourceARN, you would need to call getRole after parsing the role name from the sts ARN. The problem is then, the caller identity data source will then require the iam:GetRole permission, which could theoretically break workflows.

Copy link
Member Author

Choose a reason for hiding this comment

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

😞 absolutely right

@github-actions github-actions bot added provider Pertains to the provider itself, rather than any interaction with AWS. service/iam Issues and PRs that pertain to the iam service. size/XL Managed by automation to categorize the size of a PR. and removed size/M Managed by automation to categorize the size of a PR. labels Jun 25, 2021
@YakDriver YakDriver changed the title d/caller_identity: Add source_arn for assumed roles d/aws_iam_assumed_role_source: New data source Jun 25, 2021
@YakDriver YakDriver requested a review from dpowley June 25, 2021 21:14
Copy link
Collaborator

@DrFaust92 DrFaust92 left a comment

Choose a reason for hiding this comment

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

Some minor comments, otherwise LGTM

@@ -109,3 +111,22 @@ func Policies(conn *iam.IAM, arn, name, pathPrefix string) ([]*iam.Policy, error

return results, err
}

// RoleByName returns a role's ARN given the role name
func RoleByName(conn *iam.IAM, name string) (*iam.Role, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can this just be named Role? This is just a wrapper on getRole, and the above RO functions are named simply by resource.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, love it


// roleNameSessionFromARN returns the role and session names in an ARN if any.
// Otherwise, it returns empty strings.
func roleNameSessionFromARN(rawARN string) (string, string) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This function would ideally return an error if there is no session name to grab (i.e. its not a valid sts assumed-role arn), right?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, the problem is that good Go says we should return an error if we don't plan on dealing with it, which we don't.

d.Set("source_arn", arn)
d.Set("session_name", "")
d.Set("role_name", "")
d.Set("role_path", "")
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think returning role_path is necessary. Seems like an arbitrary field to choose to return from GetRole output. Maybe we could leave it off and if folks want more role context for this datasource we can always add it later?

Copy link
Member Author

Choose a reason for hiding this comment

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

Removed.

if roleName, sessionName = roleNameSessionFromARN(arn); roleName == "" {
d.Set("source_arn", arn)
d.Set("session_name", "")
d.Set("role_name", "")
Copy link
Contributor

Choose a reason for hiding this comment

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

The field name structure for source_arn and role_name seems inconsistent. Could we name them both with source? Or we could even return arn, role_arn, role_name and session_name

Copy link
Member Author

Choose a reason for hiding this comment

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

the challenge here is if you're using an IAM user, you should still be able to get out a passed through user ARN from source_arn.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah I see what you are saying. Don't love it, but can't think of a better way to do it.

Copy link
Member Author

Choose a reason for hiding this comment

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

issuer_arn?

return "", ""
}

if reRole.MatchString(parsedARN.Resource) && parsedARN.Service != "iam" {
Copy link
Contributor

Choose a reason for hiding this comment

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

My take on this is we should spit IAM role ARNs out just like we would any non-sts assumed-role ARN. If they want metadata on a role, they should use the aws_iam_role data source.

Copy link
Member Author

Choose a reason for hiding this comment

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

Since we have the role name, I figure I'd also verify IAM role - no cost.

Copy link
Contributor

Choose a reason for hiding this comment

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

Its not costly, but I think it muddies the water a little on what this data source does. Its easiest to say if you pass in an assumed-role arn we'll give you back all of the metadata and if not you only get sourceArn. Once you add in a case for IAM role, I think the use case isn't quite as clear.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I can see that.

Copy link
Member Author

Choose a reason for hiding this comment

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

Water un muddied

@YakDriver YakDriver changed the title d/aws_iam_assumed_role_source: New data source d/aws_iam_session_context: New data source Jun 28, 2021
@@ -0,0 +1,3 @@
```release-note:new-data-source
aws_iam_assumed_role_source
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
aws_iam_assumed_role_source
aws_iam_session_context

Type: schema.TypeString,
Computed: true,
},
"source_arn": {
Copy link
Contributor

Choose a reason for hiding this comment

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

Swap with session_name for alphabetic ordering?

Copy link
Member Author

Choose a reason for hiding this comment

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

it's now called issuer_name and should be in the correct order

@ewbankkit ewbankkit removed the service/sts Issues and PRs that pertain to the sts service. label Jun 28, 2021
@YakDriver
Copy link
Member Author

Tests after many refactors:

GovCloud:

--- PASS: TestAccAWSDataSourceIAMSessionContext_notAssumedRoleUser (13.07s)
--- PASS: TestAccAWSDataSourceIAMSessionContext_notAssumedRole (17.41s)
--- PASS: TestAccAWSDataSourceIAMSessionContext_notAssumedRoleWithPath (17.42s)
--- PASS: TestAccAWSDataSourceIAMSessionContext_basic (19.85s)
--- PASS: TestAccAWSDataSourceIAMSessionContext_withPath (19.85s)
--- PASS: TestAssumedRoleRoleSessionName (0.00s)
    --- PASS: TestAssumedRoleRoleSessionName/not_an_ARN (0.00s)
    --- PASS: TestAssumedRoleRoleSessionName/regular_role_ARN (0.00s)
    --- PASS: TestAssumedRoleRoleSessionName/assumed_role_ARN (0.00s)
    --- PASS: TestAssumedRoleRoleSessionName/'assumed-role'_part_of_ARN_resource (0.00s)
    --- PASS: TestAssumedRoleRoleSessionName/user_ARN (0.00s)
    --- PASS: TestAssumedRoleRoleSessionName/assumed_role_from_AWS_example (0.00s)
    --- PASS: TestAssumedRoleRoleSessionName/multiple_slashes_in_resource (0.00s)
    --- PASS: TestAssumedRoleRoleSessionName/not_an_sts_ARN (0.00s)
    --- PASS: TestAssumedRoleRoleSessionName/role_with_path (0.00s)
    --- PASS: TestAssumedRoleRoleSessionName/wrong_service (0.00s)

us-west-2:

--- PASS: TestAccAWSDataSourceIAMSessionContext_notAssumedRoleUser (8.84s)
--- PASS: TestAccAWSDataSourceIAMSessionContext_notAssumedRole (11.30s)
--- PASS: TestAccAWSDataSourceIAMSessionContext_notAssumedRoleWithPath (11.30s)
--- PASS: TestAccAWSDataSourceIAMSessionContext_basic (11.50s)
--- PASS: TestAccAWSDataSourceIAMSessionContext_withPath (11.65s)
--- PASS: TestAssumedRoleRoleSessionName (0.00s)
    --- PASS: TestAssumedRoleRoleSessionName/not_an_ARN (0.00s)
    --- PASS: TestAssumedRoleRoleSessionName/regular_role_ARN (0.00s)
    --- PASS: TestAssumedRoleRoleSessionName/assumed_role_ARN (0.00s)
    --- PASS: TestAssumedRoleRoleSessionName/'assumed-role'_part_of_ARN_resource (0.00s)
    --- PASS: TestAssumedRoleRoleSessionName/user_ARN (0.00s)
    --- PASS: TestAssumedRoleRoleSessionName/assumed_role_from_AWS_example (0.00s)
    --- PASS: TestAssumedRoleRoleSessionName/multiple_slashes_in_resource (0.00s)
    --- PASS: TestAssumedRoleRoleSessionName/not_an_sts_ARN (0.00s)
    --- PASS: TestAssumedRoleRoleSessionName/role_with_path (0.00s)
    --- PASS: TestAssumedRoleRoleSessionName/wrong_service (0.00s)

@YakDriver YakDriver requested review from dpowley and ewbankkit June 28, 2021 19:14
@github-actions github-actions bot added the service/sts Issues and PRs that pertain to the sts service. label Jun 28, 2021
Copy link
Contributor

@ewbankkit ewbankkit left a comment

Choose a reason for hiding this comment

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

LGTM 🚀.

Commercial
% make testacc TEST=./aws TESTARGS='-run=TestAccAWSDataSourceIAMSessionContext_'        
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -count 1 -parallel 20 -run=TestAccAWSDataSourceIAMSessionContext_ -timeout 180m
=== RUN   TestAccAWSDataSourceIAMSessionContext_basic
=== PAUSE TestAccAWSDataSourceIAMSessionContext_basic
=== RUN   TestAccAWSDataSourceIAMSessionContext_withPath
=== PAUSE TestAccAWSDataSourceIAMSessionContext_withPath
=== RUN   TestAccAWSDataSourceIAMSessionContext_notAssumedRole
=== PAUSE TestAccAWSDataSourceIAMSessionContext_notAssumedRole
=== RUN   TestAccAWSDataSourceIAMSessionContext_notAssumedRoleWithPath
=== PAUSE TestAccAWSDataSourceIAMSessionContext_notAssumedRoleWithPath
=== RUN   TestAccAWSDataSourceIAMSessionContext_notAssumedRoleUser
=== PAUSE TestAccAWSDataSourceIAMSessionContext_notAssumedRoleUser
=== CONT  TestAccAWSDataSourceIAMSessionContext_basic
=== CONT  TestAccAWSDataSourceIAMSessionContext_notAssumedRoleWithPath
=== CONT  TestAccAWSDataSourceIAMSessionContext_notAssumedRole
=== CONT  TestAccAWSDataSourceIAMSessionContext_withPath
=== CONT  TestAccAWSDataSourceIAMSessionContext_notAssumedRoleUser
--- PASS: TestAccAWSDataSourceIAMSessionContext_notAssumedRoleUser (14.35s)
--- PASS: TestAccAWSDataSourceIAMSessionContext_notAssumedRoleWithPath (16.05s)
--- PASS: TestAccAWSDataSourceIAMSessionContext_notAssumedRole (17.17s)
--- PASS: TestAccAWSDataSourceIAMSessionContext_basic (17.21s)
--- PASS: TestAccAWSDataSourceIAMSessionContext_withPath (17.32s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	21.124s
GovCloud
% make testacc TEST=./aws TESTARGS='-run=TestAccAWSDataSourceIAMSessionContext_'  
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -count 1 -parallel 20 -run=TestAccAWSDataSourceIAMSessionContext_ -timeout 180m
=== RUN   TestAccAWSDataSourceIAMSessionContext_basic
=== PAUSE TestAccAWSDataSourceIAMSessionContext_basic
=== RUN   TestAccAWSDataSourceIAMSessionContext_withPath
=== PAUSE TestAccAWSDataSourceIAMSessionContext_withPath
=== RUN   TestAccAWSDataSourceIAMSessionContext_notAssumedRole
=== PAUSE TestAccAWSDataSourceIAMSessionContext_notAssumedRole
=== RUN   TestAccAWSDataSourceIAMSessionContext_notAssumedRoleWithPath
=== PAUSE TestAccAWSDataSourceIAMSessionContext_notAssumedRoleWithPath
=== RUN   TestAccAWSDataSourceIAMSessionContext_notAssumedRoleUser
=== PAUSE TestAccAWSDataSourceIAMSessionContext_notAssumedRoleUser
=== CONT  TestAccAWSDataSourceIAMSessionContext_basic
=== CONT  TestAccAWSDataSourceIAMSessionContext_notAssumedRoleWithPath
=== CONT  TestAccAWSDataSourceIAMSessionContext_notAssumedRoleUser
=== CONT  TestAccAWSDataSourceIAMSessionContext_notAssumedRole
=== CONT  TestAccAWSDataSourceIAMSessionContext_withPath
--- PASS: TestAccAWSDataSourceIAMSessionContext_notAssumedRoleUser (18.53s)
--- PASS: TestAccAWSDataSourceIAMSessionContext_notAssumedRole (23.04s)
--- PASS: TestAccAWSDataSourceIAMSessionContext_notAssumedRoleWithPath (23.36s)
--- PASS: TestAccAWSDataSourceIAMSessionContext_basic (23.51s)
--- PASS: TestAccAWSDataSourceIAMSessionContext_withPath (23.69s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	28.284s

@ewbankkit
Copy link
Contributor

Verified that one of our nightly CI tests that fails with an assumed role passes using

data "aws_caller_identity" "current" {}

data "aws_iam_session_context" "current" {
  arn = data.aws_caller_identity.current.arn
}
% make testacc TEST=./aws TESTARGS='-run=TestAccAWSVpcEndpointServiceAllowedPrincipal_'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -count 1 -parallel 20 -run=TestAccAWSVpcEndpointServiceAllowedPrincipal_ -timeout 180m
=== RUN   TestAccAWSVpcEndpointServiceAllowedPrincipal_basic
=== PAUSE TestAccAWSVpcEndpointServiceAllowedPrincipal_basic
=== CONT  TestAccAWSVpcEndpointServiceAllowedPrincipal_basic
    resource_aws_vpc_endpoint_service_allowed_principal_test.go:19: Step 1/1 error: Error running apply: exit status 1
        2021/06/28 14:59:22 [DEBUG] Using modified User-Agent: Terraform/0.12.31 HashiCorp-terraform-exec/0.13.3
        
        Error: Error creating VPC Endpoint Service allowed principal: InvalidPrincipal: Invalid Principal: 'arn:aws:sts::123456789012:assumed-role/terraform_team1_dev-developer/[email protected]'
        	status code: 400, request id: e1a5d963-8952-41dd-a2dd-aa120a0ee9d2
        
          on terraform_plugin_test.tf line 63, in resource "aws_vpc_endpoint_service_allowed_principal" "test":
          63: resource "aws_vpc_endpoint_service_allowed_principal" "test" {
        
        
--- FAIL: TestAccAWSVpcEndpointServiceAllowedPrincipal_basic (258.09s)
FAIL
FAIL	github.com/terraform-providers/terraform-provider-aws/aws	261.617s
FAIL
make: *** [testacc] Error 1
% make testacc TEST=./aws TESTARGS='-run=TestAccAWSVpcEndpointServiceAllowedPrincipal_'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -count 1 -parallel 20 -run=TestAccAWSVpcEndpointServiceAllowedPrincipal_ -timeout 180m
=== RUN   TestAccAWSVpcEndpointServiceAllowedPrincipal_basic
=== PAUSE TestAccAWSVpcEndpointServiceAllowedPrincipal_basic
=== CONT  TestAccAWSVpcEndpointServiceAllowedPrincipal_basic
--- PASS: TestAccAWSVpcEndpointServiceAllowedPrincipal_basic (227.45s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	230.947s

@YakDriver YakDriver added this to the v3.48.0 milestone Jun 28, 2021
@YakDriver YakDriver merged commit 6c348dd into main Jun 28, 2021
@YakDriver YakDriver deleted the f-caller-id-base-role branch June 28, 2021 20:24
github-actions bot pushed a commit that referenced this pull request Jun 28, 2021
@github-actions
Copy link

github-actions bot commented Jul 8, 2021

This functionality has been released in v3.48.0 of the Terraform AWS Provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading.

For further feature requests or bug reports with this functionality, please create a new GitHub issue following the template. Thank you!

@github-actions
Copy link

github-actions bot commented Aug 8, 2021

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
documentation Introduces or discusses updates to documentation. provider Pertains to the provider itself, rather than any interaction with AWS. service/iam Issues and PRs that pertain to the iam service. service/sts Issues and PRs that pertain to the sts service. size/XL Managed by automation to categorize the size of a PR. tests PRs: expanded test coverage. Issues: expanded coverage, enhancements to test infrastructure.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Error: Some AWS operations do not support assumed roles
4 participants