Page MenuHomePhabricator

Refactor P:base::firewall to pull host directly from puppetdb
Open, LowPublic

Description

Currently profile::base::firewall pulls in a bunch of static IPv4/6 lists for various global hosts so that we can set up firewall rules e.g.

hieradata/common.yaml
bastion_hosts:
- 208.80.155.110                # bast1003.wikimedia.org
- 2620:0:861:4:208:80:155:110   # bast1003.wikimedia.org
- 208.80.153.54                 # bast2002.wikimedia.org
- 2620:0:860:2:208:80:153:54    # bast2002.wikimedia.org
- 91.198.174.6                  # bast3005.wikimedia.org
- 2620:0:862:1:91:198:174:6     # bast3005.wikimedia.org
- 198.35.26.13                  # bast4003.wikimedia.org
- 2620:0:863:1:198:35:26:13     # bast4003.wikimedia.org
- 103.102.166.6                 # bast5002.wikimedia.org
- 2001:df2:e500:1:103:102:166:6 # bast5002.wikimedia.org
- 185.15.58.6                   # bast6001.wikimedia.org
- 2a02:ec80:600:1:185:15:58:6   # bast6001.wikimedia.org
modules/profile/manifests/base/firewall.pp
class profile::base::firewall (
  Array[Stdlib::IP::Address] $bastion_hosts = lookup('bastion_hosts', {default_value => []}),
) {
  class { 'base::firewall':
    bastion_hosts => $bastion_hosts
  }
}
modules/base/manifests/firewall.pp
class base::firewall (
  Array[Stdlib::IP::Address] $bastion_hosts = [],
) {
      $bastion_hosts_str = join($bastion_hosts, ' ')
      ferm::rule { 'bastion-ssh':
          rule   => "proto tcp dport ssh saddr (${bastion_hosts_str}) ACCEPT;",
      }  
}

Proposal 1

As a first proposal i wonder if we should change theses rules so that they instead use the hostname and ferms @resolve function, which since T153468 will add both ipv4 and ipv6 addresses if they exist in dns. This change would look as follows

hieradata/common.yaml
bastion_hosts:
- bast1003.wikimedia.org
- bast2002.wikimedia.org
- bast3005.wikimedia.org
- bast4003.wikimedia.org
- bast5002.wikimedia.org
- bast6001.wikimedia.org
modules/profile/manifests/base/firewall.pp
class profile::base::firewall (
  Array[Stdlib::Fqdn] $bastion_hosts = lookup('bastion_hosts', {default_value => []}),
) {
  class { 'base::firewall':
    bastion_hosts => $bastion_hosts
  }
}
modules/base/manifests/firewall.pp
class base::firewall (
  Array[Stdlib::Fqdn] $bastion_hosts = [],
) {
      ferm::rule { 'bastion-ssh':
          rule   => "proto tcp dport ssh saddr @resolve((${bastion_hosts.join( ' ')})) ACCEPT;",
      }  
}
Advantage

The main advantage of this is that its easier to parse a hostname then an IP addresses, further it means we can garuntee that both ipv4 and ipv6 addresses have parity (T84916) and it prevents the chance of a typo in either of the addresses

Disadvantage

The main disadvantage here is that the firewall rules will depend on DNS and if ferm is unable to resolve this host name for what ever reason then a rule will not be added for this host. whoever with T166653 we shuoldn't hit this very much and if we do puppet should fix things on the next run.

Proposal 2

With the second proposal we drop the access lists in ferm all together and instead rely on puppetdb (wmflib::role_host) to inform us of the host

modules/profile/manifests/base/firewall.pp
class profile::base::firewall (
   class { 'base::firewall':
    bastion_hosts => wmflib::role_hosts('bastionhost')
  }
}
modules/base/manifests/firewall.pp
class base::firewall (
  Array[Stdlib::Fqdn] $bastion_hosts = [],
) {
      ferm::rule { 'bastion-ssh':
          rule   => "proto tcp dport ssh saddr @resolve((${bastion_hosts.join( ' ')})) ACCEPT;",
      }  
}
Advantage

The main advantage of this is that we no longer need to maintain manual lists of these global hosts and we can rely on puppet adding them and removing them as our infrastructre changes

Disadvantage

The main disadvantage here is also the fact that we pull the information from puppetdb. Using puppetdb add some additional complexity and moving parts to adding firewall rules. specificity if we where to add a new bastion_host then we would first need to ensure the new host has the 'bastionhost' role (i.e. not system::spare) and run puppet on the new bastion host before its ip address is known to pupetdb and other hosts can add them to there rules.

Another minor disadvantage is this would not work with cloud as they do not have puppetdb however it would be fairly trivial to support both use cases in the profile with e.g.

modules/profile/manifests/base/firewall.pp
class profile::base::firewall (
  Optional[Array[Stdlib::Fqdn]] $bastion_hosts = lookup('bastion_hosts', {default_value => undef}),
) {
  class { 'base::firewall':
    # im not the biggest fan of lest so we could use some other syntax but you get the idea
    bastion_hosts => $bastion_hosts.lest || { wmflib::role_hosts('bastionhost') }
  }
}