search

geerlingguy / ansible-role-firewall

Ansible Role - iptables Firewall configuration.
https://galaxy.ansible.com/geerlingguy/firewall/
MIT License
529 stars 219 forks source link

Add firewall_conditionally_allowed_ports #14

Closed hamish closed 4 years ago

hamish commented 8 years ago

Hi,

Thanks for your ansible roles. I'm using a lot of them.

This patch adds the ability to open ports only to particular hosts and subnets. I'm using this to restrict ssh access to only certain networks amongst other uses.

I'm very happy to take feedback on how to improve the patch.

Best Wishes

Hamish Currie

geerlingguy commented 8 years ago

@hamish - Thanks SO much for this PR. I think it does have a lot of usefulness, but I don't want to have to maintain the extra weight in tests and in templating to make sure that all the components of these conditionally allowed ports work.

Rather, I'd recommend users use firewall_additional_rules if they need to open a port to a specific host or range of hosts. For many of my database servers, munin reporting, etc., I'll add a rule or two in that list as needed, like:

firewall_additional_rules:
    - "iptables -A INPUT -p tcp --dport 4949 -s 1.2.3.4 -j ACCEPT"

This would open 4949 to IP 1.2.3.4 (for Munin).

Again, thanks for submitting the PR, and while I think this is a valid use case (and if the syntax helps you, I'd recommend forking the role and maintaining your version with this added feature), I don't want to maintain the feature in this role.

hamish commented 8 years ago

Hi @geerlingguy - I can understand your desire to limit your maintenance burden. You are looking after a lot of roles.

Regarding this patch, I particularly liked the ability to have an inventory group drive the rule - so that we don't need to specify the IP addresses of our hosts in multiple places.

Is there another way that I can achieve this - or is there a change I could make to make this patch more acceptable? One possibility that comes to mind is to remove the rule.sources branch - as that's quite easy to achieve with firewall_additional_rules and just keep the rule.ansible_hosts section. This removes the elif block and it's associated for loop. (I added the rule.sources feature because I read your comment about not forcing people to specify addresses like hostvars[host]['ansible_eth0']['ipv4']['address'][] = '12.34.56.78' in PR #1 and thought it would make the feature more likely to be accepted, so I'm happy to drop it.)

I'm willing to fork and maintain the role if needed, but I'd prefer to find a way that I can contribute and use your role directly.

thanks

Hamish

geerlingguy commented 8 years ago

I'll leave it open and think further on it... don't have time to dig in this week as I'm at DrupalCon.

stale[bot] commented 4 years ago

This pull request has been marked 'stale' due to lack of recent activity. If there is no further activity, the PR will be closed in another 30 days. Thank you for your contribution!

Please read this blog post to see the reasons why I mark pull requests as stale.

stale[bot] commented 4 years ago

This pull request has been closed due to inactivity. If you feel this is in error, please reopen the pull request or file a new PR with the relevant details.