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.

Sign up for GitHub

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

Jump to bottom

Support multiple email-sending domains #153

Merged
merged 12 commits into from
Dec 1, 2021
Merged

Support multiple email-sending domains #153

merged 12 commits into from
Dec 1, 2021

Conversation

dav3r
Copy link
Member

@dav3r dav3r commented Nov 30, 2021

🗣 Description

This PR enables support for multiple email-sending domains by Gophish and Teamserver instances in an assessment environment.

💭 Motivation and context

Assessment teams require the ability to have multiple Teamservers deployed in a single assessment environment, each configured with their own email-sending domain. This is useful if one domain can no longer be feasibly used (e.g. it gets blocked), then the team can quickly switch over to using a different domain.

Both Teamservers and Gophish instances follow the same logic for utilizing domains:

Instances are deployed with each sequential domain in the provided list of email-sending domains, so teamserver0 and gophish0 will get the first domain, teamserver1 and gophish1 will get the second domain, and so on. If there are more Teamserver or Gophish instances than email-sending domains, the domains in the list will be reused in a wrap-around fashion. For example, if there are three Teamservers deployed and only two email-sending domains, teamserver0 will get the first domain, teamserver1 will get the second domain, and teamserver2 will wrap-around back to using the first domain.

Resolves #127 and resolves cisagov/cool-system-internal#42.

🧪 Testing

I tested these changes by deploying a varying number of Gophish and Teamserver instances, along with a varying number of email-sending domains, then confirming that the expected domain was applied to each instance.

✅ Checklist

  • This PR has an informative and human-readable title.
  • Changes are limited to a single goal - eschew scope creep!
  • All relevant type-of-change labels have been added.
  • I have read the CONTRIBUTING document.
  • These code changes follow cisagov code standards.
  • All relevant repo and/or project documentation has been updated
    to reflect the changes in this PR.
  • All new and existing tests pass.

dav3r added 8 commits November 30, 2021 17:35
@dav3r
Change email_sending_domain variable from a string to a list
f383ca3 
This allows us to support multiple email-sending domains in an assessment environment.
@dav3r
Create a certificate reading role for each email-sending domain
cdc7332
@dav3r
Update cloud-init and IAM resources to handle multiple email-sending …
0fda172 
…domains

This change only applies to Gophish and Teamserver instances.
@dav3r
Use an index to select the correct Gophish instance profile
502018f 
Previously, there was only one instance profile for all Gophish instances, but now that instances may be assigned different email-sending domains, we need a different instance profile for each Gophish instance.
@dav3r
Use an index to select the correct Teamserver instance profile and us…
385851c 
…er data

Previously, there was only one instance profile and user-data for all 
Teamserver instances, but now that instances may be assigned different 
email-sending domains, we need a different instance profile and 
user-data for each Teamserver instance.
@dav3r
Update output descriptions to reflect recent changes
ebc5aed
@dav3r
Update example code to reflect an output change
87f217d 
The email_sending_domain_certreadrole output is now indexed to support 
multiple certificate read roles in an assessment environment.
@dav3r
Update README with latest output from terraform-docs
7fa1ac0
@dav3r dav3r added the improvement This issue or pull request will add or improve functionality, maintainability, or ease of use label Nov 30, 2021
@dav3r dav3r requested review from felddy and jsf9k as code owners November 30, 2021 23:11
@dav3r dav3r self-assigned this Nov 30, 2021
@jsf9k jsf9k requested a review from mcdonnnj December 1, 2021 03:05
jsf9k
jsf9k reviewed Dec 1, 2021
View reviewed changes
Copy link
Member

@jsf9k jsf9k left a comment

Choose a reason for hiding this comment

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

Please consider my comments regarding giving a few output variables names that reflect the plurality of the values they contain.

gophish_cloud_init.tf Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@dav3r dav3r requested a review from jsf9k December 1, 2021 03:36
jsf9k
jsf9k approved these changes Dec 1, 2021
View reviewed changes
Copy link
Member

@jsf9k jsf9k left a comment

Choose a reason for hiding this comment

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

Thanks for taking my comments into consideration @dav3r. Strong work here.

mcdonnnj
mcdonnnj approved these changes Dec 1, 2021
View reviewed changes
Copy link
Member

@mcdonnnj mcdonnnj left a comment

Choose a reason for hiding this comment

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

Strong work 💪💪💪. I would like us to come to a consensus on how we name non-singular resources like roles and policies, and if it would result in a change I'd like an issue created to track the work (so it's not holding up this PR). I did think it was straightforward enough to update comments to represent the fact that things are now plural, so please see my suggestions for your consideration.

gophish_iam.tf Outdated Show resolved Hide resolved
gophish_iam.tf Outdated Show resolved Hide resolved
gophish_iam.tf Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
email_sending_domain_certreadrole.tf Outdated Show resolved Hide resolved
teamserver_iam.tf Outdated Show resolved Hide resolved
teamserver_iam.tf Outdated Show resolved Hide resolved
email_sending_domain_certreadrole.tf Outdated Show resolved Hide resolved
gophish_iam.tf Outdated Show resolved Hide resolved
teamserver_iam.tf Outdated Show resolved Hide resolved
@dav3r dav3r mentioned this pull request Dec 1, 2021
Open
2 tasks
@dav3r
Copy link
Member Author

dav3r commented Dec 1, 2021

I would like us to come to a consensus on how we name non-singular resources like roles and policies, and if it would result in a change I'd like an issue created to track the work (so it's not holding up this PR).

Please take a look at cisagov/development-guide#63 - feel free to add/update/remove content in that issue as you see fit.

@dav3r dav3r merged commit 601d8c7 into develop Dec 1, 2021
@dav3r dav3r deleted the improvement/multiple-email-domains branch December 1, 2021 19:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mcdonnnj mcdonnnj mcdonnnj approved these changes

@jsf9k jsf9k jsf9k approved these changes

@felddy felddy Awaiting requested review from felddy felddy is a code owner

Assignees

@dav3r dav3r

Labels
improvement This issue or pull request will add or improve functionality, maintainability, or ease of use
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Support multiple email-sending domains per assessment
3 participants
@dav3r @jsf9k @mcdonnnj