terraform-provider-databricks icon indicating copy to clipboard operation
terraform-provider-databricks copied to clipboard

Enhance DataAwsAssumeRolePolicy to allow the role to assume itself

Open unterstein opened this issue 1 year ago • 7 comments

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"
        	            	        }
        	            	      }
        	            	    }
        	            	  ]
        	            	}
  • [x] 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 avatar May 24 '24 11:05 unterstein

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

nkvuong avatar May 24 '24 13:05 nkvuong

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

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%) :arrow_up:

... and 1 file with indirect coverage changes

codecov-commenter avatar May 24 '24 13:05 codecov-commenter

@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 🙏

unterstein avatar May 27 '24 10:05 unterstein

@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

nkvuong avatar May 28 '24 09:05 nkvuong

@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.

unterstein avatar Jun 04 '24 06:06 unterstein

@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

nkvuong avatar Jun 04 '24 09:06 nkvuong

@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.

unterstein avatar Jun 04 '24 12:06 unterstein