[PUP-1840] Let user change hashing algorithm, to avoid crashing on FIPS-compliant hosts Created: 2014/03/03  Updated: 2017/05/09  Resolved: 2014/05/01

Status: Closed
Project: Puppet
Component/s: None
Affects Version/s: None
Fix Version/s: PUP 3.6.0

Type: New Feature Priority: Normal
Reporter: redmine.exporter Assignee: Unassigned
Resolution: Fixed Votes: 3
Labels: redmine
Remaining Estimate: Not Specified
Time Spent: Not Specified
Original Estimate: Not Specified

Issue Links:
blocks ENTERPRISE-65 PE not currently functional on system... Closed
relates to PUP-2427 Pluginsync will download every file e... Closed
relates to PUP-4963 "puppet module build" fails on FIPS-e... Closed
relates to PUP-2511 Add parser function digest: uses dige... Closed
relates to PUP-2420 CRLs should be signed with SHA256 whe... Closed
relates to PUP-2423 Filebucket server should warn, not fa... Closed
Epic Link: FIPS-Enabled Puppet
Story Points: 3
Sprint: Week 2014-4-02 to 2014-4-09, Week 2014-4-09 to 2014-4-16, Week 2014-4-23 to 2014-4-30, Week 2014-4-16 to 2014-4-23, Week 2014-4-30 to 2014-5-7


I'm using Puppet in part to ensure [Federal Information Processing Standard 140-2](http://csrc.nist.gov/publications/fips/fips140-2/fips1402.pdf) (FIPS 140-2) compliance on my network. Part of this compliance for the system underlying Puppet is to make sure that only [FIPS Approved](http://csrc.nist.gov/publications/fips/fips140-2/fips1402annexa.pdf) algorithms are used. OpenSSL does this by ensuring that any attempts to run an unapproved algorithm result in either a SIGSEGV or a SIGABRT. MD5 has been broken enough that it is no longer a FIPS Approved algorithm.

The consequence for Puppet is that, if it tries to use MD5 on a FIPS-compliant system, it will crash. Here is where I have seen Puppet crash for this reason:

1. the puppet/util/checksums.rb, used by File resources;
2. the puppet/parser/functions/md5.rb, implementation of the md5 DSL function;
3. certificate signature in puppet/ssl/certificate_request.rb;
4. certificate fingerprinting in puppet/ssl/base.rb;
5. outside Puppet, in the session ID code in openssl/ssl-internal.rb, class OpenSSL::SSLServer, due to using WEBrick.

It was easy enough to replace MD5 with SHA256 in all those places - and, in case 4, it appears I may not have needed to change the code; but the DSL function is still called md5, and MD5 is still named in some of the messages. My changes lack the refinement necessary to be useful to others.

What I think I need is to be able to say, in one place like puppet.conf, "use SHA256, not MD5," and algorithms and messages alike will change. I think the `md5` DSL function would need to be replaced with a `digest` function which uses the configured algorithm, and there should also be a way in the DSL to find out which digest is being used, like a `digestname` function.

Then, in some years when SHA2 is decertified, I can tell Puppet, "use SHA3, not SHA2," instead of filing an issue like this one and doing code changes. (I don't know what migration issues this scenario may pose.)

[How can I make Red Hat Enterprise Linux 5 FIPS 140-2 compliant?](https://access.redhat.com/kb/docs/DOC-39230) (Red Hat login required)

Comment by Eric Sorenson [ 2014/03/03 ]

Andrew Parker Adrien Thebo there were concerns with the original pull request about doing this in a non-semver-major release – can you check back over the logic in redmine #8120 and see if the set of strategies we've developed for dealing with breaking changes still consider these breaking? There are a number of commercial support requests stacked up against this change so it'd be great to either (a) figure out how to incorporate the contributed code without incurring a major-version break, or (b) line it up for the 4.0 change and do it then.

Comment by Adrien Thebo [ 2014/03/03 ]

It would take a while to scope out the entire breadth of work that will be needed to make this possible. However I can identify a number of touch points where we will definitely have to change the existing behavior but can do so in a reasonably backwards compatible manner. The filebucket code is one of the areas that relies heavily on MD5 digests, and I think that we can extend the existing filebucket code to filebucket to multiple algorithms at once, which will address some of the compatibility issues.

I'll spend more time tomorrow getting a better estimate of the time needed for this.

Comment by Joseph Yaworski [ 2014/03/05 ]

I believe I'm running into this on a RHEL6 FIPS system as well. Whenever I run "puppet agent --test" I get:

md5_dgst.c(78): OpenSSL internal error, assertion failed: Digest MD5 forbidden in FIPS mode! Aborted (core dumped)

Comment by Luke Schierer [ 2014/03/07 ]

Ideally Puppet would simply handle this, not make me select it.

Comment by Jared Jennings [ 2014/03/19 ]

I've redone the pull request against the current master branch of puppet, and, taking into consideration Adrien Thebo's concerns in https://github.com/puppetlabs/puppet/pull/1035, have split it into three pull requests.

https://github.com/puppetlabs/puppet/pull/2451 adds the SHA-256 algorithm to Puppet::Util::Checksums. I think it is entirely uncontroversial.

https://github.com/puppetlabs/puppet/pull/2452 makes the digest algorithm used for file checksums switchable, and doesn't deal with migration concerns, thus enabling users to shoot themselves in the foot. The digest_algorithm setting defaults to md5, as before, which someone said was dumb in the Redmine issue.

https://github.com/puppetlabs/puppet/pull/2453 adds a digest function, by analogue with the md5 function presently available in the Puppet DSL. digest computes a checksum using the presently configured digest_algorithm.

Comment by Jared Jennings [ 2014/03/19 ]

I asked a while ago in the Redmine issue what the configuration setting should be called, and no one gave a passionate answer; I ended up calling it `digest_algorithm`. But I've seen the string `digest_algorithm` pop up in the SSL parts of the Puppet code. It needs to be clear that what we are configuring here is not the SSL bits, and the file resource and file bucket parts of Puppet call this thing a "checksum" not a "digest," so now, if no one has an idea loudly, I think it should be called `file_checksum_algorithm`. I have not made this change in the code, though.

Comment by Jared Jennings [ 2014/03/19 ]

Adrien Thebo, it's a splendid idea to bucket with multiple algorithms. I see two problems.

  1. Whoever is concerned about migration will have some files already bucketed. Some tool would need to be made that could go through the whole file bucket directory and re-bucket all the files with a different algorithm.
  2. When bucketing files with multiple algorithms, which ones should be used? In the FIPS 140-2 regime, I always want to move from MD5 to SHA-256; but when I enter super-awesome-secret-mode I may want to move from SHA-256 to Skein.

So let us assume a new `file_checksum_algorithms` (plural) setting, which names all the algorithms in play, with the one I'm trying to move toward listed first. The bucket is changed to store files using all algorithms named. When it is searching for a file, it looks using each algorithm in `file_checksum_algorithms`*, in order. Let us also assume the rebucketing tool exists.

Given those things, if I want to migrate to a new file checksum algorithm, I do these things:

  1. I upgrade Puppet to the new version that contains all this code we're talking about, on all of my hosts.
  2. I contrive to set `file_checksum_algorithms = sha256, md5` in the puppet.conf on all of my hosts. The bucket begins storing files using both algorithms. ^*^Most files sought by the bucket at this stage are not found using SHA-256, but are found using MD5.
  3. On every host with a bucket, I run the re-bucketing tool. As it runs, files begin being found using SHA-256; files not yet re-bucketed are still found using MD5.
  4. Now every bucketed file can be found using SHA-256.
  5. I contrive to set `file_checksum_algorithms = sha256` on all of my hosts.
  6. Now I can safely configure the hosts for FIPS 140-2 compliance.

Now, Luke Schierer, it would be great for Puppet to "simply handle this." Perhaps we could hard-code the equivalent of `file_checksum_algorithms = sha256, md5` in the scenario above, then try an MD5 checksum at startup time, and if it fails, don't try to do it thereafter. Then when storing or retrieving a bucketed file, we use all algorithms available, coolest first. This would make the bucket slower on non-compliant systems, because multiple checksums are being calculated; and the addition of the fallback may add some obscure security loophole. You would still have to manually re-bucket files that were bucketed before you upgraded to the new Puppet version, ahead of your FIPS compliance migration. And if Puppet takes on this convention rather than leaving it up to admins as configuration, Puppet's code will need to change when SHA-256 falls out of favor as MD5 did. Migration between digest algorithms would have to become a part of Puppet's release management scheme: when will Puppet add SHA-3? when will it drop MD5?

But then my migration scenario might go like so:

  1. I upgrade Puppet to the new version that contains all this code we're talking about, on all of my hosts.
  2. The bucket begins storing files using multiple algorithms, all on its own.
  3. On every host with a bucket, I run the re-bucketing tool.
  4. Now I can safely configure the hosts for FIPS 140-2 compliance.
  5. On compliant hosts, Puppet can't use the MD5 algorithm. It detects this at startup and stops using it.

That's a lot better.

Comment by Luke Schierer [ 2014/03/19 ]

Your second scenario looks like a reasonable implementation of my request that it simply handle things. I would then work on an exec in my puppet module to run the re-bucketing tool if MD5 directories still exist, and make the FIPS-enabling module dependent on that exec. At that point my migration would handle itself.

Comment by Jared Jennings [ 2014/03/20 ]

Joseph Yaworski Wait, what, the interpreter dumps core?! That should already have been fixed in ruby-, years ago. But I have ruby-, and sure enough, it dumps core. That has to be fixed (again?) for Puppet to be able to detect at startup that it can't use MD5.

Comment by Jared Jennings [ 2014/03/20 ]

Joseph Yaworski, oho, the crash that was fixed in 2011 was when you do

ruby -ropenssl -e "puts OpenSSL::Digest::MD5.new('hi').hexdigest"

The crash I see now is when I do

ruby -rdigest -e "puts Digest::MD5.new('hi').hexdigest"

Filed vendor bug https://bugzilla.redhat.com/show_bug.cgi?id=1079042 and upstream bug https://bugs.ruby-lang.org/issues/9659.

Comment by Rob Reynolds [ 2014/03/26 ]

From the triage: The first PR (#2451) would make it into 3.6.0. Once we have that in we will need to re-evaluate if the rest is going to be breaking and thus 4.0.0. Does this sound correct Andrew Parker, Adrien Thebo?

Comment by Rob Reynolds [ 2014/03/26 ]

First PR (#2451) merged into master at https://github.com/puppetlabs/puppet/commit/d1f0a45903143f0e675d5124a88f371c70cd3bd8

Comment by Rob Reynolds [ 2014/03/26 ]

Once this has been validated against CI, this issue should be moved back to TODO and assigned to Adrien Thebo for parts 2 & 3.

Don't shoot me or tell me I'm doing JIRA wrong, I'm just the messenger.

Comment by Kurt Wall [ 2014/03/31 ]

Assigning to Adrien Thebo and moving back to TODO per Rob's last update.

#include <std_disclaimer>

Comment by Adrien Thebo [ 2014/04/03 ]

Rob Reynolds the concerns with backwards compatibility were with respect to what would happen to filebucketed files when the digest algorithm switched, correct?

Comment by Rob Reynolds [ 2014/04/03 ]

Adrien Thebo haha, I don't remember. It was discussed in the triage and I think you were the one that mentioned it might break compatibility and thus be subject to 4.0.

Comment by Adrien Thebo [ 2014/04/03 ]

Rob Reynolds remembering things is haaaaard

Comment by Adrien Thebo [ 2014/04/16 ]

I've rebased gh-2452 in https://github.com/puppetlabs/puppet/pull/2537 and closed the original pull request. This issue should be moved back into To Do after that pull request is merged and reviewed so that we can work on the final pull request in this series.

Comment by Andrew Parker [ 2014/04/29 ]

Merged into master in 282837

Comment by Josh Cooper [ 2014/04/30 ]

Merge into master cf61ec9

Comment by Jared Jennings [ 2014/05/01 ]

About the digest parser function, I've replaced gh-2453 with gh-2603 https://github.com/puppetlabs/puppet/pull/2603.

Comment by Josh Cooper [ 2014/05/01 ]

Verified MD5 is not used for any certificate signing operation. CSR and certs signatures default to SHA256WithRSAEncryption, fall back to SHA1WithRSAEncryption, otherwise fail. CRLs default to SHA1, see PUP-2420.

This change requires that the digest_algorithm=sha256 setting be applied globally. Filed PUP-2423. Once agent and master settings are modified, the agent can successfully backup files:

In site.pp, create a filebucket resource, the path => false part is important:

filebucket { 'main':
  path => false,
File { backup => 'main' }

Notice: /Stage[main]/Main/File[/Users/josh/.puppet/puppet.conf]/content: content changed
  '{sha256}415e8d4611cefaffa4392d3cd282451b076c650fe7f56bb836d26b801b471be2' to

Verified that the puppet file application works:

$ bundle exec puppet file download {sha256}415e8d4611cefaffa4392d3cd282451b076c650fe7f56bb836d26b801b471be2 --digest_algorithm sha256
server = puppetmaster.solar.lan

And the puppet filebucket application works also:

$ bundle exec puppet filebucket get 415e8d4611cefaffa4392d3cd282451b076c650fe7f56bb836d26b801b471be2 --digest_algorithm=sha256
server = puppetmaster.solar.lan

Comment by Josh Cooper [ 2014/05/01 ]

Nicholas Fagerlund Please document two known issues for the 3.6.0 release. If the digest_algorithm setting is not consistently set across all nodes, e.g. sha256 on the master, but default md5 on the agent, then:

  1. pluginsync will download every file every time it runs. See PUP-2427
  2. if you are using filebuckets, then the agent will fail to save into the filebucket, and the run will fail. See PUP-2423
Comment by Nicholas Fagerlund [ 2014/05/02 ]

Josh Cooper Noted! Thanks for the ping.

Comment by Rob Reynolds [ 2014/05/07 ]

Moved the last part of this functionality to a new ticket PUP-2511 to track separately as it crossed release boundaries. Thanks!

Generated at Tue Jan 22 08:17:53 PST 2019 using JIRA 7.7.1#77002-sha1:e75ca93d5574d9409c0630b81c894d9065296414.