Uploaded image for project: 'Puppet'
  1. Puppet
  2. PUP-1840

Let user change hashing algorithm, to avoid crashing on FIPS-compliant hosts

    Details

    • Type: New Feature
    • Status: Closed
    • Priority: Normal
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: PUP 3.6.0
    • Component/s: None
    • Labels:
    • Template:
    • 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

      Description

      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)

        Attachments

          Issue Links

            Activity

            Hide
            eric.sorenson Eric Sorenson added a comment -

            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.

            Show
            eric.sorenson Eric Sorenson added a comment - 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.
            Hide
            adrien Adrien Thebo added a comment -

            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.

            Show
            adrien Adrien Thebo added a comment - 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.
            Hide
            jyaworski Joseph Yaworski added a comment -

            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)

            Show
            jyaworski Joseph Yaworski added a comment - 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)
            Hide
            luke.schierer Luke Schierer added a comment -

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

            Show
            luke.schierer Luke Schierer added a comment - Ideally Puppet would simply handle this, not make me select it.
            Hide
            jared.jennings.ctr Jared Jennings added a comment -

            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.

            Show
            jared.jennings.ctr Jared Jennings added a comment - 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 .
            Hide
            jared.jennings.ctr Jared Jennings added a comment -

            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.

            Show
            jared.jennings.ctr Jared Jennings added a comment - 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.
            Hide
            jared.jennings.ctr Jared Jennings added a comment -

            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.

            Show
            jared.jennings.ctr Jared Jennings added a comment - Adrien Thebo , it's a splendid idea to bucket with multiple algorithms. I see two problems. 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. 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: I upgrade Puppet to the new version that contains all this code we're talking about, on all of my hosts. 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. 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. Now every bucketed file can be found using SHA-256. I contrive to set `file_checksum_algorithms = sha256` on all of my hosts. 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: I upgrade Puppet to the new version that contains all this code we're talking about, on all of my hosts. The bucket begins storing files using multiple algorithms, all on its own. On every host with a bucket, I run the re-bucketing tool. Now I can safely configure the hosts for FIPS 140-2 compliance. On compliant hosts, Puppet can't use the MD5 algorithm. It detects this at startup and stops using it. That's a lot better.
            Hide
            luke.schierer Luke Schierer added a comment -

            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.

            Show
            luke.schierer Luke Schierer added a comment - 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.
            Hide
            jared.jennings.ctr Jared Jennings added a comment -

            Joseph Yaworski Wait, what, the interpreter dumps core?! That should already have been fixed in ruby-1.8.7.352-3.el6, years ago. But I have ruby-1.8.7.352-13.el6, 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.

            Show
            jared.jennings.ctr Jared Jennings added a comment - Joseph Yaworski Wait, what, the interpreter dumps core?! That should already have been fixed in ruby-1.8.7.352-3.el6, years ago. But I have ruby-1.8.7.352-13.el6, 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.
            Hide
            jared.jennings.ctr Jared Jennings added a comment -

            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.

            Show
            jared.jennings.ctr Jared Jennings added a comment - 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 .
            Hide
            rob Rob Reynolds added a comment -

            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?

            Show
            rob Rob Reynolds added a comment - 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 ?
            Hide
            rob Rob Reynolds added a comment -
            Show
            rob Rob Reynolds added a comment - First PR (#2451) merged into master at https://github.com/puppetlabs/puppet/commit/d1f0a45903143f0e675d5124a88f371c70cd3bd8
            Hide
            rob Rob Reynolds added a comment - - edited

            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.

            Show
            rob Rob Reynolds added a comment - - edited 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.
            Hide
            kurt.wall Kurt Wall added a comment -

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

            #include <std_disclaimer>

            Show
            kurt.wall Kurt Wall added a comment - Assigning to Adrien Thebo and moving back to TODO per Rob's last update. #include <std_disclaimer>
            Hide
            adrien Adrien Thebo added a comment -

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

            Show
            adrien Adrien Thebo added a comment - Rob Reynolds the concerns with backwards compatibility were with respect to what would happen to filebucketed files when the digest algorithm switched, correct?
            Hide
            rob Rob Reynolds added a comment -

            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.

            Show
            rob Rob Reynolds added a comment - 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.
            Hide
            adrien Adrien Thebo added a comment -

            Rob Reynolds remembering things is haaaaard

            Show
            adrien Adrien Thebo added a comment - Rob Reynolds remembering things is haaaaard
            Hide
            adrien Adrien Thebo added a comment -

            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.

            Show
            adrien Adrien Thebo added a comment - 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.
            Hide
            andy Andrew Parker added a comment -

            Merged into master in 282837

            Show
            andy Andrew Parker added a comment - Merged into master in 282837
            Hide
            josh Josh Cooper added a comment -

            Merge into master cf61ec9

            Show
            josh Josh Cooper added a comment - Merge into master cf61ec9
            Hide
            jared.jennings.ctr Jared Jennings added a comment -

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

            Show
            jared.jennings.ctr Jared Jennings added a comment - About the digest parser function, I've replaced gh-2453 with gh-2603 https://github.com/puppetlabs/puppet/pull/2603 .
            Hide
            josh Josh Cooper added a comment -

            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
              '{sha256}84c3582c512959fc92ad3bde581f8ec58c34ebfc6f89fca4459cb670e90e2d9f'
            

            Verified that the puppet file application works:

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

            And the puppet filebucket application works also:

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

            Show
            josh Josh Cooper added a comment - 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 '{sha256}84c3582c512959fc92ad3bde581f8ec58c34ebfc6f89fca4459cb670e90e2d9f' Verified that the puppet file application works: $ bundle exec puppet file download {sha256}415e8d4611cefaffa4392d3cd282451b076c650fe7f56bb836d26b801b471be2 --digest_algorithm sha256 [main] server = puppetmaster.solar.lan And the puppet filebucket application works also: $ bundle exec puppet filebucket get 415e8d4611cefaffa4392d3cd282451b076c650fe7f56bb836d26b801b471be2 --digest_algorithm=sha256 [main] server = puppetmaster.solar.lan
            Hide
            josh Josh Cooper added a comment -

            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
            Show
            josh Josh Cooper added a comment - 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: pluginsync will download every file every time it runs. See PUP-2427 if you are using filebuckets, then the agent will fail to save into the filebucket, and the run will fail. See PUP-2423
            Hide
            nick.fagerlund Nicholas Fagerlund added a comment -

            Josh Cooper Noted! Thanks for the ping.

            Show
            nick.fagerlund Nicholas Fagerlund added a comment - Josh Cooper Noted! Thanks for the ping.
            Hide
            rob Rob Reynolds added a comment - - edited

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

            Show
            rob Rob Reynolds added a comment - - edited Moved the last part of this functionality to a new ticket PUP-2511 to track separately as it crossed release boundaries. Thanks!

              People

              • Assignee:
                Unassigned
                Reporter:
                redmine.exporter redmine.exporter
              • Votes:
                3 Vote for this issue
                Watchers:
                13 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved:

                  Zendesk Support