(DIO-563) addresses CVE-2020-7943

[suckatrash]

This PR addresses the changes to metrics endpoints required by CVE-2020-7943:

• Returns ['localhost'] in localhost_or_hosts_with_pe_profile() if puppet_version is greater than 6.14.0 9 or greater than 5.5.19 but below v6) when the function runs on the master. If not run on the master, an empty array is returned.
• Changes the default of puppetdb_host in puppet_metrics_dashboard::profile::puppetdb to localhost. This would be overridden when called from puppet_metrics_dashboard::telegraf::config when a list of puppetdb hosts is returned from localhost_or_hosts_with_pe_profile()
• Changes metrics URLs as appropriate
• Updates dashboards with new metric value and uses the host tag instead of server (so we don't see localhost on all the dashes).
• Adds test coverage for running on / not running on master
• Adds a tidy resource to cleanup unmanaged dashboards

[tkishel]

The changes themselves look good!
Any change you could replace doc refs to Puppetdb with PuppetDB?
Sorry, I can't help with Travis ...

[tkishel]

@suckatrash I assume we should wait for this to be merged before merging the release PR and publishing 2.2.0 and releasing the coupled puppet_metrics_collector 6.0.0?

[suckatrash]

@tkishel I don't want to hold anything up with this. I may not finish it today so I'd say go ahead with a release

[tkishel]

Well, we made the same endpoint change in puppet_metrics_collector 6.0.0.
I've restarted the tests as they don't seem to be failing because of this.

[suckatrash]

I think we might need to reconsider using pe_server_version since that won't work with opensource puppet.

[tkishel]

I think we might need to reconsider using pe_server_version since that won't work with opensource puppet.

Well, it cleared all the Travis errors :)

I use the Puppet version to detect PE version here:

https://github.com/tkishel/pe_tune/blob/master/lib/puppet_x/puppetlabs/tune.rb#L1050

So how about this:

  # Puppet 6.14 correlates to PE 2019.5
  if (versioncmp($facts['puppetversion'], '6.14.0') >= 1) and ($profile == 'puppetdb') {

[tkishel]

Yay!

[tkishel]

Note that it's possible that the agent's Puppet version differs from the Master (or that of the PuppetDB host). We could check $server_facts['serverversion'] but that will not be populated if/when declaring a class via puppet apply

[tkishel]

For PE 2018.1.13 ...

if ($profile == 'puppetdb') {
  # PE 2019.5 or newer with Puppet Server 6.9.2 or newer, or
  # PE 2018.1.13 or newer PE 2018 with Puppet Server 5.3.12 or newer
  # As per: https://puppet.com/docs/pe/latest/component_versions_in_recent_pe_releases.html
  if ((versioncmp($facts['puppetversion'], '6.14.0') >= 1) or
      (versioncmp($facts['puppetversion'], '5.5.19') >= 1) and (versioncmp($facts['puppetversion'], '6.0.0') == -1)) {
    notice ( 'In puppetserver version 6.9.2 or 5.3.12 and newer you must apply the puppet_metrics_dashboard::profile::puppetdb class to any puppetdb hosts. Metrics cannot be collected by telegraf from a remote agent.')
    $hosts = []
  } else {

  }     
}

[tkishel]

Travis is failing because there is no else block to the if ($profile == 'puppetdb') { statement.

We can easily resolve that, I don't think we should emit a notice as even if the user applies the puppet_metrics_dashboard::profile::puppetdb class to PuppetDB hosts, the notice will continue to be emitted during every run.

# @summary function used to determine hosts with a Puppet Enterprise profile
#
# Queries PuppetDB for hosts with the specified Puppet Enterprise profile.
# Used by this module to identify hosts with Puppet Enterprise API endpoints.
#
# @param profile [String]
#   The short name of the Puppet Enterprise profile to query.
#
# @return [Array[String]]
#   An array of certnames from the query, or the certname of the localhost when the query returns no hosts.

function puppet_metrics_dashboard::localhost_or_hosts_with_pe_profile(
  String $profile,
) >> Array {
  # storeconfigs is: https://puppet.com/docs/puppet/latest/configuration.html#storeconfigs
  if $settings::storeconfigs {
    $_profile = capitalize($profile)
    $hosts = puppetdb_query("resources[certname] {
      type = 'Class' and
      title = 'Puppet_enterprise::Profile::${_profile}' and
      nodes { deactivated is null and expired is null }
    }").map |$nodes| { $nodes['certname'] }

  } else {
    $hosts = []
  }

  if empty($hosts) {
    [$trusted['certname']]
  } else {
    # PuppetDB in PE 2019.5 or newer (or PE 2018.1.13 or newer PE 2018) do not allow remote access to metrics, as a result of CVE-2020-7943.
    # Mapping of Puppet to PE to PuppetDB versions is: https://puppet.com/docs/pe/latest/component_versions_in_recent_pe_releases.html
    $puppetdb_no_remote_metrics = ((versioncmp($facts['puppetversion'], '6.14.0') >= 0) or (versioncmp($facts['puppetversion'], '5.5.19') >= 0) and (versioncmp($facts['puppetversion'], '6.0.0') == -1))
    if ($profile == 'puppetdb') and $puppetdb_no_remote_metrics) {
      # Enabling this would result in a notice during every run, even if you apply the class.
      # notice ('With this version of PuppetDB, you must apply the puppet_metrics_dashboard::profile::puppetdb class to PuppetDB hosts: metrics cannot be remotely collected by Telegraf.')
      $hosts.filter |$host| { $host == $trusted['certname'] }  
    } else {
      sort($hosts)  
    }
  }
}

[suckatrash]

Right, I forgot about that use-case. Then we need to return ['localhost'] in that case since I think since the service only listens there in the versions we're accommodating.

[tkishel]

True. Maybe ...

$localhosts = $hosts.filter |$host| { $host == $trusted['certname'] }
$localhosts.map |$host| { '127.0.0.1' }

[tkishel]

Thank you for this, I know its an "unnecessary" assignment for a return value, but it makes it easier to read!

[suckatrash]

I'll squash these commits once all looks good here

[tkishel]

That's a lot of good work!

[genebean]

Code looks fine to me. One thing that stood out is it seems this chunk is used all over the place:

if ((versioncmp($facts['puppetversion'], '6.14.0') >= 0) or 
         (versioncmp($facts['puppetversion'], '5.5.19') >= 0) and (versioncmp($facts['puppetversion'], '6.0.0') < 1))

Maybe that should be pulled out into a function to simplify things.

[genebean]

Nice work everyone