Make Terraform configuration into a Terraform module

[clong]

POC is ready, but I'm going to have some folks who know Terraform a lot better than I sanity check my config and ensure I'm using best practices here.

https://twitter.com/Centurion/status/1044819879910678528

[brianebeyer]

Hey @clong, we may be interested in DetectionLab for some of our testing in AWS.

We’re exclusively a Terraform shop and happy to do the review if you’d like.

[clong]

@brianebeyer I know it's a couple months later, but I just submitted a config for folks who are able to import their VMs as AMI's - would love any feedback you have!

https://github.com/clong/DetectionLab/blob/master/Terraform/Method1/main.tf

[brianebeyer]

Awesome work @clong, this looks great! The biggest suggestion I have is making this a module that makes all this modular and reusable.

The great thing is that you've written things perfectly prepared to make it a module. Here's a quick PR I sketched up to show roughly what it looks like: https://github.com/brianebeyer/DetectionLab/pull/1

It'll be a few days until I can spin up a AWS account specifically to test this in - let me know if you'd like me to do that or if you want to run with that PR.

Other small comments:

---

Suggest making these a variable so users can configure multiple labs that each user a different CIDR https://github.com/clong/DetectionLab/blob/3157fa12e62972c545a283668ad4877260dc7e79/Terraform/Method1/main.tf#L34 https://github.com/clong/DetectionLab/blob/3157fa12e62972c545a283668ad4877260dc7e79/Terraform/Method1/main.tf#L52

It seems like it might be overkill to grant an entire /16 to each lab? Seems like we're generally not getting larger than 255 hosts, so maybe we grant each lab 192.168.X.0/24. Then we can run up to 255 labs in a single network.

---

Do these need to specify a root disk or is it automatically inheriting from the AMI? https://github.com/clong/DetectionLab/blob/master/Terraform/Method1/main.tf#L177

---

Finally, these are complete nits/style suggestions, but I'd recommend adding descriptions to all these variables so the user is guided on what they need to provide: https://github.com/clong/DetectionLab/blob/master/Terraform/Method1/main.tf#L5

That'll help clarify that shared_credentials_file is for AWS and key_name / public_key_path are for the AMIs.

[clong]

Thanks so much for the review @brianebeyer ! If you want to run with the PR, go for it! I'm also happy to share the AMIs with you if you want to shoot me over your account ID. I'm not familiar with Terraform modules, but I'll do some reading and see if I can catch on.

---

Do these need to specify a root disk or is it automatically inheriting from the AMI? https://github.com/clong/DetectionLab/blob/master/Terraform/Method1/main.tf#L177

Unfortunately inheriting from the AMI that was imported as an OVA. It may be possible to change that setting during import, but not sure where to do that quite yet. That option is added there to ensure the EBS volumes get removed during termination and don't waste $$ as the AMI keeps them by default.

---

It seems like it might be overkill to grant an entire /16 to each lab?

Good point. Will adjust that.

---

Finally, these are complete nits/style suggestions, but I'd recommend adding descriptions to all these variables so the user is guided on what they need to provide: https://github.com/clong/DetectionLab/blob/master/Terraform/Method1/main.tf#L5

So I do have comments in https://github.com/clong/DetectionLab/blob/master/Terraform/Method1/terraform.tfvars - is that a less standard way of having people fill out variables? Should I just move all that to main.tf?

[clong]

It seems like it might be overkill to grant an entire /16 to each lab?

Actually, now that I think about it, since all the hosts are hardcoded to IPs within 192.168.38.0/24, I don't think we can simply adjust that to a /24 without also adjusting the IP addresses of all the hosts as well.

Unfortunately a lot of IP addresses in the lab hosts are hardcoded for certain things, so converting the networking to be modular may not be that simple :(

I never considered the fact that folks might want to spin up multiple instances of the entire lab!

[brianebeyer]

That's ok- I just did some research and it DOES look possible to have multiple VPCs in the same AWS account share the same CIDR. So that means all your IPs can stay hardcoded, and each lab gets its own VPC.

You'd never be able to route between them, but that's fine.

I'll DM you our lab AWS account # so I can grab the AMIs and finish that PR.

[brianebeyer]

Regarding the comments, it's really just a style thing. The parallel to any other software language is that you might comment about defining a variable:

# Subnets listed here can access the lab over the internet
ip_whitelist_range = []

But you'd still definitely document the arguments passed to a method as part of the method doc:

def create_detection_labe(ip_whitelist_range)
    """Example function with types documented in the docstring.

    Args:
        ip_whitelist_range (array[string]): Subnets listed here can access the lab over the internet.
    """

In Terraform, the variable definitions are the the equivalent of "describing the argument to a module"

[cappetta]

Hey @clong @brianebeyer - I'm reading through and have an interest in knowing more about the networking configs and how to update. I updated the cidr to a 10.0.1.x range and think it hosed the AMI import when trying to blend this into secdevops-cuse/CyberRange project.

I made a slight adjustment to a few powershell IPs, built locally, exported as ova and I'm seeing a

FirstBootFailure: This import request failed because the instance failed to boot and establish network connectivity error on the image import process.

Not trying to hi-jack the thread but I thought it might be related to the IP change I made before the local build given this project works as it currently exists in the us-west-1 region.

If you think its possibly related to the IP / networking piece then share some thoughts pls to guide how to update so I can blend the 2 projects together.

Currently in us-east-1 as well so I can't easily copy the AMI's.

BTW - Combine the dynamic aws asset building w/ AWS-nuke... nice combo just be very careful and triple check your whitelisted assets.

[clong]

Much of the lab depends on hardcoded IP addresses (do a search for 192.168.38 in the Vagrant folder). This is certainly possible to work around, but that's likely going to be one of the primary blockers here.

[clong]

Moving this to the wishlist for now: https://github.com/clong/DetectionLab/wiki/Wishlist