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

Publish postfix (Docker) ports to standard mail ports on host #39

Merged
merged 7 commits into from
Apr 26, 2021

Conversation

dav3r
Copy link
Member

@dav3r dav3r commented Apr 22, 2021

🗣 Description

This PR publishes the following ports from the postfix Docker container to the host:

  • Internal 25 -> external 25 (was previously 1025)
  • Internal 587 -> external 587 (was previously 1587)
  • Internal 993 -> external 993 (was previously not published to the host)

💭 Motivation and context

From #37:

We want this instance to be able to send and receive mail on official SMTP ports (25 and 587).
Also, we want to expose port 993 (IMAPS) so that the Dockerized dovecot IMAP server can be accessed by a local mail client.

Note that this was not quite as simple as I hoped- please see 66d88b7 for details. If we end up having to make many more changes to the default docker-compose.yml file from cisagov/pca-gophish-composition, I think it will make sense to simply overwrite it completely with a file stored in this repository (rather than simply layering in changes via the docker-compose.production.yml file).

Resolves #37 and is part of the work required for cisagov/cool-system#90.

🧪 Testing

I tested the Ansible change by temporarily adding it in to a cisagov/ansible-role-pca-gophish-composition playbook and then running molecule converge and molecule login -h ... to ensure that it functioned as expected (it does).

I also tested this by modifying the docker-compose.yml and docker-compose.production.yml files on an existing Gophish instance to verify that the expected ports were correctly published:

  1. Modified docker compose files
  2. Restarted service (sudo systemctl restart pca-gophish-composition)
  3. Confirmed correct ports were published by Docker (sudo netstat -ltnp)

Before I merge this PR, I plan to create a fresh AMI in Staging and confirm that all is well (hence the blocked label).

✅ 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.
  • Build a Staging AMI and verify that
    • Executing sudo netstat -ltnp on the instance shows that ports 25, 587, and 993 are bound to a docker-proxy process
    • Mail can be sent/received via ports 25 or 587
    • The IMAPS server can be accessed via localhost:993

dav3r added 2 commits April 22, 2021 09:41
I was hoping to override the default published postfix ports for target 
(internal container) ports 25 and 587 by including our preferred 
published ports in docker-compose.production.yml. However, when Docker 
Compose merges the contents of the default docker-compose.yml with the 
contents of docker-compose.production.yml, it does not intelligently 
overwrite the published ports.  For example, instead of having target 
port 25 published to only host port 25 (as specified in 
docker-compose.production.yml), we end up with target port 25 published 
to BOTH host port 25 (from docker-compose.production.yml) and host port 
1025 (from docker-compose.yml).

That is why we remove the default published ports from 
docker-compose.yml so that only the ports specified in 
docker-compose.production.yml will be published.
@dav3r dav3r added the improvement This issue or pull request will add or improve functionality, maintainability, or ease of use label Apr 22, 2021
@dav3r dav3r self-assigned this Apr 22, 2021
@dav3r dav3r requested review from felddy and jsf9k as code owners April 22, 2021 18:33
@dav3r dav3r requested review from hillaryj and mcdonnnj April 22, 2021 18:33
@dav3r dav3r mentioned this pull request Apr 22, 2021
6 tasks
@dav3r dav3r added the blocked This issue or pull request is awaiting the outcome of another issue or pull request label Apr 22, 2021
Comment on lines +65 to +76
- name: Remove default postfix published ports
ansible.builtin.replace:
path: "{{ pca_gophish_composition_path }}/docker-compose.yml"
regexp: "^\\s*ports:\\s*$\n\
^\\s*-\\s*target:\\s*['\"]25['\"]\\s*$\n\
^\\s*published:\\s*['\"]1025['\"]\\s*$\n\
^\\s*protocol:\\s*tcp\\s*$\n\
^\\s*mode:\\s*host\\s*$\n\
^\\s*-\\s* target:\\s*['\"]587['\"]\\s*$\n\
^\\s*published:\\s*['\"]1587['\"]\\s*$\n\
^\\s*protocol:\\s*tcp\\s*$\n\
^\\s*mode:\\s*host\\s*$\n"
Copy link
Member

Choose a reason for hiding this comment

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

This is pretty gnarly. What do you think about creating an issue for figuring out a better way to do this?

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree that it's a nasty regex. I looked at ansible.builtin.blockinfile, but that would require us to put some marker lines around the text block that we want to modify, so I didn't love that solution.

If you think it's cleaner, I can switch to overwriting the entire docker-compose.yml file with one customized for this AMI, which would also mean that we could get rid of docker-compose.production.yml. I didn't go that way originally because it means that whenever cisagov/pca-gophish-composition/docker-compose.yml changes, we would also need to make similar updates in this repo, but maybe that's not so terrible compared to this regex. What do you say @jsf9k?

Copy link
Member

Choose a reason for hiding this comment

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

I thought about those options too, but I don't really like any of them. Ansible doesn't have a built-in module to manipulate YAML files (which is really what we need here), so I thought of these options:

  1. Convert the YAML to JSON, use ansible.builtin.command to run jq, then convert the JSON back to YAML.
  2. Install and use yq via ansible.builtin.command.
  3. Figure out a way to use the from_yaml, to_json, to_yaml, and json_query Ansible filters to achieve this.
  4. Use jsonpatch.

I'm fine with punting for now, so long as there is an issue to remind us to revisit this.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sounds good to me- I made issue #42 for this.

BTW, this may also be something that a future version of docker-compose can help us overcome:

Copy link
Member

Choose a reason for hiding this comment

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

This sort of thang might be an OK solution as well.

You'll want to add a (short) paragraph to the comment before this task, stating that the regex is ugly and mentioning the issue you just created. There is a line in the checklist in the PR template for this sort of thing that you will also need to re-add.

Copy link
Member Author

Choose a reason for hiding this comment

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

You'll want to add a (short) paragraph to the comment before this task, stating that the regex is ugly and mentioning the issue you just created. There is a line in the checklist in the PR template for this sort of thing that you will also need to re-add.

Good call - see a305491.

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.

I approve this, gnarly regex and all. Thanks for getting this functionality implemented 💪💪💪

@dav3r dav3r removed the blocked This issue or pull request is awaiting the outcome of another issue or pull request label Apr 26, 2021
@dav3r dav3r merged commit f588a66 into develop Apr 26, 2021
@dav3r dav3r deleted the improvement/publish-standard-mail-ports branch April 26, 2021 21:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
improvement This issue or pull request will add or improve functionality, maintainability, or ease of use
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Publish Dockerized postfix ports to host ports 25, 587, and 993
3 participants