Foreman » Installer

Are you sure this works? It looks like it disables omapi unless a key and secret are set. This is something that the proxy actually needs to make modifications. To verify, you should create/modify/delete a DHCP reservation via the Smart Proxy API.

It also breaks on Debian where we don't set ACLs and Smart Proxy needs to read the config files to determine the hardcoded leases.

I also don't like the modification of file permissions from another module because it feels very unreliable. This may make a cherry pick easier so I'd be OK with that just for the picks. For upstream we should really modify puppet-dhcp as well. For example, expose the directory mode. Then I'd also be OK with a breaking change that makes it secure by default.

Going forward I'd suggest we write up a minimal verification script that checks DHCP works. Currently we have no such verification.

Setting the mode on /etc/dhcp/dhcpd.conf to 640 breaks things because we don't set an ACL in that - only the directory itself. This can be seen in the Smart Proxy log:

Jul 28 10:43:09 centos7-katello-nightly.wisse.example.com smart-proxy[7201]: /usr/share/foreman-proxy/modules/dhcp_isc/isc_state_changes_observer.rb:170:in `read': Permission denied @ rb_sysopen - /etc/dhcp/dhcpd.conf (Errno::EACCES)

You can then see that the API returns no subnets anymore:

# curl -s --cert /etc/foreman-proxy/foreman_ssl_cert.pem --key /etc/foreman-proxy/foreman_ssl_key.pem https://$HOSTNAME:9090/dhcp | jq
[]

Compare this to what it returned prior to the change:

# curl -s --cert /etc/foreman-proxy/foreman_ssl_cert.pem --key /etc/foreman-proxy/foreman_ssl_key.pem https://$HOSTNAME:9090/dhcp | jq
[
  {
    "network": "192.168.122.0",
    "netmask": "255.255.255.0",
    "options": {}
  }
]

And for the record, to create a reservation, you can use this:

curl -s --cert /etc/foreman-proxy/foreman_ssl_cert.pem --key /etc/foreman-proxy/foreman_ssl_key.pem https://$HOSTNAME:9090/dhcp/192.168.122.0 -X POST -d ip=192.168.122.100 -d mac=00:00:00:ff:ff:ff -d hostname=host.example.com

Then list it:

# curl -s --cert /etc/foreman-proxy/foreman_ssl_cert.pem --key /etc/foreman-proxy/foreman_ssl_key.pem https://$HOSTNAME:9090/dhcp/192.168.122.0 | jq

See https://projects.theforeman.org/projects/smart-proxy/wiki/API as well.

Also note that by default the DHCP package sets the mode of /etc/dhcp/dhcpd.conf to 644 so that should be safe - the directory is sufficient.

What should also be done is to create an omapi key by default, otherwise you can still access port 7911 without auth and setting the directory mode is useless.

You are right, not enabling the omapi at all is stupid.

I do not believe enforcing the key should be part of this threat though.

If you don't set a key, you may have other means to secure the port from unauthorized access. You haven't sacured it by choice.
This is about the key exposure, so about the situations, when you consider the port to be your weakpoint and you want to secure it.

I agree this should be just for cherry-picks and once we lift the embargo, I'm going to send a PR to the puppet-dhcp.

I have pretty much no experience with overriding attributes this way so I don't know how reliable it is. Normally my approach would be to add parameters to puppet-dhcp (which is another module we do control). It makes for a more complicated cherry pick (since you need to patch two modules), but it is more straight forward in reasoning about the code. There is also no chance of a mismatch in filenames, though that could also be mitigated by using $dhcp::dhcp_dir instead of hardcoding /etc/dhcp.

Also wondering if applying ACLs on Debian is better than setting the directory group (note you missed a $ in the patch). The reason we set ACLs on Red Hat is that the DHCP package enforces a directory mode of 0750 so a yum update reverted the installer changes. I'd check for similar behavior on Debian.

Well, we maintain both modules. It might be good to have two versions: the simple to cherry pick and the proper upstream fix.

Solution should be https://github.com/theforeman/puppet-dhcp/pull/177
and once accepted use this to do what patch 0004 is doing through those parameters.