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

feat: Add ability to pass custom IAM policy to VPC endpoints #497

Conversation

kostyaplis
Copy link

Description

In order to fix #437
Inspired by #341

Added ability to pass custom IAM policy as JSON string to any VPC endpoint that supports it atm

Motivation and Context

Default Full Access endpoints policy is a security concern. At least, accordingly to ISO27001

Breaking Changes

No breaking changes

Been tested with custom and default IAM policies.

@kostyaplis kostyaplis changed the title Feature. Add ability to pass custom IAM policy to VPC endpoints feat: Add ability to pass custom IAM policy to VPC endpoints Sep 9, 2020
default = ""
}

variable "dynamodb_endpoint_policy" {
Copy link
Member

Choose a reason for hiding this comment

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

I don't see DynamoDB on the list of services that support endpoint policies - do we have any supporting evidence that states otherwise? https://docs.aws.amazon.com/vpc/latest/userguide/integrated-services-vpce-list.html

default = ""
}

variable "ecr_dkr_endpoint_policy" {
Copy link
Member

Choose a reason for hiding this comment

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

likewise, I see ECR supported but I don't know if that includes the Docker registry API endpoint https://docs.aws.amazon.com/vpc/latest/userguide/integrated-services-vpce-list.html

@@ -1,3 +1,17 @@
data "aws_iam_policy_document" "default" {
Copy link
Member

Choose a reason for hiding this comment

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

we shouldn't need this, instead we can set the policy to null most likely. see below

@@ -12,6 +26,7 @@ resource "aws_vpc_endpoint" "s3" {

vpc_id = local.vpc_id
service_name = data.aws_vpc_endpoint_service.s3[0].service_name
policy = var.s3_endpoint_policy == "" ? data.aws_iam_policy_document.default.json : var.s3_endpoint_policy
Copy link
Member

Choose a reason for hiding this comment

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

I haven't tested myself yet, but I think something like this should work without requiring the default policy data source:

Suggested change
policy = var.s3_endpoint_policy == "" ? data.aws_iam_policy_document.default.json : var.s3_endpoint_policy
policy = var.s3_endpoint_policy == "" ? null : var.s3_endpoint_policy

one other thing that might work as well (again, haven't tested this here) would be:

  1. Change the policy default value to null like:
variable "s3_endpoint_policy" {
  description = "Custom IAM policy for S3 endpoint"
  type        = string
  default     = null
}
  1. Then simply do this:
  policy       = var.s3_endpoint_policy

again, it would need to be tested and validated on either approach but I think it might work and makes it cleaner. I don't know what the AWS provider or AWS API will provide when a null is provided (or no policy is provided)

default = ""
}

variable "workspaces_endpoint_policy" {
Copy link
Member

Choose a reason for hiding this comment

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

looks like workspaces endpoints do not support policies https://docs.aws.amazon.com/vpc/latest/userguide/integrated-services-vpce-list.html

default = ""
}

variable "access_analyzer_endpoint_policy" {
Copy link
Member

Choose a reason for hiding this comment

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

I don't see anything for IAM access analyzer on this list https://docs.aws.amazon.com/vpc/latest/userguide/integrated-services-vpce-list.html

@bryantbiggs
Copy link
Member

@kostyaplis could you update the complete-vpc for the changes you are proposing here please? this not only gives others an example on how to configure these modules, but it also is how we test the modules. I would encourage you to update the complete example and try running yourself to make sure everything provisions successfully

@kostyaplis
Copy link
Author

Hi @bryantbiggs, thanks for your review!

  1. Regarding VPC endpoints custom policies availability.
    Unfortunately, AWS documentation is known for some incompleteness and inaccuracies.
    So upon developing this PR I had to go VPC -> Endpoints -> Create Endpoint and manually check EACH endpoint for Full Access/Custom policies support.
    I was able to find documentation that mention endpoint policies for some of the endpoints you listed above. While for others I cannot find explicit docs, so you may check yourself (in VPC -> Endpoints -> Create Endpoint) or just trust my word:)

DynamoDB: https://docs.aws.amazon.com/vpc/latest/userguide/vpc-endpoints-ddb.html
ECR DKR: ???
Workspaces: https://docs.aws.amazon.com/workspaces/latest/adminguide/infrastructure-security.html#interface-vpc-endpoint
IAM Access Analyzer: ???

  1. Default aws_iam_policy_document is there for the reason.
    It seems to me that AWS provider treats null value for aws_vpc_endpoint.policy in some weird way.
    I've implemented and tested changes you suggested.
    TF 0.13.5
    AWS Provider 3.16.0
variable "s3_endpoint_policy" {
  description = "Custom IAM policy for S3 VPC endpoint"
  type        = string
  default     = null
}

resource "aws_vpc_endpoint" "s3" {
 ...
  policy = var.s3_endpoint_policy
 ...
}

Results:

  1. Apply module with enable_s3_endpoint = true - endpoint correctly created with default Full Access policy
  2. Declare variable s3_endpoint_policy as valid JSON string - endpoint policy correctly updated
  3. Set variable s3_endpoint_policy back to null - no changes generated by tf plan/apply. We'd expect it to revert the policy to its default value
  4. Modify s3_endpoint_policy in UI or CLI while variable s3_endpoint_policy = null. Run terraform plan/apply - no changes generated. We'd expect it to revert the policy to its default value

So that was the reason I put default endpoint policy there. Seems that if declared it has to be valid JSON string and completely skipped if set to null.

  1. Noted your request on example update. Will try to find some more time this weekend, will ping you once done.

@bodgit
Copy link

bodgit commented Jan 27, 2021

Just came looking through this module repository as I noticed I couldn't set an S3 endpoint policy so I'm keen to see this PR merged so I can use this module instead of our homegrown version. FWIW I've set a DKR endpoint policy.

@github-actions
Copy link

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 Oct 31, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

VPC Endpoint policy
3 participants