Add VPC endpoints #93

jsf9k · 2021-01-21T19:17:04Z

🗣 Description

This pull request:

Adds several VPC endpoints for various AWS services
Configures the operations and private subnets to make use of said VPC endpoints
Shares a single VPC route table among the private subnets. Since the private subnets do not each use a different NAT gateway in a different operations subnet, there is no need for them to have separate VPC route tables.

(It would be nice to remove the NAT gateway from the operations subnet, since the private subnets no longer require it to communicate with AWS services, but they still require it to pull the Docker images from Docker Hub for the Guacamole Docker composition.)

💭 Motivation and context

Resolves #94.

🧪 Testing

I deployed these changes to env0 in our staging COOL environment and verified that the cloud-init scripts, CloudWatch agent, and SSM agent are all running without errors on all private and operations instances. I also verified that the FreeIPA initialization for the guacamole instance still works.

I also redeployed with the external route removed from the operations subnet's route table and verified that everything still worked, with the exception of the downloading of the Docker images required for the Guacamole Docker composition. This serves as an ironclad check that all AWS API calls are being made to the VPC endpoints and none are being made to the public endpoints.

✅ Checklist

This PR has an informative and human-readable title.
Changes are limited to a single goal - eschew scope creep!
All future TODOs are captured in issues, which are referenced in code comments.
All relevant type-of-change labels have been added.
I have read the CONTRIBUTING document.
These code changes follow cisagov code standards.
All new and existing tests pass.

Now that we are no longer using a separate NAT gateway for each private subnet, the private subnets' route tables are all the same.

…TS and SSM services

* Remove egress via 443 to 0.0.0.0/0 * Allow egress via 443 to the STS and SSM security groups * Allow egress via 443 to the S3 gateway endpoint

These are necessary for the CloudWatch Agent to work. This agent is responsible for forwarding our system logs to CloudWatch.

This is necessary because adding the VPC endpoints to the VPCs makes the VPC-internal DNS for sts.us-east-1.awsamazon.com, for example, resolve to the internal IP of the VPC endpoint instead of a public AWS IP.

The operations subnet could still access S3 via the internet gateway, but it may as well use the gateway endpoint since it has to use the interface endpoints for all the other AWS services. Also rename some route-related Terraform resources to avoid collision between operations resources and private resources.

This reverts commit 6b091f0. I realized that the guacamole instance still needs the NAT gateway and external web access in order to download the Docker images used in the guacamole docker composition.

This is necessary so that the guacamole instance can download the Docker images required for the guacamole Docker composition.

dav3r · 2021-01-21T21:47:51Z

Removes the NAT gateway from the operations subnet, since the private subnets no longer requires it to communicate with AWS services.

The description of this PR should be amended to remove this bullet, due to 597a541.

Also, doesn't this mean that a16b36a needs to be reverted as well?

jsf9k · 2021-01-22T03:24:06Z

Removes the NAT gateway from the operations subnet, since the private subnets no longer requires it to communicate with AWS services.

The description of this PR should be amended to remove this bullet, due to 597a541.

Also, doesn't this mean that a16b36a needs to be reverted as well?

I amended the PR description.

The commit a16b36a does not need to be reverted. As it turns out, the assessment VPCs were never set up like the shared services VPC with N public and N private subnets spread across AZs. In that case we have each private subnet route its external traffic to the public subnet in the same AZ, so it makes sense to have separate route tables for each private subnet. In this case the idea of separate route tables was implemented but not the more important idea of cross-AZ redundancy.

Sharp eyes!

…S calls STS used to be un-regioned, like S3, but now it is regioned. This is the one case where boto3 _does not_ do the right thing when you set the region. We have to set the region-specific endpint URL manually. This is important since the STS VPC endpoint _only_ sets a local DNS record to override the _local region's_ public STS endpoint. If we don't do this then boto3 will reach out to the _global_ https://sts.amazonaws.com URL, and that DNS entry will still point to an external IP. See this boto/boto3#1859 for more information about boto3's perverse behavior in the case of STS.

…STS calls STS used to be un-regioned, like S3, but now it is regioned. The AWS CLI by default uses the global endpoint URL https://sts.amazonaws.com instead of the region-specific one, even when you specify the region. Therefore, we have to set the region-specific endpint URL manually. This is important since the STS VPC endpoint _only_ sets a local DNS record to override the _local region's_ public STS endpoint. If we don't specify the endpoint URL then the AWS CLI will reach out to the _global_ https://sts.amazonaws.com URL, and that DNS entry will still point to an external IP.

sts_endpoint_sg_rules.tf

hillaryj · 2021-01-22T15:12:27Z

vpc_endpoints.tf

+
+  # Normally we would use a for_each here, but the private subnets are
+  # _all in the same AZ_ in this case.  Why even have multiple subnets
+  # if you're not going to spread them around?  Harumph!


Harumph! https://www.youtube.com/watch?v=LmnaeU0k80I

Also this bit from "Blazing Saddles":

hillaryj

One suggestion which is only a comment change, and otherwise: 🏆

dav3r

Excellent work and really great job on the comments! 🐎

...except for one comment that needs a slight revision.

private_routing.tf

I noticed that previously we were using the name "aws_region" in the cloud-init templates, whereas I had used the name "region" in the code that I added. For consistency's sake, I changed the name in the code that I added.

Co-authored-by: Hillary <[email protected]>

Co-authored-by: dav3r <[email protected]>

felddy · 2021-01-22T16:16:19Z

private_acl_rules.tf

@@ -139,7 +139,7 @@ resource "aws_network_acl_rule" "private_egress_to_operations_via_vnc" {
  network_acl_id = aws_network_acl.private[each.value].id
  egress         = true
  protocol       = "tcp"
-  rule_number    = 120 + index(var.private_subnet_cidr_blocks, each.value)
+  rule_number    = 130 + index(var.private_subnet_cidr_blocks, each.value)


🧙 Seems magical. Is this counting by 10 jazz useful? Does this break if index ≥ 10?

Yes, it breaks if index >= 10. Our numbering of the ACL rules has always sucked. Do you have any thoughts as to how it can be improved?

subnets.tf

cloud-init/render-guac-connection-sql-template.py

felddy

Wonderful. This makes a lot of sense. 💯
I had a question about preloading the Docker images into the Guac AMI so that we can remove the NAT gateway. We should discuss that. 🐳 🥑

My motto was always to keep swinging. Whether I was in a slump or feeling badly or having trouble off the field, the only thing to do was keep swinging. --Hank Aaron ⚾

mcdonnnj

This is a pretty big lift and some strong work 💪💪💪
I had some suggestions/questions, but I was also curious if we have a standard on capitalization of protocols? We seem to use for example https and HTTPS without clear purpose.

mcdonnnj · 2021-01-22T16:22:02Z

cloud-init/install-certificates.py

+# https://sts.amazonaws.com URL, and that DNS entry will still point
+# to an external IP.
+#
+# See this link for more information about boto3's perverse behavior


boto3's perverse behavior

🤣

operations_acl_rules.tf

private_acl_rules.tf

vpc_endpoints.tf

desktop_gateway_sg_rules.tf

Co-authored-by: Nick M. <[email protected]>

jsf9k · 2021-01-26T14:54:09Z

I was also curious if we have a standard on capitalization of protocols? We seem to use for example https and HTTPS without clear purpose.

We do not have such a standard, but you are welcome to propose one. Capitalization is probably the way to go.

Co-authored-by: Nick M <[email protected]>

mcdonnnj · 2021-01-26T16:46:03Z

I was also curious if we have a standard on capitalization of protocols? We seem to use for example https and HTTPS without clear purpose.

We do not have such a standard, but you are welcome to propose one. Capitalization is probably the way to go.

If I'm proposing, then yes, I would vote for defaulting to capitalization for protocols.

jsf9k · 2021-01-26T17:10:26Z

I was also curious if we have a standard on capitalization of protocols? We seem to use for example https and HTTPS without clear purpose.

We do not have such a standard, but you are welcome to propose one. Capitalization is probably the way to go.

If I'm proposing, then yes, I would vote for defaulting to capitalization for protocols.

You could add something to this long-running PR.

Co-authored-by: Nick M <[email protected]>

…re-commit_version Update the `ansible-lint` Version in the pre-commit Configuration

jsf9k added 10 commits January 21, 2021 09:28

Remove NAT gateway and associated route

6b091f0

Use a single route table for all private subnets

a16b36a

Now that we are no longer using a separate NAT gateway for each private subnet, the private subnets' route tables are all the same.

Add permissions for creating, modifying, and deleting VPC endpoints

da4cf75

Add security groups for instances using VPC endpoints to access the S…

6e68e6d

…TS and SSM services

Add VPC endpoints requires to use the STS, SSM, and S3 services

b1e39fa

Add a route for S3 access via the S3 VPC gateway endpoint

1464d41

Modify desktop gateway SG rules to use VPC endpoints

1941acd

* Remove egress via 443 to 0.0.0.0/0 * Allow egress via 443 to the STS and SSM security groups * Allow egress via 443 to the S3 gateway endpoint

Add some CloudWatch VPC interface endpoints

f108ee6

These are necessary for the CloudWatch Agent to work. This agent is responsible for forwarding our system logs to CloudWatch.

Allow operations subnet to use the VPC endpoints in private subnet

62be561

This is necessary because adding the VPC endpoints to the VPCs makes the VPC-internal DNS for sts.us-east-1.awsamazon.com, for example, resolve to the internal IP of the VPC endpoint instead of a public AWS IP.

jsf9k added breaking change This issue or pull request involves changes to existing functionality improvement This issue or pull request will add or improve functionality, maintainability, or ease of use labels Jan 21, 2021

jsf9k self-assigned this Jan 21, 2021

jsf9k requested a review from dav3r January 21, 2021 19:17

jsf9k added 2 commits January 21, 2021 14:33

Revert "Remove NAT gateway and associated route"

597a541

This reverts commit 6b091f0. I realized that the guacamole instance still needs the NAT gateway and external web access in order to download the Docker images used in the guacamole docker composition.

Add SG and ACL rules to allow guacamole to use the NAT GW for HTTPS

a8ec8bb

This is necessary so that the guacamole instance can download the Docker images required for the guacamole Docker composition.

jsf9k marked this pull request as ready for review January 21, 2021 21:29

jsf9k requested a review from felddy as a code owner January 21, 2021 21:29

jsf9k requested review from hillaryj and mcdonnnj January 21, 2021 21:29

jsf9k added 4 commits January 21, 2021 22:43

Improve some ACL rule comments

7095452

Allow the operations subnet to use the STS VPC endpoint

24d3c09

hillaryj reviewed Jan 22, 2021

View reviewed changes

sts_endpoint_sg_rules.tf Outdated Show resolved Hide resolved

hillaryj reviewed Jan 22, 2021

View reviewed changes

hillaryj approved these changes Jan 22, 2021

View reviewed changes

dav3r approved these changes Jan 22, 2021

View reviewed changes

private_routing.tf Outdated Show resolved Hide resolved

jsf9k and others added 3 commits January 22, 2021 10:27

Consistently name AWS region variable

93fa57e

I noticed that previously we were using the name "aws_region" in the cloud-init templates, whereas I had used the name "region" in the code that I added. For consistency's sake, I changed the name in the code that I added.

Add issue link to a comment

5c087ea

Co-authored-by: Hillary <[email protected]>

Update comment to agree with reality

5a40a3a

Co-authored-by: dav3r <[email protected]>

felddy reviewed Jan 22, 2021

View reviewed changes

subnets.tf Outdated Show resolved Hide resolved

felddy reviewed Jan 22, 2021

View reviewed changes

cloud-init/render-guac-connection-sql-template.py Show resolved Hide resolved

felddy approved these changes Jan 22, 2021

View reviewed changes

mcdonnnj approved these changes Jan 26, 2021

View reviewed changes

Capitalize "Guacamole" and "Docker" in comments

02d26ca

Co-authored-by: Nick M. <[email protected]>

Add comments to clearly separate VPC interface and gateway endpoints

2d42f57

Co-authored-by: Nick M <[email protected]>

jsf9k requested a review from mcdonnnj January 26, 2021 15:02

jsf9k and others added 2 commits January 26, 2021 12:20

Clean up comment formatting

9ad872d

Reword comment for consistency with similar comments made elsewhere

d9efdca

Co-authored-by: Nick M <[email protected]>

jsf9k merged commit 08cf22e into develop Jan 26, 2021

jsf9k deleted the improvement/vpc-endpoints branch January 26, 2021 17:35

hillaryj mentioned this pull request Jan 26, 2021

Add style guide and improve documentation cisagov/development-guide#44

Merged

8 tasks

cisagovbot pushed a commit that referenced this pull request Jan 11, 2022

Merge pull request #93 from cisagov/improvement/update_ansible-lint_p…

14d2edb

…re-commit_version Update the `ansible-lint` Version in the pre-commit Configuration

mcdonnnj mentioned this pull request Jul 26, 2023

Implement VPC endpoints for AWS services cisagov/cyhy_amis#665

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add VPC endpoints #93

Add VPC endpoints #93

jsf9k commented Jan 21, 2021 •

edited

Loading

dav3r commented Jan 21, 2021 •

edited

Loading

jsf9k commented Jan 22, 2021

hillaryj Jan 22, 2021 •

edited

Loading

jsf9k Jan 22, 2021

hillaryj left a comment •

edited

Loading

dav3r left a comment

felddy Jan 22, 2021

jsf9k Jan 22, 2021

felddy left a comment

mcdonnnj left a comment

mcdonnnj Jan 22, 2021

jsf9k commented Jan 26, 2021 •

edited

Loading

mcdonnnj commented Jan 26, 2021

jsf9k commented Jan 26, 2021

Add VPC endpoints #93

Add VPC endpoints #93

Conversation

jsf9k commented Jan 21, 2021 • edited Loading

🗣 Description

💭 Motivation and context

🧪 Testing

✅ Checklist

dav3r commented Jan 21, 2021 • edited Loading

jsf9k commented Jan 22, 2021

hillaryj Jan 22, 2021 • edited Loading

Choose a reason for hiding this comment

jsf9k Jan 22, 2021

Choose a reason for hiding this comment

hillaryj left a comment • edited Loading

Choose a reason for hiding this comment

dav3r left a comment

Choose a reason for hiding this comment

felddy Jan 22, 2021

Choose a reason for hiding this comment

jsf9k Jan 22, 2021

Choose a reason for hiding this comment

felddy left a comment

Choose a reason for hiding this comment

mcdonnnj left a comment

Choose a reason for hiding this comment

mcdonnnj Jan 22, 2021

Choose a reason for hiding this comment

jsf9k commented Jan 26, 2021 • edited Loading

mcdonnnj commented Jan 26, 2021

jsf9k commented Jan 26, 2021

jsf9k commented Jan 21, 2021 •

edited

Loading

dav3r commented Jan 21, 2021 •

edited

Loading

hillaryj Jan 22, 2021 •

edited

Loading

hillaryj left a comment •

edited

Loading

jsf9k commented Jan 26, 2021 •

edited

Loading