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

Enhance DataAwsAssumeRolePolicy to allow the role to assume itself #3615

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

Conversation

unterstein
Copy link

Changes

AWS roles used to be implicitly able to assume itself. As of somewhen last year this is no longer the case, therefore whenever a role wants to assume itself, it has to be configured explicitly in the assume role policy. We are using the DataAwsAssumeRolePolicy in our terraform stacks for a lot of roles but recently learned that this might cause issues. Therefore we would like to add the capabilities to this terraform resource, that the self assumption is also possible.

When creating roles with terraform, it is a little tricky to do this self assumption. Since ARNs are predictable, you could think, that you just can predict the ARN of your -to-be-created-role and add it to the list of Allow-sts:AssumeRole directly. But AWS will perform a check and will fail if the ARN is not already present. Since we are just about to create the role with that ARN, this is not possible. However, going in and adding the concrete check for the role as Condition-ArnLike, we will postpone the check if the ARN is actually existing to the runtime. At this time the ARN is present and the check will work as expected.

We do this kind of IAM approach for role self assumptions at a lot of other places ever since AWS announced their policy change last year and we didn't had problems with it so far.

Tests

I ran make test and verified that this addition is not breaking existing behaviour/existing tests. I added another test and verified the output policy of my test:

        	            	{
        	            	  "Version": "2012-10-17",
        	            	  "Statement": [
        	            	    {
        	            	      "Effect": "Allow",
        	            	      "Action": "sts:AssumeRole",
        	            	      "Principal": {
        	            	        "AWS": "arn:aws:iam::414351767826:root"
        	            	      },
        	            	      "Condition": {
        	            	        "StringEquals": {
        	            	          "sts:ExternalId": "abc"
        	            	        }
        	            	      }
        	            	    },
        	            	    {
        	            	      "Sid": "ExplicitSelfRoleAssumption",
        	            	      "Effect": "Allow",
        	            	      "Action": "sts:AssumeRole",
        	            	      "Principal": {
        	            	        "AWS": "arn:aws:iam::aws-account:role/root"
        	            	      },
        	            	      "Condition": {
        	            	        "ArnLike": {
        	            	          "aws:PrincipalArn": "arn:aws:iam::aws-account:role/role-name"
        	            	        }
        	            	      }
        	            	    }
        	            	  ]
        	            	}
  • make test run locally
  • relevant change in docs/ folder
  • covered with integration tests in internal/acceptance
  • relevant acceptance tests are passing
  • using Go SDK

@unterstein unterstein requested review from a team as code owners May 24, 2024 11:02
@unterstein unterstein requested review from hectorcast-db and removed request for a team May 24, 2024 11:02
@nkvuong
Copy link
Contributor

nkvuong commented May 24, 2024

@unterstein thank you for getting around to this - do you mind updating the doc for the data source as well?

@codecov-commenter
Copy link

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 82.33%. Comparing base (fe9747a) to head (1980f46).
Report is 1 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #3615      +/-   ##
==========================================
+ Coverage   82.31%   82.33%   +0.02%     
==========================================
  Files         191      191              
  Lines       19329    19346      +17     
==========================================
+ Hits        15910    15929      +19     
+ Misses       2487     2486       -1     
+ Partials      932      931       -1     
Files Coverage Δ
aws/data_aws_assume_role_policy.go 86.95% <100.00%> (+7.64%) ⬆️

... and 1 file with indirect coverage changes

@unterstein
Copy link
Author

@nkvuong thank you for your feedback. Is the place I added the documentation the right one and is it ok that way? Please feel free to adjust the docs in this PR to your liking.

thank you 🙏

@nkvuong
Copy link
Contributor

nkvuong commented May 28, 2024

@unterstein I just checked out documentation, and the cross-account policy does not require support for self assumption. It is only the Unity Catalog IAM role that requires support for self assumption, but we currently do not have a data source for that

@unterstein
Copy link
Author

@nkvuong we were informed by our partner from databricks, that we need to make some of our roles self assuming. For all roles that were called out, we use this particular DataAwsAssumeRolePolicy to generate it's assume role policy. In order to fulfil the request to make those role able to assume themselves, we need to adjust this component.

@nkvuong
Copy link
Contributor

nkvuong commented Jun 4, 2024

@unterstein confusingly, there are 2 different trust policies necessary, one for workspace creation & one for Unity Catalog. I've raised a separate PR #3622 for the second type of trust policies

the IAM role for workspace creation does not require self-assumption, but the IAM role for Unity Catalog requires it

@unterstein
Copy link
Author

unterstein commented Jun 4, 2024

@nkvuong we don't use databricks provider components for our unity catalog role and we didn't got those roles flagged from databricks. Where as we got the workspaces (where we use this component) called out that we need to fix them.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants