Bug #1938


Foreman shouldn't use the FQDN fact to identify the node when facts are uploaded

Added by Nacho Barrientos over 11 years ago. Updated almost 11 years ago.

Target version:
Fixed in Releases:
Found in Releases:



When new facts are uploaded to Foreman, at some point models/host.rb:importHostAndFacts is executed. That function relies on certname and fqdn facts as keys to fetch the host from Foreman's database. This is dangerous because a malicious user with root privileges on a puppet managed machine could do as follows:

  • Modify puppet.conf to remove certname directive
  • Tweak facter to return a different value for the FQDN fact (target's machine for instance)

in order to change the facts of a different machine.


Chuck has root access on machine and wants to replace fact "uptime_seconds" on Foreman for machine (owned by Alice), so Chuck follows the procedure above (he fakes FQDN and uptime_seconds facts) and runs Puppet agent. Crafted facts are uploaded to the Puppet master:

puppetmaster /var/lib/puppet/yaml/facts # grep fqdn ibsltest*    fqdn:    fqdn: <-- this is not good for Foreman
puppetmaster /var/lib/puppet/yaml/facts  # grep uptime_seconds ibsltest*    uptime_seconds: "9965187"    uptime_seconds: "666" 

Before the catalog is compiled, the master executes Foreman's ENC to upload ibsltestw0's new set of facts to Foreman and also to get the node's classification data. There is no certname fact, so Foreman uses the FQDN fact (which value is as a key to get a host instance.

      when Puppet::Node::Facts
        certname = facts.values["certname"]
        name     = facts.values["fqdn"]
        values   = facts.values
      when Hash
        certname = facts["certname"]
        name     = facts["fqdn"]
        values   = facts
    if name == certname or certname.nil?
      h = Host.find_by_name name

so ibsltestm0's facts are replaced:

$ curl -s -k -L --cookie ssocookie-foreman.txt -H "Content-Type:application/json" -H "Accept:application/json" | grep -q '"uptime_seconds":"666"' && echo "facts injection proven" 
facts injection proven

Proposed fix:

The serialization of the object type Puppet::Node::Facts looks like this:

--- !ruby/object:Puppet::Node::Facts
  expiration: 2012-11-12 15:58:08.153312 +01:00
    [more facts here]
    [more facts here]

AFAIK, the top level key 'name' is set by the Puppet master based on the common name of the certificate associated with the node. That's data that can't be tampered agent-side and a good candidate to safely identify the node trying to replace the facts.

If an object with Hash type is received (second case) I can't think of any way to be sure what is the hostname of the node being modified. Maybe in that case we should delegate to Foreman's administrator via a config option the decision of allowing facts replacements based on the FQDN fact.

Related issues 1 (0 open1 closed)

Related to Foreman - Feature #1843: Accept a simple hash of facts to work with facter directlyClosedOhad Levy08/30/2012Actions
Actions #1

Updated by Dominic Cleal about 11 years ago

  • Category set to Facts
  • Target version set to 1.2.0
Actions #2

Updated by Mikael Fridh almost 11 years ago

Confirmed; on puppet 2.6.12 etc the following is true about the Puppet::Node::Facts object (Meaning, the following is true in the cases I observed):

  • 'certname' fact is never set (even if certname = in puppet.conf)
  • There is a 'clientcert' fact

But that's irrelevant anyway, should use the top-level key instead as suggested here.

I had to patch my Foreman 1.1 to do exactly this today since I have explicitly configured (clientcert<=>fqdn mismatching) certname = in most of my puppet agents.

Actions #3

Updated by Mikael Fridh almost 11 years ago

clientcert facts gets set from the certname setting in Puppet 2.6.x to 3.1.x. 0.24.x doesn't seem to have any such facts:

Why are we pulling the certname fact again?

Actions #4

Updated by Amos Benari almost 11 years ago

  • Status changed from New to Assigned
  • Assignee set to Amos Benari
Actions #5

Updated by Amos Benari almost 11 years ago

  • Status changed from Assigned to Closed
  • % Done changed from 0 to 100

Also available in: Atom PDF